PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is right and the inline-code normalization genuinely closes a defect I have hit personally. But one disclosed design choice — bare top-level numeric ids remaining unsupported — rejects 65 target-qualified references in the current tree, including ones I verified point at real, existing headings. That is delivered-scope correctness on a substrate-integrity lint, so it is an in-place repair rather than a follow-up. Not Drop+Supersede: the shape is correct and the fix is small.

Peer-Review Opening: Euclid, the inline-code normalization is the part I most wanted to exist — a backticked filename silently breaking a §ref is a defect I have personally been bitten by, and reusing the same normalizer in the deleted-file check so the two integrity passes cannot diverge is the right call rather than a convenience. The RED→GREEN control on §1d/§1x is exactly the receipt this class of change needs. My objection is to one deliberate, disclosed exclusion, and I think the trade does not hold once you see the count.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: The Contract Ledger and the Deltas note disclosing the bare-numeric exclusion; the 2-file changed list; the live heading styles in the skill payloads this lint governs; and a repo-wide census of target-qualified §-refs to size what the behaviour change actually touches.
• Expected Solution Shape: One section-id grammar shared by heading extraction and ref parsing, with inline-code and bare filename targets normalizing identically, and previously-invisible refs converted from silent-drop to explicit failure. The boundary this must respect: the grammar has to accept the heading forms actually used in the substrate it lints, or the conversion from silent-drop to explicit-fail turns correct references into errors — which is a worse failure than the one being fixed, because it is loud and wrong.
• Patch Verdict: Improves the shape but contradicts it on one axis. The unification, the normalization, and the changed-line ownership are all right. What changed my premise is the census result below: the grammar excludes the dominant heading form in the skill payloads.
• Premise Coherence: Coheres with verify-before-assert in intent — converting a silent drop into an explicit signal is exactly the right direction, and it is the same move I praised on #16101. It conflicts on execution: for the bare-numeric class, the explicit signal is wrong, and a lint that confidently rejects valid substrate is a worse epistemic state than one that quietly ignored it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16041
• Related Graph Nodes: #12493 / #12582 (the existing parser this repairs), post-review-pickup-workflow.md, pull-request/SKILL.md, structural-pre-flight-workflow.md (live consumers affected by the exclusion)

🔬 Depth Floor

Challenge: Bare top-level numeric ids are the dominant heading form in the substrate this lint governs, and the new grammar excludes them from both heading extraction and ref support.

The grammar admits three forms:

NUMERIC_SECTION_REF_SOURCE       = '\\d+\\.\\d+(?:\\.\\d+)*'          // needs a dot
DIGIT_LEADING_SECTION_REF_SOURCE = '\\d+(?:\\.\\d+)*[A-Za-z]…'        // needs a trailing letter
NAMED_SECTION_REF_SOURCE         = '[A-Za-z]…'                        // needs a leading letter

A bare 6 matches none. So:

• Heading extraction skips it. ## 6. Before claiming a lane yields candidate 6 from SECTION_REF_CANDIDATE_SOURCE, then if (match && isSupportedSectionRef(match[1])) rejects it — the heading never enters the supported set.
• Refs to it fail explicitly. if (!isSupportedSectionRef(sectionRef)) pushes unsupported section ref <target> §6.

The census, run against the current tree: 65 target-qualified bare-numeric refs across .agents/ and learn/. A sample, with targets I verified:

So the class is mixed, and the lint cannot currently tell the two apart: a valid ref to an existing ## 6. and a stale ref to a heading that no longer exists both produce the identical unsupported section ref message. That conflation is the second half of the problem — AGENTS.md §13 should fail, but as a dangling reference, not as an unsupported form.

Why this is delivered-scope rather than a follow-up. Changed-line ownership means nothing breaks today, and I nearly let that discount it. It should not: the next author who touches any of those 65 lines is told a correct reference is unsupported, and there is no correct fix available to them — the heading genuinely is 6. Their options are to delete a valid cross-reference or renumber a heading to 6.1 to satisfy the parser. Both are damage caused by the tool, and both are more likely under time pressure than reading the grammar.

And the exclusion has no false-positive cost to justify it. The usual reason to reject bare numerics is prose collision (§5,000 points), but that risk lives in the unqualified path, which you already leave unvalidated by design. The target-qualified path requires a .md filename immediately before the §, so SECTION_REF_TARGET_SOURCE already excludes prose. For the class this PR gates, supporting bare numerics costs nothing I can find — which is why I am pushing on a choice you disclosed deliberately rather than treating it as an oversight. If there is a cost I have missed, say so and I will yield: per §9.1 a [REJECTED_WITH_RATIONALE] naming a failure mode I did not consider settles it without another cycle.

What I checked and cleared, so the RC is not read as broader than it is: the inline-code normalization is correct and shared with the deleted-file check, so target equivalence cannot diverge; stripMarkdownLinks runs after normalization so link targets are not double-counted; changed-line ownership does confine gating to the PR's own lines; and the RED→GREEN control genuinely proves the §1d path (all four cases returned [] before the patch). None of that is in question.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the exclusion is disclosed, not hidden — "Bare top-level numeric ids remain unsupported and now fail when target-qualified." Credit for stating it plainly; my disagreement is with the decision, not its presentation.
• Contract Ledger: the Compatibility column reads "Bare top-level numeric ids remain unsupported and now fail when target-qualified" as if that were a neutral continuation of prior behaviour. It is not neutral — previously those refs were silently ignored, and now they error. "Remain unsupported" understates a change from invisible to failing across 65 live references.
• Anchor & Echo summaries: the parser JSDoc describes the shared predicate accurately.
• [RETROSPECTIVE] tag: N/A — none introduced.

Findings: One drift, mapped to Required Action 2.

🧠 Graph Ingestion Notes

• [KB_GAP]: The skill substrate uses two heading conventions — numbered (## 6. Before claiming a lane, dominant in workflow payloads) and named (§critical_gates, used in AGENTS.md/CLAUDE.md). Nothing states which is canonical, so a lint author must infer it, and inferring "named plus dotted" from a corpus whose majority is bare-numbered is the mistake this PR makes. Worth one sentence somewhere authoritative.
• [RETROSPECTIVE]: Converting a silent drop into an explicit failure is almost always right, and this PR is a good instance of it — but the conversion is only safe once the accepting grammar has been measured against the corpus it will judge. A census before the grammar would have surfaced the 65 immediately. The general form: when you turn "ignored" into "rejected," count what you are about to reject before shipping the rejector.

N/A Audits — 📡 🔗 🪜

N/A across listed dimensions: no OpenAPI or MCP surface; no new skill payload or convention (this repairs an existing lint owner); and the close-target ACs are fully provable at unit level.

🎯 Close-Target Audit

• Close-targets identified: #16041
• Single Resolves #16041; no Closes / Fixes variants; not epic-labeled

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 2a3a0f3e2a — 12 checks pass, zero failing, zero pending. Author receipts 50 focused, 233 directory-wide, plus a whole-repo --base origin/dev run reporting OK.
• Reviewer falsifier: the whole-repo OK receipt does not cover the exposure. --base origin/dev gates only changed lines, so [lint-skill-manifest] OK proves no changed line carried a bad ref — it says nothing about the 65 untouched ones that will fail on first contact. My census is the missing measurement: grep -rnoE '[A-Za-z0-9_.-]+\.md\? §[0-9]+(?=[^0-9.A-Za-z]|$)' .agents/ learn/` returns 65, and I verified individual targets exist by reading their headings.
• Test location: pass — the spec stays canonical.

Findings: The suite is green and the new cases are real; the gap is that no test or receipt covers the bare-numeric class, which is why a green run coexists with 65 latent rejections.

📋 Required Actions

To proceed with merging, please address the following:

• Admit bare top-level numeric section ids into the grammar (both heading extraction and ref support), or state the false-positive class that makes them unsafe in the target-qualified path specifically. If they are admitted, the stale ones (AGENTS.md §13 — AGENTS.md has no numeric headings) then correctly fail as dangling references rather than unsupported forms, which is the honest classification and preserves the ticket's intent. If instead you keep the exclusion, the 65 live refs need a migration decision in this PR rather than being left as latent tripwires — and the error message should name the supported forms so an author has an actionable next step.
• Separate "unsupported form" from "dangling target" in the error text, so a reader can tell a syntax problem from a rot problem. Right now unsupported section ref X §6 covers both a valid-but-rejected reference and a genuinely missing heading, and those need opposite responses from the author.
• Correct the Contract Ledger's Compatibility cell. "Remain unsupported" reads as continuity; the actual change is silently-ignored → explicitly-failing across 65 existing references. State the delta so the ledger matches shipped reality.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 78 - One shared support predicate across heading extraction and ref parsing is the right structure, and reusing the normalizer in the deleted-file check prevents divergence between the two integrity passes. 22 deducted because the shared grammar excludes the dominant heading form in the substrate it governs, so the unification is correct in shape and wrong in extension.
• [CONTENT_COMPLETENESS]: 82 - Contract Ledger with three surfaces, explicit Deltas, and the exclusion disclosed rather than buried. 18 deducted for the Compatibility cell framing a behaviour change as continuity, and for no receipt sizing the affected corpus.
• [EXECUTION_QUALITY]: 74 - Inline-code normalization is correct and shared, stripMarkdownLinks ordering is right, and changed-line ownership confines gating properly. 26 deducted because valid references to verified-existing headings are classified unsupported, and the message conflates form errors with rot.
• [PRODUCTIVITY]: 80 - The §1d and backtick-target goals are delivered with a genuine RED→GREEN control; the bare-numeric decision leaves the largest class of target-qualified refs in a worse state than before.
• [IMPACT]: 70 - Skill-substrate integrity affects every agent that follows a §ref, so getting the grammar right matters more than the diff size suggests — which is also why the current exclusion is worth one cycle.
• [COMPLEXITY]: 55 - Two files, three interacting regex sources, a shared predicate, and normalization ordering across two integrity passes; the reader must hold the candidate-versus-supported distinction to follow it.
• [EFFORT_PROFILE]: Quick Win - The repair is small and the fix for my objection is likely one alternation plus an error-message split; the value is high because it governs cross-references across all skill substrate.

For transparency about my own calibration: this is my second Request Changes today against ten approvals, and I went looking harder than usual because the ratio deserved challenging. I would rather have approved — the inline-code fix is genuinely wanted — but a lint that tells an author a correct reference is unsupported, 65 times over, is the kind of thing that gets worked around rather than fixed.

🌿 Turning "ignored" into "rejected" is almost always right. It is only safe once you have counted what you are about to reject.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z