fix(palette): restore composer remount for never-activated pet widgets by Yeachan-Heo · Pull Request #4605 · Yeachan-Heo/gajae-code

Yeachan-Heo · 2026-08-16T06:47:00Z

What

Fixes a live editor-lifecycle defect in selector and command-palette overlay transitions, with regression coverage that exercises disposal semantics instead of merely checking child replacement.

Container.clear() terminally disposes attached children. Before this change, selector opens and queued-message-selector opens called clear() while the reusable composer was attached; Editor.dispose() then removed the tab-width listener. Later restores re-mounted that disposed editor, so runtime tab-width changes stopped updating composer layout after the first overlay cycle.

The implementation uses the established TUI detach-then-readd contract:

detach the reusable editor before selector and queued-message-selector opens;
detach reusable plain/framed mounts before pet-aware and non-pet restores clear the shared container;
preserve 52dad458's successor-ownership guard in remountComposer() unchanged;
restore from the queued-message selector through the pet-aware restoreComposer() so an active pet keeps its PetFramedEditor mount and composer reserve after closing/deleting/moving through the queue selector.

Review round 3 fix-forward (owner lane)

The 2026-08-20 exact-head review (snowykr, 4 P1 findings) proved the open-path detach contract was asymmetric — close paths still re-added the raw editor. All findings are fixed in dfee62b9f:

Overlay close paths bypass pet-aware restoration (selector-controller.ts:3662-3670) → the jobs overlay close, tasks pane close, and OAuth manual-code submit now route through ctx.restoreComposer() (plain-editor-swap fallback for lightweight hosts), matching the showSelector close contract.
Pet-aware composer lost after sharing (command-controller.ts:205-208) → the custom-share restoreEditor closure restores through ctx.restoreComposer().
Share restoration bypasses the pet-aware composer contract (command-controller.ts:205-207) → the default-gist restoreEditor closure restores through ctx.restoreComposer() as well.
Share restoration drops the active pet frame (command-controller.ts:204-207) → both share paths above; plus the adjacent same-pattern site runtime-mcp-command-controller.ts:471-475 (MCP-add wizard done()) fixed identically.

New regression coverage (all proven red on the unfixed head, green on dfee62b9f): qa-pet-close-path-redteam.test.ts (5 tests: jobs, tasks, MCP wizard, OAuth manual-code, legacy-fallback) and 2 new cases in share-command-temp-security.test.ts (custom share, gist upload, with a fake gh).

Latest exact-head review fix-forward

The exact-head review of dfee62b9f identified a P2 compatibility gap: detachComposer() had become mandatory on InteractiveModeContext, even though ACP and lightweight test hosts intentionally omit optional lifecycle helpers. The replacement head dfee62b9f makes that method optional and retains guarded use at every controller call site; the concrete InteractiveMode implementation remains pet-aware.

Reconstruction

The original six commits were rebased onto current dev (latest: e73212bfb) with git rebase --committer-date-is-author-date, preserving contributor authorship Yeachan Heo <yeachan.heo@gmail.com> and original author dates. The fix-forward commit carries Co-authored-by: snowykr for the review findings. The source delta against dev is unchanged from the previously reviewed scope plus the close-path fix-forward; intervening dev commits touch only packages/ai, with zero file overlap.

Exact evidence

Head: dfee62b9f4af5e61513371f5600b7ba03e11af16
Base: dev 79fe412259c7e27c37de699cc5edf0c64bd82864 (head contains base; git merge-base --is-ancestor verified)
Binary diff digest: sha256:969e0d4b01a592b059acc3ae2c432ca1612128cb53b2044a7deabf48f79da73a from git diff --binary --full-index --no-ext-diff 79fe412259...dfee62b9f4
Local exact-head validation on dfee62b9f: 156 tests green across the 6 core suites (qa-pet-close-path-redteam 5, share-command-temp-security 6, command-palette-interactive-host 9, gajae-pet-widget 63, input-controller-keybindings 67, qa-pet-restore-redteam 6); 321 more green across the remaining PR-touched selector/controller/onboarding suites and adjacent pet/input suites (178 + 117 + 148 in three runs); bun --cwd=packages/coding-agent run check clean (biome + tsc); repository fast gate (bun scripts/verify-gjc-state-writers.ts --fail) PASS.
Current exact-head CI evidence: Dev CI run 32509577137 has the affected-path plan, evidence producer, coding-agent check, native build, CLI smoke, all 17 affected tests, and state gates passing; the only failing current-head checks are PR contract bootstrap and Validate exact-head PR contract in 32509577137/32509571550, both intentionally rejecting the needs-human verdict until independent approval.
Prior/superseded runs (not current evidence): Dev CI 32347673567 on a578085809 (superseded by the replacement head), 32185025067 (3196de229c), 32181321301 (6e5670266b), 32154701523 (6d12e769e6), 32099895353 (eef82ef5dc), cancelled 32099837032/32181054337, contract-red 32099835720/32099895625/32181321780/32497550441 (superseded by the replacement; needs-human remained correct pending independent approval).

Risk classification

low-risk — ordinary fix/maintenance; the repository owner may use the explicit merge-self-approved solo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.
regression-risk — fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-head APPROVED review the gate verifies (extra:independent:<login>; the token alone never suffices).
high-risk — large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-head APPROVED review (extra:independent:<login>).

Review state

Prior CHANGES_REQUESTED reviews by @snowykr are addressed head-on through dfee62b9f; the latest P2 compatibility finding is fixed by making InteractiveModeContext.detachComposer optional while keeping every controller call guarded. Owner: @Yeachan-Heo. The merge blocker is a fresh non-author exact-head APPROVED review from @snowykr (write permission verified) on dfee62b9f.

GJC verdict

gajae.pr-review-verdict.v1 needs-human sha256:969e0d4b01a592b059acc3ae2c432ca1612128cb53b2044a7deabf48f79da73a reviewer:human reviewer-id:pending evidence:replacement-dfee62b9f-fixes-latest-exact-head-compatibility-review-independent-exact-head-review-required

Target branch is dev
bun check passes
Tested locally
CHANGELOG updated (if user-facing)
Verdict above matches the exact PR head, not an earlier commit
Risk classification above matches the actual review path taken

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

chatgpt-codex-connector · 2026-08-16T06:47:05Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

Yeachan-Heo · 2026-08-16T06:55:46Z

Root cause

InteractiveMode.restoreComposer() routes through petWidget.remountComposer() whenever a pet widget exists — which is always after init(), even with pet.mode: "off". #4591 gated remountComposer() behind #canMutateSharedUi(), which admits only (a) the current overlay-emitter owner or (b) a previously-activated widget whose ownership epoch still matches. A widget that never activated (pet off at startup: #ownedOverlayEpoch === 0, never registered in petOverlayEmitterOwners) satisfies neither clause, so remountComposer() became a silent no-op and the palette's CommandPaletteComponent stayed mounted in editorContainer — the exact leak in the three failing tests (palette cancel path, active-command guard path, rejected-extension recovery path).

Pre-#4591, remountComposer() mounted unconditionally; for a never-activated widget #mountEditor(false) is byte-identical to the no-pet fallback (editorContainer.clear(); addChild(editor)). The fix separates composer-mount authority from overlay ownership: a never-activated widget still owns its host's composer mount and must remount the plain editor; only disposal or a live successor widget revokes the mount. #4591's successor-takeover isolation is untouched (still covered by the existing "ignores stale off and remount calls after successor takeover" test), and #canMutateSharedUi() gating in dispose()/#applyMode() is unchanged.

Validation (exact PR head `c194e79`)

Gate	Result
`bun test packages/coding-agent/test/command-palette-interactive-host.test.ts`	8 pass / 0 fail (was 3 fail at dev head `e1849e6`)
`bun test packages/coding-agent/test/gajae-pet-widget.test.ts`	62 pass / 0 fail (incl. 2 new regression tests)
`bun test packages/tui/test/gajae-pet.test.ts`	19 pass / 0 fail
`bun run --cwd packages/tui check`	clean (biome + tsc)
`bun run --cwd packages/coding-agent check`	clean
Adjacent suites (selector-controller-command-palette, qa-pet-restore-redteam, interactive-mode-editor-component, hook-editor, model-selector ×2)	104 pass / 0 fail

CI on this head (run 31932247745 + 31932247752)

Green: affected-path plan, native-build, test:packages/coding-agent/test/gajae-pet-widget.test.ts, ts-build (coding-agent), gjc-state-gates ×4, Local public surfaces, Virtual integration validation.

Red: PR contract bootstrap — the PR body lacks a gajae.pr-review-verdict.v1 merge-approved verdict line from an independent reviewer with an exact-head approval (governance gate; needs a maintainer review, not a code change). Bounded blocker report filed on #4604.

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

Yeachan-Heo · 2026-08-17T16:11:36Z

Status: rebase complete, exact-head CI driven, human-review boundary reached.

Head: b5929c80eca5074998e3875c0b13a7a67076d9d1 (force-pushed with lease; old head c194e79453 was stale/DIRTY against new dev)
Base: dev 8711c555da (was e1849e676b; dev moved and contains 52dad458, the repo owner's independent fix for the same root cause)
Semantic conflict resolution: this PR's original remountComposer() implementation was dropped in favor of dev's shipped one (owner === undefined || owner === this) — byte-identical semantics for both protected properties (never-activated remount, successor ownership). The branch's surviving delta is exactly the two regression tests dev's fix shipped without (+55 lines, test file only).
Validation on exact head: command-palette-interactive-host 8/8, gajae-pet-widget 62 pass (incl. 2 new), tui gajae-pet 19 pass, --cwd packages/tui check and --cwd packages/coding-agent check clean.
CI on exact head: plan, gjc-state-gates ×4, Local public surfaces green; PR-contract checks now parse the verdict line correctly — the only red is intentional: needs-human blocks merge until an independent review arrives.
Review request: @snowykr (documented maintainer, dev 7ca1d66a60) — please review head b5929c80ec and approve. After approval the verdict line in the PR body must be flipped to merge-approved with digest 11193ebfd7e38991253bb7bb859fff515ae7caa34af61897a83ff9a5a8794505 (exact 8711c555da...b5929c80ec diff) and reviewer-id:snowykr; I will then squash-merge and reconcile the issue.

No code or behavior delta vs dev; this PR only adds the regression coverage that would have caught #4591's leak before it shipped.

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

Yeachan-Heo · 2026-08-17T16:24:40Z

Blocked solely on human review — full state for the reviewer

Head: b5929c80eca5074998e3875c0b13a7a67076d9d1 · Base: dev 8711c555da490ad36475f94368bcdd8d8f01048e · Owner branch: fix/issue-4604-command-palette-pet-restore (force-pushed with lease)
Diff vs dev: one file, packages/coding-agent/test/gajae-pet-widget.test.ts +55 lines — two regression tests. Zero source delta: the root-cause fix itself is already on dev as 52dad458; this PR only adds the coverage it shipped without.
Tests pin: (1) never-activated widget + foreign overlay in editorContainer → remountComposer() restores [editor] (the exact fix(dev): command-palette-interactive-host failures from #4591 pet integration #4604 leak); (2) active widget → restores [framedEditor]. Successor-ownership protection remains pinned by the pre-existing takeover test.
Local reproduction: on dev 8711c555da before 52dad458, command-palette-interactive-host was 3-fail; with the fix plus this coverage it is 8/8, gajae-pet-widget 62 pass, tui gajae-pet 19 pass, tui + coding-agent check clean.
CI on exact head (run 32044402879): all code jobs green — plan, native-build, test:gajae-pet-widget, ts-build, evidence producer, Affected path validation, Virtual integration validation, gjc-state-gates ×4. (Run 32044088151 was concurrency-cancelled mid-flight when the body edit superseded it; its "native build" failure is the cancellation, not a code failure.)
Blocker: PR contract bootstrap / Validate exact-head PR contract intentionally block on the recorded needs-human verdict until an independent non-author review exists. Reviewer must hold admin/maintain/write. @snowykr is requested (documented maintainer, dev 7ca1d66a60).
After approval: flip the verdict line in the PR body to
gajae.pr-review-verdict.v1 merge-approved sha256:11193ebfd7e38991253bb7bb859fff515ae7caa34af61897a83ff9a5a8794505 reviewer:human reviewer-id:<reviewer-login> evidence:<review url>, rerun PR contract, then squash-merge to dev. Issue fix(dev): command-palette-interactive-host failures from #4591 pet integration #4604 is already closed; no further issue reconciliation needed.
Terminal disposition: hold at this boundary. No speculative merge, no self-approval. The PR is a pure test-addition on top of an already-shipped fix — lowest-risk possible merge for dev.

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

snowykr

Verdict: Request changes

gajae.pr-review-verdict.v1 request-changes sha256:1465ccdfd915bdfdc6b32a6431efa134faeecd6346a209b4ab49cd99aee45faf reviewer:human reviewer-id:snowykr evidence:https://github.com/Yeachan-Heo/gajae-code/actions/runs/32044402879

Reviewed the exact PR head b5929c80eca5074998e3875c0b13a7a67076d9d1 against dev 8711c555da across intent/contract, architecture/correctness, security/trust, verification/CI, and compatibility/platform context.

The regression-test direction is correct, and I found no actionable P1, P3, or P5 issue. The two new assertions cover the ownerless/off → plain-editor and active-owner → framed-editor remount branches, while the existing successor-takeover test preserves the stale-owner contract.

Required changes

1. Make the remount tests model the reusable editor lifecycle

Location: packages/coding-agent/test/gajae-pet-widget.test.ts:650-701

makeStubs() supplies an editor cast to CustomEditor, but the stub has no dispose() implementation. The production #mountEditor() uses editorContainer.clear(), and Container.clear() disposes every current child. CustomEditor.dispose() is not a no-op: it tears down the tab-width listener and resets pending paste state. The new tests therefore prove only child replacement; they can pass even if a repeated palette close/remount path has disposed the reusable editor.

Please either use a real/disposable editor in this regression or add a focused lifecycle assertion that repeats the restore path and verifies the editor remains usable (including the relevant disposal/listener contract). If the intended ownership model is detach-then-readd, the implementation/test should make that explicit rather than relying on a stub that omits disposal.

2. Refresh the exact-head evidence in the PR contract

The PR body’s verdict still points to https://github.com/Yeachan-Heo/gajae-code/actions/runs/32044088151, but that is the older/cancelled run and gh pr checks 4605 still reports its affected-path job as failed. The successful exact-head product run is 32044402879; its targeted gajae-pet-widget test, ts-build, native-build, affected-path plan/evidence, and integration jobs pass, while the contract jobs fail because the body still declares needs-human.

Please update the verdict evidence to the immutable successful exact-head run (or explicitly label the old URL historical), then rerun the exact-head contract gate after the review decision is recorded.

Review lane summary

P1 Intent / Policy / Contract: clear.
P2 Architecture / Correctness / Failure: changes requested for lifecycle/test fidelity; active-pet InteractiveMode integration remains a useful follow-up gap.
P3 Security / Privacy / Trust: not materially applicable; test-only change with no external or security boundary.
P4 Verification / Tests / CI: targeted changed-test CI is green, but neighboring host/TUI suites are local-only claims and the body evidence URL is stale.
P5 Context / Compatibility / Platform: clear; no generated artifacts, runtime/package changes, or changelog required for this test-only diff.

Yeachan-Heo · 2026-08-17T17:26:21Z

Exact-head CHANGES_REQUESTED at b5929c80eca5074998e3875c0b13a7a67076d9d1 is assigned to the live fix-forward lane.

Owned blockers:

Replace the child-replacement-only editor stub coverage with a real/disposable repeated restore/remount lifecycle proof, or fix production to detach/readd safely, so Container.clear() cannot silently dispose the reusable CustomEditor and its listener/paste state.
Replace the stale verdict evidence URL for cancelled run 32044088151; current successful product evidence is 32044402879, and a code push will require a new exact-head run again.

The lane will implement, push, rerun exact-head CI, obtain a fresh approval/verdict, and drive the PR to dev merge. This is not a human-only hold.

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

Yeachan-Heo · 2026-08-17T19:14:56Z

Requested changes addressed — head a890974ebb ready for re-review

Both required changes from review 4952815267 are implemented, tested red/green, and pushed to the authorized branch (lease-guarded force push from b5929c80ec).

1. Remount tests now model the reusable editor lifecycle. The stub editor was replaced with a real CustomEditor in a real Container for all three lifecycle tests. The core probe: Editor.dispose() tears down the tab-width change listener, and that listener fires editor.invalidate() on a runtime tab-width change — so counting invalidations across overlay cycles proves disposal never happened:

New widget test: 4 production-shaped open (detach → clear → overlay) / close (remountComposer) cycles; every cycle's tab-width toggle reaches the live listener (8 invalidations); a red control at the end proves the probe (a genuinely disposed editor accrues zero further invalidations).
New end-to-end host test (command-palette-interactive-host): repeated palette open/close through the real editor key path; each cycle's tab-width toggles still reach the live listener and the restored composer accepts input.
Red/green proof: reverting only the source fix makes the host test fail (8 pass / 1 fail); with the fix, 9 pass / 0 fail.

The review also exposed a real production defect, now fixed. showSelector opened overlays with editorContainer.clear() while the live composer was attached — and Container.clear() disposes children terminally. The first palette round-trip silently killed the editor's tab-width listener; every restore re-mounted a dead editor. Fix: overlay open paths detach the reusable editor first (Container.detachChild, the tui detach-then-readd reuse contract — the same pattern the hook selectors already use), and pet-aware restores detach both reusable mounts before clearing. Successor-ownership isolation is untouched (remountComposer is byte-identical with dev's 52dad458; the takeover test still pins it).

2. Verdict evidence refreshed to the immutable exact-head run. Exact-head Dev CI run 32058210086: 17 code jobs green — native-build, test:gajae-pet-widget, ts-build, affected-path plan + evidence producer, Virtual integration validation, gjc-state-gates ×4, Local public surfaces. The only red is the PR contract gate failing intentionally on the recorded needs-human verdict (blocked-report contract: no self-approval). Digest d97e9e79…cf2327 matches the exact 8711c555da...a890974ebb diff.

Validation on a890974ebb: command-palette-interactive-host 9/9, gajae-pet-widget 63/63, tui gajae-pet 19/19, 264 pass across 12 adjacent suites, tui + coding-agent check clean, coding-agent build clean.

Follow-up gap (separate issue): share/gist loader, MCP wizard, and tasks-pane paths still use the raw clear() pattern.

@snowykr — ready for a fresh review of a890974ebb.

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

Yeachan-Heo · 2026-08-17T19:19:01Z

Terminal disposition: blocked solely on fresh human review of a890974ebb

Exact head: a890974ebb46d9dcb4c75beb5f6f166e3854c2df · Base: dev 8711c555da · Branch: fix/issue-4604-command-palette-pet-restore (lease-guarded force push; no further mutations pending)
CI on exact head: Dev CI run 32058210086 — 17/17 code jobs green (native-build, test:gajae-pet-widget, ts-build, affected-path plan + evidence producer, Virtual integration validation, gjc-state-gates ×4, Local public surfaces). Sole red is the PR contract gate failing by design on the recorded needs-human verdict. Evidence URL in the PR body now points at this exact-head run (stale 32044088151 reference replaced).
Review state: snowykr's CHANGES_REQUESTED (4952815267) applied to the previous head b5929c80ec. Both required changes are implemented on a890974ebb: (1) real-editor lifecycle regression with red/green proof — reverting only the source fix fails the new host test, with it 9/9; the review additionally exposed a production defect (overlay open disposed the live composer via Container.clear()), fixed via the tui detach-then-readd contract; (2) body evidence refreshed to the immutable exact-head run. Fresh re-review requested from snowykr.
Local reproduction (dev base): widget suite 63/63; palette host suite 9/9; 264 pass across 12 adjacent suites; tui pet 19/19; --cwd packages/tui + --cwd packages/coding-agent checks clean; coding-agent build clean.
Owner: gaebal-gajae (clawdbot) lane under Yeachan-Heo auth; blocked-report contract forbids self-approval, so the merge-approved verdict cannot be issued from this side.
Blocker: one authenticated APPROVED review of a890974ebb by a non-author maintainer (snowykr re-requested). On approval: flip the body verdict to merge-approved sha256:d97e9e7944dbde6af6530a4c989097d6d01eef4352b707c8b9e554f1d3cf2327 reviewer:human reviewer-id:<approver> evidence:<run 32058210086>, rerun the PR-contract check, then squash-merge to dev and reconcile issue fix(dev): command-palette-interactive-host failures from #4591 pet integration #4604 (already closed; no further action needed there).

Lane holds here; no speculative merge.

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

Yeachan-Heo · 2026-08-17T19:36:02Z

Terminal disposition: blocked solely on fresh human review of cce1441cbe (supersedes the same disposition at a890974ebb after the dev rebase)

Exact head: cce1441cbe23d941e90b74b6c65ed42064cfed97 · Base: dev 6696988b6dd757f898ab7b33984ba830b6344109 · Branch: fix/issue-4604-command-palette-pet-restore (rebased from a890974ebb after fix(omlx): keep role thinking effort on discovered models #4631/fix(utils): recognize exported dotenv assignments #4613 landed; only conflict was the CHANGELOG Unreleased section, resolved by keeping both entries — no merged behavior and no fix(palette): restore composer remount for never-activated pet widgets #4605 fix content dropped; git diff --check clean)
Exact-head CI (run 32060135954, final): 19 product jobs green — affected-path plan, native-build, test:gajae-pet-widget, test:command-palette-interactive-host, test:selector-controller-command-palette, test:input-controller-keybindings, test:model-selector-* ×2, check:@gajae-code/coding-agent, ts-build, cli-smoke, evidence producer, Virtual integration validation, gjc-state-gates ×4, Local public surfaces. Sole red: PR contract bootstrap, failing by design on the recorded needs-human verdict (Verdict needs-human intentionally blocks merge). No product failure to classify.
Local verification on the rebased head: command-palette-interactive-host + gajae-pet-widget 72/72; 197 pass across 11 adjacent suites (incl. omlx-discovery from the newly merged fix(omlx): keep role thinking effort on discovered models #4631); tui gajae-pet 19/19; --cwd packages/tui and --cwd packages/coding-agent checks clean; git diff --check clean.
Review state: snowykr's CHANGES_REQUESTED (4952815267) still targets the pre-rebase head; both requested changes remain implemented. Fresh re-review of cce1441cbe requested; PR body evidence and digest (46049afc…06c4a, exact 6696988b6d...cce1441cbe diff) refreshed to this head.
Blocker (sole): one authenticated APPROVED review of cce1441cbe by a non-author maintainer. On approval: flip the body verdict to merge-approved sha256:46049afcda5f9b6f2c53b1e41054d1e8b54cad93191d28a24afe940778f06c4a reviewer:human reviewer-id:<approver> evidence:<run 32060135954>, rerun the PR-contract check, squash-merge to dev, reconcile fix(dev): command-palette-interactive-host failures from #4591 pet integration #4604 (already closed).

No mutations while review is absent: no self-approval, no verdict change, no source/body edits. Lane returns to external review hold.

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

Yeachan-Heo · 2026-08-17T21:57:59Z

@snowykr Fresh non-author review requested for exact head 097c3b4a1b4c30910bb00548dde89b74877a936a against dev 6696988b6dd757f898ab7b33984ba830b6344109.

The prior CHANGES_REQUESTED is stale (b5929c80ec) and is not counted. Its lifecycle-proof finding is fixed forward: the real-CustomEditor regression now proves repeated overlay transitions preserve the tab-width listener and editor usability. Local exact-head evidence is 63 pass / 0 fail for gajae-pet-widget, clean coding-agent check, and clean diff check. Binary diff digest: sha256:011a3260f7310924e4956d92da7ce6cb059f149339ac7d98fe86d6656113df34.

Current exact-head CI is running. The only expected governance blocker after product CI is a fresh independent approval and matching merge-approved verdict.

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

Yeachan-Heo · 2026-08-18T04:39:01Z

Reconstruction evidence — PR rebased onto current dev, fresh exact-head review requested

Old base → new base: dev 6696988b6dd757f898ab7b33984ba830b6344109 → 27afb732b3d25632d44176687d5bdd78d3419bb3 (14 commits; none touch this PR's source files).
Old head → new head: 097c3b4a1b4c30910bb00548dde89b74877a936a → eef82ef5dc5a620f847312057d3ea1eab7bbe55d, pushed with --force-with-lease to the authorized branch fix/issue-4604-command-palette-pet-restore (lease pinned to the old head).
Replay method: git rebase --committer-date-is-author-date 27afb732b3 097c3b4a1b — all three accepted commits (d162301d08, cce1441cbe, 097c3b4a1b) rebased cleanly; contributor authorship Yeachan Heo <yeachan.heo@gmail.com> and original author dates preserved; only the CHANGELOG.md Unreleased hunk context moved. Source+test delta vs the new base is byte-identical to the accepted PR delta vs the old base (verified by diff comparison); zero unrelated drift entered the branch.
Canonical digest: sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713 = sha256(git diff --binary --full-index --no-ext-diff 27afb732b3d25632d44176687d5bdd78d3419bb3 eef82ef5dc5a620f847312057d3ea1eab7bbe55d).
Local exact-head validation on eef82ef5dc: focused suites 172 pass / 0 fail across gajae-pet-widget (63, incl. the three lifecycle tests), command-palette-interactive-host, selector-controller-command-palette, input-controller-keybindings, model-selector-controller-batch, model-selector-profiles; bun run --cwd=packages/coding-agent check clean; bun run --cwd=packages/coding-agent build exit 0; git diff --check clean; check:schemas + check:plugins in sync.
Exact-head CI: Dev CI run 32099837032 and PR-contract run 32099835720 are running on eef82ef5dc. The PR-contract needs-human failure is expected and by design until the fresh review lands.
Verdict line: the PR body carries exactly one current gajae.pr-review-verdict.v1 needs-human line bound to the digest above.

Review request: @snowykr — fresh authorized non-author review of exact head eef82ef5dc5a620f847312057d3ea1eab7bbe55d against dev 27afb732b3d25632d44176687d5bdd78d3419bb3, please. The prior CHANGES_REQUESTED (review 4952815267) targeted b5929c80ec; its lifecycle-proof request was fixed forward in cce1441cbe with real-CustomEditor disposal-probe tests, so it is stale. After product CI is green, a fresh merge-approved verdict bound to sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713 unblocks merge.

Signed: owner lane for PR #4605, head eef82ef5dc5a620f847312057d3ea1eab7bbe55d, base 27afb732b3d25632d44176687d5bdd78d3419bb3, digest sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713, pushed 2026-08-18 via force-with-lease.

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

Yeachan-Heo · 2026-08-18T05:57:58Z

Owner-lane boundary review complete — head eef82ef5dc stands as-is; review remains the sole blocker

The owner lane has finished its adversarial boundary review of exact head eef82ef5dc5a620f847312057d3ea1eab7bbe55d vs dev 27afb732b3d25632d44176687d5bdd78d3419bb3 (digest sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713):

All 5 changed sites verified correct: detach-then-readd is the documented Container contract; every restore pairs re-add with setFocus; drafts, paste state, and the tab-width listener provably survive overlay round-trips; no double-mount/double-dispose; the framed-editor hypothesis was investigated and disproven (PetFramedEditor has no dispose, clear() calls child.dispose?.()).
Regression tests verified non-vacuous by independent red-team runs: the 4-cycle probe asserts exactly 8 invalidations with an explicit red control proving the counter detects the old bug; the host test drives a real InteractiveMode through the editor's real key handler; full focused surface 172 pass / 0 fail (JUnit: 172 tests, 0 failures, 0 errors).
Exact-head CI (run 32099895353): every product job green (ts-build, cli-smoke, native-build, all 5 affected test suites, affected-path plan + evidence, Virtual integration, gjc-state-gates ×4). The only red job is PR contract bootstrap, failing exactly as designed: Verdict needs-human intentionally blocks merge.

Advisory findings (not merge blockers, recorded for follow-up): the same terminal-disposal defect remains reachable at four pre-existing unguarded open paths — jobs overlay (alt+j, /monitors — reachable from this PR's own palette), tasks pane (alt+t), OAuth API-key paste login (~20 providers), and /debug viewers. None is a regression from this PR; all predate it on the base commit. They belong in a separate issue/PR, not scope-crept into this one.

@snowykr — the review request from the reconstruction comment still stands: fresh non-author review of exact head eef82ef5dc, base 27afb732b3. The prior CHANGES_REQUESTED (review 4952815267) targeted b5929c80ec and its lifecycle-proof request was fixed forward in cce1441cbe with real-CustomEditor disposal-probe tests, so it is stale. A fresh merge-approved verdict bound to sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713 unblocks merge; the owner lane will then merge to dev, dogfood fresh dev with bun run build, and reconcile post-merge CI.

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

snowykr

Verdict: Request changes

gajae.pr-review-verdict.v1 request-changes sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713 reviewer:human reviewer-id:snowykr evidence:https://github.com/Yeachan-Heo/gajae-code/actions/runs/32099895353

Reviewed the new exact head eef82ef5dc5a620f847312057d3ea1eab7bbe55d against dev 27afb732b3d25632d44176687d5bdd78d3419bb3 across the requested P1–P5 axes, including the detach-before-clear production changes and the new lifecycle tests.

The production direction is sound: detaching the reusable editor before Container.clear() preserves the editor's terminally-owned lifecycle, and the exact-head affected-path jobs for the changed suites are green. I found one blocking verification/compatibility issue before approval.

Required change

Update all SelectorController test contexts for the new `detachChild` requirement

Locations:

packages/coding-agent/test/custom-model-preset-creation.test.ts:854-858
packages/coding-agent/test/model-selector-profiles-redteam.test.ts:127-127,356-356
packages/coding-agent/test/provider-onboarding-wizard.test.ts:263-269
packages/coding-agent/test/provider-onboarding-wizard-redteam.test.ts:320-327

These contexts instantiate SelectorController and exercise selector-opening paths such as showModelSelector() / showCustomProviderWizard(), which all enter SelectorController.showSelector(). The new implementation now unconditionally calls this.ctx.editorContainer.detachChild(this.ctx.editor) before clear(), but these test doubles still provide only clear() and addChild(). Because they are cast to InteractiveModeContext/never, TypeScript does not catch the missing method; the affected tests will fail at runtime with detachChild is not a function when the selector opens.

Please update every lightweight editorContainer double that reaches SelectorController.showSelector() to implement detachChild with the same child-list semantics as the existing updated fixtures. A repository-wide audit should include the session-selector and login-selector test contexts as well, rather than updating only the changed-path suites.

Verification notes

P1 Intent / Contract: clear; the detach-then-readd change matches the stated lifecycle contract.
P2 Architecture / Correctness: no production blocker found in the reviewed call sites; PetFramedEditor is not disposable, and the reusable CustomEditor remains owned by InteractiveMode.
P3 Security / Privacy / Trust: not materially applicable; no new external or sensitive-data boundary.
P4 Verification / CI: the new lifecycle tests are meaningful, but current CI targets only the changed suites and therefore does not catch the unupdated selector test doubles above.
P5 Compatibility / Platform: runtime API usage is consistent with Container; test fixtures need to be brought to the same contract. The changelog entry is appropriate for the user-visible lifecycle fix.

The current exact-head product run is 32099895353; its relevant affected-path jobs pass. The contract jobs remain red by design while the PR body records needs-human.

snowykr

Verdict: Request changes

gajae.pr-review-verdict.v1 request-changes sha256:59e7bd2abe687245725790324531d00da64e9f77f6e83f31ad47bb6e07680713 reviewer:human reviewer-id:snowykr evidence:https://github.com/Yeachan-Heo/gajae-code/actions/runs/32099895353

Follow-up review of the same exact head eef82ef5dc5a620f847312057d3ea1eab7bbe55d found two additional blocking gaps in the fix-forward. The detach lifecycle direction is correct, but the queued-message path and the test/evidence contract are not complete yet.

Required changes

1. Restore the pet-aware composer from the queued-message selector

Location: packages/coding-agent/src/modes/controllers/input-controller.ts:1249-1255

#restoreEditorFocus() now detaches the raw editor and re-adds that raw editor directly. In an active pet session, the mounted child is PetFramedEditor, not ctx.editor; the detach is therefore a no-op, clear() removes the framed wrapper, and the close path mounts the bare editor. The pet remains active and continues emitting its sprite, but the composer reserve/frame is gone after closing or deleting/moving through the queued-message selector.

This is deterministic for the active-pet queue flow and is exactly the kind of reusable-composer contract this PR is fixing. Route the restore through ctx.restoreComposer() (or an equivalent pet-aware mount helper) so the active mode restores PetFramedEditor and off mode restores the plain editor. Add an active-pet queued-selector regression that asserts the framed child and reserve survive open/close.

2. Update every selector test double that now needs `detachChild`

The first follow-up only updated the changed-path fixtures. Several existing contexts still instantiate SelectorController and enter showSelector(), but provide only clear() and addChild(). Since showSelector() now calls detachChild() unconditionally, these tests can fail at runtime despite being cast through InteractiveModeContext/never:

packages/coding-agent/test/custom-model-preset-creation.test.ts:854-858
packages/coding-agent/test/model-selector-profiles-redteam.test.ts:127,356
packages/coding-agent/test/provider-onboarding-wizard.test.ts:263-269
packages/coding-agent/test/provider-onboarding-wizard-redteam.test.ts:320-327
packages/coding-agent/test/modes/components/theme-selector-input.test.ts selector contexts
packages/coding-agent/test/modes/components/thinking-selector.test.ts selector contexts
packages/coding-agent/test/selector-controller-resume-model.test.ts:11-15
packages/coding-agent/test/selector-controller-session-delete.test.ts:11-15 and 49-57

Please audit all SelectorController fixtures that reach showSelector() and add a child-list-correct detachChild implementation, not just the affected-path files. The current targeted CI does not execute all of these suites.

3. Refresh the exact-head evidence references

The PR body still identifies 32099837032 / 32099835720 as the current exact-head evidence. gh reports 32099837032 cancelled and 32099835720 failed. The completed current-head product run is 32099895353; its product/affected jobs pass, while 32099895625 is the separate contract run that fails only because the body still records needs-human.

Update the body’s exact-evidence section and verdict URL to the completed run, and describe the contract failure as the expected human-review gate. Do not leave cancelled/failed runs presented as current evidence.

Additional verification note

The new real-editor widget lifecycle tests at packages/coding-agent/test/gajae-pet-widget.test.ts:671-702 and the preceding active/off tests should explicitly dispose the CustomEditor fixture after each test. widget.dispose() does not own the editor, and CustomEditor registers a process-wide tab-width listener; the first two real-editor tests can leak that listener into later tests. The repeated lifecycle test’s red control disposes it intentionally, but the other real-editor fixtures need equivalent cleanup.

The existing raw editorContainer.clear() swaps outside SelectorController.showSelector() (OAuth code input, jobs/tasks panes, command-controller loaders, and runtime-MCP wizard) also deserve a lifecycle audit before claiming the reusable composer contract is complete; I am treating those as follow-up scope unless they are intended to be covered by this PR’s broad selector/overlay fix.

Lane summary

P1 Intent / Contract: changes requested for queued pet-aware restore and incomplete fixture contract.
P2 Architecture / Correctness: active-pet queued selector loses PetFramedEditor deterministically.
P3 Security / Privacy / Trust: no actionable finding; not materially applicable.
P4 Verification / CI: lifecycle coverage is improved, but unupdated fixtures and stale evidence remain.
P5 Compatibility / Platform: runtime Container usage is consistent; all lightweight contexts must match the new method contract.

…y opens Issue #4657. Four overlay-open paths still cleared editorContainer with the live composer attached: the jobs overlay (alt+j //monitors), the tasks pane (alt+t), the OAuth API-key paste input, and the /debug log + raw-SSE viewers. Container.clear() disposes children terminally and Editor.dispose() tears down the tab-width change listener, so the first round-trip on any of these paths silently killed the composer's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Each open now detaches the reusable editor first (Container.detachChild, the same detach-then-readd reuse contract showSelector and the extension-ui and pet-aware restore paths already follow from PR #4605), so the terminal clear disposes only the transient overlay. User-facing behavior is unchanged: the overlay mounts exactly as before and the composer is re-added on close. Regression coverage: one deterministic test per named path in composer-detach-overlay-paths.test.ts driving the real production open/close code (real JobsOverlayComponent/TasksPaneComponent focus lists, the captured production onPrompt closure over a real Input, and the real DebugSelector selection into the real DebugLogViewerComponent with an isolated agent-dir log source), each asserting the tab-width listener still fires across four overlay cycles, with a red control proving the probe detects genuine disposal. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes untouched (showSelector/extension-ui/pet restore shapes unchanged) Tested: red/green — 4/4 fail on unfixed tree, 4/4 pass with fix Tested: 107 pass across 9 adjacent suites (pet widget, editor dispose, tasks, jobs, debug, login) Tested: 190 pass across 7 selector/palette/input suites Tested: bun run --cwd=packages/coding-agent check + build clean; ci:test:smoke ok Tested: check:tools, node20-baseline, public-sync, schemas, docker-context, gjc-ui gates pass Confidence: high Scope-risk: narrow Reversibility: trivial

Yeachan-Heo · 2026-08-18T15:34:14Z

Addressed in full — fixture audit complete (review 4958123531)

Every SelectorController test context that reaches showSelector() now implements detachChild with child-list-correct semantics; the audit went beyond the four files you flagged to also cover theme-selector-input, thinking-selector, selector-controller-resume-model, selector-controller-session-delete, and the login-selector context in login-preset-recommendation. Contexts already using real Containers (credential-auto-import-flows, interactive-mode-*) or never reaching showSelector() (g007-ws5-redteam, sessions-dashboard, transcript-viewer-overlay, resume-progress-lease, resume-session-reentrancy*) needed no change. The follow-up review 4958165282 additionally found and fixed the queued-message pet-aware restore gap and the real-editor fixture disposal leak; both are resolved in the same head. All named suites re-run green locally. Details in the replies on review 4958165282.

New exact head for re-review: 6d12e769e65f174a4615b87da9aea5d9b1b1318d against dev ceb31349c2d024c75825c819cb530d5060f49380, digest sha256:4221be9bfe1f9ffad63811decb0be31cc43369d59e3ad8b607522d28dee52eba, exact-head CI run 32154050225.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Addressed — pet-aware queued-message restore + regression coverage (required change 1 of review 4958165282)

Fixed in 6d12e769e65f174a4615b87da9aea5d9b1b1318d (11f772ac64 before the dev rebase): InputController.#restoreEditorFocus() now routes through ctx.restoreComposer() — the same pet-aware helper SelectorController.showSelector's done() already prefers — and only falls back to the plain detach/clear/re-add swap for contexts that predate it. In an active pet session the restore now re-mounts PetFramedEditor (preserving the composer reserve); in off mode it re-mounts the plain editor, exactly as before.

Deterministic regressions added in packages/coding-agent/test/input-controller-keybindings.test.ts:

Close (⎋): with restoreComposer present, cancelling the queue selector calls it exactly once and the container ends with the composer, never the raw-editor swap — pins the active-pet framed-mount contract (the mock stands in for InteractiveMode.restoreComposer, which remounts the framed wrapper when the pet is active).
Delete-to-empty (⌦ ⌦): deleting down to an empty queue closes through the pet-aware restore; queue ends empty.
Move + delete (⌃↓ ⌦ ⌦): reorder through the selector, then delete to empty — still the pet-aware restore.
Fallback: when restoreComposer is absent (off mode / lightweight doubles), the close still restores the plain editor with setFocus(editor).

All 65 tests in that suite green locally.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Addressed — repository-wide SelectorController fixture audit + lifecycle fixture disposal (required changes 2 and 3 of review 4958165282)

Fixture audit. Every test context that instantiates SelectorController and reaches showSelector() was audited repository-wide (new SelectorController\( across packages/coding-agent/test). Doubles with only clear()/addChild() were brought to the detachChild contract with child-list-correct semantics (splice from the tracked children array — not a no-op), matching the already-updated fixtures:

custom-model-preset-creation.test.ts (double without a child list; no-op detach is correct there since addChild only captures selector instances)
model-selector-profiles-redteam.test.ts (both contexts, lines 127 and 356)
provider-onboarding-wizard.test.ts + provider-onboarding-wizard-redteam.test.ts (child-list-correct splice)
theme-selector-input.test.ts (both contexts)
thinking-selector.test.ts (all four contexts)
selector-controller-resume-model.test.ts
selector-controller-session-delete.test.ts
login-preset-recommendation.test.ts (the login-selector context, covering the "session/login selectors" ask)

Verified non-fixtures: credential-auto-import-flows and interactive-mode-editor-component / interactive-mode-background-activity already use real Containers (real detachChild); g007-ws5-redteam, sessions-dashboard, transcript-viewer-overlay, resume-progress-lease, and both resume-session-reentrancy suites never reach showSelector(). All named suites re-run green locally (87 pass across the nine updated files; 140 across pet/palette/queue suites).

Fixture disposal. The three real-CustomEditor pet-widget lifecycle tests now dispose their editors in finally (widget.dispose(); editor.dispose()), and the repeated-cycle test's red control is followed by an explicit idempotent editor.dispose() so the process-wide tab-width listener can never leak into later tests even if the red-control clear were removed.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Addressed — evidence refresh + adjacent-path audit (required changes 4 of 4958165282, and the audit note)

Evidence. The PR body's exact-evidence section now names head 6d12e769e6, base ceb31349c2, digest sha256:4221be9bfe1f9ffad63811decb0be31cc43369d59e3ad8b607522d28dee52eba, and cites only the fresh exact-head run 32154050225 (Dev CI) as current evidence. The old 32099837032 (cancelled) / 32099835720 / 32099895625 references are retained only as superseded history with their states labeled truthfully (contract runs failed solely on the by-design needs-human gate); 32099895353 is explicitly marked as the prior head's completed product run.

Adjacent raw clear() swaps. Audited against the contract this PR introduces. The OAuth code input, jobs/tasks panes, command-controller loaders, and runtime-MCP wizard opens share the same mechanism (disposing clear() over a live reusable editor) but are not paths this PR's changelog claims: its scope is the selector/command-palette/queued-message overlay family named in #4604, whose open/restore contract is now uniformly detach-then-readd. Those sibling paths predate this PR on the base commit and are tracked as #4657 with per-site locations, default keybindings making them reachable, and the two-line fix shape — deliberately not scope-crept here to keep this PR one reviewable contract. Happy to fold them in if you'd rather have them in this PR.

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

…y opens Issue #4657. Four overlay-open paths still cleared editorContainer with the live composer attached: the jobs overlay (alt+j //monitors), the tasks pane (alt+t), the OAuth API-key paste input, and the /debug log + raw-SSE viewers. Container.clear() disposes children terminally and Editor.dispose() tears down the tab-width change listener, so the first round-trip on any of these paths silently killed the composer's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Each open now detaches the reusable editor first (Container.detachChild, the same detach-then-readd reuse contract showSelector and the extension-ui and pet-aware restore paths already follow from PR #4605), so the terminal clear disposes only the transient overlay. User-facing behavior is unchanged: the overlay mounts exactly as before and the composer is re-added on close. Regression coverage: one deterministic test per named path in composer-detach-overlay-paths.test.ts driving the real production open/close code (real JobsOverlayComponent/TasksPaneComponent focus lists, the captured production onPrompt closure over a real Input, and the real DebugSelector selection into the real DebugLogViewerComponent with an isolated agent-dir log source), each asserting the tab-width listener still fires across four overlay cycles, with a red control proving the probe detects genuine disposal. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes untouched (showSelector/extension-ui/pet restore shapes unchanged) Tested: red/green — 4/4 fail on unfixed tree, 4/4 pass with fix Tested: 107 pass across 9 adjacent suites (pet widget, editor dispose, tasks, jobs, debug, login) Tested: 190 pass across 7 selector/palette/input suites Tested: bun run --cwd=packages/coding-agent check + build clean; ci:test:smoke ok Tested: check:tools, node20-baseline, public-sync, schemas, docker-context, gjc-ui gates pass Confidence: high Scope-risk: narrow Reversibility: trivial

Yeachan-Heo · 2026-08-18T18:03:49Z

Fix-forward complete on head 6d12e769e6 — all substantive findings implemented; blocked solely on fresh human review

Head: 6d12e769e65f174a4615b87da9aea5d9b1b1318d · Base: dev ceb31349c2d024c75825c819cb530d5060f49380 · Digest: sha256:4221be9bfe1f9ffad63811decb0be31cc43369d59e3ad8b607522d28dee52eba · Branch: fix/issue-4604-command-palette-pet-restore (force-with-lease from eef82ef5dc → 11f772ac64 → 6d12e769e6 after two dev advances; contributor authorship Yeachan Heo <yeachan.heo@gmail.com> preserved on all four commits)
Every finding of reviews 4958123531 and 4958165282 is implemented (signed per-finding replies: issuecomment-5330457089):
1. Queued-message selector close/delete/move now restore through the pet-aware ctx.restoreComposer() — active pet keeps PetFramedEditor + composer reserve; 4 deterministic regressions added (65/65 in that suite).
2. Repository-wide SelectorController fixture audit: 9 additional test files beyond the flagged four now implement child-list-correct detachChild; real-Container and non-showSelector contexts verified unaffected.
3. Real CustomEditor fixtures disposed in all three pet-widget lifecycle tests — no process-wide tab-width listener leaks.
4. PR body evidence refreshed to the exact head/base/digest; superseded runs labeled truthfully (cancelled/failed-by-design), current run cited.
5. Adjacent raw clear() paths (OAuth code input, jobs/tasks panes, command-controller loaders, runtime-MCP wizard) audited: same mechanism, outside this PR's claimed contract, tracked in fix(tui): detach reusable composer before clear() at remaining overlay open paths (jobs, tasks, oauth paste, /debug) #4657 with per-site fix shape.
Exact-head CI: Dev CI 32154701523 (rerun) — every product job green, including both suites whose first attempt hit a runner-side Install system deps stall (recovered via cancel + rerun-failed; not a code failure — both pass locally and in the same workflow's other shards). Sole red is PR contract bootstrap failing by design on the recorded needs-human verdict.
Local exact-head validation: all 15+ named suites green, check clean, build exit 0, git diff --check clean.

Remaining blocker (human-only): a fresh non-author exact-head review of 6d12e769e6 with a merge-approved verdict bound to sha256:4221be9bfe1f9ffad63811decb0be31cc43369d59e3ad8b607522d28dee52eba. @snowykr — both your reviews are addressed head-on; re-review whenever ready. On approval the owner lane will flip the verdict to merge-approved, let the contract job go green, merge to dev, dogfood fresh dev with bun run build, and reconcile post-merge CI.

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

snowykr

Verdict

Request changes. The reviewed commit is 6d12e769e65f174a4615b87da9aea5d9b1b1318d. Two evidence-grounded P1 issues remain: one leaves equivalent composer lifecycle defects in production paths, and one leaves the changed queued-message path without a regression test that exercises the relevant disposal contract.

Summary

The detach-before-clear repair is correctly applied in the reviewed selector, queued-message, and pet-remount paths. However, other production flows still clear a container with the reusable composer attached, and the queued-message tests use doubles that cannot detect the Container.clear() disposal behavior being fixed.

Findings / Required Changes

[P1] Repair the remaining composer replacement paths before clearing. packages/coding-agent/src/modes/controllers/command-controller.ts:191-199 still clears editorContainer while the reusable editor is attached in /share, then re-adds that disposed instance; the default Gist branch repeats the pattern at lines 248-256. The same lifecycle defect is reported in the MCP-add wizard, OAuth prompt, Jobs overlay, and Tasks pane. Because Container.clear() disposes attached children, these paths can remove the editor's tab-width listener and clear its paste state. Detach the reusable editor (or its framed owner) before clearing in each affected transition, and add focused regression coverage.
[P1] Cover the queued-message selector with real disposal semantics. packages/coding-agent/test/input-controller-keybindings.test.ts:198-207 uses a fake editor and a clear() mock that only truncates an array. Those tests would still pass if the new detachChild(editor) immediately before InputController's clear() were removed, so they do not prove that the changed path avoids Container.clear() → Editor.dispose(). Add an InputController-level regression using a real Container and CustomEditor; open and restore the queued-message selector, then verify the restored composer remains usable and responds to a tab-width change.

CI / Verification

No PR code, tests, linting, formatting, or local verification commands were run by the reviewers. GitHub-observed CI on the reviewed head showed the requested changed-test checks succeeding, including command-palette controller, resume-model controller, session-delete controller, command-palette interactive host, input-controller keybindings, and the relevant pet-widget suite; the overall Dev CI run was marked failed because PR contract bootstrap failed while its product checks succeeded.

Axis Coverage

A1 — Intent/architecture: Approved the scoped detach-before-clear lifecycle repair and changelog placement; no finding.
A2 — Correctness: Approved the changed selector, queued-message restoration, pet ownership, and error-path behavior; no finding.
A3 — Security: Found no actionable security, privacy, or trust issue.
A4 — Verification: Raised the queued-message real-disposal regression coverage gap.
A5 — Compatibility/completeness: Raised the remaining equivalent production composer-replacement lifecycle defects.

…g mounts Fix-forward on PR #4687 (snowykr CHANGES_REQUESTED on 39df3bc), closing all five findings on current dev (7932f8a): P1 — the /share custom-share loader, the /share gist loader (command-controller.ts #shareExport) and the /mcp add wizard (runtime-mcp-command-controller.ts #handleAdd) still called editorContainer.clear() with the live reusable CustomEditor attached, terminally disposing its tab-width listener on first round-trip — the same defect PR #4605 fixed at the five highest-traffic paths and #4687 at the selector/jobs/tasks/OAuth/debug paths. All three opens now detach the composer before clearing, and the regression suite grows live-controller cycles for both paths: /share runs handleShareCommand end to end against a gated share.ts seeded in an exclusively owned agent dir (asserting the composer probe still fires while the loader owns the container, and again after the handler restores it); /mcp add drives controller.handle("/mcp add") through two full wizard mount/cancel cycles. P2 — the /debug log viewer restored the composer (selector onDone) before awaiting log-source reads, then unconditionally cleared and mounted, so a newer overlay (or a stopped UI) during the await was disposed and replaced by the stale completion. #handleViewLogs now verifies the operation's starting restore state still holds — the composer live-mounted in the editor container — and a stale completion returns without touching the active container. The guard is pet-aware: InteractiveMode.restoreComposer delegates to GajaePetWidget.remountComposer, which mounts the PetFramedEditor wrapper (not the bare editor) as the container's content, so the check accepts either shape; a naive bare-editor-only guard was verified red against a new pet-wrapped regression (the viewer never mounts). A second regression opens logs and seizes the container with the jobs overlay before any event-loop turn, then asserts the overlay survives as the sole child and the composer stays alive while detached. P2 — the /share and /debug fixtures were not hermetic under the canonical config override: GJC_CONFIG_DIR takes precedence over PI_CONFIG_DIR, so a caller-provided override could redirect log seeding and cleanup outside the test's own tree, and cleanup removed a fixed home path. Both config selectors are now pinned to one per-run UUID-named exclusive root with a mkdtemp agent dir, the resolved config root is asserted to sit inside that exclusive tree before anything is seeded (a provenance-refused override fails loudly instead of touching the real config), and the restore closure returns the exact prior env/agent-dir state and removes only the exclusive trees. P2 — the fixture hard-reset theme/tab width instead of restoring prior values and used unbounded polls plus fixed sleeps. Each test now snapshots and exactly restores lifecycle globals (tab width plus the prior theme name through the public theme API), and every wait is a bounded condition wait (waitUntil) or an explicit mount signal. P2 — the declared TestContext.editorContainer contract in selector-controller-session-delete.test.ts now includes detachChild, so the double can no longer drift from the container contract the production open path exercises. P3 — the seeded dated log is written with Bun.write per the repo file conventions (node:fs/promises stays for directory operations), and the /share wizard double in share-command-temp-security.test.ts implements the non-disposing detach contract. Lore-id: issue-4657-composer-detach Constraint: fix-forward on PR #4687 head 39df3bc; authorship preserved Test: composer-detach-overlay-paths 8 pass (tamper-red verified: stale guard, pet-unaware guard, /share detach, /mcp add detach each fail red) Test: 545 pass across modes/ + debug + pet-widget suites Test: 133 pass across the 13 PR-touched suites Test: bun run --cwd=packages/coding-agent check clean; ci:test:smoke ok Confidence: high Scope-risk: narrow Reversibility: trivial

…y opens Issue #4657. Four overlay-open paths still cleared editorContainer with the live composer attached: the jobs overlay (alt+j //monitors), the tasks pane (alt+t), the OAuth API-key paste input, and the /debug log + raw-SSE viewers. Container.clear() disposes children terminally and Editor.dispose() tears down the tab-width change listener, so the first round-trip on any of these paths silently killed the composer's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Each open now detaches the reusable editor first (Container.detachChild, the same detach-then-readd reuse contract showSelector and the extension-ui and pet-aware restore paths already follow from PR #4605), so the terminal clear disposes only the transient overlay. User-facing behavior is unchanged: the overlay mounts exactly as before and the composer is re-added on close. Regression coverage: one deterministic test per named path in composer-detach-overlay-paths.test.ts driving the real production open/close code (real JobsOverlayComponent/TasksPaneComponent focus lists, the captured production onPrompt closure over a real Input, and the real DebugSelector selection into the real DebugLogViewerComponent with an isolated agent-dir log source), each asserting the tab-width listener still fires across four overlay cycles, with a red control proving the probe detects genuine disposal. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes untouched (showSelector/extension-ui/pet restore shapes unchanged) Tested: red/green — 4/4 fail on unfixed tree, 4/4 pass with fix Tested: 107 pass across 9 adjacent suites (pet widget, editor dispose, tasks, jobs, debug, login) Tested: 190 pass across 7 selector/palette/input suites Tested: bun run --cwd=packages/coding-agent check + build clean; ci:test:smoke ok Tested: check:tools, node20-baseline, public-sync, schemas, docker-context, gjc-ui gates pass Confidence: high Scope-risk: narrow Reversibility: trivial

…w-SSE Review follow-up on PR #4687 (snowykr CHANGES_REQUESTED): P1 — the branch assumed the generic SelectorController.showSelector open detach had already landed (PR #4605 is still unmerged), so production /debug and bare /login still disposed the reusable composer at that boundary before any downstream detach could run. showSelector now detaches the editor before its clear(), making this branch self-contained at every open path it claims. P2 — the debug regression only exercised the log viewer branch; it now alternates logs and raw-SSE entries across the four cycles, so both changed viewer branches carry the composer-lifecycle assertion. The harness also drives the real showDebugSelector/showSelector boundary instead of a local scaffold, so the P1 fix is covered by the same test (verified: reverting only the showSelector detach makes exactly this test fail). Lightweight test doubles whose editorContainer stubs predate detachChild (model-selector batch/profiles/profiles-redteam, provider-onboarding wizard and redteam, custom-model-preset-creation, login-preset-recommendation) gain the method, matching how PR #4605 updated its own doubles. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes (no other showSelector-site changes) Tested: stash of only the showSelector detach -> debug test fails (red, 3/4) Tested: 477 pass across 28 affected suites incl. the 4-path regression file Tested: bun run --cwd=packages/coding-agent check clean Confidence: high Scope-risk: narrow Reversibility: trivial

…g mounts Fix-forward on PR #4687 (snowykr CHANGES_REQUESTED on 39df3bc), closing all five findings on current dev (7932f8a): P1 — the /share custom-share loader, the /share gist loader (command-controller.ts #shareExport) and the /mcp add wizard (runtime-mcp-command-controller.ts #handleAdd) still called editorContainer.clear() with the live reusable CustomEditor attached, terminally disposing its tab-width listener on first round-trip — the same defect PR #4605 fixed at the five highest-traffic paths and #4687 at the selector/jobs/tasks/OAuth/debug paths. All three opens now detach the composer before clearing, and the regression suite grows live-controller cycles for both paths: /share runs handleShareCommand end to end against a gated share.ts seeded in an exclusively owned agent dir (asserting the composer probe still fires while the loader owns the container, and again after the handler restores it); /mcp add drives controller.handle("/mcp add") through two full wizard mount/cancel cycles. P2 — the /debug log viewer restored the composer (selector onDone) before awaiting log-source reads, then unconditionally cleared and mounted, so a newer overlay (or a stopped UI) during the await was disposed and replaced by the stale completion. #handleViewLogs now verifies the operation's starting restore state still holds — the composer live-mounted in the editor container — and a stale completion returns without touching the active container. The guard is pet-aware: InteractiveMode.restoreComposer delegates to GajaePetWidget.remountComposer, which mounts the PetFramedEditor wrapper (not the bare editor) as the container's content, so the check accepts either shape; a naive bare-editor-only guard was verified red against a new pet-wrapped regression (the viewer never mounts). A second regression opens logs and seizes the container with the jobs overlay before any event-loop turn, then asserts the overlay survives as the sole child and the composer stays alive while detached. P2 — the /share and /debug fixtures were not hermetic under the canonical config override: GJC_CONFIG_DIR takes precedence over PI_CONFIG_DIR, so a caller-provided override could redirect log seeding and cleanup outside the test's own tree, and cleanup removed a fixed home path. Both config selectors are now pinned to one per-run UUID-named exclusive root with a mkdtemp agent dir, the resolved config root is asserted to sit inside that exclusive tree before anything is seeded (a provenance-refused override fails loudly instead of touching the real config), and the restore closure returns the exact prior env/agent-dir state and removes only the exclusive trees. P2 — the fixture hard-reset theme/tab width instead of restoring prior values and used unbounded polls plus fixed sleeps. Each test now snapshots and exactly restores lifecycle globals (tab width plus the prior theme name through the public theme API), and every wait is a bounded condition wait (waitUntil) or an explicit mount signal. P2 — the declared TestContext.editorContainer contract in selector-controller-session-delete.test.ts now includes detachChild, so the double can no longer drift from the container contract the production open path exercises. P3 — the seeded dated log is written with Bun.write per the repo file conventions (node:fs/promises stays for directory operations), and the /share wizard double in share-command-temp-security.test.ts implements the non-disposing detach contract. Lore-id: issue-4657-composer-detach Constraint: fix-forward on PR #4687 head 39df3bc; authorship preserved Test: composer-detach-overlay-paths 8 pass (tamper-red verified: stale guard, pet-unaware guard, /share detach, /mcp add detach each fail red) Test: 545 pass across modes/ + debug + pet-widget suites Test: 133 pass across the 13 PR-touched suites Test: bun run --cwd=packages/coding-agent check clean; ci:test:smoke ok Confidence: high Scope-risk: narrow Reversibility: trivial

snowykr

Verdict

CHANGES_REQUESTED

Summary

The five-axis review completed against the exact head and identified 1 actionable issue, led by Required context method breaks implementer compatibility. These findings require changes before approval.

Findings / Required Changes

[P2] Required context method breaks implementer compatibility.
Reference: packages/coding-agent/src/modes/types.ts:294
detachComposer() is newly mandatory on InteractiveModeContext, so downstream hosts and lightweight implementations must add it even though controllers already support absent methods. Make it optional with guarded use, or document and update every public implementer.

CI / Verification

Reviewed the exact remote head: 5387eba0cd549c50be7ded5484b2d8f687da757a.
CI summary: 31 passing, 2 failing, 10 pending/cancelled/skipped.
Failing checks: Validate exact-head PR contract, PR contract bootstrap.
Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.

Axis Coverage

Axis	Verdict	Coverage
A1. Intent / Policy / Contract	CHANGES_REQUESTED	Overlay lifecycle correctness is established; the main compatibility risk is the newly mandatory InteractiveModeContext method.
A2. Architecture / Correctness / Failure	APPROVED	Composer detach/restore paths preserve reusable editor ownership and pet-aware mounting; no correctness or concurrency risk was established.
A3. Security / Privacy / Trust	APPROVED	Security and privacy risk was not increased by the reviewed changes; no new sensitive-data handling or trust-boundary changes were found.
A4. Verification / Tests / CI	APPROVED	A4 verification evidence is broadly passing, with no test-backed regression found; contract validation and skipped platform jobs remain bounded risks.
A5. Context / Compatibility / Platform	APPROVED	Integration and compatibility behavior appears correct under passing validation; skipped Windows and Darwin ARM checks leave platform-specific risk unverified.

Limitations

Exact-head PR contract and bootstrap CI checks failed, so required repository contract enforcement was not established.
PR contract bootstrap and exact-head contract jobs failed, so PR contract compliance was not established.
Windows and darwin-arm64 platform-specific jobs were skipped, so those platform paths were not verified.
Windows native build toolchain and darwin-arm64 tab-worker smoke checks were skipped, so those platform paths were not validated.

which only admits the current overlay owner or a widget whose epoch still matches. A widget that never activated (pet.mode "off" at startup) claims neither, so InteractiveMode.restoreComposer() -- which always routes through petWidget.remountComposer() once init() creates the widget -- silently no-opped. Palette cancel/close paths then left their CommandPaletteComponent mounted in editorContainer, leaking the modal (issue #4604, three command-palette-interactive-host failures). Composer-mount authority and overlay ownership are separate concerns: a never-activated widget still owns its host's composer mount and must remount the plain editor exactly like the no-pet fallback, while only disposal or a live successor widget revokes the mount. The active-owner case now also remounts the framed editor instead of relying on the epoch clause alone. Lore-id: 4604-palette-pet-remount Constraint: must not weaken #4591 successor-takeover isolation Tested: bun test command-palette-interactive-host.test.ts (8/8) Tested: bun test gajae-pet-widget.test.ts (62 pass incl. 2 new regressions) Tested: bun test packages/tui/test/gajae-pet.test.ts (19 pass) Not-tested: live iTerm2 pet drag interaction (requires real terminal) Confidence: high Scope-risk: narrow Reversibility: trivial

…store Container.clear() disposes children terminally, and Editor.dispose() tears down the tab-width change listener. Opening a selector cleared editorContainer with the live composer attached, so the first palette round-trip silently killed the editor's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Overlay open paths now detach the reusable editor first (Container.detachChild, the tui detach-then-readd reuse contract), and the pet-aware restores (#mountEditor, restoreComposer, queued-message restore) detach both reusable mounts before clearing. Successor ownership isolation in remountComposer is unchanged. Regression coverage: a real-CustomEditor lifecycle test in the widget suite (disposal observably stops tab-width invalidations) and an end-to-end host test that fails on the pre-fix open path and passes with the fix (verified red/green by reverting the source change). Lore-id: pr4605-composer-lifecycle Constraint: preserve #4591 successor-takeover isolation (remountComposer untouched) Tested: red/green on command-palette-interactive-host (8+1 pass with fix; new test fails without) Tested: 264 pass across 12 adjacent suites; gajae-pet-widget 63 pass Tested: tui + coding-agent check clean; coding-agent build clean Confidence: high Scope-risk: moderate Reversibility: trivial

The framed-remount regression test constructs a real editor to exercise container disposal semantics, but does not otherwise need the local binding. Removing it keeps the affected-package check warning-free. Lore-id: pr4605-lint-cleanup Tested: bun test packages/coding-agent/test/gajae-pet-widget.test.ts (63 pass) Tested: bun run --cwd=packages/coding-agent check Confidence: high Scope-risk: trivial Reversibility: trivial

…ctor Review 4958165282 on the exact head found the queued-message close path still swapped in the raw editor: in an active pet session the mounted composer child is PetFramedEditor, so #restoreEditorFocus() detached the plain editor (a no-op), let clear() drop the framed wrapper, and mounted the bare editor — the pet kept emitting while the composer lost its reserve. Route the restore through ctx.restoreComposer() (the same pet-aware helper SelectorController.showSelector's done() already prefers), falling back to the plain editor swap for contexts that predate it. Also bring every remaining SelectorController test double to the new detachChild contract (child-list-correct, not a no-op cast) and dispose the real CustomEditor fixtures in the pet-widget lifecycle tests so their process-wide tab-width listeners cannot leak into later tests. Lore-id: pr4605-review-4958165282 Constraint: preserve contributor authorship and one-PR scope Tested: bun test input-controller-keybindings (65), gajae-pet-widget, command-palette-interactive-host, selector-controller-command-palette, custom-model-preset-creation, model-selector-profiles-redteam, provider-onboarding-wizard(+redteam), theme-selector-input, thinking-selector, selector-controller-resume-model, selector-controller-session-delete, login-preset-recommendation, model-selector-controller-batch, model-selector-profiles (all green) Tested: bun run --cwd=packages/coding-agent check Confidence: high Scope-risk: moderate Reversibility: trivial

… paths Review 4964398363 on the exact head found two P1 gaps: 1. Equivalent composer-replacement defects remained in production paths: /share custom-export and default-gist loaders, the OAuth API-key code input, the jobs overlay, the tasks pane, the MCP-add wizard, and the /debug log/raw-SSE viewers all cleared editorContainer with the live composer attached, so Container.clear() disposed the editor and its tab-width listener. Each open path now detaches the reusable composer first; close-path clears were already safe once their opens detach. This closes the follow-up scope filed in #4657. 2. The queued-message regressions used an array-double editorContainer, which cannot detect Container.clear() disposal. Added an InputController-level regression over a real Container + CustomEditor: three open/restore cycles through the queue selector with tab-width toggles proving the restored composer keeps its listener and input state (red-control verified: removing the open-path detach fails it). Also gave the /share temp-security double child-list-correct detachChild semantics. Lore-id: pr4605-review-4964398363 Tested: bun test input-controller-keybindings (66 incl. new real-disposal regression), share-command-temp-security, debug/log-viewer, and the full 15-suite focused surface (264 pass) Tested: bun run --cwd=packages/coding-agent check Confidence: high Scope-risk: moderate Reversibility: trivial

Review 4966516047 on the exact head found pet-enabled overlay flows still detached and restored only the raw editor while the mounted child is PetFramedEditor: the raw-editor detach was a no-op in pet mode, so the terminal clear() disposed the framed composer and the close path reattached a disposed child. The regression coverage also never exercised the controller-level pet path. Add InteractiveModeContext.detachComposer() backed by GajaePetWidget.detachComposer() (detaches both reusable mounts) and route every overlay open through it — queued-message selector, SelectorController.showSelector, OAuth code input, jobs overlay, tasks pane, /share loaders, MCP-add wizard, /debug viewers — with the raw-editor detach kept only as the fallback for pre-pet contexts. Restores already route through the pet-aware restoreComposer(). Add a controller-level active-pet regression over a real GajaePetWidget + CustomEditor + Container: queued-selector cancel, final-item delete, and move/refresh all preserve the framed mount identity, the composer stays writable, and tab-width invalidation still reaches it (red-control verified both directions: raw-editor detach or raw-editor restore each fail the test). Also restore the custom-share loader's setFocus/requestRender transition. Lore-id: pr4605-review-4966516047 Tested: bun test input-controller-keybindings (67 incl. active-pet controller regression), the 17-suite focused surface (282 pass) Tested: bun run --cwd=packages/coding-agent check Confidence: high Scope-risk: moderate Reversibility: trivial

… path Review findings on the a578085 head (snowykr, exact-head review) proved the open-path detach contract was asymmetric: jobs/tasks pane closes, the OAuth manual-code submit, both /share restore closures, and the MCP-add wizard completion still cleared the container and re-added the raw editor, so an active pet lost its PetFramedEditor mount and reserve. All six close paths now route through ctx.restoreComposer() with the plain editor-swap fallback for hosts that predate it, and regression coverage proves the framed mount survives each transition. Lore-id: 4604-close-path-restore Constraint: keep #4591 successor-takeover isolation untouched Tested: bun test qa-pet-close-path-redteam.test.ts (5 pass, 4 red without fix) Tested: bun test modes/controllers/share-command-temp-security.test.ts (6 pass, 2 new red without fix) Tested: bun test command-palette-interactive-host + gajae-pet-widget + input-controller-keybindings + qa-pet-restore-redteam (145 pass) Confidence: high Scope-risk: narrow Reversibility: trivial Co-authored-by: snowykr <snowykr@users.noreply.github.com>

InteractiveModeContext has lightweight ACP and test-context implementers that do not expose the pet-aware detach hook. Keep the hook optional while retaining guarded controller use, so the lifecycle repair does not make downstream context construction mandatory.\n\nLore-id: pr4605-review-5387eba0\nConstraint: preserve compatibility for lightweight InteractiveModeContext hosts\nTested: bun --cwd=packages/coding-agent run check\nTested: bun test packages/coding-agent/test/gajae-pet-widget.test.ts packages/coding-agent/test/qa-pet-close-path-redteam.test.ts packages/coding-agent/test/input-controller-keybindings.test.ts packages/coding-agent/test/command-palette-interactive-host.test.ts packages/coding-agent/test/modes/controllers/share-command-temp-security.test.ts packages/coding-agent/test/qa-pet-restore-redteam.test.ts\nConfidence: high\nScope-risk: narrow\nReversibility: trivial\nCo-authored-by: snowykr <snowykr@users.noreply.github.com>

…y opens Issue #4657. Four overlay-open paths still cleared editorContainer with the live composer attached: the jobs overlay (alt+j //monitors), the tasks pane (alt+t), the OAuth API-key paste input, and the /debug log + raw-SSE viewers. Container.clear() disposes children terminally and Editor.dispose() tears down the tab-width change listener, so the first round-trip on any of these paths silently killed the composer's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Each open now detaches the reusable editor first (Container.detachChild, the same detach-then-readd reuse contract showSelector and the extension-ui and pet-aware restore paths already follow from PR #4605), so the terminal clear disposes only the transient overlay. User-facing behavior is unchanged: the overlay mounts exactly as before and the composer is re-added on close. Regression coverage: one deterministic test per named path in composer-detach-overlay-paths.test.ts driving the real production open/close code (real JobsOverlayComponent/TasksPaneComponent focus lists, the captured production onPrompt closure over a real Input, and the real DebugSelector selection into the real DebugLogViewerComponent with an isolated agent-dir log source), each asserting the tab-width listener still fires across four overlay cycles, with a red control proving the probe detects genuine disposal. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes untouched (showSelector/extension-ui/pet restore shapes unchanged) Tested: red/green — 4/4 fail on unfixed tree, 4/4 pass with fix Tested: 107 pass across 9 adjacent suites (pet widget, editor dispose, tasks, jobs, debug, login) Tested: 190 pass across 7 selector/palette/input suites Tested: bun run --cwd=packages/coding-agent check + build clean; ci:test:smoke ok Tested: check:tools, node20-baseline, public-sync, schemas, docker-context, gjc-ui gates pass Confidence: high Scope-risk: narrow Reversibility: trivial

…w-SSE Review follow-up on PR #4687 (snowykr CHANGES_REQUESTED): P1 — the branch assumed the generic SelectorController.showSelector open detach had already landed (PR #4605 is still unmerged), so production /debug and bare /login still disposed the reusable composer at that boundary before any downstream detach could run. showSelector now detaches the editor before its clear(), making this branch self-contained at every open path it claims. P2 — the debug regression only exercised the log viewer branch; it now alternates logs and raw-SSE entries across the four cycles, so both changed viewer branches carry the composer-lifecycle assertion. The harness also drives the real showDebugSelector/showSelector boundary instead of a local scaffold, so the P1 fix is covered by the same test (verified: reverting only the showSelector detach makes exactly this test fail). Lightweight test doubles whose editorContainer stubs predate detachChild (model-selector batch/profiles/profiles-redteam, provider-onboarding wizard and redteam, custom-model-preset-creation, login-preset-recommendation) gain the method, matching how PR #4605 updated its own doubles. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes (no other showSelector-site changes) Tested: stash of only the showSelector detach -> debug test fails (red, 3/4) Tested: 477 pass across 28 affected suites incl. the 4-path regression file Tested: bun run --cwd=packages/coding-agent check clean Confidence: high Scope-risk: narrow Reversibility: trivial

…g mounts Fix-forward on PR #4687 (snowykr CHANGES_REQUESTED on 39df3bc), closing all five findings on current dev (7932f8a): P1 — the /share custom-share loader, the /share gist loader (command-controller.ts #shareExport) and the /mcp add wizard (runtime-mcp-command-controller.ts #handleAdd) still called editorContainer.clear() with the live reusable CustomEditor attached, terminally disposing its tab-width listener on first round-trip — the same defect PR #4605 fixed at the five highest-traffic paths and #4687 at the selector/jobs/tasks/OAuth/debug paths. All three opens now detach the composer before clearing, and the regression suite grows live-controller cycles for both paths: /share runs handleShareCommand end to end against a gated share.ts seeded in an exclusively owned agent dir (asserting the composer probe still fires while the loader owns the container, and again after the handler restores it); /mcp add drives controller.handle("/mcp add") through two full wizard mount/cancel cycles. P2 — the /debug log viewer restored the composer (selector onDone) before awaiting log-source reads, then unconditionally cleared and mounted, so a newer overlay (or a stopped UI) during the await was disposed and replaced by the stale completion. #handleViewLogs now verifies the operation's starting restore state still holds — the composer live-mounted in the editor container — and a stale completion returns without touching the active container. The guard is pet-aware: InteractiveMode.restoreComposer delegates to GajaePetWidget.remountComposer, which mounts the PetFramedEditor wrapper (not the bare editor) as the container's content, so the check accepts either shape; a naive bare-editor-only guard was verified red against a new pet-wrapped regression (the viewer never mounts). A second regression opens logs and seizes the container with the jobs overlay before any event-loop turn, then asserts the overlay survives as the sole child and the composer stays alive while detached. P2 — the /share and /debug fixtures were not hermetic under the canonical config override: GJC_CONFIG_DIR takes precedence over PI_CONFIG_DIR, so a caller-provided override could redirect log seeding and cleanup outside the test's own tree, and cleanup removed a fixed home path. Both config selectors are now pinned to one per-run UUID-named exclusive root with a mkdtemp agent dir, the resolved config root is asserted to sit inside that exclusive tree before anything is seeded (a provenance-refused override fails loudly instead of touching the real config), and the restore closure returns the exact prior env/agent-dir state and removes only the exclusive trees. P2 — the fixture hard-reset theme/tab width instead of restoring prior values and used unbounded polls plus fixed sleeps. Each test now snapshots and exactly restores lifecycle globals (tab width plus the prior theme name through the public theme API), and every wait is a bounded condition wait (waitUntil) or an explicit mount signal. P2 — the declared TestContext.editorContainer contract in selector-controller-session-delete.test.ts now includes detachChild, so the double can no longer drift from the container contract the production open path exercises. P3 — the seeded dated log is written with Bun.write per the repo file conventions (node:fs/promises stays for directory operations), and the /share wizard double in share-command-temp-security.test.ts implements the non-disposing detach contract. Lore-id: issue-4657-composer-detach Constraint: fix-forward on PR #4687 head 39df3bc; authorship preserved Test: composer-detach-overlay-paths 8 pass (tamper-red verified: stale guard, pet-unaware guard, /share detach, /mcp add detach each fail red) Test: 545 pass across modes/ + debug + pet-widget suites Test: 133 pass across the 13 PR-touched suites Test: bun run --cwd=packages/coding-agent check clean; ci:test:smoke ok Confidence: high Scope-risk: narrow Reversibility: trivial

…y opens Issue #4657. Four overlay-open paths still cleared editorContainer with the live composer attached: the jobs overlay (alt+j //monitors), the tasks pane (alt+t), the OAuth API-key paste input, and the /debug log + raw-SSE viewers. Container.clear() disposes children terminally and Editor.dispose() tears down the tab-width change listener, so the first round-trip on any of these paths silently killed the composer's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Each open now detaches the reusable editor first (Container.detachChild, the same detach-then-readd reuse contract showSelector and the extension-ui and pet-aware restore paths already follow from PR #4605), so the terminal clear disposes only the transient overlay. User-facing behavior is unchanged: the overlay mounts exactly as before and the composer is re-added on close. Regression coverage: one deterministic test per named path in composer-detach-overlay-paths.test.ts driving the real production open/close code (real JobsOverlayComponent/TasksPaneComponent focus lists, the captured production onPrompt closure over a real Input, and the real DebugSelector selection into the real DebugLogViewerComponent with an isolated agent-dir log source), each asserting the tab-width listener still fires across four overlay cycles, with a red control proving the probe detects genuine disposal. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes untouched (showSelector/extension-ui/pet restore shapes unchanged) Tested: red/green — 4/4 fail on unfixed tree, 4/4 pass with fix Tested: 107 pass across 9 adjacent suites (pet widget, editor dispose, tasks, jobs, debug, login) Tested: 190 pass across 7 selector/palette/input suites Tested: bun run --cwd=packages/coding-agent check + build clean; ci:test:smoke ok Tested: check:tools, node20-baseline, public-sync, schemas, docker-context, gjc-ui gates pass Confidence: high Scope-risk: narrow Reversibility: trivial

…w-SSE Review follow-up on PR #4687 (snowykr CHANGES_REQUESTED): P1 — the branch assumed the generic SelectorController.showSelector open detach had already landed (PR #4605 is still unmerged), so production /debug and bare /login still disposed the reusable composer at that boundary before any downstream detach could run. showSelector now detaches the editor before its clear(), making this branch self-contained at every open path it claims. P2 — the debug regression only exercised the log viewer branch; it now alternates logs and raw-SSE entries across the four cycles, so both changed viewer branches carry the composer-lifecycle assertion. The harness also drives the real showDebugSelector/showSelector boundary instead of a local scaffold, so the P1 fix is covered by the same test (verified: reverting only the showSelector detach makes exactly this test fail). Lightweight test doubles whose editorContainer stubs predate detachChild (model-selector batch/profiles/profiles-redteam, provider-onboarding wizard and redteam, custom-model-preset-creation, login-preset-recommendation) gain the method, matching how PR #4605 updated its own doubles. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes (no other showSelector-site changes) Tested: stash of only the showSelector detach -> debug test fails (red, 3/4) Tested: 477 pass across 28 affected suites incl. the 4-path regression file Tested: bun run --cwd=packages/coding-agent check clean Confidence: high Scope-risk: narrow Reversibility: trivial

…g mounts Fix-forward on PR #4687 (snowykr CHANGES_REQUESTED on 39df3bc), closing all five findings on current dev (7932f8a): P1 — the /share custom-share loader, the /share gist loader (command-controller.ts #shareExport) and the /mcp add wizard (runtime-mcp-command-controller.ts #handleAdd) still called editorContainer.clear() with the live reusable CustomEditor attached, terminally disposing its tab-width listener on first round-trip — the same defect PR #4605 fixed at the five highest-traffic paths and #4687 at the selector/jobs/tasks/OAuth/debug paths. All three opens now detach the composer before clearing, and the regression suite grows live-controller cycles for both paths: /share runs handleShareCommand end to end against a gated share.ts seeded in an exclusively owned agent dir (asserting the composer probe still fires while the loader owns the container, and again after the handler restores it); /mcp add drives controller.handle("/mcp add") through two full wizard mount/cancel cycles. P2 — the /debug log viewer restored the composer (selector onDone) before awaiting log-source reads, then unconditionally cleared and mounted, so a newer overlay (or a stopped UI) during the await was disposed and replaced by the stale completion. #handleViewLogs now verifies the operation's starting restore state still holds — the composer live-mounted in the editor container — and a stale completion returns without touching the active container. The guard is pet-aware: InteractiveMode.restoreComposer delegates to GajaePetWidget.remountComposer, which mounts the PetFramedEditor wrapper (not the bare editor) as the container's content, so the check accepts either shape; a naive bare-editor-only guard was verified red against a new pet-wrapped regression (the viewer never mounts). A second regression opens logs and seizes the container with the jobs overlay before any event-loop turn, then asserts the overlay survives as the sole child and the composer stays alive while detached. P2 — the /share and /debug fixtures were not hermetic under the canonical config override: GJC_CONFIG_DIR takes precedence over PI_CONFIG_DIR, so a caller-provided override could redirect log seeding and cleanup outside the test's own tree, and cleanup removed a fixed home path. Both config selectors are now pinned to one per-run UUID-named exclusive root with a mkdtemp agent dir, the resolved config root is asserted to sit inside that exclusive tree before anything is seeded (a provenance-refused override fails loudly instead of touching the real config), and the restore closure returns the exact prior env/agent-dir state and removes only the exclusive trees. P2 — the fixture hard-reset theme/tab width instead of restoring prior values and used unbounded polls plus fixed sleeps. Each test now snapshots and exactly restores lifecycle globals (tab width plus the prior theme name through the public theme API), and every wait is a bounded condition wait (waitUntil) or an explicit mount signal. P2 — the declared TestContext.editorContainer contract in selector-controller-session-delete.test.ts now includes detachChild, so the double can no longer drift from the container contract the production open path exercises. P3 — the seeded dated log is written with Bun.write per the repo file conventions (node:fs/promises stays for directory operations), and the /share wizard double in share-command-temp-security.test.ts implements the non-disposing detach contract. Lore-id: issue-4657-composer-detach Constraint: fix-forward on PR #4687 head 39df3bc; authorship preserved Test: composer-detach-overlay-paths 8 pass (tamper-red verified: stale guard, pet-unaware guard, /share detach, /mcp add detach each fail red) Test: 545 pass across modes/ + debug + pet-widget suites Test: 133 pass across the 13 PR-touched suites Test: bun run --cwd=packages/coding-agent check clean; ci:test:smoke ok Confidence: high Scope-risk: narrow Reversibility: trivial

Yeachan-Heo · 2026-08-21T17:55:40Z

Replacement head dfee62b is pushed and bound to the current dev base 79fe412 (diff digest sha256:969e0d4b01a592b059acc3ae2c432ca1612128cb53b2044a7deabf48f79da73a). The latest exact-head P2 compatibility finding is fixed: InteractiveModeContext.detachComposer is optional and all controller call sites remain guarded. Local focused lifecycle coverage is 156/156 green and coding-agent check is clean; exact-head affected-path CI is green for the completed jobs. Please review this exact head and record an authenticated APPROVED review if clean; the PR contract remains needs-human until that independent review exists.

Yeachan-Heo · 2026-08-21T18:44:12Z

Evidence-backed close for PR #4605. Replacement head dfee62b is pushed and contains the preserved fix history plus the latest exact-head compatibility correction. The binary diff digest is sha256:969e0d4b01a592b059acc3ae2c432ca1612128cb53b2044a7deabf48f79da73a. Local focused lifecycle/red-team coverage is 156/156 green; coding-agent check, affected-path CI, native build, and state gates passed. The remaining exact-head contract blocker is the required independent authenticated review, which was requested but not obtained. Closing without merge preserves the review gate and source evidence; no bypass or release was performed.

PR #4605's queued-selector fix was not merged into current dev, leaving one reachable clear-with-composer path outside the prior #4687 enumeration. Detach the reusable composer before queued-selector open and prove real Container disposal semantics across repeated restore cycles.\n\nIssue: #4657\nLore-id: issue-4657-composer-detach\nConstraint: complete the remaining overlay-open lifecycle on current dev without duplicating merged equivalents\nTested: bun test packages/coding-agent/test/input-controller-keybindings.test.ts\nConfidence: high\nScope-risk: moderate\nReversibility: trivial

…y opens Issue #4657. Four overlay-open paths still cleared editorContainer with the live composer attached: the jobs overlay (alt+j //monitors), the tasks pane (alt+t), the OAuth API-key paste input, and the /debug log + raw-SSE viewers. Container.clear() disposes children terminally and Editor.dispose() tears down the tab-width change listener, so the first round-trip on any of these paths silently killed the composer's listener; every later restore re-mounted a dead editor and runtime tab-width changes stopped re-deriving composer layout. Each open now detaches the reusable editor first (Container.detachChild, the same detach-then-readd reuse contract showSelector and the extension-ui and pet-aware restore paths already follow from PR #4605), so the terminal clear disposes only the transient overlay. User-facing behavior is unchanged: the overlay mounts exactly as before and the composer is re-added on close. Regression coverage: one deterministic test per named path in composer-detach-overlay-paths.test.ts driving the real production open/close code (real JobsOverlayComponent/TasksPaneComponent focus lists, the captured production onPrompt closure over a real Input, and the real DebugSelector selection into the real DebugLogViewerComponent with an isolated agent-dir log source), each asserting the tab-width listener still fires across four overlay cycles, with a red control proving the probe detects genuine disposal. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes untouched (showSelector/extension-ui/pet restore shapes unchanged) Tested: red/green — 4/4 fail on unfixed tree, 4/4 pass with fix Tested: 107 pass across 9 adjacent suites (pet widget, editor dispose, tasks, jobs, debug, login) Tested: 190 pass across 7 selector/palette/input suites Tested: bun run --cwd=packages/coding-agent check + build clean; ci:test:smoke ok Tested: check:tools, node20-baseline, public-sync, schemas, docker-context, gjc-ui gates pass Confidence: high Scope-risk: narrow Reversibility: trivial

…w-SSE Review follow-up on PR #4687 (snowykr CHANGES_REQUESTED): P1 — the branch assumed the generic SelectorController.showSelector open detach had already landed (PR #4605 is still unmerged), so production /debug and bare /login still disposed the reusable composer at that boundary before any downstream detach could run. showSelector now detaches the editor before its clear(), making this branch self-contained at every open path it claims. P2 — the debug regression only exercised the log viewer branch; it now alternates logs and raw-SSE entries across the four cycles, so both changed viewer branches carry the composer-lifecycle assertion. The harness also drives the real showDebugSelector/showSelector boundary instead of a local scaffold, so the P1 fix is covered by the same test (verified: reverting only the showSelector detach makes exactly this test fail). Lightweight test doubles whose editorContainer stubs predate detachChild (model-selector batch/profiles/profiles-redteam, provider-onboarding wizard and redteam, custom-model-preset-creation, login-preset-recommendation) gain the method, matching how PR #4605 updated its own doubles. Lore-id: issue-4657-composer-detach Constraint: preserve PR #4605 lanes (no other showSelector-site changes) Tested: stash of only the showSelector detach -> debug test fails (red, 3/4) Tested: 477 pass across 28 affected suites incl. the 4-path regression file Tested: bun run --cwd=packages/coding-agent check clean Confidence: high Scope-risk: narrow Reversibility: trivial

…g mounts Fix-forward on PR #4687 (snowykr CHANGES_REQUESTED on 39df3bc), closing all five findings on current dev (7932f8a): P1 — the /share custom-share loader, the /share gist loader (command-controller.ts #shareExport) and the /mcp add wizard (runtime-mcp-command-controller.ts #handleAdd) still called editorContainer.clear() with the live reusable CustomEditor attached, terminally disposing its tab-width listener on first round-trip — the same defect PR #4605 fixed at the five highest-traffic paths and #4687 at the selector/jobs/tasks/OAuth/debug paths. All three opens now detach the composer before clearing, and the regression suite grows live-controller cycles for both paths: /share runs handleShareCommand end to end against a gated share.ts seeded in an exclusively owned agent dir (asserting the composer probe still fires while the loader owns the container, and again after the handler restores it); /mcp add drives controller.handle("/mcp add") through two full wizard mount/cancel cycles. P2 — the /debug log viewer restored the composer (selector onDone) before awaiting log-source reads, then unconditionally cleared and mounted, so a newer overlay (or a stopped UI) during the await was disposed and replaced by the stale completion. #handleViewLogs now verifies the operation's starting restore state still holds — the composer live-mounted in the editor container — and a stale completion returns without touching the active container. The guard is pet-aware: InteractiveMode.restoreComposer delegates to GajaePetWidget.remountComposer, which mounts the PetFramedEditor wrapper (not the bare editor) as the container's content, so the check accepts either shape; a naive bare-editor-only guard was verified red against a new pet-wrapped regression (the viewer never mounts). A second regression opens logs and seizes the container with the jobs overlay before any event-loop turn, then asserts the overlay survives as the sole child and the composer stays alive while detached. P2 — the /share and /debug fixtures were not hermetic under the canonical config override: GJC_CONFIG_DIR takes precedence over PI_CONFIG_DIR, so a caller-provided override could redirect log seeding and cleanup outside the test's own tree, and cleanup removed a fixed home path. Both config selectors are now pinned to one per-run UUID-named exclusive root with a mkdtemp agent dir, the resolved config root is asserted to sit inside that exclusive tree before anything is seeded (a provenance-refused override fails loudly instead of touching the real config), and the restore closure returns the exact prior env/agent-dir state and removes only the exclusive trees. P2 — the fixture hard-reset theme/tab width instead of restoring prior values and used unbounded polls plus fixed sleeps. Each test now snapshots and exactly restores lifecycle globals (tab width plus the prior theme name through the public theme API), and every wait is a bounded condition wait (waitUntil) or an explicit mount signal. P2 — the declared TestContext.editorContainer contract in selector-controller-session-delete.test.ts now includes detachChild, so the double can no longer drift from the container contract the production open path exercises. P3 — the seeded dated log is written with Bun.write per the repo file conventions (node:fs/promises stays for directory operations), and the /share wizard double in share-command-temp-security.test.ts implements the non-disposing detach contract. Lore-id: issue-4657-composer-detach Constraint: fix-forward on PR #4687 head 39df3bc; authorship preserved Test: composer-detach-overlay-paths 8 pass (tamper-red verified: stale guard, pet-unaware guard, /share detach, /mcp add detach each fail red) Test: 545 pass across modes/ + debug + pet-widget suites Test: 133 pass across the 13 PR-touched suites Test: bun run --cwd=packages/coding-agent check clean; ci:test:smoke ok Confidence: high Scope-risk: narrow Reversibility: trivial

PR #4605's queued-selector fix was not merged into current dev, leaving one reachable clear-with-composer path outside the prior #4687 enumeration. Detach the reusable composer before queued-selector open and prove real Container disposal semantics across repeated restore cycles.\n\nIssue: #4657\nLore-id: issue-4657-composer-detach\nConstraint: complete the remaining overlay-open lifecycle on current dev without duplicating merged equivalents\nTested: bun test packages/coding-agent/test/input-controller-keybindings.test.ts\nConfidence: high\nScope-risk: moderate\nReversibility: trivial

Yeachan-Heo mentioned this pull request Aug 16, 2026

fix(dev): command-palette-interactive-host failures from #4591 pet integration #4604

Closed

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from c194e79 to b5929c8 Compare August 17, 2026 16:01

Yeachan-Heo requested a review from snowykr August 17, 2026 16:11

snowykr requested changes Aug 17, 2026

View reviewed changes

Yeachan-Heo requested a review from snowykr August 17, 2026 19:15

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from a890974 to cce1441 Compare August 17, 2026 19:23

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from 097c3b4 to eef82ef Compare August 18, 2026 04:37

Yeachan-Heo mentioned this pull request Aug 18, 2026

fix(tui): detach reusable composer before clear() at remaining overlay open paths (jobs, tasks, oauth paste, /debug) #4657

Closed

snowykr requested changes Aug 18, 2026

View reviewed changes

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from eef82ef to 11f772a Compare August 18, 2026 15:13

Yeachan-Heo mentioned this pull request Aug 18, 2026

fix(tui): detach reusable composer before clear() at remaining overlay open paths #4687

Merged

3 tasks

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from 11f772a to 6d12e76 Compare August 18, 2026 15:22

snowykr requested changes Aug 18, 2026

View reviewed changes

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from 6d12e76 to d9af002 Compare August 18, 2026 20:05

snowykr requested changes Aug 21, 2026

View reviewed changes

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from 5387eba to 5e601ef Compare August 21, 2026 17:39

Yeachan Heo and others added 8 commits August 21, 2026 17:41

Yeachan-Heo force-pushed the fix/issue-4604-command-palette-pet-restore branch from 5e601ef to dfee62b Compare August 21, 2026 17:42

Yeachan-Heo requested a review from snowykr August 21, 2026 17:55

Yeachan-Heo closed this Aug 21, 2026

Conversation

Yeachan-Heo commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Review round 3 fix-forward (owner lane)

Latest exact-head review fix-forward

Reconstruction

Exact evidence

Risk classification

Review state

GJC verdict

Uh oh!

chatgpt-codex-connector Bot commented Aug 16, 2026

Uh oh!

Yeachan-Heo commented Aug 16, 2026

Root cause

Validation (exact PR head c194e79)

CI on this head (run 31932247745 + 31932247752)

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

snowykr left a comment

Choose a reason for hiding this comment

Verdict: Request changes

Required changes

1. Make the remount tests model the reusable editor lifecycle

2. Refresh the exact-head evidence in the PR contract

Review lane summary

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

Yeachan-Heo commented Aug 17, 2026

Uh oh!

Yeachan-Heo commented Aug 18, 2026

Uh oh!

Yeachan-Heo commented Aug 18, 2026

Uh oh!

snowykr left a comment

Choose a reason for hiding this comment

Verdict: Request changes

Required change

Update all SelectorController test contexts for the new detachChild requirement

Verification notes

Uh oh!

snowykr left a comment

Choose a reason for hiding this comment

Verdict: Request changes

Required changes

1. Restore the pet-aware composer from the queued-message selector

2. Update every selector test double that now needs detachChild

3. Refresh the exact-head evidence references

Additional verification note

Lane summary

Uh oh!

Yeachan-Heo commented Aug 18, 2026

Uh oh!

Yeachan-Heo commented Aug 18, 2026

Uh oh!

snowykr left a comment

Choose a reason for hiding this comment

Verdict

Summary

Findings / Required Changes

CI / Verification

Axis Coverage

Uh oh!

snowykr left a comment

Choose a reason for hiding this comment

Verdict

Summary

Findings / Required Changes

CI / Verification

Yeachan-Heo commented Aug 16, 2026 •

edited

Loading

Validation (exact PR head `c194e79`)

Update all SelectorController test contexts for the new `detachChild` requirement

2. Update every selector test double that now needs `detachChild`