Repository navigation
feat(sdk): validate strict task owner cleanup wire evidence - #6296
Conversation
probepark
left a comment
There was a problem hiding this comment.
Review (head feeb43a, gajae-reviewer on behalf of probepark)
CI: PR-state cause, 1 — Affected path validation / plan failed at Verify PR head contains exact base: Exact-head CI requires this PR head to contain base f4cd90d118bc864263eea99238c9c36e702a756b; rebase onto current dev. (job 111284961452). evidence producer then failed only because the plan artifact was never uploaded (Artifact not found for name: dev-affected-plan-37151087788). The aggregate is red, and no path logic ran: neither test:packages/coding-agent/test/sdk-task-artifact-owner-validation.test.ts nor check:@gajae-code/coding-agent (biome + check:types) ran at this head. gjc-state-gates/* and Local public surfaces are green.
Scope: +742 / -0, 2 files — packages/coding-agent/src/sdk/broker/task-artifact-owner-validation.ts (new, 259), packages/coding-agent/test/sdk-task-artifact-owner-validation.test.ts (new, 483, bun:test, no mock.module/spyOn). Production size matches the 259 stated in the PR body.
Conventions: changelog.d fragment none (the PR body says this is an inert internal API with no callers at this head; I accept that and am not blocking on it), generated files none, labels none, console.* none.
Notable:
- Branch state:
git merge-base --is-ancestor f4cd90d feeb43aexits 1. The head's parent is9c54c08, and basef4cd90dis a later dev merge (c401ea1+13e1221), so the two commits are siblings and neither contains the other. The diff itself is fine (the merge-base range contains exactly these 2 files), so the code review below should carry over unchanged after a rebase onto current dev (c138e28d). The exact-headbun run check/bun testexit 0 in the body is local evidence only until CI runs them at a head that contains base. task-artifact-owner-validation.ts:L226-L231+ test:L453—{ ...payloadState, taskArtifactOwnerNamespaceRetained: undefined }leaves an own key whose value isundefined, sotrueFlagthrowstask_artifact_owner_cleanup_flags_invalid(L190) before the payload/namespace agreement check is reached. The test labelled "requires disposition flags to agree in both directions" therefore never exercises that direction. Omit the key instead (e.g. destructure it out) and assert the error message. Related asymmetry:taskArtifactOwnerRetiredrequiresretirementOutcome.kind === "completed"(L224), butPayloadRetired+NamespaceRetainedwith evidence and no outcome passes (the L234 block only runs when an outcome exists). If legacy pre-outcome state is meant to be accepted, a one-line comment would help. Otherwise requireretirementOutcome?.kind === "payload_retired"the wayretireddoes.
Also verified (no issue found): parseBrokerTaskArtifactOwnerRetirementDisposition exact-key gating per kind, plus reconstruction of evidence/continuation before delegating to the merged codec parseTaskArtifactOwnerRetirementOutcome. A completed disposition sent with a continuation, or a nativeOutcome: undefined own key, fails the codec's exact-key count, so both are rejected. The session mismatch is checked against deletionEvidence.sessionId. Transcript deletion is gated to phase === "artifacts" && artifactsRemoved === true && outcome present.
Blocking: 1 — the head does not contain the immutable base, so exact-head CI (plan → affected tests + coding-agent check) never evaluated this PR. Fix by rebasing onto current dev. No code change is required for this item.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:d0187df5dfbe01b5cf7bce86857c57a0db6ff01603ba2695142e03f6caa9c5b5 reviewer:critic reviewer-id:gajae-reviewer evidence:head-not-containing-base;plan-short-circuited;tests-and-typecheck-unrun-at-head;code-read-0-defects
PR body has no verdict line (count=0), so the body was not updated.
Reconstruct strict internal owner dispositions from their compact broker DTO and reject missing or contradictory retirement flags without granting live filesystem authority. Keep public wire acceptance disabled until its complete owning effect consumer persists native outcomes safely. Lore-id: 183bc2d0 Constraint: pure DTO decoding never proves physical retirement or fresh scrub Confidence: high Scope-risk: narrow Reversibility: easy Tested: 6 pure codec boundary tests 41 assertions and targeted Biome Not-tested: exact-prefix package check pending independent qualification Not-tested: public SDK owning cleanup effect integration
Payload flags without a native-shaped disposition were accepted even though retired flags required one. Reject every cleanup claim without its evidence and outcome, and omit fields in regression fixtures so the tests reach state agreement checks rather than only the undefined-value parser guard. Lore-id: 710b64fc Constraint: decoded claims are data and never establish physical completion Confidence: high Scope-risk: narrow Reversibility: easy Tested: pure validator tests with omitted payload namespace and outcome keys Not-tested: fresh exact-prefix package and code-event CI pending
feeb43a to
f369cdd
Compare
|
Exact-base blocker repaired by owned-only rebase onto audited dev03c88bb12413241127a3024c6ba70a68e68988be. New exact head f369cdd: independent clean byte-bound-native focused tests and FULL bun --cwd=packages/coding-agent run check (vendor/Biome/TypeScript) exit0 at this exact SHA; actual dev production259/test491. Code-event37151087788 ancestry failure retained; no affected CI test/type execution credited or prior review transferred. Addressed advisory: omit namespace/payload/outcome keys rather than leave undefined, assert state_invalid; every true cleanup claim now requires original evidence AND disposition, no pre-outcome flag compatibility. New-head CI and write-access exact-head review required. |
probepark
left a comment
There was a problem hiding this comment.
Review (head f369cdd, gajae-reviewer on behalf of probepark)
CI: green — exact-head affected validation completed; Affected path validation, check:@gajae-code/coding-agent, targeted owner-validation test, Virtual integration validation, and state gates passed. gajae-approve-gate.py returned ALLOW with no pending or failed checks.
Scope: +750 / -0, 2 files — new SDK broker owner-validation production module and its Bun test coverage.
Conventions: changelog fragment 없음 (inert internal API, as documented in the PR), generated files 없음, labels 없음; no forbidden console.*, mock.module(), or uncleaned spies found.
Notable:
packages/coding-agent/src/sdk/broker/task-artifact-owner-validation.ts:54-87— exact disposition keys are checked before trusted evidence and continuation are reconstructed for the existing codec.packages/coding-agent/src/sdk/broker/task-artifact-owner-validation.ts:216-249— owner flags require matching evidence/outcome and enforce disposition agreement, including payload-retired and completed states; transcript deletion is limited to the artifacts phase after removal.packages/coding-agent/test/sdk-task-artifact-owner-validation.test.ts:448-481— the reworked omissions remove keys rather than assigningundefined, so the bidirectional agreement regression reaches the intended state check.
blocking: 없음. The prior immutable-base and missing-plan issue is resolved at this exact head; the current affected plan and package checks ran successfully.
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:2ba092e0d956d7ce910f484d81eb7d0ec9d8f55fcfeb56584b1172c884cd88de reviewer:human reviewer-id:probepark evidence:ci-green;affected-plan-green;package-check-green;owner-state-agreement-covered;code-read-0-defects
PR body verdict line count=0, so the body was not updated.
|
Merged into dev as
— |
Summary
Complete inert, independently callable pure broker owner cleanup validator. It reconstructs strict internal owner dispositions from compact wire shapes using the already-merged owner codec, binds session and profile/root context supplied by its caller, and rejects missing or contradictory evidence/outcomes/continuations and flags (including retired without a completed disposition).
This does not enable public broker DTO acceptance or session/lifecycle effects. Complete persistence/effect integration precedes those activations in later layers. Decoded DTOs never establish live native proof, deletion authority, fresh scrub, physical reclamation or writer closure.
Exact independent range
Base dev captured
03c88bb12413241127a3024c6ba70a68e68988be; headf369cdd59c0bc5910ffc3e794c4dd7122e5785c2.259 production added+deleted, both own and actual dev range; 491 test lines counted separately. Depends only on already-merged pure owner codec at runtime, native result types at compile time. No unmerged access/GC journal/storage prerequisite imported or hidden in the dev diff.
Executed exact-head verification
bun test packages/coding-agent/test/sdk-task-artifact-owner-validation.test.ts packages/coding-agent/test/task-artifact-owner-codec.test.ts packages/coding-agent/test/session-storage.test.ts— exit 0.bun --cwd=packages/coding-agent run check— exit 0 at exactlyf369cdd59c0bc5910ffc3e794c4dd7122e5785c2, vendor verification, Biome andtsc -p tsconfig.json --noEmitincluded.Independent clean tracked detached QA and genuine addon matching committed strict 0.18.5 metadata/unchanged native source; no trusted metadata rewrite. Exact before/after source clean.
13-sdk-owner-validation-base03c88-qualification.jsonrecords commands, exits and exact head; full output retained.Pure isolated validator also passed 6 tests/43 assertions. Native-shaped structural fixtures explicitly test DTO contracts only, not actual native success. Existing platform skips/warnings/info uncredited.
Exact-base CI failure and advisory correction
Prior head
feeb43a18a5ab1882c39c1481c8a177407d15e5ecode-event37151087788failed immutable-base ancestry (f4cd90dnot contained); affected test/package gates did not run in that CI. Rebased own commits onto audited current dev03c88, preserving that failure/review history and requiring new current-head CI/review.The review also identified an unexercised state-agreement branch: an own undefined flag triggered shape validation before agreement. Regression now actually omits namespace, payload and outcome keys and asserts
task_artifact_owner_cleanup_state_invalid. Tightened flags-without-outcome asymmetry: every retirement/scrub/transcript-deletion claim requires both original evidence and an outcome. Prepared evidence/continuation with no claims remains valid; no legacy pre-outcome compatibility path. Assertions retained and expanded.Retained failure history and limits
An earlier owning-prefix package check failed because the new test imported vitest (Bun runtime compatibility did not prove installed types); corrected to established bun:test. Next package run found strict Bun matcher type mismatches for structural DTO fixtures; corrected comparisons to unknown runtime values, retaining all assertions. Both failed exact-head reports archived; neither credited green. Corrected owning prefix and this genuinely independent root each passed full package gates.
No user-facing changelog for this inert internal API. Current-head code-event CI and write-access maintainer review required; no old approval transfer or leader merge. Protected44 and consolidated #6240/reference untouched. Full SDK owning effects and task-after-move activation/E2E remain separate unfinished objective obligations.