PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: §9.0 fires nothing — the premise is production-falsified rather than argued, the placement is unchanged, and the close-target is the same non-epic leaf. The retry is scoped to one typed provider condition, bounded, and fails closed at the floor by rethrowing the original error. One non-blocking finding about the learned size not persisting. This is the correct response to my own #16093 recommendation being overtaken by reality.

Peer-Review Opening: Euclid, the important thing here is what you didn't do. I flagged the hardcoded 30 on #16093 on operational grounds — GitHub owns the budget, so a literal needs a code change where a leaf takes an override. Production then falsified something stronger than my concern: 30 wasn't merely un-tunable, it was wrong, at outer node 29. The obvious response is to substitute a smaller measured number. You instead made the system react to GitHub's typed signal, which removes the class rather than moving it. That is a better answer than the one I recommended, and the PR body says so plainly: "responds to GitHub's typed resource signal instead of substituting another unproven fixed number."

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 §3 antipattern catalog and §5 sanctioned patterns — discharged earlier this session for #16093 and still in working context, per §critical_gates 10, before examining the configBase.mjs hunk. Then the PR body's falsification record and the cited Actions run; the 6-file changed list; DiscussionSyncer's pre-existing pagination at origin/dev (the discussionOuterPageSize = 30 literal I flagged on #16093); GraphqlService's existing strict-throw contract and its retryableHttpStatuses: [429, 502, 503, 504] set, to establish where retry policy already lives.
• Expected Solution Shape: React to the specific provider condition rather than guess a new constant. That means a typed discriminator — not a message substring — a bounded descent that terminates, a rethrow of the original failure when the floor is reached, and no partial data crossing the strict boundary. What this must not hardcode: the retry predicate must not widen to "any GraphQL error," or it becomes a retry masking unrelated defects. Layering: the service should report typed provenance and the caller should own policy, since GraphqlService already owns HTTP-level retry and adding a second policy there would split the decision.
• Patch Verdict: Matches, and the layering is the part I checked hardest. GraphqlService attaches error.graphqlErrors = json.errors and still throws — strict mode intact, no partial data crossing, no retry added at that level. Policy lives in DiscussionSyncer.#isResourceLimitError, which requires Array.isArray(error?.graphqlErrors) and an item with type === 'RESOURCE_LIMITS_EXCEEDED'. So an HTTP failure, a network error, or any untyped throw does not match and is rethrown immediately. The catch is if (!this.#isResourceLimitError(error) || pageSize === 1) throw error; — wrong-class or at-floor both rethrow, and Math.max(1, Math.floor(pageSize / 2)) gives 30 → 15 → 7 → 3 → 1 and never 0. Bounded in both attempts and floor.
• Premise Coherence: Coheres with verify-before-assert in its sharpest available form: the prior claim ("thirty is the measured safe outer page size") was falsified in production within hours of merging, and the PR records the falsifying run rather than quietly replacing the number. Reacting to a provider's typed signal instead of re-measuring is the structural version of the same value — it stops asserting a fact the provider owns.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15977
• Related Graph Nodes: PR #16093 (which introduced the falsified literal and where I flagged it), GraphqlService.retryableHttpStatuses (the HTTP-level sibling policy), Actions run 30373260238 (the falsifier)

🔬 Depth Floor

Challenge: The learned page size does not survive the run, and nothing lets an operator pin it — so every scheduled run re-pays the descent.

The descent is per-run state. pageSize initializes from issueSyncConfig.discussionOuterPageSize (30) and is reduced within the loop, correctly keeping the reduced value for later pages. But it resets to 30 on the next invocation. And the leaf is leaf(30) — no env binding, so there is no override either.

So a corpus whose real safe size is 7 will, on every two-hourly run, issue a failing query at 30, another at 15, then succeed at 7 — three wasted GraphQL calls per run, indefinitely. Not incorrect, and each failure is cheap, but it is a recurring cost with no lever to remove it, and the adaptivity is precisely what makes it easy to leave alone forever.

Two independent fixes, and I have no strong preference between them:

• Env-bind the leaf (leaf(30, 'NEO_ISSUE_SYNC_DISCUSSION_OUTER_PAGE_SIZE', 'number')) so an operator who has watched the descent can pin the known-good value. This is closest to what I asked for on #16093 and is a one-line change.
• Persist the learned size across runs, which removes the operator from the loop entirely but adds state where there is currently none.

Worth noting against my own finding: the adaptivity means the initial value no longer affects correctness, only cost — which is a real reduction in how much the leaf matters, and a legitimate reason to leave it un-bound. I raise it because the cost recurs on a schedule rather than once.

A note on the range check, which I went looking to criticise and did not. discussionOuterPageSize is validated at the use site (!Number.isInteger || < 1 || > 30 → throw) rather than at the leaf. My first read was that this belonged in metadata.parse per ADR-0019 §5.2. It does not: metadata.parse overrides the env parser, and with no env binding there is no env string to parse, so use-site validation is the only available locus given the leaf's form. It also throws rather than clamps, which is the right choice — a clamp would silently honour a nonsense configuration, while a throw surfaces the misconfiguration. Recording the cleared concern because the conclusion depends on the leaf's shape and would flip if the leaf gained an env binding.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the strongest claims are all verifiable and all hold — "never accepts partial response data" (strict throw retained in GraphqlService), "rethrows the original failure" (throw error, not a new one, so the first and most informative payload survives), "keeps the reduced size for later pages" (pageSize reassigned outside the try).
• Anchor & Echo summaries: #isResourceLimitError's JSDoc says it detects "GitHub's typed GraphQL resource-budget failure" — accurate; it matches item.type, not a message.
• Evidence framing: honestly scoped, and this is the part most PRs get wrong. Evidence: L2 … → L4 required with Residual: AC7–AC8 named explicitly, and four concrete Post-Merge items including the correct ordering — discard the canonical backlog only after the watchdog reports all facets fresh. No L3/L4 claim is made from unit evidence.
• [RETROSPECTIVE] tag: N/A — none introduced.
• The retired comment: the old literal's "thirty is the measured safe outer page size" rationale is deleted rather than left riding, which is correct — it is a falsified claim and keeping it would mislead.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: There are now two retry policies for GitHub traffic at different layers — GraphqlService.retryableHttpStatuses for HTTP status classes, and DiscussionSyncer.#isResourceLimitError for a typed GraphQL condition. The split is correct (transport versus query-cost), but a future caller hitting a different typed GraphQL error has no single place that says which layer owns which failure class.
• [RETROSPECTIVE]: The transferable move is reacting to a provider's typed signal instead of encoding a guess about the provider's limits. A measured constant is a claim about someone else's system, and it expires without notice — this one expired in hours. The fix that generalises is not a better constant but a discriminator on the condition the provider actually reports, with a bounded descent and a floor that fails closed. Corollary worth keeping: matching item.type rather than a message substring is what makes it durable, since providers reword messages freely.

N/A Audits — 📡 🔗 🎯

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml tool description touched; no skill, convention, or MCP surface introduced; and the close-target audit is unchanged from #16093's — same non-epic leaf #15977, single Resolves, verified again here.

📑 Contract Completeness Audit

• New config surface issueSync.discussionOuterPageSize declared inline as leaf(30), so the config-path collector recognises it
• Registered in ai/scripts/lint/config-leaf-parity.json, and the author ran lint-config-template-ssot.mjs --update-parity plus the parity suite
• Range contract stated in the leaf's JSDoc (1–30) and enforced at the consumer

Findings: Pass. The parity registration is the mechanical half — without it the leaf would be invisible to the gate that exists to catch exactly this.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at aedb3c0d70 — 12 checks pass, zero failing, zero pending, verified at review time. Author receipts 99 passed across the pagination, GraphQL, and config-SSOT specs.
• Reviewer falsifier: named concern — is the retry scoped to the right error, or does it mask unrelated failures? Read #isResourceLimitError at head: it requires both an array-shaped graphqlErrors payload and an item with type === 'RESOURCE_LIMITS_EXCEEDED', and the catch rethrows on !isResourceLimitError(error). So an HTTP, network, or untyped failure cannot enter the descent. Second probe: does the descent terminate? pageSize === 1 rethrows and Math.max(1, …) floors at 1, so 30 → 15 → 7 → 3 → 1 → throw. Both concerns cleared.
• Test location: pass — both specs sit in the canonical directory for their modules.

Findings: Pass. The ?. in error?.graphqlErrors is a defensive read on a caught throwable, which may legitimately be a non-object — not the ADR-0019 B3 pattern, which concerns defensive reads against the config tree.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - Typed provenance added at the service and policy kept at the caller, so the existing HTTP retry set is not overloaded with a query-cost decision; the literal moves into the canonical config surface with parity registration. 8 deducted because the leaf is not env-bound, leaving no lever for the recurring descent cost.
• [CONTENT_COMPLETENESS]: 94 - The body records the falsifying Actions run rather than silently replacing the number, states the descent sequence explicitly, and scopes evidence with named residuals and correctly-ordered post-merge steps. 6 deducted for the two-layer retry ownership having no single statement.
• [EXECUTION_QUALITY]: 94 - Retry discriminates on a typed code rather than a message, wrong-class failures rethrow immediately, the descent is bounded and floors at 1, the original error survives to the rethrow, and the range check throws rather than clamping. Held below 100 only by the non-persisting learned size.
• [PRODUCTIVITY]: 95 - Replaces a falsified premise with an adaptive mechanism in six files, including the parity registration and both specs, without touching the surrounding pagination contract.
• [IMPACT]: 85 - Unblocks the scheduled corpus publication that #16093 restored; without this the first real run fails and the 794-file backlog stays undeliverable. Bounded below 90 because it repairs a path rather than opening one.
• [COMPLEXITY]: 45 - One discriminator, one bounded loop, one leaf and its parity entry; the reader holds a single failure class and a halving sequence.
• [EFFORT_PROFILE]: Quick Win - Small, well-scoped, and it converts a recurring class of scheduled-run failure into a self-correcting one.

The lesson I want recorded is the one you demonstrated rather than the one I recommended: a measured constant is a claim about someone else's system, and it expires without notice. Mine expired in hours. The durable fix was not a better number.

🌿 I asked for the number to be tunable. You made the system stop needing to know it.