[rosary-44eec8] fix(store): backend guardrail parity — one BeadStore contract, both impls deliver it - #483
Merged
Conversation
…contract, both impls deliver it The two BeadStore impls each enforced a different, non-overlapping subset of guardrails (audit finding, scoped by rosary-46e7ff's 34-method parity matrix: 9 contract violations). This closes the gap set with backend-appropriate mechanisms, not a shared wrapper: - Transitions: new DoltClient::update_status_checked validates can_transition_to inside one transaction (SELECT ... FOR UPDATE) — the same guarantee SQLite's mutex-held read-check-write gives, via Dolt's native mechanism. A bare read-then-write would be a race that claims the guarantee without delivering it. The raw update_status stays as the verbatim primitive: set_status_verbatim continues to bypass BY DESIGN (bead_correct escape hatch, rosary-e0e19f), pinned by a regression test on both backends. - get_status on Dolt resolves short ids (the one method that didn't; its own close_bead always did) — unresolvable ids are Ok(None), matching SQLite's contract. - Secret scrubbing now fires on both backends for every free-text write: SQLite create title/description + comment add/update (previously Dolt-only — the DEFAULT backend wrote secrets verbatim into the store and the git-tracked JSONL), and update_bead_fields title/description on both (previously scrubbed nowhere). - close_bead stores canonical 'done' on both backends. Previously the alias 'closed': SQLite healed it on the next connect, Dolt never, so the Linear close sweep saw backend- and process-age-dependent result sets. Readers still tolerate legacy rows; 10 tests asserting the alias updated to canonical. - Dolt update_comment: read-then-conditional-write collapsed into one atomic statement (SET original_text = COALESCE(original_text, text), text = ? — MySQL evaluates SET left-to-right), closing the race where concurrent first edits corrupt the original_text audit trail. - SQLite update_bead_fields wrapped in a transaction — a mid-sequence error no longer commits a partial update. Deferred (own change, own migration sweep): Dolt readers silently defaulting malformed timestamps where the trait demands fail-loud — changing read-failure semantics can brick legacy stores. Tests (red first — 4 Dolt-half failures observed pre-implementation): 5 cross-backend parity tests drive identical sequences through SqliteBeadStore (pure) and DoltBeadStore (SandboxBeads, real dolt server, self-skips without the binary) — the first tests pinning update_status transition behavior on either backend. Evidence: ~/codebase-audits/agentic-research-rosary/3274bd9/contract-parity/ Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ
There was a problem hiding this comment.
🟡 Changes recommended
Dolt get_status can now silently mask real DB/SQL errors as “not found,” and the new Dolt-parity tests may still skip on CI, weakening the regression guarantee.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns the behavioral guardrails of the two BeadStore implementations (SQLite default backend and Dolt/MySQL-backed backend) so that the trait contract is enforced consistently across both, addressing audit-identified parity gaps.
Changes:
- Enforce transition validation on Dolt status updates (transactional,
SELECT … FOR UPDATE) while preserving theset_status_verbatimescape hatch. - Add secret scrubbing to SQLite write paths and to
update_bead_fieldson both backends; canonicalize terminal close state to"done". - Add cross-backend parity tests and update existing tests expecting
"closed"to now expect"done".
File summaries
| File | Description |
|---|---|
| tests/close_jsonl_sync.rs | Updates close sync test to expect canonical "done" status. |
| src/serve/handlers/tests.rs | Updates MCP close test to expect "done". |
| src/serve/handlers/input_validation_tests.rs | Updates forced close validation test to expect "done". |
| src/main.rs | Updates internal tests/expectations to use "done" instead of "closed". |
| src/dolt/tests.rs | Adds cross-backend parity tests for transitions, scrubbing, close canonicalization, and short-id status lookup. |
| src/dolt/query.rs | Updates Dolt get_status to resolve short IDs first. |
| src/dolt/deps.rs | Makes Dolt comment update atomic to preserve original_text under concurrency. |
| src/dolt/bead_crud.rs | Adds transactional, transition-validated status update; scrubs secrets in field updates; closes to "done". |
| src/bead_sqlite/tests.rs | Updates SQLite tests to expect "done" and documents canonical close behavior. |
| src/bead_sqlite/mod.rs | Adds secret scrubbing for create/comment/update paths; makes update_bead_fields transactional; closes to "done". |
| src/bead_move.rs | Updates move tests to expect "done" terminal status. |
| src/bead_dolt.rs | Routes Dolt update_status through the transition-validated Dolt client path. |
| .beads/beads.jsonl | Updates tracked bead records (including rosary-44eec8 / rosary-46e7ff status/metadata changes). |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Bead changes
Changed (3)
0 added · 3 changed · 0 removed |
…_id_opt, drop shadow Three Copilot findings verified valid and fixed: - update_status on BOTH backends now bails when the row vanishes between resolve and write, instead of committing a 0-row UPDATE that reports success. SQLite had the identical pre-existing hole; closed symmetrically to preserve the parity thesis. - Dolt get_status no longer masks real SQL/connection errors as Ok(None): new resolve_id_opt (shared IdResolution body with resolve_id, whose ambiguous-diagnostic error is preserved) maps only no-single-match to None and propagates genuine failures. - Dropped the confusing String->&str rebind in SQLite update_comment; the String binds via ToSql. Fourth finding (Dolt tests self-skip on CI without the dolt binary) acknowledged as the pre-existing repo-wide posture for ALL Dolt tests, not new to this PR — filed rosary-a95612 to install dolt in the CI verify job and add a skip-becomes-fail marker. Full suite 1720 green; parity suite green against sandboxed Dolt 1.86.1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ
There was a problem hiding this comment.
🟡 Changes recommended
New tests embed PAT-shaped ghp_... string literals that may trigger secret-scanning/DLP checks and block the PR despite being fake tokens.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/dolt/tests.rs:1217
- This BeadUpdate description includes a PAT-shaped
ghp_...literal and could trigger secret-scanning. If you generate the token at runtime, format the update description from that variable instead.
store
.update_bead_fields(
&id,
&BeadUpdate {
description: Some("upd ghp_0123456789abcdefghijABCDEFGHIJ0123456789".into()),
..Default::default()
},
)
.await
.unwrap();
src/dolt/tests.rs:1186
- This call also hardcodes a PAT-shaped
ghp_...literal, which may trip secret-scanning. If you switch to a runtime-generatedpatstring (see earlier in this test), build the comment body from that variable instead of embedding the token literal.
store
.add_comment(
&id,
"note ghp_0123456789abcdefghijABCDEFGHIJ0123456789",
"tester",
)
.await
.unwrap();
src/dolt/tests.rs:1201
- This update_comment test input hardcodes a PAT-shaped
ghp_...literal, which can trigger secret-scanning. Prefer generating the token at runtime and formatting the edit body from that variable.
let cid = comments.last().unwrap().id.clone();
store
.update_comment(
&cid,
"edit ghp_0123456789abcdefghijABCDEFGHIJ0123456789",
None,
)
.await
.unwrap();
- Files reviewed: 12/13 changed files
- Comments generated: 2
- Review effort level: Lite
Contributor
Bead changes
Added (1)
Changed (3)
1 added · 3 changed · 0 removed |
…shaped literal in source
Copilot round two, both valid: the parity-scrub test embedded a full
ghp_-shaped literal that could trip secret scanning/DLP, plus a
vestigial `let _ = leak;`. Fixture now constructed at runtime
(format!("ghp_{}", "A".repeat(36)) — the same pattern secrets.rs's own
unit tests use), every write and assertion keyed to the one runtime
token. Incidentally fixes two assertions the partial refactor had left
vacuous (asserting absence of a string never written).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ
There was a problem hiding this comment.
🟡 Changes recommended
The MCP tool_bead_close response still reports "status": "closed" while the store now persists canonical "done", leaving an externally observable inconsistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
Bead changes
Added (1)
Changed (3)
1 added · 3 changed · 0 removed |
…al status
Copilot round three, valid: the handler answered {"status": "closed"}
while close_bead now persists 'done' — API consumers observed a status
string the store never holds. Response now echoes the canonical form,
asserted in mcp_close_refreshes_only_the_published_jsonl_record.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently enforce the intended backend-parity contract (including canonical "done" close semantics) with tests updated/added to pin behavior, leaving only a minor doc-comment nit.
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
Bead changes
Added (1)
Changed (3)
1 added · 3 changed · 0 removed |
…tion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ
There was a problem hiding this comment.
🟡 Changes recommended
Both backends still query “closed linked beads” using status = 'closed', which will miss newly-closed beads now that close_bead persists done.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
Bead changes
Added (1)
Changed (3)
1 added · 3 changed · 0 removed |
jamestexas
added a commit
that referenced
this pull request
Sep 9, 2026
…ge train (#489) Six audit beads closed with merge evidence (44eec8/45504f/457927/45b069/ 46436d via MCP close, 46812e via the post-merge hook); rosary-b293a3 (MCP-only agents ADR design bead) published to the projection; three alias statuses canonicalized to 'done' by the freshly installed 9abbf26 binary's export. Claude-Session: https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The two
BeadStoreimpls each enforced a different, non-overlapping subset of guardrails — one advertised contract, two sets of guarantees. Found by the 2026-08-31 architecture audit; scoped precisely by rosary-46e7ff's 34-method parity matrix (9 contract violations, 20 identical, evidence:~/codebase-audits/agentic-research-rosary/3274bd9/contract-parity/):can_transition_to) fired only inSqliteBeadStore— Dolt accepted any transition, including out of terminaldone.beads.jsonl. The update path scrubbed on neither.get_statuson Dolt skipped short-id resolution (the one method that did — its ownclose_beadresolves), so known beads read asNone.close_beadstored the alias'closed'on both; SQLite healed it on the next connect, Dolt never — the Linear close sweep saw backend- and process-age-dependent result sets.update_comment's read-then-write could corrupt theoriginal_textaudit trail under concurrent first edits; SQLiteupdate_bead_fieldscould commit a partial update on mid-sequence error.Fix — same policy, backend-appropriate mechanisms
Not a shared wrapper. Each backend delivers the guarantee via its native mechanism (the skeptic-pass constraint from the audit: SQLite's mutex pattern copied onto a MySQL pool would be a race that claims the guarantee without delivering it):
update_status_checked— read-check-write inside one transaction withSELECT … FOR UPDATE, same error shape as SQLite. The rawupdate_statusremains the verbatim primitive:set_status_verbatimcontinues to bypass by design (thebead_correctescape hatch from incident rosary-e0e19f), pinned by a dedicated regression test on both backends.scrub_and_warnon every free-text write path on both backends (create title/description, comment add/update,update_bead_fieldstitle/description).'done'directly; readers still tolerate legacy'closed'rows. Ten tests asserting the alias updated.SET original_text = COALESCE(original_text, text), text = ?(MySQL evaluates SET left-to-right) — structurally race-free, no transaction needed.update_bead_fields: SQLite path wrapped in a transaction; Dolt gains scrubbing.Deferred deliberately (own change + migration sweep): Dolt readers silently defaulting malformed timestamps — flipping read-failure semantics can brick legacy stores.
Tests
TDD: 5 cross-backend parity tests written first (4 observed red on the Dolt halves against a real sandboxed Dolt 1.86.1 server before implementation). They drive identical sequences through both impls: invalid-transition rejection,
set_status_verbatimbypass survival, scrubbing across all four text paths, canonical close, short-idget_status. These are the first tests pinningupdate_statustransition behavior on either backend. Full suite: 1720 bin tests + integration suites green;task checkgreen.Closes rosary-44eec8. Depends-on rosary-46e7ff (closed — parity matrices).
🤖 Generated with Claude Code
https://claude.ai/code/session_01PMJgGCoDfQ7QF4R2vcupBJ