Repository navigation
fix: let workflow subagents inherit pi's turn retry - #58
Merged
Merged
Conversation
Subagents called session.setAutoRetryEnabled(false) to make the run-level --agent-retries policy authoritative. In pi that call is not session-scoped: it writes retry.enabled=false into the user's global ~/.pi/agent/settings.json, silently disabling auto-retry for the host and every later session. Combined with --agent-retries defaulting to 0, any transient provider failure mid-agent killed the agent, and an opted-in retry restarted it from scratch, discarding completed turns. An intermittent OpenRouter -> Meta 404 (model_not_found on ~12% of muse-spark-1.3 requests) failed most long agents. Subagents now keep pi's in-session auto-retry under the user's retry settings, so a failed turn is retried in place; each attempt is logged in the agent's progress row. --agent-retries remains the outer whole-agent restart.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retry notices are written to the workflow-wide log rather than the promised agent progress row.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Stops workflow subagents from disabling pi’s global retry setting and exposes inherited turn retries through progress telemetry.
Changes:
- Removes subagent auto-retry mutation.
- Logs and counts in-session retries.
- Updates tests and retry documentation.
| File | Description |
|---|---|
USAGE.md |
Documents layered retry behavior. |
AGENTS.md |
Records the global-settings persistence hazard. |
src/agent-session.ts |
Preserves inherited retries and handles retry events. |
src/agent-runner-types.ts |
Removes retry mutation from the session interface. |
tests/agent-session-services.test.ts |
Verifies settings remain enabled and unwritten. |
tests/agent-runner-fixtures.ts |
Updates the session fixture. |
tests/agent-retry.test.ts |
Covers retry-event logging. |
tests/agent-limits.test.ts |
Updates the session stub. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+157
to
+159
| rc.progress.log( | ||
| `${label}: transient provider failure; retrying turn ${event.attempt}/${event.maxAttempts} in ${event.delayMs}ms`, | ||
| ); |
pi's DefaultResourceLoader (jiti) in package-recognition.test.ts evicts index.ts from Bun's shared module cache. When workflow-tool.test.ts runs after it (CI file order), its direct import gets a fresh index.ts instance while the cached fixture still drives the original one, so getLastWorkflowInspection() reads state the captured tool never wrote.
- Record usage of failed turns pi retried in place: pi drops the failed assistant message from session.messages before retrying, so capture it from message_end and include it in the usage record and budget. - Include the provider error (truncated) in the turn-retry log line. - Warn once per run when pi auto-retry is disabled; 0.12.0 and 0.13.0 persisted retry.enabled=false into the user's global settings. - Describe agentRetries as the outer whole-agent restart in the tool schema and completions, and document budget/usage-limit interplay. - Assert the services test against seeded user retry settings rather than pi's defaults.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Problem
Running workflows on
openrouter/meta/muse-spark-1.3(noworkflow-models.json, so every profile uses the host model) failed most subagents mid-run with:The failing agents had already made 11–20 successful tool calls.
Root cause
curlprobes with the same key showed OpenRouter's Meta endpoint returning404 model_not_foundon about 12% of requests (3/24), for anyreasoning.effort. A few minutes later it was 0/72. Over a 13-turn agent, the chance of zero failures is 0.875¹³ ≈ 18%.provider.?returned.?error), butopenAgentSessionturned pi's in-session auto-retry off, and--agent-retriesdefaults to0. When opted in, a run-level retry restarts the whole agent and discards its completed turns.AgentSession.setAutoRetryEnabled(false)is not session-scoped: it callsSettingsManager.setRetryEnabled()→save(), which writes"retry": {"enabled": false}into the user's global~/.pi/agent/settings.json. Every workflow run has been turning off auto-retry for the host pi session and every later session too.Ruled out: model-ref parsing / provider sync. With no profile config, children receive
ctx.modelas an object (provider: "openrouter",id: "meta/muse-spark-1.3"), soparseAgentModelRefis never called, and the auth sync keys offopenrouter. Theprovider_name: "Meta"/is_byok: falsefields are OpenRouter's own upstream metadata, so the request did reach OpenRouter with the user's key.Fix
setAutoRetryEnabled(false). Subagents inherit pi's in-session auto-retry from the user's piretrysettings (default: enabled, 3 retries, 2s/4s/8s), so a failed turn is retried in place and earlier tool work is kept.auto_retry_startevents to the workflow progress log (<label>: transient provider failure (<error, truncated>); retrying turn n/N in Xms) and count them asagent.turn_retry.session.messagesbefore retrying (0.80.10 slices it off; 1.0.4 omits it through a context edit), so it is captured frommessage_endand included in usage and budget totals.retry.enabled: false.agentRetriestool-schema and completion text now describe it as the outer whole-agent restart.--agent-retrieskeeps its meaning as the outer whole-agent restart, now only after pi's turn retries are exhausted.setAutoRetryEnabledfromAgentRunnerSession, so the call can't come back without a type change. Add the pi trap to the AGENTS.md critical facts.Verification
x-session-idwith the exact OpenRouter 404 body. A drop-in workflow runs 5 parallel agents, each making 8 sequential bash calls onmuse-spark-1.3.master: 0/5 ok. Every agent died on the injected 404.~/.pi/agent/settings.jsonwas byte-identical before and after.tests/agent-session-services.test.ts: on the realcreateAgentSessionServicespath, seeded user retry settings (enabled: true, maxRetries: 5) are inherited by the child and still unchanged on disk aftersettingsManager.flush(). Withretry.enabled: false, the warning is logged exactly once across two agents. Mutation-checked: reintroducingsetAutoRetryEnabled(false)or removing the warning fails it.tests/agent-retry.test.ts: a new case asserts that pi'sauto_retry_startreaches the progress log with its reason, and that the dropped failed attempt's output tokens are counted (mutation-checked). The obsoleteautoRetrySettings == [false, false]assertion is removed.7c9d008, pre-existing onmaster):package-recognition.test.tsloads the extension through pi'sDefaultResourceLoader(jiti), which evictsindex.tsfrom Bun's shared module cache. In CI's file order,workflow-tool.test.tsthen imported a freshindex.tswhile the cached fixture still drove the old instance, sogetLastWorkflowInspection()returnedundefined. Reproduced locally with three files and onmaster. The fixture now re-exports the getter from its own binding.bun run typecheckis clean;bun run testshows 523 pass, 0 fail, in both the default and CI file order.Notes for release
"retry": {"enabled": false}in~/.pi/agent/settings.json, written by this bug. While it's there, neither the host nor subagents auto-retry. Release notes should tell them to remove it. It can't be safely auto-repaired, because a deliberate opt-out looks identical.applyOverrides). This was a deliberate choice to have subagents behave like the host.