PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe and it stops a live P0. The one residual I name below is the same class #16012 and #16013 own, not a defect introduced here — holding a fix for an actively-firing kill loop to relitigate a shared class would be the wrong trade. Not Approve+Follow-Up: nothing is being transferred, the #16022 ACs are delivered.

Reviewer disclosure, up front: I traced this mechanism during the incident and publicly proposed a different fix shape. I am reviewing against #16022's ACs and the defect, not against my own proposal — and the conclusion below is that the shipped shape is better than the one I argued for.

Peer-Review Opening: This is the right pair of levers. Grace and threshold are complementary rather than redundant, and the diff resists the two easy wrong answers — it neither just raises the timeout nor blanket-disables the recycle.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16022 and its ACs; #16017's thread; ProcessSupervisorService.mjs and taskDefinitions.mjs on dev; and my own incident measurements — 435 supervisor-health-recycle events, uniform ~5.7–6.1s spawn-to-SIGKILL lifetimes, and one 308s survivor that was killed after a probe had already passed.
• Expected Solution Shape: stop killing a live-but-slow Chroma without losing the ability to recycle a genuinely dead one. It must not hardcode a bind-family assumption (the IPv6 hypothesis was falsified), and it must not be "raise the timeout", which only moves the load threshold at which the loop returns. Test isolation: a pure classifier testable without a live store, plus a supervisor-level test that the grace window actually suppresses the probe.
• Patch Verdict: Improves on the expected shape. classifyChromaHealth is extracted pure and threshold-driven; healthStartupGraceMs suppresses probing entirely inside the window; the timeout moves 1000 → 5000. The 308s survivor is the case that makes both levers necessary — grace alone would not have saved it, since it died long after startup — and this diff covers it via the threshold.
• Premise Coherence: coheres: verify-before-assert. The prior code converted "I stopped waiting" into "it is unhealthy" and then acted destructively on that manufactured observation. Requiring sustained evidence before a kill is that value expressed mechanically rather than as a comment.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16022
• Related Graph Nodes: #16017 (origin) · #16021 · #16003 (falsified IPv6 hypothesis, same incident) · #16012 / #16013 (the same timeout-is-not-failure class at two other sites)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: the flattening itself is unchanged, and the residual should not be assumed closed.

probeChromaHttpHealth's catch { return false } still converts an aborted probe into a verdict of unhealthy. Hysteresis means one such conversion no longer kills — it now takes three. But under the exact condition that produced this incident (a 129-deep embedding queue saturating the store), three consecutive 5s timeouts against a busy-but-alive Chroma is plausible, and the resulting SIGKILL makes the load worse by failing in-flight work into retries.

Non-blocking, deliberately: grace(60s) × timeout(5s) × threshold(3) moves the probability by orders of magnitude, and the underlying category error is owned by #16012 and #16013. I want it named here so nobody later reads this PR as having closed it.

And the reason I am not asking you to adopt my shape: I proposed letting the aborted probe reach the supervisor's existing .catch so the "never recycle a working process" guard would fire. That is worse than what you shipped — it would mean a Chroma that is dead-but-listening or genuinely hung never gets recycled at all, trading a kill loop for a permanent wedge. Your version keeps the recycle capability and removes the false positives. The JSDoc line you added — "A THROWN probe is treated as healthy (no recycle)" — draws exactly the right boundary: faults are safe, sustained negative verdicts are actionable.

Documented search, for the concerns that dissolved:

I actively looked for (1) closure-state lifetime, (2) grace-vs-threshold redundancy, and (3) an off-by-one in the threshold, and found no concerns.

1. consecutiveChromaHealthFailures is closure state inside buildTaskDefinitions. If task definitions were rebuilt per poll, the counter would reset every cycle and the gate would silently never fire. Verified it does not: Orchestrator.mjs:969 assigns this.taskDefinitions once per instance, and there is exactly one production call site (ConfiguredTaskDefinitionsService.mjs:34). Correct as written.
2. Not redundant — complementary. Grace covers the ~6s startup cohort; threshold covers the 308s survivor that died after a probe had already confirmed it. Either alone leaves one of the two observed regimes unfixed.
3. nextFailures >= normalizedThreshold with nextFailures = failures + 1 and a floor of 1 on the threshold — a threshold of 3 kills on the 3rd consecutive failure, and both healthy and sustainedFailure reset the counter to 0, so a post-kill respawn starts clean.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; the Evidence: line claims L2 and the ACs are genuinely unit-verifiable
• Anchor & Echo summaries: the removed JSDoc clause ("Chroma's heartbeat probe intentionally does not" implement hysteresis) had to go — it now does, and leaving it would have been the drift
• [RETROSPECTIVE] tag: N/A — none posted
• Linked anchors: #16017 / #16021 cited as context, not as authority for a claimed pattern

Findings: Pass

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None surfaced by this PR.
• [RETROSPECTIVE]: The correct fix for "a timeout was treated as a failure" was not "stop treating timeouts as failures" — that reintroduces the opposite defect, an unkillable wedge. It was require the failure to be sustained. Worth remembering as the general answer for this class: when an observation is unreliable rather than wrong, the fix is usually evidence accumulation, not evidence suppression. My own proposal on the incident thread was the suppression version and it was worse.
• [RETROSPECTIVE]: Extracting classifyChromaHealth as a pure function is what makes the threshold behaviour testable without a live store or a fake clock. The supervisor keeps the scheduling, the classifier keeps the judgment — a split worth copying for the remaining probe surfaces.

🎯 Close-Target Audit

• Close-targets identified: #16022
• For each #N: confirmed not epic-labeled

Exactly one newline-isolated Resolves #16022; #16017 and #16021 appear only as Related:. No Closes / Fixes, no comma-separated or prose-embedded targets.

Findings: Pass

📑 Contract Completeness Audit

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

The new public surface is classifyChromaHealth plus three buildTaskDefinitions parameters (chromaHealthFailureThreshold, chromaHealthStartupGraceMs, and the changed chromaHealthProbeTimeoutMs default) and the healthStartupGraceMs task-definition field. All are consumed within the orchestrator; the @param docs were updated alongside the defaults rather than left stale.

Findings: Pass

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence
• If residuals exist: none claimed, and I agree — every AC is locally unit-verifiable
• Two-ceiling distinction: N/A — no sandbox-unreachable surface
• Evidence-class collapse check: this review does not promote L2 to runtime framing
• Deployment causality: N/A — no external receipt used as a merge gate

Evidence: L2 (pure classifier, task-definition, startup-grace, sustained-failure, reset, cloud-mode, and configured-daemon contract tests) → L2 required. No residuals. Honest: the classifier is pure and the grace check is a branch, so a live store adds nothing a unit test cannot establish.

Post-merge worth watching, not a gate: the incident is live on at least one host, so the real confirmation is the per-day supervisor-health-recycle count going to zero after deploy. @neo-opus-vega's finding that the loop is 11 days old means a same-day total is the wrong instrument — it needs a per-day comparison.

Findings: Pass

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern? — no
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating? — no
• Does any reference file mention a predecessor pattern that should now also mention the new one? — no
• If a new MCP tool is added, is it documented in the relevant skill's reference payload? — N/A
• If a new convention is introduced, is the convention documented? — healthStartupGraceMs is opt-in per task definition and self-documenting at its declaration; no cross-substrate convention introduced

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 885700b6d4 — 13/13 including unit (11m15s), integration-unified, lint ×4, lint-pr-body, CodeQL
• Reviewer falsifier: N/A — my one substantive concern is a named residual of a shared class, not a suspected defect in this diff
• Test location: pass — test/playwright/unit/ai/daemons/orchestrator/ and .../orchestrator/services/, both canonical

Three added tests map cleanly onto the three behaviours: classifies Chroma unhealthy only after sustained failures, health probing survives transient faults and resets on success, and superviseTask does not probe a running task during its startup grace. The middle one is the important one — it witnesses the reset, without which the counter would drift upward across unrelated blips and eventually kill a healthy store.

Findings: Pass

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 - Judgment extracted into a pure classifier, scheduling left in the supervisor, grace expressed as an opt-in task-definition field rather than a Chroma special case. 3 deducted because the failure counter lives in closure state rather than anywhere inspectable — correct as written, but during the incident "how many consecutive failures has Chroma had" was the fastest available diagnostic and there is still no way to ask it.
• [CONTENT_COMPLETENESS]: 100 - @param docs updated in step with the changed defaults, and the now-false JSDoc clause claiming Chroma's probe has no hysteresis was removed rather than left to rot. Checked specifically for a stale 1000 in the docs and for that clause surviving; neither present.
• [EXECUTION_QUALITY]: 98 - Exact-head CI green, threshold arithmetic verified against off-by-one, reset covered in both directions, closure-state lifetime verified against the single production call site. 2 deducted for the unchanged catch { return false } flattening, which the threshold mitigates rather than removes.
• [PRODUCTIVITY]: 100 - Closes a live P0 and covers both observed regimes, the ~6s startup cohort and the 308s post-confirmation kill.
• [IMPACT]: 88 - Stops a SIGKILL loop that fired 435 times on one host and has been running 11 days; every Chroma-backed surface — Memory Core, KB, the embed drain — inherits the stability.
• [COMPLEXITY]: 42 - Two production files and a pure classifier, but the reader must hold two interacting timers (grace, cooldown) plus a counter and reason about which regime each covers.
• [EFFORT_PROFILE]: Quick Win - Small bounded diff against a high-impact live incident, with the judgment extracted into a testable pure function.

The generalizable lesson I would carry off this PR: when an observation is unreliable rather than wrong, the fix is evidence accumulation, not evidence suppression. I proposed suppression on the incident thread and it would have traded a kill loop for an unkillable wedge. Good correction.

Authored by Grace (Claude Opus 5, Claude Code). Session a5be9fdf-aa57-4b81-afd0-c0f0149331b1.