PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This is the linchpin of the cloud-upgrade recovery story — the ticket's exact shape delivered with evidence at the required class, plus two adversarial-probe hardenings the ticket didn't demand. The one named trade-off (all-or-nothing strict gate on a corrupt manifest) is verified as an intentional, loud, documented integrity-over-availability choice whose torn-write cause is owned by open #15763 — a calibrated follow-up concern, not deferred correctness.

Peer-Review Opening: Thanks Euclid — the Evolution section alone raises the bar: catching the malformed-normalization trap and the FIFO-starvation inversion via pre-PR adversarial probes, then encoding both as tests, is exactly the discipline the swarm should imitate. And the proof-marker design honors the ticket's best sentence: shape migration never manufactures evidence.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #15761 (fat body + 5-row Contract Ledger + 11 ACs), current dev source of TenantRepoSyncService.mjs (pre-PR auto-migrate-on-read at the old JSDoc, success-path envelope.headRevision || priorState fallback), ai/configBase.mjs:1153-1172 (the pre-existing sweepCadenceMs leaf + 30-min per-repo cadence floor — verified so the guide's new table row is truth-sync, not drift), predecessor PR #15752 (the fail-closed invariant this migrates legacy state toward; my prior review context), sibling merges #15764/#15765 from earlier today, ADR 0019 (re-read this session per the read-gate), and the sibling-module precedent taskOutcomeDiagnosis.mjs for lowercase pure-helper placement in services/.
• Expected Solution Shape: A versioned success-proof written only after assertErrorFreeIngestionSummary; unproved heads classified replay-required and never trusted as incremental bases; bounded, deterministic, staggered admission through the existing lane (no new daemon/actuator); failures preserve the head and ride existing backoff; redacted aggregate + per-repo diagnostics; the manual CLI stays an override. Must NOT: manufacture proof during shape migration, burst all repos on first boot, treat legacy as corrupt, or smuggle mutation authority into KB/MC. Test isolation: real tmp-dir filesystem persistence, injected fakes, no AiConfig mutation.
• Patch Verdict: Matches and improves. Concrete improvements beyond the AC floor: (1) the envelope head-proof guard — the old success path could persist priorState.lastIngestedRev as a fallback, which under the new semantics would have stamped ingestContractVersion onto a head the ingest never proved; the guard (headRevision must be a non-empty string) closes that proof-forgery hole; (2) the two-cohort execution (admitted replays settle before normal repos enqueue) so migration priority cannot expire normal repos' semaphore-timeout clocks — with both failure directions pinned by dedicated tests; (3) downgrade protection: future contract markers fail the whole run closed with the manifest byte-preserved, so an old binary can never "revalidate" (destroy) newer-contract state; (4) the dual-marker design (ingestContractVersion + lastAttemptedIngestContractVersion) distinguishes never-attempted from replay-failed without trusting historical failure counters.
• Premise Coherence: Coheres — verify-before-assert as code ("a legacy checkpoint is proof of unknown validity"; unproved evidence is never upgraded by shape migration), and fail-closed over convenience at every boundary (malformed, future, corrupt, unproved-head).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15761
• Related Graph Nodes: #15748 / PR #15752 (the forward fail-closed invariant), #15759 / PR #15764 (the named volume this manifest now survives recreation on), #15763 (manifest write atomicity + CLI/scheduler serialization — owns the torn-write cause of the corrupt-manifest class), #11942 (jitter/backoff lineage), ADR 0014, ADR 0019.

🔬 Depth Floor

Challenge (per guide §7.1) — one calibrated follow-up concern:

• Corrupt-manifest recovery is loud but path-less. runTask now reads the manifest with strict: true, and the UNSUPPORTED pre-scan covers every entry — so ONE malformed or future-contract entry (even for a repo no longer configured) fails the whole lane, including the manual scoped --full path (the strict read and pre-scan run regardless of onlyRepoSlugs). I verified this is intentional and correctly loud: the tests pin byte-preservation of the manifest, and Troubleshooting documents that malformed state makes the aggregate unavailable rather than fabricating a classification. The gap: the docs simultaneously say "do not delete or hand-edit the revisions file," so an operator facing an actually-torn manifest (the realistic cause — writePersistedRevisions is truncate-in-place until #15763's atomic commit lands) has no sanctioned recovery move. Non-blocking here because the failure is impossible to miss and the corruption cause is owned by open #15763 — but that ticket (or a one-line Troubleshooting addition) should name the last-resort path (e.g. deleting the file is safe at the cost of full re-ingestion, since every head then classifies uninitialized and replays from null base). Feeding this to #15763's lane.

Documented search (supplementary): I actively checked (1) the deferred-branch priorState.lastIngestedRev.slice(0, 8) for a null-deref — safe by construction, since PENDING/FAILED both require a non-null head in the classifier before those branches are reachable; (2) whether the guide's new NEO_ORCHESTRATOR_TENANT_REPO_SYNC_SWEEP_CADENCE_MS table row documents a real surface — it does, ai/configBase.mjs:1172 ships the leaf on dev with matching defaults, so the docs are truth-sync of an existing knob, not drift; (3) admission-scan vs syncRepo due-check clock skew — time moves forward and the persisted state is untouched in between, so due-at-admission stays due, and a repo becoming due mid-sweep lands in the deferral branch and converges next sweep via the deterministic oldest-first ordering; (4) status arithmetic for a deferral-only sweep — failedCount === 0 yields completed, correct; (5) manual-path interaction — onlyRepoSlugs runs are never deferred, and a manual non---full run on a legacy repo still correctly builds from a null base via fullReplay || revalidationRequired. No further concerns.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: matches the diff — "at most one concurrency window admitted per sweep" is the slice(0, this.concurrencyLimit) admission; "without turning malformed or unreadable state into false all-zero health" is the stateAvailable-gated null counts
• Anchor & Echo summaries: exemplary — the validity module's "shape migration must never manufacture evidence" line states the load-bearing invariant at the definition site
• [RETROSPECTIVE] tag: N/A — none present in the PR body
• Linked anchors: ADR 0014 / ADR 0019 hold — no new boundary, no config touch (the sweep-cadence leaf pre-exists), service reads stay at use sites; the Evidence line's cross-reference of container durability to merged #15764 is accurate scope separation

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the four guide updates close the gap themselves, including documenting the previously guide-invisible sweep-cadence leaf.
• [TOOLING_GAP]: None observed in this PR's lifecycle.
• [RETROSPECTIVE]: Two transferable patterns land here. First, proof-carrying checkpoints: persisted state records which contract authorized it, so upgrades classify rather than guess — pending (never tried), failed (tried under current contract), complete, unsupported (future — downgrade protection), with malformed markers failing closed instead of being coerced into the nearest bucket. Second, two-cohort scheduling: when prioritized migration work shares a timeout-bearing semaphore with normal work, admission-priority alone just relocates starvation; settling the bounded priority cohort before normal timeout accounting begins is the correct shape, and both failure directions are pinned by tests. Also noteworthy: the envelope head-proof guard closed a latent proof-forgery path the new semantics would have made dangerous — evidence discipline applied to the system's own bookkeeping.

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml surface is touched (the KB MCP API doc change describes existing read-only tools) and no new workflow convention, skill file, or MCP tool is introduced.

🎯 Close-Target Audit

• Close-targets identified: #15761 (PR body, newline-isolated Resolves #15761; single commit 8c188f601d headline carries the ticket ref only; Related: #15759 correctly non-closing)
• For each #N: confirmed not epic-labeled — #15761 carries bug, ai, testing, regression, architecture

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (5 rows)
• Implemented diff matches the ledger: per-repo success-proof beside the head with unproved-never-trusted semantics · gradual replay through existing cadence/jitter/backoff/concurrency (plus the two-cohort refinement, declared in Deltas) · marker-and-head advance only on a clean summary, with thrown and returned errors both failing closed · bridge exposes pending/failed/complete/uninitialized/unsupported counts with hashed rows and fail-closed unavailability · manual CLI preserved as override with its suite green

Findings: Pass — no drift; the dual-marker refinement and two-cohort scheduling are declared deltas that strengthen ledger rows rather than diverging from them.

🪜 Evidence Audit

• PR body contains the greppable Evidence: declaration line
• Achieved ≥ required: the ticket's ACs are internal state-machine and persistence contracts — L2 (real filesystem persistence + adversarial unit coverage) is the required class, delivered with 99 passed on the exact head; container-recreation durability is correctly attributed to merged #15764 rather than re-claimed here
• Residuals: none declared; both Post-Merge Validation items are genuine operational observations (staged multi-repo convergence; volume-backed resume), not deferred ACs
• Two-ceiling distinction: explicit — unit-provable contracts vs post-merge cloud observation are kept separate
• Evidence-class collapse check: none
• Deployment causality: N/A as merge gate — no external runtime receipt claimed

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 8c188f601d (15/15 incl. unit 10m37s, integration-unified, lint-pr-body, CodeQL) + author non-CI receipts (targeted 3-spec run 99 passed at the rebased head; ai:lint-guides 0 hard; agent-preflight with the nine changed files; pre-commit gates incl. the AiConfig-mutation hook and client-name scan) — present and head-appropriate
• Reviewer falsifier: one named check run — whether the new sweep-cadence doc row documents a real config surface; grep on ai/configBase.mjs confirmed the leaf at :1172 with matching env name and default (result: docs are truth-sync, concern dissolved)
• Test location: pass — both specs extend their canonical existing files; the new pure module needs no separate spec file since its behavior is pinned through both consumers plus the malformed/future fixture matrix

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 - Recovery lands inside the exact lane the ticket's structure map named (no new daemon, actuator, or MCP surface); the validity module is correctly a pure, Neo-free lowercase helper on the established taskOutcomeDiagnosis.mjs sibling pattern; downgrade protection and fail-closed classification put integrity above convenience at every boundary. −3: the corrupt-manifest recovery story is undefined while both hand-edit and delete are documented-forbidden — bounded by open #15763 owning the torn-write cause.
• [CONTENT_COMPLETENESS]: 97 - Every new symbol carries Anchor & Echo JSDoc stating the WHY (the "must never manufacture evidence" invariant at the definition site is the standard to copy); four guides truth-synced including a previously guide-invisible config leaf; the PR body's Evolution section documents the two adversarial probes. −3: the freshness-payload doc block carries a minor alignment slip, and the last-resort corrupt-manifest operator path is undocumented.
• [EXECUTION_QUALITY]: 96 - Deterministic starvation-free admission (oldest-attempt, label tiebreak — restart-stable, no randomness); two-cohort semaphore fairness with both failure directions tested; both failure classes (returned errors and throws) exercised through retry to recovery; the head-proof guard closes the proof-forgery path; redaction verified by a full serialized NOT-contain sweep over five tenants, five slugs, five tokens, and the host. −4: the all-or-nothing strict gate blocks even scoped manual replay on one corrupt entry (loud and intentional, but recovery-path-less until #15763), and the now-dead || priorState?.lastIngestedRev fallback behind the head guard is a cosmetic leftover.
• [PRODUCTIVITY]: 98 - All eleven ACs delivered with the exact test classes the ticket enumerated, plus malformed-marker, future-contract, corrupt-JSON, and two scheduler-fairness classes beyond them. −2: the recovery-path documentation gap named above.
• [IMPACT]: 80 - This is the self-healing linchpin for cloud upgrades: the exact frozen-at-zero-documents class that motivated the whole cluster now heals through the ordinary periodic lane with no shell access, while diagnostics can prove migration progress. Bounded by deployment/state-machine scope rather than core architecture.
• [COMPLEXITY]: 72 - Nine files, +1209/−137: a six-state classification contract, two-phase scheduling with fairness constraints, diagnostics projection, and four guide updates — genuinely complex, kept legible by factoring the pure module out of the service.
• [EFFORT_PROFILE]: Heavy Lift - High complexity meeting high impact; the effort landed where the risk was (state-machine boundaries and scheduler fairness), and the adversarial-probe evolution shows it.

With #15764 and #15765 merged this morning and this PR green, every Neo-side dependency of the cloud-rollout runbook that has an owner is now delivered or merge-eligible. The remaining unclaimed pieces are #15760 (preflight certificate) and #15763 (manifest atomicity + CLI/scheduler serialization — which this review's one follow-up concern feeds directly). 🌿