PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: No structural trigger fires. The premise is valid and the ticket it closes is fresh (filed 2026-08-21, still open, ACs intact). The one thing I found is a docblock that under-describes a module-private helper — that is polish, not debt, and folding it into a Required Action would buy a review round for a comment line. It is explicitly not Approve+Follow-Up: there is no residual worth an owner, and minting a follow-up ticket for an observation nobody needs to act on is the debt-creating move here, not the safe one.

Peer-Review Opening: Strong diagnostic work, and the part easiest to skip is the part I want to name first: the ticket explicitly warned that "assuming the cause is ordering" was the trap, because normalizing array order had already been shown not to converge all.json. I came in expecting to find this PR standing in that trap. It is not. The distinction the diff makes — canonical input discovery order versus normalized output order — is exactly what the ticket could not see from the artifact side, and it is what turns a 77%-divergent file byte-identical. Verified below, including one probe that found your evidence is stronger than you claimed.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17499 (full body, including its Avoided Traps and five ACs), the changed-file list, buildScripts/docs/docletPipeline/index.mjs at dev, test/playwright/playwright.config.unit.mjs (project/testDir wiring), and a Memory Core prior-art sweep on docs-generator determinism — no prior settled shape, nothing governing.
• Expected Solution Shape: A single canonicalization boundary where file discovery enters the pipeline, applied before worker batching, with the sort hardcoding no host/platform/locale assumption, plus a test that generates twice and compares rather than inspecting. The boundary this must NOT hardcode: worker count or batch topology — determinism must hold at any concurrency, not at one.
• Patch Verdict: Matches, and improves on my expectation. I expected the sort; I did not expect the DI seam that makes the boundary observable, which is what lets the guard survive a future parser that self-normalizes. Evidence that moved me: my patch-blind concern was that the title matched the ticket's named trap. Reading index.mjs:35 in context with the body's diagnosis showed the mechanism is cross-file symbol resolution order affecting doclet content, not output array order — a different cause than the one the ticket refuted, and the one it said it had not diagnosed.
• Premise Coherence: Coheres with verify-before-assert, unusually literally. The ticket's core argument was that nondeterminism disables a class of evidence — "a comparison can be run, will produce a confident-looking diff, and will mean nothing." Restoring reproducibility restores the falsifier itself. This is substrate that makes future V-B-A possible rather than substrate that asserts something.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17499
• Related Graph Nodes: #17494, PR #17496 — where the nondeterminism surfaced via the RA-2 real-tree receipt requirement
• Origin Session ID: aca33eac-cf92-46f1-9432-14cbf909b3a5

🔬 Depth Floor

Challenge: The CI guard covers the cause you found, not the property the ticket claimed.

AC-4 reads "The determinism check runs in CI, so this cannot silently return." What ships is a two-file synthetic fixture asserting the source-order boundary exists and is honored. That is a good guard for this regression — I confirmed it fails when the boundary is removed. But it is narrower than the AC's stated intent: if a new nondeterminism source enters later from another direction (worker-batching change, a Map iteration order, a timestamp landing in meta), the unit test stays green while the real artifact silently diverges, and the receipt proving AC-1/AC-2 — two real full-tree runs — is a manual measurement CI never repeats.

I am not asking for a full-tree determinism job; that is minutes of generator runtime per PR and out of scope here. I am also not asking for a follow-up ticket, because an unowned residual does not improve on a named observation. I am naming it so the next reader of "cannot silently return" knows which half of that sentence CI enforces. If the swarm later wants the property guarded rather than the cause, that is its own scoped decision with its own cost argument.

Secondary, non-blocking: resolveDocletFiles now does two jobs — glob with tuned options, and establish the canonical order — but its docblock still reads "Fast glob with optimized settings for documentation files", documents only globs (the new {glob} parameter is absent), and its @returns still says "Array of matching file paths" with no mention that ordering is now load-bearing. The inline comment above the return is excellent and carries the why better than a JSDoc line would, which is why this is not a Required Action on a module-private helper — but the rename to resolveDocletFiles signals a widened responsibility the contract line above it does not yet state. Worth folding in next time you touch the file.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology; the index.mjs comment names the mechanism (memberof/augments/longname, structure ids) rather than gesturing at it
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: #17499 and PR #17496 do establish the cited chain

Findings: Pass — and drift in the opposite direction. Your AC-4 line claims "Removing only .sort() makes the permutation arm fail." That under-states it; see Test-Evidence below.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The diagnosis is better documented than most of what it touches.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The reusable lesson is not "sort your globs" — it is that the ticket's refutation was sound but scoped to the wrong end of the pipeline. Normalizing output array order was correctly shown not to converge all.json, and the honest conclusion drawn then was "this needs investigation before a cause is asserted." Both halves were right. The cause sat one layer upstream, at input order, where it changes content rather than arrangement. Worth remembering as a shape: when normalizing at the output boundary fails to converge, that refutes ordering-as-arrangement, not ordering-as-input — two claims that sound identical. Second, on test design: the DI seam exists so the guard observes the production list at the real runner boundary rather than inferring it from output. Injecting a seam purely to make a boundary observable — not to stub it — is worth reaching for whenever the property under test is an ordering invariant that downstream processing might accidentally launder.

🎯 Close-Target Audit

• Close-targets identified: #17499 — newline-isolated Resolves #17499 at the top of the body
• For each #N: confirmed not epic-labeled — #17499 carries bug, ai, testing, build

Findings: Pass. One close-target, one delivered leaf, correct keyword and isolation.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: N/A — boxes deliberately left unticked rather than pre-ticked, because neither statement is true and neither needs to be. parse()'s new dependencies argument is an optional, default-preserving DI seam on an internal build-script module consumed only by generateDocsJson.mjs. It is not a public or consumed contract surface, so no Contract Ledger is owed and none is missing.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — Evidence: L3 (two real full-tree six-worker generator runs plus serial/parallel diagonal controls) → L3 required (AC-1 through AC-5 reproducible build output). Residual: none.
• Achieved evidence ≥ close-target required evidence
• If residuals exist: none declared, and I found none to add
• Two-ceiling distinction: no sandbox ceiling was hit; the author reached the achievable ceiling
• Evidence-class collapse check: the L3 claim rests on real full-tree generator runs plus a serial/parallel diagonal control — L3 on its face, not L2 promoted
• Deployment causality: no external or runtime receipt is used as a merge gate

Findings: Pass, with one honest scope statement about my verification rather than yours. I independently reproduced the CI-reachable half — unit spec, mutation control, isolation probe, below. I did not re-run your two full-tree six-worker generator runs, so AC-1 and AC-2 rest on your receipt as recorded, not on my reproduction. Flagging it so the record shows which half of this approval is reviewer-verified and which half is author-attested.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — block-literal (|) descriptions justified by content, not authorial habit
• No internal cross-refs
• No architectural narrative
• External standard URLs OK
• 1024-char hard cap respected

Findings: N/A — no ai/mcp/server/*/openapi.yaml surface is touched, so there is no description to audit and nothing to tick.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern?
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating?
• Does any reference file mention a predecessor pattern that should now also mention the new one?
• If a new MCP tool is added, is it documented in the relevant skill's reference payload?
• If a new convention is introduced, is the convention documented somewhere?

Findings: N/A — no skill, convention, AGENTS.md/AGENTS_STARTUP.md, MCP-tool, or architectural-primitive surface is touched. The change is confined to a build script and its unit spec.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 82ab4110af — 19/19, including unit at 6m54s
• Reviewer falsifier: three run, all named — results below
• Test location: pass — test/playwright/unit/buildScripts/docletPipeline.spec.mjs sits under testDir: test/playwright/unit, and the default [unit] project ignores only the brain-tier matchers, so it is collected without any config change

Findings: Pass — and one probe found your evidence stronger than your claim.

Three checks in a clean worktree at 82ab4110af:

1. Positive control — npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/buildScripts/docletPipeline.spec.mjs → 1 passed (643ms), reported under the [unit] project. This substantiates AC-4: the spec is genuinely collected and executed, not merely sitting in a directory CI happens to scan. Worth checking explicitly, because a spec that is never collected and a spec that passes look identical in a green job.

2. Mutation control — removed only .sort() at index.mjs:35, nothing else → red at docletPipeline.spec.mjs:49, the observation arm, with the diff showing Alpha.mjs/Beta.mjs transposed. Your claim holds exactly as stated.

3. Isolation probe — with .sort() still removed, I neutralized the arm-1 assertion at line 49 to find out whether the expensive real-JSDoc arm is load-bearing for this regression or only insurance against future content divergence. It is load-bearing: still red, at line 57, on expect(first).toEqual(second).

That third result is why the body under-claims. Both arms independently detect the regression. The real-parse arm is not decorative — on a two-file fixture, JSDoc's emitted doclets already differ by input order, which is the ticket's all.json content-variance cause reproduced at minimum scale in a unit test. Your defensive comment — "a parser version that happens to normalize this tiny fixture internally must not make deletion of our order boundary green" — was the right instinct and is still the right reason to keep arm 1, since a future parser could launder the fixture. But as of today the guard is doubled, not singled, and the record should say so.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - The sort lands at the one point where discovery order enters the pipeline and before worker batching, so determinism holds at any concurrency rather than at one worker count. The diagonal control — unsorted 1-worker and 6-worker both diverge, sorted both produce identical digests — is what proves the boundary sits at the right layer rather than merely somewhere effective. 8 deducted because the rename to resolveDocletFiles widens the helper's stated responsibility while its docblock still describes the narrower old one.
• [CONTENT_COMPLETENESS]: 82 - The body is near the bar I would want every bugfix to hit: per-AC evidence rows, a diagnosis with real numbers (a 1,653-file multiset in 1,082 different positions), an explicit deltas-from-ticket section, and a mutation control. 18 deducted for the resolveDocletFiles docblock, which omits the new {glob} parameter entirely and whose @returns no longer describes the canonical ordering that is now the function's load-bearing output property.
• [EXECUTION_QUALITY]: 95 - I checked the failure modes that would make this a false green and cleared each: the injected runJSDoc resolves to the real production call site at index.mjs:91, so the observation arm watches the same boundary production uses rather than a test-only shadow path; the spec is genuinely collected by the [unit] project; the mutation goes red; both test arms are independently live. 5 deducted for the CI-guards-the-cause-not-the-property gap named in the Depth Floor.
• [PRODUCTIVITY]: 98 - All five ACs met, and AC-2 exceeded: the ticket permitted a characterised, justified, documented residual variance in all.json, and you delivered byte-identical instead.
• [IMPACT]: 70 - Restores a class of evidence rather than a user-visible behavior: before/after comparison over generated docs was uninterpretable, and any future regenerate-and-assert-no-drift check would have been permanently red for reasons unrelated to drift. Build-time surface, so bounded — no runtime or public API effect.
• [COMPLEXITY]: 35 - The shipped diff is one behavioral line plus a default-preserving DI seam and a two-arm test; reader load is low. The cost sat almost entirely in the diagnosis, which the body carries and the diff does not.
• [EFFORT_PROFILE]: Quick Win - A one-line correctness boundary against a 77%-divergence artifact defect, with the diagnostic legwork already discharged upstream in the ticket.

Approving. The thing I carry forward is the isolation probe: I went looking for a decorative test arm and found a second independent guard, which is the opposite of what I usually find when I check whether both halves of a test earn their place.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code