feat(chat): explain a turn-limit stop instead of reporting it as a failure by zaytcevcom · Pull Request #63 · duckbugio/flock

zaytcevcom · 2026-07-27T12:17:13Z

Summary

A run that exhausted the agent turn cap was reported to the user as a generic failure. The
delivered message was whatever the provider put in the result envelope — for this subtype a fixed
Reached the maximum number of turns. — under a ⚠️ Failed after … notice. That wording is wrong
in a way that costs the user real time: a turn-limit stop is a configured cutoff, not a crash,
and nothing in the message said which knob controls it or what it was currently set to.

This PR makes that stop explain itself.

Before

⚠️ Reached the maximum number of turns.

⚠️ Failed after 1m 35s

After

⚠️ The run stopped because it reached its turn limit (250 of 250 turns) — not a crash, timeout or
API error. Everything already written, committed or pushed is kept in the workspace, but the
stopped run's chat context may not carry over — restate anything you want continued in your next
message. To allow longer runs, raise CLAUDE_MAX_TURNS (current: 250) and restart the bot.
(error_max_turns)

⏹ Turn limit reached after 1m 35s

Every clause appears only when the fact behind it is actually known. With no cap configured the
message names no value and does not claim to know the provider's fallback:

…No turn cap is configured for this bot, so the run was bound by whatever default the provider
applies. Set CLAUDE_MAX_TURNS and restart the bot to control it. (error_max_turns)

Related issue

None.

Type of change

What changed, per acceptance criterion

AC1 — the message states the cause and keeps the raw token. It names the turn limit,
explicitly rules out a crash/timeout/API error, and retains error_max_turns. The token is the
provider's own stable, unlocalized name for the stop; nothing on this path logs it, so the
delivered message is the only place it ever surfaces.
AC2 — it names the knob and the cap actually in force. RunLimits{MaxTurns, MaxTurnsEnv}
is threaded into the renderer. finish() passes s.opts.MaxTurns, which is the cap that bound
the run: s.opts is written only in New and never mutated, run() copies it and overwrites
only Images/Workdir/SessionID, and the one other override (runEvaluator's EvalMaxTurns)
produces a result consumed inside the post-run loop that never reaches finish — which has a
single call site.
AC3 — the completion notice no longer says "failed". It reads ⏹ Turn limit reached, and
only when the stop is genuinely clean (runErr == nil); with a run error set, finish renders
FinalError instead, so the notice keeps the failure wording that matches the delivered answer.
AC4 — core/chat stays free of provider env names. The adapter supplies the name via the
new chat.Config.MaxTurnsEnv, derived from airunner.TurnLimitEnv(provider). There is no
CLAUDE_* string anywhere under core/. config.ClaudeMaxTurnsEnv is exported and pinned equal
to the struct tag by a reflection test.
AC5 — the knob name cannot break out of its code span. It is operator-supplied, so it is
sanitized (backticks and all Unicode whitespace stripped) before interpolation; a name that
sanitizes to empty drops the clause rather than emitting empty backticks.
AC6 — the turn count rides along only when it corroborates the cap. num_turns semantics
are not verified, so a count that contradicts the sentence is dropped rather than shown.

The zero value of chat.Config.MaxTurnsEnv reproduces the previous behaviour byte-for-byte, so
embedders that do not set it are unaffected.

How it was verified

task lint    # 0 issues
task tests   # go test -race ./... — all 30 packages ok
task build   # ok

Run in the foreground on the final tree. Coverage added for the message itself (11 table cases
spanning every combination of known/unknown cap and knob, plus a hostile knob name), the delivered
end-to-end message carrying the runtime cap, the notice wording, and the provider→knob mapping.
TestFinalKeepsWordingOutsideTurnLimit pins every other error subtype's rendering byte-for-byte
so this change cannot have moved them.

One unrelated flake was observed and characterised, not ignored: core/ghstar TestIsStarred/starred
failed once under full-suite parallel load with http: CloseIdleConnections called. It is a
t.Parallel() test against a local httptest server, core/ghstar imports nothing from this diff,
and it passed 5/5 in isolation and on the following full-suite run.

Pre-PR review

Two independent critics reviewed the local diff (correctness/security, and the reuse /
simplification / efficiency / altitude design lenses), then a third re-derived a pass from scratch
over the fixes. No round found a blocker or a major. Both later rounds were spent on findings
the critics converged on, all of which are fixed here:

The provider's boilerplate result text was being echoed above the explanation, so the
message opened with a duplicate of its own next line. Dropped — gated on the subtype rather than
on matching the sentence, so a reworded boilerplate cannot start leaking back in.
The no-cap clause contradicted the sentence it followed ("reached its turn limit" / "no turn cap
is configured"). It now names the connective without claiming to know the provider's fallback.
A new test reused assertMarkdownSafe, a helper documented for stage lines where all markup is
stripped. The turn-limit message deliberately keeps _ in the knob name, so the helper passed
only because both fixtures happened to have an even underscore count — a realistic
MAX_TURNS would have failed a perfectly safe message, pressuring the next reader into stripping
_. Replaced with a code-span-aware assertion, and covered by a case with an odd underscore
count that fails under the old helper.
The turn-cap probe's filled == 0 guard could never fire (GoalEvalMaxTurns also matches the
suffix while feeding no provider), so a rename left it silent and blamed the provider instead.
Now asserts the field that actually feeds a provider; mutation-tested to fail with the right
diagnostic.
A comment justified keeping the raw subtype as "what an operator greps for in the logs". It is
logged in exactly one place repo-wide — the goal-evaluator path, which never renders this
message. Rationale corrected.

Residual risks — what still needs a human eye

No live envelope was captured. Every claim about the provider's payload rests on
core/claude/testdata/error.jsonl, not on a real CLI run. Two things ride on it: that the
result text for this subtype is always that fixed boilerplate, and that num_turns equals the
cap. If a CLI version ever puts a partial answer in result, this PR drops it from chat
entirely — assistant text otherwise only feeds the transient progress ring. Worth one real
error_max_turns run to confirm before trusting the drop.
A turn-limit stop still clears the chat session (core/chat/service.go:631 treats any
is_error result as a poisoned session). That is precisely what forces the message to hedge
"chat context may not carry over" — the one stop where the user most wants to continue. Both
critics called this the highest-value follow-up. It changes resume semantics, so it deserves its
own acceptance criteria and its own PR rather than being slipped in here.
Final is an exported symbol of a public module path; its signature change is breaking for
any external importer.
The provider→knob drift guard is convention-bound. It catches a future provider that gains a
cap via a <Provider>MaxTurns config field; a cap wired from a differently named field reads
back as zero and passes silently. The limitation is stated in the helper's comment.
The ⚠️ prefix on the answer now sits slightly at odds with the ⏹ notice and with the
sentence's own "not a crash" — deliberate for now, since ⚠️ is what flags a non-clean result
generally, but it is a reasonable thing to challenge in review.

Checklist

task lint, task tests, and task build pass locally
Tests added/updated for the change
Docs updated if behavior or configuration changed — CLAUDE_MAX_TURNS already documented in
both .env.example files; no new variable is introduced
The change is focused — no unrelated churn
Commits follow Conventional Commits

Final() now takes a RunLimits value alongside the run result so the terminal message can explain a stop that was caused by a configured limit rather than by a failure. Nothing reads it yet: the zero value renders exactly today's output, so behaviour is byte-identical. The env-var NAME behind the turn cap arrives through the new chat.Config.MaxTurnsEnv, keeping core/chat free of any provider env name; the NUMBER comes from the Options the Service already holds. finish() reads the cap off the Service because s.opts is the single source of truth for anything that reaches it: run() copies s.opts and overwrites only Images/Workdir/SessionID, and the evaluator's EvalMaxTurns override applies to runEvaluator, whose result is consumed inside the post-run loop and never passes through finish.

…alue A run that exhausts the agent turn cap used to end with the opaque "the run ended with an error (error_max_turns)", which reads like a crash and gives an operator nothing to act on. Final now switches on the result subtype and gives error_max_turns dedicated wording: it names the cause, states explicitly that it is not a crash, timeout or API error, says the work already done survives in the workspace, and points at the environment variable that raises the cap together with the value actually in force. The raw subtype token is kept for log grepping. error_max_turns is the only subtype with wording because it is the only one whose cause we can state truthfully; every other subtype keeps the generic message, so a single case plus the existing default is all that is needed. Each clause is dropped when the limit behind it is unknown, so the message is never wrong: no cap of 0 when none was configured, no variable named when the provider has no such knob, and no "N of M turns" unless the provider's own turn counter is consistent with the cap (it may exceed it). Text that came with the result is now kept and the explanation appended after it instead of being silently replaced.

Export ClaudeMaxTurnsEnv so the knob behind the Claude turn cap can be named outside internal/config, and add airunner.TurnLimitEnv to map a configured AI_BACKEND (name or alias) to it. A test pins the constant to the struct tag that parses it, so renaming one without the other fails that test rather than producing a message that points at a variable nobody reads. Only the Claude backend passes a turn cap to its CLI — the Codex and OpenAI-compatible providers leave agent.Options.MaxTurns zero — so every other provider resolves to the empty string and names no knob at all. It is a plain function rather than a Provider method because two of the three implementations would do nothing but return "".

Both binaries now derive chat.Config.MaxTurnsEnv from the provider they built, so a run that stops on the turn cap tells the operator which variable raises it. A provider that applies no turn cap yields an empty name and the message stays silent about any knob.

…me cap An end-to-end guard over the whole path — chat.Config, the run loop, the terminal renderer — that a run stopped by the turn cap reports the cap and knob the Service was actually built with, and never a compiled-in default. Verified to fail if the limits stop being threaded through finish.

The count now rides along only when it EQUALS the cap. num_turns semantics are unverified — the provider's counter can both undershoot and overshoot it — and the sentence already states the cap was reached, so "3 of 40 turns" reads as a bug exactly like "501 of 250 turns" did. A count that does not corroborate the cap is silently omitted; the sentence stands on its own without it. A positive cap is still required so an unknown cap can never be corroborated by a zero count. Two claims are corrected as well. The message no longer implies the next message picks up where the run left off: a terminal is_error result while resuming clears the chat's stored session, so the next message can start a fresh one with no memory of what was in flight. It now says the run's chat context may not carry over and asks the user to restate what should continue. The no-cap branch no longer blames a provider built-in limit (unverified) nor offers to "raise" a value the operator explicitly zeroed — it just names the knob to set. Finally, the knob name is sanitized before it lands inside the inline-code span. chat.Config.MaxTurnsEnv is exported, so the name is whatever an embedder set, and a backtick in it would leave an odd count and take the whole message off the rich-markdown path. Only backticks are stripped (and whitespace collapsed): the stage-label set does not fit here because it drops "_", which is legal and common in an env var name.

The completion notice read "Failed after <elapsed>" for any is_error result, so it contradicted the answer bubble right above it, which states the stop was not a crash, timeout or API error. A turn-limit result now gets its own notice — "Turn limit reached after <elapsed>" — reusing the same elapsed formatting as every other branch. Any other error subtype, and a result with no subtype at all, keeps the existing wording byte-identically; so does a run error alongside the result, because finish then renders the failure text rather than the turn-limit explanation. The end-to-end test's num_turns is also separated from the configured cap, so its assertion on the cap value cannot be satisfied by the result's own counter.

The no-cap clause read as a contradiction of the sentence it follows: the message opens with "the run stopped because it reached its turn limit" and then stated "no turn cap is configured for this bot", leaving the reader to reconcile the two. Name the connective instead — the limit that bound the run was not one this bot set. Nothing new is claimed about WHAT the provider's fallback cap is, since that behaviour is still unverified, and "raise it" stays wrong because the only way to reach this branch is an explicitly zeroed cap.

An error_max_turns envelope arrives with a fixed sentence in its result — "Reached the maximum number of turns." (core/claude/testdata/error.jsonl) — which states exactly what the explanation's own opening sentence states, minus the actionable knob and the raw subtype token. Keeping it opened the delivered message with a duplicate of its own next line. Drop the result text for this subtype. The rule is gated on the subtype rather than on matching the sentence, so a reworded boilerplate cannot start leaking back in; every other subtype still keeps the text it came with. Cover both halves: the realistic boilerplate payload, and an arbitrary text that must be dropped just the same. The shared assertion now pins the cause to exactly one occurrence in every case.

The comment claimed the raw error_max_turns token is kept because an operator greps for it in the logs. Nothing on this path logs the subtype: the single site that does is the goal evaluator in postrun.go, which never renders this message. A reader following that rationale would search logs that stay silent. Keep the token and state the real reason. It is the provider's stable, unlocalized name for this stop, so it is what an operator matches against provider docs and issue reports, and since this path logs nothing the delivered message is the only place it ever appears.

…line assertMarkdownSafe is written for stage lines, whose labels are stripped of every markdown metacharacter, so it can demand an even count of each inline delimiter. The turn-limit message is not a stage line: it interpolates an operator-supplied env-var name into an inline-code span and deliberately keeps "_", because sanitizeEnvName stripping it would print a knob that does not exist. The assertion passed only because the fixtures happened to carry an even number of underscores; a realistic MAX_TURNS or GOAL_EVAL_MAX_TURNS would have failed a message that is perfectly safe, pressuring the next reader into stripping "_" from the name. Assert the invariant this message actually owes instead: its backticks are balanced so every code span closes, and no newline breaks one open, since a newline also ends a span. Underscores and asterisks inside a span are inert, so their balance is not asserted. The stage-line helper and its three callers are untouched. A new case renders a knob with an odd underscore count — the shape the old assertion rejected — and the case is pinned to stay odd so it cannot silently stop covering it.

TurnLimitEnv names the turn-cap variable only for Claude, on the grounds that no other provider passes a cap down to its CLI. That fact lives in the providers' Build methods, yet only a comment tied the two together: TestTurnLimitEnv restates TurnLimitEnv's own expectations, so it cannot see them drift apart. A provider that started setting agent.Options.MaxTurns would really enforce a cap while the terminal message stayed silent about the knob that raises it, and every existing test would still pass. Assert the biconditional over DefaultRegistry(): for every registered provider, Build returning a positive Options.MaxTurns must hold exactly when TurnLimitEnv names a knob. Providers that do not exist yet are covered without touching the test, and a provider that fails to build fails the test loudly instead of passing vacuously on a zero cap. The probe config fills every *MaxTurns field by reflection so a provider reading a cap field added later cannot read back a zero and slip through.

Three follow-ups to the turn-cap and code-span assertions, all test-only. The turn-cap probe's `filled == 0` guard could never fire: GoalEvalMaxTurns, the goal evaluator's own cap, also ends in the suffix while feeding no provider, so the count never reached zero. Renaming ClaudeMaxTurns away from the convention therefore left the guard silent and blamed the provider for a probe-config problem. Assert the field that actually feeds a provider today, so the failure names its own cause; skip unexported fields while here, since SetInt would panic on a match. State the probe's real reach in its doc comment: the biconditional covers a future provider only if its cap field follows the naming convention, because a cap wired from a differently named field reads back as zero and passes. assertCodeSpanSafe gave up more than the false positive required. Only a span's INTERIOR earns the exemption for "_" and "*"; outside one they are still live markup. Restore the balance check there, so a future clause that interpolates text outside a span stays held to it.

zaytcevcom

Review

Independent fresh pass over the whole diff (whole files, not just hunks), plus a local gate re-run in the repo's own dev-tools image: go build ./..., go vet ./..., golangci-lint run --timeout=5m ./... -> 0 issues, and go test -race ./core/chat/... ./internal/airunner/... ./internal/config/... ./core/claude/... -> all green.

Summary: no new blocker or major. The turn-limit path holds up on every reachable combination I could construct; one NIT on regression coverage below.
Must fix before merge: —
Risks/questions: low — the change is additive on a single terminal branch, a zero RunLimits reproduces the previous wording byte-for-byte, and TestFinalKeepsWordingOutsideTurnLimit pins every untouched subtype.
Inline comments: 1

What I checked and found correct

Recorded so it does not have to be re-derived by the next reader.

finish and completionNotice agree in every reachable combination. resuming is genuinelyInterrupted, which implies res == nil, so it can never collide with the new case; ctxErr != nil && res == nil && runErr == nil gives "⏹ Stopped." + "Stopped"; runErr != nil gives FinalError + "Failed" (also when a turn-limit res is present); an is_error turn-limit result gives the explanation + "Turn limit reached" — including with ctxErr != nil (a Stop/timeout landing after the Result was captured), where the answer and the notice still agree.
VK. cmd/duck-vk resolves provider from the same airunner.Build, so TurnLimitEnv(provider.Name) is right there too. The only transport-visible difference is that VK has no parse mode (adapters/vk/bot.go ignores asMarkdown), so the knob renders with literal backticks — already the established convention for core-generated text (core/chat/postrun.go:162 emits a code span the same way), so nothing to change.
The instructions the message gives the user are true for this deployment. CLAUDE_MAX_TURNS is read once at startup (internal/config, env_file: .env in compose), so "restart the bot" is accurate; nothing wipes the per-chat workspace (core/workspace.Ensure only re-renders CLAUDE.md), and finish still sweeps the outbox on this terminal path, so "everything already written, committed or pushed is kept in the workspace" holds.
The new tests discriminate. Mutation-checked the two load-bearing ones on a scratch copy: dropping the res.NumTurns == limits.MaxTurns guard fails 3 cases of TestFinalTurnLimitStop, and hardcoding the cap in finish fails TestRunExplainsTurnLimitWithRuntimeCap on "7". Neither passes vacuously. TestTurnLimitEnvMatchesProviderTurnCap also fails loudly rather than vacuously when a provider will not build.
Contract. Final's signature change is the only externally visible break (internal/... is not importable by other modules); Config.MaxTurnsEnv is additive with a zero value that preserves old behaviour, and Final has exactly one in-repo call site.

The residual risks listed in the PR description (the dropped result text, the session still cleared at service.go:631, the convention-bound drift guard, unverified num_turns, ⚠️ vs ⏹) are stated with their rationale, and I did not find that reasoning to be wrong.

…it result The turn-limit case in completionNotice guards runErr == nil but deliberately not ctxErr: run reads ctx.Err() only after the event loop drained, so a Stop press or the per-run timeout landing once the Result was already captured reaches finish with both a turn-limit result and a non-nil ctxErr. finish's switch then falls through to its default branch and renders the turn-limit explanation, so the notice below that bubble must read "Turn limit reached" as well. Nothing pinned that asymmetry: the table only ever paired a ctxErr with a nil result, so adding ctxErr == nil to the guard "for symmetry" would have put "Failed" under a turn-limit answer again on a fully green suite. Cover the combination and record why it is not redundant.

zaytcevcom added 13 commits July 27, 2026 08:59

zaytcevcom commented Jul 27, 2026

View reviewed changes

Comment thread core/chat/service.go

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(chat): explain a turn-limit stop instead of reporting it as a failure - #63

feat(chat): explain a turn-limit stop instead of reporting it as a failure#63
zaytcevcom wants to merge 14 commits into
mainfrom
duck/-5362116338/explain-turn-limit-stop

zaytcevcom commented Jul 27, 2026

Uh oh!

zaytcevcom left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

zaytcevcom commented Jul 27, 2026

Summary

Related issue

Type of change

What changed, per acceptance criterion

How it was verified

Pre-PR review

Residual risks — what still needs a human eye

Checklist

Uh oh!

zaytcevcom left a comment

Choose a reason for hiding this comment

Review

What I checked and found correct

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant