test(acp): gate the advertised session lifecycle surface in CI - #4571
test(acp): gate the advertised session lifecycle surface in CI#4571probepark wants to merge 3 commits into
Conversation
137f183 to
15387ac
Compare
|
Maintainer review/fix-forward ownership is active for this PR. The initial CI failures are confirmed and scoped:
A dedicated worktree has reconstructed the single substantive commit on current — |
|
Exact-head CI failure classified at The new test itself did not reach its assertions in shard 4. The fresh-process harness explicitly enumerated So the current wiring excludes the file from Bun's default discovery but not from the repository's fresh-process shard inventory. Meanwhile the intended dedicated ACP lifecycle task is separately scheduled. This is a deterministic duplicate/inconsistent routing defect, not CI noise. The active fix-forward lane is reconciling to this new exact head, then will centralize the exclusion in the canonical shard/discovery contract while preserving the dedicated task, add routing regressions, rerun the exact shard path plus the 12-test suite/package/workflow checks, and update the existing branch with an exact lease. Prior local head and review evidence are stale after your head update. — |
|
The dedicated ACP lifecycle task failed for the same root cause as shard 4; no lifecycle assertion ran. Job That task runner invoked plain: and inherited the new global The fix-forward scope is now explicit: centralize the discovery contract so ordinary/package shards exclude this expensive suite while the dedicated task receives a validated runnable argv, and pin both paths in planner/task-executor regressions. Ad hoc local commands are not sufficient; both exact CI command paths must pass before the branch is updated. — |
15387ac to
3effc78
Compare
|
Supersession evidence — replacement head pushed (all prior run evidence stale)
Why the rewrite was needed — both red jobs were one defect, not a flaky test. Repair (commit Local verification at
Note: Requesting independent exact-head review from @probepark (author) and @HaD0Yun on — |
|
@HaD0Yun — independent exact-head review requested for PR #4571.
Why you: the non-author merge gate requires an authenticated approving GitHub review on exactly this head. @probepark is the PR author and cannot satisfy it; the owner (gaebal-gajae lane) must not self-approve. Please review and, if it holds, submit APPROVED on What to review (7 files, +651/−4):
Defect history (both original reds were one bug): bunfig prunes the file from Bun discovery; plain Evidence at this head: both formerly-failing jobs green on replacement run 31863813716 (cancelled only by newer body-edit runs, 42 successes / 5 skips / 0 product failures at cancellation); live run — |
|
OWNER_CONFIRMATION_REQUIRED — exact-head human-review hold
Terminal CI state — run 31865324220 on exactly
What is required to unblock (human-only): an authenticated APPROVED GitHub review on exactly Review-request evidence: issuecomment-5300598643. Repair/supersession evidence: issuecomment-5300476410. — |
3effc78 to
4f2b972
Compare
|
Supersession — freshness rebase after dev advanced (#4539 merged); all prior head/run evidence stale
Reconstruction: both PR commits ( Push: Focused validation at
PR body verdict updated to the exact new digest (exactly one line). Stale runs 31863813711/16, 31864691262, 31865324220 are no longer current evidence; the run triggered by this push is the authority. @HaD0Yun — the prior exact-head review request is superseded by this freshness rebase; please review and, if it holds, submit APPROVED on exactly — |
|
SHA correction on the supersession comment above: the exact head is [repo owner's gaebal-gajae (clawdbot) 🦞] |
Current-dev blocker — exact-head product classificationPR head Replacement run This is the same failure as current-dev push run No duplicate browser fix will be added here. The remaining exact-head jobs continue to terminal; after #4574 restores green — |
|
Paused on an upstream base regression — exact dependency evidence Current #4571 state: head Run 31867707713, job Dependency: #4575 (fix for #4574) must merge to — |
4f2b972 to
ff9f896
Compare
Recovery supersession recordSigned-off-by: The sole authoritative PR #4571 review tuple is now:
Superseded and non-authoritative: submitted/recovery heads Only a fresh authenticated non-author |
Exact-head repair supersession recordSigned-off-by: Run 31872943884, shard-4 job 94984825112, proved the squash-loss root cause: the ACP lifecycle file was correctly pruned from default Bun discovery, but the prior canonical dedicated-task routing and fresh-process shard exclusion had been omitted from Superseded tuple:
Sole authoritative tuple:
The restored repair includes Only fresh checks and a fresh authenticated non-author |
79bfe83 to
ebd5683
Compare
Base-move exact-head supersession recordSigned-off-by: PR #4577 moved Superseded tuple:
Sole authoritative tuple:
Both rebased commits have stable patch IDs byte-equivalent to the prior reviewed patches. Fresh local evidence on the authoritative tuple: planner/harness Do not transplant #4575 into this PR. Only fresh exact-head CI and a fresh authenticated non-author approval for this tuple may replace |
Exact-head CI terminal classificationSigned-off-by: Authoritative tuple Every red is classified:
The repaired fresh-process inventory executed 175 ordinary files and did not schedule |
Current external merge blockerSigned-off-by: #4571 has completed its current exact-head verification and has no unclassified PR-induced source failure. Progress to the required final rebase is externally blocked on #4575. Current dependency facts:
#4571 reviewer request remains with |
ebd5683 to
e4e40d3
Compare
Latest-dev exact-head supersession recordSigned-off-by: PR #4579 advanced Superseded tuple:
Sole authoritative tuple:
Both rebased commit patch IDs remain byte-equivalent to the original ACP lifecycle and canonical routing repairs. Fresh verification on this tuple: planner/harness #4575 is not copied into this PR and remains the sole browser-regression authority. Another final rebase is mandatory if #4575 or any other change advances |
Exact-head inherited failure attributionRun 31877123400, shard-3 job 94995122660, has exactly one test-file failure:
This is the byte-for-byte current- #4571's exact diff is confined to Signed-off-by: |
Exact evidence-backed dependency holdSigned-off-by: #4571 current authoritative tuple remains:
Authoritative run 31877123400 is terminal: 38 success, 6 skipped, 4 failure. The failures are fully bounded: expected The external repair authority has advanced independently:
Per one-item authority, this lane cannot promote or merge #4575. Per #4571 freshness rules, this lane must not chase intermediate unrelated baseline movement; it must rebase once #4575 merges to green |
e4e40d3 to
b49fa28
Compare
Browser-fixed dev supersession recordSigned-off-by: #4575 merged and advanced Superseded tuple:
Sole authoritative tuple:
Both rebased commit patch IDs remain byte-equivalent to the original ACP lifecycle and canonical routing repairs. Fresh local evidence: planner/harness Only fresh exact-head CI and a fresh authenticated non-author |
Current exact-head authoritySigned-off-by:
The current bootstrap red is the expected governance block from — |
Current-dev freshness supersessionSigned-off-by: PR #4580 advanced Sole authoritative tuple:
Both rebased commit patch IDs remain byte-equivalent to the original repairs. Fresh local evidence: planner/harness An authenticated write-authorized non-author exact-head — |
Live sole-owner recovery bindingSigned-off-by: The SDK session reset has been reconciled without changing source or authority. This owner lane is again the sole live maintainer/fix-forward controller for PR #4571, bound to the current GitHub state read immediately before this record:
No stale/cancelled run will be rerun. Any head or base change invalidates this binding, CI, reviews, and verdict. Merge remains forbidden until run 31881306437 reaches product-green terminal state and HaD0Yun supplies an authenticated non-author — |
Exact-head product-green terminal holdSigned-off-by: The recovered sole-owner lane has driven replacement run 31881306437 to terminal on the still-current exact tuple:
The only remaining authority gate is human-only: This is an owner-controlled hold, not a CI retry request. Run 31881306437 is terminal and will not be rerun. No source mutation is pending. Any head or base movement invalidates this evidence and requires a new tuple; otherwise, receipt of the exact-head approval authorizes immediate verdict promotion and squash merge to — |
3c2c03f to
7eed8e5
Compare
Latest-dev exact-head supersession after recovered holdSigned-off-by:
Superseded tuple:
Sole authoritative tuple:
Both commit patch IDs are unchanged across the rebase ( Push-trigger run 31883204788 captured the old body and is stale; it is not merge authority and will not be rerun. Only run 31883223701 plus a new authenticated non-author — |
|
@HaD0Yun — fresh independent exact-head review requested for PR #4571 after the latest Authoritative review tuple:
All earlier approval requests are stale because their heads are stale. The author — |
Exact-head product-green owner-controlled holdSigned-off-by: The current authoritative tuple is stable and replacement run 31883223701 is terminal:
No source or CI defect remains to fix forward, and no stale/cancelled run will be rerun. The only remaining gate is an authenticated non-author APPROVED GitHub review from This is the fresh owner-controlled hold for the current tuple. Any base or head movement invalidates it. Otherwise, receipt of the exact-head approval authorizes immediate canonical verdict promotion followed by squash merge to — |
Fresh authoritative-turn product-green evidenceSigned-off-by: Live authority was re-read before this mutation and remains exact:
@HaD0Yun and @IYENTeam are now both requested reviewers and each currently has No CI rerun is requested. On the first effective exact-head approval, this lane will atomically promote the sole verdict to — |
7eed8e5 to
f34f1fd
Compare
Current-dev advancement supersessionSigned-off-by:
Sole authoritative tuple:
The two PR patch IDs remain exactly unchanged ( Push-trigger run 31886837219 captured the pre-update body and is stale; it will not be rerun. @HaD0Yun @IYENTeam — only an authenticated APPROVED review on exactly — |
f34f1fd to
60963fc
Compare
Exact current-dev head-change evidenceSigned-off-by: The remote branch and PR API now match the dedicated worktree reconstruction requested after #4587 advanced
Fresh verification on Runs 31886837219, 31886852616, 31887033696, and 31887047853 are stale or cancellation-superseded and will not be rerun or used as merge authority. @HaD0Yun @IYENTeam — an authenticated APPROVED review must target exactly — |
60963fc to
751d339
Compare
Further current-dev supersessionSigned-off-by:
Sole authoritative tuple:
The reconstruction is Push-trigger run 31887358286 is cancelled/superseded and will not be rerun. @HaD0Yun @IYENTeam — only an authenticated APPROVED review on exactly — |
Multi-shard product failure — inherited exact-base classificationSigned-off-by: Run 31887366726 is now terminal on stale tuple Canonical plan and inventory proof
Exact failing files/traces
The exact shard-4 command fails identically on detached PR head and detached exact base with the same package-manager trace. All nine causal files also fail identically in isolated fresh-process environments on local current-dev Validated reproducible receipt: Classification: inherited current-dev regression, not #4571. No unrelated edit/scraper repair will be smuggled into this PR, and no knowingly-red reconstruction will be pushed. The dedicated worktree already holds — |
751d339 to
6c8168a
Compare
|
Rebased onto current
Both were dev-side regressions already fixed on dev by #4597; this branch simply predated the fix. No functional changes to this PR's diff — the 3 commits were replayed unchanged (one CHANGELOG conflict resolved by keeping both entries). Local verification at new head
CI rerunning on the new head now. — |
Failure attribution at
|
| Check | Attribution | Evidence |
|---|---|---|
test:@gajae-code/tui |
pre-existing on dev |
see bisect below |
test:...:shard-8-of-8 |
pre-existing on dev |
dev's own run 31930749490 (head e1849e676) fails the same job |
evidence producer |
pre-existing on dev |
same run |
Affected path validation |
aggregate roll-up of the above | — |
PR contract bootstrap |
by design, see note | zero live reviews |
The tui failure is inherited, not introduced
packages/tui/test/resize-replay-storm.test.ts fails identically at every point on and below this branch, including the merge-base with clean dev:
| revision | result |
|---|---|
da648897e (merge-base = clean dev) |
14 pass / 2 fail |
05d33c998 (pet commit) |
14 pass / 2 fail |
60a92c7b2 (tui resize commit) |
14 pass / 2 fail |
6c8168ac4 (current head) |
14 pass / 2 fail |
Both failing cases are the same at all four revisions:
(fail) multiplexer resize replay storm regression > in a plain terminal (no multiplexer markers) > uses the host-appropriate forced redraw policy without multiplexer markers
(fail) synchronized output compatibility framing > keeps every renderer context framed and preserves write boundaries when disabled
It surfaces on this PR only because the branch carries two unrelated commits (05d33c998 pet, 60a92c7b2 tui resize) that put packages/tui/** in the changed-file set, so the planner schedules the @gajae-code/tui job. dev's own recent runs do not schedule that job at all, which is why it is latent there rather than absent.
Worth flagging separately: those two commits ride on this branch but are not part of the ACP gate. If they are meant to land independently, this PR's affected set (and its red tui job) shrinks accordingly.
On the routing defect
Fully acknowledged — that one was mine. bunfig.toml pruned the file from Bun discovery while both the fresh-process shard inventory and the per-file affected task kept scheduling a plain bun test <file>, which can never match a pruned path. I verified my own invocation and documented the filter-vs-discovery trap in the file header, but never traced it through the repository's own shard/planner machinery, so I shipped a gate that could not run on either CI route. The canonical DEDICATED_ONLY_TESTS / BUN_TEST_IGNORE_OVERRIDES / dedicatedTestCommand() contract is the right fix and is strictly better than the hand-mirrored pattern list I proposed.
The PR description has been corrected: it previously claimed the job could not run on a dev-targeting PR, which the canonical routing made false. Current evidence on this head is Affected path validation / test:packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts => success.
On the review request
I cannot satisfy PR contract bootstrap myself: the workflow rejects merge-approved when reviewer-id equals the PR author, and it verifies an authenticated exact-head APPROVED review plus write permission through the API. As author I am structurally ineligible, so this needs @HaD0Yun or @IYENTeam on the exact head.
6c8168a to
2160af5
Compare
|
Second rebase onto
Both prior failure classes ( — |
|
shard-2 failed on — |
`initialize` advertises five session lifecycle capabilities -- list, fork, resume, close, delete -- to every ACP client, and all five work. The pinned upstream `acp-core-v1` corpus contains 21 cases and exercises none of them, so that surface shipped with zero release-gate coverage. This adds a stdio smoke suite against the credential-free conformance fixture and a dedicated CI job the aggregate `test` gate depends on. The assertions are mutation-proven rather than merely written: stubbing `session/list` to ignore its `cwd` argument fails exactly the discrimination test, and a fixture that swallows `session/prompt` makes the suite fail on timeout instead of silently counting that timeout as the close postcondition. Every session the client opens is closed in teardown. Killing the ACP client does not reap broker-owned hosts -- the broker spawns one `sdk session-host-internal` per session and outlives the client -- so without that the gate leaked one orphan host per run, which accumulates permanently on a long-lived runner. Lore-id: 7c4e1a92 Constraint: cannot join the default `bun test` run -- spawns a broker plus two session hosts and costs ~9s Constraint: `bun test <path>` filters already-discovered files, so a path pruned by `pathIgnorePatterns` can only be run via `--path-ignore-patterns` override Constraint: unknown-session error shape stays unasserted -- `close`/`delete` no-op on unowned sessions by design while `resume`/`prompt` reject, and that asymmetry is a separately filed open question Rejected: add cases to acp-core-v1 | the runner validates against a pinned upstream case-ID list and rejects unknown ids Rejected: extra step inside acp_conformance | job name would stop describing its contents; independent failure attribution is worth ~20 lines of setup Rejected: assert the -32603 code and message | would cement an unreviewed contract; only the fact of rejection is asserted Confidence: high Scope-risk: narrow Reversibility: easy Directive: close every session the test opens -- teardown reaps broker-owned hosts that outlive the ACP client Directive: keep the three ignore patterns in ci.yml in sync with bunfig.toml -- `--path-ignore-patterns` replaces the list entirely, so dropping them silently widens CI discovery Tested: 12 pass / 26 expect calls, 14 consecutive clean runs; session-host count 0 before and 0 after three runs; scratch dirs 69 before and 69 after; mutation-proven cwd discrimination and close postcondition Not-tested: behavior against a real model -- the fixture is deliberately credential-free and canned
…sk argv The ACP lifecycle smoke is pruned from Bun discovery by bunfig.toml, so plain `bun test <file>` — a filter over already-discovered files — can never run it. Both the PR-mode per-file task and the fresh-process shard inventory scheduled exactly that, so shard-4 and the dedicated affected task both failed deterministically with "filters did not match any test files" (exit 1): the default exclusion worked while both routing paths kept scheduling the excluded file. One contract now owns the routing. ci-dev-affected.ts exports DEDICATED_ONLY_TESTS plus BUN_TEST_IGNORE_OVERRIDES (the complete bunfig ignore list) and dedicatedTestCommand(); addTestFileTask emits the override argv for dedicated-only files, planFullTasks schedules the suite once for Main CI, the fresh-process inventory excludes the file so package shards never spawn a pruned path, and the acp_lifecycle_smoke workflow job invokes the planner task instead of forking the pattern list. Regressions pin the argv shape, the bunfig lockstep, targeted and full-plan routing, shard exclusion, and that the workflow holds no duplicate override. Lore-id: pr4571-dedicated-routing Constraint: bunfig prune removes files from discovery; naming a pruned path on `bun test` never re-includes it Constraint: --path-ignore-patterns replaces the bunfig list, so every override must restate all canonical patterns Rejected: re-including via `bun test <path>` | filters over discovered files; pruned file can never match Rejected: removing the bunfig ignore and excluding only in shards | default `bun test` would then run a 9s broker suite per invocation Rejected: workflow keeps its own pattern list | forks BUN_TEST_IGNORE_OVERRIDES and drifts from the planner Confidence: high Scope-risk: moderate Reversibility: trivial Tested: bun test scripts/ci-dev-affected.test.ts scripts/run-bun-test-files.test.ts (108 pass) Tested: CI_FORCE_FULL=1 ci-dev-affected --task=acp-lifecycle-smoke (12 pass) Tested: PR-mode --task=test:<path> under pull_request event env (12 pass) Tested: enumerateTestFiles packages/coding-agent excludes the file (1400 files) Tested: bun test packages/coding-agent/test/acp/ default discovery (45 pass / 6 files) Not-tested: live GitHub Actions matrix fan-out (replacement run is the authority)
Timed-out ACP requests remained pending and teardown could spend the full request timeout on every session before killing the fixture. Main CI also scheduled the dedicated lifecycle task in both the generic matrix and its named fail-closed job. Bound best-effort cleanup, escalate fixture termination after a grace period, and keep named full-plan tasks out of the generic matrix while preserving the canonical --task resolution path. Lore-id: 4571f1a7 Constraint: preserve the contributor lifecycle coverage and canonical dedicated argv Rejected: run the lifecycle smoke twice on main | wastes native-backed CI and weakens dedicated-job ownership Confidence: high Scope-risk: narrow Reversibility: easy Tested: exact full-plan and PR-mode lifecycle tasks; affected CI selftests; workflow policy tests; ACP default discovery; coding-agent check
2160af5 to
f7b68fa
Compare
|
Third rebase onto Updated verdict line for reviewers (diff digest recomputed at this head): Dedicated — |
|
shard-5 on head — |
|
Confirmation: the replacement run on head — |
What
initializeadvertises five ACP session lifecycle capabilities to every client:All five work. The pinned upstream
acp-core-v1corpus contains 21 cases and exercises none of them, so that advertised surface shipped with zero release-gate coverage.This adds one:
packages/coding-agent/test/acp/acp-lifecycle-smoke.test.tsbunfig.toml[test] pathIgnorePatternsentry keeping it out of the default run..github/workflows/ci.ymlacp_lifecycle_smokejob; added to the aggregatetestjob'sneedswith a fail-closed success assertion.packages/coding-agent/CHANGELOG.mdUnreleased/Addedbullet.Derived from OpenClaw's protocol smoke testing recipe. GJC's ACP is broker/SDK-backed while OpenClaw's is a Gateway bridge, so the concept transferred and none of the code did.
Why the exclusion mechanism looks odd
The test spawns a broker plus a session host and costs about 9s, so it must stay out of the default suite. Two things about
pathIgnorePatternsare easy to get wrong:bun test <path>is a filter over already-discovered files, so a pruned file can never match. Both the bare and./-prefixed forms printfilters did not match any test files.--path-ignore-patternsreplaces the bunfig list entirely. That is the only way in, which is why the CI job repeats the repository's three existing patterns. Dropping them would silently widen discovery.Both facts are documented at all three sites.
Assertions are mutation-proven, not just written
Every gate here was attacked before being trusted:
session/listignores itscwdargument and returns every sessionsession/list discriminates on cwd instead of returning every session, receiving["session-1","session-2"]whensession-2was expected absentsession/prompt(never replies)PATHbroken so the fixture cannot spawnENOENTThe
cwddiscrimination test exists specifically because an earlier revision asserted only that the listing contained the created session, which an implementation ignoringcwdwould also pass.Deliberately not covered
The unknown-session error shape.
session/closeandsession/deleteon an unowned session return{}by design:while
resume/promptreject with-32603and a leaked"Internal error: "prefix, in two different message strings. Whether that asymmetry and that code are right is a separate question; pinning it here would cement an unreviewed contract. The post-close prompt asserts only that the call is rejected, never its code or message, so re-coding that error stays free.Verification
bun test packages/coding-agent/test/acp/gives 45 tests across 6 filesbun --cwd=packages/coding-agent run check(biome +tsc --noEmit): exit 0acp_lifecycle_smokeif:/needs:are identical toacp_conformance, and the aggregate assertstest "$lifecycle" = successsymmetrically withtest "$conformance" = success, so a skip fails closed exactly like its siblingRun it locally:
Follow-ups (recorded, not included)
-32603is JSON-RPC's internal error but this is a caller error, the"Internal error: "prefix leaks to clients, and two paths produce different strings for the same condition. Note theclose/deletesilence is an intentional ownership guard and must not be "fixed" without understanding that.openclaw acp clientequivalent). Design already settled during interview -- attach to an existing broker-registered session, idle-only, prompt on every tool call -- but blocked on a broker-side client-liveness signal that does not exist today:session.listrows carry onlysessionId/cwd/title/updatedAt,resumeSessionattaches unconditionally, and ownership is per-connectionAcpAgentstate invisible across processes.--session/--reset-session/--provenance).Correction: this PR does exercise the new test
An earlier revision of this description claimed the new job could not run on a PR targeting
dev, because.github/workflows/ci.ymltriggers only onmain. That is no longer true and the claim is withdrawn.The maintainer routed this suite through a canonical dedicated-test path on top of the original commit: it is registered in
DEDICATED_ONLY_TESTS(scripts/ci-dev-affected.ts:302), so the fresh-process shard inventory skips it and every planner and CI route invokes it through one canonical argv that applies the--path-ignore-patternsoverride, never a barebun test <file>.Evidence from the latest run on this head:
That also resolves the pattern-duplication concern raised below: the ignore list now lives in one place rather than being hand-mirrored between
bunfig.tomlandci.yml.Post-review fix: orphan session-host leak
A self-review after the gates closed found a defect none of the six review generations caught, because every lane checked assertion semantics and none checked process hygiene:
Each run leaked one
sdk session-host-internalprocess. Cause: the second session added in the final generation forcwddiscrimination was never closed. Killing the ACP client does not reap those hosts -- the broker spawns one per session and outlives the client -- so only an explicitsession/closereleases them. On a long-lived self-hosted runner this accumulates permanently.Fixed by tracking every session the client opens and closing them all in teardown, best-effort so a reaping failure cannot mask a real test failure. Verified from a clean baseline:
Suite unchanged at 12 pass / 26 expect calls, and 14 consecutive clean runs.
One anomaly worth recording rather than hiding: a single run in the middle of this investigation reported
0 pass. It occurred in the same batch in which I had just force-killed four broker-owned hosts out from under a live broker, and 14 runs before and after were clean, so the most plausible cause is my own interference rather than a race in the test. It could not be reproduced.gajae.pr-review-verdict.v1 needs-human sha256:9b2fe3d4bd7de2c9fbf9f2b6fa8d7ef88040ef39c0737bc2f1ad89465be06293 reviewer:human reviewer-id:pending evidence:exact-head-751d339-current-dev-87b540d-review-pending