Repository navigation
feat: task retry re-resolves against current configuration (ADR-0014) - #9
Merged
Merged
Conversation
A live incident showed task retry is a trap for configuration failures: it copies the frozen model resolution verbatim, so after fixing grants the retried run re-bills the whole pipeline from checkout and fails at the same step. ADR-0014 decides that retry becomes a re-submission of the pinned task — template version and params stay pinned, everything configuration-derived (bindings, models, manifest, budget, quota, repo refs) re-resolves — because re-resolution is deterministic when config is unchanged and auto-classification of resolution-shaped failures is impossible (the incident finalized as catch-all model_error). Also decides persisting submit-time agent overrides, deleting the subsumed assertResolutionCredentialed gate, and the UI confirmation.
The agents overrides a user picks on the submit form were consumed at
resolution and stored nowhere, so nothing downstream could distinguish
a deliberate slot choice from a template default that happened to
resolve the same way. A re-resolving retry (ADR-0014) needs the raw
override object back, so it now lands on tasks.agent_overrides
(migration 0008, default {}). Tasks created before the column exists
retry with template defaults — recorded as an ADR consequence.
Retry copied the previous run's frozen resolution verbatim, which made it structurally unable to heal configuration failures: after fixing grants/registry/credentials the retried run re-billed the whole pipeline from checkout and died at the same step. Per ADR-0014 retry is now a re-submission of the pinned task — template version and params snapshot stay pinned, everything configuration-derived re-resolves through resolveRunPlan, a helper shared with submit so the two paths cannot drift. With unchanged configuration re-resolution is deterministic, so transient-failure retries behave exactly as before. Retry also gains submit parity it silently lacked: the quota hard-stop, repo-ref ownership check, a transactional insert (run + latestRunId together), and an audit payload naming the source run. The narrow assertResolutionCredentialed gate (ADR-0013 amendment 1) is retired — full resolution enforces the same credential check and every stronger one. Submit error codes retry can now surface (executor_unavailable and siblings) join both locales' error catalogs.
Retry now re-resolves against current configuration (ADR-0014), which gives the button two properties worth stating before the click: the new run starts over from checkout — previously answered questions and approved plans are asked again — and it binds agents/models to today's project configuration. A ConfirmDialog says both. Re-resolution can also reject the way submit does (grants, credentials, quota), so the mutation gains a localized error toast instead of failing silently. The run-page actions are additionally hidden from viewers, who could only ever collect a 403 from the member-gated endpoints.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTask retries preserve the pinned template and parameters while re-resolving agents, models, resources, budgets, credentials, and grants from current configuration. Submit-time overrides persist across retries, configuration errors fail fast, and the UI adds confirmation, localized errors, and viewer restrictions. ChangesRetry re-resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RunDetailPage
participant RetryEndpoint
participant resolveRunPlan
participant Database
User->>RunDetailPage: Confirm retry
RunDetailPage->>RetryEndpoint: POST /tasks/:id/retry
RetryEndpoint->>Database: Load pinned template, params, and overrides
RetryEndpoint->>resolveRunPlan: Resolve current configuration
resolveRunPlan-->>RetryEndpoint: Return run plan or validation error
RetryEndpoint->>Database: Create run and audit
RetryEndpoint-->>RunDetailPage: Return new run or error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/execution.ts`:
- Around line 499-520: Update the retry creation transaction around the runs
insert to serialize retries per task: lock the task row, then re-read the
current latest run within the same transaction and validate it remains eligible
before deriving the next run number. Use that transaction-local latest run for
the inserted run’s number and copied fields, preventing concurrent retries from
relying on the stale outer latest value.
In `@apps/api/src/test/execution.integration.test.ts`:
- Around line 465-466: In the retry test beginning with “retry preserves the
user's submit-time agent overrides,” remove the duplicate consecutive const
types declaration, keeping one declaration for the test’s existing setup.
In `@apps/web/src/pages/RunDetailPage.tsx`:
- Around line 105-108: Update the retry mutation’s onError handler in
RunDetailPage to use the existing toastApiError helper instead of duplicating
ApiError-versus-string handling, preserving the current error toast behavior and
centralized errors-catalog lookup.
In `@CHANGELOG.md`:
- Around line 67-71: Update the older provider-credentials changelog entry to
remove its claim that retries re-assert credentials against a frozen resolution.
Align its wording with ADR-0014 by describing retry as using the current
configuration and full re-resolution, while preserving the entry’s unrelated
historical details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18adba82-fb08-4531-9962-03f71ac516ee
📒 Files selected for processing (23)
CHANGELOG.mdapps/api/src/routes/execution.tsapps/api/src/test/execution.integration.test.tsapps/api/src/test/providers.integration.test.tsapps/web/src/pages/RunDetailPage.tsxdocs/adr/0014-retry-re-resolves-configuration.mddocs/design/01-domain-model.mddocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/design/06-frontend.mddocs/manual/en/02-concepts.mddocs/manual/en/03-running-tasks.mddocs/manual/zh-CN/02-concepts.mddocs/manual/zh-CN/03-running-tasks.mdpackages/db/drizzle/0008_agent-overrides.sqlpackages/db/drizzle/meta/0008_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/runs.tspackages/i18n/locales/en/errors.jsonpackages/i18n/locales/en/runs.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/i18n/locales/zh-CN/runs.jsonpackages/orchestration/src/resolve.ts
Comment on lines
+465
to
+466
| it("retry preserves the user's submit-time agent overrides", async () => { | ||
| const types = await jsonOf<Array<{ id: string; slug: string }>>( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate declaration.
Line 466 declares const types twice consecutively, making this test file fail to parse/type-check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/test/execution.integration.test.ts` around lines 465 - 466, In
the retry test beginning with “retry preserves the user's submit-time agent
overrides,” remove the duplicate consecutive const types declaration, keeping
one declaration for the test’s existing setup.
Four P2 findings, all confirmed against the code: - Disabled registry rows resolved as live: the MCP slug map included disabled servers, skill authorization never verified an active skill_versions row satisfies the ref's semver range, and the v1-sentinel/demo faber path accepted taskType.defaultFaberId without the active-row check the explicit paths had. All three now fail resolution (new skill_version_unavailable code, both locales); the version-matching rule is one shared helper (pickActiveSkillVersion) used by submit-time authorization and the worker's materializer so the two cannot drift. - Concurrent retries raced: the latest-run read preceded the transaction, so two racers both computed number N+1 and the loser died on runs_task_number_uq as a 500. The number computation and terminal gate now run under SELECT ... FOR UPDATE of the task row; the second racer gets the intended 409 run_active. - Audit rows were written after their transaction committed, in retry and submit alike — an audit failure left a committed mutation unaudited, violating the invariant ADR-0013 amendment 1 documents. Both audits now ride the mutation's transaction. Minors: ARCHITECTURE.md ADR count 13 -> 14; unused isCredentialGatedExecutor import dropped. Recorded as an ADR-0014 amendment per the 0013 review-round precedent.
…t the MCP filter repo_not_in_project was thrown by verifyRepoRefs but absent from both error catalogs — reachable at submit before, but the retry toast made it the first raw-English SubmitError a zh-CN user would actually see. Both locales gain the key, and a regression test drives the retry path against a deleted repo connection. The round-1 disabled-MCP filter also ships its missing regression test: bug-localize-fix references MCP 'github' optionally, so the test proves an active granted server is pinned into the retried run's manifest and a disabled one silently drops — the observable optional-ref semantics of the active-only slug map. Both admin manuals (en + zh-CN) document the skill-version and disabled-MCP resolution rules.
CodeRabbit's pass over the original push: its retry-serialization finding was already fixed by the codex round-1 commit, and the duplicate-declaration report is a false positive (two 'const types' in different function scopes; the file type-checks and CI was green on the reviewed head). The two remaining nits are real: the retry mutation's onError duplicated the ApiError-vs-string pattern that lib/toast.ts centralizes, and the provider-credentials changelog entry still described the retired frozen-resolution re-check that ADR-0014 superseded.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/test/execution.integration.test.ts`:
- Around line 524-550: Update the “retry rejects a default faber that was
disabled since submit” and “retry rejects a required skill whose versions were
all deprecated” tests to explicitly mark the task’s existing/latest run as
terminal before invoking the retry endpoint. Reuse the file’s established
run-status setup from the other retry tests, preserving the expected
faber_unknown and skill_version_unavailable responses without relying on test
order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c69ead2-6ae2-4f2e-9f64-e2c8a3a1eee3
📒 Files selected for processing (12)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/routes/execution.tsapps/api/src/test/execution.integration.test.tsapps/web/src/pages/RunDetailPage.tsxapps/worker/src/deps/resources.tsdocs/adr/0014-retry-re-resolves-configuration.mddocs/manual/en/04-administration.mddocs/manual/zh-CN/04-administration.mdpackages/i18n/locales/en/errors.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/orchestration/src/resolve.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/pages/RunDetailPage.tsx
- docs/adr/0014-retry-re-resolves-configuration.md
- packages/i18n/locales/zh-CN/errors.json
- apps/api/src/routes/execution.ts
The two rejection tests relied on a preceding test leaving the task's latest run terminal — reordering the file would turn their expected 400s into 409 run_active (the guard this same PR adds). They now force the terminal state themselves like every other retry test in the file (CodeRabbit round 2).
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
This was referenced Jul 28, 2026
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
1 participant
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.
Closes #7.
Why
Task retry copied the previous run's frozen resolution verbatim ("pinned — retries never re-resolve"), which made it structurally unable to heal configuration failures. The motivating incident: a requirement-delivery run whose reviewer slot froze
gpt-5.1-codex— a model id ChatGPT-account Codex auth rejects with a 400. After fixing the grants, the natural next step (Retry) would have re-billed the entire pipeline from checkout — clarify loop, plan approval, full implementation — and died at review identically. The only working recovery was re-filling the submission form.Two facts settled the design (recorded in ADR-0014): resolution-shaped failures are not classifiable from
runs.error.code(the incident finalized as catch-allmodel_error;executor_unavailablenever reaches a run's error), andresolveAgentBindingsis deterministic — with unchanged configuration, re-resolution reproduces the previous bindings exactly, so it strictly generalizes copying. Transient-failure retries therefore behave as before; behavior changes only when configuration changed, which is precisely when the operator wants it to.What
One commit per concern:
docs(adr)— ADR-0014. Retry is a re-submission of the pinned task: template version + params snapshot stay pinned; agent bindings, model resolution, resource manifest, budget, quota headroom, and repo-ref ownership re-derive against current configuration. Records the rejected alternatives (status quo, two-button UX, failure-code classification, override-reconstruction heuristics).feat(db)— persist submit-time agent overrides. The submit form'sagentsslot overrides were consumed at resolution and stored nowhere, so a re-resolving retry couldn't distinguish a deliberate executor choice from a template default. They now land ontasks.agent_overrides(migration 0008) and re-apply at retry; pre-migration tasks retry with template defaults (recorded consequence).feat(api)— the re-resolution. AresolveRunPlanhelper shared by submit and retry (the two paths cannot drift). Retry gains full submit parity it silently lacked: quota hard-stop, repo-ref ownership, refreshed manifest/budget, transactional insert (run +latestRunIdtogether), audit payload naming the source run.assertResolutionCredentialed(ADR-0013 amendment 1) is retired — full resolution enforces the same credential gate and every stronger one. Six submit error codes retry can now surface (executor_unavailableand siblings) join both locales' error catalogs.feat(web)— honest Retry. A confirm dialog states the new run starts over from checkout under today's configuration and re-asks earlier answers/approvals; rejections surface as localized toasts; run-page actions are hidden from viewers (member-gated endpoints — they could only collect a 403).Verification
bun run check,bun test(250 pass / 0 fail, integration suites ran against Postgres — none skipped),bun run templates:validate,bun run build,bunx commitlint --from main --to HEAD— all green.templateVersionId/paramsSnapshotprovably stay pinned; a submit-time reviewer override survives retry; empty grants → 400skill_not_granted; exhausted quota → 400quota_exhausted. The pre-existing credential-gate retry test passes unchanged through the new path.[Unreleased].Summary by CodeRabbit
run_active).