fix(cron): retry ACP process death on the type, not the message wording

[SebastianYuSun]

Problem / Motivation

A cron job whose ACP child dies mid-turn is recorded as a hard failure rather than
retried, even though the failure handler contains a retry branch built for exactly
that case. The branch identifies process death by string-matching the exception
message for "not running" or "process exited", but the most common death is
discovered as a broken pipe on the next write and is worded "ACP process pipe broken: <cause>" — which matches neither substring.

So the retry that exists for this failure is skipped for the signature that
produces it most often. Observed shape on a real job: last_status: "error",
last_error: "ACP process pipe broken: Connection lost", on a job whose previous
run completed normally and whose host was not persistently broken.

Why it matters

consecutive_failures auto-pauses a job at _AUTO_PAUSE_THRESHOLD, and a paused
job never fires again. So repeated one-off child deaths durably disable a healthy
job instead of self-healing. The degradation is silent in the worst way: the retry
looks present in the code, so the behaviour reads as "ACP death is handled".

The generic transient ladder does not cover the gap either. AcpError.__init__
defaults transient=None and the raise site passes no transient= kwarg, so
acp_error_is_transient falls back to marker-matching a message that carries no
throttle or 5xx marker, and returns False. Both ladders miss, so control reaches
record_failure().

What changed (motivation → approach → change)

Symptom: a cron job accumulates hard failures for a recoverable transient.

Root cause: the retry predicate reads the exception's wording instead of its
type. AcpProcessDied is raised at every site that discovers a dead child, is a
subclass of AcpError, and is already imported in gateway.py — it was simply not
consulted.

Change: decide on a resubmit_safe flag the ACP layer sets at the raise site,
and keep the two legacy substrings as a fallback arm scoped to plain AcpError.

The full surface, declared.

1. slack/gateway.py — the cron retry predicate reads the exception's type plus
   resubmit_safe instead of its message wording.
2. acp/client.py — new public resubmit_safe attribute + __init__ kwarg on
   AcpProcessDied, mirroring the existing AcpError.transient precedent.
3. acp/client.py — _send_prompt re-tags a caught death as resubmit-safe.
4. acp/session_provider.py — the pre-conversation liveness check passes
   resubmit_safe=True.

The invariant is about WHEN the death was discovered, not which subsystem
noticed it. With a turn in flight a tool may already have completed its side
effects, so resubmitting the prompt can repeat a mutation.

The default REFUSES, and only two sites claim otherwise — each one where the
turn provably had not started:

site	turn in flight?	resubmit_safe
client.py _send_prompt	no — this write STARTS the turn	True (explicit)
session_provider.py runtime not alive	no — no conversation started yet	True (explicit)
every other AcpProcessDied construction	yes, or not established	False (default)

The claim is made at _send_prompt, not at the transport write, and that placement
is the whole point. A transport write cannot know turn state: the shared
_send_request also carries session/steer and commands/execute, and its siblings
_send_response / _send_error exist ONLY to answer requests the child raises
mid-turn — tool-permission replies and unknown-method rejections. Tagging the shared
write safe therefore mislabels the majority of its callers, and the concrete harm is a
replay: a cron turn completes tool 1, the agent requests permission for tool 2, the
child dies, the approval write raises a safe-tagged death, and the resubmitted prompt
runs tool 1 again. An earlier revision of this PR did exactly that; reviewers caught
it, and the fix was to move the claim rather than to defend the tag.

Why a drain() failure at _send_prompt implies nothing ran — this is not
obvious and was measured rather than assumed. write() never raises; the death always
surfaces at drain(); and drain() fails iff unflushed bytes remain (a child that
consumed the whole payload drains clean even if it dies immediately after). Wire
messages are newline-delimited JSON-RPC, so bytes still queued mean the child never
received a complete session/prompt line, and it cannot dispatch a tool for a request
it has not finished reading. Partial consumption is reachable — a child that reads
a prefix then dies does raise here — but a prefix is not a parseable request. The code
carries this reasoning, including the reachable case, so it does not read as a
counterexample later.

Defaulting to True and having each in-flight site opt out would cost about the same
in kwargs, but it is only correct while the set of in-flight sites is completely
enumerated — and that set is not locally checkable. Reviewers of earlier revisions
found in-flight sites that explicit audits had missed, including ones reachable only
through session_provider._translate_dead. Fail-closed removes the dependency on that
enumeration being exhaustive, so the tests pin the enumerable direction instead:
exactly which sites claim True.

The substring arm is scoped to plain AcpError, and that scoping is load-bearing.
Five raise sites spell process death as a plain AcpError, not AcpProcessDied
(AcpError("ACP process not running") for a missing stdin, and
AcpError(f"ACP process exited (code=...)")), so dropping the arm would silently
stop retrying all five and two existing tests pin that. But left unrestricted the arm
out-votes the type's own verdict: AcpProcessDied("Process exited during prompt ...")
is raised with a turn in flight, and its message CONTAINS process exited, so the
wording resurrected a death the classification had already refused. Restricting the
arm to non-AcpProcessDied errors is what makes a typed death decided solely by its
flag.

The narrower alternative — adding "pipe broken" to the substring list — closes
this instance but leaves the retry decision coupled to exception wording, which is
the defect class rather than the instance.

Known residual, deliberately out of scope. resubmit_safe is consulted at 1
of 3 resubmit sites. dashboard/chat_runner.py and task_executor.py also
resubmit on AcpProcessDied without reading it, so the mutation-repeat hazard
persists at both. That is pre-existing, in files this PR does not touch, and the
right non-resubmit behaviour differs for a foreground turn versus a cron wake — a
design question rather than a mechanical port. With the fail-closed default those
handlers now inherit a refusing verdict on any unclassified death rather than a
permissive one, so reading the flag is a strict improvement whenever they choose to.

Tests

test/test_cron_acp_retry.py. Verified load-bearing by reverting src/ to the
previous published head and re-running: the three tests below fail, naming the site.

Load-bearing:

• test_mid_prompt_death_is_not_retried_despite_legacy_wording — the death whose
  wording matched the legacy substring and out-voted its own resubmit_safe=False.
  Asserts the stream is entered exactly once. This is the test for the defect a
  reviewer found in the previous revision, and it fails on that revision.
• test_only_the_audited_sites_claim_resubmit_safety — exactly 1 + 1 + 0 sites claim
  resubmit-safety across client.py, session_provider.py, session_handle.py.
  Deliberately pins the OPT-IN direction: the in-flight set is not reliably
  enumerable, so a new opt-in is what must justify itself.
• test_resubmit_safe_states_the_verdict_without_exporting_the_hierarchy — the
  default refuses, so a death nobody classified cannot be resubmitted.
• test_acp_pipe_broken_triggers_retry — the real pipe broken wording resets the
  session and retries once. Returning rather than raising is also what keeps the run
  off the auto-pause ladder: CronService._execute calls record_failure() only in
  its except arm and record_success() (which zeroes consecutive_failures) when
  the callback returns.
• test_resubmit_safe_death_retries_regardless_of_wording[something nobody predicted]
  — shares no substring with the legacy guard, pinning the decision to the
  classification rather than to today's vocabulary.
• test_pipe_broken_retries_only_once — the _acp_retried marker bounds the new
  arm exactly as it bounds the substring arm.

Controls — these pass on fixed and unfixed source alike, and each exists because
without it the fix could silently become a no-op or over-reach:

• test_resubmit_safe_death_retries_regardless_of_wording[a wording that happens to say process exited] — a safe death must retry BECAUSE it is classified safe, not
  because its wording matched.
• test_plain_acp_error_still_matches_the_legacy_wording_arm — the five plain
  AcpError death sites are not AcpProcessDied, so scoping the substring arm to
  plain errors leaves them matched. Without this, the scoping could narrow the guard
  past the defect it repairs.
• test_non_process_death_acp_error_still_not_retried — widening to the type did not
  widen to every AcpError.
• TestAcpProcessDiedRaiseSites (2 tests) — anchor the wording at the real
  _send_request raise site and the transient=None classification, so a future
  rewording cannot quietly invalidate the reason this fix exists.

The pre-existing tests in the file are unchanged and still pass.

Manual verification

N/A — unit coverage sufficient. The tests drive the real
GatewayOrchestrator._cron_callback and the real AcpClient._send_request (with a
real BrokenPipeError) rather than a stand-in, so the wording and the retry
decision are both measured rather than assumed.

The predicate was additionally measured directly against every death shape the code
raises, which is how the previous revision's defect was located rather than argued:
of seven shapes, six agreed with the declared contract and one — the mid-prompt exit
— contradicted it. That one shape is now its own test.

The per-site classification is pinned at source level because reaching those sites
needs a child that dies mid-turn. The assertion is on which sites pass
resubmit_safe=True, not on which take the refusing default: the opt-ins are few and checkable,
whereas asserting the in-flight set is complete would restate the assumption that
failed twice.

Screenshots / video

N/A — no user-visible UI change. The diff touches the cron failure handler, the
ACP exception classification, and tests; there is no panel, component, layout or
theme surface involved.

Related Issues

Fixes #7395

Pattern harvest

Rule candidate: review-prompt
Pattern: a retry/classification decision string-matches an exception message when a
typed exception carrying the same meaning is already in scope at the decision site.
The message is presentation; the type is the contract. This generalizes — the same
shape appears wherever recovery logic greps str(exc) instead of testing
isinstance, and each instance fails silently the first time somebody rewords a
message.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 9f07f6d1d1259d672b0161cbe8a2811062130189 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/acp/client.py:7784 -- “never from write()” contradicts _send_request, which catches failures from both write() and drain() -> Fix: document both failure sites.
[GPT-REVIEWED] 9f07f6d

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 9f07f6d1d1259d672b0161cbe8a2811062130189 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base tree. I have enough to render the design review.

Design-Verdict: CONCERNS

Sound root-cause fix — type over wording, fail-closed — but the retained substring arm still permits the exact mid-turn replay the new invariant forbids.

Watch

The legacy substring arm remains a mid-turn replay channel that contradicts the PR's own invariant. client.py:6228 raises plain AcpError("ACP process exited (code=...)") from _read_message's EOF path — reachable mid-prompt, after a tool has completed — and _send_response's AcpError("ACP process not running") fires only when answering a mid-turn child request. Both match the retained wording arm ("a death discovered with a turn in flight must NOT be retried" per the new docstring), so the cron resubmit can still repeat a completed mutation through the untyped spellings, and test_plain_acp_error_still_matches_the_legacy_wording_arm now pins that hole open as if it were the contract. Pre-existing, and the PR narrows the overall surface — but the known-residual section names chat_runner/task_executor and omits this one, so the comments overstate what is enforced.
Clears when: the five plain-AcpError death sites are re-raised as AcpProcessDied with an explicit resubmit_safe verdict (pre-turn sites True, _send_response/_read_message sites default False) and the substring arm is retired — or the residual is at least documented next to the other two so the invariant's actual coverage is honest.

Suggestions

• File the follow-up now (an issue referencing bug(cron): ACP-death retry never fires for the common signature -- guard string-matches the message instead of testing AcpProcessDied #7395) covering both declared residuals plus the legacy-arm gap above, so the three unread/unfenced resubmit paths are tracked in one place rather than rediscovered.

[DESIGN-REVIEWED] 9f07f6d

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 9f07f6d1d1259d672b0161cbe8a2811062130189 via the fork AI-review pipeline; updated in place on each push.

Review details

Based on my analysis, I've independently traced the candidate. Let me confirm one final detail: that _read_message at 6228 is a shared path reachable both pre-turn (safe to retry) and mid-turn — which determines whether the fix can live in the diff.

I've now verified:

• client.py:6228 raises plain AcpError("ACP process exited (code=...)") on EOF-with-returncode, and this fires from _prompt_loop (6744, mid-turn) and _wait_for_response (6321, "Safe for init/handshake callers", pre-turn) — same message either way.
• The PR's new else arm is byte-identical to the pre-PR condition; "process exited" in exc_msg matches only the 6228 message, and it retries it.
• The sibling mid-turn death at client.py:6749 is now correctly typed AcpProcessDied (resubmit_safe=False) and refused — but 6228 was left as plain AcpError and still retries.
• Because 6228 is shared with pre-turn callers, narrowing the arm in-diff would break legitimate handshake retries; the real fix is in untouched code.

No blocking issues; one advisory finding.

FINDING — src/kiro_crew/slack/gateway.py:5261 — the retained "process exited" in exc_msg arm still retries a mid-turn death raised as plain AcpError("ACP process exited (code=...)") at client.py:6228 (fires when the child is already reaped at the EOF read — e.g. reaped while _prompt_loop was parked processing a just-completed tool), so the cron callback resubmits the prompt and replays that tool's side effects — the exact replay resubmit_safe=False was added to stop for the sibling 6749 path → Fix: raise the mid-turn (_prompt_loop/_dispatch_events) EOF-with-returncode death at client.py:6228 as AcpProcessDied(..., resubmit_safe=False) instead of plain AcpError (untouched code; narrowing the arm in-diff is not viable since _wait_for_response raises the same message pre-turn where retry is safe).

[OPUS-REVIEWED] 9f07f6d

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 9f07f6d1d1259d672b0161cbe8a2811062130189 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base tree. The counts in the description match my greps exactly: 3 "ACP process pipe broken" raise sites, 5 plain-AcpError death spellings, and the base predicate at gateway.py:5257 is the substring match described. I confirmed no existing mechanism does this job: AcpError.transient answers a different question (backend weather, retried with backoff and no session reset), and the gateway's _prompt_dispatched flag is set on attempt (gateway.py:4795, 4936) before the stream call, so it cannot distinguish "prompt never delivered" from "mid-turn death" — only the raise site can. The declared residual (2 resubmit sites not reading the flag: chat_runner.py:11670, task_executor.py:597) also checks out.

First-Principles-Verdict: PASS

Scoping the substring arm removes base's retry of mid-prompt "Process exited during prompt" deaths — cron jobs that self-healed those now take a hard failure; confirm that trade.

What this change ships

Inventory (5 items)

Intent: stop a healthy cron job from being durably auto-paused when its ACP child dies before the prompt was ever delivered — a FIX.

1. Cron retries once on a child death worded "ACP process pipe broken" instead of recording a hard failure — justified
2. Cron no longer retries a mid-prompt death whose wording contains "process exited" (base retried it) — justified
3. New AcpProcessDied.resubmit_safe attribute any death handler can read — justified
4. The prompt-starting write marks its own death safe to resubmit — justified
5. The pre-conversation liveness failure marks its death safe to resubmit — justified

The defect has checkable provenance: the base predicate at src/kiro_crew/slack/gateway.py:5257 matches neither substring against the wording raised at client.py:6137/6151/6170, and the added tests fail on base. AcpError.transient and _prompt_dispatched were both checked as existing mechanisms; neither answers the delivery-state question (the flag is set on attempt, before the stream call). Counted: 3 pipe-broken sites, 5 plain-AcpError death sites preserved by the scoped arm, 1 consumer of resubmit_safe, 2 declared out-of-scope resubmit siblings. The fix sits at cause level (classification at the raise site) rather than adding a third substring.

[FIRST-PRINCIPLES-REVIEWED] 9f07f6d

[SebastianYuSun]

Rebased onto a030091b4; head is now 2621b28a. This head carries the fix for the blocking finding.

The finding was correct. isinstance(exc, AcpProcessDied) alone did admit AcpToolStalled, which client.py raises after a tool has already been dispatched -- so a mutating tool that completed and then stalled would have been retried and could have run twice.

The fix is not the prescribed revert. Reverting the type arm would also drop the three pipe-broken sites this PR exists to cover, and it would put the retry decision back on exception wording, which is the defect class itself. Instead the guard now excludes the one subclass that is unsafe to retry:

(isinstance(exc, AcpProcessDied) and not isinstance(exc, AcpToolStalled))

AcpToolStalled was made a subclass of AcpProcessDied for this, so the exclusion is a type test rather than a message test, and any future stall-shaped death inherits the exclusion automatically instead of needing a new string added to a list.

The legacy substring arm is still there for the five plain-AcpError death sites (client.py:3822, :3837, :3856, :3876, :3922), so the two pre-existing retry cases are unchanged.

Tests: test/test_cron_acp_retry.py is +299, covering the pipe-broken retry, an arbitrary-wording AcpProcessDied (no legacy substring present), the two legacy cases still retrying, no auto-pause accounting on a successful retry, and a stalled-tool case asserting stream_and_collect is called exactly once so the double-mutation path stays closed.

[SebastianYuSun]

Round 4 on 20c259a2e. All three lanes addressed; the blocking finding was correct on substance and is now closed at cause rather than by revert.

GPT 5.6 (blocking) — closed

The finding is right: resubmit_safe defaulted True on process deaths raised with a turn in flight, so a completed tool mutation could be replayed. Opus 4.8 independently argued the hazard is pre-existing (the pre-PR arm already retried "process exited (code=...)"), and that is true — but it does not excuse it, because my own docstring asserted those deaths "never reached a running child", which is false.

Enumerating every raise site rather than reasoning from the taxonomy:

raise site	turn in flight?	resubmit_safe
client.py transport writes, pipe broken (×3)	no — write never reached a running child	True
client.py exited during prompt	yes	False
client.py tool stalled	yes — a tool was dispatched	False
session_handle.py died awaiting response	yes	False
session_handle.py died during prompt	yes	False

The last two are sites First-Principles named that I had missed, and died awaiting response neither review named — I found it by enumerating grep -rn "raise AcpProcessDied" src/ instead of trusting the docstring's claim.

The invariant is now about when the death was discovered, not which subsystem noticed. Only transport-write sites are retryable, which is exactly the class this PR exists to cover.

Not the prescribed revert, for the third round: reverting the type arm drops the three pipe-broken sites this PR is for, puts the decision back on message wording (the defect class), and leaves the mid-turn hazard untouched at all four sites.

First Principles (advisory) — subtraction taken

AcpToolStalled is deleted. It had one construction site and one distinguishing consumer, so resubmit_safe=False on the raised AcpProcessDied instance does the same work with strictly less surface. grep -rn AcpToolStalled src/ is now empty and every except AcpProcessDied handler is unchanged.

The undeclared-surface point is fixed: the description now lists all four items (predicate, attribute + kwarg, two client.py raise sites, two session_handle.py raise sites) and carries the table above. It previously said "one predicate in the cron failure handler", which was wrong.

The false docstring premise is rewritten to state the real invariant, and records that the earlier wording was false so it is not reintroduced.

Deferred, with reasons

resubmit_safe is read at 1 of 3 resubmit sites — dashboard/chat_runner.py and task_executor.py also resubmit on AcpProcessDied without consulting it (grep -rn "except AcpProcessDied" src/ → 4 handlers, 2 resubmit). Left out on purpose: both are pre-existing, in files this PR does not touch, and the correct non-resubmit branch differs for a foreground turn with a user waiting versus a cron wake. That is a design decision, not a mechanical port, and bundling it would put an unmeasured behaviour change for two subsystems into a PR whose evidence covers the cron path. The classification they would need is now correct and available.

Tests

test_every_in_flight_raise_site_declares_itself_resubmit_unsafe pins all four in-flight sites. Load-bearing: removing the kwarg at the session_handle.py mid-prompt site fails it with the site named. A companion control pins that the pipe-broken sites stay True — without it the fix could silently become a no-op.

903 passed across the cron, ACP-client, ACP-error-surface and slack-gateway suites. All local gates green, including the repo's own 14 bare-invocation script gates (check_agent_sdk_boundary.py among them — an earlier round of this PR was red on exactly that, for adding an ACP import; the attribute approach adds zero boundary edges).

[SebastianYuSun]

Round 5 — all three lanes named the same defect, and it is real

Opus 4.8 (blocking), Design and First Principles independently converged on one
finding, so it got checked first rather than triaged by verdict severity. It
reproduces, and it is my own previous fix being a no-op at one site.

Measured before changing anything. The shipped predicate, driven with every
death shape the code actually raises:

BUG  retried=True  want=False  client.py exited during prompt
OK   retried=False want=False  client.py tool stalled
OK   retried=False want=False  session_handle awaiting response
OK   retried=False want=False  session_handle during prompt
OK   retried=True  want=True   client.py pipe broken
OK   retried=True  want=True   legacy plain AcpError "not running"
OK   retried=True  want=True   legacy plain AcpError "process exited"

1 site(s) contradict the declared contract

"Process exited during prompt (exit code N)".lower() contains
"process exited". So the retained substring arm matched and out-voted that
death's own resubmit_safe=False. The flag was set, declared in the description,
and covered by a test — and unreachable at that site. Exactly as described.

Fix, at cause. The substring arm is now scoped so a typed death is decided
solely by its flag; wording can no longer resurrect a refusal. The five plain
AcpError death sites still match it, which a new control pins.

The default is inverted, per @Design's argument

Design and First Principles both showed the audit was incomplete —
session_provider.py:262/:314 and session_handle.py:1494, none of which I had
classified, all reachable with a turn in flight. Round 4 had already missed
session_handle.py:1911.

That is two consecutive rounds of an incomplete enumeration, and it is the
argument for inverting rather than extending: the kwarg cost is identical (four
sites either way), but a fail-closed default does not depend on my list being
complete. resubmit_safe now defaults to False; only four sites claim True,
each one where the turn provably never started.

claims True	why no tool can have run
client.py transport writes ×3	the write never reached the child
session_provider.py runtime not alive	no conversation started yet
(7 other construction sites)	default — refuses

The three sites you named are now correct without being listed, which is the point.

So the test changed direction too. Asserting "every in-flight site opts out"
restates the assumption that failed twice. It now pins the enumerable converse —
exactly 3 + 1 + 0 sites claim True per module — so a new opt-in is what has to
justify itself.

Subtractions taken

• resubmit_safe=False kwargs deleted at all four sites; redundant against a
  refusing default. session_handle.py leaves the diff entirely.
• Review-round narration removed from the docstrings per AGENTS.md's comments
  rule — including the test-class docstring's used to, which I had missed when
  fixing the same pattern in gateway.py earlier.

On the one point where the lanes disagreed

An earlier round had Opus calling the hazard pre-existing while another lane called
it introduced. Both were tested rather than choosing the convenient one, and both
hold: the pre-PR substring arm already retried "process exited (code=...)", and
the description asserted those deaths never reached a running child, which is false.
Pre-existing does not excuse shipping a live hazard at sites the description
declared safe.

Verification

test_mid_prompt_death_is_not_retried_despite_legacy_wording fails on the previous
published head, naming the site. Two controls guard against the fix becoming a
no-op or over-reaching: a safe death must retry because it is classified safe, not
because its wording matched; and plain AcpError deaths must still match the legacy
arm, or the scoping would have narrowed the guard past the defect it repairs.

Local gates on this head: black (repo wrapper), isort, flake8, mypy over
src/kiro_crew/ all clean; scrub-lint stage 1 clean; all 14 repo script gates
derived from ci.yml pass.

One diff-scope note: formatting the touched test file graduated it out of
.github/black-baseline.txt, and the repo's own gate asks for the entry to be
pruned, so that one-line deletion is in the diff.

The chat_runner.py / task_executor.py residual stays out of scope and is
declared in the description — though with the default now refusing, those handlers
inherit a safe verdict on any unclassified death instead of a permissive one.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #8215 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8215: KEEP. Complementary verdicts for different faults; neither covers the other. The shared insertion window in client.py plus the shared session_provider.py touch means the later of the two may need a trivial textual rebase, not a design change. Files: src/kiro_crew/acp/client.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Rebased onto main 41dcadf2 by a maintainer as part of the 2026-09-08 open-PR audit. Old head cb85ad90, new head 79ef8046.

Clean rebase, no conflicts. The diff is unchanged (5 files, +520/-13); only the surrounding code moved, so the gateway cron hunk now lands at gateway.py:5255 and the client hunks shifted with main.

Gates run locally on the changed files only: black --check (4 files clean, so the black-baseline.txt prune still holds), isort --check-only, flake8 all pass. pytest test/test_cron_acp_retry.py is 18 passed. The wider ACP suite is 786 passed with 6 pre-existing failures in test_acp_client.py (TestInitializeSession, TestReadNewToolResultsSync) that reproduce identically on plain main, so they are not from this PR.

Please review the rebase result. A maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is needed. Reply if anything looks wrong.

[SebastianYuSun]

Thanks for the rebase @bolichen97 — all four of your factual claims verify, and one consequence needs flagging because it is not visible from the rebase itself.

Your claims, checked against the code:

• 41dcadf2 is an ancestor of 79ef8046 ✅
• diff unchanged at 5 files / +520/-13 ✅
• clean rebase, no conflicts ✅ — I diffed your rebased tree against the old cb85ad90 tree with hunk offsets stripped: content-identical, so your rebase added nothing and lost nothing
• cron hunk near gateway.py:5255 ✅ (comment anchor at 5257)

The consequence: the rebase carried forward a head that three lanes had already blocked.

cb85ad90 was the round-5 head. Opus, Design and GPT all blocked on it, independently, for the same defect — and that defect is still live on 79ef8046:

on 79ef8046:  transport writes tagged resubmit_safe=True  -> 3
on 79ef8046:  _send_prompt retag present                  -> 0

_send_response and _send_error exist only to answer requests the child raises mid-turn (tool-permission replies, unknown-method rejections), and _send_request also carries session/steer and commands/execute. Tagging those writes resubmit-safe mislabels the majority of their callers. Opus's replay chain is the concrete harm: a cron turn completes tool 1, the agent asks permission for tool 2, the child dies, the approval write raises a safe-tagged death, the prompt is resubmitted, and tool 1's mutation replays.

That was fixed locally before your audit ran, but the fix had not been pushed — my mistake, and the reason your rebase picked up the stale head. Now pushed, rebased onto your 41dcadf2.

What changed from 79ef8046:

	79ef8046	new head
transport writes claiming safety	3	0 (refusing default)
_send_prompt retag	absent	present — the one write that provably precedes any tool call
.github/black-baseline.txt	in diff	removed (see below)
files / lines	5, +520/-13	4, +576/-14

Why the safe claim is sound at _send_prompt specifically — measured, not assumed. write() never raises; the death always surfaces at drain(); and drain() fails iff unflushed bytes remain (a child that consumed the whole payload drains clean even if it dies immediately after). Wire messages are newline-delimited JSON-RPC, so bytes still queued mean the child never received a complete session/prompt line, and it cannot dispatch a tool for a request it has not finished reading. Partial consumption is reachable — a child that reads a prefix then dies does raise here — but a prefix is not a parseable request. The code carries this reasoning including the reachable case, so it does not read as a counterexample later.

One gate interaction worth your attention, unrelated to my diff. .github/black-baseline.txt was in the old diff because the black gate explicitly instructed it:

1 baselined file is now black-clean … run python3 scripts/check_black_formatting.py --update-baseline

But fork-workflow-guard.yml:132 greps ^\.github/ — any path, not just workflows/ — so a fork PR that follows the black gate's own instruction gets blocked and needs a maintainer label. I resolved it on my side by leaving the touched test file legitimately baselined instead, so black passes with 0 new offenders and the diff touches no .github/ file. Flagging it because any fork contributor who obeys that black-gate hint will hit the same wall.

Local gates re-run on the new head, not inherited from the pre-rebase one.

---

Separate finding for you, not caused by this diff: main is failing its own comment-history ratchet

Your rebase surfaced this because 41dcadf2 is where I re-based, and the new
check_comment_history.py gate then put slack/gateway.py in scope. Measured with the
gate's own violations_in_source:

comment-history-baseline.json  src/kiro_crew/slack/gateway.py = 67
origin/main (41dcadf2c)  actual                              = 69
this branch              actual                              = 69   (diff adds 0)

The gate itself says so: "this diff adds none of the matched lines." Corroborating,
main-ratchet-audit.yml has concluded failure on its last five heads, including
41dcadf2c:

2026-09-08T13:29 failure  53987e756
2026-09-08T13:06 failure  41dcadf2c   <- this PR's base
2026-09-08T12:28 failure  c36866cf9
2026-09-08T12:04 failure  02d7a2d06
2026-09-08T12:04 failure  1f8ba0be8

ci.yml:446 runs the same gate on PR scope, so any PR touching gateway.py inherits
this. I have deliberately not "fixed" it here, because neither available move is correct
work from a fork:

• rewording two pre-existing upstream comment lines would be scope creep into code this
  PR has no business touching;
• raising the baseline 67 -> 69 fights the tool's design — _shrunken_baseline is
  min(recorded, now), so the refresh path only ever lowers counts. The ratchet is
  intentionally one-way and it is not a fork's place to crank it backwards.

I did fix the one real hit that was mine: a test docstring cited the issue number, which
the gate flags. That is gone.

Two gates now share a shape worth noting: this one and the .github/black-baseline.txt
case above both ask a fork contributor to edit repo-wide bookkeeping in order to land an
unrelated change. Happy to send a separate PR that reconciles the gateway.py entry if you
would rather have it from me than fix it on main.

[SebastianYuSun]

Follow-up on the comment-history ratchet, now with CI's own verdict rather than my local measurement — and I've pinpointed the commit, so this should be a two-minute fix for whoever owns it.

CI confirms the diff is not the cause

Backend Lint & Type Check (3.12) on 79ef8046, job 102077367730:

comment-history gate scope: merge HEAD^1..HEAD (5 changed file(s))
comment-history gate FAILED: 0 new offender(s), 1 grown count(s),
  0 file(s) with markers on added lines, 0 entr(y/ies) to lower.

0 new offender(s) and 0 file(s) with markers on added lines — CI itself says the contribution introduced none of it. The single grown count is slack/gateway.py.

Where the 2 markers came from

5b1dc662f  src/kiro_crew/slack/gateway.py = 67   (matches baseline)
a21d112e8  src/kiro_crew/slack/gateway.py = 69   <-- grew here

a21d112e8 — Joe Guo, 2026-09-07, "feat(messaging): spool inbound messages the shutdown gate refuses, and notice them (#8913)" — introduced issue #2217 twice. On current origin/main (53987e756) they sit at:

• src/kiro_crew/slack/gateway.py:1825
• src/kiro_crew/slack/gateway.py:11856

Baseline still records 67; main measures 69. Consistent with that, main-ratchet-audit.yml has concluded failure on every recent main head (53987e756, 41dcadf2c, c36866cf9, …) — so the growth landed while the audit was already red and nothing gated it.

Why this blocks any PR touching gateway.py, including after approval

ci.yml:446 runs the same gate scoped to the PR's changed files. Since gateway.py is in scope, the drift is inherited. Concretely: approving the fork workflows on my current head will not clear this — it will fail identically, for a reason my diff did not create. Flagging that explicitly so an approval isn't spent expecting a green.

What I have and haven't done

Fixed, because it was genuinely mine: a test docstring cited the issue number, which the gate flags. Gone.

Not touched, deliberately:

• rewording gateway.py:1825 / :11856 would be scope creep into a teammate's commit from a PR about ACP retry classification;
• bumping the baseline 67 → 69 fights the tool's design — _shrunken_baseline is min(recorded, now), so --write-baseline only ever lowers. There is no supported path for a grown count, because the design assumes main-ratchet-audit prevents growth. That assumption is what broke here.

Either fix is a one-liner for someone with commit rights on main: reword the two lines, or reconcile the entry. Happy to send a standalone PR doing whichever you prefer — just say which, since it is your ratchet policy and not a fork's call to crank it backwards.

Worth noting this is the second gate in this PR that asks a fork contributor to edit repo-wide bookkeeping to land an unrelated change; the other was .github/black-baseline.txt, which fork-workflow-guard.yml:132 then blocks.