Repository navigation
feat(filesystem): CAS-guarded delete_if_version on RootFilesystem - #5749
Conversation
put has a CAS precondition (CasExpectation) but delete is blind, so no
caller can remove a record only-if-unchanged. Add an additive trait
method delete_if_version(path, expected) with a default Unsupported
impl (put's pattern) — ~20 blind-delete call sites are untouched.
Semantics: single-key only (no subtree/event-log/sequence sweep, unlike
blind delete); absent row → NotFound (already gone, benign) vs row at
another version → VersionMismatch{expected, found} (gone stale) — a new
two-branch diagnosis, since put's CAS paths collapse absent into
VersionMismatch{found: None}; CasExpectation::Absent is meaningless for
a delete and fails closed with Unsupported (decoded once in
CasExpectation::required_delete_version, shared by all backends).
Implemented for in-memory, libSQL, and Postgres (named SQL consts +
single-round-trip/single-key pin test, mirroring the PUT_*_SQL idiom),
with ScopedFilesystem and CompositeRootFilesystem passthroughs.
Tests: dual-backend contract coverage (correct-version delete, stale
version loses with entry surviving at the bumped version, missing path,
single-key event-log survival) in db_root_filesystem_contract.rs plus
in-memory crate tests and composite routing in catalog_contract.rs;
Postgres tests skip without a reachable DB per suite convention.
Mutation-checked: collapsing absent into VersionMismatch and deleting
despite a mismatch each turn the pinned assertions red.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
⏳ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
github-actions
Bot
added
size: L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesCAS-conditional delete_if_version
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: 58160f8a98c0b3a5e04d5c1910c2f3b5bd1a15d1
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The new CAS-delete API is implemented for the main SQL/in-memory backends and composite routing, with focused contract tests. One existing backend now advertises CAS+Delete but still inherits the new Unsupported default, so generic callers can get a runtime failure despite mount capabilities saying the backend supports the needed primitives.
Findings
1. ❌ [MEDIUM] Implement CAS delete for HSM backend or stop advertising CAS delete support
Location: crates/ironclaw_filesystem/src/root.rs:174-179
Adding RootFilesystem::delete_if_version with an Unsupported default leaves existing implementors on the default unless they opt in. HsmBackend currently advertises Capability::Delete plus TxnCapability::Cas and delegates put/get/delete to its in-memory backend, but it does not override this new method, so a /secrets mount backed by HSM will pass capability validation and then fail at runtime for the new CAS-delete path. Please either delegate HsmBackend::delete_if_version to inner.delete_if_version(...) or adjust its declared capabilities/API contract so callers cannot treat it as CAS-delete capable.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
| /// and the meaningless-for-delete `CasExpectation::Absent` is rejected | ||
| /// with [`FilesystemError::Unsupported`]. Default impl is `Unsupported`, | ||
| /// same as [`put`](Self::put): backends opt in natively. | ||
| async fn delete_if_version( |
There was a problem hiding this comment.
This new default means existing backends silently become Unsupported for CAS delete. HsmBackend still advertises Delete + TxnCapability::Cas but does not override this method, so generic callers can pass mount capability checks and then fail at runtime. Please delegate the HSM implementation to its inner backend or make the capability contract distinguish that this backend cannot serve CAS delete.
There was a problem hiding this comment.
Fixed in 6d97cd2: HsmBackend.delete_if_version now delegates to self.inner.delete_if_version, mirroring the existing put/get/stat/delete delegations. Grepped all other impl RootFilesystem in the crate — LocalFilesystem is the only other production backend and it doesn't override capabilities() (defaults to all-false), so it never overclaims Delete/Cas and isn't affected. Added hsm::tests::delete_if_version_delegates_to_inner_backend (wrong version → VersionMismatch, correct version deletes, re-delete → NotFound) — mutation-verified: reverting the delegation to Err(Unsupported) turns this RED.
There was a problem hiding this comment.
Fixed pre-session in 6d97cd2 (round 1): HsmBackend::delete_if_version now delegates to its inner backend, with delegation coverage. Predates this session (round 5) — leaving this thread unresolved for the reviewer/maintainer of record, but confirmed hsm.rs's current state already addresses it.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.34% — 280958 / 329213 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-5749 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add CAS-guarded single-key delete_if_version to RootFilesystem across supported backends without changing existing blind delete behavior.
Stats: 6 findings (from 8 raw, 6 after dedup/filter) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2
conventions
- Medium New filesystem API is missing from the contract docs (
crates/ironclaw_filesystem/src/root.rs:174-180, confidence 90) — anchor: crates/ironclaw_filesystem/AGENTS.md:7 (no diff position — body only)
The diff adds public RootFilesystem::delete_if_version semantics, but crates/ironclaw_filesystem/AGENTS.md says docs/reborn/contracts/filesystem.md is the source of truth before behavior changes. That contract still lists only delete in the permission table and its API sketch omits delete_if_version, so consumers cannot derive the delete permission, single-key behavior, backend support/default Unsupported behavior, or NotFound/VersionMismatch/Unsupported contract from the authoritative docs.
tests
- Medium Scoped CAS delete boundary is untested (
crates/ironclaw_filesystem/src/scoped.rs:541-550, confidence 90) — anchor: crates/ironclaw_filesystem/src/scoped.rs:541
The new public ScopedFilesystem::delete_if_version resolves the scoped path with Delete permission before delegating to the root backend, but the added tests call only root/composite/backend APIs. A wrong permission operation or path resolution at the scoped boundary would not fail any current test. - Medium libSQL Any delete branch has no contract test (
crates/ironclaw_filesystem/src/libsql.rs:1005-1010, confidence 85) — anchor: crates/ironclaw_filesystem/src/libsql.rs:1005
The libSQL tests exercise Version current/stale/missing behavior, but no test calls delete_if_version with CasExpectation::Any. That leaves the unconditional single-key DELETE SQL branch and its second-delete NotFound behavior unexercised for this backend; the in-memory Any test does not cover the libSQL query path. - Medium Postgres Any delete branch has no runtime test (
crates/ironclaw_filesystem/src/postgres.rs:1502-1502, confidence 85) — anchor: crates/ironclaw_filesystem/src/postgres.rs:1502
The Postgres contract test covers Version current/stale/missing and the SQL pin test checks statement shape, but no runtime test calls delete_if_version with CasExpectation::Any. A regression in the unconditional execution path or its zero-row NotFound diagnosis would not be caught.
performance
- Medium Stale CAS deletes open a second libSQL connection (
crates/ironclaw_filesystem/src/libsql.rs:1020-1022, confidence 82) — anchor: crates/ironclaw_filesystem/src/libsql.rs:1020
When a version-guarded delete affects zero rows, the diagnostic path calls self.current_version(path), which opens a fresh libSQL connection instead of using the existing connection that performed the DELETE. In a contended CAS-delete path, each loser now pays an extra connection open and SELECT, amplifying SQLite single-writer contention and transient file-open pressure.
maintainability
- Medium Narrow delete_if_version to the only supported precondition (
crates/ironclaw_filesystem/src/root.rs:174-177, confidence 75) — anchor: crates/ironclaw_filesystem/src/root.rs:174 (no diff position — body only)
The new public trait API accepts the full CasExpectation enum even though the stated need is version-guarded deletion. That broad parameter commits the contract to two extra modes: Absent, which every backend rejects as meaningless, and Any, which creates a second unconditional single-key delete path with no named production caller. A RecordVersion parameter would satisfy the CAS-delete need without adding hidden blind-delete semantics under a CAS-named method.
Also flagged by: approach/Low
|
|
||
| /// Delete the single entry at `path` only when its version satisfies | ||
| /// `expected`. See [`RootFilesystem::delete_if_version`]. | ||
| pub async fn delete_if_version( |
There was a problem hiding this comment.
Medium — Scoped CAS delete boundary is untested.
The new public ScopedFilesystem::delete_if_version resolves the scoped path with Delete permission before delegating to the root backend, but the added tests call only root/composite/backend APIs. A wrong permission operation or path resolution at the scoped boundary would not fail any current test.
Fix: Add tests::scoped::delete_if_version_requires_delete_permission_and_routes_to_backend covering no-delete PermissionDenied and successful CAS delete through a scoped mount.
There was a problem hiding this comment.
Fixed in 6d97cd2: added scoped::tests::delete_if_version_denies_when_delete_missing (mount has read/write/list but not delete → PermissionDenied{operation: Delete} before backend dispatch) and scoped::tests::delete_if_version_succeeds_with_delete_and_routes_to_backend (correct-version CAS delete through the scoped mount actually removes the entry, proving it isn't a passthrough stub). Mutation-verified both: hardcoding the Delete permission check to true turned the first test RED.
There was a problem hiding this comment.
Fixed pre-session in 2c1bc52 (round 2): scoped/tests.rs now has delete_if_version_denies_when_delete_missing and delete_if_version_succeeds_with_delete_and_routes_to_backend covering exactly this boundary. Predates this session — leaving unresolved for the reviewer of record.
| ) | ||
| .await | ||
| } | ||
| None => { |
There was a problem hiding this comment.
Medium — libSQL Any delete branch has no contract test.
The libSQL tests exercise Version current/stale/missing behavior, but no test calls delete_if_version with CasExpectation::Any. That leaves the unconditional single-key DELETE SQL branch and its second-delete NotFound behavior unexercised for this backend; the in-memory Any test does not cover the libSQL query path.
Fix: Add tests::db_root_filesystem_contract::libsql_delete_if_version_any_deletes_single_key_unconditionally covering Any delete, event log survival, and second Any returning NotFound.
There was a problem hiding this comment.
Fixed in 6d97cd2: added libsql_delete_if_version_any_deletes_single_key_unconditionally in tests/db_root_filesystem_contract.rs — Any deletes regardless of current version, the event log at the same path survives (single-key, not a subtree sweep), and a second Any against the now-absent row is NotFound rather than a silent no-op.
There was a problem hiding this comment.
Moot as of round 3 (5e5fe58, predates this session): delete_if_version's signature was narrowed from CasExpectation to a plain RecordVersion (maintainer verdict on the ABA finding), so Any/Absent are no longer representable at this API at all — there's no Any branch left to test. Leaving unresolved since the resolution predates this session; flagging so the thread isn't mistaken for still-open.
| ) | ||
| .await | ||
| } | ||
| None => cached_execute(client, DELETE_IF_VERSION_ANY_SQL, &[&path.as_str()]).await, |
There was a problem hiding this comment.
Medium — Postgres Any delete branch has no runtime test.
The Postgres contract test covers Version current/stale/missing and the SQL pin test checks statement shape, but no runtime test calls delete_if_version with CasExpectation::Any. A regression in the unconditional execution path or its zero-row NotFound diagnosis would not be caught.
Fix: Add tests::db_root_filesystem_contract::postgres_delete_if_version_any_deletes_single_key_unconditionally covering Any delete, event log survival, and second Any returning NotFound.
There was a problem hiding this comment.
Fixed in 6d97cd2: added postgres_tests::postgres_delete_if_version_any_deletes_single_key_unconditionally in the existing env-gated contract suite (skips cleanly via postgres_root() -> None when no DATABASE_URL/IRONCLAW_FILESYSTEM_POSTGRES_URL is set). Same shape as the libsql sibling: Any deletes unconditionally, event log survives, second Any on the now-absent row is NotFound. Ran against a live Postgres in this environment and it passed.
There was a problem hiding this comment.
Same as the sibling libSQL Any-delete thread: moot since round 3 (5e5fe58) narrowed delete_if_version to a plain RecordVersion, removing CasExpectation::Any/Absent from this API entirely. Predates this session; leaving unresolved for the reviewer of record.
HsmBackend advertised Delete+Cas but fell through to Unsupported at runtime; now delegates to its inner backend. Adds scoped-mount permission/routing tests, drops the libsql delete's connection before the zero-rows current_version lookup (pool-exhaustion guard ahead of #5751), and adds Any-branch contract tests for libsql and postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add CAS-guarded delete_if_version to RootFilesystem with backend implementations and contract tests while preserving existing blind deletes.
Stats: 4 net-new findings (from 7 raw; duplicate live-thread findings suppressed) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Findings
-
Medium Filesystem contract docs omit the new API (
crates/ironclaw_filesystem/src/root.rs:166-180, confidence 100) — anchor:crates/ironclaw_filesystem/AGENTS.md:7
The diff extends the publicRootFilesystemcontract withdelete_if_version, butdocs/reborn/contracts/filesystem.mdstill omits the CAS delete operation from its API sketch and contract text.CasExpectation's public docs also still describe the enum as put-only, so the source-of-truth contract and generated API docs drift from the new public behavior. -
Medium Scoped CAS delete lacks stale-version propagation coverage (
crates/ironclaw_filesystem/src/scoped.rs:550, confidence 75) — anchor:crates/ironclaw_filesystem/src/scoped.rs:550
The new scoped wrapper is tested for delete permission denial and correct-version success, but not for forwarding a staleCasExpectation::Versionto the backend. A regression that accidentally ignoredexpectedand performed an unconditional delete would still pass the current scoped tests. -
Medium libSQL CAS delete lacks
Absentrejection coverage (crates/ironclaw_filesystem/src/libsql.rs:993, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql.rs:993
The libSQL backend callsrequired_delete_version, but backend contract tests coverVersionandAnyonly. The PR definesCasExpectation::Absentas fail-closedUnsupportedfor delete, and no libSQL test asserts that error path leaves the entry intact. -
Medium Postgres CAS delete lacks
Absentrejection coverage (crates/ironclaw_filesystem/src/postgres.rs:1491, confidence 75) — anchor:crates/ironclaw_filesystem/src/postgres.rs:1491
The Postgres backend callsrequired_delete_version, but runtime contract tests coverVersionandAnyonly. The same fail-closedAbsentcontract should be pinned for Postgres so a backend-specific bypass cannot silently become an unconditional delete.
| /// and the meaningless-for-delete `CasExpectation::Absent` is rejected | ||
| /// with [`FilesystemError::Unsupported`]. Default impl is `Unsupported`, | ||
| /// same as [`put`](Self::put): backends opt in natively. | ||
| async fn delete_if_version( |
There was a problem hiding this comment.
Medium — Filesystem contract docs omit the new API.
This adds RootFilesystem::delete_if_version, but docs/reborn/contracts/filesystem.md still omits the CAS delete operation from its API sketch and contract text. CasExpectation's public docs also still describe the enum as put-only, so both the source-of-truth contract and generated API docs drift from the new behavior.
Fix: Update docs/reborn/contracts/filesystem.md with delete_if_version semantics, Delete permission requirements, single-key behavior, and the scoped/root API shape; also update the CasExpectation enum docs to mention delete semantics and that Absent is rejected for delete.
There was a problem hiding this comment.
Fixed the rustdoc half in 2c1bc52 — scoping to this PR: CasExpectation's doc comment now says it's the precondition for both put and delete_if_version, spells out the two meaningful delete branches (Version = guarded delete, Any = unconditional) and states that Absent is rejected with Unsupported for delete rather than being folded into an unconditional delete; each variant's own doc comment got the delete-specific caveat too. delete_if_version's own rustdoc on the RootFilesystem trait (root.rs) already stated the NotFound/VersionMismatch/Unsupported taxonomy from the original PR, so no change was needed there. Deliberately NOT touching docs/reborn/contracts/filesystem.md here — that's owned by the docs PR #5748 landing separately; updating it in both PRs would just create merge friction.
There was a problem hiding this comment.
Declining to fix in this session: the 'Initial Rust API sketch' section (docs/reborn/contracts/filesystem.md §14) is already broadly stale relative to the current unified entry-plane API — it doesn't show put/get/CasExpectation either, which predate this PR. Bolting on just delete_if_version would misrepresent the doc's currency; recommend a follow-up pass that modernizes §14 wholesale. The CasExpectation-docs half of this comment is now accurate again: round 3 (5e5fe58) narrowed delete_if_version off CasExpectation entirely, so CasExpectation genuinely is put-only.
| let started_at = live_latency_started_at(); | ||
| let virtual_path = | ||
| self.resolve_with_permission(scope, path, FilesystemOperation::Delete)?; | ||
| let result = self.root.delete_if_version(&virtual_path, expected).await; |
There was a problem hiding this comment.
Medium — Scoped CAS delete lacks stale-version propagation coverage.
The new scoped wrapper is tested for delete permission denial and correct-version success, but not for forwarding a stale CasExpectation::Version to the backend. A regression that ignored expected and performed an unconditional delete would still pass the current scoped tests.
Fix: Add scoped::tests::delete_if_version_rejects_stale_version_and_preserves_entry, using a stale version through ScopedFilesystem::delete_if_version and asserting VersionMismatch plus entry preservation.
There was a problem hiding this comment.
Fixed in 2c1bc52: added scoped::tests::delete_if_version_rejects_stale_version_and_preserves_entry — puts, bumps to v2, then calls ScopedFilesystem::delete_if_version(scope, path, CasExpectation::Version(v1)) (stale), asserts FilesystemError::VersionMismatch{expected: Some(v1), found: Some(v2)}, then reads the entry back and asserts it's still present at v2. Mutation-checked: patched delete_if_version to ignore its expected argument and forward CasExpectation::Any to self.root instead — this new test goes RED (panics 'expected an error') while the existing permission-denial and correct-version-success tests stay green, proving only this test catches an expected-swallowing regression. Reverted after confirming.
There was a problem hiding this comment.
Fixed pre-session in 2c1bc52 (round 2): scoped/tests.rs's delete_if_version_rejects_stale_version_and_preserves_entry drives a stale CasExpectation::Version through ScopedFilesystem::delete_if_version and asserts VersionMismatch + entry preservation. Predates this session; leaving unresolved for the reviewer of record.
| // Single-key CAS delete: unlike `delete`, no subtree/event/sequence | ||
| // sweep. `is_dir = 0` scopes it to the record plane, matching `put`'s | ||
| // Version arm and `current_version`. | ||
| let required_version = expected.required_delete_version(path)?; |
There was a problem hiding this comment.
Medium — libSQL CAS delete lacks Absent rejection coverage.
The libSQL backend calls required_delete_version, but the backend contract tests cover Version and Any only. The PR defines CasExpectation::Absent as fail-closed Unsupported for delete, and no libSQL test asserts that error path leaves the entry intact.
Fix: Add db_root_filesystem_contract::libsql_delete_if_version_rejects_absent_expectation covering Unsupported and no deletion when CasExpectation::Absent is passed.
There was a problem hiding this comment.
Fixed in 2c1bc52: added libsql_delete_if_version_rejects_absent_expectation to tests/db_root_filesystem_contract.rs — puts an entry, calls delete_if_version(CasExpectation::Absent), asserts FilesystemError::Unsupported{operation: Delete}, then re-reads and asserts the entry survives unchanged. Ran live against a real libSQL DB and passed. Mutation-checked: reverting CasExpectation::required_delete_version's Absent arm to Ok(None) (unconditional delete) turns this test RED (panicked on unwrap_err with an Ok(()) value) while the sibling Version/Any tests stay green — confirms the assertion is load-bearing.
There was a problem hiding this comment.
Moot as of round 3 (5e5fe58, predates this session): delete_if_version's signature was narrowed to a plain RecordVersion, so CasExpectation::Absent is no longer representable at this API — there's no Absent branch left to reject. Leaving unresolved since the resolution predates this session.
| path: &VirtualPath, | ||
| expected: CasExpectation, | ||
| ) -> Result<(), FilesystemError> { | ||
| let required_version = expected.required_delete_version(path)?; |
There was a problem hiding this comment.
Medium — Postgres CAS delete lacks Absent rejection coverage.
The Postgres backend calls required_delete_version, but runtime contract tests cover Version and Any only. The same fail-closed Absent contract should be pinned for Postgres so a backend-specific bypass cannot silently become an unconditional delete.
Fix: Add postgres_delete_if_version_rejects_absent_expectation to the env-gated Postgres contract suite, asserting Unsupported and entry preservation.
There was a problem hiding this comment.
Fixed in 2c1bc52: added postgres_tests::postgres_delete_if_version_rejects_absent_expectation to the env-gated Postgres contract suite in tests/db_root_filesystem_contract.rs — puts an entry, calls delete_if_version(CasExpectation::Absent), asserts FilesystemError::Unsupported{operation: Delete}, then re-reads and asserts the entry survives at its original version/body. No live Postgres is reachable in this environment (no docker, no IRONCLAW_FILESYSTEM_POSTGRES_URL/DATABASE_URL), so the test exercises the documented clean-skip path here; the assertion logic mirrors the sibling Version/Any tests that ran live in the prior lane. Mutation-checked: reverting required_delete_version's Absent arm to Ok(None) (fall through to unconditional delete) turns this RED alongside the libSQL counterpart.
There was a problem hiding this comment.
Same as the sibling libSQL Absent-rejection thread: moot since round 3 (5e5fe58) removed CasExpectation from delete_if_version's signature entirely. Predates this session; leaving unresolved for the reviewer of record.
…coverage Round-2 review fixes for #5749: - postgres/libsql: Absent rejects with Unsupported, entry preserved - scoped: stale Version through delete_if_version surfaces VersionMismatch and forwards `expected` rather than swallowing it - CasExpectation rustdoc now covers delete_if_version semantics Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
size: XL
Apply 7 review findings plus one import from PR #5749: header metadata shape, bounded boot-recovery concurrency + roster cardinality, a roster-before-edge-consistent rewrite of the residual delete/roster race (and its test), batched (not per-edge) settled-drain transcript writes, a bounded LIMIT-8 run-budget query in place of an open-ended walk, a lease (owner+expires_at, lazy expiry) on the human-priority reservation marker, in-path agent/project axis encoding for await-edge and roster paths (mount alone only partitions tenant/user), and the delete_if_version CAS contract in the filesystem contract doc. Thermo-reviewed twice: first pass caught an off-by-one in the budget query and a placeholder-spelling drift, both fixed and re-verified clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add an additive CAS-guarded delete_if_version primitive to RootFilesystem across supported filesystem backends with contract tests.
Stats: 2 findings (from 6 raw, 2 after dedup/live-thread suppression) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Bugs
- High CAS delete can remove a recreated row with the same version (
crates/ironclaw_filesystem/src/postgres.rs:1650-1663, confidence 75) — anchor:crates/ironclaw_filesystem/src/postgres.rs:1650
The guarded delete only checks the row's numericRecordVersion, but a path deleted and recreated withCasExpectation::Absentstarts back at version 1. A stale caller that read v1 before delete/recreate can later calldelete_if_version(..., Version(v1))and delete the new row, so the delete is not actually guarded against an unchanged record. The same version-only token shape exists in libSQL and in-memory. Also flagged by: performance/High.
Maintainability
- Medium Default CAS delete leaves MountScopedRootFilesystem out of sync (
crates/ironclaw_filesystem/src/root.rs:174-180, confidence 75) — anchor:crates/ironclaw_filesystem/src/root.rs:174; crates/ironclaw_host_runtime/src/invocation_services.rs:363
The new defaultUnsupportedmethod makes forwarding wrappers easy to miss.MountScopedRootFilesystemdelegates capabilities from the inner root and forwardsdeleteafter permission/path resolution, but it does not overridedelete_if_version, so host-runtime filesystem views can advertise the inner backend's CAS/Delete support and then fail withUnsupportedat call time.
| ) -> Result<(), FilesystemError> { | ||
| let required_version = expected.required_delete_version(path)?; | ||
| let deleted = match required_version { | ||
| Some(expected_version) => { |
There was a problem hiding this comment.
High — CAS delete can remove a recreated row with the same version.
The guarded delete only checks the row's numeric RecordVersion, but a path deleted and recreated with CasExpectation::Absent starts back at version 1. A stale caller that read v1 before delete/recreate can later call delete_if_version(..., Version(v1)) and delete the new row, so the delete is not actually guarded against an unchanged record. The same version-only token shape exists in libSQL and in-memory.
Fix: Make the CAS token generation-stable across delete/recreate, for example by preserving a monotonic per-path version/tombstone or adding a row generation id, then match delete_if_version on that token instead of a resettable row version.
Also flagged by: performance/High
There was a problem hiding this comment.
Maintainer decision made pre-session in 5e5fe58 (round 3): explicitly declined generation-stable tokens, narrowed the signature to a plain RecordVersion instead, and documented the ABA hazard as a caller invariant on the trait doc. This session added regression pins for that exact documented behavior (delete_if_version_is_vulnerable_to_aba_across_delete_recreate_cycles, in_memory + libsql + postgres, round A/B) but did not revisit the generation-stable-tokens design call itself — leaving unresolved since that decision predates this session.
| /// and the meaningless-for-delete `CasExpectation::Absent` is rejected | ||
| /// with [`FilesystemError::Unsupported`]. Default impl is `Unsupported`, | ||
| /// same as [`put`](Self::put): backends opt in natively. | ||
| async fn delete_if_version( |
There was a problem hiding this comment.
Medium — Default CAS delete leaves MountScopedRootFilesystem out of sync.
The new default Unsupported method makes forwarding wrappers easy to miss. MountScopedRootFilesystem delegates capabilities from the inner root and forwards delete after permission/path resolution, but it does not override delete_if_version, so host-runtime filesystem views can advertise the inner backend's CAS/Delete support and then fail with Unsupported at call time.
Fix: Add delete_if_version to MountScopedRootFilesystem beside delete, resolving with FilesystemOperation::Delete and forwarding the expected CasExpectation to the inner root, with a caller-level test for the scoped host-runtime path.
There was a problem hiding this comment.
Fixed pre-session in 5e5fe58 (round 3): MountScopedRootFilesystem::delete_if_version now resolves with FilesystemOperation::Delete and forwards to the inner root, with delegation coverage. Predates this session; leaving unresolved for the reviewer of record.
…-only API Round 3 review fixes for PR #5749: - MountScopedRootFilesystem (ironclaw_host_runtime) and SkillManagementRootFilesystem (ironclaw_skills) both forward capabilities() to their inner backend but omitted delete_if_version, so a CAS-capable mount silently fell through to the trait default Unsupported. Both now resolve/delegate delete_if_version like their existing delete override. Workspace-wide audit of every RootFilesystem-wrapping decorator found only these two plus the already-fixed HsmBackend and the already-correct CompositeRootFilesystem — delegation tests added for both new fixes and mutation-checked (MountScoped) by reverting the override. - Maintainer verdict on the ABA finding: narrow delete_if_version's signature from CasExpectation to a plain RecordVersion (Any/Absent are unrepresentable for a delete) and document the ABA hazard as a caller invariant on the trait method's rustdoc instead of adding generation-stable tokens. Threaded the signature change through in_memory/libsql/postgres/scoped/catalog/hsm and every call site; along the way this also fixed a pre-existing libsql.rs build break (delete_if_version referenced a nonexistent self.current_version). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Round 3 review fixes pushed (5e5fe58). Finding 1 (Medium — MountScoped wrapper hole): Fixed. Workspace-wide audit of every type implementing
Delegation tests added for both fixed wrappers, mirroring the HSM delegation test (seed via CAS put, wrong-version → Finding 2 (High — ABA, per verdict): Applied per the maintainer verdict. The signature change threaded through every implementer and call site ( Verified: |
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add CAS-guarded delete_if_version to RootFilesystem with backend support so records can be deleted only when unchanged.
Stats: 5 net-new findings (from 9 raw; duplicate live-thread findings and low-value nits suppressed) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Performance
-
Medium Failed CAS delete diagnosis races concurrent writers (
crates/ironclaw_filesystem/src/libsql.rs:934-946, confidence 78) — anchor:crates/ironclaw_filesystem/src/libsql.rs:934
After the conditional DELETE affects zero rows,delete_if_versiondrops the connection and opens a new one before reading the current version. A concurrent delete/recreate or update in that window can flipNotFoundintoVersionMismatchorVersionMismatchintoNotFound, even though the new API documents those branches as distinct CAS outcomes. -
Medium Zero-row CAS delete classification is not atomic (
crates/ironclaw_filesystem/src/postgres.rs:1653-1660, confidence 76) — anchor:crates/ironclaw_filesystem/src/postgres.rs:1653
The DELETE and follow-up current-version SELECT run as separate READ COMMITTED statements. A concurrent writer can mutate or remove the row between them, so callers may seeNotFoundfor a stale-version delete orVersionMismatchfor an absent row despite the new API documenting distinct failure semantics.
Tests
-
Medium libSQL CAS delete overflow branch is untested (
crates/ironclaw_filesystem/src/libsql.rs:913, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql.rs:913
delete_if_versionnow propagatesrecord_version_to_i64(path, expected_version)?, but the libSQL contract tests only cover missing, stale, and current versions. No caller-level test passes an out-of-rangeRecordVersionand assertsCorruptRecordVersioninstead of a silent mismatch or backend error. -
Medium Postgres CAS delete overflow branch is untested (
crates/ironclaw_filesystem/src/postgres.rs:1642, confidence 75) — anchor:crates/ironclaw_filesystem/src/postgres.rs:1642
postgres_delete_if_version_with_clientnow propagatesrecord_version_to_i64(path, expected_version)?, but the Postgres contract tests only cover missing, stale, and current versions. No caller-level test exercises the typedCorruptRecordVersionpath for an out-of-range expected version.
Conventions
- Medium PR body still says HsmBackend is out of scope (
crates/ironclaw_filesystem/src/hsm.rs:115-126, confidence 100) — anchor:.claude/rules/review-discipline.md:52
The current PR body listsHsmBackendas out of scope, but the diff implementsHsmBackend::delete_if_versionand adds HSM coverage. The review-discipline rule says a PR's title and body must match its diff, so the description is now stale after the HSM fix landed.
| // Single-key CAS delete: unlike `delete`, no subtree/event/sequence | ||
| // sweep. `is_dir = 0` scopes it to the record plane, matching `put`'s | ||
| // Version arm and `current_version_libsql`. | ||
| let expected_raw = record_version_to_i64(path, expected_version)?; |
There was a problem hiding this comment.
Medium — libSQL CAS delete overflow branch is untested.
delete_if_version now propagates record_version_to_i64(path, expected_version)?, but the libSQL contract tests only cover missing, stale, and current versions. No caller-level test passes an out-of-range RecordVersion and asserts CorruptRecordVersion instead of a silent mismatch or backend error.
Fix: Add db_root_filesystem_contract::libsql_delete_if_version_rejects_out_of_range_expected_version covering RecordVersion::from_backend(u64::MAX) returns FilesystemError::CorruptRecordVersion.
There was a problem hiding this comment.
Fixed in 1792aeb: added db_root_filesystem_contract::libsql_delete_if_version_rejects_out_of_range_expected_version — RecordVersion::from_backend(u64::MAX) -> FilesystemError::CorruptRecordVersion, and the entry is confirmed to survive untouched (the guard fires in record_version_to_i64 before any DELETE runs). Mutation-checked: temporarily bypassing the guard (expected_version.get() as i64 instead of record_version_to_i64) flipped the assertion to a bogus VersionMismatch — confirmed RED for the right reason, then reverted.
There was a problem hiding this comment.
Fixed pre-session in 1792aeb (round 4): libsql_delete_if_version_rejects_out_of_range_expected_version asserts RecordVersion::from_backend(u64::MAX) surfaces CorruptRecordVersion with the entry preserved. Predates this session; leaving unresolved for the reviewer of record.
| path: &VirtualPath, | ||
| expected_version: RecordVersion, | ||
| ) -> Result<(), FilesystemError> { | ||
| let expected_raw = record_version_to_i64(path, expected_version)?; |
There was a problem hiding this comment.
Medium — Postgres CAS delete overflow branch is untested.
postgres_delete_if_version_with_client now propagates record_version_to_i64(path, expected_version)?, but the Postgres contract tests only cover missing, stale, and current versions. No caller-level test exercises the typed CorruptRecordVersion path for an out-of-range expected version.
Fix: Add db_root_filesystem_contract::postgres_delete_if_version_rejects_out_of_range_expected_version covering RecordVersion::from_backend(u64::MAX) returns FilesystemError::CorruptRecordVersion.
There was a problem hiding this comment.
Fixed in 1792aeb: added db_root_filesystem_contract::postgres_tests::postgres_delete_if_version_rejects_out_of_range_expected_version — same shape as the libSQL sibling, env-gated (skips cleanly via postgres_root() -> None without a reachable DATABASE_URL/IRONCLAW_FILESYSTEM_POSTGRES_URL), asserts CorruptRecordVersion and that the entry survives untouched.
There was a problem hiding this comment.
Same as the sibling libSQL overflow thread: fixed pre-session in 1792aeb (round 4) via postgres_delete_if_version_rejects_out_of_range_expected_version. Predates this session; leaving unresolved for the reviewer of record.
| self.inner.delete(path).await | ||
| } | ||
|
|
||
| async fn delete_if_version( |
There was a problem hiding this comment.
Medium — PR body still says HsmBackend is out of scope.
The current PR body lists HsmBackend as out of scope, but the diff implements HsmBackend::delete_if_version and adds HSM coverage. The review-discipline rule says a PR's title and body must match its diff, so the description is now stale after the HSM fix landed.
Fix: Update the PR body to include HsmBackend support, or remove the HsmBackend implementation and tests from this PR.
There was a problem hiding this comment.
Fixed in 1792aeb: updated the PR body via gh pr edit. Scope now lists every backend that implements delete_if_version (in-memory, libSQL, Postgres, HsmBackend) and every forwarding wrapper (ScopedFilesystem, CompositeRootFilesystem, MountScopedRootFilesystem in ironclaw_host_runtime, SkillManagementRootFilesystem in ironclaw_skills), notes the narrowed RecordVersion -> i64 overflow guard (CorruptRecordVersion before any statement runs), and states the ABA (version-not-generation-stable) invariant. Footer (🤖 Generated with [Claude Code](https://claude.com/claude-code)) preserved.
There was a problem hiding this comment.
Fixed pre-session in round 4 (1792aeb's commit message notes the PR-body sync). Confirmed the current PR body correctly lists HsmBackend as delegating to its inner backend, not out of scope. Predates this session; leaving unresolved for the reviewer of record.
Review round 4 (PR #5749): the zero-rows NotFound/VersionMismatch diagnosis in delete_if_version ran as a second, separate statement on both libSQL and Postgres, leaving a window for a concurrent delete+recreate to commit in between and misclassify the outcome. libSQL now wraps the conditional DELETE and the diagnosis SELECT in one BEGIN IMMEDIATE/COMMIT transaction on a single connection (same idiom as put). Postgres folds both into one WITH statement: a `locked` CTE takes SELECT ... FOR UPDATE, and the `deleted` CTE depends on it so the lock is held before the conditional delete runs; the SQL pin test now asserts FOR UPDATE and the CTE dependency. Also adds the missing out-of-range expected_version coverage on both backends (RecordVersion::from_backend(u64::MAX) -> CorruptRecordVersion, entry preserved), and syncs the PR body's stale HsmBackend-out-of-scope claim with the actual diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add CAS-guarded delete_if_version support to RootFilesystem and supported filesystem backends for subagent await-edge delivery.
Stats: 4 findings (from 4 raw, 4 after dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2.
Bugs
- High Postgres CAS delete references a missing CTE column (
crates/ironclaw_filesystem/src/postgres.rs:1651-1651, confidence 100) — anchor:crates/ironclaw_filesystem/src/postgres.rs:1651
DELETE_IF_VERSION_ATOMIC_SQLusespath IN (SELECT path FROM locked), but thelockedCTE only selectsversion. Every Postgresdelete_if_versioncall will fail at statement execution with an undefined-column error instead of deleting, returningNotFound, or returningVersionMismatch.
Tests
- Medium Default CAS delete Unsupported path is untested (
crates/ironclaw_filesystem/src/root.rs:181-187, confidence 75) — anchor:crates/ironclaw_filesystem/src/root.rs:181
RootFilesystem::delete_if_versionadds the opt-in default that existing non-overriding backends rely on, but the added tests only call overriding backends or forwarding wrappers. A regression that changed the defaultUnsupportedfallback would not fail.
Conventions
- Medium PR body still describes the old CasExpectation delete API (
crates/ironclaw_filesystem/src/root.rs:181-185, confidence 100) — anchor:.claude/rules/review-discipline.md:52(no diff position — body only)
The diff exposesdelete_if_version(path, RecordVersion), but the PR body still saysCasExpectation::Absentfails closed for delete and referencesrequired_delete_versionplusAny/Absentbranch coverage. The PR body should describe the currentRecordVersion-only API.
Local Patterns
- Low RecordVersion rustdoc still describes the old CAS surface (
crates/ironclaw_filesystem/src/record.rs:180-182, confidence 75) — anchor:crates/ironclaw_filesystem/src/record.rs:180(no diff position — body only)
RecordVersionsays versions are only compared viaCasExpectation::Version, but this PR addsRootFilesystem::delete_if_version(path, RecordVersion)as a second public comparison path. Update the exported primitive docs to mention both usages.
Note: this contains a High finding that would normally use REQUEST_CHANGES, but GitHub rejects request-changes reviews on self-authored PRs, so this was posted as COMMENT.
…ault-Unsupported test Round-5 review findings on PR #5749: 1. `DELETE_IF_VERSION_ATOMIC_SQL`'s `locked` CTE only projected `version`, while the `deleted` CTE's predicate referenced `path IN (SELECT path FROM locked)`. Verified live against Postgres 16 (EXPLAIN VERBOSE): Postgres does not error on the missing projection — it silently resolves the unqualified `path` as a correlated reference back to the outer DELETE's own row, making the join trivially self-referential rather than a real semi-join against `locked`. That happened to still return correct results here because the DELETE's other predicates already pin the exact row, but it's a latent correctness footgun tied to a design intent (force lock-then-delete sequencing via a genuine CTE dependency) that the implicit correlation doesn't actually provide. Fix: project `path` explicitly in `locked`'s SELECT list, so the semi-join is real. Confirmed identical behavior before/after via direct psql repro (delete-success, version-mismatch, path-absent) and the full live postgres_tests suite (76/76). 2. Added `delete_if_version_default_returns_unsupported` in root.rs, reusing the existing `DefaultBoundedBackend` test fixture, to pin the previously-untested trait-default `Unsupported` arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y + ABA pin Round-A self-review (3 sonnet subagents: bugs/correctness, concurrency, tests) over the full PR diff. Bugs/correctness reviewer found nothing beyond the already-fixed CTE bug. Fixes from the other two lenses: Concurrency: - libsql delete_if_version now validates expected_version (overflow guard) before self.connect()/BEGIN IMMEDIATE, so an out-of-range version fails closed before taking a pool checkout and the SQLite write lock — avoids needless contention under CAS storms. - Reworded three test comments (in_memory.rs, db_root_filesystem_contract.rs x2) that overclaimed "concurrent-writer interleaving" for what are actually single-task sequential scripts; they now point at concurrent_cas_storm.rs for genuine parallel coverage. Tests: - Added a delete_if_version concurrency storm (all 3 backends): WRITERS-way contention racing to delete_if_version the same path at a known version across DELETE_STORM_ROUNDS recreate/delete cycles, asserting exactly one winner per round and no backend/infrastructure errors. The prior storm test only ever exercised put/cas_update concurrency — delete_if_version (this PR's new op, and the target of the BEGIN IMMEDIATE/FOR UPDATE atomicity fix) had none. - Added delete_if_version_is_vulnerable_to_aba_across_delete_recreate_cycles in in_memory.rs, pinning the ABA hazard the trait doc comment already documents (version restarts at 1 after a full delete, so a stale token can match a later incarnation) — previously asserted only in prose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…+ ABA parity + postgres ordering Round-B self-review (3 fresh sonnet subagents, same 3 lenses, over the Round-A-updated diff, explicitly checking Round A for regressions). Bugs/correctness and concurrency reviewers found no regressions from Round A. Fixes from the tests lens plus one concurrency polish item: - Round A's new delete_if_version concurrency storm (concurrent_cas_storm.rs) is real coverage for N-way contention/pool-exhaustion, but a sharp tests-lens finding showed it cannot discriminate a regression to the pre-1792aebb2 two-statement delete+diagnose race (every racer shares one pre-fetched version; nothing recreates the path mid-round, so ordinary single-row locking passes the test with or without that fix). Added a deterministic, non-flaky pin instead: libsql.rs::delete_if_version_diagnosis_reuses_the_delete_connection_under_a_size_one_pool builds a size-1 pool and drives the stale-version (0-rows) branch — if the diagnosis ever checks out a second connection, it deadlocks against itself and times out. Verified this discriminates: temporarily reintroduced a duplicate self.connect() and confirmed the test fails with a pool-checkout-timeout error, then reverted. - Reworded run_delete_storm's doc comment to stop overclaiming it guards the atomicity fix; it now correctly describes what it proves (contention/pool-exhaustion correctness) and points at the new deterministic test and postgres's existing SQL-shape pin test for the atomicity regression itself. - Tightened the storm's win assertion to a per-round check (was summed across all DELETE_STORM_ROUNDS, so a 0-win round and a 2-win round elsewhere could have canceled out) — Round-B concurrency finding. - postgres delete_if_version now validates expected_version before self.client() (was after), matching libsql's Round-A fix for the same reason — consistency finding from the tests lens. - Added libsql/postgres ABA-hazard pin tests mirroring in_memory.rs's Round-A test, since both DB backends share the same "version resets to 1 after full delete" precondition and had no equivalent pin. - Added composite_delete_if_version_returns_mount_not_found, mirroring the existing append_batch mount-not-found test (LOW finding, cheap gap-fill). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ironclaw_filesystem/tests/concurrent_cas_storm.rs`:
- Around line 231-242: The helper that builds the Postgres-backed test setup is
swallowing configured failures in the URL parse, pool build, and migration path,
which turns real regressions into skipped tests. In the test setup function
around `PostgresRootFilesystem::new`, replace the `ok()?` and `is_err() { return
None; }` flow with explicit handling that returns a failure once
`IRONCLAW_FILESYSTEM_POSTGRES_URL` or `DATABASE_URL` is present but invalid.
Keep skipping only when no URL env var is set, and make `run_migrations()`
failures propagate loudly instead of converting them to `None`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 66280d15-9a44-4b2d-aa52-28c6a21a0279
📒 Files selected for processing (7)
crates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_filesystem/src/root.rscrates/ironclaw_filesystem/tests/catalog_contract.rscrates/ironclaw_filesystem/tests/concurrent_cas_storm.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
| let url = std::env::var("IRONCLAW_FILESYSTEM_POSTGRES_URL") | ||
| .or_else(|_| std::env::var("DATABASE_URL")) | ||
| else { | ||
| return; | ||
| }; | ||
| // silent-ok: an unparsable URL means the environment's Postgres config | ||
| // isn't usable here; skip rather than fail on a config format issue | ||
| // this test doesn't own. | ||
| let Ok(config) = url.parse::<tokio_postgres::Config>() else { | ||
| return; | ||
| }; | ||
| .ok()?; | ||
| let config = url.parse::<tokio_postgres::Config>().ok()?; | ||
| let manager = deadpool_postgres::Manager::new(config, tokio_postgres::NoTls); | ||
| // silent-ok: pool construction failure means the environment can't | ||
| // stand up a Postgres pool right now; skip rather than fail this | ||
| // storm test on infrastructure it doesn't own. | ||
| let Ok(pool) = deadpool_postgres::Pool::builder(manager) | ||
| let pool = deadpool_postgres::Pool::builder(manager) | ||
| .max_size(4) | ||
| .build() | ||
| else { | ||
| return; | ||
| }; | ||
| .ok()?; | ||
| let root = Arc::new(ironclaw_filesystem::PostgresRootFilesystem::new(pool)); | ||
| // silent-ok: an unreachable/misconfigured Postgres fails migrations; | ||
| // skip rather than fail this storm test on connectivity it doesn't own. | ||
| if root.run_migrations().await.is_err() { | ||
| return; | ||
| return None; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t turn configured Postgres failures into skipped tests.
ok()? and is_err() { return None; } make malformed DSNs, pool setup failures, and migration regressions pass vacuously. Skip only when no Postgres is configured; fail loud once a URL is present.
As per path instructions, “Fail loud: flag silent-failure patterns — .ok()? dropping errors.” As per coding guidelines, DB/IO reads must not use .ok()? on a Result without an explicit silent fallback reason.
Proposed fix
- let url = std::env::var("IRONCLAW_FILESYSTEM_POSTGRES_URL")
- .or_else(|_| std::env::var("DATABASE_URL"))
- .ok()?;
- let config = url.parse::<tokio_postgres::Config>().ok()?;
+ let url = match std::env::var("IRONCLAW_FILESYSTEM_POSTGRES_URL")
+ .or_else(|_| std::env::var("DATABASE_URL"))
+ {
+ Ok(url) => url,
+ Err(_) => return None, // silent-ok: optional Postgres storm tests need a configured DSN.
+ };
+ let config = url
+ .parse::<tokio_postgres::Config>()
+ .expect("configured Postgres storm URL must parse");
let manager = deadpool_postgres::Manager::new(config, tokio_postgres::NoTls);
let pool = deadpool_postgres::Pool::builder(manager)
.max_size(4)
.build()
- .ok()?;
+ .expect("configured Postgres storm pool must build");
let root = Arc::new(ironclaw_filesystem::PostgresRootFilesystem::new(pool));
- if root.run_migrations().await.is_err() {
- return None;
- }
+ root.run_migrations()
+ .await
+ .expect("configured Postgres storm database must run filesystem migrations");📝 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.
| let url = std::env::var("IRONCLAW_FILESYSTEM_POSTGRES_URL") | |
| .or_else(|_| std::env::var("DATABASE_URL")) | |
| else { | |
| return; | |
| }; | |
| // silent-ok: an unparsable URL means the environment's Postgres config | |
| // isn't usable here; skip rather than fail on a config format issue | |
| // this test doesn't own. | |
| let Ok(config) = url.parse::<tokio_postgres::Config>() else { | |
| return; | |
| }; | |
| .ok()?; | |
| let config = url.parse::<tokio_postgres::Config>().ok()?; | |
| let manager = deadpool_postgres::Manager::new(config, tokio_postgres::NoTls); | |
| // silent-ok: pool construction failure means the environment can't | |
| // stand up a Postgres pool right now; skip rather than fail this | |
| // storm test on infrastructure it doesn't own. | |
| let Ok(pool) = deadpool_postgres::Pool::builder(manager) | |
| let pool = deadpool_postgres::Pool::builder(manager) | |
| .max_size(4) | |
| .build() | |
| else { | |
| return; | |
| }; | |
| .ok()?; | |
| let root = Arc::new(ironclaw_filesystem::PostgresRootFilesystem::new(pool)); | |
| // silent-ok: an unreachable/misconfigured Postgres fails migrations; | |
| // skip rather than fail this storm test on connectivity it doesn't own. | |
| if root.run_migrations().await.is_err() { | |
| return; | |
| return None; | |
| let url = match std::env::var("IRONCLAW_FILESYSTEM_POSTGRES_URL") | |
| .or_else(|_| std::env::var("DATABASE_URL")) | |
| { | |
| Ok(url) => url, | |
| Err(_) => return None, // silent-ok: optional Postgres storm tests need a configured DSN. | |
| }; | |
| let config = url | |
| .parse::<tokio_postgres::Config>() | |
| .expect("configured Postgres storm URL must parse"); | |
| let manager = deadpool_postgres::Manager::new(config, tokio_postgres::NoTls); | |
| let pool = deadpool_postgres::Pool::builder(manager) | |
| .max_size(4) | |
| .build() | |
| .expect("configured Postgres storm pool must build"); | |
| let root = Arc::new(ironclaw_filesystem::PostgresRootFilesystem::new(pool)); | |
| root.run_migrations() | |
| .await | |
| .expect("configured Postgres storm database must run filesystem migrations"); |
🤖 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/ironclaw_filesystem/tests/concurrent_cas_storm.rs` around lines 231 -
242, The helper that builds the Postgres-backed test setup is swallowing
configured failures in the URL parse, pool build, and migration path, which
turns real regressions into skipped tests. In the test setup function around
`PostgresRootFilesystem::new`, replace the `ok()?` and `is_err() { return None;
}` flow with explicit handling that returns a failure once
`IRONCLAW_FILESYSTEM_POSTGRES_URL` or `DATABASE_URL` is present but invalid.
Keep skipping only when no URL env var is set, and make `run_migrations()`
failures propagate loudly instead of converting them to `None`.
Sources: Coding guidelines, Path instructions
…oses - Add a live two-connection Postgres integration test that drives the actual delete-then-recreate race delete_if_version's atomicity fix targets, asserting NotFound (not a misclassified VersionMismatch). Manually verified it fails against a reverted non-atomic implementation, confirming it's non-vacuous. - Harden the SQL string-inspection test against two concrete mutants that kept its substring checks green while breaking atomicity (FOR UPDATE SKIP LOCKED, misplaced version guard). - Add explicit-directory-row exclusion tests for delete_if_version on both DB backends (mirrors existing put-side coverage). - Prove the libsql size-1-pool connection returns to a clean, reusable state after a VersionMismatch, not just that it didn't deadlock. - Add delete=false-but-other-grants-true denial coverage at the MountScopedRootFilesystem layer (mirrors the ScopedFilesystem-layer test). - Document why delete_if_version takes a bare RecordVersion instead of CasExpectation; fix an unverifiable PR-number comment reference. Found by a fresh multi-lens review cycle (correctness/SQL-tracing, concurrency/pool, test-adequacy, API-contract, security, maintainability) run after 4 external + 2 internal review rounds; only test-adequacy gaps surfaced, no correctness/concurrency/security majors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review-until-clean loop — converged after 2 cyclesRan the maintainer-directed loop: 3 fresh sonnet reviewers per cycle over the full diff, rotating lenses, until a full cycle raises zero MAJOR findings. Cycle 1 — correctness/SQL-tracing, concurrency/pool/txn, test-adequacy
Cycle 1 majors (both fixed):
Cheap minors also fixed: pool-recovery follow-up assertion on the libsql size-1-pool test; explicit-directory-row exclusion tests for Cycle 2 — API-contract coherence, security/permission layering, maintainabilityAll three lenses: zero majors. Findings were all minor:
Zero majors in cycle 2 → converged. (Stopped at 2 of the allotted 3 cycles.) Final verification
Pushed as |
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add CAS-guarded delete_if_version to RootFilesystem and supported backends for only-if-unchanged filesystem deletes.
Stats: 2 fresh findings (from 5 raw, 2 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0. Duplicate-covered by current live threads: 2. Merged same-line overlap: 1.
Local Patterns
- Low libSQL CAS-delete diagnosis can report a read operation (
crates/ironclaw_filesystem/src/libsql.rs:1390, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql.rs:1390
delete_if_version_libsql_innerreusescurrent_version_libsqlfor the zero-row diagnosis, but that helper maps query/row errors asFilesystemOperation::ReadFile; the sibling Postgres CAS-delete path maps the same delete-side query failure asDelete.
Tests
- Low Postgres corrupt stored version path is untested (
crates/ironclaw_filesystem/src/postgres.rs:1695, confidence 75) — anchor:crates/ironclaw_filesystem/src/postgres.rs:1695
The new Postgres diagnosis convertslocked_versionthroughrecord_version_from_i64, but the contract tests only cover out-of-rangeexpected_versioninput, not a corrupt persisted row version.
Duplicate-Covered Raw Findings
- The ABA/generation-stability security finding is already covered by unresolved thread
PRRT_kwDORHZ7Z86PBheo. - The Postgres storm setup
.ok()?/silent-skip finding is already covered by unresolved CodeRabbit threadPRRT_kwDORHZ7Z86PEmVd.
| // 0 rows: absent row → NotFound (already gone, benign); row present | ||
| // at another version → VersionMismatch (gone stale). Distinct from | ||
| // put's diagnosis, which collapses absent into VersionMismatch. | ||
| if let Some(found) = current_version_libsql(conn, path).await? { |
There was a problem hiding this comment.
Low — libSQL CAS-delete diagnosis can report a read operation.
delete_if_version_libsql_inner reuses current_version_libsql for the zero-row diagnosis, but that helper maps query/row errors as FilesystemOperation::ReadFile. The sibling Postgres CAS-delete path maps the same delete-side query failure as FilesystemOperation::Delete, and the surrounding libSQL delete statements also use Delete, so a libSQL diagnosis failure from a delete call would surface with the wrong operation shape in errors and telemetry.
Fix: Either pass the caller operation into current_version_libsql or add a delete-specific current-version helper so the delete_if_version diagnosis maps backend errors with FilesystemOperation::Delete.
Also flagged by: tests/Low (libSQL corrupt stored version path is untested)
| return Err(FilesystemError::VersionMismatch { | ||
| path: path.clone(), | ||
| expected: Some(expected_version), | ||
| found: Some(record_version_from_i64(path, raw)?), |
There was a problem hiding this comment.
Low — Postgres corrupt stored version path is untested.
The new Postgres diagnosis converts locked_version through record_version_from_i64, but the contract tests only cover out-of-range expected_version input. No test corrupts the persisted row version and verifies delete_if_version returns CorruptRecordVersion from this new propagation path.
Fix: Add db_root_filesystem_contract::postgres_tests::postgres_delete_if_version_reports_corrupt_found_version covering a stale delete against a row whose stored version is negative.
* docs(reborn): canonical subagent thread-harness design; supersede WU-C durability spec Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): fix stale delivery-layer pointers, WebUI authz, roster race, safety-scan claims - Add supersession banners to phase-1/2/3 docs and tighten README pointers so implementers land on thread-harness-design.md for the delivery/ durability layer instead of the dead tombstone/reconciler/gate-store path. - thread-harness-design.md: state the children-endpoint authorization contract (owner-bound TurnScope resolution, no existence oracle) and add the cross-user not-found case to the required P5.2 test. - thread-harness-design.md §4.0: close the residual roster-prune race with two compensating self-heal checks (spawn-side and boot-side) and split the required test accordingly. - Correct README's "child output is safety-scanned" claims (§6 table, §8 item 7) — no SafetyLayer wiring exists on this ingress path today; describe the actual controls (framing/sanitisation + approval-bubbling) and point at the tracked platform-wide follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): fold 4 maintainer amendments into thread-harness design Gate resolution now accepted from any owner-authenticated surface (surfacing stays root-only, CAS'd single-winner makes two doors safe); abandon is mode-scoped (background parent-completion is normal delivery, not abandonment); new §6a covers human-priority reservation + interrupt/take-over with no content queue; capacity semantics made explicit (non-terminal cap counts gate-parked children deliberately, never release-on-park, subagent_extend re-claims at admission). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): round-2 review fixes for subagent thread-harness design Apply 7 review findings plus one import from PR #5749: header metadata shape, bounded boot-recovery concurrency + roster cardinality, a roster-before-edge-consistent rewrite of the residual delete/roster race (and its test), batched (not per-edge) settled-drain transcript writes, a bounded LIMIT-8 run-budget query in place of an open-ended walk, a lease (owner+expires_at, lazy expiry) on the human-priority reservation marker, in-path agent/project axis encoding for await-edge and roster paths (mount alone only partitions tenant/user), and the delete_if_version CAS contract in the filesystem contract doc. Thermo-reviewed twice: first pass caught an off-by-one in the budget query and a placeholder-spelling drift, both fixed and re-verified clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): round-3 review fixes for subagent thread-harness design Ten review findings: lazy-recovery admission rejection instead of inline recovery, two-trigger ThreadBusy-heal test, run-budget boundary tests, delete_if_version fail-closed TDD cases + ScopedFilesystem sketch + default-Unsupported body, reservation-release-before-delete ordering, and flattening the scope roster to one file per scope (the prior nested-directory encoding made a single global list_dir miss every scope not directly under the roster root). Plus three verdict items from a parallel review lane: delete_if_version narrows to (path, expected_version: RecordVersion) with no CasExpectation param, an ABA caller-invariant for recreated paths, and LiveSourceRoute gate-kind dispatch so approval/auth prompts in a shared-conversation tree page the owner's preference target instead of the shared channel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): round-4 review fixes + full-document thermo pass Three maintainer-flagged findings on the subagent thread-harness design: - Roster filename delimiter collision: validate_scope_id has no underscore restriction, so the flattened `__`-joined scope key could genuinely collide (e.g. tenant="a__b"/user="c" vs tenant="a"/ user="b__c" both produce the same prefix). Each component is now percent-encoded before the join, with a named collision regression test. - Close-sequence stale version: the 3-step edge close named the wrong version token for delete_if_version — using the terminal-state CAS's version instead of the reservation-release CAS's (the last write), which would fail VersionMismatch on every close. Fixed consistently across §2, §4.0, and §5.5. - Lazy recovery fan-out: per-scope lazy recovery tasks now share the same BOOT_RECOVERY_MAX_CONCURRENT_SCOPES limiter as the boot pass instead of fanning out unbounded; foreground admission still never blocks on it. Followed by a full-document (not diff-scoped) thermo-nuclear pass per maintainer request: fixed stale delete_if_version line citations drifted from the still-open shipped PR, added the missing MountScopedRootFilesystem/ SkillManagementRootFilesystem/HsmBackend wrappers to the passthrough list (verified against origin/feat/filesystem-delete-if-version), and added P3.3 (per-flavor model override) to the PR3 staging row it was missing from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): round-5 review fixes for subagent thread harness design Addresses 5 PR review findings on thread-harness-design.md: - Mark stale general/researcher flavor naming in sibling docs as historical, pointing to §10 (current: general/explorer/coder/planner). - Escalate the child->parent drain safety-scan gap to a hard PR2 prod-enable gate, with a small drain-scoped fallback scan if platform SafetyLayer ingress wiring hasn't landed yet. - Extend the boot-recovery limiter with a bounded pending queue and per-tenant in-flight cap, so one tenant's cold-boot burst can't starve others. - Shard the scope roster into 256 blake3-hash-prefix directories so boot enumeration is memory-bounded instead of one unbounded global list_dir; reject query()+Page (OFFSET-based, unsafe under concurrent mutation) and materialize-with-tripwire alternatives with rationale. - Close a roster-prune crash window by making spawn's post-edge self-heal an unconditional version-bumping touch instead of get-then-put-if-absent. Thermo-nuclear pass + two rounds of adversarial self-review (mechanism- correctness, implementability, internal-consistency) surfaced and fixed further issues: a double-release race in the reservation-release tri-state (§5.5) now closed by a bounded, edge-lifecycle-scoped idempotency key rather than the generic LRU-evictable idempotency cache; an invented "admission token" reference corrected to the real TurnRunId already in scope; stale "single global list_dir" phrasing left behind in three spots after the sharding change; and a stale line-count citation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): round-6/7 convergence fixes for subagent thread-harness design Three review-until-clean cycles (9 fresh reviewers) over the full doc set. Canonical design (thread-harness-design.md): - §5.5 round-7: released_children dedup entry pruned BEFORE delete_if_version (two records, no spanning txn — delete-then-prune orphaned entries on crash, breaking the boundedness claim); check-insert-decrement pinned as one atomic single-record CAS; stalled in-flight releaser named as accepted residual with a two-party invariant and a detection story; crash-injected tests - §8.3 new: derived System-wake streak cap (K=16, Human resets) closes the unbounded spawn→settle→wake cascade; subsumes never-built AutonomousContinuationBudget; honest liveness bound + thread-wide trade-off - §3: retires all three dead production_readiness sibling fields (tombstone, autonomous-budget :363, restart-reconciler :364) with verified citations - §4.5: roster prune no longer boot-only (close-path opportunistic prune); write-ordering superset base case pinned by test - §6/§7: subagent_activation_provenance field pulled forward to PR2 (P2.4) — fixes a staging inversion where PR2's cap read a PR4 field - §9.2/§6a: owner check named as surface-layer requirement with non-owner rejection tests; human_waiting reservation owner-gated - §8.2: trigger-2 Continue-only scope note; trigger-3 drains without activate - §10: required tests added for per-flavor budget/model overrides - citation fixes: llm_admin/llm_key_store path, row_store path, resume_turn_once :2616, over-release floor :1716-1720 Sibling docs: README/phase-1/2/3 superseded annotations completed for every RestartReconciler/AutonomousContinuationBudget/#4147 mention (6 previously un-annotated contradictions); phase banners gain point-in-time drift disclosures with verified shipped names; phase-1 banner scope widened to §3.2/§3.3/§3.6; contracts/filesystem.md sketch uses FilesystemOperation::Delete (no DeleteIfVersion variant exists); static-vs-dynamic diagram relabeled to await-edge files and regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): cycle-4 certification fixes for thread-harness design Cycle-4 fresh-reviewer pass (whole-system, mechanism re-walk, adversarial) found 1 major + 3 cheap minors: - MAJOR: $4.0 Consequences (b) still described the roster prune as boot-only, contradicting $4.5's round-7 close-path-prune ruling in the same document — now caller-agnostic with an explicit pointer. - $6a/$9.2: 'README $8.2' citations pointed at a nonexistent subsection; now 'README $8 item 2 (Approval ownership)'. - $8.3: young-thread under-K admit branch stated explicitly, mirroring $6's spelled-out rule instead of leaving it implied. - $13 cross-map $4.3 row: round-6 boot-pass-drop clause added for symmetry with the round-4/5 entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What / why
Adds the CAS-delete primitive required by the subagent await-edge delivery design (P1.0b, PR1 merge-gate):
putalready takes aCasExpectationprecondition butdeleteis blind, so no store can remove a record only-if-unchanged.RootFilesystem::delete_if_version(path, expected)is additive — default impl isUnsupported(same pattern asput), so the ~20 existing blind-delete call sites keep their behavior and no implementor breaks.Semantics (deliberately distinct from both blind
deleteandput's CAS diagnosis):OR-joined subtree band (mirrors thePUT_*_SQLidiom).NotFound(already gone, benign for idempotent cleanup); row present at another version →VersionMismatch{expected, found}(gone stale). This is new logic:put's CAS paths collapse the absent case intoVersionMismatch{found: None}.CasExpectation::Absentis meaningless for a delete and fails closed withUnsupported, decoded once by a sharedrecord_version_to_i64/required_delete_versionpath used by all three backends. That same shared conversion also narrowsRecordVersioninto the backend'si64column type: an out-of-rangeexpected_version(>i64::MAX) surfacesFilesystemError::CorruptRecordVersionbefore any statement runs, rather than a silently wrapped bind parameter that could never match a real row.putafter a prior delete, so anexpected_versioncaptured before a delete+recreate cycle can match a different incarnation of the same path.delete_if_versionis a sound standalone precondition only for paths that are never recreated; callers that do recreate paths must pair every successful delete with an unconditional postcondition recheck. Documented on the trait method itself.Implemented for every
RootFilesystem: in-memory, libSQL, Postgres,HsmBackend(delegates to its inner backend), and the composite/scoping layers that forward the unified entry plane —ScopedFilesystem,CompositeRootFilesystem,MountScopedRootFilesystem(ironclaw_host_runtime), andSkillManagementRootFilesystem(ironclaw_skills). Out of scope by design:LocalFilesystem(already rejectsVersionCAS forput) andStorageTxn(multi-key transactions are a separate atomicity story).Test evidence
crates/ironclaw_filesystemcargo test --all-features: 217 passed, 0 failed (110 lib + 8 catalog_contract + 75 db_root_filesystem_contract + 24 filesystem_contract).ironclaw_skillsandironclaw_host_runtime(own theSkillManagementRootFilesystem/MountScopedRootFilesystemforwarding wrappers): compile and test clean under this change (host_runtime's 3 pre-existing Docker-socket test failures are environmental — no Docker daemon in this sandbox — and unrelated to this diff).getis empty; stale version loses withVersionMismatch{Some(v1), Some(v2)}while the entry survives at the concurrent writer's bumped version; missing path →NotFound; the event log at the same path survives the CAS delete (single-key, where blind delete sweeps it); composite routesdelete_if_versionto the matched mount; out-of-rangeexpected_version(u64::MAX) surfacesCorruptRecordVersionon both libSQL and Postgres with the entry left untouched.BEGIN IMMEDIATE/COMMITtransaction on a single connection (mirrorsput's existing idiom, and keeps the call to one connection checkout). Postgres folds both into oneWITHstatement: alockedCTE takes aSELECT ... FOR UPDATErow lock, and thedeletedCTE depends on it (path IN (SELECT path FROM locked)) so Postgres sequences lock-then-delete instead of running them independently; the Postgres SQL pin test now also asserts theFOR UPDATElock and the CTE dependency are present.VersionMismatchflipped theNotFoundassertions in both the in-memory and libSQL tests; deleting despite a mismatch flipped the entry-survival assertion; bypassing the overflow guard (expected_version.get() as i64instead ofrecord_version_to_i64) flipped the new libSQL overflow test fromCorruptRecordVersionto a bogusVersionMismatch; droppingFOR UPDATEfrom the Postgres CTE flipped the SQL pin test.state.lock().awaitguard spanning the whole read-check-delete sequence — no change needed there.is_dir = FALSE, version-guarded, row-locked before the conditional delete.cargo fmt;cargo clippy --all --benches --tests --examples --all-features: zero warnings;cargo build --tests --all-featuresclean.🤖 Generated with Claude Code