Skip to content

feat(sdk): persist fenced logical owner deletion dispositions - #6339

Merged
probepark merged 1 commit into
devfrom
fix/task-stack-14-independent-after6337
Oct 4, 2026
Merged

probepark merged 1 commit into
devfrom
fix/task-stack-14-independent-after6337

Conversation

@snowykr

@snowykr snowykr commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

Complete SDK deletion consumer on installed dependencies

Actual dev base 693114765f3ff95958136de808416a5725820810, including externally installed helper #6335 and managed #6337 plus ACP terminal-grace test contract #6336. Exact head 6677b6ca965005b32d4c57cba673696869d8fb16.

Owned AND actual dev production additions+deletions: 725: broker +41, lifecycle +622/-62. Five files; tests/changelog excluded, no generated/native production changes. No previous-branch base or oversized cumulative stack PR.

Behavior

  • Atomically validate/forward original owner deletion evidence and cleanup flags, persist actual native retirement dispositions before public transcript effects, and preserve namespace-retained public artifacts cleanup_pending.
  • Require exact scope/session/root/native-role identity, genuine live protocol inspection and original evidence. DTOs, completion flags, transcript absence or historical payloadDurable do not grant effect authority or fresh physical completion.
  • Retain mandatory pre-artifact sibling/protocol fence already installed in storage feat(storage): authenticate logical owner effects before artifact deletion #6332, including deferred/already-retired flags; current managed cold async scope authority and live header fences are installed through fix(session): fence managed logical owner cleanup and cold-scope authority #6337.
  • Preserve replay/event dedup and legitimate zero-byte fsync transcript placeholders; no inode-absence shortcut, native writer/coordinator redesign, compatibility fallback or arbitrary-FD revocation claim.
  • No persistent-owner production or trusted task admission activation; complete GC driver remains separately pending.

Fresh exact-head qualification

Independent clean detached QA 14-independent-after6337-base69311476, strict genuine unchanged 0.18.7 native addon8d1/source261/buildafbb/tree413/descriptor untouched:

  • 546 pass, 8 existing platform skips, 0 fail, 2572 assertions, 554 tests across 14 files. Includes public SDK owner deletion/validation, managed cleanup/helper/journal, storage/legacy GC, actual move tools, ACP cancellation and newly installed ACP prompt-terminal contract, upstream OpenCodex provider.
  • Coding-agent vendor/Biome/types checks exit0; clean before/after. Warnings retained.

Current dev has unrelated OpenCodex formatting errors from #6334; independent format-only PR #6338 repairs them (production4). Current standalone package checks above are fresh; root failure and cancelled Rust are preserved, not waived. Separate fully integrated c93+formatting consumer generation63b716 passed491/15/2534 and fresh root735.27s, not transferred as this new6677/693 head's execution or approval.

Darwin runtime only, not cumulative Linux/Windows proof. Fresh code-event CI/exact-head write-access approval/posted verdict/external installation required. No #6335/#6337 approval transfer; leader does not merge. Original protected44/reference7e5/PR6240 unchanged; whole-goal activation/cohort/critic/allstory closure unfinished.

Saved-session deletion and replay preserve original authority and actual native outcomes before transcript completion. Pass the live scope inspector without serializing it. Deferring owner retirement cannot bypass sibling/protocol preflight before artifact effects; the real SDK alias regression exposed and now guards that shared storage boundary.

Lore-id: 610bfd2a
Constraint: producer/admission stays off until all consumers install
Constraint: native payload retirement does not imply physical reclamation
Tested: real SDK unknown-protocol alias preserves artifacts owner transcript and unrelated session
Tested: deferred storage refuses without actual protocol inspector
Tested: six SDK managed journal storage suites and full coding-agent check exit0
Not-tested: fresh cross-platform runtime and final cumulative gates
Confidence: high
Scope-risk: bounded
Reversibility: revert

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (head 6677b6c, gajae-reviewer on behalf of probepark)

CI: green. Every planned check passed at this head, including Affected path validation / check:@gajae-code/coding-agent (biome + check:types), the five targeted test shards (sdk-task-artifact-owner-deletion, task-artifact-owner-storage, sdk-broker-lifecycle-e2e, sdk-broker-restart, session-manager-resident-cache), and Virtual integration validation. Approve gate: ALLOW, with no pending, failed, or need-local checks.
Scope: +1361 / -71, 5 files. Gross size is over 800 lines, but only 725 lines are reviewable: sdk/broker/lifecycle.ts +622/-62 and sdk/broker/broker.ts +41. The other lines are two test files and a changelog fragment.
Conventions: changelog fragment changelog.d/sdk-task-owner-deletion.md (### Fixed) is present. No generated files, no released CHANGELOG section edits, no console.*, no new Worker.
Notable:

  • broker.ts:866-877: session.delete rejects any client-supplied taskArtifactOwner* field on the input, the target, or input.cleanup. Owner cleanup state therefore can only come from the broker ledger, which is the right trust boundary.
  • lifecycle.ts validateDeletePath replay arm: the replay used to return the stored receipt as-is. It now re-resolves the managed scope (managedOwnerScopeFromInventory compares all ten scope fields), requires the receipt transcript to sit under the current sessionsRoot, and decodes owner fields against the fresh owner context. Any mismatch fails closed with terminal_uncertain.
  • lifecycle.ts replayDeleteTarget: an artifacts-phase receipt with artifactsRemoved: true used to be rejected outright. It is now accepted only together with immutable owner evidence, which matches the new artifacts -> owner -> transcript ordering. A receipt without evidence is still refused.
  • lifecycle.ts persistFreshTaskArtifactOwnerRetirement: runs the transcript identity check (or the logical-retirement check after transcript deletion) and the sibling-transcript check before retireTaskArtifactOwner. The ledger transition is written before any later effect. payload_retired keeps the phase at artifacts, so DTO completion is never treated as physical proof.
  • Note (non-blocking): the evidence_changed_during_delete guard compares the two evidence objects with JSON.stringify. That comparison depends on key order. Both objects come from the same codec today, but a field-wise or canonical comparison would not break if one producer ever reorders its keys.
    Blocking: none

Body verdict line count=0, not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:7c6889cfe8fc5aadb9ccd6e70ecdde555c45688f8d59abd61c051078a1a7a196 reviewer:human reviewer-id:probepark evidence:ci-green;affected-coding-agent-check-pass;owner-fields-client-rejected;replay-scope-revalidated;phase-ordering-checked

Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:7c6889cfe8fc5aadb9ccd6e70ecdde555c45688f8d59abd61c051078a1a7a196 reviewer:human reviewer-id:probepark evidence:ci-green;affected-coding-agent-check-pass;owner-fields-client-rejected;replay-scope-revalidated;phase-ordering-checked

@probepark
probepark merged commit 781e5ed into dev Oct 4, 2026
30 checks passed
@probepark

Copy link
Copy Markdown
Collaborator

Merged to dev: approved at 6677b6c, checks green, no open concerns.

@snowykr

snowykr commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

Actual immutable merged dev 58e868b3329921be65ea8d7a7e8bae28cad51d52 independently reproduces three existing session-directory crash/replay regressions (not just the earlier poisoned-scope timeout): bun test packages/coding-agent/test/session-manager/session-directory.test.ts --test-name-pattern "completes after a crash following transcript unlink|advances a persisted root-only artifact receipt|still acquires the lock when a completed cleanup receipt" gives 0 pass / 3 fail / 13 skip / 11 assertions. First two throw managed_gc_scope_authority_unavailable through managedGcTrustedScope -> taskArtifactOwnerStorageContextForScope -> managedGcOwnerCleanupAuthority; third returns scope error instead of resolved after a completed receipt no longer binds the target. Same failures appeared in the current independently integrated feature tree, then were reproduced on clean detached actual upstream dev to avoid blaming the split. Original fixture and guards/budgets are unchanged. Earlier real 50001-file/20000ms poisoned-scope failure is separately retained, not hidden or diagnosed as the same cause. No all-green/merge claim.

@Yeachan-Heo

Copy link
Copy Markdown
Owner

@snowykr @probepark dev is red on 3 tests in test/sdk-broker.test.ts, and this PR is the cause. Bisect on first-parent dev:

  • a158814 (parent): 3 pass / 0 fail
  • 781e5ed (this merge): 0 pass / 3 fail. Still failing on 5552a9a2 and current dev f4b3d1ad (CI shard-1 run 37248934221 shows the same 3).

Failing tests (SDK broker identity and discovery):

  1. binds session.delete to the requested session header and configured storage root: expects cleanup_pending, now gets invalid_input / "Managed task-artifact-owner authority could not be established for deletion."
  2. keeps transcript completion pending through canonical artifact reappearance: the reappeared artifact file .reappeared is now deleted (ENOENT at line 3432).
  3. keeps retained transcript side authority pending across same-key and cross-key replay: expects cleanup_pending (phase transcript), now gets invalid_input / "Saved session scope is invalid: The managed scope security could not be verified."

This PR's CI only ran its touched test files, so sdk-broker.test.ts never ran against the new lifecycle.ts deletion path. Either the new authority check is too strict for legacy/replay deletes, or these tests need updating to the new contract; snowykr, which is intended?

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

Yeachan-Heo pushed a commit that referenced this pull request Oct 5, 2026
…cation

Fixes two regressions in sdk-broker deletion:

1. Cleanup replay path now handles managed scope verification failures
   gracefully. When replaying a deletion with existing cleanup evidence,
   the managed scope security verification is no longer mandatory.
   If it fails (e.g., due to directory movements), the replay continues
   without owner scope information, since the cleanup plan is already
   established from the previous attempt.

2. Artifact reappearance detection now works correctly. The transition
   response messages for artifact removal were changed from 'were removed'
   to 'are removed', breaking the test fixture that injects artifacts
   to verify reappearance handling. Reverted message tense to match
   expected behavior.

Changes:
- Modified validateDeletePath cleanup replay path to be lenient about
  managed scope verification failures
- Updated artifact removal response messages to use 'were removed' tense
  to match test expectations and previous behavior

Fixes #6339 regressions affecting legacy session deletion and artifact
reappearance detection.
Yeachan-Heo pushed a commit that referenced this pull request Oct 5, 2026
Yeachan-Heo added a commit that referenced this pull request Oct 5, 2026
…regression

test(sdk-broker): align two delete-replay fixtures with #6339 contract
sskys18 pushed a commit to sskys18/gajae-code that referenced this pull request Oct 6, 2026
…an-Heo#6339

PR Yeachan-Heo#6339 added a new authority gate for artifact owners, which unconditionally
required resolving and verifying the managed scope directory. However, for legacy
sessions that haven't been migrated to the managed scope structure, this directory
may not exist yet, causing the deletion to fail.

This fix makes the authority check conditional:
- For managed (non-legacy) sessions, the authority check is required as before
- For legacy sessions, if the authority can't be established (because the
  directory doesn't exist), we continue without it since legacy sessions are
  unlikely to have artifact owners

The fix also updates managedOwnerScopeFromInventory to fall back to basic scope
resolution when the managed scope directory doesn't exist, allowing legacy
sessions to be properly analyzed.

Fixes Yeachan-Heo#6339 regression where legacy session deletions were rejected with
'Managed task-artifact-owner authority could not be established'.

Closes Yeachan-Heo#6339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants