PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe as delivered — all nine ACs are met with deterministic test mapping and exact-head CI is green. The one real hazard is cross-PR coordination (sibling #15747's 200-user ceiling vs this PR's deliberate --limit=800 preservation), which both bodies already name and which #15746's Out of Scope explicitly excludes from this ticket — not an in-place defect, so no Request Changes; and no correctness is deferred, so no Follow-Up.

Peer-Review Opening: Thanks Emmy — this is a clean extraction of the Git state machine out of YAML into a testable module, with the #13798 SyncService precedent adapted semantically rather than coupled mechanically. Approving; the notes below are coordination-facing, not code-facing.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #15746 (Contract Ledger, 9 ACs, avoided traps), changed-file list, origin/dev workflow (inline commit-rebase-push), ai/services/github-workflow/SyncService.mjs L290–350 (autoPushGeneratedContent bounded recovery + recoverGeneratedContentCheckout), the ADR-0004 citation, sibling #15745 / PR #15747, and a Memory-Core sweep (no prior settled shape).
• Expected Solution Shape: Extract the mutation-sensitive publish path into a spawn-boundaried .mjs; capture the emission base SHA; freshness checks after emission, after staging, and pre-push; on advance, reset to remote authority and rerun the complete emission once; bounded attempts with clean-runner failure; allowlisted staging expressed as data; never rebase, never force-push; deterministic temporary-repository harness. This must NOT hardcode the allowlist inline in shell or bury the attempt bound in YAML.
• Patch Verdict: Matches. Verified at exact head e762808a60: restore path is fetch → reset --hard origin/dev → clean -fd on the allowlist; assertGeneratedOnly runs after the allowlisted git add; push rejection is classified by a post-hoc fetch (stale → bounded retry, foreign failure → loud throw with cause). git check-ignore confirms none of the generated paths are ignored, so clean -fd / add -A semantics hold; no nested files exist under apps/devindex/resources/data/ on dev, so the flat-JS-regex vs recursive-git-glob asymmetry is latent-only and fails loud in the safe direction.
• Premise Coherence: Coheres — friction→gold (a 2-of-100-runs conflict census converted into a deterministic regeneration contract that operationalizes ADR-0004's regeneratable-cache principle) and verify-before-assert (red-first import failure, real-git harness instead of a mocked race).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15746
• Related Graph Nodes: Related: #15744 (identity sibling), #15745 / PR #15747 (budget sibling — reconciliation duty), #13798 (SyncService recovery precedent), #9869 (unified pipeline origin), #13260 (derived-content coupling precedent), #15130 (adjacent, ticketed out of scope). Origin session 72bb1088-8ed5-48b7-a835-c288cf30e814.

🔬 Depth Floor

Challenge (cross-PR unverified assumption, non-blocking): emissionCommands hardcodes --limit=800. If this PR lands after #15747, it silently re-introduces the 800 ceiling that #15747's AC5 removes — the reconciliation duty is named in both PR bodies but enforced by no mechanism, and both PRs also edit Orchestrator.md's pipeline section, so the second-lander will textually conflict there as well. Recommendation: whichever sibling lands second rebases and — if that is this PR — flips the constant to 200 in the same pass; a one-line comment marking the constant as sibling-gated would make the duty visible at the edit site.

Non-blocking observations: (1) the NEO_SKIP_TICKET_ARCHAEOLOGY rationale comment from the old YAML was not carried to the module's commit call site — the exemption now appears without its why; (2) the push-rejected path fetches twice (fetchRemoteDev then restoreRemoteDev's internal fetch) — harmless on a disposable runner.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "bounded Data Sync regeneration" matches the diff; "deliberately preserves the current Updater ceiling of 800" verified literally true in emissionCommands (and is itself the hazard named above)
• Anchor & Echo summaries: precise @summary mechanism language throughout the module
• [RETROSPECTIVE] tag: none used — N/A
• Linked anchors: the SyncService.mjs precedent cited at L298–347 verified real (autoPushGeneratedContent, bounded at maxAttempts = 2, reset-and-re-emit recovery)

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None observed — the author correctly distinguished the Actions substrate (ephemeral runner, HEAD:dev push) from the SyncService substrate and adapted rather than copied.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The durable win is the extraction pattern: YAML owns scheduling and permissions, a spawn-boundaried module owns the mutation-sensitive Git state machine, and a temporary-repository harness makes an inherently racy production contract deterministic in CI. The #13798 precedent was adapted semantically (bounded discard-and-re-emit) without mechanical coupling — exactly the ticket's guidance.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15746 — newline-isolated at body top; sole commit e762808a60 subject carries (#15746) with an empty body
• #15746 labels are bug, ai, regression, architecture, build — confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (3 rows)
• Implemented diff matches the ledger: publish path is now verify-head / discard-and-re-emit-once ✓; explicit base/current SHA guard before publication ✓; SyncService precedent shipped as a substrate-appropriate adapter (spawn boundary, no rebase) ✓

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line (L3 deterministic temporary-repository integration coverage … Residual: none for the ticket acceptance criteria)
• Achieved L3 delivers AC9's mandated deterministic harness pre-merge; the scheduled-run observation is correctly declared non-closing (never-reopen contract preserved)
• Two-ceiling distinction: sandbox-achievable contract vs post-merge operational witness is explicit
• Evidence-class collapse check: no promotion of L1/L2 to L3/L4 framing
• Deployment causality: no external/runtime receipt gates the merge

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

• Consumer sweep: only RebuildContentIndexesAndSeo.spec.mjs asserted the old inline workflow shape — updated in this PR ✓; the LabelService.spec.mjs mention is comment-only ✓
• Orchestrator.md rewritten to the delegated shape, including fixing the stale devindex-pipeline.yml filename (pre-#9869 name); OptIn.md:76 still carries the same stale filename — pre-existing drift this PR did not create and its claims do not cover; a one-token drive-by fix is welcome here or in a later hygiene pass
• No skill file, AGENTS_STARTUP.md, or MCP surface touched

Findings: no in-scope integration gaps; one pre-existing adjacent doc nit noted.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at e762808a60 (unit, integration-unified, CodeQL, lint suites all SUCCESS); the new spec runs real git against temporary bare/runner/peer repositories (unchanged-dev one-shot; stale → discard-and-re-emit with command-log assertions of no rebase and no --force*; double-advance clean-fail with runner left at origin/dev; allowlist staging against an unrelated tracked file; workflow-delegation text test)
• Reviewer falsifier: N/A — static checks performed instead (git check-ignore, nested-tree census, consumer grep)
• Test location: test/playwright/unit/ai/buildScripts/ beside the existing docs/ sibling convention ✓

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 95 — YAML keeps scheduling/permissions, the module owns the Git state machine, the allowlist is exported data, and the execute/emit/log injection boundaries are clean; −5 for the allowlist truth living in two forms (git pathspec glob + JS regex predicate) that can drift (recursive vs flat), mitigated by the loud-and-safe assert direction.
• [CONTENT_COMPLETENESS]: 95 — @summary JSDoc on every function plus the module, an accurate guide rewrite, and a fat-ticket body; −5 for dropping the NEO_SKIP_TICKET_ARCHAEOLOGY rationale comment at the commit call site.
• [EXECUTION_QUALITY]: 95 — deterministic real-git harness, command-log no-rebase/no-force assertions, correct push-failure taxonomy (stale vs foreign), green exact-head CI; −5 for the redundant double fetch in the push-rejected path.
• [PRODUCTIVITY]: 100 — all nine ACs delivered and test-mapped (actively re-checked each against the spec file); the PMV observation is correctly non-closing.
• [IMPACT]: 70 — eliminates the dev-advanced rebase-conflict failure class (2 of the censused 100 runs) and converts an opaque YAML failure into explicit attempt/SHA diagnostics; operational resilience, not core architecture.
• [COMPLEXITY]: 65 — a four-checkpoint Git state machine plus retry boundary; the 462-line module is linear, and the temp-repo harness adds the reader load.
• [EFFORT_PROFILE]: Heavy Lift — new module, workflow surgery, and a fresh integration harness on the live hourly pipeline.

Two merge-gate notes for the human merger, not the author: (1) reviewRequests still holds neo-opus-ada's slot — per §10.1 it needs disposal (formal review, step-out, or unrequest) before strict merge-readiness; (2) the sibling reconciliation against #15747 (the 200 ceiling inside emissionCommands, plus the Orchestrator.md overlap) belongs to whichever PR lands second — tracked in both bodies and the A2A thread.