Advisory ledger-reference ratchet lint (#3328)

[jwbron]

Closes #3328. Follow-up to #3288 (durability guardrail deferred by HITL cq-2).

What

#3288 de-ledgered the docs corpus and reframed the documenter to emit snapshots, but — by explicit operator decision (cq-2 = "out of scope, note as follow-up") — did not add an automated guard to keep slice-N / TASK-N / cq-N ledger references from creeping back in. This adds that deferred guard as an advisory (warn, never block) CI ratchet.

How

New custom check scripts/check-ledger-references.py, auto-discovered by make lint-custom (any scripts/check-*.py):

• Detects the three unambiguous SDLC ledger token classes — slice-N, TASK-N, cq-N — across repo markdown plus non-test Python source (docstrings/comments).
• Ratchet semantics via scripts/ledger-references-baseline.yaml (per-file counts): flags only net-new tokens, so the long pre-existing tail (tracked separately, decaying as docs are touched) does not fire on every run. Lowering a count never warns. Re-snapshot with --update-baseline.
• Advisory by default: prints warnings and exits 0 so it never blocks a PR. --strict makes net-new a hard failure (intentionally not wired into CI yet — per the issue's "advisory first, tune the allowlist before a hard gate").

False-positive controls (the reason the guard was deferred)

The issue calls out live-runtime vocabulary that shares these tokens. Handled by:

• Hyphenated patterns skip live identifiers: slice_id, contract.slices, EGG_*_SLICES, "the slice DAG" all use underscores / dotted access / bare words and don't match.
• Structural exclusions: docs/templates/ (plan.md's TASK-N is the live plan format), .egg-state/ (pipeline state + BRC transcripts are a ledger by design), and test files (fixtures use slice-N / task-N-N as live data).
• Per-line escape hatch: ledger-ok in a comment on the line excludes it (e.g. a legitimate new slice-DAG error message).

Deliberately out of scope for v1

Change-log prose ("what was removed", "used to … now …") — it can't be regex-matched without high false positives against the issue links that legitimately justify why the current system is shaped the way it is (which #3288 explicitly keeps). Noted in the script docstring as a tuning follow-up.

Tests

tests/scripts/test_check_ledger_references.py (30 tests): pattern match/non-match, occurrence counting, suppress marker, exclusions, ratchet evaluate (over/at/under baseline, new file), baseline round-trip, and a guard that the committed baseline is clean against the live corpus (so a fresh checkout produces no spurious advisory noise).

• make lint-custom → ledger-references runs clean.
• ruff check / ruff format --check / mypy clean on new files.

[james-in-a-box]

Autofixer: Unit Tests

Fixed the in-scope failure and identified two pre-existing failures that are not caused by this PR.

Fixed — test_no_net_new_against_committed_baseline

CI tests the pull/3333/merge corpus (this branch merged with main). Merging main pulls in #3332's session-store files (orchestrator/session_state_store.py, orchestrator/routes/session_state.py, sandbox/egg_lib/session_state_sync.py) plus edits to concurrent_executor.py / consensus_wrapper.py, which introduce SDLC ledger tokens (slice-N / TASK-N / cq-N) absent from the committed baseline. The advisory ratchet correctly flagged them as net-new.

Fix: merged main and regenerated the baseline via scripts/check-ledger-references.py --update-baseline (commit d31cefb). The ledger baseline test now passes locally.

Report-only — two docs tests fail on main, unrelated to this PR

tests/docs/test_context_pr_doc_terminology.py::TestReferenceOrchestratorCliContextFields::test_cross_references_issue_2548
tests/docs/test_context_pr_doc_terminology.py::TestConcurrentExecutionContextPrSection::test_slice_1_paragraph_ties_to_context_branch

This PR only adds three files (scripts/check-ledger-references.py, scripts/ledger-references-baseline.yaml, tests/scripts/test_check_ledger_references.py) — none touch docs. These two tests assert content of docs/reference/orchestrator-cli.md and docs/guides/concurrent-execution.md:

• orchestrator-cli.md must contain #2548 — current main has no 2548 reference at all.
• concurrent-execution.md must place the egg/<id>/context literal in the same paragraph as a slice-1 mention — current main does not.

Both reproduce against the pull/3333/merge tree, whose docs are byte-identical to main, so they are red on main independently of this change. They look like an unlanded slice-1 documentation update for #2548.

These require editing unrelated docs to satisfy #2548's documentation acceptance criteria — out of scope for the ledger-ratchet PR and a content decision I shouldn't guess at. They will keep the Unit Tests check red until main's docs are brought into line with the #2548 tests (separately).

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 3}

[jwbron]

Pushed e5ceb22c8 to re-scope this PR and unblock CI.

What changed

• Reverted the bundled Slice PRs are missing analysis/plan docs and all BRC history; need a 'context' PR for refine+plan phases plus per-slice BRC in each slice PR #2548 doc edits (concurrent-execution.md, orchestrator-cli.md). Those two doc-test failures (test_cross_references_issue_2548, test_slice_1_paragraph_ties_to_context_branch) are red on main independently of this PR and are owned by the separate, green docs: clarify slice-1 context-PR base and restore #2548 cross-ref #3325. Keeping the edits here duplicated docs: clarify slice-1 context-PR base and restore #2548 cross-ref #3325 and would conflict on whichever lands second.
• Refreshed the baseline so the ratchet's own self-test (test_no_net_new_against_committed_baseline) stays green in both merge orderings: concurrent-execution.md goes 15→16, matching docs: clarify slice-1 context-PR base and restore #2548 cross-ref #3325's single added slice-1 token. With the baseline at the post-docs: clarify slice-1 context-PR base and restore #2548 cross-ref #3325 count and this PR's tree doc-free, net-new is empty whether or not docs: clarify slice-1 context-PR base and restore #2548 cross-ref #3325 has landed.

This PR is now scoped to its three ledger files (scripts/check-ledger-references.py, scripts/ledger-references-baseline.yaml, tests/scripts/test_check_ledger_references.py).

Merge order: land #3325 first (it clears the #2548 doc tests on main), then this PR's merge tree goes fully green.

Caveat (design): test_no_net_new_against_committed_baseline checks the committed baseline against the live merge corpus, so it will re-flag whenever a later PR brings new slice-N/TASK-N/cq-N tokens from main — a maintenance treadmill inside the blocking unit suite, somewhat at odds with the issue's "advisory, never block" intent. Worth considering moving that self-test out of the blocking tier (keep it as a --update-baseline reminder in the advisory script).

[egg-reviewer]

Review: Advisory ledger-reference ratchet lint (#3328)

I traced the full data flow (iter_scanned_files → scan_file → evaluate → _print_net_new / baseline I/O), the make lint-custom auto-discovery path, the make test-all test wiring, and ran the check + the 30-test suite locally (both green; --strict clean against the committed baseline). The implementation is solid, the detector is genuinely well-tested, and the feature works end-to-end. No blocking issues. Approving with several non-blocking observations below.

Verified working

• make lint-custom invokes $(PYTHON) scripts/check-*.py with no args (Makefile:315), so the script runs in advisory mode and exits 0 — confirmed it never blocks the lint target, as the PR claims.
• The slice-N / TASK-N / cq-N detection is independently verified by TestPattern, TestScanFile, and TestEvaluate with hand-authored expected counts (e.g. test_counts_occurrences_not_lines asserts count == 3). I specifically checked whether TestRepoBaselineIsClean is a self-seeding golden — it is not a blocking concern: the baseline is impl-generated, but the detector's correctness is pinned by those independent unit tests, so the repo-baseline test is a legitimate freshness guard, not a circular detector golden. Good design.
• The script correctly excludes itself (iter_scanned_files, scripts/check-ledger-references.py:156-157,175) so the slice-4 / TASK-4-5 examples in its own docstring don't self-trip.

Non-blocking observations

1. The bundled freshness test is a de-facto hard CI gate — worth making explicit.
The PR is framed as "advisory (warn, never block); --strict intentionally not wired into CI yet." But TestRepoBaselineIsClean (tests/scripts/test_check_ledger_references.py:165-177) scans the whole live corpus and asserts zero net-new against the committed baseline, and it runs under make test-all (Makefile:398 roots include tests/). Net effect: any PR that introduces a token matching the pattern — including legitimate live-machinery false positives (a new slice-\d+ regex string, a TASK-\d+ docstring example, a cq-\d+ reference) — turns make test-all red until the author runs --update-baseline or adds ledger-ok. That partially front-loads the exact false-positive tuning cost the PR says it's deferring. This is the standard ratchet pattern and arguably correct (it keeps the baseline from rotting), but the "never blocks a PR" wording in the description understates it. Suggest documenting that the test, not the lint exit code, is the enforcement point.

2. Markdown under test directories is not excluded (asymmetry with .py).
is_test_path is only applied in the .py loop (check-ledger-references.py:171); the markdown loop (:159-163) filters via is_excluded only. So a future fixture like orchestrator/tests/fixtures/foo.md using slice-1 / task-1-1 as live test data would be scanned and counted, while its .py sibling would not. Currently latent — the one test-dir markdown (gateway/tests/README-integration.md) has no tokens — but the asymmetry will surprise someone. Suggest applying is_test_path in the md loop too, or noting the asymmetry in the docstring.

3. Case sensitivity. slice-\d+ / cq-\d+ are lowercase-only and TASK-\d+ uppercase-only (:98-102). Slice-4 at a sentence start, CQ-2, or a lowercase task-5 in prose would be missed. Probably fine given repo conventions, but since the detector is the whole point, a case-insensitive variant (with the underscore/dotted-access guards kept) would be more robust.

4. Docstring wording vs behavior (minor). The docstring says the Python scan covers "docstrings/inline comments" (:43-44), but scan_file regexes every line. In practice - can't appear in a Python identifier, so tokens only land in strings/comments anyway — the effect matches, but the wording implies a parse that doesn't happen.

5. Performance (minor). repo_root.rglob("*.md") (:159) walks the entire tree — including .git/, .venv/, node_modules/ — before filtering via is_excluded. Fine at current repo size; prune during traversal if those dirs grow.

None of these block merge. Nice, well-tested guardrail.

— Authored by egg

[james-in-a-box]

Review feedback addressed (commit 4e6fa8bb6)

Thanks for the thorough trace and the design caveat. Per-item disposition:

1. Blocking freshness self-test → de-facto hard CI gate (@jwbron's caveat + reviewer obs #1) — fixed-in-PR (commit 4e6fa8b)
Agreed. Removed TestRepoBaselineIsClean::test_no_net_new_against_committed_baseline from the blocking unit suite. The committed-baseline freshness check now lives only in the advisory scripts/check-ledger-references.py (surfaced via make lint-custom), which already prints net-new files plus the --update-baseline reminder without reddening make test-all. A comment in the test module records why the check is intentionally advisory-only, so it doesn't get re-added. This removes the maintenance treadmill that contradicted the issue's "advisory, never block" intent.

2. Markdown under test dirs not excluded (asymmetry with .py) (reviewer obs #2) — fixed-in-PR (commit 4e6fa8b)
Agreed. is_test_path is now applied to the markdown loop as well, so test-dir markdown is excluded symmetrically with .py. Added test_test_dir_markdown_excluded to pin it. No baseline change (no test-dir markdown currently carries tokens).

3. Case sensitivity (reviewer obs #3) — fixed-in-PR (commit 4e6fa8b) for slice/cq; disagree on TASK
Split decision, backed by measuring the corpus:

• slice-N and cq-N are now case-insensitive, so sentence-initial Slice-4 and upper-cased CQ-2 are caught. Empirically this picks up only legitimate capitalized Slice-N ledger refs (e.g. in event_loop.py, routes/pipelines.py, concurrent-execution.md) — zero false positives. Baseline regenerated to absorb them.
• TASK-N stays UPPERCASE-only (disagree on folding its case). Lowercase task-N is pervasive live runtime vocabulary, not ledger narration: timestamped run ids (task-20251129-222239), tool-doc example ids (task-123), and contract task identifiers in handler code. A case-insensitive TASK flagged 50+ files of these false positives in the live corpus — exactly the live-vs-ledger conflation the underscore/dotted-access guards already avoid. The plan-format casing is the discriminator, so I kept it. Added test cases asserting lowercase task-N is ignored.

4. Docstring wording vs behavior (reviewer obs #4) — fixed-in-PR (commit 4e6fa8b)
Agreed. The docstring no longer implies a docstring/comment-only parse; it now states every line is scanned and notes that tokens only land in strings/comments in practice because - isn't a Python identifier character.

5. Performance — rglob walks .git/.venv/node_modules before filtering (reviewer obs #5) — fixed-in-PR (commit 4e6fa8b)
Agreed. iter_scanned_files now uses os.walk with in-place directory pruning, so traversal never descends into .git/, .venv/, node_modules/, .egg-state/, docs/templates/, or test directories. Existing exclusion tests still pass against the new walker.

All 36 tests in tests/scripts/test_check_ledger_references.py pass; advisory run and make lint-custom are clean. (The make lint mypy errors are pre-existing in shared/egg_agent/client.py, untouched by this PR.)

— Authored by egg

[egg-reviewer]

Re-review: Advisory ledger-reference ratchet lint (#3328)

Re-reviewed against the current branch tip (4e6fa8b). The PR is scoped to exactly its three ledger files — scripts/check-ledger-references.py, scripts/ledger-references-baseline.yaml, tests/scripts/test_check_ledger_references.py (confirmed via gh pr diff --name-only; the doc/pipelines.py deltas a two-dot diff shows are just main drift, the branch is behind on those files and does not author them). All five non-blocking observations from my prior review are addressed substantively, not superficially. No blocking issues. Approving.

Verified fixes

1. Blocking freshness self-test dethroned — TestRepoBaselineIsClean::test_no_net_new_against_committed_baseline is gone from the blocking suite, replaced by a module-level NOTE (test_check_ledger_references.py:178-190) that records why it must stay advisory-only so it doesn't get re-added. This removes the de-facto hard CI gate / maintenance treadmill that contradicted the issue's "advisory, never block" intent. The committed-baseline freshness check now lives only in make lint-custom (exit 0). Correct call.

2. Markdown/Python test-dir exclusion now symmetric — is_test_path(rel) is applied to every file in _walk_in_scope (check-ledger-references.py:171), covering both the .md repo-wide walk and the .py source-root walks. test_test_dir_markdown_excluded pins it. The prior .md-vs-.py asymmetry is closed.

3. Case sensitivity — slice-N / cq-N are now case-insensitive via scoped inline groups (?i:slice) / (?i:cq), while TASK-N stays uppercase-only. I verified the compiled LEDGER_PATTERN empirically across 17 cases: Slice-4/SLICE-7/CQ-2/Cq-3 match; task-5/task-123/task-20251129-222239 (live runtime vocabulary) correctly do not; slice_id/contract.slices/subslice-4 stay excluded. I agree with the split decision to leave TASK uppercase-only — lowercase task-N is genuinely pervasive live vocabulary (timestamped run ids, contract task ids), and the plan-format casing is the right discriminator. Both directions are covered by TestPattern.

4. Docstring wording — the "Scope" section now states every line is scanned and explains that tokens only land in strings/comments in practice because - isn't a Python identifier char. Matches behavior.

5. Performance — iter_scanned_files now uses os.walk with in-place dirnames[:] pruning (_walk_in_scope), so traversal never descends into .git/ / .venv/ / node_modules/ / .egg-state/ / docs/templates/ / test dirs. Replaces the prior rglob("*.md") whole-tree walk.

Test quality is sound: hand-authored expectations throughout (no self-seeding goldens), update_baseline exercised through the real production path, evaluate tested in isolation with scan_file/scan_all covered separately.

Non-blocking observations (residual, advisory-only impact)

• test (singular) directory not excluded. EXCLUDED_DIR_NAMES is {"tests", "__pycache__"} and is_test_path only checks those parts. A future **/test/ directory (a tester-writable pattern per agent-roles.md) carrying slice-N/cq-N fixture data would be scanned, unlike its tests/ sibling. Latent today; worth folding "test" in for completeness. Advisory-only, so at worst it's noise in make lint-custom.

• Dangling-symlink robustness. scan_file uses read_text(errors="replace"), which tolerates bad encodings but not a broken symlink surfaced by os.walk (would raise OSError). Extremely unlikely in-tree, advisory path only, but a try/except OSError: return zero-count would harden it.

Neither blocks. Clean, well-tested guardrail — the feedback was handled thoroughly.

— Authored by egg

[james-in-a-box]

Review feedback addressed (commit 5abfb5c)

Thanks for the re-review. Both residual non-blocking observations are now handled in-PR. Per-item disposition:

1. test (singular) directory not excluded (re-review residual obs #1) — fixed-in-PR (commit 5abfb5c)
Agreed. Folded "test" into EXCLUDED_DIR_NAMES (now {"tests", "test", "__pycache__"}), so a **/test/ subtree carrying slice-N / cq-N fixture data is pruned during traversal and excluded from is_test_path symmetrically with its tests/ sibling. Added test_singular_test_dir_skipped (unit-level is_test_path) and test_singular_test_dir_pruned (end-to-end iter_scanned_files walk) to pin both the path predicate and the traversal pruning. No baseline change — the repo has no token-bearing singular-test/ files today, and the exclusion only ever lowers counts (which never warns).

2. Dangling-symlink / unreadable-path robustness (re-review residual obs #2) — fixed-in-PR (commit 5abfb5c)
Agreed. scan_file now wraps the read_text in try/except OSError, degrading to a zero-count FileFindings rather than crashing the whole advisory run. errors="replace" already tolerated bad encodings but not an OSError from a broken symlink surfaced by os.walk; this closes that gap. Added test_unreadable_path_degrades_to_zero, which symlinks to a non-existent target and asserts a clean zero-count finding.

All 39 tests in tests/scripts/test_check_ledger_references.py pass; ruff check, ruff format --check, and mypy are clean on both changed files; the advisory and --strict runs both exit 0 against the committed baseline.

— Authored by egg

[egg-reviewer]

Re-review: Advisory ledger-reference ratchet lint (#3328)

Reviewed the delta 4e6fa8b..5abfb5c (12+/3- in scripts/check-ledger-references.py, 24+ in the test module) — the two residual non-blocking observations from my prior re-review. Both are addressed substantively, with genuine tests that exercise the production path. No blocking issues. Approving.

Verified fixes

1. Singular test/ directory now excluded — EXCLUDED_DIR_NAMES is {"tests", "test", "__pycache__"} (check-ledger-references.py:102). This feeds both enforcement points: is_test_path (:170, exact part in … match) and the in-place pruning in _walk_in_scope (:186, exact name not in …). The match is exact-on-path-component, so there's no over-reach — latest/, contest/, and a file literally named test.md (part "test.md" != "test") are all unaffected; I checked. Pinned by test_singular_test_dir_skipped (predicate) and test_singular_test_dir_pruned (end-to-end iter_scanned_files walk). Symmetric with tests/ now, as intended.

2. scan_file hardened against unreadable paths — the read_text is wrapped in try/except OSError returning a zero-count FileFindings (:223-230). os.walk surfaces a dangling symlink in filenames, and read_text(errors="replace") tolerates bad encodings but not the resulting FileNotFoundError/OSError. Degrading to zero rather than crashing the whole run is the correct call for an advisory-only scan, and the broad OSError is safe here: _walk_in_scope only ever passes regular files, and the input is the repo tree being scanned, not deliberately-set operator config — so this is legitimate graceful degradation, not a misconfiguration-masking fallback. Pinned by test_unreadable_path_degrades_to_zero (real dangling symlink → real scan_file).

Verification performed

• find over orchestrator|gateway|sandbox|shared|scripts|config for singular test/ dirs → none exist today, confirming the author's "no baseline change" claim. The exclusion is purely forward-looking.
• No baseline entry contains a /test/ segment (113 entries scanned) → the broadened exclusion has zero effect on the current committed baseline.
• python3 scripts/check-ledger-references.py (advisory) and --strict both exit 0 against the committed baseline.
• pytest tests/scripts/test_check_ledger_references.py → 39 passed.

Test quality remains sound: hand-authored expectations throughout, no self-seeding goldens, the new tests construct real symlinks / real directory trees and drive the actual scan_file / is_test_path / iter_scanned_files production code (no hand-built fixtures bypassing the helpers).

Nothing residual worth flagging. Clean, well-tested guardrail — the feedback was handled thoroughly.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Review feedback — disposition summary

The latest review (egg-reviewer, APPROVED at the current HEAD 5abfb5c) found no residual actionable items — "Nothing residual worth flagging. Clean, well-tested guardrail — the feedback was handled thoroughly." No line-level review comments are open.

Every observation raised across the three-review cycle was already resolved in-PR. Recording the full ledger for completeness:

Review 1 (commit e5ceb22) — five non-blocking observations

1. Blocking freshness self-test was a de-facto hard CI gate — fixed-in-PR (commit 4e6fa8b): removed test_no_net_new_against_committed_baseline from the blocking suite; the freshness check now lives only in the advisory make lint-custom path.
2. Markdown under test dirs not excluded (asymmetry with .py) — fixed-in-PR (commit 4e6fa8b): is_test_path now applied to the markdown loop too; pinned by test_test_dir_markdown_excluded.
3. Case sensitivity — fixed-in-PR (commit 4e6fa8b) for slice/cq (now case-insensitive); disagree on folding TASK case — lowercase task-N is pervasive live runtime vocabulary (timestamped run ids, contract task ids), and a case-insensitive TASK flagged 50+ false-positive files; the plan-format uppercase casing is the correct discriminator. The reviewer concurred with this split in re-review.
4. Docstring wording vs behavior — fixed-in-PR (commit 4e6fa8b): docstring now states every line is scanned and explains tokens only land in strings/comments because - isn't a Python identifier char.
5. Performance (rglob whole-tree walk) — fixed-in-PR (commit 4e6fa8b): switched to os.walk with in-place dir pruning.

Review 2 (commit 4e6fa8b) — two residual observations

1. Singular test/ directory not excluded — fixed-in-PR (commit 5abfb5c): folded "test" into EXCLUDED_DIR_NAMES; pinned by test_singular_test_dir_skipped and test_singular_test_dir_pruned.
2. Dangling-symlink / unreadable-path robustness — fixed-in-PR (commit 5abfb5c): scan_file wraps read_text in try/except OSError, degrading to a zero-count finding; pinned by test_unreadable_path_degrades_to_zero.

Review 3 (commit 5abfb5c, current HEAD) — clean approval, no actionable items.

No new code changes were required for this round. All 39 tests in tests/scripts/test_check_ledger_references.py pass and the advisory/--strict runs both exit 0 against the committed baseline.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

13 previous review(s) hidden.