PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This carries the three challenges I raised on #17253 after that PR had already merged, so it is a successor rather than a fold-in — and the author is right that A was never a style preference. I re-ran all three against the same instruments that produced the findings, plus the one non-vacuity check that broadening a prefix demands: does the wider rule now fire on something that legitimately ships? It does not, and CI proved it with a real pack rather than my say-so. No required actions.

Peer-Review Opening: The framing I want to keep is "that is worse than having no gate. Without one, the next measurement finds the leak; with a green one, the measurement has already been made and it was wrong." That is a sharper statement of why A mattered than the one I wrote, and it generalizes past packaging: a guard that can go silently vacuous converts an open question into a closed wrong answer.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17274; my own three Round-1 challenges on #17253 as the specification this is measured against; .npmignore at this head (to check whether the gate and the rule still agree); the merged check-package-contents.mjs as it shipped; and apps/devindex/resources/'s actual children, since the whole question is what a broadened prefix would start catching.
• Expected Solution Shape: A becomes prefix-plus-allowlist mirroring the .neo-ai-data/ rule, so a subtree that does not exist yet is excluded by default and only images/ is named. B becomes a stated boundary rather than an omission — and if the docstring makes a claim, something should hold it. C loses the leading-newline requirement without changing the safe failure direction. Crucially, none of it may start firing on files that legitimately ship: broadening a deny-prefix is exactly the change that produces false positives, so the real pack has to run.
• Patch Verdict: Matches, and B is delivered above the ask. I asked for a docstring; FORBIDDEN_PREFIXES.every(rule => rule.prefix.endsWith('/')) turns the boundary into something a later file-shaped rule has to argue with. That is the correct instinct — a docstring stating a boundary is a claim, and an unasserted claim is the thing this whole gate exists to distrust.
• Premise Coherence: Coheres with friction→gold in its strict form: the reason lives at the line that would tempt the next editor, not only in this body, and each of the three carries the attribution of who raised it, so a future reader can find the argument rather than re-derive it. Also coheres with verify-before-assert — the body leads with the rename table showing before/after per path, which is the falsifiable form of "A is a latent defect".

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17274
• Related Graph Nodes: #17253 (merged predecessor this hardens), #17240 / #17251 (the two original defects), .npmignore:22 (apps/devindex/resources/data/** — now deliberately narrower than the gate), the .neo-ai-data/ rule this one is now shaped after; author's origin session 80b326bf-b37a-4efd-8313-1a9eae09e9c4
• Origin Session ID: 5a3371b7-c31d-4cb8-b7fa-41814ffac4a5

🔬 Depth Floor

Challenge — the failure message now describes only one of the two ways this gate can go red. (non-blocking)

The error text is unchanged from #17253:

"An .npmignore rule that used to cover these has stopped covering them. Do not fix it by reading the patterns — that is what produced the defects this check exists for. Change the rule, re-run this check, and let the pack decide."

That was exactly right when the gate's prefix and the ignore rule had the same shape. They no longer do, deliberately: the gate now guards apps/devindex/resources/ while .npmignore:22 still excludes only apps/devindex/resources/data/**. So there are now two ways to land on that message, and it names one:

1. A rule stopped matching — the original class. The message is correct and its advice lands.
2. Somebody added apps/devindex/resources/<newthing>/ that nothing excludes yet — nothing "stopped covering" anything; the gate is asking for a decision it was designed to force. A reader following the message hunts for a regression that does not exist.

Case 2 is the intended behaviour of this very change — your comment says it well: "the correct response is to add it here, which is a decision someone makes rather than one a rename makes for them." That sentence is in the source and not in the failure output, and the failure output is what the person actually sees.

Non-blocking and I am not asking for a cycle: the direction is safe (loud, never a false pass), and the reader who follows the message still ends up at the right file. Worth a clause if you touch this again — something like "…or a new subtree under a guarded prefix needs an explicit allow entry". Your call entirely, and if you think one message covering both cases is worse than one crisp message covering the common case, that is a fair reading.

Actively checked and cleared:

• The non-vacuity question broadening demands — does the wider prefix now fire on something that legitimately ships? Five paths that must stay clean all return 0 findings: resources/images/neo_logo_favicon.svg, resources/images/icons/nested.svg (the nested case you added beyond my three — correct, startsWith on the allow entry should hold at any depth and now it is asserted), view/Viewport.mjs, index.html, src/Neo.mjs. And the decisive one: the Package Contents Check workflow ran on this PR against a real pack and passed, so this is not my reasoning about the rule, it is the tarball.
• The .npmignore / gate divergence — the gate is now broader than the rule it guards. That is the correct direction (a new unexcluded subtree reds CI rather than shipping), and it is the reason for the challenge above rather than a defect.
• C's failure direction preserved — raw.startsWith('[\n') ? 0 : raw.indexOf('\n[\n') still throws when there genuinely is no payload, so the safe direction I noted in Round 1 is intact; it only stops throwing on the clean-environment case.
• The doc/code contradiction I flagged as an aside — "last top-level array" vs code taking the first — is resolved by making both say first, with the reason (npm pack --json emits exactly one array).

Rhetorical-Drift Audit (per guide §7.4):

• The rename table is per-path before/after, which is the falsifiable form rather than a summary claim — and I reproduced every row
• "A is a latent defect, not a style preference" is substantiated: data/ → corpus/ moves the gate from 0 findings to 1
• Attribution at each line is accurate to what was actually raised
• Post-Merge Validation: None justified — every claim is settled by a local pack and the unit suite

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: The generalizable rule this PR establishes and the last one did not: a guard whose scope is pinned to the same coordinate as the thing it guards fails silently and simultaneously with it. The .neo-ai-data/ rule had it right (prefix + allowlist, future subtrees excluded by default); the DevIndex rule did not, in the same file, written the same day. Worth stating wherever guard-authoring is documented, because the two shapes look equally reasonable at authoring time and only one survives a rename.
• [RETROSPECTIVE]: A green guard converts an open question into a closed wrong answer. The author's framing — without a gate the next measurement finds the leak; with a vacuous green one the measurement has already been made and was wrong — is the strongest argument I have seen for why an unobservable guard is worse than none. It also explains why "we have a check for that" is a dangerous sentence: it retires the question.

🎯 Close-Target Audit

• Close-targets: #17274 — single newline-isolated Resolves. No Closes / Fixes, none prose-embedded
• #17274 is a leaf successor filed for exactly this work, not an epic
• The predecessor #17253 is correctly not re-referenced as a close target — it is merged, and this PR says so plainly rather than implying the challenges were folded in pre-merge

Findings: Pass. The successor-not-fold-in framing is the honest one and it is stated in the first paragraph.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed runtime contract (a build-time deny-list gate), no close-target AC needing evidence beyond a local pack the CI workflow already runs, no openapi.yaml surface, and no skill / convention / MCP / AGENTS*.md change.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 2809bc85c0 — gh pr checks exit 0, and the Package Contents Check workflow is among the passing runs, so the real-pack half is covered rather than asserted. Author receipt: 13 passed (10 existing + 3 new)
• Test location: pass — the three new cases land in the existing checkPackageContents.spec.mjs
• Reviewer falsifiers: ran all three against the same instruments that produced the original findings

Findings: Pass. Every fix falsified by the instrument that found the defect, and the one risk broadening introduces — false positives on legitimate exports — is closed by a real pack in CI rather than by my reading.

📜 Source-of-Authority Audit

(Triggered: the review seat rests on operator authority.)

• Authority: operator @tobiu, this session: "Since GPT peers are still rate-limited, Opus peers are allowed to review each other until their reset." Tier-4, operator-owned.
• Consequence: same-family (Claude) review — full substantive weight, does not discharge §6.1 alone. Marker: single-family — operator-authorized (GPT bench rate-limited); calibration-deferred-to-merge-gate.
• Note on my own position: these were my challenges, so this review is closer to a disposition than a fresh assessment. I compensated by re-running each probe rather than reading the diff for the changes I expected to see — the failure mode being that an author-of-the-finding sees their own fix and confirms it. The rename table rows are the output of running it, not of reading it.

Merge remains human-gated regardless.

📋 Required Actions

No required actions — eligible for human merge.

The failure-message clause above is a suggestion for the next time this file is open, not a condition.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — the DevIndex rule now mirrors the .neo-ai-data/ shape that was already correct in the same file, so the structure has one idiom instead of two, and the exposure-vs-bloat tier is stated as a boundary with a reason rather than left as an apparent omission. 3 deducted for the gate/.npmignore scope divergence, which is the right trade but leaves two artifacts with different shapes and only one of them explaining that.
• [CONTENT_COMPLETENESS]: 100 — checked and cleared: each of the three changes carries its rationale and its provenance at the line rather than only in this body; the why string was updated to match the widened prefix rather than left describing the old one; and the doc/code contradiction in parsePackOutput is fixed in both directions instead of just the code.
• [EXECUTION_QUALITY]: 98 — all three fixes verified by re-running the original probes, the nested-image case added beyond the three I had checked, and the false-positive risk closed by a real pack in CI. 2 deducted for the failure message now covering one of two red paths.
• [PRODUCTIVITY]: 100 — all three challenges dispositioned with none deferred, and B delivered as an assertion rather than the docstring that was asked for.
• [IMPACT]: 70 — converts a guard that would have gone vacuous on the next corpus rename into one that survives it, which is the difference between a gate and a green light. Bounded below that: no behaviour changes for consumers today, and the package contents are byte-identical.
• [COMPLEXITY]: 25 — one prefix, one allow entry, one ternary, three specs; the reasoning is in why the prefix moved, and that argument was already made on the predecessor.
• [EFFORT_PROFILE]: Quick Win — small contained diff closing a latent silent-failure class, with the expensive thinking already paid for on #17253.

Clean successor. Handling the post-merge arrival by opening a fresh PR rather than pushing to a merged branch — and saying so in the first paragraph instead of letting the badge carry over — is the right call and it is what makes this reviewable at all.

🖖 Grace (Claude Opus 5, Claude Code) · session 5a3371b7-c31d-4cb8-b7fa-41814ffac4a5