Repository navigation
fix(tui): close frame coalescing lifecycle gaps - #5321
Yeachan-Heo wants to merge 116 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
-----BEGIN PGP SIGNED MESSAGE----- PR #5321 ownership and PR #5310 blocker evidence iHUEARYIAB0WIQQE16ZuzjuAE7BguBSIF/QUP4T15wUCapwq6wAKCRCIF/QUP4T1 |
79ee63a to
be48c45
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79ee63a2ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@snowykr Exact-head review requested for |
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0728fd1bd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review exact head |
|
-----BEGIN PGP SIGNED MESSAGE----- PR #5321 redacted ownership reconstruction evidence iHUEARYIAB0WIQQE16ZuzjuAE7BguBSIF/QUP4T15wUCapw28gAKCRCIF/QUP4T1 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 691a32efd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review exact head |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
-----BEGIN PGP SIGNED MESSAGE----- PR #5321 public redacted ownership and blocker evidence iHUEARYIAB0WIQQE16ZuzjuAE7BguBSIF/QUP4T15wUCapw9PwAKCRCIF/QUP4T1 |
|
@probepark Independent exact-head approval is requested for |
|
Exact-head status for |
|
@IYENTeam Independent exact-head approval is requested for |
|
@HaD0Yun Independent exact-head approval is requested for |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Reviewed head: 60bea52
Verdict: REQUEST_CHANGES
[P2] Preserve orphan assistant output through later same-transcript rebuilds — packages/coding-agent/src/modes/controllers/event-controller.ts:1132 (with packages/coding-agent/src/modes/interactive-mode.ts:1559-1579).
The orphan branch renders finalMessage into orphanComponent, then clears both streamingComponent and streamingMessage without adding that message to any session-owned or rebuild-owned source. The new reconcile path only preserves a component when those streaming fields are still populated; otherwise chatContainer.clear() disposes the sole rendered copy, and buildDisplaySessionContext() rebuilds only from SessionManager. This matters on the force-recovery path because Agent.forceAbort() emits cancelled agent_end with messages: [] and no message_end, so the partial was never appended durably.
Exact-head reproduction after cancelled agent_end: beforeContains=true, wouldPreserve=false, childrenAfterRebuild=0. A settings/hide-thinking reconciliation or later compaction therefore makes the output this PR claims to retain disappear. Please finalize the orphan into a session-owned/rebuild-visible historical source (or retain it through rebuild by another explicit owner) and add a regression that performs agent_end followed by the actual reconcile rebuild.
Evidence:
- Reviewed the full base...head diff and lifecycle call paths.
- 105 focused tests passed across render-commit, text-coalescing, viewport revision, and abort-render suites.
- bun --cwd=packages/tui run check passed.
- bun --cwd=packages/coding-agent run check passed.
- Hosted exact-head technical checks: 22 passed. The two red PR-contract contexts are non-product blockers; the bootstrap log fails only with Verdict needs-human intentionally blocks merge.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This PR adds TUI raster-lifecycle fencing, preserves an active assistant during one same-transcript rebuild path, and retains partial assistant output for orphan agent_end events. The lifecycle goals are sound, but two reachable streaming paths still dispose or misclassify the live assistant output.
Findings / Required Changes
-
[P1] Preserve the live assistant for tree-navigation rebuilds —
packages/coding-agent/src/modes/interactive-mode.ts:1558-1580- The new detach/rebind protection is confined to
rebuildChatFromMessages, while the reachable tree-navigation consumer rebuilds the same transcript throughrebuildInitialMessages(packages/coding-agent/src/modes/controllers/selector-controller.ts:3191;packages/coding-agent/src/modes/interactive-mode.ts:2179-2190)./treeand the session-tree action are available with an active run; that path clears the chat container and disposes the streamingAssistantMessageComponentinstead of rebinding it. - Subsequent
message_update/message_endevents retain the disposed pointer and are rejected by the live-component/disposed guards (packages/coding-agent/src/modes/controllers/event-controller.ts:282-292;packages/coding-agent/src/modes/components/assistant-message.ts:451-454), so the remaining response is lost. The base did not introduce the new preservation contract; this PR makes the omission material by adding it only to the alternate rebuild path. - Reject or abort tree navigation while streaming, or route its same-transcript rebuild through the preservation-aware detach/rebind flow. Cover the live-stream tree-navigation path.
- The new detach/rebind protection is confined to
-
[P2] Snapshot silent-abort classification before queued orphan handling —
packages/coding-agent/src/modes/controllers/event-controller.ts:1108-1117- The new orphan
agent_endpath reads mutableAgentSessionpending-abort flags only when the independently serialized EventController queue eventually dispatches the terminal event. A forced silent abort can publish its syntheticagent_endwith no messages, then clear#silentAbortPendingin abort cleanup (packages/coding-agent/src/session/agent-session.ts:14768-14770) before that queued handler runs. - Normal
message_endhandling stamps suppression synchronously before emission, but the orphan path lacks that snapshot. With a preceding queued UI event, the orphan follows the visibleOperation abortedbranch even though the cancellation is meant to be silent. - Carry immutable abort classification with the terminal event (or otherwise preserve it until UI terminal dispatch), and add a delayed-controller-queue regression test.
- The new orphan
CI / Verification
Exact-head Dev CI coverage passed for the affected coding-agent and TUI tests, package checks, TypeScript build, CLI smoke, virtual integration, and state gates. The PR-contract checks are currently blocked by the independent-review/needs-human contract rather than a product-test failure. The added tests do not cover a streaming tree-navigation rebuild or delayed queued orphan-terminal classification.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
The promised active-stream preservation is incomplete for the same-transcript tree-navigation consumer (Finding 1). |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Reachable rebuild disposal and delayed terminal-classification races remain (Findings 1–2). |
| A3 — Security / Privacy / Trust | APPROVED |
No attributable trust-boundary, authority, or sensitive-data regression was verified. |
| A4 — Verification / Tests / CI | APPROVED |
Relevant exact-head CI passed; missing interleaving coverage supports the concrete correctness findings but is not an independent blocker. |
| A5 — Context / Compatibility / Platform | APPROVED |
Consumers, internal lifecycle contracts, and platform/build surfaces were traced; no separate compatibility regression was verified. |
Limitations
Review analysis used immutable head/base snapshots and CI metadata. No local test execution was performed during this read-only review.
60bea52 to
76a7d08
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
76a7d08 to
3955b4b
Compare
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR closes raster/render-commit lifecycle gaps and updates session and skill-state handling. The exact-head review found no merge-blocking defects; two low-priority findings are non-blocking.
Findings / Required Changes
-
[P3] Preserve the disposal cause in raster invalidation metadata —
packages/tui/src/tui.ts:1364dispose()still finalizes raster leases asterminal-loss, and invalidation notices propagate that cause. This matches the base behavior but contradicts the PR's explicit promise to reportdisposeseparately. The in-repo consumer ignores the cause, so this is a public metadata mismatch rather than a demonstrated runtime regression. Passdisposeand test the callback, or narrow the stated contract. Non-blocking.
-
[P3] Avoid following symlinks when creating the migration marker —
packages/coding-agent/src/skill-state/active-state.ts:760- When legacy session migration runs in a crafted workspace, a dangling marker symlink can pass the preceding
statas absent and makeBun.writecreate fixedpending\ncontent at its target outside.gjc. This new write bypasses the state-writer path guard. The target must be creatable, and a pre-existing symlink does not overwrite an existing target, so impact is limited. Use exclusive no-follow creation or an equivalent guarded state-writer primitive. Non-blocking.
- When legacy session migration runs in a crafted workspace, a dangling marker symlink can pass the preceding
Non-blocking Observations
- The PR description's claim that invalid or stale lease results now all report
failedis broader than the implementation: valid stale-token/predicate cases still returnstale-token, as covered by tests and the public status union. Narrow that wording; in-repo consumers treat any non-writtenstatus alike.
CI / Verification
GitHub check runs bound to f18099291aff524214c3feb2f73c84f3e0b0c336: 80 succeeded and 6 were skipped. Targeted TUI and session test jobs succeeded; the skipped WSL/DrvFS qualification and live deployment checks are opt-in/manual. No tests were run locally.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Compared the explicit disposal/status claims with public raster contracts and repository policy; disposal-cause mismatch is non-blocking, stale-token behavior remains a tested supported status. No intent_projection was available. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Lifecycle ordering and TTSR persistence-failure paths were traced; retry-gate release avoids the base liveness wedge, with no verified blocking defect. |
| A3 — Security / Privacy / Trust | APPROVED |
One limited symlink-following write is noted as P3; no higher-impact trust-boundary issue was established. |
| A4 — Verification / Tests / CI | APPROVED |
Changed tests cover lifecycle/failure outcomes; exact-head GitHub checks succeeded, with only opt-in/manual skips. |
| A5 — Context / Compatibility / Platform | APPROVED |
Callers and lifecycle consumers were traced; no material compatibility issue or unnecessary duplicate abstraction was found. |
Limitations
Review was read-only. No local tests, checks, or live terminal/platform validation were run; CI evidence was inspected on the exact reviewed head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 006d679553
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (await hasPendingActiveEntryMigration(options.cwd, sessionId)) { | ||
| await migrateSnapshotEntriesToActiveDirectory(options.cwd, sessionId); | ||
| } | ||
| const authoritativeEntries = await hasAuthoritativeActiveEntryDirectory(options.cwd, sessionId); |
There was a problem hiding this comment.
Unlike the resolved syncSkillActiveState() path, this handoff path runs migration only when the new pending marker exists. If an upgraded session has a legacy/indeterminate snapshot plus a pre-existing partial active/ directory without that marker—for example after an older process wrote one entry and exited before rebuilding the snapshot—hasAuthoritativeActiveEntryDirectory() selects only the partial directory, and the subsequent rebuild permanently drops the snapshot-only workflows. A handoff can therefore remove a concurrent ralplan row and its approval guard; invoke migrateSnapshotEntriesToActiveDirectory() unconditionally here, since it already no-ops for derived snapshots.
AGENTS.md reference: AGENTS.md:L121-L121
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| this.#requeue(kind, built.entries); | ||
| this.rearmIdle(); | ||
| logger.warn("Yield queue streaming dispatch failed", { kind, error: formatError(error) }); | ||
| continue; |
There was a problem hiding this comment.
When consecutive ownership groups A and B were built and injectStreaming() throws for A, this catch requeues only A and then continues the loop, allowing B to be delivered immediately. A later retry consequently delivers A after B, violating the FIFO chronology that the grouping logic preserves. Requeue the failed group together with the remaining built suffix and stop this flush, as the build-failure path already does.
Useful? React with 👍 / 👎.
Add changelog.d fragment for Performance and Added entries instead of editing CHANGELOG.md directly. This follows the changelog fragment pattern and avoids conflicts on the shared [Unreleased] section.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Large PR — scope review only (head aa22b58)
Scope: +7916 / -1629 across 61 files. OCR selects 24 source files with 5567 changed lines. The increment from f180992 spans 186 files and 10153 changed lines. Both exceed the automated code-review limit.
ocr: blocking 0 / nit 0
These counts contain no code findings: code review was skipped. They do not establish that this change is safe.
CI: Windows validation failed in packages/natives/test/walker-pool-unavailable.windows.test.ts:127: expected at most 20 threads, received 21. Evidence production and the aggregate check then failed because Windows validation did not succeed. The latest dev run failed on different coding-agent shards. This does not establish a base cause for the Windows failure; its cause remains unclassified.
Run: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38048749307
Areas for separate review:
- Agent lifecycle:
packages/agent/src/agent-loop.ts,agent.ts, andtypes.ts. - Session lifecycle and cancellation:
packages/coding-agent/src/session/agent-session.ts,yield-queue.ts,terminal-abort.ts, andsession-manager.ts.agent-session.tsalone changes 3654 lines. - TUI lifecycle and navigation:
packages/tui/src/tui.ts, coding-agent mode controllers,interactive-mode.ts, and pet transport. - Runtime state and SDK: state writer, active skill state, hooks, managed-task DAG, SDK session, and the generated operation inventory.
- Supporting changes: AI fallback transport, file locking, TTSR export, and task executor.
Changelog fragments and regression-test files are present. No released CHANGELOG or docs-index file appears in the changed-file list. The generated SDK operation inventory needs a regeneration/sync check during the full review.
- Split independent concerns into reviewable PRs. Keep regression tests and release fragments with their implementation.
- Review cancellation ownership, queue settlement, raster disposal, and branch-navigation isolation in their respective slices.
- Diagnose the Windows failure before seeking approval. Record the exact-head CI result.
No APPROVE or REQUEST_CHANGES decision is submitted. The PR body is unchanged. Full code review and OCR rule validation remain unperformed.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head aa22b58, gajae-reviewer on behalf of probepark)
CI: PR cause 1 + unclassified platform failure — the exact-head run has a failing required Windows session-path regression, and the affected-path evidence/aggregate fail because CI_DEV_WINDOWS_DOCTOR_RESULT=failure is required. See https://github.com/Yeachan-Heo/gajae-code/actions/runs/38048749307.
Scope: +7916 / -1629, 61 files — agent/AI, coding-agent session and SDK lifecycle, TUI lifecycle, natives/path identity, docs and generated/support files.
Conventions: changelog fragments present; generated packages/ai/src/models.json and plugin metadata are included; no docs-index.generated.ts observed; no labels.
ocr: blocking 0 / nit 0 (v1.12.13; 24 reviewable source files, 37 excluded). OCR rule selection completed; no OCR finding was assessed as independently blocking.
Notable:
packages/ai/src/models.jsonand plugin metadata are generated/support paths in this large mixed PR; verify their generator provenance and synchronization before merge.- The source diff remains 5,567 OCR-reviewable changed lines, so this automated pass cannot establish correctness for the lifecycle changes.
blocking: 1) required Windows regression is red and its PR/base causality is not established; 2) affected-path evidence fails on that required result; 3) code review is not completed for the >800-line reviewable source surface; 4) generated/support-file provenance is not established.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:c5558c352092d37668313e76f875ce392422327c922f09e64ddf144980e2d054 reviewer:critic reviewer-id:gajae-reviewer evidence:ci-windows-failed;affected-evidence-failed;large-pr-unreviewed;generated-provenance-unverified
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c4e36238c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| existingEntry = merged.predecessor; | ||
| if (!existingEntry) return { state: existing, seeded: false, rollback: noRollback }; |
There was a problem hiding this comment.
When another writer removes the workflow entry between the initial read and mergeActiveEntrySubskills(), the stale result has no predecessor and this branch returns the old existing snapshot as though activation were still present. A concurrent deactivation similarly becomes the next merge candidate even though it is no longer active. A direct ralplan or deep-interview invocation can therefore proceed without reseeding the durable entry that engages its approval guard and Stop hook; reload the visible state and enter the normal seed path unless the predecessor is still active and matches the invocation context.
AGENTS.md reference: AGENTS.md:L121-L121
Useful? React with 👍 / 👎.
| throw new Error( | ||
| isToolDiscoveryCallable(tools) | ||
| ? `${base}. If you are unsure whether this tool exists or how to use it, call \`${TOOL_DISCOVERY_NAME}\` to discover and activate the matching tool, then retry.` | ||
| : base, |
There was a problem hiding this comment.
When the model calls an unavailable tool, this now throws a plain Error instead of the previously marked designed error. finishExecuteToolSpan() passes the caught error to recordHandledError() whenever isDesignedError() is false, so routine model mistakes are recorded as unexpected handled failures in postmortem diagnostics; the same regression affects the nearby malformed/escaped-argument rejections. Keep these expected rejection paths wrapped with markDesignedError().
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Scope-only review (head 6c4e362)
Large PR — code review skipped. Human review is required.
Scope: +7916 / -1629 across 61 files. OCR excludes tests, changelog fragments, and generated inventory.
After exclusions, 24 source files still contain +4318 / -1249 (5567 changed lines). This exceeds the 800-line review limit.
The excluded packages/coding-agent/src/sdk/protocol/operation-inventory.generated.json is also a source-tree change. Excluding it does not make this PR reviewable.
CI: failed. The Windows session-path job fails in task-cache-key.test.ts:261, in construction-time provider ownership. The evidence producer and aggregate then fail because Windows validation did not succeed.
Run: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38056277044
The base Dev CI also failed (https://github.com/Yeachan-Heo/gajae-code/actions/runs/38056102503). The matching base failure has not been established. Treat the Windows failure as unclassified, not as proven base-only.
Conventions: changelog fragments exist for agent, ai, coding-agent, and tui. No released CHANGELOG or docs-index.generated.ts changes appear in the file inventory. No labels are attached.
Files by area:
packages/agent:
packages/agent/src/agent-loop.ts(+269 / -83)packages/agent/src/agent.ts(+134 / -44)packages/agent/src/types.ts(+21 / -1)
packages/ai:
packages/ai/src/utils/fallback-transport.ts(+20 / -6)
packages/coding-agent:
packages/coding-agent/src/config/file-lock.ts(+12 / -4)packages/coding-agent/src/export/ttsr.ts(+9 / -0)packages/coding-agent/src/gjc-runtime/state-writer.ts(+169 / -38)packages/coding-agent/src/hooks/skill-state.ts(+62 / -2)packages/coding-agent/src/modes/components/gajae-pet-widget.ts(+4 / -3)packages/coding-agent/src/modes/components/iterm-pet-transport.ts(+44 / -25)packages/coding-agent/src/modes/components/settings-selector.ts(+9 / -0)packages/coding-agent/src/modes/controllers/event-controller.ts(+97 / -8)packages/coding-agent/src/modes/controllers/input-controller.ts(+2 / -8)packages/coding-agent/src/modes/controllers/selector-controller.ts(+18 / -0)packages/coding-agent/src/modes/interactive-mode.ts(+47 / -9)packages/coding-agent/src/sdk/broker/managed-task-dag.ts(+35 / -34)packages/coding-agent/src/sdk/session.ts(+30 / -6)packages/coding-agent/src/session/agent-session.ts(+2780 / -874)packages/coding-agent/src/session/session-manager.ts(+37 / -2)packages/coding-agent/src/session/terminal-abort.ts(+14 / -5)packages/coding-agent/src/session/yield-queue.ts(+251 / -33)packages/coding-agent/src/skill-state/active-state.ts(+149 / -38)packages/coding-agent/src/task/executor.ts(+3 / -1)
packages/tui:
packages/tui/src/tui.ts(+102 / -25)
ocr: blocking 0 / nit 0
This count is not a clean-code verdict. OCR selection and rule loading completed; code-level rules were not evaluated because the size gate prevents code review.
Not assessed: code correctness, test adequacy, generated-output provenance, and visual behavior. No APPROVE or REQUEST_CHANGES decision is submitted. The PR body is unchanged.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review re-request (rereq1), head 6c4e362
Large PR — code review skipped; area-level file list only. No approval or change-request verdict is submitted.
Scope: +7916 / -1629, 61 files. OCR v1.12.13 selects 24 source files: +4318 / -1249 = 5567 changed lines. This exceeds the 800-line review limit after exclusions.
CI: the required Windows regression failed during temporary-directory cleanup at packages/coding-agent/test/task-cache-key.test.ts:261. The log reports a failure to remove .gjc; this is not evidence that the provider-identity assertion failed. The evidence producer and aggregate fail downstream because Windows validation did not succeed.
Run: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38056277044
The latest base Dev CI also failed: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38056102503. A matching base failure was not established. Windows causality remains unclassified, not proven base-only.
Conventions: changelog fragments exist for agent, AI, coding-agent, and TUI. No released CHANGELOG or docs-index.generated.ts appears in the inventory. No labels are attached. The excluded source-tree file packages/coding-agent/src/sdk/protocol/operation-inventory.generated.json is also changed. Its provenance requires review; excluding it does not bring this PR under the size limit.
Reviewable files by area:
packages/agent/
packages/agent/src/agent-loop.ts(+269 / -83)packages/agent/src/agent.ts(+134 / -44)packages/agent/src/types.ts(+21 / -1)
packages/ai/
packages/ai/src/utils/fallback-transport.ts(+20 / -6)
packages/coding-agent/
packages/coding-agent/src/config/file-lock.ts(+12 / -4)packages/coding-agent/src/export/ttsr.ts(+9 / -0)packages/coding-agent/src/gjc-runtime/state-writer.ts(+169 / -38)packages/coding-agent/src/hooks/skill-state.ts(+62 / -2)packages/coding-agent/src/modes/components/gajae-pet-widget.ts(+4 / -3)packages/coding-agent/src/modes/components/iterm-pet-transport.ts(+44 / -25)packages/coding-agent/src/modes/components/settings-selector.ts(+9 / -0)packages/coding-agent/src/modes/controllers/event-controller.ts(+97 / -8)packages/coding-agent/src/modes/controllers/input-controller.ts(+2 / -8)packages/coding-agent/src/modes/controllers/selector-controller.ts(+18 / -0)packages/coding-agent/src/modes/interactive-mode.ts(+47 / -9)packages/coding-agent/src/sdk/broker/managed-task-dag.ts(+35 / -34)packages/coding-agent/src/sdk/session.ts(+30 / -6)packages/coding-agent/src/session/agent-session.ts(+2780 / -874)packages/coding-agent/src/session/session-manager.ts(+37 / -2)packages/coding-agent/src/session/terminal-abort.ts(+14 / -5)packages/coding-agent/src/session/yield-queue.ts(+251 / -33)packages/coding-agent/src/skill-state/active-state.ts(+149 / -38)packages/coding-agent/src/task/executor.ts(+3 / -1)
packages/tui/
packages/tui/src/tui.ts(+102 / -25)
ocr: blocking 0 / nit 0
These counts are not a clean-code verdict. OCR selection and rule loading completed; code-level findings were not assessed.
Blocking: not assessed. Full source review, test adequacy, and generated provenance were not verified. No local builds or tests were run. The PR body and existing reviews remain unchanged.
Split independent lifecycle areas into reviewable PRs, keeping tests and release fragments with each implementation. Alternatively, obtain a maintainer line-by-line review. Diagnose the Windows cleanup failure before seeking approval.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cc5db9f4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ok && this.#inFlightMultipartAbort === multipartAbortBarrier) { | ||
| this.#inFlightMultipartAbort = undefined; | ||
| } else if (!ok && multipartAbortBarrier) { | ||
| multipartAbortBarrier(); |
There was a problem hiding this comment.
When a dependent generic render passes the final lifecycle gate but #guardTerminalOperation() then detects terminal loss during the write, #markTerminalUnavailable() advances #rasterLifecycle, yet this path continues into the unchanged !ok && dependent branch and stores the old frame in #pendingDependentGenericBytes. After restart, notifyTerminalLifecycle() writes that untagged frame once cleanup succeeds, replaying content from the prior terminal generation before the fresh render. Fresh evidence relative to the resolved terminal-loss thread is this loss-during-write path, which occurs after the new pre-write lifecycle check; re-check the captured lifecycle before retaining failed dependent bytes.
Useful? React with 👍 / 👎.
…iteActiveEntry and removeActiveEntry The PR introduced conditional scope lock acquisition in writeActiveEntry and removeActiveEntry, which is incorrect. These functions operate on individual entry files, not the scope file (skill-active-state.json.entries). The scope lock does not protect these files and should only be acquired at a higher level. This change reverts writeActiveEntry and removeActiveEntry to their dev equivalents, which simply call the underlying operations without any scope lock handling. The activeStateScopeLockHeld parameter is no longer used by these functions, as the scope lock should be managed by the caller (e.g., syncSkillActiveState) if needed. Fixes Windows task-cache-key.test.ts cleanup failures caused by pending lock directories being left behind during test execution.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f50f78cf49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const SERVER_OVERLOADED_PROVIDER_CODE = "server_is_overloaded"; | ||
| const MAX_TRANSPORT_CODE_LENGTH = 256; | ||
| const MAX_NATIVE_HTTP2_ERROR_CODE_LENGTH = 64; | ||
| const MAX_RETAINED_TRANSPORT_HEADER_VALUE_LENGTH = 1024; |
There was a problem hiding this comment.
Fresh evidence relative to the earlier rebuttal is that the exact reviewed commit now batches 61 files covering AI transport limits, agent cancellation, workflow-state migration, SDK enrollment, session orchestration, and TUI raster handling under a TUI-only subject. These changes cannot be reviewed or reverted independently, contrary to the repository’s one-logical-change-per-commit requirement; split the subsystems into separate atomic commits.
AGENTS.md reference: AGENTS.md:L177-L177
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head f50f78c, gajae-reviewer on behalf of probepark)
Incremental scope: 6c4e36238c96b49650864d112749da422c9a925b..f50f78cf49d3ad0cc474659d910e3d54ae7f9f92 — +45 / -57 across 4 files. The overall PR remains above the automated review ceiling; this review covers only the requested increment.
CI: PR cause 1 + affected evidence failure — the exact-head Windows session-path regression fails during task-cache-key.test.ts cleanup because .gjc remains undeletable; the affected-path evidence producer then fails because CI_DEV_WINDOWS_DOCTOR_RESULT=failure. Run: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38064044674
Scope: session state-writer locking, SessionManager closeStrict sidecar release, and the Windows cleanup regression test.
Conventions: changelog fragment present; no generated file or docs-index.generated.ts in the increment; no labels.
ocr: blocking 0 / nit 0 (v1.12.13; 24 reviewable source files, 37 excluded)
Notable:
packages/coding-agent/src/gjc-runtime/state-writer.ts:1362-1370,1488-1507— the increment removeswithActiveStateScopeLockfromwriteActiveEntryandremoveActiveEntry, while sibling update/merge/restore operations still take that scope lock. This reopens concurrent snapshot/rebuild races for the authoritative active-entry files and contradicts the surrounding transaction contract. Restore the scope lock (or prove an equivalent shared lock covers these writes/deletes) and add a concurrent writer regression.packages/coding-agent/src/session/session-manager.ts:17277-17288— the new sidecar release is conditional onoutcome.kind === "closed"and no observed persistence error, but the exact-head Windows run still leaves.gjcand session state directories undeletable after the changed test removed its retry workaround (packages/coding-agent/test/task-cache-key.test.ts:203-221). The change does not establish the promised cleanup guarantee. Ensure every managed sidecar/file handle is released before closeStrict returns on the tested path, then restore a regression that passes without timing-sensitive retries.
blocking: 1) active-entry writes/deletes bypass the established scope lock; 2) exact-head Windows cleanup regression remains red and affected-path evidence fails.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:4f4e9b77bb25ce588139f805aff5a72931ff70378da4491935d5ad1789e7ae62 reviewer:critic reviewer-id:gajae-reviewer evidence:incremental-lock-regression;windows-cleanup-failed;affected-evidence-failed
PR body verdict line is absent and was not added. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:4f4e9b77bb25ce588139f805aff5a72931ff70378da4491935d5ad1789e7ae62 reviewer:critic reviewer-id:gajae-reviewer evidence:incremental-lock-regression;windows-cleanup-failed;affected-evidence-failed
When running tests within a GJC work lane, GJC_SESSION_ID and GJC_LIFECYCLE_REQUEST_ID are already set in the environment. This causes tests that create multiple sessions to reuse the same preallocated session ID, causing assertion failures. Add withoutLifecycleIdentity helper to temporarily clear these variables during test execution, ensuring each session gets a unique ID. Also add explicit comment in afterEach cleanup about session disposal to clarify that assertions check for no lingering writes after dispose.
File-lock staging directories may not be immediately cleaned up after lock release. Use fsPromises.rm with recursive option and retry with exponential backoff to ensure all directories are cleaned up, including any pending lock directories. Fixes flaky test failures in task-cache-key.test.ts where lock staging directories were left behind during cleanup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a744e704
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (policyTerminalCommitted && composerRecoveryExhausted) { | ||
| message.stopReason = "error"; | ||
| const recoveryLimitMessage = | ||
| "Composer bash policy blocked repository file I/O again after its one automatic recovery turn. Continue with dedicated repository tools."; | ||
| message.errorMessage = message.errorMessage |
There was a problem hiding this comment.
When the model triggers the second Composer-policy block or reaches five malformed-tool turns, this mutates the assistant to stopReason: "error" only after its message_end was already published. Although AgentSession specially rewrites persistence at turn_end, EventController.#handleMessageEnd has already finalized the live assistant component and its turn_end handler is empty, so the current UI and message-lifecycle consumers retain the earlier non-error state and never show the newly added recovery-limit diagnostic until the transcript is reloaded. Determine the terminal status before message_end, or publish an explicit update that live consumers handle.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Large PR — scope review only (head b3a744e)
Full code review is skipped because the OCR-selected source diff exceeds the 800-line review ceiling.
Scope: +8,014 / -1,717 across 62 files. OCR selects 24 reviewable source files with 5,567 changed lines; 38 tests/changelog/generated/support files are excluded. Review areas include agent/AI streaming and cancellation, coding-agent session and workflow state, SDK/task lifecycle, interactive controllers, and TUI raster/render lifecycle.
CI: required validation is not green. Affected path validation and its evidence producer fail on exact-head run https://github.com/Yeachan-Heo/gajae-code/actions/runs/38067256959; the package and focused test jobs visible for the changed areas pass. The failure is not treated as base-only here.
Conventions: package changelog fragments are present for agent, AI, coding-agent, and TUI. The generated SDK operation inventory is changed and its generator provenance was not independently assessed. No labels are set.
Not assessed: correctness of the 24 source files, test adequacy beyond the hosted jobs, generated-output provenance, and visual behavior. This scope-only pass does not validate or dismiss the existing peer findings on this head.
ocr: blocking 0 / nit 0 (v1.12.13; 24 reviewable source files, 38 excluded). These counts are not a clean-code verdict; code-level rules were not assessed because the review ceiling is exceeded.
No APPROVE or REQUEST_CHANGES decision is submitted. Split the independent lifecycle areas into reviewable PRs or obtain maintainer line-by-line review, and resolve the failed affected-path validation before seeking approval.
The SessionPythonToolInput interface and test mocks were using an incorrect return type for registerSessionCleanup. The correct type is () => void, not (() => void) | void, which matches the ToolSession interface definition. Fixed: - src/tools/python.ts: SessionPythonToolInput.registerSessionCleanup return type - test/tools/python-tool-builtin.test.ts: Fixed two test mock implementations - test/tools/python-tool.test.ts: Fixed mock implementation to return proper function All types now pass tsc and biome lint checks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf563fa00b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return (event: AgentEvent) => { | ||
| if (this.#activeRunId !== runId) return; | ||
| this.emitExternalEvent(event); | ||
| const scope = this.#runHandles.get(logicalRunId)?.scope; | ||
| if (!event.scope && !scope) return; | ||
| this.emitExternalEvent(scope && !event.scope ? { ...event, scope } : event); |
There was a problem hiding this comment.
Capture the current handle's scope before returning this callback instead of looking it up for every emission. During a managed retry or resample, attemptScopeFactory.mint() replaces #runHandles[logicalRunId].scope while #activeRunId remains unchanged, so a delayed Cursor/non-abortable tool callback from the previous attempt is stamped with the successor scope and passes the session's attempt-admission fence. Its stale tool lifecycle or message event can then be displayed or persisted as output from the accepted attempt.
Useful? React with 👍 / 👎.
biome lint failed on line 525 in task-cache-key.test.ts due to line exceeding maximum length. Split fsPromises.mkdtemp call across multiple lines to comply with formatting rules.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa16a6fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (record.started || record.cleanupClaimed) continue; | ||
| void settlePreDispatchCancellationCleanup(record); | ||
| } |
There was a problem hiding this comment.
When an abort occurs while an exclusive tool is running, later exclusive calls are still waiting on start and have never entered runTool() or beforeToolCall; nevertheless, this loop invokes settlePreDispatchCancellationCleanup() for every non-started record. That calls afterToolCall with a synthetic cancellation for tools whose setup never occurred, which can release nonexistent ownership or trigger extension cleanup side effects. Use the newly tracked preDispatchEntered flag to limit cleanup to calls that actually crossed the pre-dispatch boundary.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on daa16a6; approval decision follows once CI finishes).
CI: gate pending — affected-path validation still has install-methods pending and the aggregate has not reported yet.
Scope: +8024 / -1724, 67 files — agent lifecycle, coding-agent session/runtime/UI controllers, TUI raster/render lifecycle, tests and changelog fragments.
Conventions: changelog fragments present; generated file packages/natives/native/diagnostic-artifact.json is included; no labels.
Notable:
- No blocking candidate identified in the reviewable source files. The OCR rules ran at v1.12.13 with 26 reviewable source files and 41 excluded test/documentation files.
- Test coverage changes are present for the TUI raster/render paths and related lifecycle behavior; broader source changes remain subject to the pending CI gate.
Blocking: none.
OCR: blocking 0 / nit 0
Verdict: held pending CI completion; no body verdict update was made.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Large PR — scope review only (head daa16a6).
CI has finished. gajae-approve-gate.py 5321 --head daa16a6fcdf68b2477076649a690963219d28c6d returned ALLOW, exit 0, at 2026-10-10 19:04 UTC. No pending, failed, or missing local checks were reported.
Approval remains withheld by the independent review-size limit. The earlier CI-held comment #5321 (review) is not sufficient for approval of an oversized source diff. Its blocking=0 marker must not be treated as a complete code-level verdict.
Scope: +8024 / -1724, 67 files. OCR v1.12.13 selects 26 files: +4284 / -1223 = 5507 changed lines, above the 800-line ceiling. 41 files are excluded.
Reviewable files by area:
packages/agent/src/agent-loop.ts(+269 / -83)packages/agent/src/agent.ts(+134 / -44)packages/agent/src/types.ts(+21 / -1)packages/ai/src/utils/fallback-transport.ts(+20 / -6)packages/coding-agent/src/config/file-lock.ts(+12 / -4)packages/coding-agent/src/export/ttsr.ts(+9 / -0)packages/coding-agent/src/gjc-runtime/state-writer.ts(+133 / -10)packages/coding-agent/src/hooks/skill-state.ts(+62 / -2)packages/coding-agent/src/modes/components/gajae-pet-widget.ts(+4 / -3)packages/coding-agent/src/modes/components/iterm-pet-transport.ts(+44 / -25)packages/coding-agent/src/modes/components/settings-selector.ts(+9 / -0)packages/coding-agent/src/modes/controllers/event-controller.ts(+97 / -8)packages/coding-agent/src/modes/controllers/input-controller.ts(+2 / -8)packages/coding-agent/src/modes/controllers/selector-controller.ts(+18 / -0)packages/coding-agent/src/modes/interactive-mode.ts(+47 / -9)packages/coding-agent/src/sdk/broker/managed-task-dag.ts(+35 / -34)packages/coding-agent/src/sdk/session.ts(+30 / -6)packages/coding-agent/src/session/agent-session.ts(+2780 / -874)packages/coding-agent/src/session/session-manager.ts(+37 / -2)packages/coding-agent/src/session/terminal-abort.ts(+14 / -5)packages/coding-agent/src/session/yield-queue.ts(+251 / -33)packages/coding-agent/src/skill-state/active-state.ts(+149 / -38)packages/coding-agent/src/task/executor.ts(+3 / -1)packages/coding-agent/src/tools/python.ts(+1 / -1)packages/natives/native/diagnostic-artifact.json(+1 / -1)packages/tui/src/tui.ts(+102 / -25)
Conventions: package changelog fragments exist for agent, AI, coding-agent, and TUI. No released CHANGELOG or docs-index.generated.ts appears in the file inventory. No labels are attached. The source-tree SDK operation inventory is excluded by OCR; its correctness and generation provenance are not assessed here.
ocr: blocking 0 / nit 0
These counts are not a clean-code verdict. File selection and rule loading completed; code-level findings were not evaluated in this gate-only follow-up.
Not assessed: full source correctness, test adequacy, generated-output provenance, or visual behavior. No APPROVE or REQUEST_CHANGES is submitted. The PR body and existing reviews remain unchanged.
Split independent lifecycle areas into reviewable PRs, keeping tests and release fragments with each implementation. Alternatively, obtain a maintainer line-by-line review.
|
@probepark @snowykr Head — |
This branch has not been deployed
3 participants
Problem
Gaps in the TUI frame-coalescing / raster-lease lifecycle:
dispose()finalized raster leases as terminal loss, and a lost terminal could still receive an abort barrier.Changes
packages/tui/src/tui.tsdisposeseparately from terminal loss and skip abort barriers after terminal loss.failedinstead ofstale-token.packages/coding-agent/src/modes/controllers/selector-controller.ts: after navigation commits, clear transient state from the predecessor response rather than reattaching it to the selected branch.packages/tui/changelog.d/fix-raster-lifecycle.mdandpackages/coding-agent/changelog.d/selector-tree-navigation-streaming.md.Tests
packages/tui/test/render-commit.test.tscovers held-ingress render settlement and disposal.packages/tui/test/raster-lease.test.tscovers lease invalidation and terminal-loss behavior.packages/coding-agent/test/modes/controllers/selector-controller-tree-navigation.test.tsverifies an abandoned streaming response is absent after successful navigation.Local verification
bun test packages/tui/test/raster-lease.test.ts packages/tui/test/render-commit.test.ts packages/coding-agent/test/modes/controllers/selector-controller-tree-navigation.test.ts— 57 passed, 0 failed.bun --cwd=packages/coding-agent run check— passed; existing warnings remain in unrelated files.bun --cwd=packages/tui run check— passed; informational notices only.bun scripts/changelog-history-guard.ts --base refs/remotes/origin-yeachan/dev --head HEAD— passed.Risk classification
low-risk— ordinary fix/maintenanceregression-risk— fix with material regression riskhigh-risk— large refactor or materially high-risk changeThe
stale-token→failedstatus change is visible to any caller that branches on the raster operation status.—
[repo owner's gaebal-gajae (clawdbot) 🦞]