Skip to content

Decouple the Review Ledger and the Skills Digest From the Working Tree - #1328

Merged
ptr727 merged 5 commits into
developfrom
feature/decouple-ledger-digest
Sep 4, 2026
Merged

Decouple the Review Ledger and the Skills Digest From the Working Tree#1328
ptr727 merged 5 commits into
developfrom
feature/decouple-ledger-digest

Conversation

@ptr727

@ptr727 ptr727 commented Sep 4, 2026

Copy link
Copy Markdown
Owner

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

  • The burn-down is no longer tracked. reports/canonical-review.md is removed from the tree. canonical_review.py report renders the burn-down from the JSON ledger to standard output, and .github/actions/validate/action.yml appends that rendering to the run's job summary on every event. The report --check gate 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.
  • record holds a lock across its read, merge, and replace of the ledger, reusing local_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 in reports/.
  • One digest stamp per skill. .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_stale compares 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/, which spec/files.json declares a verbatim tree carried whole.
  • Every surface naming the tracked rendering was swept: 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", and local-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.
  • Each new case was run against the develop engines first and failed there: the lock cases error, the report cases fail on the file the old engine still wrote, and every build_dist case errors on the missing per-skill directory.

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, and docker_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 in write_ledger still 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": the merge-and-release sentence says Skills are refreshed "first", while that skill refreshes them at its step 7 after the dispatch. The .husky/pre-push sentence 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 own OPERATIONS.md "Local Verification" holds its invocations. carried-instruction-file-guard is 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 normalization spec/section-model.md applies. "The destination is declared fleet-wide" names no place the declaration lives. "or carries no such file" binds to OPERATIONS.md rather 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's Paths: 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

ptr727 and others added 3 commits September 4, 2026 09:25
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>
Copilot AI lite review requested due to automatic review settings September 4, 2026 16:49
@qodo-code-review

Copy link
Copy Markdown

ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 65656dbe-3190-4e61-83b1-62bcd0823ad8


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Decouple Review Ledger and Skill Digests From the Working Tree

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Render the review burn-down on demand and publish it through CI job summaries.
• Lock ledger updates in Git metadata to prevent concurrent record loss.
• Split skill hashes into per-skill stamps to eliminate unrelated branch conflicts.
Diagram

graph TD
  A["Record Command"] --> B["Git-dir Lock"] --> C[("JSON Ledger")] --> D["Report Output"] --> E["CI Summary"]
  F["Skill Sources"] --> G["Build Dist"] --> H["Per-skill Stamps"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Publish the burn-down as a CI artifact
  • ➕ Preserves each generated report for later download
  • ➕ Keeps generated content outside the repository
  • ➖ Makes the report less visible than the job summary
  • ➖ Adds artifact upload and retention configuration
2. Store all skill hashes in one manifest
  • ➕ Uses fewer files
  • ➕ Allows additional digest metadata in one structured document
  • ➖ Retains a shared merge hotspot for unrelated skill edits
  • ➖ Requires custom merge handling to achieve the same branch isolation

Recommendation: Keep the PR's approach. Standard output plus the GitHub job summary makes the derived burn-down visible without tracking duplicate state, while separate stamp files align Git's merge granularity with independently edited skills. A CI artifact is useful only if long-term report retention becomes necessary.

Files changed (38) +291 / -164

Bug fix (2) +83 / -87
build_dist.pyGenerate and validate per-skill digest stamps +35/-19

Generate and validate per-skill digest stamps

• Replaces the global source digest with one digest file per skill. Staleness checks now reject missing, orphaned, malformed, hand-edited, or source-mismatched stamps independently.

scripts/build_dist.py

canonical_review.pyLock ledger updates and stream reports +48/-68

Lock ledger updates and stream reports

• Serializes ledger read-merge-write operations with a worktree Git-directory lock. Removes tracked report writing and report --check, rendering current Markdown coverage only to standard output.

scripts/canonical_review.py

Refactor (1) +5 / -4
local_review.pyGeneralize shared lock documentation +5/-4

Generalize shared lock documentation

• Clarifies that held_lock protects both local review receipts and canonical review ledger updates at the supplied lock path.

scripts/local_review.py

Tests (2) +135 / -28
test_build_dist.pyTest isolated skill stamps and stale detection +56/-10

Test isolated skill stamps and stale detection

• Updates existing digest tests for per-skill files and verifies disjoint edits affect disjoint stamps. Adds coverage for hand-edited, missing, and orphaned stamp detection.

scripts/tests/test_build_dist.py

test_canonical_review.pyTest ledger concurrency and ephemeral reports +79/-18

Test ledger concurrency and ephemeral reports

• Adds ordering, branch-merge, lock refusal, and Git-directory lock tests for recording. Reworks report tests to consume standard output and verifies recording writes only the ledger into the tree.

scripts/tests/test_canonical_review.py

Documentation (7) +27 / -27
SKILL.mdDocument target-aware canonical review recording +5/-5

Document target-aware canonical review recording

• Passes the target branch explicitly to both canonical review commands and clarifies that only the JSON ledger is tracked. Replaces references to the committed burn-down with on-demand report rendering.

.agents/skills/local-strict-review/SKILL.md

SKILL.mdRegenerate Claude local review skill +5/-5

Regenerate Claude local review skill

• Propagates target-aware recording and on-demand burn-down guidance into the generated Claude plugin skill.

.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md

SKILL.mdRegenerate GitHub local review skill +5/-5

Regenerate GitHub local review skill

• Propagates target-aware recording and on-demand burn-down guidance into the generated GitHub skill tree.

.github/skills/local-strict-review/SKILL.md

AGENTS.mdReference the ledger-backed report workflow +1/-1

Reference the ledger-backed report workflow

• Updates the rule index to describe the burn-down as a rendering generated from the tracked JSON ledger.

AGENTS.md

GOVERNANCE.mdDefine the ledger as the sole tracked review state +2/-2

Define the ledger as the sole tracked review state

• Revises verification and pre-push policy to remove the committed burn-down from recording requirements. Clarifies that canonical review recording writes one tracked ledger file.

GOVERNANCE.md

OPERATIONS.mdRemove the obsolete report freshness gate +1/-2

Remove the obsolete report freshness gate

• Drops report --check from the local gate set and documents standard-output rendering and CI job-summary publication.

OPERATIONS.md

README.mdDocument ephemeral reports and per-skill stamps +8/-7

Document ephemeral reports and per-skill stamps

• Defines the ledger as state and the burn-down as an uncommitted rendering. Documents Git-directory locking and independent digest files for each generated skill.

scripts/README.md

Other (26) +41 / -18
add-host-toolAdd add-host-tool source stamp +1/-0

Add add-host-tool source stamp

• Adds the generated per-skill digest for add-host-tool.

.claude-plugin/fleet-skills/.source-digests/add-host-tool

agent-conductAdd agent-conduct source stamp +1/-0

Add agent-conduct source stamp

• Adds the generated per-skill digest for agent-conduct.

.claude-plugin/fleet-skills/.source-digests/agent-conduct

audit-a-repoAdd audit-a-repo source stamp +1/-0

Add audit-a-repo source stamp

• Adds the generated per-skill digest for audit-a-repo.

.claude-plugin/fleet-skills/.source-digests/audit-a-repo

backlog-burndownAdd backlog-burndown source stamp +1/-0

Add backlog-burndown source stamp

• Adds the generated per-skill digest for backlog-burndown.

.claude-plugin/fleet-skills/.source-digests/backlog-burndown

carried-instruction-file-guardAdd carried-instruction guard source stamp +1/-0

Add carried-instruction guard source stamp

• Adds the generated per-skill digest for carried-instruction-file-guard.

.claude-plugin/fleet-skills/.source-digests/carried-instruction-file-guard

code-reviewAdd code-review source stamp +1/-0

Add code-review source stamp

• Adds the generated per-skill digest for code-review.

.claude-plugin/fleet-skills/.source-digests/code-review

comment-and-doc-styleAdd documentation-style source stamp +1/-0

Add documentation-style source stamp

• Adds the generated per-skill digest for comment-and-doc-style.

.claude-plugin/fleet-skills/.source-digests/comment-and-doc-style

copilot-instructions-keeperAdd Copilot keeper source stamp +1/-0

Add Copilot keeper source stamp

• Adds the generated per-skill digest for copilot-instructions-keeper.

.claude-plugin/fleet-skills/.source-digests/copilot-instructions-keeper

dotnet-codestyleAdd .NET codestyle source stamp +1/-0

Add .NET codestyle source stamp

• Adds the generated per-skill digest for dotnet-codestyle.

.claude-plugin/fleet-skills/.source-digests/dotnet-codestyle

drive-prAdd drive-pr source stamp +1/-0

Add drive-pr source stamp

• Adds the generated per-skill digest for drive-pr.

.claude-plugin/fleet-skills/.source-digests/drive-pr

fleet-conformance-checkAdd fleet conformance source stamp +1/-0

Add fleet conformance source stamp

• Adds the generated per-skill digest for fleet-conformance-check.

.claude-plugin/fleet-skills/.source-digests/fleet-conformance-check

git-commit-conventionsAdd Git conventions source stamp +1/-0

Add Git conventions source stamp

• Adds the generated per-skill digest for git-commit-conventions.

.claude-plugin/fleet-skills/.source-digests/git-commit-conventions

local-strict-reviewAdd local review source stamp +1/-0

Add local review source stamp

• Adds the generated per-skill digest for local-strict-review.

.claude-plugin/fleet-skills/.source-digests/local-strict-review

merge-and-releaseAdd merge-and-release source stamp +1/-0

Add merge-and-release source stamp

• Adds the generated per-skill digest for merge-and-release.

.claude-plugin/fleet-skills/.source-digests/merge-and-release

operational-vs-release-workflowAdd workflow model source stamp +1/-0

Add workflow model source stamp

• Adds the generated per-skill digest for operational-vs-release-workflow.

.claude-plugin/fleet-skills/.source-digests/operational-vs-release-workflow

pr-review-conductAdd PR review conduct source stamp +1/-0

Add PR review conduct source stamp

• Adds the generated per-skill digest for pr-review-conduct.

.claude-plugin/fleet-skills/.source-digests/pr-review-conduct

python-codestyleAdd Python codestyle source stamp +1/-0

Add Python codestyle source stamp

• Adds the generated per-skill digest for python-codestyle.

.claude-plugin/fleet-skills/.source-digests/python-codestyle

repo-worktreeAdd worktree source stamp +1/-0

Add worktree source stamp

• Adds the generated per-skill digest for repo-worktree.

.claude-plugin/fleet-skills/.source-digests/repo-worktree

resync-a-repoAdd repository resync source stamp +1/-0

Add repository resync source stamp

• Adds the generated per-skill digest for resync-a-repo.

.claude-plugin/fleet-skills/.source-digests/resync-a-repo

shell-codestyleAdd shell codestyle source stamp +1/-0

Add shell codestyle source stamp

• Adds the generated per-skill digest for shell-codestyle.

.claude-plugin/fleet-skills/.source-digests/shell-codestyle

skill-lifecycleAdd skill lifecycle source stamp +1/-0

Add skill lifecycle source stamp

• Adds the generated per-skill digest for skill-lifecycle.

.claude-plugin/fleet-skills/.source-digests/skill-lifecycle

standup-a-repoAdd repository standup source stamp +1/-0

Add repository standup source stamp

• Adds the generated per-skill digest for standup-a-repo.

.claude-plugin/fleet-skills/.source-digests/standup-a-repo

upstream-contribution-workflowAdd upstream workflow source stamp +1/-0

Add upstream workflow source stamp

• Adds the generated per-skill digest for upstream-contribution-workflow.

.claude-plugin/fleet-skills/.source-digests/upstream-contribution-workflow

workflow-ci-contractAdd CI contract source stamp +1/-0

Add CI contract source stamp

• Adds the generated per-skill digest for workflow-ci-contract.

.claude-plugin/fleet-skills/.source-digests/workflow-ci-contract

action.ymlPublish burn-down to the Actions summary +5/-6

Publish burn-down to the Actions summary

• Replaces the tracked-report freshness gate with an informational report command appended to GITHUB_STEP_SUMMARY. The rendering still runs after earlier failures unless the workflow is cancelled.

.github/actions/validate/action.yml

canonical-review.jsonRefresh canonical review ledger entries +12/-12

Refresh canonical review ledger entries

• Updates digests, findings, provenance commits, and timestamps for the canonical documentation units changed by this PR.

reports/canonical-review.json

@qodo-code-review

qodo-code-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. README uses change framing ✓ Resolved 📜 Skill insight ✧ Quality
Description
The documentation explains the current design through the former rendering and digest mechanisms
using past-tense before-and-after framing. Project documentation must state current behavior
directly rather than narrating what the replaced implementation did.
Code

scripts/README.md[253]

+**The ledger is state, and the burn-down is a rendering of it. A rendering is never committed.** `report` writes the burn-down to standard output, and the hub's [`.github/actions/validate`][validate-hook] hook writes that same rendering to the run's job summary on every event, so nothing writes it into the tree. The distinction is what lets concurrent branches merge: a ledger sorted by unit key puts two units' passes at two places, which git merges unless both are first-ever entries with no entry between them, where a rendering carrying global counts changed the same line to the same value on both sides, merged silently to a count one short of the ledger's, and a gate over that committed file then failed the next unrelated pull request ([ptr727/ProjectTemplate#1268][ledger-merge-issue], [#1290][burndown-claim-issue]).
Relevance

●●● Strong

Recent precedent accepts rewriting documentation and docstrings away from historical change framing.

PR-#1068
PR-#1179

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The rule prohibits past-tense change framing in documentation. Line 253 narrates how the committed
rendering previously changed, merged, and failed, while line 273 contrasts per-skill stamps with the
former single stamp that conflicted.

scripts/README.md[253-253]
scripts/README.md[273-273]
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The README uses past-tense change framing to contrast the current design with replaced mechanisms.

## Issue Context
PR Compliance ID 2826805 permits before-and-after framing in changelogs, commit messages, and PR descriptions, but not ordinary documentation.

## Fix Focus Areas
- scripts/README.md[253-253]
- scripts/README.md[273-273]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Reserved skill name crashes build ✗ Dismissed 🐞 Bug ☼ Reliability
Description
A source skill named .source-digests is accepted by skill_names(), but regeneration then tries
to write that skill's stamp to the .source-digests directory itself and raises
IsADirectoryError. This prevents generating distributions for that otherwise discoverable skill
and the non-check CLI path does not catch the resulting OSError.
Code

scripts/build_dist.py[R158-161]

+    DIGEST_DIR.mkdir(parents=True, exist_ok=True)
+    for name in names:
+        # LF, same as write_plugin_manifest, explicit for the same Windows-platform-default reason.
+        (DIGEST_DIR / name).write_text(skill_digest(name) + "\n", encoding="utf-8", newline="\n")
Relevance

●●● Strong

This is a concrete crash for a discoverable input and a straightforward robustness defect.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Skill discovery has no naming restriction beyond the presence of SKILL.md, while the new digest
directory has the fixed name .source-digests. For that source skill name, DIGEST_DIR / name
resolves to the already-created digest directory; writing text to it raises IsADirectoryError. The
normal regeneration path catches only ValueError, not this OSError.

scripts/build_dist.py[40-44]
scripts/build_dist.py[36-36]
scripts/build_dist.py[158-161]
scripts/build_dist.py[236-240]
.agents/skills/README.md[3-7]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`build_dist.py` discovers any source child directory containing `SKILL.md`, including `.source-digests`. The new stamp path uses that same name as a directory, so writing the stamp for that skill targets a directory and crashes.

## Issue Context
Either reject reserved/internal skill names during discovery with a clear `ValueError`, or use a collision-free encoding/layout for stamp filenames. Add a regression test creating `.agents/skills/.source-digests/SKILL.md`.

## Fix Focus Areas
- scripts/build_dist.py[33-44]
- scripts/build_dist.py[158-161]
- scripts/tests/test_build_dist.py[48-51]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Comments exceed two lines ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
The PR adds three-line explanatory comment blocks even though comments must be one line by default
and at most two lines for genuine constraints. These blocks should be condensed or moved into
appropriate documentation.
Code

scripts/build_dist.py[R33-35]

+# One digest file per skill, named for the skill, rather than one stamp over every skill's bytes.
+# Two branches editing two skills then touch two files and merge, where the single stamp conflicted on every concurrent skill edit (ptr727/ProjectTemplate#1240).
+# Under the plugin root and not under .github/skills/, which spec/files.json declares a verbatim tree carried whole to every fleet repository, so a stamp there would be carried too.
Relevance

●●● Strong

Recent precedent accepts condensing explanatory comment blocks beyond the repository’s two-line
limit.

PR-#982
PR-#1068

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The cited rule limits comments to one line by default and two lines only for genuine constraints.
Each cited region introduces a three-line prose comment block.

scripts/build_dist.py[33-35]
scripts/canonical_review.py[83-85]
.github/actions/validate/action.yml[68-70]
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Several added explanatory comments exceed the permitted two-line maximum.

## Issue Context
PR Compliance ID 2826677 requires comments to use one line by default, with a second line only for a genuine constraint.

## Fix Focus Areas
- scripts/build_dist.py[33-35]
- scripts/canonical_review.py[83-85]
- .github/actions/validate/action.yml[68-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (1)
4. Ledger rule is duplicated ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
scripts/README.md restates the cross-cutting commit-order contract defined in GOVERNANCE.md
instead of referencing the canonical rule. Maintaining the obligation in both places risks policy
drift.
Code

scripts/README.md[265]

+`record` holds a lock across its read, merge, and write of the ledger, the lock `local_review.py` guards its receipt with, since two overlapping records would otherwise each read a ledger without the other's pass and the second write would drop one ([#1151][record-lock-issue]). The lock lives in the worktree's own git directory rather than beside the ledger, so a record killed mid-write leaves nothing in `reports/` for an add to stage. The ledger is tracked, so it has to be committed before the push, [`.husky/pre-push`][pre-push] refusing tracked content that differs from HEAD before it runs either gate. The receipt above is not tracked and is recorded after the last commit instead, so the two records sit on opposite sides of it, and recording this one first, then committing it with the change, is the shortest order that satisfies both. `record` binds the digest to the read for `--expect-digest`'s reason above: recording a unit by name alone would stamp whatever the file holds at record time, so an edit between the review and the record would be attested to by a reviewer who never saw it. It refuses an unknown unit, a digest the content has moved past, and a reviewer outside `local_review.py`'s own backend vocabulary, since two spellings of one reviewer make the two records impossible to read together. A pass is recorded whatever it found, including nothing, for the same reason a receipt is: the record says a review ran over exactly this text, never that the text is clean.
Relevance

●● Moderate

The duplication concern is plausible, but no closely matching accepted or rejected precedent
appeared.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
GOVERNANCE.md line 185 establishes that the tracked ledger is committed before pushing while the
untracked receipt is recorded after the final commit. The modified scripts documentation repeats the
same tracked-tree condition, ordering, and opposite-side-of-commit obligation rather than merely
linking to that governance section.

Rule 2826346: Do not duplicate cross-cutting rules from AGENTS.md and GOVERNANCE.md in other repository files
scripts/README.md[265-265]
GOVERNANCE.md[185-185]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The scripts documentation duplicates the ledger and receipt commit-order rule maintained in GOVERNANCE.md.

## Issue Context
PR Compliance ID 2826346 requires repository files outside AGENTS.md and GOVERNANCE.md to reference cross-cutting rules without restating their substantive conditions or obligations.

## Fix Focus Areas
- scripts/README.md[265-265]
- GOVERNANCE.md[185-185]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

5. Test docstring exposes internals ✓ Resolved 📜 Skill insight ✧ Quality
Description
The test docstring describes ledger sorting and Git's unchanged-line merge mechanics rather than the
test's observable behavior contract. Such implementation rationale makes documentation unnecessarily
sensitive to refactoring.
Code

scripts/tests/test_canonical_review.py[R620-623]

+    def test_two_branches_recording_different_units_merge_without_conflict(self) -> None:
+        """The acceptance case: each branch records a pass over a different unit, and git merges
+        the two ledgers without a conflict. The base ledger holds entries that sort between the
+        two, since git merges two insertions only where unchanged lines separate them."""
Relevance

●●● Strong

Recent precedent accepts reframing test docstrings around observable behavior instead of
implementation mechanics.

PR-#1179
PR-#1068

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The rule requires behavior-focused docstrings. The cited docstring discusses entries sorting between
insertions and unchanged lines separating them, which are implementation details rather than the
contract that independently recorded units merge without conflict.

scripts/tests/test_canonical_review.py[620-623]
Skill: python-codestyle

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test docstring documents internal ledger layout and Git merge mechanics instead of the behavior being verified.

## Issue Context
PR Compliance ID 2827096 requires docstrings to focus on behavior callers or tests can rely on, leaving implementation rationale to inline comments where necessary.

## Fix Focus Areas
- scripts/tests/test_canonical_review.py[620-623]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 86 rules
✅ Skills: 5 invoked
  comment-and-doc-style
  dotnet-codestyle
  python-codestyle
  shell-codestyle
  workflow-ci-contract
Review mode: 🧠 Deep: This is a bug-dense, cross-cutting behavioral change spanning ledger locking and merge semantics, report/CI behavior, per-skill digest validation, generated artifacts, and extensive documentation/tests, with many independent paths where redundant review can catch subtle defects.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread scripts/build_dist.py Outdated
Comment thread scripts/README.md Outdated
Comment thread scripts/README.md Outdated
Comment thread scripts/tests/test_canonical_review.py Outdated
Comment thread scripts/build_dist.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 from reports/canonical-review.json to 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-digest with 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>
Copilot AI review requested due to automatic review settings September 4, 2026 17:07
@ptr727

ptr727 commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Answers to Qodo's comment-only list on head 4879792, five findings (5), each also answered in its own thread.

  1. "Comments exceed two lines", scripts/build_dist.py, scripts/canonical_review.py, .github/actions/validate/action.yml. Fixed in c29c332: each block is two lines, one sentence per line.
  2. "README uses change framing", scripts/README.md. Fixed in c29c332: both sentences state the current behaviour in the present tense.
  3. "Reserved skill name crashes build", scripts/build_dist.py. Disproven: the stamp for a skill named .source-digests is the file DIGEST_DIR / ".source-digests", a child of the stamp directory, so nothing writes to a directory. Reproduced against HEAD with a temporary tree holding foo/SKILL.md and .source-digests/SKILL.md: regenerate() returns both names, is_stale() reports current, and the directory holds one stamp file per skill. Evidence in the thread.
  4. "Ledger rule is duplicated", scripts/README.md. Fixed in c29c332: the restated commit order is a pointer to GOVERNANCE.md "Verification Discipline" and the local-strict-review Skill's "The Carried-Content Pass".
  5. "Test docstring exposes internals", scripts/tests/test_canonical_review.py. Fixed in c29c332: the docstring states the behaviour and the rationale sits in an inline comment beside the seeding it explains.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 because Path.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 without is_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>
Copilot AI review requested due to automatic review settings September 4, 2026 17:16
@ptr727

ptr727 commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Answer to the suppressed finding in Copilot's round on head c29c332, one finding (1):

scripts/build_dist.py:103, "has_exact_entries() currently treats symlinks as valid entries because Path.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 without is_stale() necessarily flagging it." Fixed in 3f38a9d: an entry that is a symlink now fails the shape check outright, matching the refusal the source tree already applies, and test_a_stamp_replaced_by_a_symlink_reports_stale replaces a stamp by a link to a file holding the right digest and watches is_stale() report stale. The generated skill directory case was already refused by reject_symlinks inside tree_digest, and now fails the shape check first as well.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@ptr727
ptr727 merged commit d857170 into develop Sep 4, 2026
9 checks passed
@ptr727
ptr727 deleted the feature/decouple-ledger-digest branch September 4, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants