Decouple the Review Ledger and the Skills Digest From the Working Tree - #1328
Conversation
Two tracked artifacts were renderings rather than state, and both collided between concurrent branches. The burn-down carried global counts, so two branches each recording one pass merged silently to a count one short of the ledger's and the report gate failed the next unrelated pull request. The single skills digest was one hash over every skill's bytes, so any two concurrent skill edits conflicted on it. Stop tracking reports/canonical-review.md. `report` renders the burn-down from the JSON ledger to standard output, the validate action appends it to the run's job summary, and `report --check` goes with the file it checked. `record` holds the sibling engine's lock across its read, merge, and replace of the ledger, in the worktree's git directory so a crash leaves nothing in the tree. Replace .source-digest with one stamp per skill under .claude-plugin/fleet-skills/.source-digests/, kept out of the carried .github/skills/ tree, and check staleness per skill. Sweep every surface that named the tracked rendering, state once in scripts/README.md that a rendering is never committed, and add the cases that watch each gate: records in either order produce one ledger, two branches recording different units merge clean, a held lock refuses the record, a record writes only the ledger, and two skill edits move two disjoint stamps. Closes #1268, #1290, #1151. Relates #1240, #1295. Part of #1311, per #1313. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The local diff pass found the rendering rule stated on five surfaces where the issue asked for one, a comment claiming an atomic replace over a ledger written in place, the merge claim stated twice without its adjacency limit, the lock helper's docstring naming only the receipt it no longer guards alone, and the job-summary render step skipped after an earlier failure. Keep the rule in scripts/README.md and point the other surfaces at it, say the lock guards two interleaved records rather than a replace, state the merge property once with the case it does not cover, generalize the docstring, and restore `!cancelled()` on the render step. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two sentences this branch wrote still called the in-place ledger write a replace, attributing to it the atomicity the receipt has and the ledger lacks, and one still said the pre-push hook refuses a tree that differs from HEAD where it tests tracked content only, the wording the same branch had already corrected elsewhere. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 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 |
PR Summary by QodoDecouple Review Ledger and Skill Digests From the Working Tree
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently remove tracked renderings, introduce a worktree-local lock for ledger writes, and update CI/docs/tests to match without leaving residual references.
Pull request overview
This pull request removes two derived artifacts from being tracked as working-tree state to avoid cross-branch collisions: the canonical-review burn-down report and the single monolithic skills source digest. It also adds a lock around canonical_review.py record so concurrent records in the same worktree cannot interleave and drop entries.
Changes:
- Stop tracking
reports/canonical-review.md; render the burn-down fromreports/canonical-review.jsonto stdout and append it to the GitHub Actions job summary. - Add a worktree-local lock (in the worktree’s git dir) around canonical-review ledger updates.
- Replace
.claude-plugin/fleet-skills/.source-digestwith per-skill stamps under.claude-plugin/fleet-skills/.source-digests/, and update tests accordingly.
File summaries
| File | Description |
|---|---|
| scripts/tests/test_canonical_review.py | Adds coverage for ordering/merge/lock behaviors and updates report assertions to read stdout. |
| scripts/tests/test_build_dist.py | Updates tests for per-skill digest stamps and new stale/orphan cases. |
| scripts/README.md | Documents burn-down as a rendering (not committed) and per-skill digest stamp behavior. |
| scripts/local_review.py | Updates held_lock docstring and notes reuse by canonical review ledger locking. |
| scripts/canonical_review.py | Renders report to stdout, removes report file + --check, adds ledger lock in git dir. |
| scripts/build_dist.py | Implements per-skill digest stamps under .source-digests/ and per-skill stale checks. |
| reports/canonical-review.md | Removes the tracked burn-down rendering from the repository. |
| reports/canonical-review.json | Updates recorded pass entries for edited canonical units. |
| OPERATIONS.md | Removes the report --check invocation and documents report as stdout rendering. |
| GOVERNANCE.md | Updates Verification Discipline prose to reflect ledger-only tracked state. |
| AGENTS.md | Updates references to the burn-down as canonical_review.py report output rather than a tracked file. |
| .github/skills/local-strict-review/SKILL.md | Updates the carried-content pass instructions to pass --target to record and removes tracked-report references. |
| .github/actions/validate/action.yml | Appends canonical_review.py report output into $GITHUB_STEP_SUMMARY on every event. |
| .claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md | Mirrors the local-strict-review instruction updates in the plugin distribution. |
| .claude-plugin/fleet-skills/.source-digests/add-host-tool | Per-skill digest stamp file for add-host-tool. |
| .claude-plugin/fleet-skills/.source-digests/agent-conduct | Per-skill digest stamp file for agent-conduct. |
| .claude-plugin/fleet-skills/.source-digests/audit-a-repo | Per-skill digest stamp file for audit-a-repo. |
| .claude-plugin/fleet-skills/.source-digests/backlog-burndown | Per-skill digest stamp file for backlog-burndown. |
| .claude-plugin/fleet-skills/.source-digests/carried-instruction-file-guard | Per-skill digest stamp file for carried-instruction-file-guard. |
| .claude-plugin/fleet-skills/.source-digests/code-review | Per-skill digest stamp file for code-review. |
| .claude-plugin/fleet-skills/.source-digests/comment-and-doc-style | Per-skill digest stamp file for comment-and-doc-style. |
| .claude-plugin/fleet-skills/.source-digests/copilot-instructions-keeper | Per-skill digest stamp file for copilot-instructions-keeper. |
| .claude-plugin/fleet-skills/.source-digests/dotnet-codestyle | Per-skill digest stamp file for dotnet-codestyle. |
| .claude-plugin/fleet-skills/.source-digests/drive-pr | Per-skill digest stamp file for drive-pr. |
| .claude-plugin/fleet-skills/.source-digests/fleet-conformance-check | Per-skill digest stamp file for fleet-conformance-check. |
| .claude-plugin/fleet-skills/.source-digests/git-commit-conventions | Per-skill digest stamp file for git-commit-conventions. |
| .claude-plugin/fleet-skills/.source-digests/local-strict-review | Per-skill digest stamp file for local-strict-review. |
| .claude-plugin/fleet-skills/.source-digests/merge-and-release | Per-skill digest stamp file for merge-and-release. |
| .claude-plugin/fleet-skills/.source-digests/operational-vs-release-workflow | Per-skill digest stamp file for operational-vs-release-workflow. |
| .claude-plugin/fleet-skills/.source-digests/pr-review-conduct | Per-skill digest stamp file for pr-review-conduct. |
| .claude-plugin/fleet-skills/.source-digests/python-codestyle | Per-skill digest stamp file for python-codestyle. |
| .claude-plugin/fleet-skills/.source-digests/repo-worktree | Per-skill digest stamp file for repo-worktree. |
| .claude-plugin/fleet-skills/.source-digests/resync-a-repo | Per-skill digest stamp file for resync-a-repo. |
| .claude-plugin/fleet-skills/.source-digests/shell-codestyle | Per-skill digest stamp file for shell-codestyle. |
| .claude-plugin/fleet-skills/.source-digests/skill-lifecycle | Per-skill digest stamp file for skill-lifecycle. |
| .claude-plugin/fleet-skills/.source-digests/standup-a-repo | Per-skill digest stamp file for standup-a-repo. |
| .claude-plugin/fleet-skills/.source-digests/upstream-contribution-workflow | Per-skill digest stamp file for upstream-contribution-workflow. |
| .claude-plugin/fleet-skills/.source-digests/workflow-ci-contract | Per-skill digest stamp file for workflow-ci-contract. |
| .claude-plugin/fleet-skills/.source-digest | Removes the previous single digest stamp file. |
| .agents/skills/local-strict-review/SKILL.md | Updates the authored local-strict-review skill text consistent with the distributions. |
Review details
- Files reviewed: 40/40 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review round one found three comment blocks a line over the two-line ceiling, two README sentences narrating the replaced mechanism in the past tense, the ledger-and-receipt ordering restated where a pointer to GOVERNANCE.md belongs, and a test docstring carrying merge mechanics that belong in an inline comment beside the seeding they explain. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Answers to Qodo's comment-only list on head 4879792, five findings (5), each also answered in its own thread.
|
There was a problem hiding this comment.
🔵 Needs a closer look
scripts/build_dist.py’s has_exact_entries() currently accepts symlink entries, which can weaken the integrity guarantees of the new per-skill stamp checks.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/build_dist.py:103
has_exact_entries()currently treats symlinks as valid entries becausePath.is_file()/is_dir()follow symlinks. That allows a hand-edited digest stamp (or generated skill directory) to be replaced with a symlink to an external path withoutis_stale()necessarily flagging it, weakening the integrity check.
- Files reviewed: 40/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
Review round two found that has_exact_entries() accepted a symlink as a stamp or a generated skill directory, since is_file() and is_dir() follow one, so a stamp replaced by a link to a file outside the tree read as current whenever its target held the right digest. The source tree already refuses symlinks for the same reason, so the generated tree now does too, with a case that replaces a stamp by a symlink and watches the check fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Answer to the suppressed finding in Copilot's round on head c29c332, one finding (1):
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently remove tracked derived artifacts, add a tested lock around ledger writes, and update CI/docs/tests to match with no apparent regressions.
Review details
- Files reviewed: 40/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
Part of #1311, executing #1313 unchanged, per the re-assessment on #1311.
Two tracked artifacts were renderings rather than state, and both collided between concurrent branches. This decouples them from the working tree.
What changed
reports/canonical-review.mdis removed from the tree.canonical_review.py reportrenders the burn-down from the JSON ledger to standard output, and.github/actions/validate/action.ymlappends that rendering to the run's job summary on every event. Thereport --checkgate is gone with the file it checked, since a rendering that is never committed has no state to go stale. The JSON ledger stays tracked, sorted, one entry per unit.recordholds a lock across its read, merge, and replace of the ledger, reusinglocal_review.held_lock. The lock file lives in the worktree's own git directory rather than beside the ledger, so a record killed mid-write leaves nothing untracked inreports/..claude-plugin/fleet-skills/.source-digest, one hash over every skill's bytes, is replaced by.claude-plugin/fleet-skills/.source-digests/<name>, one file per skill, so two edits to two skills touch two files.is_stalecompares source, plugin, and.github/skills/per skill and refuses an orphaned, missing, or hand-edited stamp. The stamps stay under the plugin root, not under.github/skills/, whichspec/files.jsondeclares a verbatim tree carried whole.scripts/README.md(where the "a rendering is never committed" rule is now stated, once),OPERATIONS.md,GOVERNANCE.md"Verification Discipline" and the linter section,AGENTS.md"Where the Rules Live", andlocal-strict-review"The Carried-Content Pass", with the generated skill trees regenerated.Tests
test_canonical_review.py: two records applied in either order produce one ledger, two branches recording different units merge without a conflict, a held lock refuses the record rather than writing past it, the lock lives in the git directory, and a record writes the ledger and nothing else into the tree. The report cases now read the rendering from standard output.test_build_dist.py: two skill edits change two disjoint stamp files, and a hand-edited, missing, or orphaned stamp reports stale.Verification
Local gate set green from the worktree: ruff format and check, mypy, the whole self-test surface,
build_dist.py --check,canonical_review.py check(the three carried units this change moved were each read whole and recorded), prose lint,spec/validate.py,repo_gate.py, anddocker_lint.py.Residual
The JSON ledger is still one tracked file every canonical-content worker writes. It merges wherever the two passes' entries do not land on the same line, which is every case but two first-ever entries for units that sort adjacently with no entry between them. That is the shape #1313 asked to keep, and
backlog-burndown's no-shared-file rule still names no exception for it, which is that skill's own decision under #1323 rather than this change's. The ledger is written in place, as before, and one docstring clause inwrite_ledgerstill says "the read this replaces", meaning supersedes.Carried-content passes
The three carried units this change moved were each read whole twice, since the first read's fixes are themselves edits. Findings in text this branch wrote were fixed. Findings in text it did not write are listed here for the maintainer rather than fixed or filed, since each is a design call in a verbatim fleet-wide unit:
AGENTS.md"Where the Rules Live": themerge-and-releasesentence says Skills are refreshed "first", while that skill refreshes them at its step 7 after the dispatch. The.husky/pre-pushsentence describes enforcement a carrier cannot get, since the hook runs hub-hosted scripts from the repo top level. The Skills paragraph says the Skills are hub-local while.github/skills/is a carried verbatim tree. The lint row routes a carrier to a hub-only section where its ownOPERATIONS.md"Local Verification" holds its invocations.carried-instruction-file-guardis named nowhere in the file. "canonical content this repository authors and others carry" binds "this repository" to the carrier in a verbatim unit, where "the hub" is meant.GOVERNANCE.md"Verification Discipline": "byte for byte wherever the declared fidelity is verbatim" omits the EOL and action-pin normalizationspec/section-model.mdapplies. "The destination is declared fleet-wide" names no place the declaration lives. "or carries no such file" binds toOPERATIONS.mdrather than to the repository. "Two Profiles" is quoted where the skill's heading reads "Two profiles", declined as style consistent with that skill's own cross-references.local-strict-review"The Carried-Content Pass": the brief'sPaths:line never says what a unit key looks like, that(preamble)names the text before the first level-two heading, or where a section ends, so a subagent inheriting nothing cannot follow it literally. The cross-checkout sentence never says an engine is pointed by its working directory. "text the target has since changed" reads as since the fork point where the mechanism needs since the stale ref's tip..husky/pre-push, outside any unit: its dirty-tree remedy line ("commit what is being pushed, then run the pass and record it, then push") states the receipt's order only, and where the dirty content is the canonical ledger it points the reader the wrong way round.Closes #1268, #1290, #1151. Relates #1240, #1295.
🤖 Generated with Claude Code