Repository navigation
refactor(session): separate retention orchestration from storage - #6282
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
probepark
left a comment
There was a problem hiding this comment.
Review (head 9d1f305, gajae-reviewer on behalf of probepark)
CI: PR cause 1 — Affected path validation / plan fails with Exact-head CI requires this PR head to contain base 100502f8f96727953856b9f57597a2d6a96f6c6d; rebase onto current dev. (job 111225482420). Confirmed locally: git merge-base --is-ancestor 100502f8 9d1f305 → exit 1; merge-base is 2a97f5f. Because plan short-circuits, evidence producer / the aggregate also fail and every affected shard (coding-agent check/types/tests, native-build) is unevaluated on this head, not passing.
Scope: +408 / -173, 13 files vs merge-base 2a97f5f — 3 commits: b7fafa1 (#6277, natives), a6ab76a (#6281, readonly settings snapshots), 9d1f305 (this PR, session retirement extraction).
Conventions: changelog.d fragments present for the settings and natives commits; none for the retirement extraction, which is reasonable for an internal, behavior-neutral move. No generated files touched by hand (packages/natives/native/diagnostic-artifact.json comes from #6277). No labels.
Notable:
packages/coding-agent/src/session/session-retirement.ts:1-155— I diffed this against the removed block ofsession-storage.ts(prior lines 3535–3677). It is a byte-identical move. The only changes are the new import block andcreateHash/randomUUID→crypto.createHash/crypto.randomUUID.SessionDeleteVerificationError,VerifiedSessionDeleteTargetandSessionStorageSnapshotremain exported fromsession-storage.ts(L402, L417, L57), so the type/value imports resolve. No import cycle:session-retirement→session-storage/session-transcript-headeronly.packages/coding-agent/src/gjc-runtime/gc-runtime.ts:33is the only remaining consumer ofprobeSessionRetirement/retireSessionTranscriptat this head (grep overpackages/), so removing the old exports without aliases breaks no caller.parseFirstJsonlLinemoved verbatim tosession-transcript-header.ts, andsession-storage.tsstill uses it at L2796/L3466.utf8Decoderstays used in storage.
Blocking:
- Stale base / stale stack prefix. The head does not contain current
dev(100502f8). #6277 was squash-merged into dev as8de425e, but this branch still carries the pre-squash commitb7fafa1, so the PR diff re-applies the natives changes (path_identity.rs,diagnostic-artifact.json, the natives changelog fragment) on top of their squashed copy. It also bundles #6281'sa6ab76a, which is still open and unreviewed. Rebase onto currentdevso the exact-head plan runs: after #6281 lands, or with #6281's commit kept as an explicit, reviewed prefix. Then re-request review. On the current tree the code itself looks approvable; this verdict is about CI evidence and the stack shape only.
PR body has no verdict line (count=0), so the body was not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:24f108341a832c5b19663d31c0caec28b6e12c3b7a5c332977a1b8c38dbefca5 reviewer:critic reviewer-id:gajae-reviewer evidence:stale-base-plan-fail;head-lacks-dev-100502f8;pre-squash-6277-commit-carried;retirement-move-byte-identical
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:24f108341a832c5b19663d31c0caec28b6e12c3b7a5c332977a1b8c38dbefca5 reviewer:critic reviewer-id:gajae-reviewer evidence:stale-base-plan-fail;head-lacks-dev-100502f8;pre-squash-6277-commit-carried;retirement-move-byte-identical
16b1330 to
98d0f1a
Compare
probepark
left a comment
There was a problem hiding this comment.
Review (head 98d0f1a, gajae-reviewer on behalf of probepark)
CI: green — 21 pass / 17 skipping, 0 pending, 0 failed at this head; approve gate ALLOW (check:@gajae-code/coding-agent covered by the head CI plan, Affected path validation green).
Scope: +399 / -169, 10 files — packages/coding-agent/src/session (retirement extraction), src/gjc-runtime/gc-runtime.ts (import), and the stacked #6281 commit ce22d61 (config/settings.ts, capability/*, discovery/helpers.ts, settings test, changelog fragment). Head contains base 8de425ed (ahead 2 / behind 0), so the stale-base blocker from my CHANGES_REQUESTED on 9d1f305 no longer applies; this review supersedes it.
Conventions: changelog fragment present for the settings change (changelog.d/task-stack-readonly-settings.md); none for the internal extraction, which the body declares behavior-neutral — fine. No generated files, no released CHANGELOG section edits.
Notable:
src/session/session-retirement.ts:1-155—planSessionRetirement/probeSessionRetirement/retireSessionTranscript/SessionRetirementOutcomeare a byte-equivalent move of the block removed fromsession-storage.ts(old lines 3534-3677); onlycreateHash/randomUUIDbecamecrypto.*.session-storage.tsstill importscreateHash, randomUUIDfor its own remaining uses (lines 622, 786, 906, ...), so no dead import.src/session/session-transcript-header.ts:4—parseFirstJsonlLinemoved verbatim;git grepat head shows the only callers aresession-storage.ts:2796,3466,session-retirement.ts, andgc-runtime.ts:33imports the retirement functions from the new module. No stale importer of the removed exports.- Note (non-blocking): this PR carries the still-open #6281 commit
ce22d61; merging #6281 first keeps the dev history matching the declared stack.
Blocking: none
Body has no gajae.pr-review-verdict.v1 line (count=0), not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:15b6ab0fc761be0db54ea285c3d46ca3d9f90ce5ab405f1124bef5ced99a5763 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;byte-equivalent-move-checked;no-stale-importers;base-current
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:15b6ab0fc761be0db54ea285c3d46ca3d9f90ce5ab405f1124bef5ced99a5763 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;byte-equivalent-move-checked;no-stale-importers;base-current
The merge-base changed after approval.
Retention owns the lifecycle transaction and must be able to depend on both storage and managed scope without introducing a storage-scope cycle. Keep the existing verified deletion behavior and share one header parser. Lore-id: 47e0b219 Constraint: no compatibility re-exports or owner activation Confidence: high Scope-risk: narrow Reversibility: easy Tested: GC disk retention session scope and storage in 196-pass focused union Tested: package vendor Biome and TypeScript checks Not-tested: owner-aware GC effects reserved for later consumer layer
98d0f1a to
8515563
Compare
probepark
left a comment
There was a problem hiding this comment.
Review (head 8515563, gajae-reviewer on behalf of probepark)
CI: green — affected-path validation, coding-agent checks, targeted session/GC tests, state gates, and virtual integration validation passed; approve gate ALLOW.
Scope: +175 / -161, 4 files — GC orchestration, session retirement, session storage, transcript-header parsing.
Conventions: CHANGELOG fragment not added (internal ownership-only extraction as documented), generated files none, labels none.
Notable:
- Checked the extracted retirement plan/probe/retire flow and its sole GC caller for preserved behavior and stale imports.
- Checked the shared JSONL header parser extraction, identity-bound deletion handoff, cleanup-pending handling, and exact-head CI coverage.
- No independent local test execution performed; verification relies on the exact-head CI evidence above.
Blocking: none
Verdict: gaja.pr-review-verdict.v1 merge-approved sha256:5ef4effec61f49c17a8c8264929e032ddd7a68d26b1389519a0409cf04f7078b reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;retirement-extraction-checked;parser-extraction-checked
Body verdict line count=0, not updated.
|
Merged into dev as
— |
Task admission stack — 03: separate retention orchestration from storage
Depends on settings/native predecessors (#6277 and #6281) in the prepared branch stack. Base stays dev. Own production added+deleted 336 lines; actual dev diff at approved settings merge
345b2782…336 lines, this layer only. Native #6277 and settings #6281 were externally approved/merged; neither preceding delta is re-applied. Extraction additions AND deletions count.Move complete existing retention plan/probe/retire orchestration to
session-retirement.ts; owning storage remains the verified-delete primitive. Extract one shared first-JSONL-line parser, update the sole direct runtime caller in GC, remove old exports without compatibility aliases. This semantic boundary avoids the storage↔managed-scope runtime cycle when owner-aware retirement arrives later.Behavior-neutral: no new owner publication, destructive capability, journal schema, task admission or legacy parsing behavior is enabled. No user-facing changelog fragment is added for this internal ownership-only extraction.
Exact-prefix verification
Owned-only conflict-free integration onto actual approved settings merge
345b27820380071509222b734f45b41b739e50d0. Neither native nor settings duplicate reapplied. Every listed prefix test/package gate freshly rerun at this current exact head. Prior head98d0f1…code-event37132359578succeeded but its review is DISMISSED, not current approval; old stale-base REQUEST_CHANGES is audited. Actual851556…code-event37135926599completed successfully. Exact-head review5401752481is APPROVED byprobepark(write permission verified). External maintainerYeachan-Heomerged that head at2026-10-03T16:56:29Z, merge commit8e709f5964987ab7dc206ca9d4b86c9e039f3803. The later prepared/rebased branch75a7abf1…was pushed during the external merge transition; it is NOT the reviewed or merged head and receives no transferred approval.Head
85155636fe40056498a99674bdb90aa338ef1568independently checked in clean detached nonnested QA with real byte-bound unchanged-native-source addon and committed strict provenance; no metadata rewrite:bun testGC disk-retention, GC session-scope and session-storage suites: exit0; full counts/output retained.git diff --checkpassed.Reference #6240 and protected44-staged original untouched. Later owner codec/access/retirement/GC/SDK layers remain separate, with production activation last. External exact-head approval and merge observed as recorded above; the leader did not merge. No approval applies to the later prepared branch or unfinished consumers.