fix(subagent): stop the injection notice contradicting its own reason

[leonlaiyc]

Problem / Motivation

When a sub-agent reaches a terminal state but its report cannot be injected into
the parent session, notify_injection_failed builds a notice whose second line
is the caller's reason and whose fourth is _injection_notice_outcome(info).
For a run that actually completed, that outcome line is:

return "The agent finished but result delivery timed out."

Only two of the six call sites pass a timeout. The other four pass something
else entirely — slack/gateway.py:

line	reason
6305	"provider dead after prompt-busy retries"
6326	"ACP process died"
7066	str(t.exception()) from the failed injection turn
7338	_last_failure_reason after the attempt cap

So the message contradicts itself:

[Subagent completion event]
Agent `a1` ❌ ACP process died
Task: …
The agent finished but result delivery timed out.

This is the residual the reviewer named on merged #5900, which introduced the
outcome-aware line:

One residual worth noting: three of the gateway's notify_injection_failed
call sites pass non-timeout reasons ("provider dead", "ACP process died", last
failure reason), and the completed branch still asserts "timed out".

(There are four, counting str(exception) at 7066.)

Why it matters

failure_msg is not only rendered in the dashboard slot — it is queued into
slot._pending_subagent_failures and drained into the LLM's context on the
next _run_chat turn. So the reader deciding what to do next is a model, and
the two mechanisms call for different responses: a delivery timeout invites a
retry of the injection, while "the ACP process died" or "provider dead after
prompt-busy retries" means the transport is gone and the sensible move is to
read the result off disk. Handing the model a confident, specific, wrong
mechanism is worse than handing it none.

docs/system-specs/common/injected-messages.md documents the string, so the
wrong copy is also the specified copy.

What changed (motivation → approach → change)

Root cause is that one branch of the helper describes the mechanism of the
delivery failure while the notice already prints that mechanism, from a value
the helper cannot see. The two can only agree by accident.

The completed branch now states the outcome and nothing else:

return "The agent finished, but its result could not be delivered."

• A real timeout loses nothing. Its callers already say so in reason
  ("injection timed out after 300s", "delivery timed out"), so the specific
  cause still reaches the reader — and the old copy said "timed out" twice.
• The stopped and failed branches are untouched. They describe the run's
  own terminal state, which the record does know, and they are not in tension
  with reason.
• The docstring records the constraint so the next edit does not put a
  mechanism back: the cause belongs to reason, this line states the outcome.
• docs/system-specs/common/injected-messages.md is updated in the same
  commit, per AGENTS.md, including a note on why the line names no
  mechanism.

Tests

New TestInjectionNoticeDoesNotContradictItsReason in
test/test_subagent_coverage.py, driving the real
notify_injection_failed and reading the queued failure_msg — the same
end-to-end shape the neighbouring TestNotifyInjectionFailedOutcomeCopy uses:

• test_a_non_timeout_reason_is_not_overridden_by_the_outcome_line, parametrized
  over the four reasons the non-timeout call sites actually pass. Asserts the
  caller's reason is still shown, that "timed out" does not appear anywhere in
  the notice, and that the new line is present.
• test_a_real_timeout_still_reads_as_one — a control: with
  reason="injection timed out after 300s" the reader still learns it was a
  timeout, so the neutral copy costs nothing.
• test_the_result_recovery_hint_still_agrees_with_the_line — a control
  against the obvious way this could go wrong: "could not be delivered" must not
  read as "there is nothing to read", so with a result_path present the notice
  still points at the file and still says Use the read tool.

Two existing tests asserted the old wording and are updated, not deleted:
test_completed_keeps_the_finished_copy becomes
test_completed_states_the_outcome_without_a_mechanism (and now also asserts
"timed out" not in line), and
test_post_run_delivery_timeout_keeps_existing_copy_and_hint becomes
test_post_run_delivery_failure_keeps_the_completed_copy_and_hint.

Red-before, measured against pristine origin/main production code
(10c422628) with the new tests in place — 8 failed / 262 passed. The four
parametrized failures state the defect in one line each:

AssertionError: the notice says the delivery timed out while its own
                reason says 'ACP process died'
AssertionError: the notice says the delivery timed out while its own
                reason says 'provider dead after prompt-busy retries'
AssertionError: the notice says the delivery timed out while its own
                reason says 'AcpError: stream closed while waiting for result'
AssertionError: the notice says the delivery timed out while its own
                reason says 'no active session for parent'

Green-after: 270 passed in that module. Blast radius: 1303 passed / 26
skipped across the whole -k "subagent or injected" selection. That run also
reports 32 collection errors in test_bench_download_fd.py and
test_discord_doctor.py (KeyError: 'file'), pre-existing on this machine and
unrelated — verified earlier today by stashing a change and re-running those two
files on pristine main for the same result.

flake8, isort and mypy are clean.

On scripts/docs-lint.sh: I do not have a pass to report from this host. Its
wrapper execs python3, which here resolves to the Windows Store shim (exit 49
before the lint runs), and the real 3.10 install ships python.exe with no
python3.exe, so putting it first on PATH does not shadow the shim. What the
lint enforces is that every doc is indexed and every link resolves; this change
edits one line of prose inside an existing section of an existing doc and
creates, moves, renames and deletes nothing, so it cannot affect either
property. CI runs the real gate.

Manual verification

N/A — unit coverage sufficient: the defect is the text of a generated message,
and the tests read that message end to end out of the queued event rather than
asserting on the helper alone.

Related Issues

Residual named by the reviewer on merged #5900 (fix: derive injection-timeout notice copy from the run's outcome (#5882)). Independent of the open
run-coordinator stack — #5281 adds notify_injection_failed call sites but
touches neither _injection_notice_outcome nor this wording, so there is no
overlapping hunk. No separate issue was filed.

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

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#66809557]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: The only real red is the black gate — not black-formatted: test/test_subagent_coverage.py (the new TestInjectionNoticeDoesNotContradictItsReason class). The Backend Tests (3.10, 4) and Coverage Gate reds are the same lint-lane rollup / base drift. Fix plan: rebase onto latest main, run black --target-version py310 test/test_subagent_coverage.py, push. The docs + subagent.py copy change ("finished but result delivery timed out" → "finished, but its result could not be delivered") is behavior-preserving and fully unit-tested.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[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 #8003 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 #8003: KEEP. Same message envelope, complementary changes that merge cleanly; no coordination beyond reviewing the final combined wording. Files: docs/system-specs/common/injected-messages.md, src/kiro_crew/subagent.py.

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

[bolichen97]

@leonlaiyc Thanks for this. It is still valid: on current main, _injection_notice_outcome in src/kiro_crew/subagent.py still returns the "result delivery timed out" copy on the completed branch, and docs/system-specs/common/injected-messages.md still specifies that wording, so the contradiction you describe is intact. #5900 made the notice outcome-aware for every branch except this one, and your change closes exactly that residual.

Two notes before review.

Overlap with #8003 (@welikoiwanenko, subagent credit usage). It edits the same two files and the same injected-messages.md envelope section, but for a different purpose: it inserts a Usage: ... line above the outcome line in src/kiro_crew/subagent_manager/terminal.py and does not touch _injection_notice_outcome or the string you replace. Nothing needs to absorb anything, both can land. Since yours is a one-line production change, the simplest order is to land this PR first and let #8003 rebase on top. Whoever lands second should re-read the combined notice text once, so the Usage line and the new outcome sentence read as one message.

Rebase needed. The branch is 1837 commits behind main and the notice builder moved to src/kiro_crew/subagent_manager/terminal.py in #6944, so the slack/gateway.py line numbers in your description are stale, though the call sites and the defect still exist. The one-line source change and the docs hunk still apply cleanly; the test hunks do not, because TestInjectionNoticeOutcome now starts near test/test_subagent_coverage.py:1270. Please rebase and re-run black.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

[bolichen97]

Rebased onto main 41dcadf2 by a maintainer as part of the 2026-09-08 open-PR audit.

Clean rebase, no conflicts. The only edit on top of your commit is black formatting inside the new TestInjectionNoticeDoesNotContradictItsReason class (the pinned formatter reflows the _info(...) calls and the multi-line assert ... , f"..."); no assertion, string, or behaviour changed. Your production one-liner and the docs hunk still apply verbatim, since main has not adopted the new wording.

Gates run locally on the changed files only: black --check, isort --check-only, flake8 all clean, and pytest test/test_subagent_coverage.py 253 passed.

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

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] f756fb6

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] f756fb6

[github-actions]

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

Premise-level review of f756fb6ea6cc53345a10c8ac1ead0e8f0d2fa94b 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 six call sites and their verbatim reasons check out (gateway.py:7467, 7488, 8261, 8533, 8588 plus terminal.py:239), the completed branch at subagent.py:1485 does say "timed out", both renderers of the outcome line print reason above it, and the new test provably fails on base by reading. One leftover: the old copy survives as a fixture in website/src/test/SubagentCompletionCard.test.tsx:90, whose own comment claims the fixtures are "byte-shaped like the real messages".

First-Principles-Verdict: PASS

One stale sibling: website/src/test/SubagentCompletionCard.test.tsx:90 still carries the old copy as a fixture its comment calls "byte-shaped like the real messages".

Grepped result delivery timed out: 4 non-doc sites; this PR updates 3, leaving that frontend fixture quoting a message the backend will never emit again. The parser test still passes either way, so it is a verify-before-merge note, not a defect.

What this change ships

Intent: stop the injection-failure notice telling the LLM "timed out" when its own reason line says otherwise — a FIX.

Inventory (5 items)

1. The completed-run notice now says "its result could not be delivered" instead of "delivery timed out", in the LLM's queued context and the dashboard card — justified
2. The injected-messages spec's completed line updated in the same commit, with a why-note — justified
3. The helper's docstring now records that the completed line names no mechanism — justified
4. New tests pin that a non-timeout reason is never contradicted, with timeout and recovery-hint controls — justified
5. Two existing tests re-pinned from the old copy to the new — justified

The rewritten pins carry their evidence in-tree: the base code itself shows four callers passing non-timeout reasons into a line asserting a timeout, so the old pins recorded the contradiction, not a decision.

[FIRST-PRINCIPLES-REVIEWED] f756fb6

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

The frontend parser reads outcome from the glyph, not the English payload text, so the old string there is inert fixture data. Everything checks out: four of the six notify_injection_failed call sites really do pass non-timeout reasons (slack/gateway.py:7467,7488,8261,8533), the reason really prints one line above the outcome line (terminal.py:653-658), the owning spec is updated in the same commit, and the copy change is the root-cause fix rather than a reason-string wording contract.

Design-Verdict: PASS

A one-line copy fix that removes a false mechanism claim contradicted by four of six call sites; cause stays in reason, verified against the base tree.

[DESIGN-REVIEWED] f756fb6