feat(session): add fenced logical owner cleanup coordination - #6335
Conversation
probepark
left a comment
There was a problem hiding this comment.
Review (head 6838bd6, gajae-reviewer on behalf of probepark)
CI: PR-caused (branch state), 1 — Affected path validation / plan failed at step Verify PR head contains exact base: Exact-head CI requires this PR head to contain base df6c32605f666b2a3598650cbe985cd69313c1f5; rebase onto current dev. (job). evidence producer and the Affected path validation aggregate failed downstream of it. No path logic ran, so the affected shards and the changelog guards are unevaluated, not passing. The managed-task-owner-cleanup-api.test.ts + package check results in the PR body are local-only evidence for this head. gjc-state-gates / native addon was still pending.
Scope: +972 / -0, 3 files. ocr reviewable: +472 (packages/coding-agent/src/session/internal/managed-task-owner-cleanup.ts). Excluded: test (+498) and changelog fragment (+2).
Conventions: changelog fragment changelog.d/task-owner-cleanup-helper.md present. No generated files. No labels. No console.*, no new Worker, no mock.module/spyOn. Body has no verdict line.
Branch state: head parent is ccdce81 → 6a2a4b9 (#6332). Current dev = df6c326 = merge of #6322 on top of 6a2a4b9, so head and base are siblings off 6a2a4b9 and git merge-base --is-ancestor df6c326 6838bd6 is false. The base-side delta is only modes/acp/acp-agent.ts + test/acp/acp-cancel-settlement.test.ts (+20/-1), which does not overlap this PR. The code review below therefore carries over verbatim after a rebase onto df6c326.
Notable:
managed-task-owner-cleanup.ts:325-345/:376-396:owner_pendingreceipts are published withoutownerRetirementAttempt. I checked that this is correct.publishManagedGcSessionRetirementReceiptderives the attempt itself (managed-session-scope.ts:3521,current.ownerRetirementAttempt + 1or1) and enforcescontinuationExtendsPrevious(:3530).managedGcReceiptForPublicationaccepts an absent attempt (:2512-2514). The tombstone-sibling inheritance arm (:264-271) does not callverifyTaskArtifactOwnerPhysicalRetirementitself, but the publisher re-verifies physical retirement for everyowner_retiredpublication (managed-session-scope.ts:3552). So that arm is not fail-open.managed-task-owner-cleanup.ts:237:if (!evidence) throw …evidence_missingis unreachable, becauseManagedGcSessionRetirementReceipt.taskArtifactOwnerDeletionEvidenceis non-optional (managed-gc-retirement-codec.ts:29) and the codec parser throws on corrupt evidence. Non-blocking.test/managed-task-owner-cleanup-api.test.ts:465accepts eitherowner_retiredorpending|payload_retireddepending on the native outcome, so neither arm is pinned on any given platform. Consider forcing one deterministic arm per test.
Checked and clean: the 4c/legacy-scope fence (:106-117, :123-127), the prepared-receipt immutability checks (:128-163), the cross-scope inheritance guards (:277-296), the outcome/flag consistency check (:367-374), and the lock assertOwned bracketing around every sibling/protocol inspection and native retire call.
Blocking:
- Rebase onto current
dev(df6c32605f666b2a3598650cbe985cd69313c1f5) and push, so exact-headplancan run the affected shards for this helper and its test. No code change is requested. The code review above found no blocking defect.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:beb08bbe7cb50696f0a38c9afc068a5e12ac628aaa7285c33a030a6c2897fc26 reviewer:critic reviewer-id:gajae-reviewer evidence:head-not-containing-base-df6c326;plan-unevaluated;helper-code-read-no-defect;publisher-attempt-and-physical-verify-checked
Supply an independently callable cleanup API with immutable journal evidence, actual native outcome persistence and same-evidence inheritance. Require the live protocol inspector across async sibling checks before effects; snapshots never grant deletion permission. Current Scope/SDK/GC activation stays separately gated. Lore-id: a1930de8 Constraint: preserve pending native dispositions and original evidence Constraint: no serialized scope capability or protocol snapshot authority Tested: real journal/storage/helper suites35pass211assertions; full coding-agent check exit0 Not-tested: full managed/SDK/GC consumer integration or cross-platform runtime Confidence: high Scope-risk: bounded Reversibility: revert
Deliver the complete helper on externally installed verified storage without enabling owner producers or transplanting future consumers. Keep dependency-sized review and actual native/evidence boundaries explicit. Lore-id: d750a462 Constraint: helper data and receipts cannot grant deletion authority Tested: helper, journal, storage, retirement and disk behavior suites Tested: coding-agent vendor, Biome and TypeScript checks Confidence: high Scope-risk: narrow Reversibility: revert
6838bd6 to
0a139e6
Compare
|
Rebased onto actual installed dev df6c326 as requested; new exact head0a139e6e8bc9a1e5b34ef05dc4bc8e41e0df2bbb contains that base. Helper code is unchanged, owned AND actual production472. Fresh independent clean exact-head QA: 272 pass / 8 existing skips / 0 fail / 1535 assertions, including upstream ACP cancellation settlement; full coding-agent vendor/Biome/types exit0. Existing strict native version/source/digest/build-tree checks passed. Previous6838 CI plan-unevaluated failure and review remain historical; no gate/approval transfer. Fresh code-event CI and review required on this new head. |
|
Current exact head |
probepark
left a comment
There was a problem hiding this comment.
Review (head 0a139e6, gajae-reviewer on behalf of probepark; re-requested 2026-10-04T17:44:42Z)
CI: green. All checks are completed on this head: Affected path validation aggregate SUCCESS (18:02:49Z), which includes check:@gajae-code/coding-agent, ts-build for coding-agent, native-build, cli-smoke, and the 3 targeted test shards. gjc-state-gates SUCCESS. Virtual integration validation SUCCESS (18:12:10Z).
Scope: +972 / -0, 3 files: packages/coding-agent/src/session/internal/managed-task-owner-cleanup.ts (+472), its test (+498), and a changelog.d fragment (+2).
Conventions: changelog fragment present, no generated files changed, no labels.
Notable:
- This head is a pure rebase onto dev
df6c326(git merge-base --is-ancestor df6c326 0a139e6passes).git diff --binary --full-index base...headsha256 isbeb08bbe7cb50696f0a38c9afc068a5e12ac628aaa7285c33a030a6c2897fc26, the same digest as the6838bd6diff reviewed in 5407338592. That review found no code defects. - The only blocking finding from 5407338592 was "head does not contain base, so exact-head plan failed". That is resolved:
Affected path validation / planSUCCESS at 17:45:34Z. Trigger: #6335 (comment)
Blocking: none. The earlier CHANGES_REQUESTED on6838bd6is dismissed as superseded.
This is a COMMENT, not an APPROVE: the reviewer's local approve-gate check could not run in this pass (API budget guard). An approval will follow in a later pass once the gate runs.
Body has no verdict line; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:beb08bbe7cb50696f0a38c9afc068a5e12ac628aaa7285c33a030a6c2897fc26 reviewer:human reviewer-id:probepark evidence:rebase-only-same-digest;ci-green-incl-coding-agent-check;prior-blocker-resolved
Blocking findings resolved; superseded by the follow-up verdict.
probepark
left a comment
There was a problem hiding this comment.
Review (head 0a139e6, gajae-reviewer on behalf of probepark)
This is the approval promised in review 5407488609. The local approve gate has now run on this head: ALLOW, pending=0, failed=0, need_local=0, covered=check:@gajae-code/coding-agent.
CI: green. Only the human-approval gates are outstanding (Merge approval*, PR contract); they are waiting on this verdict.
Scope: +972 / -0, 3 files. ocr reviewable: +472 (packages/coding-agent/src/session/internal/managed-task-owner-cleanup.ts). Excluded: test (+498) and the changelog fragment (+2).
Conventions: changelog fragment changelog.d/task-owner-cleanup-helper.md present. No generated files. No labels. No console.*, no new Worker, no mock.module/spyOn.
Notable:
- I re-read the helper at this head.
git diff --binary --full-index df6c326...0a139e6has sha256beb08bbe…6fc26, identical to the6838bd6diff reviewed in 5407338592, so that code review stands. The lockassertOwnedbracketing wraps every sibling scan (:252-260,:311-319,:431-439) and the native retire call (:335-337). The cross-scope inheritance (:291-310) is fenced on agentDir, sessionsRoot, and directoryPath, and it re-verifies physical retirement before publishing. - Non-blocking:
managed-task-owner-cleanup.ts:251(if (!evidence) throw …evidence_missing) is unreachable, because the receipt evidence field is non-optional. The test atmanaged-task-owner-cleanup-api.test.ts:465accepts either native outcome arm, so neither arm is pinned on any given platform.
Blocking: none.
Body has no verdict line; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:beb08bbe7cb50696f0a38c9afc068a5e12ac628aaa7285c33a030a6c2897fc26 reviewer:human reviewer-id:probepark evidence:approve-gate-allow;ci-green-incl-coding-agent-check;rebase-only-same-digest;helper-code-read-no-defect
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:beb08bbe7cb50696f0a38c9afc068a5e12ac628aaa7285c33a030a6c2897fc26 reviewer:human reviewer-id:probepark evidence:approve-gate-allow;ci-green-incl-coding-agent-check;rebase-only-same-digest;helper-code-read-no-defect
Summary
Install the complete managed logical task-owner cleanup helper on externally merged verified storage #6332. This is an independently reviewable API layer, not producer/admission activation or a partial caller stub.
payload_retired/namespace-pending distinctions.Exact source and budget
Base dev
df6c32605f666b2a3598650cbe985cd69313c1f5: external #6332 exact6c78 installation and upstream #6322 ACP acknowledged-cancellation settlement. Preserve readonly evidence capture, mandatory pre-artifact sibling fence, #6329 native/reaper and unrelated upstream changes.Exact head
0a139e6e8bc9a1e5b34ef05dc4bc8e41e0df2bbb.Owned AND actual dev production 472 additions+deletions, after formatting. Helper +472, tests +498 and per-package changelog +2 excluded. Three files, no native/generated descriptor change.
Verification
Independent clean exact-head QA qualifier
10c-independent-after6332-basedf6c3260, existing strict runner with explicit source/version/digest/build-tree agreement and missing-test-file refusal:QA re-executed on NEW exact head after rebase, clean before/after; warnings retained. Initial6838/base6a2 qualification is historical, not transferred. Include actual upstream ACP cancellation behavior in this fresh qualification.
Initial exact6838 Dev CI37219463412 failed its immutable-base planning gate after dev advanced to df6: affected helper plan/shards were unevaluated, not executed changed-code failures. Actual CHANGES_REQUESTED review found no blocking code defect and requested rebase; retain its posted needs-human beb08bbe7cb50696f0a38c9afc068a5e12ac628aaa7285c33a030a6c2897fc26. Rebased without changing helper code, with new source/base qualification and no approval/CI transfer. Fresh current code-event CI is required.
Native runtime remains genuine installed release0.18.7: addon SHA256
8d1c6b2dcaafc1889eeb5719c74075834d0088ba903bffdf1beac71606a93325, path_identity source2617654e11e48dd58b5b591f5d0f0b21913ad75302bc6edbf66135e5cf27fd0d; native build source afbb254, crate tree413eef4a2281495c911651fc6f93446d98ddbed9 identical to this head. Installed descriptor unchanged; no QA metadata rewrite, old-byte relabeling or fabricated fallback.Scope and limits
Managed/SDK/GC caller installation is intentionally unchanged in this API prerequisite and requires subsequent complete qualified layers. Existing conservative unsupported-owner fences stay in place until those consumers install. Persistent-owner production and trusted post-move task admission remain OFF. No whole-goal, frozen cohort/critic, real retained task execution or repaired official binary claim.
Darwin local runtime; existing skips are not Linux/Windows execution. Fresh code-event CI and exact-head write-access maintainer approval required independently. #6332 Linux/code-CI approval belongs to its exact6c78 head, not this helper head; no transfer.
Protected original HEAD1a12 with44staged files and reference #6240 HEAD7e5 remain untouched. Leader does not merge.