[mashkovd]

Fixes #360.

Rebased onto main now that #367 has merged (2c72f6c); no longer stacked. kind
gained "refused" in #367's FollowupKind Literal, FollowupSubprocessError keeps
main's kind-only signature plus reason, and the counter round-trip and un-stick
tests main added were widened to cover refusals rather than duplicated.

The problem

exit 42 conflated three outcomes:

outcome	should charge a MAX_REVIEW_ATTEMPTS slot?	before	after
implementer crashed / spec unimplementable	yes	yes	yes
findings already addressed, nothing to change	no	yes	no
refusal honouring an explicit operator decision on the PR	no — this is a success	yes	no

On portfolio#56 the refusal was correct twice, burned 2 of 5 attempts, pushed a healthy
PR into the cap, and forced a GitOps reset (mctl-gitops#1205).

The signal, and why a marker file

The implementer now reports "I deliberately did not commit, and here is why" distinctly
from "I ran and produced nothing". The agent writes .implementer-refusal.json in the
worktree root:

{"refused": true, "reason": "Findings 2 and 3 are out of scope by explicit operator decision recorded on the PR; not applying."}

Why not scrape the final message. There is a final message — the prompt already asks
the agent to "STOP without committing and explain why" — but the prose is the agent's
free composition, unstable across model versions and prompt edits. Any regex over it has
two failure modes, and they are not symmetric: missing a real refusal costs one attempt,
while matching a crash narration ("I was unable to make any changes…") as a refusal
would exempt a genuinely broken proposal from the cap forever. A named JSON file with an
exact shape is an act the agent cannot perform by accident while doing something else, and
it either exists or it does not.

The marker is honoured only when all of these hold — the asymmetry above is the reason
each check errs toward "not a refusal":

• it is small enough to read at all (MAX_REFUSAL_MARKER_BYTES, 64 KiB), checked with
  stat() before the read and refused outright rather than truncated — the file is
  written by an LLM with a Bash tool into a cloned worktree, and an implementer OOM is
  the least diagnosable failure this platform has;
• parses as a JSON object with refused exactly true (not 1, not "true") and a
  non-empty string reason;
• the file is untracked. This check is not belt-and-braces, so please do not drop it:
  without it, committing .implementer-refusal.json into a target repo would make every
  future follow-up on that repo read as a refusal — the repo would be permanently exempt
  from the attempt cap, silently and for good. _stage_implementer_agent registers the
  marker in .git/info/exclude so a broad git add -A cannot commit it in the first
  place: the exclude entry is the defence, this check the backstop for anything that got
  committed anyway (exclude does not affect already-tracked files);
• the run produced no commits. A commit beats the marker: the push still happens.

A stale marker cannot survive between runs. _clone_target() clones into a
timestamped tmp directory (rmtree first if the path somehow exists) and
review_feedback_one uses only that clone, so the marker is always an untracked
working-tree file created during the current run. The only way one could pre-exist a run
is by being committed — which is exactly what the untracked check rejects.

Anything else logs a warning and falls back to the ordinary "no follow-up commits"
failure, which still charges an attempt.

End to end

1. Prompt (_build_prompt, review-feedback branch): step 5 now specifies the marker,
   its exact shape, and that it is only for a considered no-op — never next to a commit,
   never as a progress note, never with a placeholder reason. The new-PR prompt is
   unchanged (no attempt budget there).
2. Exit code: EXIT_DELIBERATE_NO_OP = 47, mapped in _review_feedback_exit_code()
   in the existing string-prefix style (deliberate no-op: …). --refusal-out <path>
   hands the reason to the caller as JSON; it is advisory — the exit code alone carries
   the decision, so a child that dies before writing is still not charged.
3. Shepherd: _refusal_codes() classifies 47 as its own kind, "refused". It is kept
   as a separate function rather than widening fix(implementer): await the sub-agent the CLI launched in the background #367's _followup_code_sets() tuple — a
   refusal is neither of those two things (not deterministic: the proposal is not at
   fault; not a harness failure: nothing was lost), and it keeps the stacked diff off the
   function fix(implementer): await the sub-agent the CLI launched in the background #367 is still editing. Same deferred run_implementer import, so the Temporal
   worker still does not pull in the SDK (refactor(temporal): check the Argo execution boundaries the worker reasons about #149).
4. process_one: no review_attempts increment, no status change, reason recorded in
   .status.yaml notes (bounded by MAX_NOTES_CHARS) and in the run summary line,
   decision wait so a later tick can act on new information.

The retry is bounded, on its own counter

A refusal moves no head SHA and re-triggers no review, so the next tick hands the agent
the identical bundle and pays for the identical answer. Following the same shape #367 just
introduced for harness failures: a separate refusals counter with MAX_REFUSALS = 3,
counted per head SHA (refusals_head) — the premise is "the same bundle produces the
same answer", which stops holding the moment the branch moves, so a refusal on a new head
starts the count over rather than inheriting a budget spent on different code.
At the bound the proposal flips to review-stuck with a note saying explicitly that
review_attempts was never charged and the proposal is not at fault — a standoff between
a reviewer finding and an operator decision is something only a human can settle.
review_attempts is never touched on this path.

The counter contract

outcome	review_attempts	harness_failures	refusals / refusals_head
success	+1	clear	clear
deterministic (42/43/44)	+1	clear	clear
refused (47)	untouched	clear	+1, anchored to the head
harness (46)	untouched	+1	untouched
transient	untouched	untouched (writes nothing)	untouched

Two mirrored rules: an outcome proving the handoff worked clears
harness_failures; an outcome proving the agent engaged with the findings clears
refusals. The mirror stops short in one place on purpose — a harness failure clears
neither, because the child never reached a terminal state, and because with 47 clearing
the harness counter a 46/47 alternation that also cleared refusals would trip neither
cap and run as an unbounded paid loop. Pinned by one parametrised table
(test_counter_contract_across_outcomes) plus the alternating-sequence regression,
rather than one test per cell.

reconcile_one's review-stuck un-stick path clears refusals too, by exactly the
argument in b28c823: un-stuck on one axis only is un-stuck in name only. That commit's
test is extended to cover all three counters (and renamed to
test_reconcile_unstick_clears_every_counter) rather than gaining a near-duplicate — the
invariant is "every counter that can flip a proposal to review-stuck is cleared here",
and it is worth failing on as a whole. Verified it fails without the one-line change.

(mctl-agents#359 — inline comments re-anchored onto a new head inflating the finding count
— is the upstream cause of the findings that refusal was declining. Separate fix.)

Tests

New tests/test_run_implementer_refusal.py (27) plus a block in
tests/test_run_shepherd.py (14, with two of main's existing tests widened rather than
duplicated):

• the marker contract: valid, absent, not-JSON, not-an-object, refused missing / false /
  "true" / 1, reason missing / blank / non-string, committed marker ignored,
  reason normalised and capped;
• review_feedback_one: no commits + marker → deliberate no-op: … → 47; no commits
  alone → the old error → 42; a commit beats the marker;
• main() writes the reason to --refusal-out on 47 and writes nothing on 42;
• the prompt specifies the marker (and the new-PR prompt does not);
• shepherd: 47 classified refused on its own, reason carried through the real
  --refusal-out plumbing, refusal without a reason file still not charged, temp file
  cleaned up, outer loop leaves counter + status untouched and records the reason, note
  bounded, counter bounded → review-stuck;
• the marker cannot become tracked: both .git/info/exclude entries present and
  idempotent, and the property asserted against real git — stage a marker plus a real
  change, git add -A, assert the change is staged and the marker is not;
• the prompt's JSON example is parsed with json.loads so it cannot regress into an
  unparseable shape, and every implementer.md sub-agent definition carries the contract;
• counter lifecycle: _discover_refs round-trips every counter (the read that makes any
  cap bind at all), refusals accumulate on one head, reset when the branch moves, and the
  un-stick path clears the head alongside the count;
• test_42_43_44_still_charge_an_attempt_end_to_end — drives the real classification
  in apply_followup for each deterministic sentinel and feeds the resulting exception to
  process_one, asserting the attempt is charged. shepherd charges an attempt when the implementer correctly refuses to act (exit 42 conflates refusal with failure) #360 must not weaken [wip] feat(shepherd): tier 3 PR shepherd — tasks 1 + 2 #12's fix.

uv run pytest -q → 1107 passed, 8 skipped. uv run ruff check and uv run mypy clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1

[claude]

Claude finished @mashkovd's task in 5m 52s —— View job

---

Code review — ca7ee7a

• Read repository conventions
• Read the pinned diff (19f1146...ca7ee7a)
• Analyze the refusal-marker contract in run_implementer.py
• Analyze classification and the process_one refusal path in run_shepherd.py
• Assess test coverage
• Post inline findings and submit the verdict

Verdict: changes requested — 0 P1, 1 P2, 3 P3. Review submitted against ca7ee7a959e9d5834facaffca7e6d0145a59ab69.

P2 — the marker has no .git/info/exclude protection

orchestrator/run_implementer.py:232. The untracked check is the right guard but it is the only defence, and it fails closed silently and permanently. _stage_implementer_agent already documents this exact threat model for the other runtime file it stages — "so even a broad git add -A from the sub-agent leaves it untouched" — yet .implementer-refusal.json gets no exclude entry. If a run ever commits the marker alongside a real fix (commit beats marker, so the push proceeds), the marker lands on feat/agents-<slug>; from the next tick on it is tracked, every genuine refusal falls back to exit 42, and review_attempts is charged again — #360 reverts for exactly the kind of PR that motivated it. One line in _stage_implementer_agent fixes it.

P3

• run_implementer.py:661 — the JSON example the prompt says to copy "exactly" wraps its reason placeholder across three lines, so the shape shown is not valid JSON; an agent mirroring it literally produces a file json.loads rejects, which lands back on the charged path. Also: the marker contract exists only in the outer prompt, while agents/_generic/.claude/agents/implementer.md (the sub-agent that actually decides) still says only "STOP without committing and explain".
• run_shepherd.py:532 — _read_refusal_reason returns the string uncapped; MAX_NOTES_CHARS is applied at two of four consumers (the review-stuck ShepherdResult.notes at L2037 and the info: log are unbounded). Capping at the boundary makes the backstop terminal.
• run_shepherd.py:834 — no test exercises the refusals read from .status.yaml, and that read is the only thing making MAX_REFUSALS bind at all (same gap as harness_failures, so parity rather than a new omission). Separately, refusals is lifetime-per-PR, not consecutive-per-finding-set, so refusals against an old bundle still count after the findings change.

What holds up

The asymmetry argument is right and the code follows it consistently: every ambiguous marker errs toward charging, a commit beats the marker, the exit code — not the advisory --refusal-out file — carries the decision, 47 is classified in its own set instead of widening #367's tuple, the deferred run_implementer import keeps the worker off the SDK, review_attempts is untouched on the refusal path, and reconcile_one clears the new counter along with the others. test_42_43_44_still_charge_an_attempt_end_to_end is the right guard that #360 does not weaken #12.

Not verified: I could not run pytest, ruff, or mypy here — those commands needed approval in this environment, so the reported "1066 passed, 8 skipped" is the author's result, not one I reproduced.
· branch fix/agents-360-refusal-not-an-attempt

[claude]

P2 — the marker has no .git/info/exclude protection, so a single git add -A can permanently disable the fix for that PR.

This tracked-check is the right guard, but it is the only defence, and it fails closed in a way that is silent and permanent. _stage_implementer_agent (L581-585) documents the exact threat model for the other piece of runtime scaffolding it stages:

We register it in .git/info/exclude (a per-clone, untracked-by-design ignore list) so even a broad git add -A from the sub-agent leaves it untouched.

.implementer-refusal.json is the same category of file — written into the worktree root by the agent, never meant to reach the PR — but it gets no exclude entry. Concrete sequence:

1. Tick N: the agent writes the marker and also commits a real fix with git add -A (the prompt forbids it, but the repo already treats broad staging as realistic). The commit beats the marker, _push_followup runs, and the marker lands on feat/agents-<slug>.
2. Tick N+1: a genuine refusal. The fresh clone checks out the branch, the marker is now tracked, this check trips, and the run falls back to implementer produced no follow-up commits -> 42 -> review_attempts charged.

From that point on the marker can never be honoured on that branch again, so #360 silently reverts to today's behaviour for exactly the PR that already demonstrated the failure — plus a junk file in a target repo's PR diff.

The fix is one line in _stage_implementer_agent: register REFUSAL_MARKER_FILENAME in .git/info/exclude alongside .claude/agents/implementer.md (loop over a tuple of entries rather than the single entry at L609). That makes the tracked-check pure belt-and-braces instead of the only line of defence, and it keeps the marker out of the PR in the commit-beats-marker case too.

Fix this →

[mashkovd]

Fixed in 8097ea4 — agreed, and the framing ("the untracked check is the backstop, not the defence") is the right one.

_stage_implementer_agent now loops over a tuple of entries and registers REFUSAL_MARKER_FILENAME alongside .claude/agents/implementer.md. Because the failure you describe is silent and permanent, the reason is in the code rather than only in the commit message: the docstring now says the exclude entry is the defence and the tracked-check the backstop, and names the consequence (every later refusal on that branch falls back to 42).

Two tests, in tests/test_run_implementer_refusal.py:

• test_staging_excludes_the_refusal_marker — both entries present, and re-staging is idempotent.
• test_excluded_marker_survives_git_add_dash_a — asserts the property against real git rather than the file's text: stage a marker plus a real change, run git add -A, assert the change is staged, the marker is not, and _read_refusal_marker still honours it.

Note the backstop still does its job for a marker that became tracked before this change: .git/info/exclude does not affect already-tracked files, so git ls-files --error-unmatch still trips on one.

[claude]

P3 — the example the agent is told to copy "exactly" is not valid JSON.

The placeholder reason string wraps across three prompt lines, so the shape shown is a JSON string containing raw newlines — json.loads rejects that. The instruction says "with exactly this shape", which is the one place an agent is most likely to mirror the layout literally, and the failure is the exact one #360 is fixing: _read_refusal_marker warns, returns None, the run falls back to 42, and the attempt is charged.

Putting the example on a single line (and shortening the placeholder, e.g. {"refused": true, "reason": "<what you declined, and the evidence: quote the operator note or the code that already satisfies the finding>"}) removes the ambiguity at no cost.

Related, same section: the marker contract lives only in this outer prompt, while agents/_generic/.claude/agents/implementer.md L69-71 still tells the sub-agent that actually makes the decision to "STOP without committing and explain what's missing in your final message" — with no mention of the marker. That relies on the outer agent noticing the sub-agent declined and writing the file on its behalf. Worth stating the contract in implementer.md too so the decider can record it directly.

[mashkovd]

Both fixed in 8097ea4.

The example. You are right that this is the worst place for it — "exactly this shape" invites a literal mirror, and the failure lands on the charged path. It is one line now:

{"refused": true, "reason": "<what you declined, and why>"}

with the guidance about what to put in reason (quote the operator note, or the code that already satisfies the finding) moved to prose beside it, so the sample stays inside the 120-column limit while the instruction keeps its substance. test_prompt_example_is_itself_valid_json pulls the {"refused"...} line out of the rendered prompt and json.loads it, so the example cannot regress into something unparseable.

The sub-agent. Also right, and it was the weaker half of the design: relying on the parent to notice the child declined and write the file on its behalf. All nine agents/*/.claude/agents/implementer.md definitions now carry the contract as the first bullet under "Rules of engagement", stating the marker, the one-line JSON shape, and the consequence of omitting it (the orchestrator cannot tell a considered decision from a crashed run, and charges an attempt). test_subagent_definitions_carry_the_marker_contract asserts every definition mentions the marker filename, so a newly added per-service implementer cannot ship without it.

[claude]

P3 — the shepherd-side cap is applied at the use sites, not at this boundary.

MAX_NOTES_CHARS is described as the backstop against agent-authored prose, but this reader returns the string unbounded. Two consumers then diverge: the wait note and the review-stuck note both slice to MAX_NOTES_CHARS, while the review-stuck ShepherdResult.notes at L2037 (f"repeated refusal; proposal not at fault ({reason})") and the info: log line at L2014-2020 do not. In practice the implementer caps at MAX_REFUSAL_REASON_CHARS, so this only bites if the child writes the file some other way — but capping here ([:MAX_NOTES_CHARS]) makes the backstop actually terminal and removes the per-call-site discipline.

[mashkovd]

Fixed in 8097ea4 — capped at the boundary, return " ".join(reason.split())[:MAX_NOTES_CHARS].

You named the real problem: a cap applied per call site is one new log line away from being incomplete, and it already was (the review-stuck ShepherdResult.notes and the info: line were unbounded). Capping where agent-authored text enters the shepherd makes the backstop terminal for every consumer instead of a discipline each one has to remember. The docstring now says that is why it lives there. test_read_refusal_reason_caps_at_the_trust_boundary pins it directly.

[claude]

P3 — two notes on the new counter's lifecycle; neither blocks merge.

1. No test covers this read. MAX_REFUSALS only bounds anything if refusals survives a tick, and this line is the only thing that carries it back from .status.yaml into ProposalRef. test_refusals_are_bounded_and_flip_to_review_stuck sets ref.refusals in memory, so if this read regressed the counter would reset every tick and the cap would never trip — an unbounded series of paid SDK runs with no terminal state, which is precisely what the counter exists to prevent. harness_failures (L833) has the same gap, so this is parity rather than a new omission; one _discover_refs round-trip test would pin both.

2. refusals is cleared only on a successful follow-up or a reconcile un-stick, so it is effectively lifetime-per-PR rather than consecutive-per-finding-set. If the reviewer re-runs and the findings change (which shepherd never merges an approved PR: re-anchored inline comments are counted as current findings #359 makes likely), refusals recorded against the old bundle still count toward the cap, and a third refusal on genuinely new findings flips a healthy PR to review-stuck — a milder version of the shepherd charges an attempt when the implementer correctly refuses to act (exit 42 conflates refusal with failure) #360 failure mode. The review-stuck note does say the proposal is not at fault, so a human sees the right framing; if the shepherd already tracks a reviewed head SHA, resetting refusals when it moves would make the counter mean what the MAX_REFUSALS comment says it means.

[mashkovd]

Both taken, in 8097ea4.

1. The read is now tested. test_discover_refs_round_trips_every_counter writes review_attempts, harness_failures, refusals and refusals_head into a real .status.yaml and asserts _discover_refs carries all four onto the ProposalRef. You are right that this was the load-bearing gap: seeding the ref in memory hides it, and a regression there would silently make every cap non-binding — an unbounded series of paid SDK runs, the exact failure the counters exist to prevent. One test pins both counters, so the harness_failures parity gap closes with it.

2. refusals is now per head SHA, not per PR lifetime. New refusals_head field: a refusal increments only when the recorded head matches the PR's current head, and otherwise starts the count at 1. That makes the counter mean what the MAX_REFUSALS comment claims — "the same bundle produces the same answer" only holds while the branch has not moved — and removes the milder #360 replay you describe, where refusals against a superseded bundle push a healthy PR to review-stuck. Cleared alongside refusals everywhere, including reconcile_one's un-stick path (test_unstick_clears_the_refusal_head_too — a cleared counter with a stale head would resume counting at the cap).

Tests: test_refusals_accumulate_while_the_head_is_unchanged, test_refusals_reset_when_the_branch_moves.

One case deliberately still counts: the reviewer re-running on the same head and producing a different bundle (which #359 makes likely). The head is the only evidence the shepherd has that the code changed, and I would rather the cap be slightly conservative there than key it on a findings hash that #359's re-anchoring churn would make meaningless. At the cap the review-stuck note says explicitly that review_attempts was never charged and the proposal is not at fault, so the human sees the right framing.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3.

P2: .implementer-refusal.json gets no .git/info/exclude entry, unlike the other runtime file staged into the same worktree. If a broad git add -A ever commits it, the marker becomes tracked on that branch and _read_refusal_marker ignores every subsequent refusal — #360 silently reverts for that PR, permanently. One line in _stage_implementer_agent.

Otherwise the design holds up: the marker contract errs toward charging in every ambiguous case, 47 is classified on its own rather than folded into either existing set, the exit code (not the advisory reason file) carries the decision, review_attempts is untouched on the refusal path, and the bound lives on a separate counter that reconcile clears. test_42_43_44_still_charge_an_attempt_end_to_end is the right guard against weakening #12.

Note: I could not run pytest/ruff/mypy in this environment (command approval), so the author's reported 1066 passed is unverified here.

[mashkovd]

Round 1 addressed in 8097ea4 — 1 P2 and 3 P3, each answered on its thread with the change.

• P2 .git/info/exclude for the marker. _stage_implementer_agent registers both entries; the docstring now states that the exclude entry is the defence and the untracked check the backstop, since the failure it prevents is silent and permanent for that branch. Asserted against real git (git add -A leaves the marker unstaged, and it is still honoured afterwards).
• P3 prompt example is one line of valid JSON, verified by json.loads on the rendered prompt; the marker contract added to all nine implementer.md sub-agent definitions, with a test that every definition carries it.
• P3 _read_refusal_reason caps at MAX_NOTES_CHARS at the trust boundary rather than at each consumer.
• P3 refusals is now counted per head SHA (refusals_head), and _discover_refs has a round-trip test over every counter — that read is the only thing that makes any cap bind.

Audit: can the refusal path reach the counter-less transient arm?

Prompted by the drain_until_settled finding on #367 (a non-positive timeout raised at the point of use, got swallowed into a generic exit 1, and landed in the shepherd's transient arm — which has no counter, so one bad env var produced an unbounded paid retry loop). I traced every way the refusal path can fail:

failure	lands on	bounded by
marker missing / malformed / refused not true / blank reason	_read_refusal_marker returns None → "implementer produced no follow-up commits" → 42	MAX_REVIEW_ATTEMPTS
marker unreadable (OSError)	same as above — the except is (OSError, ValueError) → 42	MAX_REVIEW_ATTEMPTS
--refusal-out path unwritable	_write_refusal_out warns and returns; the run still exits 47	MAX_REFUSALS
--refusal-out file missing or malformed on the shepherd side	_read_refusal_reason returns None; kind is still refused (the exit code decides, not the file)	MAX_REFUSALS
git ls-files exceeding the command timeout	ImplementerOperationTimeout → "operation timed out:" → 44	MAX_REVIEW_ATTEMPTS

No arm falls through to the generic exit 1. The one path that reaches transient is a subprocess.CalledProcessError, and the tracked-check runs with check=False so it cannot raise one. test_apply_followup_refusal_survives_a_missing_reason_file pins the third row specifically — the decision must not depend on a best-effort file.

Still stacked on #367; I will rebase once that branch settles.

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit ca7ee7a.

No significant issues found.

[claude]

Claude finished @mashkovd's task in 4m 43s —— View job

---

Code review — c85cf6a

• Read repository conventions (CLAUDE.md / AGENTS.md)
• Read the pinned diff (2c72f6c...c85cf6a)
• Analyze the refusal-marker contract in run_implementer.py
• Analyze classification and the process_one refusal path in run_shepherd.py
• Assess test coverage
• Post inline findings and submit the verdict

Verdict: changes requested — 0 P1, 1 P2, 2 P3. Review submitted against c85cf6a43975621723a33ce2eb48f226dfc02ce7.

P2 — the refusal arm does not clear harness_failures

orchestrator/run_shepherd.py:2090. harness_failures is cleared on success (L2208) and on a deterministic failure (L2185), where test_deterministic_failure_also_clears_the_harness_counter spells out why: "'Consecutive' has to be true of every proof that the handoff works … the child ran to a terminal state and the driver adjudicated its output." Exit 47 is the strongest such proof in the set — the child ran, reasoned, wrote a marker, exited on a sentinel, and apply_followup read the reason back through the real --refusal-out plumbing. The new arm is the only adjudicated-terminal path that leaves the counter standing.

So on one head SHA, 46, 47, 46, 47, 46 trips MAX_HARNESS_FAILURES and flips to review-stuck with "The platform lost the implementer's work 3 time(s) in a row … This is a harness defect … see mctl-agents#366." Three orphans across five ticks with two clean terminal runs between them is not a harness defect, and it lands there two ticks before MAX_REFUSALS would have got there with the correct note ("review_attempts was never charged — the proposal is not at fault"). One kwarg on both update_status calls in the arm; extending the existing test to kind="refused" beats a near-duplicate, since the invariant is "every terminal, adjudicated child run clears the harness counter".

P3

• agents/*/.claude/agents/implementer.md:68 — the marker bullet opens broadly ("When you stop without committing on a review follow-up … ALSO write") and sits directly above "If the proposal is unclear, self-contradicting, or an open question blocks it, STOP without committing". That second case is the PR's own "spec unimplementable" row, which must charge. Nothing excludes it, so a genuine failure can be recorded as a refusal — bounded (MAX_REFUSALS 3 < MAX_REVIEW_ATTEMPTS 5), but it converges to a note asserting the proposal is not at fault when it is. The outer _build_prompt step 5 is correctly scoped; only the nine sub-agent definitions need the clause.
• orchestrator/run_implementer.py:236 — every other check in _read_refusal_marker treats "cannot establish the fact" as not a refusal; this one inverts it. Only returncode == 0 counts as tracked, so any other git ls-files failure reads as untracked and the marker is honoured. Low probability inside a fresh clone, but it is the single exception to the rule the docstring above it states absolutely.

What holds up

Round 1 was addressed properly, not papered over. The .git/info/exclude entry as the defence with the untracked check as the backstop is the right pairing (and correctly noted that exclude does nothing for already-tracked files). _read_refusal_reason capping at the trust boundary makes the backstop terminal for every consumer rather than a per-call-site discipline. The prompt example is one line of valid JSON, pinned by json.loads over the rendered prompt. refusals anchored to refusals_head makes the counter mean what MAX_REFUSALS claims, and reconcile_one clears both alongside the others. _discover_refs now round-trips every counter — the read that is the only thing making any cap bind. 47 is classified in its own set rather than widening #367's tuple, the deferred run_implementer import keeps the Temporal worker off the SDK, the exit code (not the advisory reason file) carries the decision, and test_42_43_44_still_charge_an_attempt_end_to_end guards that #360 does not weaken #12.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported "1077 passed, 8 skipped" is the author's result, not one reproduced here.
· branch fix/agents-360-refusal-not-an-attempt

[mashkovd]

Rebased onto main (2c72f6c) — single commit c85cf6a, squashed so the review-round history does not replay as separate conflicts. No longer stacked; fix/agents-366-await-async-subagent is untouched.

What #367 changed at this seam, and how it was reconciled:

• FollowupKind gained "refused" rather than the type widening back to str, with the comment extended to say why it belongs in the Literal for the same reason as "harness": it is bounded by MAX_REFUSALS, and a typo would fall through to the counter-less transient arm. FollowupSubprocessError keeps main's kind-only signature (transient stays the derived property) plus the reason keyword.
• update_status's counter write-back — main replaced the per-field is not None guards with a loop that mirrors "None means delete the key" as a reset to 0. refusals joined that loop; refusals_head is handled one line below because it clears to None, not 0.
• reconcile_one un-stick — main's test_reconcile_unstick_clears_both_counters is now test_reconcile_unstick_clears_every_counter and covers refusals and refusals_head too, with the docstring stating the invariant ("every counter that can flip a proposal to review-stuck is cleared here") rather than enumerating the current two.
• Counter round-trip — main's test_harness_failures_round_trips_through_disk and my test_discover_refs_round_trips_every_counter were near-duplicates; they are one test, test_counters_round_trip_through_disk, over all four fields, including the absent-key defaults. My duplicate is gone.
• _positive_seconds — not applicable: the refusal path reads no wall-clock knob from the environment and adds no new timeout. It reuses _run's existing IMPLEMENTER_COMMAND_TIMEOUT_SECONDS for the one git ls-files call, and that call is check=False, so it cannot raise into the counter-less arm either (the timeout path maps to 44).

Gates on the rebased tree: uv run pytest -q → 1092 passed, 8 skipped; ruff check and mypy clean.

All four findings from round 1 (1 P2, 3 P3) were fixed before the rebase and answered on their threads; the P2 exclude-entry fix and the per-head refusals counter are both in this commit. The review has auto-fired on c85cf6a.

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit c85cf6a.

No significant issues found.

[claude]

P2 — the refusal arm does not clear harness_failures, so an alternating 46/47 series reports a platform incident that did not happen.

test_deterministic_failure_also_clears_the_harness_counter (tests/test_run_shepherd.py:3500) codifies the invariant this arm misses:

"Consecutive" has to be true of every proof that the handoff works. A deterministic 42/43/44 proves it as well as a success does: the child ran to a terminal state and the driver adjudicated its output. Without clearing here, the interleaving 46, 42, 46, 42, 46 reaches the cap and reports "the platform lost the work 3 time(s) in a row" — false, and that counter is what an operator reads to decide whether this is a platform incident.

Exit 47 is the strongest proof of a working handoff in the whole set: the child ran, reasoned, wrote a marker, exited on a sentinel, and apply_followup read the reason back out of the real --refusal-out file. Yet harness_failures is cleared on success (L2208), on a deterministic failure (L2185) — and not here.

Concrete sequence, all on one head SHA:

1. tick 1 — 46 (orphaned sub-agent): harness_failures 0 -> 1.
2. tick 2 — 47: refusals 0 -> 1; harness_failures stays 1.
3. tick 3 — 46: 1 -> 2.
4. tick 4 — 47: refusals 1 -> 2.
5. tick 5 — 46: 2 -> 3 == MAX_HARNESS_FAILURES -> review-stuck, notes: "The platform lost the implementer's work 3 time(s) in a row ... This is a harness defect ... see mctl-agents#366."

Three orphans spread across five ticks with two clean terminal runs in between is not a harness defect, and the note points the operator at #366 instead of at the standoff that MAX_REFUSALS was written to surface. The proposal also lands in review-stuck two ticks before MAX_REFUSALS would have got there with the correct note ("review_attempts was never charged — the proposal is not at fault").

Fix is one kwarg on both update_status calls in this arm (L2066-2091), matching L2185: harness_failures=None. Worth extending test_deterministic_failure_also_clears_the_harness_counter to the refusal kind rather than adding a near-duplicate — the invariant is "every terminal, adjudicated child run clears the harness counter".

(The transient arm not clearing it is not the same case: it writes nothing to .status.yaml at all by design, and a transient exit does not prove the driver adjudicated anything.)

Fix this →

[mashkovd]

Fixed in aae6b76 — correct, and the sequence you traced is exactly right: three orphans across five ticks with two clean terminal runs in between is not a platform incident, and the note sent the operator to #366 instead of to the standoff.

harness_failures=None on both update_status calls in the arm, and the arm's opening comment now carries the rule rather than leaving it implicit.

The mirror, since you raised the invariant rather than the instance. Checking both directions gave two rules:

• any outcome proving the handoff worked clears harness_failures — success, deterministic, refused;
• any outcome proving the agent engaged with the findings clears refusals/refusals_head — success, deterministic.

The deterministic arm was the asymmetric one, so it now clears refusals/refusals_head at the same call site where it already cleared harness_failures; the run of refusals before it is no longer consecutive, and that tick is charged to review_attempts, which bounds the sequence from there.

The mirror deliberately stops short in one place: a harness failure clears neither. The child never reached a terminal state, so it proves nothing about the findings — and, decisively, now that 47 clears the harness counter, a 46/47 alternation that also cleared refusals would trip neither cap and run as an unbounded paid loop. That is the one outcome both counters exist to prevent, so the asymmetry is load-bearing and is stated where the code makes the choice.

Pinned by one table, per your steer and the un-stick precedent:

• test_counter_contract_across_outcomes — parametrised over success | deterministic | refused | harness, seeding all three counters on disk as well as in memory (the harness arm writes only its own key, so "untouched" is only observable in the file). A new outcome kind is a missing row, not a test nobody writes.
• test_alternating_harness_and_refusal_reaches_the_refusal_cap — drives 46, 47, 46, 47, 46, 47 and asserts it converges on MAX_REFUSALS with review_attempts still 0, the note saying "not at fault", and no "harness defect" claim in it.

Both fail without the one kwarg (verified by removing it).

[claude]

P3 — this bullet sits directly above the "blocked, so stop" bullet, and nothing tells the agent the marker does not apply there.

The opening clause is broad ("When you stop without committing on a review follow-up ... ALSO write") and only the parenthetical narrows it. The very next bullet is:

If the proposal is unclear, self-contradicting, or an open question blocks it, STOP without committing and explain what's missing in your final message.

That is the "spec unimplementable" row of the PR description's own table — the one that should charge an attempt. An agent that reads the two bullets together and writes the marker when blocked converts a genuine failure into a non-charged no-op, which is the exact asymmetry the design argues against ("matching a crash narration as a refusal would exempt a genuinely broken proposal from the cap"). The blast radius is bounded — MAX_REFUSALS (3) is tighter than MAX_REVIEW_ATTEMPTS (5), so it still converges — but it converges to a review-stuck note asserting "the proposal is not at fault" when the proposal is precisely what is at fault, which is what a human reads to decide where to look.

One clause closes it, e.g. "This is only for a finding you decided not to act on. If you are blocked — missing context, unclear spec, an unimplementable finding — stop and explain, but do NOT write the marker: that is a failed attempt, not a refusal." Same asymmetry the outer prompt already states; the sub-agent is the one actually making the call.

The outer prompt (_build_prompt step 5) is correctly scoped, so this is only about the nine sub-agent definitions. All nine carry identical text, so it is one edit replicated.

Fix this →

[mashkovd]

Fixed in aae6b76 — and you picked the one direction this design must not err in: mistaking a blocked run for a refusal converts the case that must charge the cap into one that does not, and then tells a human "the proposal is not at fault" about the proposal that is precisely at fault. Bounded blast radius is not much comfort when the note points the triage in the wrong direction.

All nine definitions now end the bullet with:

And ONLY for a finding you decided not to act on — if you are BLOCKED (missing context, an unclear or self-contradicting spec, an unimplementable finding), stop and explain as the next bullet says, but do NOT write the marker. That is a failed attempt, not a refusal, and it must still charge the cap: recording it as a deliberate decision would tell a human "the proposal is not at fault" about the one case where the proposal is exactly what is at fault.

It names the next bullet explicitly, since adjacency was what made the two readable as one instruction. test_subagent_definitions_exclude_the_blocked_case asserts every definition carries the exclusion, alongside the existing test that every definition carries the contract at all — so a new per-service implementer cannot ship with half of it.

[claude]

P3 — this is the one check in the function that errs toward "it is a refusal" when it cannot tell.

Every other branch below treats "I could not establish the fact" as "not a refusal": unreadable file, unparseable JSON, refused not exactly True, reason missing or blank. This one inverts it — only returncode == 0 is treated as tracked, so any other failure of the git ls-files invocation (git missing from PATH, a corrupt or absent index, an unexpected 128) reads as "untracked" and the marker is honoured.

In practice the probability is low: _read_refusal_marker only runs inside a fresh _clone_target clone, and a timeout raises ImplementerOperationTimeout before reaching here rather than returning a code. But the docstring right above states the rule as absolute — "Every check below exists to make 'the agent refused' something that cannot be produced by accident" — and this line is the exception to it. Distinguishing 0 (tracked) / 1 (untracked) / anything else (unknown, so do not honour) costs one condition and makes the function match what it says about itself.

Fix this →

[mashkovd]

Fixed in aae6b76 — agreed, and the argument that the docstring states the rule as absolute while this line was the exception to it is the one that settles it, independently of how likely the failure is.

Now: 0 is tracked (ignore), 1 is the real "no such path in the index" answer (honour), anything else — git missing from PATH, a corrupt or absent index, an unexpected 128 — is not an answer at all and returns None with a warning. The docstring's second bullet says "UNTRACKED, provably so" and spells out that an invocation which fails to answer the question counts as a no, not a yes.

Two tests: test_unknown_ls_files_result_is_not_treated_as_untracked (stubs exit 128 and asserts the marker is discarded) and test_exit_one_is_still_honoured_as_untracked, so the narrowing cannot quietly break the ordinary case it guards.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3.

P2: the kind == "refused" arm of process_one does not clear harness_failures, unlike the success path (L2208) and the deterministic path (L2185). test_deterministic_failure_also_clears_the_harness_counter already states the invariant this arm misses — exit 47 proves the handoff works at least as well as a 42 does. An alternating 46/47 series on one head reaches MAX_HARNESS_FAILURES and flips to review-stuck reporting "the platform lost the work 3 time(s) in a row", which is false and points the operator at #366 instead of at the standoff MAX_REFUSALS exists to surface. One kwarg on both update_status calls.

P3: the sub-agent implementer.md bullet does not exclude the blocked/unimplementable case that sits in the very next bullet, so a genuine failure could be recorded as a refusal; and _read_refusal_marker treats any non-zero git ls-files result as untracked, the one check in that function that errs toward honouring the marker.

Round 1 was addressed well. The exclude entry plus the tracked-check backstop is the right pairing, the cap now lives at the trust boundary in _read_refusal_reason, the prompt example is one line of valid JSON asserted by json.loads, refusals is anchored to a head SHA, and _discover_refs round-trips every counter — the read that makes any cap bind. test_42_43_44_still_charge_an_attempt_end_to_end is the right guard that #360 does not weaken #12.

Note: pytest/ruff/mypy needed command approval in this environment, so the reported 1077 passed is the author's result, not one reproduced here.

[claude]

Claude finished @mashkovd's task in 5m 32s —— View job

---

Code review — aae6b76

• Read repository conventions (CLAUDE.md / AGENTS.md)
• Read the pinned diff (2c72f6c...aae6b76)
• Verify round-2 findings were addressed
• Analyze the refusal-marker contract in run_implementer.py
• Analyze classification and the process_one refusal path in run_shepherd.py
• Assess test coverage
• Post inline findings and submit the verdict

Verdict: approved — 0 P1, 0 P2, 1 P3. Review submitted against aae6b761048bda4b5b37d0f6fba979aab508d13c.

Round 2 is closed, and generalised rather than patched

The P2 fix is the right shape: the refused arm clears harness_failures, and the mirror was followed through — the deterministic arm now clears refusals/refusals_head for the same reason it already cleared the harness counter. The one place the mirror deliberately stops (a harness failure clears neither) is argued in run_shepherd.py:2050-2068 and, importantly, the argument is now load-bearing: with 47 clearing the harness counter, a 46/47 alternation that also cleared refusals would trip neither cap and become the unbounded paid loop both counters exist to prevent.

I traced every interleaving of {success, deterministic, harness, refused} on one head SHA and each converges on exactly one cap:

sequence	converges on
47, 47, 47	MAX_REFUSALS → review-stuck, "not at fault"
46, 47, 46, 47, 46, 47	MAX_REFUSALS (the regression test), not "harness defect"
47, 42, 47, 42, …	MAX_REVIEW_ATTEMPTS (each 42 clears refusals and charges)
47, success, 47, success, …	MAX_REVIEW_ATTEMPTS
46, 46, 46	MAX_HARNESS_FAILURES (unchanged)

COUNTER_CONTRACT as a parametrised table over all four kinds is the right test for this — a fifth kind is a missing row rather than a test nobody wrote — and the seeding-on-disk detail matters, since "untouched" is only observable in the file.

Both P3s are closed as well. _read_refusal_marker now distinguishes 1 (genuinely not in the index) from any other non-zero (fact not established → not a refusal), which makes the function match the absolute rule its docstring states. And all nine implementer.md definitions now exclude the blocked/unimplementable case explicitly — that mattered because the excluded case is the only one that must charge, and recording it as a refusal would land a human on a note asserting "the proposal is not at fault" about a faulty proposal. The exclusion is pinned by test_subagent_definitions_exclude_the_blocked_case, and a blocked run still exits 42 → deterministic → charged, so the design table holds end to end.

P3 (non-blocking)

• orchestrator/run_shepherd.py:2098 — the review-stuck note interpolates reason last and slices the whole f-string at MAX_NOTES_CHARS, so ~210 chars of fixed prose eat the budget and a reason the trust boundary admits at 700 is clipped to ~490. It is the terminal note on the one path whose argument is "a human must reconcile this", and the agent's evidence is the part that gets dropped. Budget the reason instead of the sum, or put it first. The wait note is unaffected (28-char prefix).

What holds up

Nothing in the earlier rounds regressed under the rebase. The .git/info/exclude entry as the defence with the untracked check as the backstop, _read_refusal_reason capping at the trust boundary, the one-line JSON example pinned by json.loads over the rendered prompt, refusals anchored to refusals_head, _discover_refs round-tripping every counter (the read that makes any cap bind), 47 classified in its own set rather than widening #367's tuple, the deferred run_implementer import keeping the Temporal worker off the SDK, and test_42_43_44_still_charge_an_attempt_end_to_end guarding that #360 does not weaken #12. Dataclass field insertion is safe — every ProposalRef(...) construction in orchestrator/ and tests/ is keyword-only — and _update_status_if_changed already treats None as delete-if-present, so reconcile_one clearing refusals_head cannot cause per-tick GitOps churn.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1092 passed / 8 skipped is the author's result, not one reproduced here.
· branch fix/agents-360-refusal-not-an-attempt

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit aae6b76.

Here is the review of the provided diff.

Severity: P2
File: orchestrator/run_implementer.py (approx. line 201)
Anchor: data = json.loads(path.read_text(encoding="utf-8"))
Concrete failure scenario: A malicious repository (or a compromised dependency executing a script during the implementer's sanity check) can write an extremely large (e.g., multi-gigabyte) .implementer-refusal.json file to the workspace. Because path.read_text() reads the entire file into memory at once without any size limit, this will cause the orchestrator Python process to crash with an out-of-memory (OOM) error, leading to an uncontrolled denial of service.
Proposed fix: Check the file size before attempting to read it, and ignore the marker if it exceeds a reasonable maximum bound (e.g., 100 KB). For example: if path.stat().st_size > 100 * 1024: return None.

[claude]

P3 — the reason is last in this note and shares the 700-char budget with ~210 chars of fixed prose, so the one field a human needs can be clipped mid-sentence.

MAX_NOTES_CHARS is 700 and _read_refusal_reason already caps the reason at exactly that, so the slice on this f-string is charged against the sum: the fixed prose ("The implementer declined to act N time(s) … Last reason: ") is ~210 characters, leaving ~490 for a reason that the trust boundary happily admits at 700 (or 600 when the implementer wrote it). Everything beyond that is dropped, and because the reason is interpolated last it is the only part that can be dropped.

This is the terminal note on the one path whose whole argument is "a human must reconcile the review with the operator decision" — the fixed prose is boilerplate they can reconstruct, the agent's evidence ("quote the operator note, or the code that already satisfies the finding") is not. The wait note above is unaffected: its prefix is 28 characters, so a 600-char reason survives whole.

Budgeting the reason rather than the whole string keeps the cap terminal and the evidence intact, e.g. build the prose first and interpolate reason[:MAX_NOTES_CHARS - len(prose)], or put Last reason: {reason} at the front. Not a blocker — the truncation is graceful and only bites long reasons.

[mashkovd]

Fixed in 3b8bdd6 — budgeting the reason, not the whole string.

_stuck_note(refusals, reason) builds the fixed prose first and gives the reason MAX_NOTES_CHARS - len(prose), with the docstring stating why this note in particular gets that treatment: its entire argument is "a human must reconcile the review with the operator decision", the boilerplate is reconstructible, and the evidence the agent quoted is not.

Two tests: test_stuck_note_spends_its_budget_on_the_evidence (a 4000-char reason yields a note exactly at the cap that still ends in the reason, so the prose cannot silently eat the budget again) and test_refusal_cap_note_keeps_the_reason end to end through .status.yaml.

Left the wait note as a plain slice, as you noted — a 28-char prefix cannot crowd out a reason the trust boundary already caps.

[claude]

No P1/P2 findings (1 P3). Good to merge.

Round 2's P2 is fixed and generalised: the refusal arm clears harness_failures, the deterministic arm now clears refusals/refusals_head, and the one asymmetry that must hold — a harness failure clears neither — is argued in the code and pinned by COUNTER_CONTRACT plus the 46/47 alternation regression. I traced every interleaving of {success, deterministic, harness, refused} on one head: each converges on review_attempts (5), harness_failures (3) or refusals (3), so no sequence reaches the counter-less transient arm. Both P3s are closed too — git ls-files now distinguishes 1 (untracked) from anything else (unknown, not honoured), and all nine implementer.md definitions exclude the blocked/unimplementable case that must still charge.

P3 (non-blocking): the review-stuck refusal note interpolates the reason last and slices the whole string at MAX_NOTES_CHARS, so ~210 chars of fixed prose eat into the budget and a long reason — the one field the human needs to settle the standoff — is clipped. Inline comment has the suggestion.

Note: pytest/ruff/mypy needed command approval in this environment, so the reported 1092 passed, 8 skipped is the author's result, not one reproduced here.

[mashkovd]

agy's P2 (unbounded marker read) — fixed in 3b8bdd6

Taking it on the reasoning that makes it stand rather than the malicious-repo framing, which I agree is the weakest version of the argument:

• the marker lives in a cloned target repo's worktree and is written by an LLM holding a Bash tool, so a redirect into the wrong path, a loop that appends, or a stray tee produces an enormous file with no malice required;
• every other check in _read_refusal_marker already treats the marker as untrusted input — size was the one dimension it did not check;
• an implementer OOM is the least diagnosable failure available here: memory pressure on that workload is a live issue, and an OOMKill does not surface in the container's last_terminated_reason.

stat() before read_text(), refusing above MAX_REFUSAL_MARKER_BYTES (64 KiB — generous next to a legitimate marker whose reason is itself capped at 600 chars), with the constant's comment carrying that reasoning.

Not a truncating read, deliberately: a truncated marker would fail the JSON check and land on the charged path, which is the right outcome for the wrong reason and would hide an operational problem behind a correct-looking one. Refusing outright and warning is honest about what happened.

The same unbounded read_text existed on the shepherd side (_read_refusal_reason), on the same class of file, so it has the same bound — the exit code carries the decision there, so refusing by size costs only the prose.

Tests:

• test_oversized_marker_is_refused_without_being_read — makes read_text fatal, so an implementation that ever reads first and checks afterwards fails here instead of quietly reintroducing the unbounded read. That is the property; the size check is only how it is achieved today.
• test_a_marker_at_the_cap_is_still_honoured — inclusive boundary, so the bound cannot clip a legitimate marker.
• test_oversized_marker_falls_back_to_the_charged_path — refusing the marker leaves the ordinary 42 intact rather than crashing.
• test_vanished_marker_between_stat_and_read_is_not_a_crash — is_file() then stat() is a race by construction; it degrades to "not a refusal".
• test_oversized_refusal_reason_file_is_not_read — the shepherd-side mirror.

Gates on 3b8bdd6: uv run pytest -q → 1107 passed, 8 skipped; ruff check and mypy clean.

Worth recording for #359's neighbourhood: claude[bot] approved aae6b76 with 0 P1/P2 while agy failed the same commit on this P2 — two reviewers, one head, opposite verdicts. The finding was real.

[claude]

P3 — this is a no-op refactor: it produces byte-for-byte the same note the sliced f-string did, so the P3 it answers is still open.

For any prefix p with len(p) <= N, (p + r)[:N] == p + r[:N - len(p)]. That is exactly the transformation here: prose is unchanged (the line re-wrapping does not alter the concatenated text), reason was already interpolated last, and the old slice was at the same MAX_NOTES_CHARS. So both forms yield prose plus the first 700 - len(prose) (~487) characters of the reason.

Which means the docstring's claim is not true of the code: it says it "budgets MAX_NOTES_CHARS for the REASON rather than for the whole string", but the reason's budget is still MAX_NOTES_CHARS - len(prose) — the fixed prose still spends from the same 700. An implementer whose reason is at its own 600-char cap still loses ~113 characters off the end, on the one note whose argument is that the agent's evidence is the part a human cannot reconstruct.

test_stuck_note_spends_its_budget_on_the_evidence does not catch this, and cannot: len(note) == MAX_NOTES_CHARS and note.endswith("E" * 400) were both already true of the sliced version, so the test passes against the code it was written to rule out. Its comment ("not what is left of a budget the boilerplate already spent") describes a property neither form has.

Two ways to actually close it, either fine:

• give the reason the full MAX_NOTES_CHARS and let the note exceed it (the cap is on agent-authored prose, and the prose here is ours) — then assert note.count("E") == MAX_NOTES_CHARS;
• or put Last reason: {reason} first and the boilerplate after, so the truncation drops the reconstructible half — then assert the reason survives whole at the implementer's 600-char cap.

Either way the test wants an assertion that fails against a plain (prose + reason)[:MAX_NOTES_CHARS], since that is the implementation being ruled out.

Fix this →

[mashkovd]

Fixed in d99d4d1 — budgeting is now real rather than algebraically identical.

You are right on the arithmetic, and the consequence you drew from it matters more than the bug: the test passed against the implementation it was written to forbid, so it was documenting the old behaviour while claiming to rule it out.

Took your first option. MAX_NOTES_CHARS now bounds the agent-authored portion of a note rather than the finished note — our framing is fixed-size and reconstructible, the evidence is not, so charging boilerplate against the same budget spends the wrong thing. The constant's comment says that explicitly, since "bound on a note" was the reading that made the slice look right.

_stuck_note is prose + reason[:MAX_NOTES_CHARS], and test_stuck_note_gives_the_whole_budget_to_the_evidence asserts note.count("E") == MAX_NOTES_CHARS — which is 700 here and 487 under the plain slice, so it fails against the implementation being ruled out. test_a_boundary_capped_reason_survives_whole_in_both_notes pins the realistic case: nothing the trust boundary admits is ever clipped by the framing around it.

Two existing tests moved with the contract rather than being left asserting the old one: test_refusal_note_is_bounded now asserts the agent's text is capped and the note is still bounded (a fixed prefix plus a bounded reason is bounded), and test_refusal_cap_note_keeps_the_reason asserts the reason survives whole instead of merely being present.

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit 3d1880b.

Severity: P2
File: orchestrator/run_implementer.py
Line: ~104
Anchor: + data = json.loads(path.read_text(encoding="utf-8"))

The _read_refusal_marker function attempts to prevent an out-of-memory (OOM) denial of service by checking path.stat().st_size against MAX_REFUSAL_MARKER_BYTES before reading the file. However, this relies on a Time-of-Check to Time-of-Use (TOCTOU) pattern. Because the marker file .implementer-refusal.json resides in the repository workspace, the untrusted LLM agent can launch a background process (e.g., a bash loop) that continuously appends to this file. The stat() check may observe a small size that passes the limit, but the subsequent path.read_text() reads the file until EOF. If the background process keeps writing, read_text() will continuously buffer data until the orchestrator process exhausts all memory and crashes, bypassing the cap and causing an OOM DoS.

To fix this, do not rely on stat() to enforce the size cap. Open the file and explicitly read up to the maximum allowed bytes, rejecting it if it exceeds the limit:

        with path.open("r", encoding="utf-8") as f:
            content = f.read(MAX_REFUSAL_MARKER_BYTES + 1)
        if len(content) > MAX_REFUSAL_MARKER_BYTES:
            print(f"warn: {REFUSAL_MARKER_FILENAME} is over the cap")
            return None
        data = json.loads(content)

Severity: P3
File: orchestrator/run_shepherd.py
Line: ~350
Anchor: + data = json.loads(marker.read_text(encoding="utf-8"))

Similar to run_implementer.py, _read_refusal_reason relies on stat().st_size before calling read_text(), which introduces the same TOCTOU pattern. While this specific file is a temporary file managed by the orchestrator outside of the agent's workspace (making it inaccessible to the agent and thus not exploitable in the same way), it replicates an unsafe file-reading pattern that fails to safely bound memory usage if the file were to change.

Replace stat and read_text with an open and bounded read, mirroring the proposed fix for run_implementer.py:

        with marker.open("r", encoding="utf-8") as f:
            content = f.read(MAX_REFUSAL_FILE_BYTES + 1)
        if len(content) > MAX_REFUSAL_FILE_BYTES:
            print(f"warn: refusal reason file {path} is over the cap")
            return None
        data = json.loads(content)

[claude]

P3 — the stated reason for duplicating the bound is not the actual one, and the real one has a cheaper fix.

The comment says the constant is duplicated because "this is read on a path that runs before that import". It is not: _read_refusal_reason is called at L1788, seven lines after apply_followup already does from orchestrator import run_implementer (L1781) — and _refusal_codes() imports it again on the same call. The genuine reason is narrower and sufficient: this is a module-level constant, so it cannot reference a deferred module at all. Worth correcting, since the comment is the thing a future reader will trust when deciding whether the copy is still needed.

Given that, the duplication is avoidable: read it inside _read_refusal_reason from the module it belongs to (run_implementer.MAX_REFUSAL_MARKER_BYTES, deferred import inside the function, exactly like _refusal_codes), and the two bounds cannot drift. If you prefer the module-level constant for readability, a one-line equality assertion in the tests pins the copy instead — nothing else in the diff would notice a silent drift here, since both values are generous and either one "works".

Fix this →

[mashkovd]

Fixed in d99d4d1 — kept the module-level constant, corrected the comment, added the drift test.

You are right that the stated reason was false: _read_refusal_reason is called after apply_followup's deferred import, not before it. The real constraint is the narrower one you name — a module-level constant cannot reference a deferred import at all — and that is what the comment says now, along with why the equality is load-bearing in one direction (this is the outer bound, so a raised implementer-side cap left un-mirrored would honour a marker whose reason the shepherd then refuses to read, losing the evidence for exactly the oversized case the bound exists to surface).

I kept the constant rather than importing inside the function: at the point of use the shepherd is validating a file it did not necessarily write, and a module-level bound reads as a property of this module's input handling rather than as a borrowed value. test_the_duplicated_byte_cap_matches_the_implementers makes the copy fail loudly instead of drifting — the same treatment as COUNTER_CONTRACT and the sub-agent contract tests, per your point that the rule should be in a test and not only in prose.

[claude]

No P1/P2 findings (2 P3). Good to merge.

Round 3 closed agy's P2 in the right shape: stat() before read_text() on both sides, refused outright rather than truncated (a truncated marker would fail the JSON check and reach the charged path for an accidental reason), the boundary inclusive so a legitimate marker cannot be clipped, and the stat/read race degrading to "not a refusal". test_oversized_marker_is_refused_without_being_read making read_text fatal pins the property rather than today's implementation.

I traced the refusal path end to end on this commit and found no way for it to reach the counter-less transient arm: every malformed/unreadable/oversized marker returns None -> exit 42 (charged), an unwritable --refusal-out still exits 47, a missing or oversized reason file leaves the kind as refused (the exit code decides), the git ls-files check is check=False so it cannot raise CalledProcessError, and a timeout maps to 44. Counter interleavings on one head each converge on exactly one cap (47xN -> MAX_REFUSALS; 46/47 alternation -> MAX_REFUSALS with no "harness defect" claim; 47/42 and 47/success -> MAX_REVIEW_ATTEMPTS), pinned by COUNTER_CONTRACT plus the alternation regression. _stage_implementer_agent registering the marker in .git/info/exclude (asserted against real git) with the untracked check as backstop, refusals anchored to refusals_head, _discover_refs round-tripping every counter, 47 in its own code set, the deferred run_implementer import, and test_42_43_44_still_charge_an_attempt_end_to_end all hold. No other module or schema enumerates the counter fields, and every ProposalRef(...) construction is keyword-only, so the field insertion is safe.

P3 (non-blocking): _stuck_note is a no-op refactor — prose + reason[:N - len(prose)] is byte-identical to the old (prose + reason)[:N], so the reason still gets ~487 of the 600 chars the implementer can emit, the docstring's claim about the budget is not true of the code, and test_stuck_note_spends_its_budget_on_the_evidence passes against the implementation it was written to rule out. Second P3: the comment justifying the duplicated MAX_REFUSAL_FILE_BYTES states a reason that is not the case (_read_refusal_reason runs after the deferred import, not before). Both inline with suggestions; neither blocks.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1107 passed / 8 skipped is the author's result, not one reproduced here.

[claude]

Claude finished @mashkovd's task in 4m 57s —— View job

---

Code review — d99d4d1

• Read repository conventions (CONTRIBUTING.md / LLMS.md — no CLAUDE.md in this repo)
• Read the pinned diff (197c13b...d99d4d1)
• Verify the round-3 fixes (bounded marker read, _stuck_note, the duplicated byte cap)
• Analyze the refusal-marker contract in run_implementer.py
• Analyze classification and the process_one refusal path in run_shepherd.py
• Assess test coverage
• Post inline findings and submit the verdict

Verdict: changes requested — 0 P1, 1 P2, 2 P3. Review submitted against d99d4d12be23bbf593bc22cca00015c40e1e4d77.

P2 — the shepherd now warns on every healthy follow-up

orchestrator/run_shepherd.py:1833. _read_refusal_reason(refusal_path) is called unconditionally, before the if proc.returncode != 0 test, on a file mkstemp pre-created empty at L1806-1810. So on a successful follow-up — and on 42/43/44, 46, and every transient exit — the child never passed through _write_refusal_out, the file is still zero bytes, and json.loads raises straight into the broad handler:

warn: could not read refusal reason from /tmp/shepherd-refusal-mctl-web-<slug>-xxxx.json (JSONDecodeError: Expecting value: line 1 column 1 (char 0))

address-review, the path that worked, now emits a warn: line every tick. CONTRIBUTING.md keeps warn:/error: as the greppable signal for unattended cron/Argo logs specifically so an operator can filter on it; a warning true of 100% of healthy runs is the fastest way to make that filter worthless — and it is what someone will hit first when they go looking for the real refusal warnings this PR adds.

The comment at L1804-1806 also does not hold up: pre-creating the file to distinguish "empty" from "absent" buys nothing, since both return None with the same warning (test_apply_followup_refusal_survives_a_missing_reason_file already covers the absent case) — the pre-creation is precisely what turns "nothing to say" into "present but unparseable". Gate the read on proc.returncode in _refusal_codes(). No test asserts what a non-refusal run prints, which is how this got through.

P3

• orchestrator/run_implementer.py:294 — the git ls-files call is the one step in _read_refusal_marker outside a handler. A missing git binary raises FileNotFoundError (not a return code — the comment at L302-307 lists "git missing from PATH" among the codes it absorbs, which it cannot be), it is an OSError so review_feedback_one's except subprocess.CalledProcessError misses it, and it lands on the generic handler → exit 1 → the shepherd's counter-less transient arm. That is the escape the docstring above spends a paragraph ruling out for RecursionError, and the one the PR's own audit table says is unreachable from this path. Practically unreachable inside a fresh clone, but the function holds itself to a higher standard than that, and the comment claims coverage it does not have.
• orchestrator/run_shepherd.py:1806 — round 3's leak finding is half-closed. The try moved down to cover the deferred import, but bundle_path is still created above it and this mkstemp is now what sits in the gap; a missing TMPDIR, ENOSPC, or EMFILE here orphans the bundle file on a process that ticks on a schedule.

What holds up

Round 3 is otherwise closed properly. The bounded binary read replaces stat() + read_text() and cannot be raced by a background appender; reading one byte past the cap makes the boundary inclusive without a second syscall; over-cap is refused rather than truncated, so an operational problem does not hide behind a correct-looking charged run. _stuck_note is no longer the no-op slice the last round caught — the reason gets the whole MAX_NOTES_CHARS and the fixed prose sits outside it, and test_refusal_cap_note_keeps_the_reason asserts notes.count("D") == 500, which does fail against (prose + reason)[:N]. The duplicated byte cap's comment now states the real reason (a module-level constant cannot reference a deferred import) and is pinned by an equality test. _declined_note brings the non-terminal note under the same rule. Both broad except Exception handlers are argued from the consequence — an escape exits 1 into the one arm with no counter — and pinned by 20k-deep nesting tests on each side.

Everything from the earlier rounds survives: the .git/info/exclude entry as the defence with the untracked check as the backstop, git ls-files distinguishing 1 from any other non-zero, the exit code rather than the advisory file carrying the decision, all nine implementer.md definitions carrying the contract and excluding the blocked case that must still charge, refusals anchored to refusals_head, _discover_refs round-tripping all four counters, reconcile_one clearing every one on un-stick, the deferred import keeping the Temporal worker off the SDK, COUNTER_CONTRACT plus the 46/47 alternation pinning the four-kind contract, and test_42_43_44_still_charge_an_attempt_end_to_end guarding that #360 does not weaken #12.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1107 passed / 8 skipped is the author's result, not one I reproduced.
· branch fix/agents-360-refusal-not-an-attempt

[mashkovd]

Round 4 — agy's two P2s, fixed in d99d4d1

P2 (a), the RecursionError route. Correct, and the destination is the part that matters: {"refused": true, "reason": [[[[...]]]]} fits inside the 64 KiB cap, json.loads raises RecursionError (a RuntimeError, so (OSError, ValueError) misses it), review_feedback_one's catch-all turns it into RecursionError: ..., _review_feedback_exit_code matches no prefix, and exit 1 is classified transient — the one arm with no counter, so a paid implementer runs every tick forever. (agy's write-up says the process crashes with exit 1; it does not crash, the catch-all catches it. Same destination, so the verdict stands.)

Fixed in the general form rather than by adding RecursionError to a tuple: the handlers around the read, the parse, the --refusal-out write and the shepherd's reader are now broad. That is safe precisely because every failure funnels to "not a refusal" → exit 42, the conservative charged path — and a narrow tuple that must be extended whenever someone finds a new exception type is the same class of bug as a bound written value <= 0 that misses nan. BaseException is deliberately still uncaught. test_any_read_failure_means_not_a_refusal is parametrised over RecursionError, UnicodeDecodeError, MemoryError and a plain RuntimeError and pins the class — the next type nobody enumerated must land on the charged path without anyone adding it first.

P2 (b), the TOCTOU. Also correct, and a bounded read is strictly better, so I took the proposed shape as given. stat() established a fact about a syscall that had already returned, not about the read that followed it; the marker sits in the agent's own workspace with a Bash tool pointed at it, so a background appender passes the check and then makes the read grow without bound.

Both functions now do fh.read(cap + 1) and reject on length — no race, no stat(), and "exactly at the cap" stays distinguishable from "over it" without a second syscall. One refinement on the proposal: the read is binary, because read(n) on a text handle counts characters, which would let a multi-byte payload pull up to 4n bytes past a cap named in bytes; nothing is decoded unless the whole file arrived under the cap, so a partial UTF-8 sequence is never in play.

The stat/read race test is replaced, not kept — a test cannot fix a racy pattern, only document it. test_the_read_itself_is_bounded wraps the handle and asserts the function never asks the file for more than cap + 1 bytes, which is the property; test_an_enormous_marker_is_refused covers a real 5 MiB file.

P3, the shepherd's copy of the pattern. Fixed the same way. agy is right that it is far less exploitable — orchestrator-managed temp path, outside the agent's workspace — and that is in the docstring, along with why it is fixed anyway: an unsafe reading pattern kept "because this caller is fine" is how it ends up copied somewhere that is not.

Re-run of the counter-less-arm audit, on this commit

You asked for the result either way. No route from the refusal path reaches the transient arm:

failure	lands on	bounded by
marker missing / malformed / wrong shape / blank reason	None → "no follow-up commits" → 42	MAX_REVIEW_ATTEMPTS
marker unreadable, undecodable, nested, or any other exception	broad handler → None → 42	MAX_REVIEW_ATTEMPTS
marker over the cap	refused before the read completes → 42	MAX_REVIEW_ATTEMPTS
git ls-files non-0/1, or unavailable	None → 42	MAX_REVIEW_ATTEMPTS
git ls-files exceeding its bound	ImplementerOperationTimeout → 44	MAX_REVIEW_ATTEMPTS
--refusal-out unwritable, or any exception in that write	warned, still exits 47	MAX_REFUSALS
reason file missing / oversized / unparseable	kind stays refused; the exit code decides	MAX_REFUSALS

git ls-files runs with check=False, so it cannot raise CalledProcessError into the generic arm. Two residuals, both pre-existing and neither introduced here: the transient arm itself still has no counter (reachable via review_feedback_one's clone/push failures, which is what it is for), and apply_followup's tempfile calls abort the tick if /tmp is unwritable — the bundle's NamedTemporaryFile has had that exposure since PR #12 and runs first, so the refusal temp file adds no new route.

Gates on d99d4d1: uv run pytest -q → 1151 passed, 8 skipped; ruff check and mypy clean.

Nothing to push back on this round: I checked both findings against the source before taking them, and both are real. The only correction is the crash-vs-catch-all detail above, which does not change either verdict.

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit d99d4d1.

Severity: P2
File: orchestrator/run_shepherd.py, line ~1826
Anchor: + refusal_reason = _read_refusal_reason(refusal_path)
Concrete failure scenario: _read_refusal_reason is unconditionally called after the subprocess finishes, regardless of the exit code. When the implementer succeeds or fails for a non-refusal reason (e.g., exits 0, 42, 46), it intentionally does not write to this file, leaving it completely empty. _read_refusal_reason then attempts to parse the 0-byte file, catches a JSONDecodeError, and unconditionally prints a spurious warning (warn: could not read refusal reason from ...) to the logs. This will heavily spam the orchestrator logs with warnings on almost every non-refusal tick.
Proposed fix: Move the call to _read_refusal_reason inside the block that checks if the exit code indicates a refusal (i.e., if proc.returncode in _refusal_codes():), so it only attempts to parse the marker file when the agent actually signaled a deliberate no-op.

Severity: P3
File: orchestrator/run_shepherd.py, line ~1806
Anchor: + refusal_fd, refusal_path = tempfile.mkstemp(
Concrete failure scenario: The comment states that the try block must cover the paths' whole lifetime to prevent leaking files. However, the try block only begins after tempfile.mkstemp is called. If tempfile.mkstemp fails and raises an exception (for example, an OSError due to the system running out of file descriptors or inodes), the exception propagates immediately without entering the try block, and the previously created bundle_path file is never unlinked in the finally block, causing a temporary file leak.
Proposed fix: Move the try: statement up so that it precedes the tempfile.mkstemp(...) call, ensuring that bundle_path is safely cleaned up in the finally block even if the second temporary file cannot be created.

[claude]

P2 — this runs on every follow-up, and the empty file mkstemp just created makes it warn on every healthy run.

refusal_path is pre-created empty at L1806-1810 (deliberately: "a child that dies before writing leaves an empty file rather than an ambiguous absence"). This read is then unconditional — it happens before the if proc.returncode != 0 test, so it fires on a successful follow-up, on 42/43/44, on 46, and on every transient exit. In all of those the child never passed through _write_refusal_out, so the file is still zero bytes, and json.loads(b"".decode()) raises JSONDecodeError straight into the broad handler:

warn: could not read refusal reason from /tmp/shepherd-refusal-mctl-web-<slug>-xxxx.json (JSONDecodeError: Expecting value: line 1 column 1 (char 0))

So the dominant path of the shepherd — address-review, the one that worked — now emits a warn: line every tick. CONTRIBUTING.md keeps warn:/error: as the greppable signal for unattended cron/Argo logs precisely so an operator can filter on it; a warning that is true of 100% of healthy runs is the fastest way to make that filter worthless, and it will be the first thing someone sees when they go looking for the real refusal warnings this PR adds.

Two things also do not line up with the comment at L1804-1806. The stated benefit of pre-creating the file — distinguishing "empty" from "absent" — is not realised: both land on the same return None with the same warning, and test_apply_followup_refusal_survives_a_missing_reason_file shows the absent case is already handled. And the file being pre-created is what turns "absent, so nothing to say" into "present but unparseable, so warn".

Gating the read on the code that is the only thing it is for keeps the plumbing identical and the log quiet:

        proc = subprocess.run(cmd, check=False, text=True)
        refusal_reason = (
            _read_refusal_reason(refusal_path)
            if proc.returncode in _refusal_codes()
            else None
        )

_refusal_codes() re-imports the already-imported run_implementer, so this costs nothing and stays inside the try/finally. Worth a test too — this block has no coverage of what a non-refusal run prints, which is why a warning on the happy path got through: capsys on an apply_followup that returns 0, asserting "refusal reason" not in out.

Fix this →

[mashkovd]

Fixed in 5f9bbfb — the read is gated on proc.returncode in _refusal_codes().

Settled rather than argued: agy found this independently on the same commit, and the reasoning is right. A warn: on the dominant healthy path is worse than the missing prose it was reporting — it destroys the greppable signal this whole feature exists to produce, and a real refusal warning would arrive buried under one from every success.

The part worth keeping is your closing remark, so I turned it into the test rather than a fix for this instance: nothing asserted what a non-refusal run prints. test_a_non_refusal_run_prints_no_warning is parametrised over 0, 42, 43, 44, 46, 1 and 137 and asserts no warn: on any of them — six of the seven fail without the gate. test_the_empty_marker_file_is_only_read_on_a_refusal states the gate directly (the reader is called exactly once, on 47).

I also checked the adjacent question rather than assuming: refusal_path is still created unconditionally, and the reason is now written at the call site. mkstemp is not there for the child's benefit — _write_refusal_out creates the file if missing — but it is the only stdlib call that reserves a unique path for a child process atomically (mktemp is racy and deprecated), and with the read gated the empty file it leaves is inert. The old comment claiming it distinguishes "died before writing" from "absent" is gone, since the gate makes both cases identical.

[claude]

P3 — this _run sits outside every handler in a function whose docstring argues that enumerating exception types is the bug, and the comment below it names a case that is not a return code at all.

L302-307 lists "git missing from PATH" among the non-zero return codes this branch absorbs. It is not one: subprocess.run(["git", ...]) raises FileNotFoundError when the binary is absent, regardless of check=False, so nothing is returned to inspect. And unlike the file read above and the json.loads below, this call is not wrapped, so the exception leaves _read_refusal_marker entirely. It is an OSError, so review_feedback_one's except subprocess.CalledProcessError does not take it either — it lands on the trailing except Exception, becomes error="FileNotFoundError: ...", matches no prefix in _review_feedback_exit_code, and returns EXIT_GENERIC_FAILURE. Exit 1 is the shepherd's transient arm: the one with no counter, retried on every tick.

That is exactly the escape route the docstring above spends a paragraph ruling out for RecursionError ("an escape from here does not fail safe ... the one arm with no counter at all"), and the same route the PR description's audit table asserts is unreachable from this path. Practically it is unreachable — _clone_target has already run git several times in the same process — but the argument the rest of this function makes is that "practically unreachable" is not the standard it holds itself to, and a comment claiming coverage it does not have is what a future reader will trust.

Two ways to close it, either fine: wrap the _run call in the same broad except Exception → warn → return None as the two reads around it (which also makes the branch below true as written), or drop "git missing from PATH" from the comment and say plainly that the enumerated cases are return codes only. The first is better — it makes the whole function honour the rule its docstring states, at no cost, since "could not establish the fact" already means "not a refusal" here.

Fix this →

[mashkovd]

Fixed across 5f9bbfb and 05f2747 — both halves, and you were right about both.

The unwrapped call. Now inside a broad handler like everything else in the function. It deliberately swallows ImplementerOperationTimeout too: a slow git ls-files would otherwise escape as a 44, asserting that the implementer timed out, when the honest statement is narrower — we could not establish whether the marker is tracked, so it is not a refusal (42). Both charge an attempt; only one of them is true.

The comment naming a case that is not a return code. Corrected. FileNotFoundError from a missing binary never reaches the returncode branches, so listing it there described a path that does not exist; the comment now says explicitly that the handler above absorbs it, not that branch.

Two tests for exactly the routes the comment was wrong about: test_a_missing_git_binary_is_not_a_refusal and test_a_slow_ls_files_is_not_a_refusal_either. Both would previously have escaped to the counter-less transient arm (the first) or mislabelled the run (the second).

[claude]

P3 — the try moved down to cover the deferred import, but bundle_path is still created above it, and this mkstemp is now what sits in the gap.

Round 3's finding was that both temp paths were created before the try; the fix moved the try to L1816, which covers refusal_path but leaves bundle_path (written at L1797-1803) outside it — and inserts a new failure point, this mkstemp, between them. tempfile.mkstemp is not a pure-Python call: a missing or unwritable TMPDIR, ENOSPC, or EMFILE all raise here, and bundle_path is then orphaned. Same failure the round-3 comment was about, on a process that ticks on a schedule; only the leaking file changed.

Closing it is a one-line move — create bundle_path inside the same try as everything else, or start the try immediately after the NamedTemporaryFile block and register refusal_path in the finally list only once it exists:

    bundle_path = ...  # NamedTemporaryFile, as today
    paths = [bundle_path]
    try:
        refusal_fd, refusal_path = tempfile.mkstemp(...)
        os.close(refusal_fd)
        paths.append(refusal_path)
        ...
    finally:
        for path in paths:
            ...

Low stakes on its own — the failure is loud and it is one small JSON file — but the finally was widened specifically to own these paths' whole lifetime, and it does not yet.

Fix this →

[mashkovd]

Fixed in 5f9bbfb — the try now starts above mkstemp.

You are right that round 3's fix only moved the gap rather than closing it: mkstemp is not a pure-Python call, and with the try starting at it, bundle_path was already on disk and the finally was never entered. refusal_path is initialised to None before the try and the cleanup loop skips it, so a failed mkstemp still cleans the bundle.

test_bundle_is_cleaned_up_when_mkstemp_itself_fails forces EMFILE out of mkstemp and asserts the bundle path is gone — the case that was leaking.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3.

P2: apply_followup calls _read_refusal_reason(refusal_path) unconditionally, before the returncode != 0 test, on a file mkstemp pre-created empty. Every non-refusal run — success, 42/43/44, 46, transient — therefore parses zero bytes, raises JSONDecodeError into the broad handler, and prints warn: could not read refusal reason from /tmp/shepherd-refusal-.... A warn: line on 100% of healthy ticks devalues the exact greppable signal CONTRIBUTING.md reserves for unattended logs, and it will be the first thing an operator hits when looking for the real refusal warnings this PR adds. Gate the read on proc.returncode in _refusal_codes(); no test covers what a non-refusal run prints, which is how it got through.

P3: the git ls-files call in _read_refusal_marker is the one step in that function outside a handler — a missing git binary raises FileNotFoundError, escapes to review_feedback_one's generic handler, and exits 1 into the counter-less transient arm, which is the escape the docstring above it spends a paragraph ruling out (and the comment lists "git missing from PATH" as a return code, which it is not). And bundle_path is still created above the widened try, with the new mkstemp now in the gap, so round 3's leak finding is half-closed.

Round 3 is otherwise properly closed. The bounded binary read replaces stat()+read_text() and cannot be raced by an appender; the one extra byte makes the boundary inclusive without a second syscall; over-cap is refused rather than truncated. _stuck_note now genuinely gives the reason the whole budget and lets the fixed prose sit outside it — no longer the no-op slice — and test_refusal_cap_note_keeps_the_reason asserts notes.count("D") == 500, which fails against a plain slice. The duplicated byte cap's comment states the real reason (a module-level constant cannot use a deferred import) and is pinned by an equality test. _declined_note puts the non-terminal note under the same rule. Broad except Exception in both readers is argued from the consequence — an escape lands on exit 1, the one arm with no counter — and both are pinned by a 20k-deep nesting test.

The rest of the design holds end to end: the exclude entry as the defence with the untracked check as the backstop, git ls-files distinguishing 1 from any other non-zero, the exit code rather than the advisory file carrying the decision, refusals anchored to refusals_head, _discover_refs round-tripping all four counters, reconcile_one clearing every one on un-stick, the deferred import keeping the Temporal worker off the SDK, COUNTER_CONTRACT plus the 46/47 alternation pinning the four-kind contract, and test_42_43_44_still_charge_an_attempt_end_to_end guarding that #360 does not weaken #12.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1107 passed / 8 skipped is the author's result, not one reproduced here.

[claude]

Claude finished @mashkovd's task in 8m 25s —— View job

---

Code review — 5f9bbfb

• Read repository conventions (CONTRIBUTING.md / LLMS.md — no CLAUDE.md in this repo)
• Read the pinned diff (197c13b...5f9bbfb)
• Verify round-4 findings (unconditional reason read, unwrapped git ls-files, temp-file leak)
• Analyze the refusal-marker contract in run_implementer.py
• Analyze classification and the process_one refusal path in run_shepherd.py
• Assess test coverage
• Post inline findings and submit the verdict

Verdict: changes requested — 0 P1, 1 P2, 2 P3. Review submitted against 5f9bbfb9b2ee53bbc0885e67a3bf04ba355fb6fe.

Round 4 is closed, and closed in the right shape

The P2 both reviewers found is fixed: the read is gated on proc.returncode in _refusal_codes(), so the empty file mkstemp leaves behind is inert. The test is the part that matters — test_a_non_refusal_run_prints_no_warning, parametrised over 0/42/43/44/46/1/137, pins the class rather than this instance, which is the honest read of how it got through: the gap was never the handler, it was that nothing asserted what a healthy run prints. test_the_empty_marker_file_is_only_read_on_a_refusal states the gate directly. The try now starts above mkstemp with refusal_path None-guarded in the cleanup loop, and test_bundle_is_cleaned_up_when_mkstemp_itself_fails forces the exact EMFILE failure rather than asserting the shape of the code.

P2 — the new git ls-files handler is the one part of _read_refusal_marker with no test

orchestrator/run_implementer.py:300. The fix is right; the coverage is not. 5f9bbfb's test changes are entirely in tests/test_run_shepherd.py, and the two tests that touch git ls-files stub return codes through a fake _run that cannot raise, so both pass with or without the try. The sibling handler this mirrors is pinned as a class (test_any_read_failure_means_not_a_refusal over RecursionError/UnicodeDecodeError/MemoryError/RuntimeError) precisely because the narrow-tuple pattern is what the docstring argues against — and this call sat unwrapped through four commits on that same argument. If it ever re-narrows: FileNotFoundError is an OSError, so review_feedback_one's except subprocess.CalledProcessError misses it, _review_feedback_exit_code matches no prefix, exit 1 is classified transient, and the counter-less arm retries a paid run every tick — the route this PR's own audit table asserts is unreachable, currently asserted only in prose. Worth including the timeout case too: swallowing ImplementerOperationTimeout here is a deliberate behaviour change (a slow git ls-files now reports 42, not 44) recorded only in the commit message.

P3

• orchestrator/run_implementer.py:2101 — the review-feedback summary prints fail <service>/<slug>: deliberate no-op: … for exit 47. Every other statement on this path says the opposite, and this is the line that lands in the cron/Argo log on inherited stdio next to the shepherd's info: — and the only record of the reason when --refusal-out is omitted, which the flag's own help text supports. code is already computed two lines down.
• orchestrator/run_shepherd.py:625 — the non-dict and blank/non-string-reason rejections are the only silent ones in _read_refusal_reason, and they are the case where the child did write a file and it was unusable (_write_refusal_out cannot produce it, so it implies a real bug elsewhere). The operator gets the arm's fallback no reason recorded by the implementer, which reads as "the child didn't bother", and no greppable warn: — the filter round 5 was protecting.

What holds up

I re-traced the safety property on this commit: every interleaving on one head SHA converges on exactly one cap — 47×3 → MAX_REFUSALS; 46, 47, … → MAX_REFUSALS with no "harness defect" claim (47 clears the harness counter, a harness failure clears neither, and that asymmetry is load-bearing); 47, 42, … and 47, success, … → MAX_REVIEW_ATTEMPTS; 1, 47, … → MAX_REFUSALS, since the transient arm writes nothing. 45 stays unreachable from this path.

Checked beyond the diff: update_status_file has no field allowlist and treats None as delete-the-key, so both new fields persist and every clear is real on disk and mirrored in memory; review_feedback_one has exactly one caller; _run goes through run_capturing, so git ls-files's stderr on the untracked (happy) path is captured rather than leaked as an error: line into the same log the P2 of round 4 was about; skip_subprocess returns before any temp file exists; and all nine implementer.md definitions carry both the marker contract and the blocked-case exclusion.

Everything from the earlier rounds survives: the .git/info/exclude entry as the defence with the untracked check as the backstop, the bounded binary read that cannot be raced by an appender, git ls-files distinguishing 1 from any other non-zero, the exit code rather than the advisory file carrying the decision, _stuck_note/_declined_note budgeting the evidence rather than the sum, the duplicated byte cap pinned by an equality test, refusals anchored to refusals_head, _discover_refs round-tripping all four counters, reconcile_one clearing every one on un-stick, the deferred import keeping the Temporal worker off the SDK, COUNTER_CONTRACT plus the 46/47 alternation pinning the four-kind contract, and test_42_43_44_still_charge_an_attempt_end_to_end guarding that #360 does not weaken #12.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1151 passed / 8 skipped is the author's result, not one I reproduced.
· branch fix/agents-360-refusal-not-an-attempt

[claude]

Claude finished @mashkovd's task in 8m 46s —— View job

---

Code review — 05f2747

• Read repository conventions (CONTRIBUTING.md / LLMS.md)
• Read the pinned diff (197c13b...05f2747)
• Verify round-4 findings (unconditional reason read, unwrapped git ls-files, temp-file leak)
• Analyze the refusal-marker contract in run_implementer.py
• Analyze classification and the process_one refusal path in run_shepherd.py
• Assess test coverage
• Post inline findings and submit the verdict

Verdict: approved — 0 P1, 0 P2, 3 P3. Review submitted against 05f2747088da30e7873ce14683bf90edd0623206.

Round 5 is closed, and closed in the right shape

The reason read is gated on proc.returncode in _refusal_codes(), so the empty mkstemp file no longer warns on every healthy tick. test_a_non_refusal_run_prints_no_warning, parametrised over 0/42/43/44/46/1/137, pins the class rather than the instance — the right response to a gap that was about coverage, not the handler. The try now opens above mkstemp with refusal_path None-guarded in the cleanup loop, so the finally owns both paths for their whole lifetime (test_bundle_is_cleaned_up_when_mkstemp_itself_fails). And the git ls-files call sits inside a broad handler like the rest of _read_refusal_marker, deliberately absorbing ImplementerOperationTimeout too — the honest statement is the narrower one (could not establish whether the marker is tracked → 42, not "the implementer timed out" → 44) — with the comment no longer claiming a missing git binary arrives as a return code, and both routes tested.

I re-traced the path end to end on this commit. No route reaches the counter-less transient arm:

failure	lands on	bounded by
marker malformed / unreadable / oversized / wrong shape	None → "no follow-up commits" → 42	MAX_REVIEW_ATTEMPTS
git ls-files non-0/1, missing binary, or timed out	marker discarded → 42	MAX_REVIEW_ATTEMPTS
--refusal-out unwritable, or any exception in that write	warned; still exits 47	MAX_REFUSALS
reason file missing / empty / unparseable / oversized	kind stays refused — the exit code decides	MAX_REFUSALS

Every interleaving on one head converges on exactly one cap: 47×3 → MAX_REFUSALS; 46, 47, … → MAX_REFUSALS with no "harness defect" claim; 47, 42, … and 47, success, … → MAX_REVIEW_ATTEMPTS; 46×3 → MAX_HARNESS_FAILURES. COUNTER_CONTRACT plus the alternation regression pin that, and seeding the counters on disk as well as in memory matters — "untouched" is only observable in the file.

Checked beyond the diff: nothing outside run_shepherd.py and its tests enumerates the counter fields, update_status_file treats None as delete-the-key so every clear is real on disk and mirrored in memory, and the agent-authored reason is whitespace-normalised and capped at the trust boundary before it reaches a YAML note — and never reaches a command line (the only gh pr comment body is a fixed literal).

P3 (non-blocking)

• run_implementer.py:2098 — the review-feedback summary prints a refusal as fail mctl-web/<slug>: deliberate no-op: …. CONTRIBUTING.md reserves ok/fail/skip as the greppable classification for unattended logs, and a refusal is the one outcome in this PR's own table that is explicitly not a failure. Same argument round 5 accepted for the warn: on healthy ticks, one level up.
• run_implementer.py:363 — the "reason" key is the whole wire format to run_shepherd._read_refusal_reason (L627), and each side is asserted only against its own hand-written literal; neither consumes the other's output. Rename it on either side and both suites stay green while production silently falls back to "no reason recorded by the implementer" — the evidence a human needs to settle the standoff. Same drift class test_the_duplicated_byte_cap_matches_the_implementers already pins, on a constant with a smaller consequence.
• README.md:279 — "after five consecutive address-review ticks" is no longer how a proposal reaches review-stuck: there are three counters and three doors, and a tick can charge none of them. Parity note, not a new omission — Implementer follow-up ends its session while its own async sub-agent is still running; the lost work is charged as a review attempt #366/fix(implementer): await the sub-agent the CLI launched in the background #367 did not document harness_failures there either.

What holds up

Nothing from the earlier rounds regressed. The .git/info/exclude entry as the defence with the untracked check as the backstop (asserted against real git via git add -A); the bounded binary read that cannot be raced by a background appender, refusing over-cap rather than truncating; git ls-files distinguishing 1 from any other non-zero; _stuck_note/_declined_note giving the reason the whole MAX_NOTES_CHARS with the framing outside it, pinned by assertions that fail against a plain slice; 47 classified in its own set rather than widening #367's tuple; the deferred run_implementer import keeping the Temporal worker off the SDK; refusals anchored to refusals_head and reset when the branch moves; _discover_refs round-tripping all four fields (the read that makes any cap bind); reconcile_one clearing every counter on un-stick; all nine implementer.md definitions carrying the contract and excluding the blocked case that must still charge; and test_42_43_44_still_charge_an_attempt_end_to_end guarding that #360 does not weaken #12.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1107 passed / 8 skipped is the author's result, not one I reproduced. I did check line length (≤120) and the new noqa: BLE001 markers by hand.
· branch fix/agents-360-refusal-not-an-attempt

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit 5f9bbfb.

No significant issues found.

[github-actions]

Antigravity (agy) review

Model: gemini-3.1-pro-high · commit 05f2747.

No significant issues found.

[claude]

P2 — the handler added for round 4's escape route is the one part of this function with no test, and it guards the counter-less arm.

This is the right fix, and the reasoning in the comment is right too. But 5f9bbfb's test changes are entirely in tests/test_run_shepherd.py — nothing exercises this except. The two tests that do touch git ls-files stub return codes through a fake _run that cannot raise:

def fake_run(cmd, *args, **kwargs):
    if cmd[:2] == ["git", "ls-files"]:
        return subprocess.CompletedProcess(cmd, 128, "", "fatal: not a git repository")

so test_unknown_ls_files_result_is_not_treated_as_untracked and test_exit_one_is_still_honoured_as_untracked both pass with or without the try.

Every sibling check in this function is pinned, and the one this mirrors is pinned as a class for exactly this reason — test_any_read_failure_means_not_a_refusal is parametrised over RecursionError/UnicodeDecodeError/MemoryError/RuntimeError with the docstring "no exception type escapes this function", because the narrow-tuple pattern is the bug the docstring argues against. The _run call sat unwrapped through four commits on precisely that argument, and what would catch a future re-narrowing is a test, not the comment above it.

What a regression costs is the reason it is worth one: FileNotFoundError (git absent) is an OSError, so review_feedback_one's except subprocess.CalledProcessError misses it, the trailing except Exception turns it into error="FileNotFoundError: ...", _review_feedback_exit_code matches no prefix, exit 1 is classified transient — the one arm with no counter — and a paid implementer runs every tick. That is the route this PR's own audit table asserts is unreachable from the refusal path, and right now the assertion lives only in prose.

Mirroring the existing test is a few lines:

@pytest.mark.parametrize(
    "exc",
    [FileNotFoundError(2, "No such file or directory: 'git'"),
     run_implementer.ImplementerOperationTimeout("command exceeded 600s: git ls-files"),
     RuntimeError("something nobody enumerated")],
    ids=["git-missing", "timeout", "unknown"],
)
def test_a_failing_tracked_check_is_not_a_refusal(repo, monkeypatch, exc) -> None:
    """Not establishing the fact must not escape into the counter-less arm."""
    _write_marker(repo, {"refused": True, "reason": "r"})

    def boom(*_a, **_kw):
        raise exc

    monkeypatch.setattr(run_implementer, "_run", boom)
    assert run_implementer._read_refusal_marker(repo) is None

The timeout case is worth including on its own: swallowing ImplementerOperationTimeout here is a deliberate behaviour change (a slow git ls-files now reports 42 instead of 44), and the commit message is currently the only record of that decision.

Fix this →

[claude]

P3 — the implementer's own summary line calls a deliberate no-op a fail.

This is the review-feedback summary, so a correct refusal prints:

=== Review-feedback summary ===
  fail mctl-web/my-slug: deliberate no-op: Findings 2 and 3 are out of scope by explicit operator decision recorded on the PR

The whole PR argues this outcome is not a failure — the exit code is kept out of deterministic_codes, the shepherd logs info: ... not charging a review attempt, and the terminal note says "the proposal is not at fault". The one place that states the outcome in the implementer's own log says the opposite. It matters more than cosmetics because of where it is read: the shepherd forks this child with inherited stdio, so this line is what lands in the cron/Argo log next to the shepherd's info:, and it is also the only record of the reason when --refusal-out is not passed (which the --refusal-out help text explicitly supports).

code == EXIT_DELIBERATE_NO_OP is already computed a few lines below; branching the label on it (no-op / declined, or reusing the skip shape) keeps the summary honest. The result.error field carrying a non-failure is the underlying awkwardness, but the label is the part an operator reads.

Fix this →

[claude]

P3 — these two rejections are the only silent ones in the reader, and they are the cases where the child did write a file.

Round 5 established that what the shepherd prints on each path is part of the contract. This reader now warns on over-cap and on any exception, but returns None with no output when the payload parses and is either not an object or carries a blank/non-string reason. That is the one shape where the child exited 47 and produced a file and the file is unusable — the case most likely to mean a real bug in _write_refusal_out or in something else writing this path, since _write_refusal_out cannot produce it.

What the operator sees instead is the refusal arm's fallback, no reason recorded by the implementer, which reads as "the child didn't bother" rather than "we read its file and rejected it" — and it is not greppable as warn:, so it does not show up in the filter round 5 was protecting. A one-line warning before each return None makes the reader's own log match the standard the rest of the path now holds to. (run_implementer._read_refusal_marker warns on both of the equivalent checks.)

Fix this →

[claude]

Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3.

P2 is a coverage gap on correct code, not a defect: the broad except Exception added around the git ls-files call in _read_refusal_marker (5f9bbfb's implementer-side fix) has no test. The commit's test changes are all in tests/test_run_shepherd.py, and the two tests that touch git ls-files stub return codes through a fake _run that cannot raise, so both pass with or without the try. The sibling handler it mirrors is pinned as a class (test_any_read_failure_means_not_a_refusal) precisely because the narrow-tuple pattern is what the docstring argues against — and this call sat unwrapped through four commits on that same argument. If it re-narrows, FileNotFoundError is an OSError, so review_feedback_one's except subprocess.CalledProcessError misses it, exit 1 is classified transient, and the counter-less arm retries a paid run every tick. Swallowing ImplementerOperationTimeout here is also a deliberate behaviour change (a slow git ls-files now reports 42, not 44) recorded only in the commit message. A parametrised test mirroring the existing one closes both.

P3: the review-feedback summary prints fail <service>/<slug>: deliberate no-op: ... for exit 47, in the one log line an operator reads, contradicting every other statement on this path; and _read_refusal_reason's non-dict / blank-reason rejections are the only silent ones in the function, which is the case where the child did write a file and it was unusable — the operator gets no reason recorded by the implementer and no greppable warn:.

Round 4 is properly closed. The read is gated on proc.returncode in _refusal_codes(), and test_a_non_refusal_run_prints_no_warning parametrised over 0/42/43/44/46/1/137 pins the class rather than the instance — the gap was never the handler, it was that nothing asserted what a healthy run prints. test_the_empty_marker_file_is_only_read_on_a_refusal states the gate directly. The try now starts above mkstemp with refusal_path None-guarded in the cleanup loop, and test_bundle_is_cleaned_up_when_mkstemp_itself_fails forces the exact failure.

I re-traced the safety property on this commit and every interleaving on one head SHA still converges on exactly one cap: 47×3 → MAX_REFUSALS; 46, 47, … → MAX_REFUSALS with no "harness defect" claim (47 clears the harness counter, a harness failure clears neither); 47, 42, … and 47, success, … → MAX_REVIEW_ATTEMPTS (both clear refusals/refusals_head and charge); 1, 47, … → MAX_REFUSALS, since the transient arm writes nothing. 45 remains unreachable from this path. Verified independently of the diff: update_status_file has no field allowlist, so the two new keys persist; None means delete-the-key, so every clear is real on disk and mirrored in memory; review_feedback_one is called from exactly one place; _run uses run_capturing, so git ls-files's stderr on the untracked (happy) path is captured rather than leaked as an error: line; and all nine implementer.md definitions carry both the contract and the blocked-case exclusion.

Everything from the earlier rounds holds: the .git/info/exclude entry as the defence with the untracked check as the backstop, the bounded binary read that cannot be raced by an appender, git ls-files distinguishing 1 from any other non-zero, the exit code rather than the advisory file carrying the decision, _stuck_note/_declined_note budgeting the evidence rather than the sum, the duplicated byte cap pinned by an equality test, refusals anchored to refusals_head, _discover_refs round-tripping all four counters, reconcile_one clearing every one on un-stick, the deferred import keeping the Temporal worker off the SDK, and test_42_43_44_still_charge_an_attempt_end_to_end guarding that #360 does not weaken #12.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1151 passed / 8 skipped is the author's result, not one I reproduced.

[claude]

P3 — the implementer still prints a deliberate no-op as fail, which is the log-level version of the bug this PR fixes.

The summary line two lines above (L2098) runs for every non-None result.error, so a refusal now prints:

  fail mctl-web/test-slug: deliberate no-op: Findings 2 and 3 are out of scope by explicit operator decision...

CONTRIBUTING.md makes ok / fail / skip the greppable status prefixes for unattended cron/Argo logs, for the same reason round 5 treated a warn: on 100% of healthy ticks as a P2: the prefix is the operator-facing classification, and a correct outcome labelled with the failure prefix devalues it. A refusal is the one outcome in this PR's table that is explicitly not a failure — the shepherd charges nothing and the design note says the proposal is not at fault — yet the implementer's own log says it failed. An operator grepping fail across implementer runs (or reading the summary to decide whether a tick went wrong) gets the pre-#360 picture back.

One branch before the print, e.g.:

        if result.error:
            code = _review_feedback_exit_code(result.error)
            prefix = "info" if code == EXIT_DELIBERATE_NO_OP else "fail"
            print(f"  {prefix} {result.ref.service}/{result.ref.slug}: {result.error}")

(keeping the exit code exactly as it is — the classification that matters is unchanged). Non-blocking; the shepherd-side framing is already right.

Fix this →

[claude]

P3 — the writer and the reader of this file agree only by coincidence; nothing fails if the key drifts.

The "reason" key is the entire wire format between the two processes, and each side is pinned against its own hand-written literal:

• test_main_writes_the_refusal_reason_and_exits_47 asserts this writer produces {"refused": True, "reason": reason};
• every shepherd-side test (test_apply_followup_raises_refused_and_carries_the_reason, test_the_empty_marker_file_is_only_read_on_a_refusal, …) hand-writes {"refused": true, "reason": ...} in its own fake_run and then asserts _read_refusal_reason returns it.

Neither side ever consumes the other's output. Rename the key here (or on the reader at run_shepherd.py:627, which also never checks refused — only reason) and both suites stay green while production silently degrades: _read_refusal_reason returns None, the note becomes implementer declined to act: no reason recorded by the implementer, and the one thing a human needs to settle the standoff is gone. Bounded — the exit code still carries the decision, so no attempt is charged — but it is invisible, and the evidence is the whole reason this file exists.

This is the same cross-module drift class the PR already decided to pin rather than document: test_the_duplicated_byte_cap_matches_the_implementers exists for exactly this, on a constant with a smaller consequence. One test feeding the writer's output straight to the reader closes it:

def test_the_reason_survives_the_real_round_trip(tmp_path) -> None:
    """The two processes agree on the wire format, not just on their own literals."""
    path = tmp_path / "refusal.json"
    run_implementer._write_refusal_out(path, "operator deferred this to a separate issue")
    assert run_shepherd._read_refusal_reason(str(path)) == (
        "operator deferred this to a separate issue"
    )

Fix this →

[claude]

P3 — README.md:279 now describes the cap in a way that is wrong for three of the five outcomes.

Review-attempt cap. The outer loop tracks review_attempts: in .status.yaml. After five consecutive address-review ticks without resolving the findings, the next tick flips the proposal to status: review-stuck (terminal) instead of forking the implementer again.

After this PR there are three counters and three review-stuck doors, and an address-review tick can end without charging anything (transient), charging harness_failures (46), or charging refusals (47). So "five consecutive address-review ticks" is no longer the rule for how a proposal reaches review-stuck — and a reader trying to answer the exact question #360 is about ("did that tick cost the PR an attempt?") gets the pre-#366 answer from the only operator-facing doc that covers it.

Parity note, not a new omission: #366/#367 did not document harness_failures there either — grep -rn harness_failures outside run_shepherd.py and its tests finds nothing. That is what makes it worth a line here rather than there: this PR is the second counter added past that sentence, and it is the one that makes the sentence actively misleading about cost. Three sentences naming the three counters (5 / 3 / 3, and which outcomes clear which) would settle it. tools/diagram_facts.py:62 only extracts MAX_REVIEW_ATTEMPTS, so the generated diagram has the same gap.

Non-blocking, and squarely the author's call on whether README carries this level of detail at all — the in-code rationale is already thorough.

Fix this →

[claude]

No P1/P2 findings (3 P3). Good to merge.

Round 5 is closed in the right shape. The refusal-reason read is gated on proc.returncode in _refusal_codes(), so the empty mkstemp file no longer produces a warn: on every healthy tick — and test_a_non_refusal_run_prints_no_warning, parametrised over 0/42/43/44/46/1/137, pins the class rather than the instance, which is the right response to a gap that was about coverage rather than the handler. The try now opens above mkstemp with refusal_path None-guarded in the cleanup loop, so the finally owns both paths for their whole lifetime (test_bundle_is_cleaned_up_when_mkstemp_itself_fails). The git ls-files call is inside a broad handler like the rest of _read_refusal_marker, deliberately absorbing ImplementerOperationTimeout too — the honest statement is the narrower one (we could not establish whether the marker is tracked → 42, not "the implementer timed out" → 44) — and the comment no longer claims a missing git binary arrives as a return code, with both routes tested.

I re-traced the path end to end on this commit and found no way for it to reach the counter-less transient arm: every malformed, unreadable, oversized, or unverifiable marker returns None → 42 (charged); an unwritable --refusal-out still exits 47; a missing or unparseable reason file leaves the kind refused, since the exit code decides; git ls-files runs check=False so it cannot raise the one exception that reaches transient. Every interleaving on one head converges on exactly one cap — 47×3 → MAX_REFUSALS; 46, 47, … → MAX_REFUSALS with no "harness defect" claim; 47, 42, … and 47, success, … → MAX_REVIEW_ATTEMPTS; 46×3 → MAX_HARNESS_FAILURES — pinned by COUNTER_CONTRACT and the alternation regression, with the counters seeded on disk as well as in memory because "untouched" is only observable in the file. Also checked beyond the diff: nothing outside run_shepherd.py and its tests enumerates the counter fields, update_status_file treats None as delete-the-key so every clear is real on disk and mirrored in memory, and the agent-authored reason is whitespace-normalised and capped at the trust boundary before it reaches a YAML note (and never reaches a command line — the only gh pr comment body is a fixed literal).

P3s, all non-blocking and inline: the implementer still prints a refusal under the fail prefix CONTRIBUTING reserves for failures, which is the log-level version of the bug this PR fixes; the "reason" wire format between _write_refusal_out and _read_refusal_reason is pinned only by each side asserting its own literal, so a key rename keeps both suites green while production silently loses the evidence; and README.md:279 still describes the cap as "five consecutive address-review ticks" now that three counters and three review-stuck doors exist.

Not verified: pytest, ruff, and mypy needed command approval in this environment, so the reported 1107 passed / 8 skipped is the author's result, not one I reproduced. I did check line length (≤120) and the new noqa: BLE001 markers by hand.