feat(gc): retire logical task owners through immutable native journals - #6341
Conversation
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 2b1c9de, gajae-reviewer on behalf of probepark)
CI: green — all planned checks pass at this head, including check:@gajae-code/coding-agent, ts-build, Virtual integration validation, gjc-state-gates, and the 4 targeted test shards (gc-task-artifact-owner-retirement, gc-runtime, session-manager-resident-cache, notifications-live-stream). Approve gate: ALLOW (no pending, no failed, no need-local).
Scope: +1535 / -23, 4 files. Reviewable production code is 772 lines: packages/coding-agent/src/gjc-runtime/gc-runtime.ts (+196/-9) and packages/coding-agent/src/session/session-retirement.ts (+553/-14). The new test file (+783) and the changelog fragment are excluded from the line count.
Conventions: changelog fragment changelog.d/gc-task-owner-retirement.md is present (Fixed). No generated files. No console.* calls. No new workers. No labels.
Notable:
session-retirement.ts:retireGcOwnerTranscript: the journal order isprepared→ artifact phase →artifacts_removed→ native scrub →owner_pending/owner_retired→ transcript delete. Every state is published only after the effect it records has returned. A replay fromowner_pendingrevalidates the stored continuation (verifyTaskArtifactOwnerRetirementContinuation) before callingretireTaskArtifactOwneragain. The sibling-transcript check is repeated before each destructive step. I checked this and it is consistent.retireOrphanSessionTranscript: when the transcript is missing, the code never treats that as deletion authority. It re-reads the receipt and requires deep equality, rejects a reappeared transcript, refuses atprepared, and always ends incleanup_pendingwith phasetranscriptand the action kept (neverreclaimed). This matches the changelog claim.- Note (not blocking):
gc-runtime.ts~L1221. The new helperssessionRetirementContinuation/orphanRetirementRecordwere inserted below the existingClassify (and optionally retire) session transcripts…JSDoc, so that comment now documents the wrong function. Move it back aboverunGcDiskSessions. - Note (not blocking): this PR builds on #6337 (
taskArtifactOwnerStorageContextForScope,bindManagedGcSessionRetirementTarget, etc.), and #6343 (an open revert of #6337) would remove that base. If #6343 lands first, this PR must be rebased or re-scoped.
Blocking: none
Body has no verdict line, so I did not edit it. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:2203de0d49c6c557748c28747fbe30fe4d4c18df9388deaa775092388a367eb8 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;reviewable-772-read;journal-order-checked;orphan-no-delete-authority;changelog-fragment
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:2203de0d49c6c557748c28747fbe30fe4d4c18df9388deaa775092388a367eb8 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;reviewable-772-read;journal-order-checked;orphan-no-delete-authority;changelog-fragment
|
Holding the merge until #6343 (revert of #6337) lands. probepark's approval is on the exact head
So it only breaks in combination with #6337's code. I'll merge right after #6343. — |
|
Updated exact head |
GC must retain the original owner evidence across every publication phase and persist actual native disposition before transcript effects. Resume residual journals through read-only scope authority, keeping native pending namespaces visible rather than treating absent canonical paths as success. Lore-id: 04fb618d Constraint: original prepared deletion evidence never expands or is recaptured Constraint: native owner completion precedes GC transcript retirement Constraint: automatic persistent owner and task admission activation stays off Tested: 106 tests and 856 assertions across seven native consumer suites Tested: coding-agent vendor, Biome and TypeScript checks Not-tested: Linux and Windows runtime behavior of this driver Confidence: high Scope-risk: narrow Reversibility: revert
Deferred owner retirement still requires trusted protocol inspection before artifact effects. Supply the real scope inspector rather than bypassing the sibling boundary. Lore-id: 2a7e05bc Constraint: fixture authentication does not enable persistent owner production Tested: GC retirement, disk retention and journal suites Tested: coding-agent vendor, Biome and TypeScript checks Confidence: high Scope-risk: narrow Reversibility: revert
The GC driver calls installed storage and journal authority directly; it does not depend on the new SDK deletion consumer. Exercise the actual old broker before and after genuine GC retirement instead of inferring that boundary from cumulative tests. Preserve real pending SDK receipts from an injected native transcript I/O refusal, then prove restart cannot promote retained, absent or fsynced empty authorities into completion or touch owner continuations. Lore-id: 7206e3a1 Constraint: SDK production remains byte-identical to installed dev Rejected: fabricate SDK cleanup DTOs | data would masquerade as authority Confidence: high Scope-risk: narrow Reversibility: easy Tested: standalone GC 18 pass including 8 mixed-consumer cases and coding-agent checks Not-tested: cumulative Linux or Windows execution Directive: empty unauthorized headers remain rejected, not completion proof
The composed consumer tree exposed a test pinning the old SDK unsupported- owner refusal before GC, even though the new SDK now legitimately captures native owner authority. Test the actual invariant after GC: fresh SDK calls cannot adopt missing owner authority or mutate original namespaces/journals. Keep all genuine legacy-receipt cases and run both current consumer products. Lore-id: b0427c51 Constraint: no production guard or native behavior changes Confidence: high Scope-risk: narrow Reversibility: easy Tested: composed GC and SDK suites 26 pass, 298 assertions, coding-agent checks Directive: distinguish truthful artifacts pending from completed deletion
d9a3d30 to
f22a95d
Compare
|
Actual dependencies #6338/#6339 were externally installed (probepark,21:00:02Z/21:00:11Z); currentdev58 also includesPaseo6340. Rebased complete GC onto |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head f22a95d, gajae-reviewer on behalf of probepark) — re-request after the rebase from 2b1c9de/d9a3d30
Verdict: no blocking candidate. APPROVE is held only because CI for this head is still running.
CI: pending — Dev CI run 37238945591 is still in_progress. These are green: check:@gajae-code/coding-agent, ts-build, cli-smoke, native-build, and the 4 targeted shards (gc-task-artifact-owner-retirement, gc-runtime, session-manager-resident-cache, notifications-live-stream). Two jobs have not finished: gjc-state-gates / native addon and Windows dev:doctor + session-path regression. The approve gate returns NEED_LOCAL because a duplicate skipped Dev CI run hides the aggregate. The run itself is pending, so I treat this as ci-pending.
Scope: +1531 / -23, 4 files, base dev 58e868b. The production files are gjc-runtime/gc-runtime.ts and session/session-retirement.ts, 772 reviewable lines in total.
Delta from the head I approved earlier (2b1c9de): gc-runtime.ts and session-retirement.ts have the same blob SHAs, and so does the changelog fragment. Only test/gc-task-artifact-owner-retirement.test.ts differs (+7/-11). It drops the "installed SDK refuses the owner before GC" step from the prepared and artifacts_removed cases, and accepts cleanup_pending with phase: "artifacts" and no taskArtifactOwnerRetired as a valid SDK replay result. This matches the SDK deletion change that is now on dev (#6339). The earlier failure, 179 passed / 2 failed in the combination run, came from #6337's code. The two tests it hit are the ones this commit rewrites. The test still asserts the owner tree and transcript are unchanged after the replay, so the stale-authority guard is still exercised.
Conventions: the changelog fragment is present (Fixed). No generated files, no console.*, no new workers, no labels.
Notable:
test/gc-task-artifact-owner-retirement.test.ts:~367-375— thecleanup_pendingbranch is not strict about which of the two outcomes the SDK should return. A single expected outcome would catch regressions faster. This is a note only.- The owner's hold in the issue comment (5984692186) is about #6343 landing first. #6343 is still open, so that ordering is a merge-time matter for the maintainer. It is not a code finding on this head.
blocking: none
Approve will be submitted when the remaining Dev CI jobs finish green at this exact head (the pr-scout re-queues on ci-pending). Body verdict line not edited.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head f22a95d, gajae-reviewer on behalf of probepark): exact-head verdict after CI finished
CI: green. Dev CI run 37238945591 completed with conclusion success at f22a95d. The two jobs that were still pending in my earlier held review (5408571935), gjc-state-gates / native addon and Windows dev:doctor + session-path regression, have both finished green. Affected path validation (aggregate), check:@gajae-code/coding-agent, ts-build, cli-smoke, Virtual integration validation, and the 4 targeted shards also pass. The skipped run 37239957185 is the body-edit "Not code evidence" duplicate. Approve gate: ALLOW (no pending, no failed, no need-local).
Scope: +1531 / -23, 4 files, base dev 58e868b. Reviewable production code is 772 lines: gjc-runtime/gc-runtime.ts (+205 total churn) and session/session-retirement.ts (+567 total churn). The test file and the changelog fragment are excluded from that count.
Conventions: the changelog fragment changelog.d/gc-task-owner-retirement.md is present. No generated files, no console.*, no new workers, no labels.
Notable (unchanged from 5408571935; the production blobs are identical to the 2b1c9de I reviewed in full):
test/gc-task-artifact-owner-retirement.test.ts:~367-375: thecleanup_pendingbranch accepts either SDK replay outcome. Pinning a single expected outcome would catch regressions sooner. This is a note only.- The maintainer's hold (issue comment, 21:42Z) waits for #6343 to land first. #6343 is still OPEN. Merge ordering is a maintainer decision, not a code finding on this head.
Blocking: none
Body has no verdict line, so I did not edit it. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:e23ad4a8205b2f238f82257a779fd9a501d2bee29945f63bb483aa0845d1ffb6 reviewer:human reviewer-id:probepark evidence:ci-green-run-37238945591;approve-gate-allow;prod-blobs-identical-to-reviewed-2b1c9de;test-delta-read;changelog-fragment
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:e23ad4a8205b2f238f82257a779fd9a501d2bee29945f63bb483aa0845d1ffb6 reviewer:human reviewer-id:probepark evidence:ci-green-run-37238945591;approve-gate-allow;prod-blobs-identical-to-reviewed-2b1c9de;test-delta-read;changelog-fragment
|
Merged into dev. probepark approved the exact head
So it is safe whether or not #6343 lands. — |
|
For maintainer hold5984692186: the concrete four separate #6337 regression findings are now reproduced and repaired in #6346 (exactbaf/current58, full BOTH requestedfiles98/15/0/658 plus fresh full933tests21files910/23/0/6416/packagecheck0; complete managed layer787<=800). #6341's old179/2 combined failures were the separate two before-GC old-SDK unsupported-owner assertions already corrected test-only (+7/-11) at currentf22; its production blobs unchanged. CurrentGC20fileproof830/8/0/5921/package0 includes installedSDK6339. No falseclaimthat historical6337gatescaughtthese4; no reverting/closing your PR/order or leadermerge. Activation staysOFF while current managed repair/GC installation are unresolved. |
3 participants
Complete independently safe GC product
Actual dev base
58e868b3329921be65ea8d7a7e8bae28cad51d52, including installed helper #6335/managed #6337 and upstream retry identity #6333/Phase-A benchmark #6318. Exact headf22a95d208ad99889f1a762803b41b710911853f.Owned AND actual dev production additions+deletions: 772: gc-runtime +196/-9 =205, session-retirement +553/-14 =567. Four files; tests/changelog excluded. No generated/native changes, previous-branch base or oversized cumulative stack PR.
Dependency evidence and behavior
Existing canonical plan id11 depends on installed storage/journal/readonly authority/protocol, not the separate SDK effect consumer14. Bounded read-only architect source assessment found no call into the new SDK lifecycle consumer; source identity confirms all GC authority APIs are already installed. This is not inferred from producer-OFF, typechecking or historical branch ancestry.
SDK broker and lifecycle production are byte-identical to actual dev, which now includes externally installed complete SDK #6339. This GC PR does not bundle or reactivate its production delta. Standalone old-SDK qualification on42d remains historical dependency evidence; new exact current58 qualification executes the actually installed complete SDK and GC together. Actual GC owns immutable original prepared evidence, authenticated live profile/root/sibling/protocol/native side-role authority, shrinking pending continuations and native outcome publication before transcript effects. Receipt discovery survives canonical transcript absence. Payload retirement/namespace retention remains public artifacts cleanup_pending/reclaim_failed, not reclaimed or physical completion from absence/flags.
Observable mixed-consumer proof
Eight new real-broker/real-GC cases exercise installed SDK before and after GC prepared/artifacts_removed phases and restart. Six create genuine persisted legacy SDK pending receipts via one explicitly injected native transcript I/O failure (all other native calls delegate; never fabricate native success), then test retained, absent and fsync-empty canonical states after actual GC native retirement. SDK refuses completion, preserves canonical/replacement bytes and actual retained owner namespace/root identities, and leaves original journal/outcome immutable. Empty unauthorized headers correctly fail invalid_input and journal authority re-read; they are not promoted to native completion. Two further cases prove fresh SDK authority cannot be adopted after GC retirement: the original SDK rejects invalid_input while the complete SDK truthfully remains artifacts cleanup_pending; neither may complete or mutate owner namespaces/journals. Composed old-oracle24pass2fail is retained; test-only forward contract correction passed composed26/298 and current standalone18/161, without production changes or reducing case cardinality.
Wrong initial test oracles (expecting a pending receipt from a fresh owner-bearing SDK request, or allowing an empty unauthenticated header re-read) are preserved as failed attempts; corrected to actual stricter refusals, not by weakening safety checks. Full root/child dev/ino/mode/size/mtime/ctime/content snapshots retained. No new skips or test deadline/cardinality reduction.
Fresh exact current-head qualification
Independent clean detached QA
11-installed-SDK-base58e868b3, strict genuine unchanged 0.18.7 addon8d1/source261/buildafbb/tree413/descriptor untouched:Upstream OpenCodex formatting error is independently repaired by #6338; no root gate waived. Prior integrated63b716/c93 full-root735.27s is historical, not this2b/42d runtime or approval. Darwin execution only; current exact code-event CI/write-access approval/posted verdict/external installation still required. No SDK6339/6337 approval transfer or leader merge. Persistent-owner producer and trusted task admission remain OFF until all current consumers install. Original44/reference7e5/PR6240 unchanged; real retained execution/final frozen cohort/critic/allstory closure unfinished.
Current installed dependencies, no historical proof transfer
Externally merged formatting6338 exactf1 viaa1588149332bf73933c034ed5ccaf87fdba66a2a at21:00:02Z and SDK6339 exact667 via781e5ed5e39754e2a24e0d495d736370119907eb at21:00:11Z, mergerprobepark; latestactual58 also preservesPaseo6340 strict-port behavior. Freshnewf22/58 proof830/8/5921/20files/package0 includesSDK deletion andPaseo tests, unchanged genuine0187native/descriptor. Rebasedall4ownedGCcommits, productionstill772. Exact leased9->f22, new code-eventCI/exactheadreviewrequired. Oldd9/2b gates/CI/reviews are not transferred. Producer/taskadmissionstillOFF.