Persist orchestration and payment events for audit history (#26)#73
Persist orchestration and payment events for audit history (#26)#73Kirtiman-55 wants to merge 1 commit into
Conversation
|
Hey @Kirtiman-55! 👋 It looks like this PR isn't linked to any issue. If this PR is for one of the issues assigned to you as part of a Wave, please link it to ensure your contribution is tracked properly. You can do this by adding a keyword to the PR description (e.g.,
|
|
@Kirtiman-55 is attempting to deploy a commit to the flamki's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces configurable run history and audit logging for orchestration and payment transactions. It adds environment variables and configuration for storage mode, persistent file-backed or in-memory storage of run events and transaction proofs, server-side initialization and orchestrate endpoint integration, and a new ChangesRun History Audit Logging
Sequence DiagramsequenceDiagram
participant client as Client
participant server as /api/orchestrate
participant store as runHistoryStore
participant status as /api/status
client->>server: POST/GET orchestrate(task, budget)
server->>store: createRun({ task, budget, source })
server->>server: broadcast events with runId
server->>store: appendEvent(runId, event)
server->>server: orchestrate logic
server->>store: completeRun(runId, result)
server->>client: response + runId
client->>status: GET /api/status
status->>store: (reads config)
status->>client: status + runHistory config
client->>server: GET /api/runs?limit=20
server->>store: listRecent(limit)
server->>client: recent runs list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/server.js (1)
45-45: ⚡ Quick winRun-history init failure is fatal to startup.
This top-level
awaitrejects if the store fails to initialize (e.g., the data directory isn't writable), preventing the server from booting. That's inconsistent with the non-fatal handling of x402 init below (warn-and-continue). Since run history is an audit/observability feature, consider degrading gracefully (warn + fall back to in-memory) rather than blocking the whole server.Confirm whether a boot-time failure of the audit store should be fatal for this service, or whether availability of the core API should be preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.js` at line 45, The top-level await of createRunHistoryStore can abort startup if it rejects; wrap the call to createRunHistoryStore so initialization failures are caught, log a warning via the same logger used elsewhere, and fall back to an in-memory/no-op run-history implementation instead of letting the process exit; update the runHistoryStore binding (the variable assigned from createRunHistoryStore) to point to the fallback when createRunHistoryStore throws, and ensure callers that expect runHistoryStore (e.g., any functions referencing runHistoryStore) still work with the in-memory implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.js`:
- Line 44: The config value runHistoryStorage should be normalized to a
consistent case at source so consumers match; change the exported
runHistoryStorage assignment to lowercased value (e.g., set runHistoryStorage to
(process.env.RUN_HISTORY_STORAGE || 'file').toLowerCase()) so
createRunHistoryStore and the comparisons in src/server.js (where
config.runHistoryStorage === 'file' is used) see the same lowercase token;
update the runHistoryStorage export only (leave createRunHistoryStore intact) to
ensure endpoints return the correct store instance.
In `@src/storage/run-history.js`:
- Around line 21-26: The object assignment for cost and totalSpent in
run-history.js incorrectly uses the logical OR fallback (event.cost || null,
event.totalSpent || null) which converts legitimate 0 values to null; update
these assignments to use the nullish coalescing operator (event.cost ?? null and
event.totalSpent ?? null) so only undefined or null become null while preserving
numeric 0, leaving other fields (paidVia, txHash, explorerUrl, status)
unchanged.
- Around line 123-128: The catch in the load routine is overwriting a
possibly-corrupt JSON file by calling this.persist() on any non-ENOENT error;
change the handler to first move/backup the bad file (e.g., rename original
filename to filename + `.corrupt.<timestamp>` or `.bak`) before calling
this.persist(), log the backup path and original error, and only then persist
the in-memory this.runs; update the catch in run-history.js that references
this.persist and this.runs to perform the backup/rename with a timestamp and
handle rename errors gracefully (log and abort persist if rename fails).
- Around line 131-136: The persist() method in run-history.js can race because
multiple callers (appendEvent, completeRun, failRun) write to the same temp
file; fix by making persist() use a unique temp filename (e.g., include a
timestamp/UUID or process+pid+counter) instead of `${this.filePath}.tmp` and
serialize concurrent persists with an internal write queue or mutex on the
RunHistory instance (add a simple Promise-based queue or lock property used by
persist()). Update persist() to await acquiring the lock, perform writeFile to
the unique temp name and rename, then release the lock; ensure appendEvent,
completeRun, and failRun continue to call persist() but will now be ordered and
safe.
---
Nitpick comments:
In `@src/server.js`:
- Line 45: The top-level await of createRunHistoryStore can abort startup if it
rejects; wrap the call to createRunHistoryStore so initialization failures are
caught, log a warning via the same logger used elsewhere, and fall back to an
in-memory/no-op run-history implementation instead of letting the process exit;
update the runHistoryStore binding (the variable assigned from
createRunHistoryStore) to point to the fallback when createRunHistoryStore
throws, and ensure callers that expect runHistoryStore (e.g., any functions
referencing runHistoryStore) still work with the in-memory implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 717cec9f-e4b9-4d14-bbd0-316b8ef588d5
📒 Files selected for processing (5)
.env.exampleREADME.mdsrc/config.jssrc/server.jssrc/storage/run-history.js
| anthropicRetryBaseDelayMs: Math.max(100, toNumberOr(process.env.ANTHROPIC_RETRY_BASE_DELAY_MS, 500)), | ||
|
|
||
| // Run history persistence | ||
| runHistoryStorage: process.env.RUN_HISTORY_STORAGE || 'file', |
There was a problem hiding this comment.
Normalize runHistoryStorage casing to match the factory.
createRunHistoryStore lowercases this value ((config.runHistoryStorage || 'file').toLowerCase()), but src/server.js compares it case-sensitively (config.runHistoryStorage === 'file') in both /api/status and /api/runs. With RUN_HISTORY_STORAGE=FILE, the file store is still created, but those endpoints report file: null. Normalize once here so all consumers agree.
🔧 Proposed fix
- runHistoryStorage: process.env.RUN_HISTORY_STORAGE || 'file',
+ runHistoryStorage: (process.env.RUN_HISTORY_STORAGE || 'file').toLowerCase(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.js` at line 44, The config value runHistoryStorage should be
normalized to a consistent case at source so consumers match; change the
exported runHistoryStorage assignment to lowercased value (e.g., set
runHistoryStorage to (process.env.RUN_HISTORY_STORAGE || 'file').toLowerCase())
so createRunHistoryStore and the comparisons in src/server.js (where
config.runHistoryStorage === 'file' is used) see the same lowercase token;
update the runHistoryStorage export only (leave createRunHistoryStore intact) to
ensure endpoints return the correct store instance.
| cost: event.cost || null, | ||
| paidVia: event.paidVia || null, | ||
| txHash: event.txHash || null, | ||
| explorerUrl: event.explorerUrl || null, | ||
| status: event.status || null, | ||
| totalSpent: event.totalSpent || null, |
There was a problem hiding this comment.
|| null drops legitimate numeric 0 for cost/totalSpent.
A genuine 0 cost or 0 total-spent (e.g., a skipped/free step) is coerced to null, misrepresenting the audit trail. Use nullish coalescing.
🔧 Proposed fix
- cost: event.cost || null,
+ cost: event.cost ?? null,
paidVia: event.paidVia || null,
txHash: event.txHash || null,
explorerUrl: event.explorerUrl || null,
status: event.status || null,
- totalSpent: event.totalSpent || null,
+ totalSpent: event.totalSpent ?? null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cost: event.cost || null, | |
| paidVia: event.paidVia || null, | |
| txHash: event.txHash || null, | |
| explorerUrl: event.explorerUrl || null, | |
| status: event.status || null, | |
| totalSpent: event.totalSpent || null, | |
| cost: event.cost ?? null, | |
| paidVia: event.paidVia || null, | |
| txHash: event.txHash || null, | |
| explorerUrl: event.explorerUrl || null, | |
| status: event.status || null, | |
| totalSpent: event.totalSpent ?? null, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/run-history.js` around lines 21 - 26, The object assignment for
cost and totalSpent in run-history.js incorrectly uses the logical OR fallback
(event.cost || null, event.totalSpent || null) which converts legitimate 0
values to null; update these assignments to use the nullish coalescing operator
(event.cost ?? null and event.totalSpent ?? null) so only undefined or null
become null while preserving numeric 0, leaving other fields (paidVia, txHash,
explorerUrl, status) unchanged.
| } catch (err) { | ||
| if (err.code !== 'ENOENT') { | ||
| console.warn(` run history load warning: ${err.message}`); | ||
| } | ||
| await this.persist(); | ||
| } |
There was a problem hiding this comment.
Parse failure silently overwrites existing history.
On any non-ENOENT error (e.g., a partially-written/corrupt JSON file), the catch logs a warning and immediately persist()s, overwriting the file with the current (empty) in-memory runs. That discards potentially recoverable audit data. Consider backing up the bad file before overwriting.
🛡️ Suggested approach
} catch (err) {
if (err.code !== 'ENOENT') {
console.warn(` run history load warning: ${err.message}`);
+ await fs.rename(this.filePath, `${this.filePath}.corrupt-${Date.now()}`).catch(() => {});
}
await this.persist();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/run-history.js` around lines 123 - 128, The catch in the load
routine is overwriting a possibly-corrupt JSON file by calling this.persist() on
any non-ENOENT error; change the handler to first move/backup the bad file
(e.g., rename original filename to filename + `.corrupt.<timestamp>` or `.bak`)
before calling this.persist(), log the backup path and original error, and only
then persist the in-memory this.runs; update the catch in run-history.js that
references this.persist and this.runs to perform the backup/rename with a
timestamp and handle rename errors gracefully (log and abort persist if rename
fails).
| async persist() { | ||
| const payload = JSON.stringify({ runs: this.runs.slice(0, this.maxRuns) }, null, 2); | ||
| const tempPath = `${this.filePath}.tmp`; | ||
| await fs.writeFile(tempPath, payload, 'utf8'); | ||
| await fs.rename(tempPath, this.filePath); | ||
| } |
There was a problem hiding this comment.
Concurrent persists race on a shared temp file and can corrupt history.
appendEvent is invoked fire-and-forget from both /api/orchestrate handlers, and completeRun/failRun also persist. With concurrent runs (or an unawaited appendEvent overlapping completeRun on the same run), two persist() calls run simultaneously. Both write to the same ${this.filePath}.tmp path and then rename it: the writes can interleave (corrupt JSON) and the second rename can fail with ENOENT once the first already moved the temp file. Use a unique temp name and serialize writes.
🔒 Proposed fix (unique temp + write queue)
async persist() {
- const payload = JSON.stringify({ runs: this.runs.slice(0, this.maxRuns) }, null, 2);
- const tempPath = `${this.filePath}.tmp`;
- await fs.writeFile(tempPath, payload, 'utf8');
- await fs.rename(tempPath, this.filePath);
+ this._writeChain = (this._writeChain || Promise.resolve()).then(async () => {
+ const payload = JSON.stringify({ runs: this.runs.slice(0, this.maxRuns) }, null, 2);
+ const tempPath = `${this.filePath}.${process.pid}.${crypto.randomBytes(4).toString('hex')}.tmp`;
+ await fs.writeFile(tempPath, payload, 'utf8');
+ await fs.rename(tempPath, this.filePath);
+ });
+ return this._writeChain;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/run-history.js` around lines 131 - 136, The persist() method in
run-history.js can race because multiple callers (appendEvent, completeRun,
failRun) write to the same temp file; fix by making persist() use a unique temp
filename (e.g., include a timestamp/UUID or process+pid+counter) instead of
`${this.filePath}.tmp` and serialize concurrent persists with an internal write
queue or mutex on the RunHistory instance (add a simple Promise-based queue or
lock property used by persist()). Update persist() to await acquiring the lock,
perform writeFile to the unique temp name and rename, then release the lock;
ensure appendEvent, completeRun, and failRun continue to call persist() but will
now be ordered and safe.
1 participant
Summary
This PR adds durable run history so orchestration and payment activity can be reviewed after restarts for debugging and demos.
What changed
fileormemory)GET /api/runsto fetch recent runsAudit data captured
totalSpent, payment protocol, tx counts, elapsed)txHash,explorerUrl, payment method)Config
RUN_HISTORY_STORAGE(fileormemory)RUN_HISTORY_FILE(default:./data/run-history.json)RUN_HISTORY_MAX_RUNS(default:200)Acceptance criteria
filemode)Validation
npm testpasses