Fix #507: warn on stale ledger reads - #511
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe ledger now records its last write time and evaluates configurable staleness. CLI read commands warn for stale data. SDK, Node, and MCP responses expose freshness metadata. Schema migrations, configuration tests, smoke tests, and changelogs cover the new behavior. ChangesLedger freshness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReadCommand
participant LedgerHandle
participant archive_state
participant Renderer
ReadCommand->>LedgerHandle: request ledger freshness
LedgerHandle->>archive_state: read last_write_at_ms
archive_state-->>LedgerHandle: return write timestamp
LedgerHandle-->>ReadCommand: return stale status and threshold
ReadCommand->>Renderer: emit warning and render result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1866848c93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/relayburn-sdk/src/ledger/writer.rs (1)
314-361: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOnly refresh ledger freshness when
append_stampwrites derived rows.
append_stampis reachable fromburn stamp writeand ingest-side pending-stamp resolution. It currently callstouch_last_write(&tx)after every stamp insert, but stamps are first-party data andappend_stampis considered part of derive replay. If nosynthesized.relationshipsrow is written,last_write_at_msshould not advance; update the clock only whensynthesized.is_some(), or change the freshness contract and docs if stamp inserts should count as derived-ledger writes.🤖 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 `@crates/relayburn-sdk/src/ledger/writer.rs` around lines 314 - 361, Update append_stamp so touch_last_write(&tx) is called only when synthesize_relationship(stamp) returns Some and a derived relationship row is written; leave stamp-only inserts without refreshing last_write_at_ms.
🧹 Nitpick comments (1)
packages/mcp/src/tools/fingerprint.test.ts (1)
18-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the complete freshness contract in both tests.
Both tests assert only
ledgerFreshness.stale === false. A regression that dropslastWriteAtMs, changesstaleAfterMs, or always returnsfalsewould pass. Assert all three fields and add astale: truecase.
packages/mcp/src/tools/fingerprint.test.ts#L18-L18: assert the complete freshness object and cover the stale case.packages/mcp/src/tools/session-cost.test.ts#L28-L28: apply the same assertions to the session-cost response.🤖 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 `@packages/mcp/src/tools/fingerprint.test.ts` at line 18, Expand the freshness assertions in both tests: packages/mcp/src/tools/fingerprint.test.ts at lines 18-18 and packages/mcp/src/tools/session-cost.test.ts at lines 28-28 must verify lastWriteAtMs, staleAfterMs, and stale, not only stale === false. Add a stale: true scenario to each test while preserving the existing fresh-case coverage.
🤖 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 `@crates/relayburn-cli/tests/smoke.rs`:
- Around line 74-126: Remove the inherited RELAYBURN_STALE_AFTER_HOURS override
from the freshness smoke-test setup by updating burn() or each of
stale_warning_is_uniform_across_requested_read_surface,
fresh_ledger_does_not_warn, and never_written_ledger_warns. Ensure these tests
use the staleness threshold from config.json and retain their expected warning
behavior.
In `@crates/relayburn-sdk-node/src/lib.rs`:
- Around line 391-403: Update the #[napi] ledger_freshness function to use the
project’s native asynchronous N-API pattern and move Ledger::open plus
ledger_freshness into tokio::task::spawn_blocking, returning the joined result
through the existing BurnError mapping while preserving the LedgerFreshness
conversion.
In `@packages/sdk-node/test/conformance.test.js`:
- Around line 89-94: Update the freshness assertions in the ledgerFreshness
conformance test to accept staleAfterMs as either a number or null, and add or
extend a fixture case with a negative thresholdHours to verify the
disabled-warning response shape if coverage is not already present.
In `@README.md`:
- Around line 215-222: Update the RELAYBURN_CONTENT_STORE table entry in the
README to escape each literal pipe character in the value, preserving the
documented full, hash-only, and off options while keeping the Markdown table at
two columns.
---
Outside diff comments:
In `@crates/relayburn-sdk/src/ledger/writer.rs`:
- Around line 314-361: Update append_stamp so touch_last_write(&tx) is called
only when synthesize_relationship(stamp) returns Some and a derived relationship
row is written; leave stamp-only inserts without refreshing last_write_at_ms.
---
Nitpick comments:
In `@packages/mcp/src/tools/fingerprint.test.ts`:
- Line 18: Expand the freshness assertions in both tests:
packages/mcp/src/tools/fingerprint.test.ts at lines 18-18 and
packages/mcp/src/tools/session-cost.test.ts at lines 28-28 must verify
lastWriteAtMs, staleAfterMs, and stale, not only stale === false. Add a stale:
true scenario to each test while preserving the existing fresh-case coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7487436-3b45-44ca-b531-103f77f3551f
📒 Files selected for processing (32)
CHANGELOG.mdREADME.mdcrates/relayburn-cli/src/commands/freshness.rscrates/relayburn-cli/src/commands/hotspots/mod.rscrates/relayburn-cli/src/commands/mcp_server.rscrates/relayburn-cli/src/commands/mod.rscrates/relayburn-cli/src/commands/sessions.rscrates/relayburn-cli/src/commands/state.rscrates/relayburn-cli/src/commands/summary/mod.rscrates/relayburn-cli/tests/smoke.rscrates/relayburn-sdk-node/src/lib.rscrates/relayburn-sdk/src/ledger.rscrates/relayburn-sdk/src/ledger/config.rscrates/relayburn-sdk/src/ledger/db.rscrates/relayburn-sdk/src/ledger/schema.rscrates/relayburn-sdk/src/ledger/tests.rscrates/relayburn-sdk/src/ledger/writer.rscrates/relayburn-sdk/src/lib.rscrates/relayburn-sdk/src/query_verbs/state.rscrates/relayburn-sdk/src/query_verbs/tests.rspackages/mcp/CHANGELOG.mdpackages/mcp/src/end-to-end.test.tspackages/mcp/src/tools/fingerprint.test.tspackages/mcp/src/tools/fingerprint.tspackages/mcp/src/tools/session-cost.test.tspackages/mcp/src/tools/session-cost.tspackages/sdk-node/CHANGELOG.mdpackages/sdk-node/src/binding.d.tspackages/sdk-node/src/index.cjspackages/sdk-node/src/index.d.tspackages/sdk-node/src/index.jspackages/sdk-node/test/conformance.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the two non-inline CodeRabbit findings in
Validation also includes |
1 participant
Summary
Validation
Fixes #507
🤖 Generated with Claude Code.