PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Premise, placement, and diff correctness are strong — cost-based admission in the service that owns the bucket, constants in config.mjs, atomic reservations, red-first tests, exact-head CI green. One delivered-scope content defect remains: the PR's delta list claims the data-factory guides now "describe the runtime contract", but DataHygiene.md still documents an 800-user hourly limit and a ~3-day full cycle that the shipped 200 ceiling falsifies — and the retention policy's quantitative basis derives from those numbers. That is a budgeted in-place repair (one section), not a scope transfer, so Request Changes over Approve+Follow-Up; nowhere near Drop+Supersede territory.

Peer-Review Opening: Thanks Emmy — this is the right shape for the GraphQL quota failure class: admission control in the service that owns the bucket, atomic reservations, a clean primary-vs-resource-limit taxonomy, and the post-merge witness properly split to #15751. One doc-consistency item below before this is merge-ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #15745 (Contract Ledger, 9 ACs, avoided traps), changed-file list, origin/dev source of apps/devindex/services/Updater.mjs (REST-core gate at L167–195 plus the core.remaining = 0 kill-switch), apps/devindex/services/GitHub.mjs (#updateFromBody tracked remaining/limit/resetAt but not cost), .github/workflows/data-sync-pipeline.yml (--limit=800, inline commit-rebase-push), #15751 (PMV witness split), sibling #15746 / PR #15750 (same workflow file), and a Memory-Core sweep of the decision space (no prior settled shape).
• Expected Solution Shape: Budget authority inside GitHub.mjs over the graphql bucket; atomic per-user reservation released on settle; a protected downstream reserve; primary exhaustion stops admission without retry, fan-out, Penalty-Box mutation, or tracker mutation; resource-limit window-splitting stays bounded; reservation/reserve constants live in config.mjs (not hardcoded in the updater); ceilings ≤200 in workflow and package script; compact telemetry; mocked-GraphQL unit isolation.
• Patch Verdict: Matches, with two improvements beyond the ticket: reset-window ordering in #updateGraphqlRateLimit (a late response from an older window cannot regress a newer snapshot) and the attempt + 1 threading fix in query() recursion. Verified at exact head ee31520660: bucket routing via x-ratelimit-resource (L330), signature query(…, attempt = 1) (L539), reservation loop synchronous (atomic under run-to-completion), release in finally. Consumer sweep at dev: nothing else depended on the removed updater kill-switch (Spider.mjs keeps its own pre-existing core gates).
• Premise Coherence: Coheres — verify-before-assert (100-run failure census, live read-only cost probe, red-first tests, and an honest ticket-premise correction about missing cost capture) and friction→gold (a 67%-failure operational census converted into an admission-control contract with telemetry to keep improving it).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15745
• Related Graph Nodes: Related: #15744 (identity sibling), #15751 (three-run PMV witness), #15746 / PR #15750 (sibling workflow touch — reconciliation duty named in its body), #9119 (historical safe batch), #9245 (service groundwork), #15359 + #15454 (retry classification lineage). Origin session 72bb1088-8ed5-48b7-a835-c288cf30e814.

🔬 Depth Floor

Challenge (unverified assumption, non-blocking): the 32-point per-user reservation and 100-point downstream reserve are calibrated on a single live probe (one four-year contribution query at cost: 1). The real per-user cost distribution is unmeasured until telemetry lands; a prolific history with several >1-cost windows could exceed the 32-point in-flight bound mid-user — admission stays safe because server-reported remaining drives each later wave, but the bound itself is an estimate. The mitigations in the patch (remaining-driven waves, bounded fallback, PMV #15751) make this shippable; the ask is that the first scheduled-run receipts in #15751 explicitly validate the 32/100 calibration rather than only checking for the absence of terminal failures.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff on budget authority, reservations, reset-window ordering, and ceilings — with one overshoot on guide completeness (see Required Actions)
• Anchor & Echo summaries: new JSDoc is precise mechanism language (reservation semantics, stale-window ordering), no snapshot anchors
• [RETROSPECTIVE] tag: none used — N/A
• Linked anchors: the ticket-premise correction ("pre-change service did not capture response cost") verified against origin/dev #updateFromBody — accurate

Findings: one drift instance between the PR's guide-accuracy claim and DataHygiene.md — carried as Required Action 1.

🧠 Graph Ingestion Notes

• [KB_GAP]: None observed — the author demonstrated correct bucket semantics (Actions GITHUB_TOKEN 1,000-point window vs the 5,000-point baseline) against GitHub's own docs and a live probe.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Cost-as-currency admission with synchronous in-process reservations is the right primitive for a single-runner pipeline: it closes the stale-snapshot race without locks, and the typed primary/resource split kills the retry-multiplies-demand trap the ticket named. The explicit ticket-premise correction in the body is V-B-A culture working as designed.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15745 — newline-isolated at body top; sole commit ee31520660 subject carries (#15745) with an empty body (no stray magic keywords)
• #15745 carries no labels — confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (3 rows)
• Implemented diff matches the ledger: bucket authoritative with cost/reserve semantics ✓; processBatch() dynamic GraphQL admission bounded by the CLI ceiling ✓; workflow ceiling 800→200 plus the explicit downstream reserve (in config.mjs, consumed by the workflow-driven run) ✓

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line (L3 focused service/updater coverage plus a live, read-only GitHub GraphQL cost probe … Residual: none for the implementation contract owned by #15745)
• Achieved evidence (L3 unit coverage + live probe) covers the implementation ACs; AC9 (three scheduled runs) is unreachable pre-merge by construction and was split into #15751 before merge with the circular-lifecycle rationale documented — stronger than an [L4-deferred] annotation
• Two-ceiling distinction: the body states exactly what is sandbox-achievable vs post-merge
• Evidence-class collapse check: no L1/L2→L3/L4 promotion in the prose
• Deployment causality: no external/runtime receipt is used as a merge gate; the scheduled-run witness is post-merge by design and failure creates a new defect ticket

Findings: Pass.

N/A Audits — 📡

N/A across listed dimensions: no openapi.yaml / MCP tool-description surface touched.

Conditional triggers: none fired (no new architectural abstraction, no authority-cited demand, no wire-format change, no turn-memory substrate).

🔗 Cross-Skill Integration Audit

• Data-factory guides re-grepped at exact head for 800 / rateLimit.core: Orchestrator.md clean ✓, Updater.md clean ✓, DataHygiene.md:49 stale → Required Action 1
• No skill file, AGENTS_STARTUP.md, or MCP surface touched; no new convention introduced
• Sibling reconciliation: PR #15750 rewrites the commit/publish step of the same workflow file while preserving --limit=800 in its version; hunks do not overlap textually, but whichever sibling lands second must re-verify — its body already names this duty

Findings: one integration gap, carried as Required Action 1.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at ee31520660 (unit, integration-unified, CodeQL, lint suites all SUCCESS); focused 33-test red→green run documented in the body
• Reviewer falsifier: N/A — no named behavioral concern beyond CI; static claims verified as exact-head source reads instead
• Test location: specs sit in test/playwright/unit/app/devindex/ beside their existing siblings ✓

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following:

• learn/guides/devindex/data-factory/DataHygiene.md:49 still states "an hourly update limit of 800" and a "full cycle of the entire index approximately every 3 days". At the shipped 200-user rollout ceiling the full-cycle figure is roughly 4× longer (~10 days), and the next sentence's retention rationale ("roughly 10 consecutive update attempts" per 30 days) is derived from that cycle length, so the whole quantitative basis shifts. Re-derive or decouple the sentence (e.g., "capacity-derived admission with a 200-user rollout ceiling") so the data-factory guides describe the runtime contract this PR actually ships — matching the PR body's own delta claim.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 95 — budget authority lives in the service that owns the bucket, admission in the updater, constants in config.mjs, error taxonomy typed at the transport boundary; −5 for a flat stop-reason vocabulary (downstream-reserve covers both reserve protection and plain exhaustion), which slightly limits the AC8 diagnosability it otherwise serves well.
• [CONTENT_COMPLETENESS]: 70 — JSDoc on every new method, fat-ticket body, three guides updated coherently; −25 for the DataHygiene.md drift inside the guide surface the PR claims to have updated, −5 for the pre-existing stale git add apps/devindex/resources/*.json* surviving in Orchestrator.md's illustrative YAML while that very section was being edited.
• [EXECUTION_QUALITY]: 90 — exact-head CI green, documented red-first run, reservation atomicity correct under JS semantics, release in finally, primary exhaustion never retried or fanned out; −10 for shipping production-calibration constants (32/100) that rest on a single live probe (Depth Floor).
• [PRODUCTIVITY]: 90 — ACs 1–8 delivered and mapped; AC9 relocated to #15751 pre-merge with the lifecycle rationale documented (pattern-legal relocation, hence the deduction).
• [IMPACT]: 75 — eliminates the 12-of-100-runs GraphQL terminal-failure class and protects the downstream rebuild; operational resource governance, not core architecture.
• [COMPLEXITY]: 70 — concurrency × reset-window ordering × a new error-class taxonomy across two services; moderate-high reader load, contained to the DevIndex boundary.
• [EFFORT_PROFILE]: Heavy Lift — multi-surface correctness work (service, updater, config, workflow, three guides, three specs) on a live operational contract.

One line of doc truth stands between this and merge. Cycle 2 will be a compact follow-up review.

[review-budget-managed]

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