Conversation
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
Failure-finalization job-store operations remain unbounded and may still block cleanup and slot release.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR bounds post-ACK provenance writes during agent resume to prevent indefinite stalls during Redis outages.
Changes:
- Adds a five-second timeout around provenance persistence.
- Adds regression coverage for stalled job-store writes.
| File | Description |
|---|---|
api/server/controllers/agents/resume.js |
Applies the provenance-write timeout. |
api/server/controllers/agents/__tests__/resume.spec.js |
Tests timeout failure handling and slot release. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d38da4957d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d38da49 to
7de0d9e
Compare
berry-13
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8935c5830
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...(userSubmittedPaths.length > 0 && { userSubmittedPaths }), | ||
| ...(userSubmittedMessageFieldPaths.length > 0 && { userSubmittedMessageFieldPaths }), |
There was a problem hiding this comment.
When a scheduled approval contains an edit, response, rejection reason, or answer and finalizeScheduleResumeClaim subsequently returns false or throws because the schedule changed, these fields have already been persisted by the approval CAS. The controller then calls abortJob without ever running resumeCompletion; abortJob reconstructs the pre-decision chunks but copies this new provenance into its final response, so, for example, the original model-generated tool arguments are mislabeled as user-authored. Keep the decision provenance distinguishable until it is applied, or clear/transform it on this abort path so cancellation and restored-session attribution remain accurate.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab931f. Confirmed abortJob stamps jobData provenance onto the aborted response. On a schedule-fence rejection the controller now restores the job's pre-claim provenance (createdAt-fenced updateJob) before abortJob, so the pre-decision chunks are not attributed to the user. Covered by resume.spec.js 'restores pre-claim provenance before aborting a resume the schedule fence rejected', which fails without the restore. The residual case where the restore write itself fails is tracked at berry-13#166.
There was a problem hiding this comment.
Correction to the reply above: the restore-updateJob approach from dab931f (74bd0e0 after the rebase) was replaced in d30fa44. No pre-claim restore write remains; on a schedule-fence rejection the controller passes the pre-claim provenance from job.metadata to GenerationJobManager.abortJob, which publishes it in the same transition as the abort, so there is no separate write that can fail. Covered by steering.spec.ts (abort publishes caller provenance) and resume.spec.js (abortJob receives the pre-claim provenance, no updateJob). berry-13#166 is closed by that change; competing abort winners (schedule or account deletion, Stop) are tracked at berry-13#167.
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dab931f658
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a78502d to
d30fa44
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d30fa44ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| provenance: { | ||
| userSubmittedPaths: job.metadata.userSubmittedPaths, | ||
| userSubmittedMessageFieldPaths: job.metadata.userSubmittedMessageFieldPaths, | ||
| }, |
There was a problem hiding this comment.
When a schedule or account is deleted after approvals.resolve commits the decision provenance but before this fence check finishes, deleteScheduleForOwner independently reaches abortScheduledJob → abortJob without this override (packages/api/src/schedules/service.ts:611-643,1359-1364). If that abort wins the running → aborted transition, it publishes the newly claimed edit/answer paths against the pre-decision chunks; this call then receives already_settled and cannot correct the final event, so cancellation can still label model-authored content as user-authored. Fresh evidence beyond the earlier resolved thread is this competing schedule-deletion abort path; preserve the pre-claim provenance durably for every possible abort winner or pass it through that path as well.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and the same root as a pre-existing dev window: abortJob publishes the job record's HITL provenance on content rebuilt from pre-decision chunks, so any abort between the provenance commit and resumeCompletion applying the decision mislabels it (on dev, a Stop during initializeClient after the post-ACK write). This PR moves the start of that window from the ACK to the claim; the controller's own fence-rejection abort passes pre-claim provenance, but competing abort winners (schedule/account deletion, Stop) need abortJob to publish only applied-decision provenance. Tracked as the shared fix at berry-13#167.
There was a problem hiding this comment.
Fixed in cf8ef09 instead of deferring. The approval claim CAS now also records the provenance it replaces (preResumeProvenance), and abortJob publishes that whenever the post-terminal-CAS record shows the resumed provider never crossed its start CAS (providerExecutionStartedId, which the claim clears). That covers every abort winner (this fence, deleteScheduleForOwner via abortScheduledJob, account deletion, Stop) without a caller override. steering.spec.ts claim-then-abort cases fail without it; scenario an-edited-approval-keeps-its-user-submitted-provenance passes on 86465f4 (desktop light/dark, mobile).
…pre-claim paths from any unapplied abort The claim CAS now records the provenance it replaces. An abort that lands before the resumed provider crosses its start CAS publishes those paths, so schedule deletion, account deletion and user stops no longer label model-authored chunks as user-submitted.
d30fa44 to
86465f4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86465f45f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const provenance = | ||
| !providerStarted && jobData.preResumeProvenance != null | ||
| ? jobData.preResumeProvenance | ||
| : jobData; |
There was a problem hiding this comment.
When an abort wins after beginProviderExecution but before resumeClient() applies an edited approval, providerExecutionStartedId is already set (resume.js:1891-1902) while abortContent still contains the original model arguments, so this branch labels those arguments as user-authored. Conversely, the abort route can stamp a claimed ask_user_question answer into the content while this marker is still unset, causing this branch to omit the answer's provenance. Fresh evidence on this head is the new steering test that calls beginProviderExecution and then abortJob without applying any decision content, yet expects the claimed provenance; select provenance based on the content actually published rather than provider-start status.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
| const provenance = | ||
| !providerStarted && jobData.preResumeProvenance != null | ||
| ? jobData.preResumeProvenance | ||
| : jobData; |
There was a problem hiding this comment.
On a pre-provider abort, these local variables make the live final event use preResumeProvenance, but AbortResult.jobData still contains the newly claimed paths. The abort route's beforePublish callback saves pendingAbortResult.content with jobData.userSubmittedPaths and jobData.userSubmittedMessageFieldPaths (api/server/routes/agents/index.js:755-815), so after a competing schedule/account/user abort the database row still labels the original model content as user-authored and a reload disagrees with the SSE. Fresh evidence on this head is that the new provenance selection is confined to final-event construction; return the selected provenance to persistence or build both outputs from the same response message.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
danny-avila
added
the
🗺️ Agent Runtime
Pull Request
Summary
When a user answers an
ask_user_questionor edits, rejects or answers a tool approval,ResumeAgentControllerwins the approval claim (approvals.resolve, which spends the pending action and flips the job torunning), sends the 200 ACK, and only then writes the user-submitted provenance paths to the job store with a separate, unboundedupdateJob. ioredis can queue that command indefinitely on a half-open socket or in Cluster mode, so the continuation never starts, the failed-resume cleanup never runs, and a retry is answered 409 because the action is spent.The provenance paths are already computed before the claim, and the claim's
JobMetadataPatchalready carries both fields, so they now ride in the claim's own CAS alongside the owner capability fields. The post-ACK write is gone: provenance commits atomically with the transition that spends the action, and a job-store failure there happens before the ACK, where the existing claim error path answers 500 and releases the concurrency slot.The one exit between the claim and the ACK is a scheduled resume whose schedule changed under it: the fence rejects it and the job is aborted without the decision ever being applied.
abortJobnow takes the provenance to publish, and that path passes the job's pre-claim values, so the aborted response does not mark the original model output as user-authored and no separate rollback write is needed.Addresses berry-13#120, a deferred finding from #13811, and berry-13#166.
Type of change
Testing
Tested environments/configuration:
RedisJobStoretransition against real Redis in the cache integration workflow.Automated tests:
api/server/controllers/agents/__tests__/resume.spec.js: the provenance cases now assert the paths arrive in theapprovals.resolvepatch and that no post-ACKupdateJobcarries them; approve-only resumes send no provenance; a schedule-fence rejection passes the pre-claim provenance toabortJob. 151 passed; the provenance cases fail against the previous controller.e2e/specs/mock/scenarios/approval-provenance.spec.ts(@scenario:an-edited-approval-keeps-its-user-submitted-provenance): edits a paused MCP tool call in the mock harness, waits for the resumed run to execute the edit, and reads the saved reply through/api/messages, which marks the edited arguments as user-submitted. The shared approval helpers moved totool-approvals.helpers.tsso the scenario reuses them.packages/api/src/stream/__tests__/steering.spec.ts: an abort given caller provenance publishes it instead of the job record's.packages/api/src/stream/__tests__/pendingAction.spec.ts: resolving throughGenerationJobManager.approvalspersists both provenance shapes on the resumed job.packages/api/src/stream/__tests__/RedisJobStore.stream_integration.spec.ts: arequires_actiontorunningtransition with a provenance patch reads both shapes back from Redis (runs in CI; no local Redis was available).tsc --noEmitforpackages/apiclean.Screenshots / recordings
No user-facing change.
Risk / compatibility
The provenance fields move from a follow-up
HSETinto the claim's transition patch; both go throughserializeJob, so the stored shape is unchanged and readers need no change. No new configuration.Checklist