Add budget tracking and enforcement for sandbox sessions (v1.1) - #52
Merged
MacroscopeApp / Macroscope - Correctness Check
succeeded
Jan 30, 2026 in 4m 13s
No issues identified (54 code objects reviewed).
• Merge Base:
5270a6b
• Head:c3a8ddf
Details
| ✅ | File Path | Comments Posted |
|---|---|---|
| ✅ | CLAUDE.md |
0 |
| ✅ | src/cli.ts |
0 |
| ✅ | src/config.ts |
0 |
| ✅ | src/db.ts |
0 |
| ✅ | src/e2b/claude-runner.ts |
0 |
| ✅ | src/e2b/sandbox-manager.ts |
0 |
| ✅ | src/types.ts |
0 |
Filtered Issues Details
src/cli.ts
- line 1931: The budget enforcement logic is implemented in
SandboxManager.checkBudgetLimitbut is never invoked, rendering the--budgetfeature non-functional. Incli.ts, the--budgetoption is parsed and passed tosandboxManager.setBudgetLimit, but the execution flow subsequently waits onexecuteClaudeInSandbox. Neithercli.tsnorclaude-runner.ts(which runssandbox.commands.run) establishes a monitoring loop or interval to periodically callcheckBudgetLimit. As a result, theBudgetExceededErrorlogic inSandboxManager.tsis unreachable code, and sandboxes will continue running and accruing costs beyond the specified limit until the hard timeout is reached. [ Out of scope ] - line 3048: The logic for identifying budget-related keys relies on a loose substring check
normalizedKey.includes('budget'). This implicitly traps any configuration key containing the word "budget" (e.g.,budget_tracker.url,my_budget.enabled) into the numeric validation block. Consequently, users cannot set string or boolean values for these keys; the CLI will incorrectly attempt to parse them as numbers and throw anInvalid numbererror. [ Already posted ] - line 3189: The
budgetStatusActionfunction callsdb.migrateToLatest(), but theSessionDBclass definition insrc/db.tsdoes not define this method (it definesrunMigration(version)instead). This will result in aTypeError: db.migrateToLatest is not a functionat runtime. [ Already posted ] - line 3207: The
budgetStatusActionfunction callstracker.generateBudgetStatus(period), which internally callsthis.db.getOrCreateBudgetTrackingRecord(insrc/budget-tracker.ts). However, theSessionDBclass insrc/db.tsonly implementscreateBudgetTrackingRecordandgetBudgetTrackingRecord, not the combinedgetOrCreate...method. This mismatch will cause aTypeErrorat runtime. [ Already posted ] - line 3262: The
budgetStatusActionfunction callsdb.close()in thefinallyblock. TheSessionDBclass insrc/db.ts(which wrapsbetter-sqlite3) does not expose a publicclose()method in the provided references. Unless inherited from a superclass not shown, this will cause aTypeErroron every execution cleanup. [ Already posted ]
src/config.ts
- line 68: The
ConfigManagerconstructor implicitly schedules an uncatchable asynchronous file write when initializing a missing configuration file. Ifload()(called in the constructor) detects a missing file, it callssave(), which schedulesflushSync()viasetTimeout. IfflushSync()fails (e.g., due to a read-only filesystem or permission errors on the directory), it explicitly re-throws the error. Since this error occurs inside a timer callback outside the constructor's execution stack, it cannot be caught by wrappingnew ConfigManager()in atry/catchblock, resulting in an unhandled exception that crashes the process. [ Already posted ] - line 141: The debounced save mechanism causes the application to crash on file system errors (e.g., read-only file system, disk full, permissions) or serialization errors. The
savemethod schedulesflushSyncusingsetTimeout. InsideflushSync, errors fromwriteFileSyncorJSON.stringify(e.g., if a circular reference was introduced viaset) are caught, but any error other thanENOENTis re-thrown. Because this re-throw happens inside asetTimeoutcallback, it results in an unhandled exception that terminates the Node.js process, transforming a recoverable I/O error into a denial of service. [ Already posted ] - line 175: The
flushSyncmethod creates a risk of permanent configuration data loss due to non-atomic file writes. Line 175 callswriteFileSyncdirectly onthis.configPath. If the process crashes or loses power during this write operation, the configuration file will be corrupted (partially written or empty). Upon restart, theloadmethod (line 101) will fail to parse the corrupted JSON. The catch block (lines 114-123) then backs up the 'corrupted' file and returns default values. This recovery mechanism combined with the non-atomic write means a single write failure results in the permanent loss of the user's valid configuration settings. [ Already posted ] - line 179: The
flushSyncmethod contains a high-severity runtime crash vector when handling non-serializable data. Thesetmethod allows insertion of any value (typed asunknown) intothis.config, including objects with circular references or BigInts. Thesavemethod schedules an asynchronous write viasetTimeout. When the timer fires,flushSyncattempts to serialize the configuration usingJSON.stringify(line 175). Ifthis.configcontains a circular reference,JSON.stringifythrows aTypeError. This error is caught but explicitly re-thrown (line 179) because it is not an 'ENOENT' error. Since this occurs inside asetTimeoutcallback, the thrown error becomes an uncaught exception in the Node.js event loop, causing the entire process to crash. This allows a local denial-of-service via configuration updates. [ Already posted ] - line 286: The
deletemethod permits the removal of the rootbudgetconfiguration key, which violates the internalConfiginterface and corrupts the object state. ThevalidateKeyPartmethod only checks for reserved words and format, not for required schema keys. If a caller invokesdelete('budget'),this.config.budgetbecomesundefined. Subsequently, callinggetBudgetConfig()executesstructuredClone(this.config.budget). SincestructuredClonethrows aDataCloneErroronundefined(in many environments) or returnsundefined(violating theBudgetConfigreturn type contract), this leads to a runtime crash orTypeErrorwhen accessing properties on the result. [ Already posted ]
src/e2b/sandbox-manager.ts
- line 432: In
checkBudgetLimit, the budget warning thresholds array from the configuration (this.config.budgetWarningThresholds) is sorted in-place using.sort((a, b) => b - a). Since arrays are passed by reference in JavaScript/TypeScript andthis.config.budgetWarningThresholdsis a reference to the array in the persistent configuration object (which might be the sharedDEFAULT_CONFIGor a user-provided array), this operation mutates the shared configuration state. This means the order of thresholds is permanently changed for subsequent calls or other sandboxes sharing the same config object. While sorting descending is intended for the logic inside this function, mutating a shared config object is a dangerous side effect that can lead to unexpected behavior if other parts of the system rely on the original order. [ Already posted ] - line 440: In
checkBudgetLimit, the code callsthis.config.budgetWarningThresholds.sort(...). Thesortmethod mutates the array in-place. Becausethis.config.budgetWarningThresholdsrefers to the array fromDEFAULT_CONFIG(if not overridden) or a shared configuration object passed into the constructor, this mutation affects all instances ofSandboxManageror any other code sharing that configuration object. Over time, or with concurrent usage patterns, unexpected re-ordering of a shared configuration array can lead to race conditions or confusing side-effects in other parts of the application relying on a specific order (though here it sorts descending every time, repeated sorting of a shared mutable reference is bad practice and can lead to issues if the config is frozen or used elsewhere). [ Already posted ]
Loading