fix(advisory): add a recovery verb for schema 1 advisory choice stores - #2392
Conversation
An intent whose advisory choice store predates the schema 2 migration had no way forward. ADR-9 deliberately refuses to translate a schema 1 store — a `humanTurn`-only receipt has no meaning under the provenance union, and the safe answer is to ask the human again — so every reader falls closed to a hold. That refusal is right, but on its own it is terminal: `report` answers `advisory choice evidence is invalid: ...` and no human answer can clear it, because the store the answer would be written to is the unreadable one. `recover-schema-1` is the way out. It leaves parseStore untouched, salvages the pending advisories through the unchanged parsePending (pending has been schema 1 all along and did not migrate), discards the schema 1 receipts rather than translating them, and writes a schema 2 store. Discarding is the point rather than a cost: an advisory with no receipt is one the checkpoint asks again, which is the "ask the human again" the hold already stood for. It operates on one store — the active intent's, or the one --project-dir names — and refuses loudly, changing nothing, when the salvaged pending belongs to another intent, so a store reached through a stale intent cursor is never emptied by accident. The outcome names the receipts dropped, whether re-presentation is required, and the formal-check attempt counts that reset with the discarded run-now receipts. All logic lives in an exported seam that t470 drives in-process; the module dispatch arm is one call plus process.exit, spawn-verified by t470 and allowlisted for the patch gate. t458's schema 1 refusal pin is unchanged. Refs #2330
t470 spawns amadeus-advisory-choice.ts to prove the recover-schema-1 dispatch arm is really wired, which makes it a none->cli member of the mechanism honesty ratchet. The ledger is a deliberate manual pin: a new spawning test must not change the cli surface without a human edit, so the file is named here rather than the assertion relaxed. Refs #2330
The misdirection check read the intent run off the salvaged pending rows only, which left it vacuous in the one case where the entire content of the store is about to be discarded: a schema 1 store holding receipts and no pending. A stale intent cursor could therefore reach another intent's receipts-only store and empty it, which is the accident the check exists to prevent. The receipts carry an identity too, so the check now falls back to reading the intent run off them when no pending row carries it. This is a safety read, not a translation: nothing about what the receipt MEANT is interpreted — ADR-9's refusal stands — and a receipt too malformed to show an intent run yields nothing, so an unreadable receipt can only withhold a refusal, never grant a permission. The same-intent receipts-only path is unchanged: it still recovers to a schema 2 store with empty pending and reports the receipts it dropped. Refs #2330
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesschema 2 の advisory store に provenance union を追加しました。schema 1 store は自動変換せず拒否します。 Advisory store recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/reference/12-state-machine.md`:
- Around line 336-339: Update the command example fenced blocks to specify the
shell language by changing the opening fences to ```sh in
docs/reference/12-state-machine.md lines 336-339 and
docs/reference/12-state-machine.ja.md lines 295-298.
In `@packages/framework/core/tools/amadeus-advisory-choice.ts`:
- Around line 1479-1485: Update foreignReceiptIntentRuns in
packages/framework/core/tools/amadeus-advisory-choice.ts (1479-1485) to return
missing or invalid identity.intentRun values as rejection markers, ensuring
receipts-only recovery fails closed instead of deleting unverified receipts.
Update the recovery contract in
packages/framework/core/knowledge/amadeus-shared/audit-format.md (114-119),
docs/reference/12-state-machine.md (348-360), and
docs/reference/12-state-machine.ja.md (302-302) to state that stores with
unverifiable receipt ownership are rejected and not written. Extend
tests/integration/t470-advisory-store-recovery.integration.test.ts (185-206) to
verify a receipts-only store lacking identity.intentRun is rejected and its file
remains unchanged.
🪄 Autofix
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: 5b676006-824c-420c-b8db-ba3630074c07
📒 Files selected for processing (7)
docs/reference/12-state-machine.ja.mddocs/reference/12-state-machine.mdpackages/framework/core/knowledge/amadeus-shared/audit-format.mdpackages/framework/core/tools/amadeus-advisory-choice.tstests/.coverage-patch-allowlist.jsontests/integration/t-coverage-mechanism-ratchet.test.tstests/integration/t470-advisory-store-recovery.integration.test.ts
The patch gate found the two arms that guard the READ itself uncovered: the catch around JSON.parse and the shape check that follows it. Both are exercised by the spawned CLI, but bun --coverage does not attribute a child process to the parent report, so nothing measured them — and an arm nothing measures is an arm nothing protects. Both are now driven through the exported seam in-process, with the reason string asserted exactly, because the reason is the only thing that says which refusal actually ran: an identical "store is unreadable" message exists in readStore, and a test that merely asserts a refusal cannot tell the two apart. Refs #2330
The ownership check skipped any receipt whose identity it could not read. That looked conservative and was the opposite: on the receipts-only path there is no pending row to name the owner, so an unreadable receipt was deleted with its owner never established — a hole in the very fail-closed contract the check exists to hold. Silence is not evidence of belonging. Reading an owner now yields `undefined` for "the receipt does not say", and on the receipts-only path a single such receipt refuses the recovery instead of being passed over. The path with pending rows is untouched: there the pending identity already settles the owner. Also labels the recover-schema-1 fence as `sh` in both documentation faces. Refs #2330
|
レビュー2件へ対応しました(head Major(所有者未確認 receipt の fail-open): 指摘どおり実害でした — Minor(コードフェンス言語): 両面へ |
1 participant
概要
Refs #2330 / #2385(Bolt 2 — intent
260807-failclosed-recovery-path、scope self-fix)advisory choice store の schema 1→2 回復経路不在(pending 実在 intent の stage report 恒久ブロック)を、
parseStore無変更のまま回復 verb の追加で是正する(#2385 Q3 裁定 = #2318 の「schema 1 を翻訳しない」設計の保存)。変更
recover-schema-1verb 追加(amadeus-advisory-choice.ts+92): salvage は別関数で既存parsePendingを再利用(pending は schema 1 のまま)。schema 1 receipts は翻訳せず破棄(「ask the human again」の設計を保存)。出力に dropped 件数 / re-presentation 要否 / formal check attempt リセットを明示--project-dir明示、既定 = cwd 解決の active intent — ユーザー裁定)12-state-machine.md/.ja.md+audit-format.mdに schema 2 / provenance union の移行経路(feat(advisory): resolve pending advisory choices through the autonomy ladder (#2253 Bolt advisory-auto-resolution) #2318 時点からの既存 drift 解消を同乗)検証(§12a 2イテレーション READY)
edfee5818)NO_SILENT_DROP_OKedfee5818、競合ゼロ — merge-tree 非破壊プローブで事前確認)クローズ条件
着地後、schema 1 store を持つ実 intent での回復 → report ブロック解消の実測を経て #2330 をクローズする。
Note
Medium Risk
Recovery mutates authoritative advisory-choice evidence under the audit lock and intentionally discards legacy receipts, so wrong intent targeting or operator misuse could reopen advisories or reset formal-check attempt numbering; guards and loud refusal on foreign intent mitigate but the path is operational and security-adjacent for checkpoint holds.
Overview
Intents stuck on schema 1
.amadeus-advisory-choice.jsonstores can no longer be cleared by normal choices because readers fail closed on the old shape. This PR adds an explicit migration exit while keepingparseStoreand ADR-9’s “do not translate schema 1 receipts” rule unchanged.recover-schema-1inamadeus-advisory-choice.tsruns under the audit lock on a single store (--project-dir, default active intent). It reads schema 1 JSON directly (notparseStore), re-parses pending rows via existingparsePending, drops all legacy receipts (no provenance-union guessing), and writes schema 2 with empty receipts. Open advisories become unanswered again so checkpoints can re-prompt humans—the same outcome the invalid-store hold intended, but now reachable.Safety: recovery refuses without writing if the store’s intent run (from pending, or from receipt identities when pending is empty) does not match the active intent. Success JSON reports
receipts_dropped,re_presentation_required, andformal_check_attempts_reset(run-now receipt count only).Docs (
12-state-machine,audit-format) document schema 2, the provenance union, and this verb. t470 integration tests cover salvage, foreign-intent refusal, receipts-only stores, CLI/spawn wiring, and post-recoveryadvisoryReportHoldReasonbehavior; coverage allowlist notes the thinimport.meta.maindispatch arm.Reviewed by Cursor Bugbot for commit 82d5235. Configure here.
Summary by CodeRabbit
新機能
recover-schema-1コマンドを追加しました。改善