fix(coding-agent): retain deferred shell output - #3697
Conversation
7beb64d to
7f2380c
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — terminal review of PR #3697 at head 7f2380c83ac2ea1178d5d81ac8023028a4b7a775.
The fetched review base is origin/dev at c1bf3be50e2cf0dfdf51b57eb43f7a99521c94b8; this PR's merge base is b40bc271502a0802e212e5538c5d0e8436643029, so the PR is stale and must be rebased onto current dev before approval.
Hostile deferred-shell-output finding:
-
Blocking race:
handleBashCommand()setsrecorded = trueonly afterexecuteBash()resolves. If the agent emitsagent_endwhile a deferred bash command is still running,flushPendingBashComponents()intentionally keeps the currentctx.bashComponentinpendingBashComponents. If that command then rejects, the catch path completes the component but leavesrecorded === false; the final promotion condition therefore never detaches it frompendingMessagesContaineror adds it tochatContainer. The error/result is stranded in the pending area after the agent is idle, and there is no later agent-end flush to deliver it. The new test exercises a rejection, but it manually callsflushPendingBashComponents()after awaiting the command, so it does not cover this ordering race. Promote the component on rejection as well, or add a completion-time delivery path that handles both success and failure after the terminal flush. -
REQUEST_CHANGES / stale base (blocking): Rebase onto
c1bf3be50e2cf0dfdf51b57eb43f7a99521c94b8, then rerun the deferred-output and agent-end lifecycle tests against that base.
I did not merge or mutate the PR. Terminal verdict for this exact head: REQUEST_CHANGES.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head red-team review for 7f2380c.
No implementation defect found in the deferred shell-output patch after reviewing the ownership/flush paths and running:
- bun test packages/coding-agent/test/modes/controllers/bash-command.test.ts packages/coding-agent/test/agent-session-user-shortcut-hooks.test.ts (5 pass, 58 assertions)
- bun --cwd=packages/coding-agent run check (Biome passed; local TypeScript process was interrupted by the shell runner after starting)
REQUEST CHANGES: GitHub reports this exact head as CONFLICTING/DIRTY against dev. The PR base is b40bc27, while current dev is be3940a. Rebase or otherwise update the PR onto current dev, rerun CI, and request review again. This is a mergeability blocker, not a code finding.
Review receipt: gajae.pr-review-verdict.v1 needs-changes sha256:7f2380c83ac2ea1178d5d81ac8023028a4b7a775 reviewer:Yeachan-Heo evidence:terminal-exact-head-red-team
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Corrected exact-head red-team review for 7f2380c.
BLOCKING CODE FINDING in packages/coding-agent/src/modes/controllers/command-controller.ts: handleBashCommand() sets recorded = true only on successful executeBash() resolution. For a deferred command that is still running when the agent-end flush runs, flushPendingBashComponents() correctly leaves the current component pending. If executeBash() then rejects, the catch path calls setComplete() but leaves recorded false; the completion path therefore does not detach/promote it, and no later agent-end flush is guaranteed. The failed command remains stranded in pendingMessagesContainer after the agent is idle. The added rejection test manually calls flushPendingBashComponents() after awaiting the command, so it does not exercise this late-rejection ordering. Promote completed deferred components on both success and failure, or add a completion-time delivery path that handles rejection after the terminal flush.
The exact head is also stale/conflicting: base b40bc27 versus current dev be3940a. Rebase, add the late-rejection regression, rerun CI, and request review again.
Review receipt: gajae.pr-review-verdict.v1 needs-changes sha256:7f2380c83ac2ea1178d5d81ac8023028a4b7a775 reviewer:Yeachan-Heo evidence:terminal-exact-head-red-team
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Corrected exact-head red-team review for 7f2380c.
BLOCKING CODE FINDING in packages/coding-agent/src/modes/controllers/command-controller.ts: handleBashCommand() sets recorded = true only on successful executeBash() resolution. For a deferred command still running when the agent-end flush runs, flushPendingBashComponents() leaves the current component pending. If executeBash() then rejects, the catch path calls setComplete() but leaves recorded false; the completion path does not detach/promote it, and no later agent-end flush is guaranteed. The failed command remains stranded in pendingMessagesContainer after the agent is idle. The added rejection test manually calls flushPendingBashComponents() after awaiting the command, so it misses this late-rejection ordering. Promote completed deferred components on both success and failure, or add a completion-time delivery path handling rejection after terminal flush.
The exact head is also stale/conflicting: base b40bc27 versus current dev be3940a. Rebase, add the late-rejection regression, rerun CI, and request review again.
Review receipt: gajae.pr-review-verdict.v1 needs-changes sha256:7f2380c83ac2ea1178d5d81ac8023028a4b7a775 reviewer:Yeachan-Heo evidence:terminal-exact-head-red-team
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head red-team review for 7f2380c83ac2ea1178d5d81ac8023028a4b7a775 (single commit on merge base b40bc2715; current dev tip is 54b14cb6e).
Verdict: REQUEST_CHANGES
Blocking code finding — late-rejection race in handleBashCommand()
command-controller.ts:1158-1181: recorded is set to true only after executeBash() resolves. For a deferred command still running when the agent emits agent_end, flushPendingBashComponents() intentionally retains the active ctx.bashComponent in pendingBashComponents (the component === ctx.bashComponent guard at ui-helpers.ts:1262). If executeBash() then rejects:
- The
catchpath callsbashComponent.setComplete(undefined, false)butrecordedstaysfalse. - The promotion condition at line 1174 (
isDeferred && recorded && !this.ctx.session.isStreaming && pendingIndex !== -1) never fires, so the component is never detached frompendingMessagesContaineror added tochatContainer. ctx.bashComponentis cleared at line 1179, so the nextflushPendingBashComponents()will move it — but no later agent-end flush is guaranteed if the agent is already idle.
The failed command is stranded in the pending area after the agent is idle.
Verified empirically — I wrote a focused reproduction at the PR head that constructs a deferred command, drives the agent idle (isStreaming = false), runs flushPendingBashComponents() (which keeps it pending), then rejects executeBash():
RESULT: strandedInPending=true promotedToChat=false pendingBashComponentsLen=1 bashComponentUndefined=true
The component is stranded in pendingMessagesContainer, never promoted to chatContainer, and ctx.bashComponent is already cleared.
The added rejection test (failing-command scenario) manually calls flushPendingBashComponents() after awaiting the command, so it exercises the normal ordering — not this late-rejection-after-flush race.
Fix direction: promote completed deferred components on both success and failure (drop the recorded gate from the promotion condition, or add a catch-path promotion), and add a regression that rejects executeBash() after the agent-end flush has already run.
Mergeability — stale base (blocking)
GitHub reports the PR as CONFLICTING/DIRTY. I confirmed via a real git merge --no-commit against current dev (54b14cb6e) that the textual content auto-merges cleanly (zero conflict markers; only CHANGELOG.md context shifts and agent-session.ts auto-merges). The DIRTY status is a stale PR metadata base reference (b40bc2715), not a textual conflict. Rebase or update the PR branch to refresh the merge base, then rerun CI.
What is correct
- The
detachPendingExecutionComponents/restorePendingExecutionComponentspair inui-helpers.tscorrectly preserves in-flight components acrossrenderInitialMessages()andupdatePendingMessagesDisplay()rebuilds. - The
flushPendingBashComponents()active-component retention guard is the right design for the still-running case. - The
agent-session.tsdual flush sites (rawagent_endin#handleAgentEventat line 3744, and pre-publish inpublish()at line 2530) correctly persist deferred results before the public terminal boundary. - The
agent-session-user-shortcut-hooks.test.tsintegration test correctly verifies that results recorded during rawagent_endare flushed before the public event.
Verification run
bun test packages/coding-agent/test/modes/controllers/bash-command.test.ts— 1 pass, 38 assertions (PR head)- Focused late-rejection reproduction — defect confirmed (see above)
tsc --noEmiton changed source files — no errors incommand-controller.ts,event-controller.ts,ui-helpers.ts,agent-session.tsgit merge --no-commit origin/dev pr-3697-head— clean auto-merge, zero conflict markers
I did not merge or mutate the PR.
Review receipt: gajae.pr-review-verdict.v1 needs-changes sha256:7f2380c83ac2ea1178d5d81ac8023028a4b7a775 reviewer:Yeachan-Heo evidence:terminal-exact-head-red-team
7f2380c to
1a99ba4
Compare
|
Resolved. Rebased the PR onto the latest dev (54b14cb), fixed the deferred-shell rejection race so a command that settles after agent_end is promoted from pending to chat on both success and failure, and updated the regression test to cover agent_end before rejection. Validation: all 5 targeted tests pass and the packages/coding-agent check passes. The PR now reports MERGEABLE / CLEAN. |
|
Exact-head follow-up against Focused verification passes (
Required direction: introduce an explicit execution-publication phase/barrier (not Terminal admission also remains serialized behind unresolved #3742/#3743 |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head re-review at new head 1a99ba4 (base 54b14cb; MERGEABLE/CLEAN).
Verdict: MERGE_READY
My prior CHANGES_REQUESTED at 7f2380c identified a blocking late-rejection race: handleBashCommand set recorded=true only on success, so a deferred command still running when agent_end flush runs would strand on rejection.
Fix verified
New commit 1a99ba4 (deliver rejected deferred shell output) removes the recorded flag entirely. The promotion condition at command-controller.ts now reads: isDeferred && !this.ctx.session.isStreaming && pendingIndex !== -1 — no recorded gate. Both success and rejection paths reach the promotion block, so a late-rejected deferred command is promoted to chatContainer instead of stranded in pendingMessagesContainer.
I verified the fix empirically with the same reproduction that showed strandedInPending=true at the old head:
- strandedInPending=false
- promotedToChat=true
Verification
- bun test bash-command.test.ts agent-session-user-shortcut-hooks.test.ts — 5 pass, 60 assertions
- Late-rejection reproduction — defect resolved
- biome check command-controller.ts — clean
- tsc --noEmit — no errors in command-controller.ts
- CI — 16 pass, 6 skip
- mergeable: MERGEABLE/CLEAN
Review receipt: gajae.pr-review-verdict.v1 merge-ready sha256:1a99ba419897d20ca00ad27d9553cb9bace6f1ac reviewer:Yeachan-Heo evidence:terminal-exact-head-red-team
|
Current- Verification on that integration passed:
Independent current-base lifecycle review remains BLOCKED on two HIGH exact-once durability/publication defects:
Please repair these on the existing contributor-owned PR and request exact-head/current-dev re-review. No duplicate PR was opened. |
|
Current-dev corrected candidate is published without mutating the contributor branch or opening a duplicate PR:
The candidate persists deferred Bash results before agent state and public terminal publication, reconciles every uncertain append by exact entry id including retry-committed outcomes, retains unrelated running displays, retires only the matching persisted live display during rebuild, and restores the exact terminal event/resource lease after publication failure for bounded retry. Adoption still requires the contributor to update this PR from the published candidate or an explicit maintainer takeover decision. |
|
Exact current-dev corrected candidate refresh:
The candidate resolves the published exact-once blockers: durable-first append, exact-entry reconciliation including retry-committed uncertainty, exact pending dequeue, stable live-display association, matching-only rebuild retirement, unrelated running-display retention, and retryable terminal publication preserving the event/resource lease. No duplicate PR was opened and contributor ownership remains intact. Adoption requires updating this PR from the published candidate or an explicit maintainer takeover decision. |
probepark
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact contributor head 1a99ba419897d20ca00ad27d9553cb9bace6f1ac.
The current contributor head still lacks the durable-first/exact-entry reconciliation and matching-only rebuild retirement required by the published findings. Merging it can create non-durable agent-state ghosts, duplicate retained Bash entries after uncertain append, and persisted-plus-live duplicate visibility during transcript rebuild.
A corrected exact-current-dev candidate is published at probepark/fix/issue-3639-reviewed-candidate head f10bb050948ca5571bbced0acfe246194527ba49. It passed 10 focused tests / 50 assertions, packages/coding-agent check, exact-range diff check, and independent reviews 138-RebasedCandidateAudit plus 139-G004ChangelogFinalReview (CLEAR / APPROVE).
Please update this contributor-owned PR from that candidate and request exact-head re-review. No duplicate PR has been opened.
1a99ba4 to
9f350aa
Compare
|
Applied the published corrected candidate to this existing contributor-owned PR and rebased it onto current
A fresh independent exact-head audit found the applied candidate is still blocked:
Required repair: retain the uncertain append entry id on the pending record until reconciliation proves the prior append absent, and serialize/join terminal publication retries through one shared promise.
|
Yeachan-Heo
left a comment
There was a problem hiding this comment.
SUPPORTED_SUPERSESSION — signed maintainer reconciliation, exact head 9f350aa0f638935717414f4b95e7820bcf19550f
Branch ownership. This PR's head is sj0618/gajae-code:fix/issue-3639-deferred-shell-output (cross-repo, isCrossRepository: true). sj0618 is the sole mutation owner of record — the only account with push access to this branch. probepark/fix/issue-3639-reviewed-candidate is a non-authoritative reference branch used to publish reviewed diffs without writing to the contributor's branch; it does not carry ownership and no PR was opened from it, consistent with the no-duplicate-mutation convention already followed across this thread.
Reconciliation performed. sj0618 already applied the current-dev-corrected candidate (f10bb050948ca5571bbced0acfe246194527ba49, CLEAR/APPROVE per probepark's 138-RebasedCandidateAudit + 139-G004ChangelogFinalReview) to this PR and rebased onto dev@162313f98816e3814b03519a9517dc1ea58fe4ba, producing the current exact head 9f350aa0 with an identical patch ID (19be159c…) and green CI. That satisfies the "update/rebase the contributor PR" reconciliation this backlog pass was checking for — no further rebase or candidate-adoption action is outstanding.
Standing verdict at this exact head. The same commit's own follow-up audit (tag gajae.pr-review-verdict.v1 merge-blocked sha256:9f350aa0f638935717414f4b95e7820bcf19550f reviewer:architect evidence:local-diff-review) already found two live defects and is the governing verdict for 9f350aa0:
- HIGH — a committed append whose recovery attempt rejects can lose its exact uncertain entry id, risking a duplicate Bash-result append on the next terminal-publication attempt.
- MEDIUM — concurrent publication waiters can diverge when one starts a retry while another rethrows the stale failure.
- Missing regression coverage for committed-append/recovery-rejection, concurrent waiters, partial dequeue, and lease settlement.
I've independently checked this verdict is well-formed (specific mechanisms, not vague) and consistent with the durability/reconciliation constraints this PR's own commit message declares (current_append uncertainty reconciles by exact entry id before retry), so I am not re-deriving a duplicate finding — I'm signing and superseding the stale CHANGES_REQUESTED review pinned to 1a99ba419 (two commits behind current head) with this one. That prior review no longer reflects the applied state and should not gate further action.
Resuming existing owner. @sj0618 — please apply the required repair from the standing verdict directly on fix/issue-3639-deferred-shell-output (retain the uncertain append entry id on the pending record until reconciliation proves the prior append absent; serialize/join terminal-publication retries through one shared promise; add the four missing regression cases), then request re-review at the new exact head. No duplicate PR, merge, or CI control action is being taken here.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
9f350aa to
5be7c7b
Compare
|
Resolved the standing exact-head blockers directly on the existing contributor branch and rebased onto current New exact head: Repairs:
Verification:
Requesting maintainer exact-head re-review; no duplicate PR was opened. |
|
REQUEST_CHANGES Exact-current hostile re-review
The exact-head diff has the correct architectural direction and appears to address the standing late-rejection, exact uncertain-entry reconciliation, partial FIFO dequeue, shared publication waiter, transcript rebuild identity, and settlement/lease findings. This batch found no new code blocker in that diff. The integration gate still blocks approval. Since merge base Current
— |
Two artifacts made a rebase onto `dev` fail for reasons unrelated to the change being rebased, and both are removable rather than manageable. `packages/*/CHANGELOG.md merge=union` only ever applied locally. GitHub does not honour the driver when it computes mergeability, so it reported `dirty` on every PR that touched a CHANGELOG while git merged them cleanly: 15 of 24 open PRs marked CONFLICTING merged with zero conflicts under the repo's own attributes, and disabling the union line reproduced GitHub's answer exactly (100% agreement, 24/24, no exceptions). Worse than the phantom, union never conflicts -- it concatenates both sides of an overlapping hunk. Release commits insert `## [X.Y.Z]` directly beneath the surviving `## [Unreleased]` heading, so a branch that added entries under Unreleased overlaps exactly that region and its entries land *inside* the version that already shipped, with no marker and nothing visible in the PR diff. Reproduced on Yeachan-Heo#3697: its entry resolves under `## [0.12.12]`. An audit of `dev` finds 35 entries already sitting in released sections across 5 packages, including one authored 2026-08-06 filed under `## [0.12.8] - 2026-08-02`. `docs-index.generated.ts` inlines each doc's full body onto a single source line, so two edits to the same doc produce a whole-line conflict git cannot three-way merge. It is already listed in .gitignore -- and also tracked, which makes the ignore rule inert, because 23983ef added both in one commit. 6 of the 10 real conflicts among open PRs are this file; 4 PRs are blocked by it alone. It churned in 31 commits in 30 days. Untracking is safe: the root `prepare` hook regenerates it on every `bun install` (verified, including `--frozen-lockfile`), every CI job that compiles or tests runs that install, and `npm pack` still ships it because the `files` allowlist outranks ignore rules (verified three ways -- 1436 files packed either way). A real conflict an author resolves is strictly better than a silent misfile, so no replacement merge driver is introduced. Constraint: the published tarball must keep the generated docs index Rejected: keep union, add a CI guard against released-section edits | leaves the phantom conflicts and the forced rebases in place Rejected: emit one line per paragraph so union can merge | shrinks the conflict without removing it, and GitHub still disagrees Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test scripts/check-public-version-sync.test.ts packages/coding-agent/test/docs-index-lazy.test.ts packages/coding-agent/test/{bot-integration-docs,package-files,docs-utility-surface-cleanup}.test.ts (34 pass); both new guards verified non-vacuous by re-adding the file; bun run check:tools and packages/coding-agent check:types clean Not-tested: a full release publish
Two artifacts made a rebase onto `dev` fail for reasons unrelated to the change being rebased, and both are removable rather than manageable. `packages/*/CHANGELOG.md merge=union` only ever applied locally. GitHub does not honour the driver when it computes mergeability, so it reported `dirty` on every PR that touched a CHANGELOG while git merged them cleanly: 15 of 24 open PRs marked CONFLICTING merged with zero conflicts under the repo's own attributes, and disabling the union line reproduced GitHub's answer exactly (100% agreement, 24/24, no exceptions). Worse than the phantom, union never conflicts -- it concatenates both sides of an overlapping hunk. Release commits insert `## [X.Y.Z]` directly beneath the surviving `## [Unreleased]` heading, so a branch that added entries under Unreleased overlaps exactly that region and its entries land *inside* the version that already shipped, with no marker and nothing visible in the PR diff. Reproduced on Yeachan-Heo#3697: its entry resolves under `## [0.12.12]`. An audit of `dev` finds 35 entries already sitting in released sections across 5 packages, including one authored 2026-08-06 filed under `## [0.12.8] - 2026-08-02`. `docs-index.generated.ts` inlines each doc's full body onto a single source line, so two edits to the same doc produce a whole-line conflict git cannot three-way merge. It is already listed in .gitignore -- and also tracked, which makes the ignore rule inert, because 23983ef added both in one commit. 6 of the 10 real conflicts among open PRs are this file; 4 PRs are blocked by it alone. It churned in 31 commits in 30 days. Untracking is safe: the root `prepare` hook regenerates it on every `bun install` (verified, including `--frozen-lockfile`), every CI job that compiles or tests runs that install, and `npm pack` still ships it because the `files` allowlist outranks ignore rules (verified three ways -- 1436 files packed either way). A real conflict an author resolves is strictly better than a silent misfile, so no replacement merge driver is introduced. Constraint: the published tarball must keep the generated docs index Rejected: keep union, add a CI guard against released-section edits | leaves the phantom conflicts and the forced rebases in place Rejected: emit one line per paragraph so union can merge | shrinks the conflict without removing it, and GitHub still disagrees Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test scripts/check-public-version-sync.test.ts packages/coding-agent/test/docs-index-lazy.test.ts packages/coding-agent/test/{bot-integration-docs,package-files,docs-utility-surface-cleanup}.test.ts (34 pass); both new guards verified non-vacuous by re-adding the file; bun run check:tools and packages/coding-agent check:types clean Not-tested: a full release publish
Two artifacts made a rebase onto `dev` fail for reasons unrelated to the change being rebased, and both are removable rather than manageable. `packages/*/CHANGELOG.md merge=union` only ever applied locally. GitHub does not honour the driver when it computes mergeability, so it reported `dirty` on every PR that touched a CHANGELOG while git merged them cleanly: 15 of 24 open PRs marked CONFLICTING merged with zero conflicts under the repo's own attributes, and disabling the union line reproduced GitHub's answer exactly (100% agreement, 24/24, no exceptions). Worse than the phantom, union never conflicts -- it concatenates both sides of an overlapping hunk. Release commits insert `## [X.Y.Z]` directly beneath the surviving `## [Unreleased]` heading, so a branch that added entries under Unreleased overlaps exactly that region and its entries land *inside* the version that already shipped, with no marker and nothing visible in the PR diff. Reproduced on #3697: its entry resolves under `## [0.12.12]`. An audit of `dev` finds 35 entries already sitting in released sections across 5 packages, including one authored 2026-08-06 filed under `## [0.12.8] - 2026-08-02`. `docs-index.generated.ts` inlines each doc's full body onto a single source line, so two edits to the same doc produce a whole-line conflict git cannot three-way merge. It is already listed in .gitignore -- and also tracked, which makes the ignore rule inert, because 23983ef added both in one commit. 6 of the 10 real conflicts among open PRs are this file; 4 PRs are blocked by it alone. It churned in 31 commits in 30 days. Untracking is safe: the root `prepare` hook regenerates it on every `bun install` (verified, including `--frozen-lockfile`), every CI job that compiles or tests runs that install, and `npm pack` still ships it because the `files` allowlist outranks ignore rules (verified three ways -- 1436 files packed either way). A real conflict an author resolves is strictly better than a silent misfile, so no replacement merge driver is introduced. Constraint: the published tarball must keep the generated docs index Rejected: keep union, add a CI guard against released-section edits | leaves the phantom conflicts and the forced rebases in place Rejected: emit one line per paragraph so union can merge | shrinks the conflict without removing it, and GitHub still disagrees Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test scripts/check-public-version-sync.test.ts packages/coding-agent/test/docs-index-lazy.test.ts packages/coding-agent/test/{bot-integration-docs,package-files,docs-utility-surface-cleanup}.test.ts (34 pass); both new guards verified non-vacuous by re-adding the file; bun run check:tools and packages/coding-agent check:types clean Not-tested: a full release publish
Deferred composer shell commands could reach terminal publication before their transcript append became durable, and transcript rebuilds could render both persisted and still-live copies. Persist before publication, reconcile uncertain appends by exact entry id, retain unrelated running displays, and keep failed terminal publication retryable. Lore-id: 3639-deferred-shell-durability Constraint: session transcript append precedes terminal agent_end publication Constraint: current_append uncertainty reconciles by exact entry id before retry Constraint: transcript rebuild retires only the matching persisted display Rejected: append agent state before durable transcript | creates ghosts and duplicates Confidence: high Scope-risk: medium Reversibility: easy Co-authored-by: probe <re2rar@gmail.com>
5be7c7b to
3a859cf
Compare
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
Two artifacts made a rebase onto `dev` fail for reasons unrelated to the change being rebased, and both are removable rather than manageable. `packages/*/CHANGELOG.md merge=union` only ever applied locally. GitHub does not honour the driver when it computes mergeability, so it reported `dirty` on every PR that touched a CHANGELOG while git merged them cleanly: 15 of 24 open PRs marked CONFLICTING merged with zero conflicts under the repo's own attributes, and disabling the union line reproduced GitHub's answer exactly (100% agreement, 24/24, no exceptions). Worse than the phantom, union never conflicts -- it concatenates both sides of an overlapping hunk. Release commits insert `## [X.Y.Z]` directly beneath the surviving `## [Unreleased]` heading, so a branch that added entries under Unreleased overlaps exactly that region and its entries land *inside* the version that already shipped, with no marker and nothing visible in the PR diff. Reproduced on #3697: its entry resolves under `## [0.12.12]`. An audit of `dev` finds 35 entries already sitting in released sections across 5 packages, including one authored 2026-08-06 filed under `## [0.12.8] - 2026-08-02`. `docs-index.generated.ts` inlines each doc's full body onto a single source line, so two edits to the same doc produce a whole-line conflict git cannot three-way merge. It is already listed in .gitignore -- and also tracked, which makes the ignore rule inert, because 23983ef added both in one commit. 6 of the 10 real conflicts among open PRs are this file; 4 PRs are blocked by it alone. It churned in 31 commits in 30 days. Untracking is safe: the root `prepare` hook regenerates it on every `bun install` (verified, including `--frozen-lockfile`), every CI job that compiles or tests runs that install, and `npm pack` still ships it because the `files` allowlist outranks ignore rules (verified three ways -- 1436 files packed either way). A real conflict an author resolves is strictly better than a silent misfile, so no replacement merge driver is introduced. Constraint: the published tarball must keep the generated docs index Rejected: keep union, add a CI guard against released-section edits | leaves the phantom conflicts and the forced rebases in place Rejected: emit one line per paragraph so union can merge | shrinks the conflict without removing it, and GitHub still disagrees Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test scripts/check-public-version-sync.test.ts packages/coding-agent/test/docs-index-lazy.test.ts packages/coding-agent/test/{bot-integration-docs,package-files,docs-utility-surface-cleanup}.test.ts (34 pass); both new guards verified non-vacuous by re-adding the file; bun run check:tools and packages/coding-agent check:types clean Not-tested: a full release publish
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review of head 474ccae1f. The standing verdict's three items are all addressed; I verified each rather than reading the diff summary.
The standing blockers are closed
That verdict named:
- HIGH — a committed append whose recovery attempt rejects can lose its exact uncertain entry id, risking a duplicate Bash result.
- MEDIUM — concurrent publication waiters diverging when one retries while another rethrows the stale failure.
- Missing regression coverage for committed-append/recovery-rejection, concurrent waiters, partial dequeue, and lease settlement.
All four coverage gaps now have named cases, and they map one-to-one onto the findings rather than being adjacent tests:
reconciles a committed deferred shell append after recovery rejects <- (1)
shares terminal retry settlement between concurrent public waiters and emits once <- (2)
dequeues only durable deferred shell records after a partial failure <- partial dequeue
retries a failed terminal publication without losing the deferred shell result
reconciles a retry that commits after the first append was proven absent
reconciles an immediate shell result that committed before append failure
reconciles a deferred shell result that committed before append failure
Executed on this exact head, in a hermetic environment (see the caveat below):
bun test packages/coding-agent/test/agent-session-user-shortcut-hooks.test.ts \
packages/coding-agent/test/modes/controllers/bash-command.test.ts
-> 13 pass, 0 fail
-t "recovery rejects" -> 1 pass, 8 expect() calls
-t "concurrent public waiters" -> 1 pass
The two targeted cases pass individually, so they are not passing by side effect of an earlier test in the file. emits once in the concurrent-waiter case is the right assertion — it pins the divergence (a second emission) rather than merely that both waiters resolved.
Changelog recovered
This PR was among the twelve damaged when I removed merge=union in #3932 (see #3942). It is fixed here — packages/coding-agent/CHANGELOG.md is back to 312,554 bytes, restored in the head commit. Nothing further needed. Merges cleanly into current dev.
Environment caveat worth knowing
My first run of the adjacent registry suites showed failures that were not from this PR — model-registry.test.ts reads provider API-key environment variables from the host shell, so a developer with credentials configured sees red on current dev too. I filed that as #3945. It does not touch this PR's suites, but if you see unexplained red locally in that area, that is why.
One remaining note
The description says the HIGH item risked "a duplicate Bash result". The test asserts reconciliation after recovery rejection, which covers the entry-id retention — but I could not find an assertion that the transcript ends up with exactly one Bash result in that scenario. If the observable harm was duplication, asserting the count directly (not just that reconciliation happened) would pin the user-visible property rather than the mechanism. Non-blocking, and cheap to add.
gajae.pr-review-verdict.v1 merge-approved sha256:474ccae1f9e4d86e952b3792d03dafe38a234910 reviewer:architect evidence:13 pass / 0 fail on this head with *API_KEY* unset and isolated HOME; the two named blocker cases pass individually; CHANGELOG restored to 312554 bytes; merge-tree vs origin/dev clean
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
What
agent_endpublication and reconcile uncertain appends by exact entry id.Why
Fixes #3639.
Supersedes #3660. This existing contributor-owned PR contains the reviewed candidate plus the exact-head durability/publication repairs requested in maintainer review, rebased onto current
dev. Contributorsj0618remains the primary commit author and candidate authorproberemains co-author.Testing
bun test packages/coding-agent/test/modes/controllers/bash-command.test.ts packages/coding-agent/test/agent-session-user-shortcut-hooks.test.ts— 13 passed, 71 assertionsbun test --rerun-each 5 packages/coding-agent/test/agent-session-user-shortcut-hooks.test.ts— 55 passed, 285 assertionsbun --cwd=packages/coding-agent run check— passedgit diff --check upstream/dev...HEAD— passedGJC verdict
dev## [Unreleased]