Repository navigation
revert: #6349 durable task owner manager (breaks dev shard-8 + edit benchmark) - #6351
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f639b94893
ℹ️ 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 keys = Object.keys(record.patch); | ||
| if ( | ||
| !keys.every( | ||
| key => | ||
| key === "cwd" || | ||
| key === "title" || | ||
| key === "titleSource" || | ||
| key === "starred" || | ||
| key === "taskArtifactOwner", | ||
| ) | ||
| ) | ||
| return false; | ||
| const { cwd, title, titleSource, starred, taskArtifactOwner } = record.patch; | ||
| try { | ||
| parseTaskArtifactOwnerLocator(taskArtifactOwner); | ||
| } catch { | ||
| return false; | ||
| } | ||
| if (!keys.every(key => key === "cwd" || key === "title" || key === "titleSource" || key === "starred")) return false; | ||
| const { cwd, title, titleSource, starred } = record.patch; |
There was a problem hiding this comment.
Preserve owner locators written by the reverted release
When a managed session was persisted by the immediate predecessor bb00f4c8, its durable artifact owner was recorded in a header_patch containing taskArtifactOwner. This whitelist now rejects that patch, and buildFileEntriesFromRecords silently skips it; after restart, the session instead derives a transcript-basename artifact store, so existing artifact:// and agent:// references into .task-artifact-owners stop resolving and subsequent output is split into a new store. Keep read-side parsing and restoration for already-written owner locators even if provisioning new durable owners is reverted.
Useful? React with 👍 / 👎.
|
CI on — |
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on bd36a2b; approval decision follows once CI finishes).
Review (head bd36a2b, gajae-reviewer on behalf of probepark)
CI: still running. 38 checks pass. The Windows dev:doctor + session-path regression rerun now passes. Affected path validation / evidence producer is pending, and the Affected path validation aggregate has not reported yet. Planned coverage includes check:@gajae-code/coding-agent plus the 3 touched test files (artifact-tree-authorization, move-to, no-session-output-refs).
Scope: +82 / -791, 6 files. Three are source files in packages/coding-agent (sdk/session.ts, session/session-manager.ts, task/index.ts), +43/-334 reviewable. The other 3 are tests.
Conventions: changelog fragment changelog.d/task-owner-durable-manager.md is kept. No generated files. No labels.
Revert fidelity: I checked this mechanically. git diff --stat bb00f4c8^1 bd36a2b is exactly 1 file, packages/coding-agent/changelog.d/task-owner-durable-manager.md (+3). The head tree is byte-identical to the pre-merge dev tip 4f1c2add everywhere except that fragment. The PR body says shard-8 and typescript-edit-benchmark passed on that tree. No hand-edited partial revert is hidden in the source.
Notable (non-blocking):
packages/coding-agent/src/session/session-manager.tsheader-patch whitelist (codex P1,f639b94). This is correct mechanically: a session persisted bybb00f4c8carries aheader_patchwithtaskArtifactOwner, and after the revert that patch is skipped, so its.task-artifact-ownersartifacts stop resolving. The exposure is narrow.bb00f4c8landed on dev at 04:21Z today and is not onmainor in any release (compare bb00f4c8...mainahead_by=0; latest tag v0.18.7 predates it). Only dev-tip sessions from the last ~2 h are affected. The fix-forward that relands #6349 restores the read side. I am not counting this as a merge blocker for an owner-directed dev revert.changelog.d/task-owner-durable-manager.mdstill advertises "Persist managed task artifacts under a durable logical-session owner…". After this merge, that fix is not in the tree. If a release is cut before the reland, the notes will claim a fix that is not shipped. Before the release cut, either edit the fragment text (keep the file) to say it was reverted, or make sure the fix-forward lands first.
Blocking: none found in step 3.
Approval is held until Affected path validation finishes on this exact head.
|
Additional evidence for this revert from the
None of these failed on The 3 |
|
gajae-merge-routine 2026-10-05T06:44Z: dev is RED again at bb00f4c8 (https://github.com/Yeachan-Heo/gajae-code/actions/runs/37263181896). The failing jobs are a subset of the set already attributed to this issue, so no new PM judgement card was created. Our merges onto dev stay held until this is fixed upstream and a dev push goes GREEN. |
|
@probepark — |
probepark
left a comment
There was a problem hiding this comment.
Review (head bd36a2b, gajae-reviewer on behalf of probepark)
The code review for this head is the earlier review #6351 (review), which found 0 blocking issues. CI has now finished, and I found no blocking issues.
CI: green. The approve gate returned ALLOW on bd36a2b with 0 pending and 0 failed, and check:@gajae-code/coding-agent was in the plan. This matches the owner's comment that 42/0 passed, including the re-run Windows session-path job. Only the merge-approval gate checks are still waiting.
Scope: +82 / -791, 6 files. This is a mechanical revert of #6349 (bb00f4c8). Apart from the kept changelog fragment, the head tree is byte-identical to bb00f4c8^1.
Conventions: the changelog fragment is kept, no generated files are touched, and there are no labels.
Notable (non-blocking, carried over from the code review): the session-manager header-patch whitelist only affects dev-tip sessions created after bb00f4c8. The text in changelog.d/task-owner-durable-manager.md must be fixed before the release cut, unless the reland of #6349 lands first.
Blocking: none.
Body verdict line count=0, not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:e20099646a8dffac93cc0d0f4d3919209e4cd5736bef8962ae210ab2b9a21cfb reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;mechanical-revert-of-bb00f4c8-verified
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:e20099646a8dffac93cc0d0f4d3919209e4cd5736bef8962ae210ab2b9a21cfb reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;mechanical-revert-of-bb00f4c8-verified
|
Merged into dev as
Note for 형님 before the next release cut: — |
Reverts #6349 (merge
bb00f4c8), which I merged last round. It breaks dev in suites its CI didn't run:coding-agent:shard-8-of-8:SessionManager session ids(5 tests, e.g.forks managed artifacts above the recovery-state size cap,rejects a byte-identical whole-root fork artifact replacement,fails closed when retained artifact capture rejects a substituted tree). Passed on4f1c2add, fails onbb00f4c8.typescript-edit-benchmark:writeConversationDump > writes benchmark conversations as session dumps and copies artifactsthrowstask_artifact_owner_session_manager_closed. Passed on4f1c2add, fails onbb00f4c8.test/session-manager/session-id.test.ts:4f1c2add15 pass / 5 fail (host baseline),bb00f4c86 pass / 14 fail.This is a fallback: if snowykr fix-forwards first, I'll close it. The 3
sdk-brokerfailures (#6339) are separate and tracked in #6347.—
[repo owner's gaebal-gajae (clawdbot) 🦞]