feat(review): enumerate all invalid compact recovery edges in one pass (#1582)#1607
Conversation
Completes the change folder for feat-1582-inspect-authority so reviewers can trace every decision back to its source. The exploration.md maps the code surface (compact_store.go compactAuthorityLeaves at :474, validateCompactRecoveryEdge at :318, sentinel errors at :42 and :47). The proposal.md locks the four product decisions (load errors as diagnostics, joined combined anomaly class, valid edges as count only, per-edge scope only). The spec.md codifies R1-R9 with RFC 2119 keywords and S1-S11 in Given/When/Then form. The design.md fixes the architecture and exact path:line citations for the walker, CLI dispatch, and reuse of newReviewFlagSet / encodeReviewJSON. The verify-report.md captures focused test results, gate byte-identity check, JSON smoke evidence, and the documented size:exception.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds read-only compact recovery inspection with anomaly classification, deterministic diagnostics, cancellation handling, and JSON output through the new ChangesCompact authority inspection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@CHANGELOG.md`:
- Line 21: Add the required single trailing newline at the end of CHANGELOG.md,
without changing the existing changelog content.
- Line 14: Update the changelog wording for the edge-reporting command to state
that it emits all edges as JSON, with valid edges included and invalid edges
classified as anomalies. Preserve the existing sorting detail by successor
lineage and revision.
In `@internal/reviewtransaction/compact_inspect_test.go`:
- Around line 176-197: Add a sibling test to
TestInspectCompactAuthorityLoadError that constructs a valid successor whose
Recovery.PredecessorLineageID references a lineage absent from records, then
calls InspectCompactAuthority and asserts a missing_predecessor diagnostic is
emitted with a non-empty message. Keep the existing load_failure test unchanged
and verify the report’s diagnostic count and code.
In `@internal/reviewtransaction/compact_store.go`:
- Around line 592-616: Simplify compactInspectionAnomalyClass by removing the
repaired/provenance construction and duplicate validation calls; when unchanged
and successor.Recovery is non-nil, set authorization directly from whether
Recovery.MaintainerAuthorization differs from the exact binding and does not use
the recovery authorization schema prefix, preserving the existing switch
behavior.
- Around line 550-568: Update the inspection logic around the records-loading
loop and the second store loop to maintain a separate map keyed by each store’s
directory-derived lineageID for self-successor lookup, while retaining records
keyed by record.State.LineageID for predecessor references. Detect duplicate
content identities during loading, emit an appropriate diagnostic instead of
overwriting the existing record, and ensure stores with mismatched or missing
self identities are reported rather than silently skipped.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7baa628-64cc-4602-819d-0a0f63f21cc1
⛔ Files ignored due to path filters (7)
openspec/changes/feat-1582-inspect-authority/apply-progress.mdis excluded by!openspec/**openspec/changes/feat-1582-inspect-authority/design.mdis excluded by!openspec/**openspec/changes/feat-1582-inspect-authority/exploration.mdis excluded by!openspec/**openspec/changes/feat-1582-inspect-authority/proposal.mdis excluded by!openspec/**openspec/changes/feat-1582-inspect-authority/spec.mdis excluded by!openspec/**openspec/changes/feat-1582-inspect-authority/tasks.mdis excluded by!openspec/**openspec/changes/feat-1582-inspect-authority/verify-report.mdis excluded by!openspec/**
📒 Files selected for processing (8)
CHANGELOG.mdinternal/app/app_test.gointernal/cli/review_facade.gointernal/cli/review_inspect.gointernal/cli/review_inspect_test.gointernal/reviewtransaction/compact_inspect_test.gointernal/reviewtransaction/compact_store.gointernal/reviewtransaction/testdata/inspect_authority_determinism.golden
| - `gentle-ai review inspect-authority --cwd <repo>` (#1582): read-only | ||
| enumeration of every compact-v2 recovery edge. The walker classifies each | ||
| edge through the existing `validateCompactRecoveryEdge` source of truth, | ||
| emits invalid edges as JSON (sorted by successor lineage and revision) and |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document that valid edges are included too.
The command must report valid edges as well as invalid ones, but “emits invalid edges as JSON” can be read as an invalid-only output contract. Clarify that all edges are emitted and anomalies are classified within the report.
Proposed wording
- emits invalid edges as JSON (sorted by successor lineage and revision) and
+ emits all edges as JSON, classifying invalid edges (sorted by successor
+ lineage and revision), and📝 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.
| emits invalid edges as JSON (sorted by successor lineage and revision) and | |
| emits all edges as JSON, classifying invalid edges (sorted by successor | |
| lineage and revision), and |
🤖 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 `@CHANGELOG.md` at line 14, Update the changelog wording for the edge-reporting
command to state that it emits all edges as JSON, with valid edges included and
invalid edges classified as anomalies. Preserve the existing sorting detail by
successor lineage and revision.
| strings are `unchanged_target`, `malformed_recovery_authorization`, and the | ||
| joined `unchanged_target,malformed_recovery_authorization` for edges that | ||
| fail both sentinels. This prerequisite surface unblocks the batch | ||
| reconciliation plan tracked under #1452. No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required trailing newline.
markdownlint-cli2 reports that this file does not end with a single newline.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 21-21: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 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 `@CHANGELOG.md` at line 21, Add the required single trailing newline at the end
of CHANGELOG.md, without changing the existing changelog content.
Source: Linters/SAST tools
| func TestInspectCompactAuthorityLoadError(t *testing.T) { | ||
| repo := initSnapshotRepo(t) | ||
| validInspectRecoveryFixture(t, repo) | ||
| root, _, err := reviewAuthorityRoot(context.Background(), repo) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| bad := filepath.Join(root, "v2", "inspect-corrupt") | ||
| if err := os.MkdirAll(bad, 0o755); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(bad, "review-state.json"), []byte("not json\n"), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| report, err := InspectCompactAuthority(context.Background(), repo) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if report.Summary.TotalEdges != 1 || len(report.Diagnostics) != 1 || report.Diagnostics[0].Code != "load_failure" || report.Diagnostics[0].Message == "" { | ||
| t.Fatalf("load-error inspection = %#v", report) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No dedicated test for the missing_predecessor diagnostic.
TestInspectCompactAuthorityLoadError covers load_failure, but there's no sibling test asserting the missing_predecessor diagnostic (compact_store.go's other diagnostic code, emitted when a successor's predecessor record isn't in records). Given this is one of only two diagnostic codes the walker emits, and diagnostics drive the CLI's error-exit behavior, it's worth a dedicated regression test (e.g., write a successor whose Recovery.PredecessorLineageID points at a lineage with no corresponding store).
🤖 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 `@internal/reviewtransaction/compact_inspect_test.go` around lines 176 - 197,
Add a sibling test to TestInspectCompactAuthorityLoadError that constructs a
valid successor whose Recovery.PredecessorLineageID references a lineage absent
from records, then calls InspectCompactAuthority and asserts a
missing_predecessor diagnostic is emitted with a non-empty message. Keep the
existing load_failure test unchanged and verify the report’s diagnostic count
and code.
| records := make(map[string]CompactRecord, len(stores)) | ||
| for _, store := range stores { | ||
| record, loadErr := store.Load() | ||
| if loadErr != nil { | ||
| report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "load_failure", Path: store.Dir, Message: loadErr.Error()}) | ||
| continue | ||
| } | ||
| records[record.State.LineageID] = record | ||
| } | ||
| for _, store := range stores { | ||
| successor, ok := records[store.lineageID] | ||
| if !ok || successor.State.Recovery == nil { | ||
| continue | ||
| } | ||
| predecessor, ok := records[successor.State.Recovery.PredecessorLineageID] | ||
| if !ok { | ||
| report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "missing_predecessor", Path: store.Dir, Message: "recovery predecessor is unavailable"}) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Successor self-lookup can silently drop or misroute records under corrupted inventories.
records is populated keyed by the loaded record's own claimed identity (record.State.LineageID, line 557), but in the second loop each store looks itself up via records[store.lineageID] (line 560) — the directory-derived identity. This is safe only if store.lineageID == record.State.LineageID always holds. If a compact store's on-disk content has a lineage_id field that doesn't match its own directory name (a plausible corruption/tamper scenario, not just JSON syntax errors), this lookup silently misses (ok == false), and the store is dropped from the report entirely with no diagnostic — violating the stated goal of being "safe against corrupt inventories" and inspecting every edge. Worse, if the mismatched LineageID happens to collide with another store's real identity, records[record.State.LineageID] = record (line 557) silently overwrites that other store's entry, so a legitimate successor could be validated against the wrong data.
Keep records keyed by content identity for predecessor cross-references (line 564 needs that), but resolve each store's own successor via a store-identity-keyed map populated at load time, and diagnose duplicate content-identity collisions instead of silently overwriting.
🛡️ Proposed fix
records := make(map[string]CompactRecord, len(stores))
+ byStore := make(map[string]CompactRecord, len(stores))
for _, store := range stores {
record, loadErr := store.Load()
if loadErr != nil {
report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "load_failure", Path: store.Dir, Message: loadErr.Error()})
continue
}
- records[record.State.LineageID] = record
+ byStore[store.lineageID] = record
+ if _, dup := records[record.State.LineageID]; dup {
+ report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "duplicate_lineage_id", Path: store.Dir, Message: "lineage_id collides with another compact store"})
+ }
+ records[record.State.LineageID] = record
}
for _, store := range stores {
- successor, ok := records[store.lineageID]
+ successor, ok := byStore[store.lineageID]
if !ok || successor.State.Recovery == nil {
continue
}📝 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.
| records := make(map[string]CompactRecord, len(stores)) | |
| for _, store := range stores { | |
| record, loadErr := store.Load() | |
| if loadErr != nil { | |
| report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "load_failure", Path: store.Dir, Message: loadErr.Error()}) | |
| continue | |
| } | |
| records[record.State.LineageID] = record | |
| } | |
| for _, store := range stores { | |
| successor, ok := records[store.lineageID] | |
| if !ok || successor.State.Recovery == nil { | |
| continue | |
| } | |
| predecessor, ok := records[successor.State.Recovery.PredecessorLineageID] | |
| if !ok { | |
| report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "missing_predecessor", Path: store.Dir, Message: "recovery predecessor is unavailable"}) | |
| continue | |
| } | |
| records := make(map[string]CompactRecord, len(stores)) | |
| byStore := make(map[string]CompactRecord, len(stores)) | |
| for _, store := range stores { | |
| record, loadErr := store.Load() | |
| if loadErr != nil { | |
| report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "load_failure", Path: store.Dir, Message: loadErr.Error()}) | |
| continue | |
| } | |
| byStore[store.lineageID] = record | |
| if _, dup := records[record.State.LineageID]; dup { | |
| report.Diagnostics = append(report.Diagnostics, InspectionDiagnostic{Code: "duplicate_lineage_id", Path: store.Dir, Message: "lineage_id collides with another compact store"}) | |
| } | |
| records[record.State.LineageID] = record | |
| } | |
| for _, store := range stores { | |
| successor, ok := byStore[store.lineageID] | |
| if !ok || successor.State.Recovery == nil { | |
| continue | |
| } |
🤖 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 `@internal/reviewtransaction/compact_store.go` around lines 550 - 568, Update
the inspection logic around the records-loading loop and the second store loop
to maintain a separate map keyed by each store’s directory-derived lineageID for
self-successor lookup, while retaining records keyed by record.State.LineageID
for predecessor references. Detect duplicate content identities during loading,
emit an appropriate diagnostic instead of overwriting the existing record, and
ensure stores with mismatched or missing self identities are reported rather
than silently skipped.
Alan-TheGentleman
added
size:exception
c8d58ce
into
Gentleman-Programming:main
2 participants
🔗 Linked Issue
Closes #1582
This PR is the prerequisite for #1452 (atomic batch reconciliation). The contract decision recorded on #1452 — atomic batch over monotonic repair, with enumeration landing first — is what made #1582 the right shape. Without a read-only enumerator that exposes every invalid edge at once, the batch command on #1452 has nothing to declare against.
🏷️ PR Type
What kind of change does this PR introduce?
type:feature— New feature (non-breaking change that adds functionality)type:bug— Bug fix (non-breaking change that fixes an issue)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes behavior)📝 Summary
Adds
gentle-ai review inspect-authority --cwd <repo>, a read-only walker that enumerates every recovery edge in the compact-v2 authority inventory and reports every anomaly instead of returning on the first one. Output is JSON with stable top-level key order (summary,edges,diagnostics).validateCompactRecoveryEdge(the same source of truth used bycompactAuthorityLeaves) so the two surfaces can never disagree on what counts as invalid.errors.Isagainst the two existing sentinels (errCompactRecoveryTargetUnchanged,errCompactRecoveryAuthorizationInexact); when both fire on the same edge, the entry uses the joined class stringunchanged_target,malformed_recovery_authorization(the wire format introduced in fix(review): reconcile dual-anomaly recovery edge #1465).internal/reviewtransaction/compact_reconcile.go:221is byte-identical withorigin/main. Verified locally withgit diff origin/main -- internal/reviewtransaction/compact_reconcile.go→ empty.review status,review start, and negotiatedreview validatekeep their fail-closed semantics. Nothing reachable from those code paths was modified.This is the missing prerequisite for the batch reconciliation command that #1452 will add.
📂 Changes
internal/reviewtransaction/compact_store.goInspectCompactAuthoritywalker; reusesvalidateCompactRecoveryEdgeat line 318 and the two sentinels at lines 42 and 47.internal/reviewtransaction/compact_inspect_test.gointernal/reviewtransaction/testdata/inspect_authority_determinism.goldeninternal/cli/review_inspect.goRunReviewInspectAuthority. ReusesnewReviewFlagSetandencodeReviewJSON.internal/cli/review_inspect_test.go--cwdresolution + JSON envelope shape.internal/cli/review_facade.goRunReviewReconcileAuthorityat lines 320-321 and a one-line usage-list edit at line 229.internal/app/app_test.goCHANGELOG.mdopenspec/changes/feat-1582-inspect-authority/*Total diff: 1739 insertions / 2 deletions across 15 files.
🧪 Test Plan
Focused tests (the surface this PR adds)
go test -count=1 -v ./internal/reviewtransaction/... ./internal/cli/...All 18 new tests pass: 10 walker scenarios in
compact_inspect_test.go(S1–S11 mapped), 2 CLI scenarios inreview_inspect_test.go, plus 6 smoke captures behind//go:build smoke///go:build smokecapture(untracked, intentionally out of the diff).Gate integrity (the most important check)
Empty output. The strict gate is untouched.
Static checks
gofmt -l . go vet ./...Both clean.
Full repo (
go test ./...)go test -count=1 ./...shows 6 pre-existing failures ininternal/cli,internal/components/communitytool,internal/reviewtransaction(TestChainBundleImportRejectsTamperingTruncationAndWrongBindings),internal/sddstatus,internal/tui(TestPickerFlowSlice), andinternal/update/upgrade. All 6 reproduce onorigin/mainbefore this PR; none reference the new code paths. Listed verbatim under Notes for Reviewers so the reviewer has the baseline.go test ./...is green — fails on 6 pre-existing tests unrelated to this PR (see Notes)📏
size:exceptionRequestTotal diff is 1739 lines, ~1339 over the 400-line review budget. This PR explicitly requests maintainer-applied
size:exception.Breakdown:
review_facade.goplusapp_test.goare 6 lines combined. This 418-line slice is itself 18 lines over the documented 400-line code budget — the test file carries per-scenario coverage rather than shared helpers, chosen for clarity of failure messages over line count.openspec/changes/feat-1582-inspect-authority/. These are the team's planning artifacts (exploration, proposal, spec, design, tasks, apply-progress, verify-report) and follow the same convention as archived changes upstream (see commitf572288 chore(sdd): archive persona-canonical-channel change).The combined size is the cost of the team's "no inline decisions" SDD process, not feature scope creep. If maintainers prefer a smaller diff, the SDD artifacts can be moved out of the PR and the diff drops to 418 lines (still 18 over budget — the per-scenario test choice is the actual code-side decision worth confirming).
🤖 Automated Checks
size:exceptionrequested in NotesCloses #1582status:approvedstatus:approved(verified 2026-07-21)type:*Labeltype:feature(contributor lacks label permission)gofmt -lemptygo vet ./...cleango test ./...✅ Contributor Checklist
status:approved(feat(review): enumerate all invalid compact recovery edges in one pass #1582)size:exceptionrequested with rationale documented in this bodytype:featurelabel and, if accepted,size:exception(contributor lacks label permission)go test ./internal/reviewtransaction/... ./internal/cli/...)gofmt -lempty)go vet ./...clean)CHANGELOG.md)Co-Authored-Bytrailers💬 Notes for Reviewers
Review focus order, by blast radius:
internal/reviewtransaction/compact_store.go— the walker. Confirm it only reads, only classifies viaerrors.Is, never mutates.internal/cli/review_inspect.go— the CLI handler. Confirm it sits next toRunReviewReconcileAuthorityand reusesnewReviewFlagSet/encodeReviewJSON.internal/reviewtransaction/compact_inspect_test.go— the per-scenario tests. Confirm S1–S11 coverage maps back to the spec.internal/cli/review_facade.go(the 4-line diff) — confirm the registration touches the same sites the existing commands do.openspec/changes/feat-1582-inspect-authority/spec.md— the source of truth for what the walker is supposed to do.Pre-existing test failures (baseline; present on
origin/mainbefore this PR):internal/cli— timeout inTestReviewFacadeStartProvableShellAndModeRiskSelectsCanonical4Rinternal/components/communitytool—TestPiCodeGraphProbeClassifiesStalledMCPResponsesAsDeadlineExceededinternal/reviewtransaction—TestChainBundleImportRejectsTamperingTruncationAndWrongBindingsinternal/sddstatus— sddstatus drift testsinternal/tui—TestPickerFlowSlice(picker-flow drift)internal/update/upgrade—TestCheckSingleTool_EngramUsesBinaryReleaseChannel,TestInstallScriptBetaGoInstallBypassesPublicGoProxyNone of these reference the new code paths. They reproduce on
origin/mainwith no changes applied. If any reviewer sees one of these fail on this PR, it is a baseline failure, not a regression from this change.What this PR deliberately does NOT do (out of scope, per the contract recorded on #1452):
compactAuthorityLeaves(the fail-closed path)compact_reconcile.go:221Smoke test files behind
//go:build smoke///go:build smokecaptureare intentionally left untracked in this PR per the verification report's design (they document the smoke methodology for future contributors but don't run on defaultgo test).Generated by SDD orchestration (
feat-1582-inspect-authority). All planning artifacts are versioned underopenspec/changes/feat-1582-inspect-authority/for traceability.Summary by CodeRabbit
New Features
review inspect-authorityto generate a read-only JSON report of compact recovery edges and entry diagnostics.Bug Fixes
Tests