feat: cross-machine usage aggregation + automatic sync #55
Conversation
…e aggregation - Add DeviceSourceData interface for per-device contribution tracking - Add devices field to SourceBreakdownData type in helpers.ts and schema.ts - Modify mergeSourceBreakdowns() to accept deviceId parameter - Add recalculateSourceAggregate() helper to sum across devices - Migrate existing data without devices field to __legacy__ device - Update submit route to pass tokenRecord.tokenId as deviceId - Add tests for same-device replacement, cross-device aggregation, and legacy migration
- Add sync.ts with setupSync(), removeSync(), syncStatus() functions - Support crontab (macOS/Linux) and Task Scheduler (Windows) - Use process.argv[1] for CLI path resolution - Crontab entry uses --quiet flag for silent execution - Log output to ~/.config/tokscale/sync.log
…ron cleanup - Store devices[deviceId] on new day inserts to prevent double-counting when same device resubmits (was migrating to __legacy__ and adding) - Fix shell injection risk in crontab setup using printf with escaped quotes - Fix Windows Task Scheduler logging by wrapping in cmd.exe for redirections - Use specific grep pattern 'tokscale submit --quiet' to avoid removing unrelated cron jobs - Add tests for insert→resubmit flow to verify no double-counting
… direct tests
- Add ?? {} defensive defaults for models access in mergeSourceBreakdowns()
- Use TOKSCALE_SYNC_MANAGED marker for cron entries (prevents false matches)
- Windows sync now uses .cmd script file approach (simpler, no quoting issues)
- Add direct mergeSourceBreakdowns() function call tests for legacy data handling
…e, Windows cmd wrapper
…pe coercion - Legacy migration now uses '__legacy__' device instead of current deviceId - This preserves historical data separately from new device contributions - Added Number(...) || 0 for all arithmetic in recalculateSourceAggregate - Handles potential string values from JSON serialization - Updated tests to expect __legacy__ entry for legacy data migrations
- Reset aggregates before empty devices check to prevent stale values - Use Number() coercion in recalculateDayTotals for consistency
Convert all || 0 patterns to Number(...) || 0 to handle cases where values may be strings from JSON parsing or database retrieval.
|
@Yeachan-Heo is attempting to deploy a commit to the Inevitable Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
junhoyeo
force-pushed
the
feat/cross-machine-aggregation-sync
branch
from
f906e18 to
6591b0e
Compare
|
Hey @Yeachan-Heo, thanks for opening a PR tackling two tasks. Appreciate the contribution despite the complexity! 🙌 One heads-up: the commits and PR body had the |
- Windows sync: Mark as experimental/disabled pending security review - bunx detection: Prevent sync setup from temp cache paths with clear error message - Crontab security: Add path validation to prevent injection via control chars - Legacy migration: Fix modelId→models conversion to preserve model breakdown Closes issues identified in PR junhoyeo#55 review.
🔧 Additional Fixes AppliedAddressed the security and stability issues identified in the review. Here's what was fixed: 1. Windows Sync → Experimental (Disabled)
2. bunx Temp Path Detection
3. Crontab Security
4. Legacy
|
| File | Changes |
|---|---|
packages/cli/src/sync.ts |
+148 lines (bunx detection, path validation, Windows experimental) |
packages/frontend/src/lib/db/helpers.ts |
+23 lines (modelId migration fix) |
packages/frontend/__tests__/api/submit.test.ts |
+53 lines (migration tests) |
Commit
5f932fc fix: address security and stability issues in cross-machine sync
All changes are surgical and independently verifiable. Ready for re-review! 🚀
|
@Yeachan-Heo fyi, I will hold this pr from merging before #64 -- there will be significant changes there! |
As of today, no users have the deprecated |
Resolved conflicts: - README.md: Keep main's structure (Overview before Contents), add Automatic Sync to TOC - cli.ts: Combine knownCommands to include both 'sync' and 'pricing' - submit.ts: Keep quiet flag feature with main's simplified structure
The submit command already has minimal necessary logs, and output is redirected to log file for cron jobs anyway. --no-spinner exists for scripts that need clean stdout.
|
@cubic-dev-ai review this pull request |
@junhoyeo I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/src/sync.ts">
<violation number="1" location="packages/cli/src/sync.ts:26">
P2: Variable `path` shadows the imported `path` module from `node:path`. This creates confusion and potential runtime errors if the function is later modified to use `path.join()` or similar methods. Rename to `cliPath` or `execPath`.</violation>
</file>
<file name="packages/cli/src/cli.ts">
<violation number="1" location="packages/cli/src/cli.ts:461">
P2: The new `--interval` flag is accepted but ignored: `setupSync` does not use the interval and the cron entry is hard-coded to hourly. Either implement interval handling or remove the flag to avoid misleading users.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| function getTokscalePath(): { path: string; isTempPath: boolean } { | ||
| const path = process.argv[1]; |
There was a problem hiding this comment.
P2: Variable path shadows the imported path module from node:path. This creates confusion and potential runtime errors if the function is later modified to use path.join() or similar methods. Rename to cliPath or execPath.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/sync.ts, line 26:
<comment>Variable `path` shadows the imported `path` module from `node:path`. This creates confusion and potential runtime errors if the function is later modified to use `path.join()` or similar methods. Rename to `cliPath` or `execPath`.</comment>
<file context>
@@ -0,0 +1,379 @@
+}
+
+function getTokscalePath(): { path: string; isTempPath: boolean } {
+ const path = process.argv[1];
+ if (!path) {
+ return { path: '', isTempPath: false };
</file context>
There was a problem hiding this comment.
I've fixed the variable shadowing issue in packages/cli/src/sync.ts. The local variable path at line 26 was renamed to cliPath to avoid shadowing the imported path module from node:path.
Changes made:
- Renamed
const path = process.argv[1]toconst cliPath = process.argv[1] - Updated all references within the function to use
cliPath - The return object still uses
pathas the property name to maintain the existing API
This prevents confusion and potential runtime errors if the function is later modified to use path.join() or similar methods from the node:path module.
diff --git a/packages/cli/src/sync.ts b/packages/cli/src/sync.ts
index cf2f967..f96abf2 100644
--- a/packages/cli/src/sync.ts
+++ b/packages/cli/src/sync.ts
@@ -23,11 +23,11 @@ function ensureConfigDir(): void {
}
function getTokscalePath(): { path: string; isTempPath: boolean } {
- const path = process.argv[1];
- if (!path) {
+ const cliPath = process.argv[1];
+ if (!cliPath) {
return { path: '', isTempPath: false };
}
- return { path, isTempPath: isBunxTempPath(path) };
+ return { path: cliPath, isTempPath: isBunxTempPath(cliPath) };
}
// Escape single quotes for shell: replace ' with '\''
Files changed:
packages/cli/src/sync.ts— Renamed local variablepathtocliPathin getTokscalePath() to avoid shadowing the imported path module
| syncCommand | ||
| .command("setup") | ||
| .description("Set up hourly automatic submission") | ||
| .option("--interval <minutes>", "Sync interval in minutes (default: 60)", "60") |
There was a problem hiding this comment.
P2: The new --interval flag is accepted but ignored: setupSync does not use the interval and the cron entry is hard-coded to hourly. Either implement interval handling or remove the flag to avoid misleading users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli.ts, line 461:
<comment>The new `--interval` flag is accepted but ignored: `setupSync` does not use the interval and the cron entry is hard-coded to hourly. Either implement interval handling or remove the flag to avoid misleading users.</comment>
<file context>
@@ -446,13 +447,43 @@ async function main() {
+ syncCommand
+ .command("setup")
+ .description("Set up hourly automatic submission")
+ .option("--interval <minutes>", "Sync interval in minutes (default: 60)", "60")
+ .action(async (options) => {
+ await setupSync({ interval: options.interval });
</file context>
There was a problem hiding this comment.
I've fixed the issue by removing the misleading --interval flag from the sync setup command.
The problem: The --interval flag was accepted by the CLI but completely ignored. The cron entry was hard-coded to 0 * * * * (hourly at minute 0), and the setupSync() function parameter was prefixed with underscore (_options) indicating it was unused.
The fix: Removed the flag entirely rather than implementing interval handling, because:
- The command description already says "Set up hourly automatic submission"
- Cron doesn't directly support arbitrary minute intervals (e.g., "every 45 minutes")
- Removing the flag avoids misleading users who expect it to work
Changes made:
packages/cli/src/cli.ts: Removed--intervaloption and simplified the action handlerpackages/cli/src/sync.ts: RemovedSyncSetupOptionsinterface and simplifiedsetupSync()signature
diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts
index 88e9083..6f1e73c 100644
--- a/packages/cli/src/cli.ts
+++ b/packages/cli/src/cli.ts
@@ -458,9 +458,8 @@ async function main() {
syncCommand
.command("setup")
.description("Set up hourly automatic submission")
- .option("--interval <minutes>", "Sync interval in minutes (default: 60)", "60")
- .action(async (options) => {
- await setupSync({ interval: options.interval });
+ .action(async () => {
+ await setupSync();
});
syncCommand
diff --git a/packages/cli/src/sync.ts b/packages/cli/src/sync.ts
index cf2f967..1d134bd 100644
--- a/packages/cli/src/sync.ts
+++ b/packages/cli/src/sync.ts
@@ -237,11 +237,7 @@ function checkWindowsTask(): { exists: boolean; error?: string } {
// Public API
// =============================================================================
-export interface SyncSetupOptions {
- interval?: string;
-}
-
-export async function setupSync(_options: SyncSetupOptions = {}): Promise<void> {
+export async function setupSync(): Promise<void> {
const credentials = loadCredentials();
if (!credentials) {
console.log(pc.red("\n Error: Not logged in to Tokscale."));
Files changed:
-
packages/cli/src/cli.ts— Removed --interval option from sync setup command -
packages/cli/src/sync.ts— Removed SyncSetupOptions interface and simplified setupSync() function signature
2 participants
Summary
tokscale synccommands for hourly crontab automationProblem Solved
New Commands
Changes
devicesfield--quietflag totokscale submitfor cron compatibility~/.config/tokscale/sync.logdevices.__legacy__Testing