From 274655dabb3035f2b1d1bc57ad956a1058e2d54c Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:02:38 +0800 Subject: [PATCH 001/110] docs: inventory Grok-assisted merged contributions Record the post-2026-08-14 mainline PR set and review boundaries before evaluating replacement changes. Generated-by: OpenAI Codex --- docs/grok-assisted-contribution-review.md | 80 +++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 docs/grok-assisted-contribution-review.md diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md new file mode 100644 index 00000000000..4c73ae27d96 --- /dev/null +++ b/docs/grok-assisted-contribution-review.md @@ -0,0 +1,80 @@ + + +# Grok-assisted contribution review + +**Baseline:** `bfb315acc` (2026-09-27). **Status:** inventory; implementation review pending. + +This is a provenance and engineering inventory, not a legal classification or +an assertion that these changes must be removed. The scope is merged PRs whose +Grok-assisted work was committed on or after 2026-08-14, the effective date of +the xAI Acceptable Use Policy change under discussion. The scan matches +`Generated-by: .*Grok` in commits reachable from `origin/main`, then checks PR +disclosures for contributions without that trailer. A trailer does not establish +which individual lines were generated; a PR without one can still disclose use. + +| PR | Main commit | Area | Evidence | +|---:|---|---|---| +| #3008 | `2632b8506` | Eval egress | Commit trailer | +| #2967 | `1ce0bc308` | Eval egress | Commit trailer | +| #3048 | `a75640f43` | Desktop transcript | Commit trailer; mixed-tool PR | +| #3063 | `0dc3e5ec3` | Runtime cleanup | Commit trailer | +| #3066 | `d6bef01d3` | Eval framework | Commit trailer | +| #3070 | `9fbdc99a2` | Desktop projects | Commit trailer | +| #3078 | `887af0c9d` | CI inventory | Commit trailer | +| #3082 | `2666a579a` | Runtime PTY | Commit trailer | +| #3099 | `4bded3b21` | Desktop rail | Commit trailer | +| #3101 | `84a579db6` | Desktop tests | Commit trailer | +| #3102 | `5ef39c350` | Desktop bridge | Commit trailer | +| #3104 | `8a11c9f2b` | Runtime graph | Commit trailer | +| #3106 | `a79f69185` | Runtime Host tests | Commit trailer | +| #3111 | `cdf023dc0` | Desktop fixtures | Commit trailer | +| #3115 | `c92d0a22a` | Rate limits | Commit trailer | +| #3117 | `12d2ab54e` | Storage cleanup | Commit trailer | +| #3118 | `dd02553aa` | xAI model discovery | Commit trailer | +| #3119 | `802fe8977` | Desktop Plan Mode | Commit trailer | +| #3123 | `2e3c82e10` | Desktop tests | Commit trailer | +| #3069 | `4ffc823fd` | Desktop search | Commit trailer | +| #3459 | `b434a491a` | Terminal handling | Commit trailer; mixed PR | +| #3364 | `3ab0605b3` | Relative time | Commit trailer; mixed PR | +| #3544 | `dc9d2f0dd` | Composer queue | Commit trailer; mixed-tool PR | +| #4345 | `472fea02b` | Architecture docs | Commit trailer; mixed-tool PR | +| #5223 | `aa3f8e538` | Project path validation | PR AI-use disclosure; no Grok trailer | + +PR #2956 disclosed Grok but merged on 2026-08-13, before the selected date, +so it is outside this inventory. Closed, unmerged PRs are not in the current +mainline and are tracked separately: #4809, #4899, #5102, #5342, #5349, +#5350, and #5497. + +## Review protocol + +For each row, verify the original behavior and tests against current `main`, +identify subsequent changes or removals, and record the current affected files +and an explicit disposition. Do not count a rewritten commit message, a +reformat, or a no-op reimplementation as a provenance remedy. Any replacement +must be an independently reasoned change with behavior and regression evidence; +the earlier history and contributor disclosures remain intact. The scope and +acceptability of any remediation need Apache project/legal review. + +## Open work + +- [ ] Inspect all 25 PRs and map their changes to current code and tests. +- [ ] Record which changes are still material, superseded, or mixed with other work. +- [ ] Decide the appropriate action with the project and ASF legal discussion. +- [ ] Implement and verify any agreed replacements in reviewable slices. From 654934f91e248a500b5c0290c0dd5392585efe54 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:05:58 +0800 Subject: [PATCH 002/110] docs: triage Grok-assisted PR surfaces against current tree Record present-path counts and review targets without asserting line-level survival or legal disposition. Generated-by: OpenAI Codex --- docs/grok-assisted-contribution-review.md | 47 ++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 4c73ae27d96..afb3ca2c227 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -62,6 +62,50 @@ so it is outside this inventory. Closed, unmerged PRs are not in the current mainline and are tracked separately: #4809, #4899, #5102, #5342, #5349, #5350, and #5497. +## First-pass engineering triage + +This pass inspected each merge's changed paths and size against the baseline, +and sampled the active #3118 and #5223 implementation paths. "Present" means +the path still exists, **not** that the original lines survive or that its +behavior is unchanged. The detailed line-level and behavioral review is still +open; these observations are not a replacement decision. + +| PR | Changed paths present now | Review target | +|---:|---:|---| +| #3008 | 3/3 | Eval CONNECT/TCP egress rules and denial tests | +| #2967 | 6/6 | Eval audit completeness and fail-closed boundary | +| #3048 | 14/14 | Mixed-author live transcript reseed and catch-up behavior | +| #3063 | 0/1 | Confirm deleted compact-cleanup has no successor or reader | +| #3066 | 11/11 | Eval framework selection across Python and TS | +| #3070 | 3/3 | Nested project registration and storage tests | +| #3078 | 5/5 | Generated inventory and CI drift gate | +| #3082 | 2/2 | PTY finalization/persist race | +| #3099 | 3/5 | Current rail grouping state and E2E successor | +| #3101 | 1/1 | Streaming remount painted-frame E2E | +| #3102 | 12/14 | Removed bridge endpoints and current IPC callers | +| #3104 | 8/9 | Removed runtime graph drive and Host graph authority | +| #3106 | 10/12 | Candidate launcher and E2E execution authority | +| #3111 | 2/3 | Daily Review fixture writer and deleted archive store | +| #3115 | 16/16 | Provider retry classification and user-facing wait state | +| #3117 | 10/14 | Removed JSON connection store and current storage paths | +| #3118 | 2/2 | xAI OAuth discovery filter is absent in current registry | +| #3119 | 1/1 | Plan Mode copy import at current module boundary | +| #3123 | 0/1 | Original prompt-rail E2E file is absent; find successor | +| #3069 | 0/2 | Both original search tests are absent; find current search tests | +| #3459 | 2/4 | Mixed-author terminal control-character handling | +| #3364 | 3/3 | Mixed-author relative-time formatting and refresh | +| #3544 | 31/35 | Mixed-tool queue protocol, Host receipts, and UI; 35-file change | +| #4345 | 1/1 | Mixed-tool architecture documentation accuracy | +| #5223 | 2/2 | Directory type validation still runs in project resolution | + +The largest original change, #3544, touched 35 files and changed more than +2,000 lines; #3117 removed over 1,600 lines. A single wholesale rewrite would +cross separate storage, protocol, Desktop, Eval, and security review boundaries. +The deleted-only #3063 and absent test paths in #3123/#3069 have no direct +current file to "rewrite". Review should identify a present behavior before +proposing a replacement, and high-risk egress/Host paths need their own focused +tests and review. + ## Review protocol For each row, verify the original behavior and tests against current `main`, @@ -74,7 +118,8 @@ acceptability of any remediation need Apache project/legal review. ## Open work -- [ ] Inspect all 25 PRs and map their changes to current code and tests. +- [x] Triage all 25 original changes by path, size, and current path presence. +- [ ] Trace current lines, behaviors, and tests for all 25 PRs. - [ ] Record which changes are still material, superseded, or mixed with other work. - [ ] Decide the appropriate action with the project and ASF legal discussion. - [ ] Implement and verify any agreed replacements in reviewable slices. From fc9d5f0a80540acbc30f841ed76e21d27c8fe7b1 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:08:00 +0800 Subject: [PATCH 003/110] docs: distinguish squash dates from original Grok use evidence Flag mixed-date #2967 and the #3123 provenance discrepancy for review. Generated-by: OpenAI Codex --- docs/grok-assisted-contribution-review.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index afb3ca2c227..36cc2e288fa 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -23,8 +23,9 @@ This is a provenance and engineering inventory, not a legal classification or an assertion that these changes must be removed. The scope is merged PRs whose -Grok-assisted work was committed on or after 2026-08-14, the effective date of -the xAI Acceptable Use Policy change under discussion. The scan matches +mainline squash commits are dated on or after 2026-08-14, the effective date +of the xAI Acceptable Use Policy change under discussion. A squash date is +**not** the date a tool produced the original contribution. The scan matches `Generated-by: .*Grok` in commits reachable from `origin/main`, then checks PR disclosures for contributions without that trailer. A trailer does not establish which individual lines were generated; a PR without one can still disclose use. @@ -62,6 +63,16 @@ so it is outside this inventory. Closed, unmerged PRs are not in the current mainline and are tracked separately: #4809, #4899, #5102, #5342, #5349, #5350, and #5497. +The original PR commit histories narrow this initial scope further. #2967 has +five Grok-tagged original commits, starting on 2026-08-13 and continuing +through 2026-08-15; it needs commit-level attribution rather than treating the +whole PR as post-change output. #3123 has a Grok trailer on the squash commit, +but none of its three original PR commits have that trailer and its original +PR description has no AI-use disclosure. Confirm that provenance before +classifying it. #5223 has no Grok-tagged commit but explicitly discloses +Cursor/Grok assistance in the PR description. The remaining 22 trailer-bearing +PRs have at least one Grok-tagged original commit dated 2026-08-14 or later. + ## First-pass engineering triage This pass inspected each merge's changed paths and size against the baseline, From c799de22310b82fee9db99b3a6fde4313320d8e0 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:14:00 +0800 Subject: [PATCH 004/110] docs: record current-main revert feasibility for queue PR The disposable #3544 reverse patch produced 31 conflicts and was aborted without product changes. Generated-by: OpenAI Codex --- docs/grok-assisted-contribution-review.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 36cc2e288fa..72a7b79a68b 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -117,6 +117,22 @@ current file to "rewrite". Review should identify a present behavior before proposing a replacement, and high-risk egress/Host paths need their own focused tests and review. +### Revert feasibility probe + +On 2026-09-27, a disposable worktree at `d89ccce01` (then-current +`origin/main`) attempted `git revert --no-commit dc9d2f0dd` for #3544. +The revert stopped with 31 unmerged paths across Desktop IPC and renderer, +Runtime Host protocol/coordinator, UI, tests, and two modify/delete cases. +The probe was aborted and its worktree removed; no revert was committed or +pushed. This is direct evidence that a wholesale revert cannot be applied +mechanically to today's tree. The current behavior and later dependents must +be resolved before a replacement can be tested. + +Some entries are removals rather than added implementation. For example, +#3118 removed one discovery filter and #3063 deleted an unused module. A +revert followed by the same deletion would leave the final product unchanged; +such a commit pair alone does not establish an independent replacement. + ## Review protocol For each row, verify the original behavior and tests against current `main`, From 071aefe5c8f61522f42632566bc673565afb26bd Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:20:00 +0800 Subject: [PATCH 005/110] revert(runtime): remove Grok-authored PTY exit reconciliation from #3082 --- packages/runtime/src/shell-run-manager.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/runtime/src/shell-run-manager.ts b/packages/runtime/src/shell-run-manager.ts index 2826b70089f..c8c766b68dc 100644 --- a/packages/runtime/src/shell-run-manager.ts +++ b/packages/runtime/src/shell-run-manager.ts @@ -510,12 +510,6 @@ export class ShellRunProcessManager }), ); } - // A real PTY can exit while that persist is still in flight, leaving a - // running snapshot here even though finalizeOnce has already started. - if (live.driverExit || live.finalizeOnce) { - record = await this.markObserved(await live.finished.join()); - return shellRunContent(record, operation); - } if (isTerminalShellRunStatus(record.status)) record = await this.markObserved(record); return clientControl ? compactShellRunContent(record) : shellRunContent(record, operation); } From 3b626a2c2b9b8aba4672f567aa0ea76c466c8781 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:23:14 +0800 Subject: [PATCH 006/110] revert(storage): remove Grok-assisted directory check from #5223 --- packages/storage/src/project-catalog.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/storage/src/project-catalog.ts b/packages/storage/src/project-catalog.ts index 164fe9d272e..4f38aa54944 100644 --- a/packages/storage/src/project-catalog.ts +++ b/packages/storage/src/project-catalog.ts @@ -861,12 +861,6 @@ export async function resolveProjectLocation(input: { path: string; }): Promise { const canonicalPath = normalize(await realpath(resolve(input.path))); - // Regular files are not valid projects. Without this check, a file inside a - // Git worktree reaches `git -C rev-parse ...`, whose numeric failure - // is not treated as an invalid path by the Host coordinator and drains it. - if (!(await stat(canonicalPath)).isDirectory()) { - throw new TypeError(`Project path is not a directory: ${canonicalPath}`); - } if (!(await hasEnclosingGitEntry(canonicalPath))) { return { canonicalPath, From 5ea512625630b7172a5f6e937fec6e84c50f36a0 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:27:59 +0800 Subject: [PATCH 007/110] fix: independently restore PTY exit and project path safeguards --- docs/grok-assisted-contribution-review.md | 52 +++++++++++++- .../src/__tests__/shell-run-manager.test.ts | 67 ++++++++----------- packages/runtime/src/shell-run-manager.ts | 3 + .../src/__tests__/project-catalog.test.ts | 50 ++++++-------- packages/storage/src/project-catalog.ts | 4 ++ 5 files changed, 103 insertions(+), 73 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 72a7b79a68b..575003f2200 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -19,7 +19,8 @@ # Grok-assisted contribution review -**Baseline:** `bfb315acc` (2026-09-27). **Status:** inventory; implementation review pending. +**Baseline:** `d89ccce01` (2026-09-27). **Status:** original-commit inventory +reviewed; two narrow replacements verified; remaining behavior reviews open. This is a provenance and engineering inventory, not a legal classification or an assertion that these changes must be removed. The scope is merged PRs whose @@ -73,6 +74,18 @@ classifying it. #5223 has no Grok-tagged commit but explicitly discloses Cursor/Grok assistance in the PR description. The remaining 22 trailer-bearing PRs have at least one Grok-tagged original commit dated 2026-08-14 or later. +A second pass read the original commit titles and trailers for all 25 PRs. +#3048, #3544, and #4345 also contain Codex-authored commits. #3459 contains +two untagged feature commits before its three Grok-tagged commits. In #3364, +the three substantive relative-time commits are tagged Maka; its only +Grok-tagged original commit adds an ASF header to a test file. Reverting the +whole #3364 change would therefore remove work not attributed to Grok. +#3101, #3102, #3104, #3106, #3111, #3117, and #3069 also have untagged +merge or follow-up commits: inspect their merged diffs, not just their branch +commit lists. For example, #3118's original branch includes a Plan Mode import +fix also merged separately in #3119, but #3118's squash diff contains only +the xAI discovery/test deletions. + ## First-pass engineering triage This pass inspected each merge's changed paths and size against the baseline, @@ -133,6 +146,36 @@ Some entries are removals rather than added implementation. For example, revert followed by the same deletion would leave the final product unchanged; such a commit pair alone does not establish an independent replacement. +### Replacement slices + +The following work was done against the 2026-09-27 baseline, preserving later +changes. It does not erase the original history or settle ASF legal questions. + +| PR | Revert-state evidence | Replacement and verification | +|---:|---|---| +| #3082 | Removing the PTY exit reconciliation made the delayed-persist test return `running` instead of `completed`. | Reconcile the control reply after persistence against finalization, then mark terminal observations. Replaced the Grok-authored test with a fresh test that pauses storage and observes driver exit without monkeypatching the driver prototype. Runtime build, shell-run-manager suite (62 pass, 4 platform skips), and Biome passed. | +| #5223 | Removing the directory check made the regular-file test fail with `Missing expected rejection`. | Check the canonical path's stat before Git discovery and reject non-directories with `TypeError`. Replaced the original test with Git/non-Git file cases, registration non-mutation, and a directory control. Storage build, project-catalog suite (23 pass), and Biome passed. | + +The #3082 and #5223 revert-state commits are separate from the replacement +commit to make the failure evidence inspectable. They are not safe to merge +without the following replacement. The remaining 23 entries have **not** been +reverted or reimplemented; #3364 and #3123 require provenance decisions before +any broad rollback. + +Three narrow paths need a disposition rather than an identical revert/reapply: + +- #3063 removed `history-compact-cleanup.ts`; the file and references to it + are absent now. Restoring and removing it again would not replace live code. +- #3118 removed the `fallback-models` filter for xAI OAuth discovery, which is + still absent in the current registry. Its test change also removed a stale + snapshot assertion. Reintroducing the filter just to delete it again would + temporarily restore a model-discovery regression without changing the final + behavior. +- #3119 moved Plan Mode types to core subpath imports. The imports survive, + with later UI-locale behavior added at the same boundary. Restoring the + removed root barrel import would break typecheck; an identical import fix is + not an independent implementation. + ## Review protocol For each row, verify the original behavior and tests against current `main`, @@ -146,7 +189,10 @@ acceptability of any remediation need Apache project/legal review. ## Open work - [x] Triage all 25 original changes by path, size, and current path presence. -- [ ] Trace current lines, behaviors, and tests for all 25 PRs. +- [x] Read original commit trailers and compare branch history with the merged + diff for the 25 candidate PRs. +- [ ] Trace current lines, behaviors, and tests for the remaining 23 PRs. - [ ] Record which changes are still material, superseded, or mixed with other work. - [ ] Decide the appropriate action with the project and ASF legal discussion. -- [ ] Implement and verify any agreed replacements in reviewable slices. +- [x] Implement and verify #3082 and #5223 in reviewable slices. +- [ ] Finish the remaining scoped reviews and any agreed replacements. diff --git a/packages/runtime/src/__tests__/shell-run-manager.test.ts b/packages/runtime/src/__tests__/shell-run-manager.test.ts index d86696042db..92551f1d820 100644 --- a/packages/runtime/src/__tests__/shell-run-manager.test.ts +++ b/packages/runtime/src/__tests__/shell-run-manager.test.ts @@ -2120,39 +2120,28 @@ describe('ShellRunProcessManager', () => { } }); - test('writeStdin itself returns terminal status when persist is held through finalization', async () => { + test('a PTY exit during control persistence is reflected in the reply', async () => { const cwd = await workspace(); - const exitGate = join(cwd, 'exit-gate'); + const exitGate = join(cwd, 'allow-exit'); const backingStore = sqliteShellRunStore(await workspace()); - const persistHeld = deferred(); - const releasePersist = deferred(); - let holdNextObservation = false; - let persistIsHeld = false; + const resumePersist = deferred(); + let pauseControlPersist = false; + let controlPersistPaused = false; const store: ShellRunStore = { createShellRun: (...args) => backingStore.createShellRun(...args), async updateShellRun(sessionId, shellRunId, patch) { - if (holdNextObservation && patch.status === undefined) { - holdNextObservation = false; - persistIsHeld = true; - persistHeld.resolve(); - await releasePersist.promise; + if (pauseControlPersist && patch.status === undefined) { + pauseControlPersist = false; + controlPersistPaused = true; + await resumePersist.promise; } return backingStore.updateShellRun(sessionId, shellRunId, patch); }, readShellRun: (...args) => backingStore.readShellRun(...args), listSessionShellRuns: (...args) => backingStore.listSessionShellRuns(...args), }; - const flushes = manualFlushScheduler(); - const manager = createManager(store, undefined, { - flushIntervalMs: 60_000, - scheduleFlush: flushes.schedule, - }); - const originalDispose = PtyProcessDriver.prototype.dispose; - let driverDisposed = false; - PtyProcessDriver.prototype.dispose = function dispose(this: PtyProcessDriver) { - driverDisposed = true; - return originalDispose.call(this); - }; + const manager = createManager(store); + const liveRuns = (manager as unknown as { live: Map }).live; let ref: string | undefined; try { @@ -2160,19 +2149,14 @@ describe('ShellRunProcessManager', () => { shellInput({ cwd, command: nodeCommand(` - const { readFileSync } = require('node:fs'); + const { existsSync } = require('node:fs'); process.stdout.write('READY\\n'); - const wait = () => { - try { - readFileSync(${JSON.stringify(exitGate)}); - } catch (error) { - if (error.code !== 'ENOENT') throw error; - setImmediate(wait); - return; + const timer = setInterval(() => { + if (existsSync(${JSON.stringify(exitGate)})) { + clearInterval(timer); + process.exit(0); } - process.exit(0); - }; - wait(); + }, 10); `), pty: true, timeoutMs: 120_000, @@ -2182,18 +2166,20 @@ describe('ShellRunProcessManager', () => { ref = initial.ref; await waitForPtyText(manager, initial.ref, /READY/); - holdNextObservation = true; + pauseControlPersist = true; const pending = manager.writeStdin({ sessionId: 'session-1', ref: initial.ref, size: { cols: 81, rows: 25 }, abortSignal: NO_ABORT, }); - await waitUntil(() => persistIsHeld, 15_000); - await persistHeld.promise; + await waitUntil(() => controlPersistPaused, 15_000); await writeFile(exitGate, 'exit'); - await waitUntil(() => driverDisposed, 15_000); - releasePersist.resolve(); + await waitUntil( + () => [...liveRuns.values()].some((live) => live.driverExit !== undefined), + 15_000, + ); + resumePersist.resolve(); const control = await pending; assertShellRunSnapshot(control); @@ -2207,10 +2193,11 @@ describe('ShellRunProcessManager', () => { assert.equal(control.output.mode, 'pty'); if (control.output.mode !== 'pty') throw new Error('expected pty output'); assert.deepEqual([control.output.cols, control.output.rows], [81, 25]); + const durable = await backingStore.readShellRun('session-1', 'shell-run-1'); + assert.equal(durable.revision, control.revision); assert.equal(manager.liveCount(), 0); } finally { - PtyProcessDriver.prototype.dispose = originalDispose; - releasePersist.resolve(); + resumePersist.resolve(); if (ref && manager.liveCount() > 0) { await manager.stopBackgroundTask('session-1', ref, NO_ABORT).catch(() => undefined); } diff --git a/packages/runtime/src/shell-run-manager.ts b/packages/runtime/src/shell-run-manager.ts index c8c766b68dc..771436905ab 100644 --- a/packages/runtime/src/shell-run-manager.ts +++ b/packages/runtime/src/shell-run-manager.ts @@ -510,6 +510,9 @@ export class ShellRunProcessManager }), ); } + // The snapshot was captured before persistence. Exit can begin during that + // await, so a control response must reconcile against the final record. + if (live.driverExit || live.finalizeOnce) record = await live.finished.join(); if (isTerminalShellRunStatus(record.status)) record = await this.markObserved(record); return clientControl ? compactShellRunContent(record) : shellRunContent(record, operation); } diff --git a/packages/storage/src/__tests__/project-catalog.test.ts b/packages/storage/src/__tests__/project-catalog.test.ts index afb03856938..f67c5fa2f01 100644 --- a/packages/storage/src/__tests__/project-catalog.test.ts +++ b/packages/storage/src/__tests__/project-catalog.test.ts @@ -83,38 +83,28 @@ function sessionInput(cwd: string, projectId: string) { }; } -test('a regular file is rejected as a project location', async () => { - const base = await mkdtemp(join(tmpdir(), 'maka-project-file-')); +test('project registration accepts directories but never regular files', async () => { + const base = await mkdtemp(join(tmpdir(), 'maka-project-path-kind-')); try { - const outsideFile = join(base, 'README.md'); - await writeFile(outsideFile, 'not a project\n'); - await assert.rejects( - () => resolveProjectLocation({ path: outsideFile }), - (error) => - error instanceof TypeError && - String(error.message).includes('Project path is not a directory'), - ); - - const repository = join(base, 'repository'); - await mkdir(repository); - await execFileAsync('git', ['init', '--quiet'], { cwd: repository }); - const insideFile = join(repository, 'README.md'); - await writeFile(insideFile, 'not a project\n'); - await assert.rejects( - () => resolveProjectLocation({ path: insideFile }), - (error) => - error instanceof TypeError && - String(error.message).includes('Project path is not a directory'), - ); - - const catalog = createProjectCatalog(join(base, 'storage')); - await assert.rejects( - () => catalog.register(insideFile), - (error) => - error instanceof TypeError && - String(error.message).includes('Project path is not a directory'), - ); + const gitRoot = join(base, 'repo'); + await mkdir(gitRoot); + await execFileAsync('git', ['init', '--quiet'], { cwd: gitRoot }); + const catalog = createProjectCatalog(join(base, 'catalog')); + + for (const parent of [base, gitRoot]) { + const file = join(parent, 'project.txt'); + await writeFile(file, 'file, not a folder'); + const canonicalFile = await realpath(file); + await assert.rejects( + () => resolveProjectLocation({ path: file }), + (error: unknown) => + error instanceof TypeError && + error.message === `Project path is not a directory: ${canonicalFile}`, + ); + await assert.rejects(() => catalog.register(file), TypeError); + } assert.deepEqual(await catalog.list(), []); + assert.equal((await resolveProjectLocation({ path: gitRoot })).kind, 'git'); } finally { await rm(base, { recursive: true, force: true }); } diff --git a/packages/storage/src/project-catalog.ts b/packages/storage/src/project-catalog.ts index 4f38aa54944..79a66361392 100644 --- a/packages/storage/src/project-catalog.ts +++ b/packages/storage/src/project-catalog.ts @@ -861,6 +861,10 @@ export async function resolveProjectLocation(input: { path: string; }): Promise { const canonicalPath = normalize(await realpath(resolve(input.path))); + const location = await stat(canonicalPath); + if (!location.isDirectory()) { + throw new TypeError(`Project path is not a directory: ${canonicalPath}`); + } if (!(await hasEnclosingGitEntry(canonicalPath))) { return { canonicalPath, From 87f81f6316c1c36a4387ed773078cb1b78923e04 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:30:31 +0800 Subject: [PATCH 008/110] test(runtime): independently assert queued PTY exit settlement --- .../src/__tests__/shell-run-manager.test.ts | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/packages/runtime/src/__tests__/shell-run-manager.test.ts b/packages/runtime/src/__tests__/shell-run-manager.test.ts index 92551f1d820..6a5ff1d7e03 100644 --- a/packages/runtime/src/__tests__/shell-run-manager.test.ts +++ b/packages/runtime/src/__tests__/shell-run-manager.test.ts @@ -2091,25 +2091,22 @@ describe('ShellRunProcessManager', () => { const control = await pending; assert.equal(await readFile(sizeBeforeExit, 'utf8'), '80x24'); - const terminal = - control.status === 'starting' || control.status === 'running' - ? await waitForTerminalShellRun(manager, initial.ref, 15_000) - : control; - assertShellRunSnapshot(terminal); - assert.equal(terminal.status, 'completed'); - assert.equal(terminal.exitCode, 0); assert.deepEqual(control.operation, { kind: 'pty_control', failed: false, resize: { cols: 81, rows: 25, applied: false, changed: false }, }); - assert.equal(terminal.output.mode, 'pty'); - if (terminal.output.mode !== 'pty') throw new Error('expected pty output'); - assert.deepEqual([terminal.output.cols, terminal.output.rows], [80, 24]); + const settled = await waitForTerminalShellRun(manager, initial.ref, 15_000); + assertShellRunSnapshot(settled); + assert.equal(settled.status, 'completed'); + assert.equal(settled.exitCode, 0); + assert.equal(settled.output.mode, 'pty'); + if (settled.output.mode !== 'pty') throw new Error('expected pty output'); + assert.deepEqual([settled.output.cols, settled.output.rows], [80, 24]); const durable = await store.readShellRun('session-1', 'shell-run-1'); assert.equal(durable.status, 'completed'); - assert.equal(durable.revision, terminal.revision); + assert.equal(durable.revision, settled.revision); assert.equal(manager.liveCount(), 0); } finally { Terminal.prototype.write = originalWrite; From 7dd44a0028679d9cc75fa56841b0c17e5e78f0fc Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:34:02 +0800 Subject: [PATCH 009/110] docs: record surviving Eval and navigation contribution boundaries --- docs/grok-assisted-contribution-review.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 575003f2200..1220167df4f 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -176,6 +176,23 @@ Three narrow paths need a disposition rather than an identical revert/reapply: removed root barrel import would break typecheck; an identical import fix is not an independent implementation. +#3008 is only partly recognizable in today's Eval egress filter. Its CONNECT +host classification remains, but #3017 subsequently replaced the raw-TCP +layer-closing path, added HTTP/TLS prefix handling, and introduced live +mitmproxy tests. Reverting the #3008 squash against today's code would also +unwind later security behavior. Treat the surviving CONNECT validation as a +separate security review with unit and live-proxy regression checks. + +#3070 remains material in `project-catalog.ts`: explicit registration and +relinking still use `resolveUserSelectedProjectLocation`, while session cwd +resolution keeps repository identity. Its behavior overlaps the #5223 path +validation, so a replacement must test nested registration, touch, relink, +session reassignment, and invalid paths together. #3099's original renderer +files moved under `features/session-navigation`; the successor +`session-rail-layout-store.ts` still hydrates and writes the grouping mode +through `session-list-layout.ts`. Review the current store and renderer reload +test, not the deleted AppShell persistence effect. + ## Review protocol For each row, verify the original behavior and tests against current `main`, From 0920aa215493c9ebdefe507bab3436c13e88907c Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:49:46 +0800 Subject: [PATCH 010/110] revert(storage): remove #3070 explicit nested project selection --- packages/storage/src/project-catalog.ts | 30 +++---------------------- 1 file changed, 3 insertions(+), 27 deletions(-) diff --git a/packages/storage/src/project-catalog.ts b/packages/storage/src/project-catalog.ts index 79a66361392..c4eebfe027c 100644 --- a/packages/storage/src/project-catalog.ts +++ b/packages/storage/src/project-catalog.ts @@ -205,7 +205,7 @@ class SqliteProjectCatalog implements ProjectCatalog { } async register(path: string, options?: ProjectRegistrationOptions): Promise { - const resolved = await resolveUserSelectedProjectLocation(path); + const resolved = await resolveProjectLocation({ path }); if (options?.withinRoot && !isPathWithin(options.withinRoot, resolved.canonicalPath)) { throw new ProjectPathBoundaryError(resolved.canonicalPath); } @@ -344,7 +344,7 @@ class SqliteProjectCatalog implements ProjectCatalog { } async relink(projectId: string, path: string): Promise { - const resolved = await resolveUserSelectedProjectLocation(path); + const resolved = await resolveProjectLocation({ path }); const timestamp = this.now(); const locationPath = resolved.kind === 'git' ? resolved.git!.worktreeRoot : resolved.canonicalPath; @@ -364,7 +364,7 @@ class SqliteProjectCatalog implements ProjectCatalog { projectId: string, path: string, ): Promise<{ project: ProjectRecord; updatedSessionIds: readonly string[] }> { - const resolved = await resolveUserSelectedProjectLocation(path); + const resolved = await resolveProjectLocation({ path }); const timestamp = this.now(); const locationPath = resolved.kind === 'git' ? resolved.git!.worktreeRoot : resolved.canonicalPath; @@ -881,30 +881,6 @@ export async function resolveProjectLocation(input: { }; } -/** - * A directory the user picked in the add/relink chooser. - * - * `resolveProjectLocation` still walks to the enclosing Git worktree so a - * historical session cwd inside a repository stays on that repository. - * The chooser must not do that: selecting `repo/child` would otherwise - * silently become `repo` and reopen the parent project. - */ -async function resolveUserSelectedProjectLocation(path: string): Promise { - const resolved = await resolveProjectLocation({ path }); - if ( - resolved.kind !== 'git' || - !resolved.git || - resolved.canonicalPath === resolved.git.worktreeRoot - ) { - return resolved; - } - return { - canonicalPath: resolved.canonicalPath, - identity: `folder:${resolved.canonicalPath}`, - kind: 'folder', - }; -} - function isPathWithin(root: string, candidate: string): boolean { const path = relative(normalize(root), normalize(candidate)); return path === '' || (!isAbsolute(path) && path !== '..' && !path.startsWith(`..${sep}`)); From 2b14d34679a1b663b04910d102763185aab09a82 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:54:42 +0800 Subject: [PATCH 011/110] fix(storage): distinguish selected folders from historical Git paths --- .../src/__tests__/project-catalog.test.ts | 27 +++++++++++++++++++ packages/storage/src/project-catalog.ts | 14 +++++++--- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/storage/src/__tests__/project-catalog.test.ts b/packages/storage/src/__tests__/project-catalog.test.ts index f67c5fa2f01..bb0ef157acb 100644 --- a/packages/storage/src/__tests__/project-catalog.test.ts +++ b/packages/storage/src/__tests__/project-catalog.test.ts @@ -164,6 +164,33 @@ test('a repository and its linked worktree resolve to one project identity', asy } }); +test('explicit selection preserves a subfolder while historical paths retain repository identity', async () => { + const base = await mkdtemp(join(tmpdir(), 'maka-project-selection-intent-')); + try { + const repository = join(base, 'repository'); + const linkedWorktree = join(base, 'linked'); + await createGitRepositoryWithWorktree(repository, linkedWorktree, 'selection-intent-test'); + const child = join(linkedWorktree, 'feature'); + await mkdir(child); + + const selected = await resolveProjectLocation({ path: child, intent: 'selected' }); + const historical = await resolveProjectLocation({ path: child }); + const worktree = await resolveProjectLocation({ path: linkedWorktree, intent: 'selected' }); + const main = await resolveProjectLocation({ path: repository, intent: 'selected' }); + + assert.deepEqual(selected, { + canonicalPath: await realpath(child), + identity: `folder:${await realpath(child)}`, + kind: 'folder', + }); + assert.equal(historical.kind, 'git'); + assert.equal(historical.identity, worktree.identity); + assert.equal(worktree.identity, main.identity); + } finally { + await rm(base, { recursive: true, force: true }); + } +}); + test('registering a nested folder keeps that folder instead of the enclosing repository', async () => { const base = await mkdtemp(join(tmpdir(), 'maka-project-nested-folder-')); try { diff --git a/packages/storage/src/project-catalog.ts b/packages/storage/src/project-catalog.ts index c4eebfe027c..9ff508444d9 100644 --- a/packages/storage/src/project-catalog.ts +++ b/packages/storage/src/project-catalog.ts @@ -205,7 +205,7 @@ class SqliteProjectCatalog implements ProjectCatalog { } async register(path: string, options?: ProjectRegistrationOptions): Promise { - const resolved = await resolveProjectLocation({ path }); + const resolved = await resolveProjectLocation({ path, intent: 'selected' }); if (options?.withinRoot && !isPathWithin(options.withinRoot, resolved.canonicalPath)) { throw new ProjectPathBoundaryError(resolved.canonicalPath); } @@ -344,7 +344,7 @@ class SqliteProjectCatalog implements ProjectCatalog { } async relink(projectId: string, path: string): Promise { - const resolved = await resolveProjectLocation({ path }); + const resolved = await resolveProjectLocation({ path, intent: 'selected' }); const timestamp = this.now(); const locationPath = resolved.kind === 'git' ? resolved.git!.worktreeRoot : resolved.canonicalPath; @@ -364,7 +364,7 @@ class SqliteProjectCatalog implements ProjectCatalog { projectId: string, path: string, ): Promise<{ project: ProjectRecord; updatedSessionIds: readonly string[] }> { - const resolved = await resolveProjectLocation({ path }); + const resolved = await resolveProjectLocation({ path, intent: 'selected' }); const timestamp = this.now(); const locationPath = resolved.kind === 'git' ? resolved.git!.worktreeRoot : resolved.canonicalPath; @@ -859,6 +859,7 @@ export interface ResolvedProjectLocation { export async function resolveProjectLocation(input: { path: string; + intent?: 'selected'; }): Promise { const canonicalPath = normalize(await realpath(resolve(input.path))); const location = await stat(canonicalPath); @@ -873,6 +874,13 @@ export async function resolveProjectLocation(input: { }; } const git = await resolveGitLocation(canonicalPath); + if (input.intent === 'selected' && canonicalPath !== git.worktreeRoot) { + return { + canonicalPath, + identity: `folder:${canonicalPath}`, + kind: 'folder', + }; + } return { canonicalPath, identity: `git:${git.commonDir}`, From d680734440695d03052c64abd5958670b7cc6321 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:55:58 +0800 Subject: [PATCH 012/110] revert(desktop): disconnect #3099 rail grouping persistence --- .../session-navigation/model/session-rail-layout-store.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts b/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts index c6793a8a337..8ba6b8045cb 100644 --- a/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts +++ b/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts @@ -24,10 +24,8 @@ import { createObservableState } from '../../../application/contracts/session-ca import { clampSessionListWidth, readSessionListCollapsed, - readSessionListViewMode, readSessionListWidth, SESSION_LIST_EXPANDED_MIN_WIDTH, - writeSessionListViewMode, } from './session-list-layout.js'; const LAYOUT_PERSIST_DEBOUNCE_MS = 200; @@ -56,7 +54,7 @@ export function createSessionRailLayoutStore() { const state = createObservableState({ collapsed: readSessionListCollapsed(), width: readSessionListWidth(), - viewMode: readSessionListViewMode(), + viewMode: 'conversation', }); const collapseHandleRef: { current: SideNavImperativeCollapseHandle | null } = { current: null }; let widthPersistHandle: ReturnType | undefined; @@ -94,7 +92,6 @@ export function createSessionRailLayoutStore() { const current = state.getState(); if (current.viewMode === next) return; state.replaceState({ ...current, viewMode: next }); - writeSessionListViewMode(next); }, }; } From f5a1672cde5ac8aab1b2a075fcbc6465d2f839e5 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:57:12 +0800 Subject: [PATCH 013/110] fix(desktop): own rail grouping persistence in layout store --- .../__tests__/session-list-layout.test.ts | 36 ++++++++++--------- .../model/session-list-layout.ts | 15 +------- .../model/session-rail-layout-store.ts | 7 ++-- .../features/session-navigation/testing.ts | 2 -- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/apps/desktop/src/main/__tests__/session-list-layout.test.ts b/apps/desktop/src/main/__tests__/session-list-layout.test.ts index 8726f5bb77b..8debb18d5e3 100644 --- a/apps/desktop/src/main/__tests__/session-list-layout.test.ts +++ b/apps/desktop/src/main/__tests__/session-list-layout.test.ts @@ -19,11 +19,7 @@ import assert from 'node:assert/strict'; import { afterEach, describe, it, mock } from 'node:test'; -import { - createSessionRailLayoutStore, - readSessionListViewMode, - writeSessionListViewMode, -} from '../../renderer/features/session-navigation/testing.js'; +import { createSessionRailLayoutStore } from '../../renderer/features/session-navigation/testing.js'; const VIEW_MODE_KEY = 'maka-chat-list-view-mode-v1'; const WIDTH_KEY = 'maka-chat-list-width-v1'; @@ -73,30 +69,38 @@ describe('session list view mode persistence', () => { it('defaults to conversation when nothing is stored', () => { cleanups.push(installMemoryLocalStorage().restore); - assert.equal(readSessionListViewMode(), 'conversation'); + assert.equal(createSessionRailLayoutStore().getState().viewMode, 'conversation'); }); - it('round-trips a project grouping through the same key the shell hydrates', () => { + it('stores a changed grouping and restores conversation when changed back', () => { const memory = installMemoryLocalStorage(); cleanups.push(memory.restore); - writeSessionListViewMode('project'); + const rail = createSessionRailLayoutStore(); + rail.setViewMode('project'); assert.equal(memory.store.get(VIEW_MODE_KEY), 'project'); - assert.equal(readSessionListViewMode(), 'project'); - }); - - it('keeps conversation when that is what was written', () => { - cleanups.push(installMemoryLocalStorage({ [VIEW_MODE_KEY]: 'project' }).restore); - writeSessionListViewMode('conversation'); - assert.equal(readSessionListViewMode(), 'conversation'); + rail.setViewMode('conversation'); + assert.equal(memory.store.get(VIEW_MODE_KEY), 'conversation'); + assert.equal(createSessionRailLayoutStore().getState().viewMode, 'conversation'); }); it('fails open to conversation for garbage or empty stored values', () => { for (const stored of ['', 'time', 'true', 'PROJECT', 'conversation\n']) { const memory = installMemoryLocalStorage({ [VIEW_MODE_KEY]: stored }); - assert.equal(readSessionListViewMode(), 'conversation', stored); + assert.equal(createSessionRailLayoutStore().getState().viewMode, 'conversation', stored); memory.restore(); } }); + + it('restores the selected grouping when the renderer creates a new rail store', () => { + const memory = installMemoryLocalStorage(); + cleanups.push(memory.restore); + + const beforeReload = createSessionRailLayoutStore(); + beforeReload.setViewMode('project'); + + assert.equal(memory.store.get(VIEW_MODE_KEY), 'project'); + assert.equal(createSessionRailLayoutStore().getState().viewMode, 'project'); + }); }); describe('session rail width persistence', () => { diff --git a/apps/desktop/src/renderer/features/session-navigation/model/session-list-layout.ts b/apps/desktop/src/renderer/features/session-navigation/model/session-list-layout.ts index 107962f1f76..ec1fe686cd6 100644 --- a/apps/desktop/src/renderer/features/session-navigation/model/session-list-layout.ts +++ b/apps/desktop/src/renderer/features/session-navigation/model/session-list-layout.ts @@ -17,25 +17,12 @@ * under the License. */ -import type { SessionViewMode } from '@maka/ui'; -import { safeLocalStorageGet, safeLocalStorageSet } from '../../../browser-storage.js'; +import { safeLocalStorageGet } from '../../../browser-storage.js'; export const SESSION_LIST_EXPANDED_DEFAULT_WIDTH = 260; export const SESSION_LIST_EXPANDED_MIN_WIDTH = 180; export const SESSION_LIST_EXPANDED_MAX_WIDTH = 480; -const SESSION_LIST_VIEW_MODE_KEY = 'maka-chat-list-view-mode-v1'; - -export function readSessionListViewMode(): SessionViewMode { - const stored = safeLocalStorageGet(SESSION_LIST_VIEW_MODE_KEY); - if (stored === 'project' || stored === 'conversation') return stored; - return 'conversation'; -} - -export function writeSessionListViewMode(mode: SessionViewMode): void { - safeLocalStorageSet(SESSION_LIST_VIEW_MODE_KEY, mode); -} - export function readSessionListWidth(): number { const stored = Number(safeLocalStorageGet('maka-chat-list-width-v1')); if (Number.isFinite(stored) && stored > 0) return clampSessionListWidth(stored); diff --git a/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts b/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts index 8ba6b8045cb..d0c665957e4 100644 --- a/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts +++ b/apps/desktop/src/renderer/features/session-navigation/model/session-rail-layout-store.ts @@ -19,7 +19,7 @@ import type { SideNavImperativeCollapseHandle } from '@astryxdesign/core/SideNav'; import type { SessionViewMode } from '@maka/ui'; -import { safeLocalStorageSet } from '../../../browser-storage.js'; +import { safeLocalStorageGet, safeLocalStorageSet } from '../../../browser-storage.js'; import { createObservableState } from '../../../application/contracts/session-catalog/observable-state.js'; import { clampSessionListWidth, @@ -29,6 +29,7 @@ import { } from './session-list-layout.js'; const LAYOUT_PERSIST_DEBOUNCE_MS = 200; +const VIEW_MODE_KEY = 'maka-chat-list-view-mode-v1'; export interface SessionRailLayoutState { readonly collapsed: boolean; @@ -51,10 +52,11 @@ export interface SessionRailLayoutState { * instance for it to be an instance of. */ export function createSessionRailLayoutStore() { + const storedViewMode = safeLocalStorageGet(VIEW_MODE_KEY); const state = createObservableState({ collapsed: readSessionListCollapsed(), width: readSessionListWidth(), - viewMode: 'conversation', + viewMode: storedViewMode === 'project' ? 'project' : 'conversation', }); const collapseHandleRef: { current: SideNavImperativeCollapseHandle | null } = { current: null }; let widthPersistHandle: ReturnType | undefined; @@ -92,6 +94,7 @@ export function createSessionRailLayoutStore() { const current = state.getState(); if (current.viewMode === next) return; state.replaceState({ ...current, viewMode: next }); + safeLocalStorageSet(VIEW_MODE_KEY, next); }, }; } diff --git a/apps/desktop/src/renderer/features/session-navigation/testing.ts b/apps/desktop/src/renderer/features/session-navigation/testing.ts index 21c3d167cd9..1ccb48f5c06 100644 --- a/apps/desktop/src/renderer/features/session-navigation/testing.ts +++ b/apps/desktop/src/renderer/features/session-navigation/testing.ts @@ -53,9 +53,7 @@ export { type SessionSelection, } from './model/session-selection.js'; export { - readSessionListViewMode, SESSION_LIST_EXPANDED_DEFAULT_WIDTH, - writeSessionListViewMode, } from './model/session-list-layout.js'; export { createSessionRailLayoutStore } from './model/session-rail-layout-store.js'; From 6042897ac340e9cd055ed79b2961730ba61d14f2 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 12:59:09 +0800 Subject: [PATCH 014/110] docs: record local replacements and no-code dispositions --- docs/grok-assisted-contribution-review.md | 46 ++++++++++++----------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 1220167df4f..4d78a532d7a 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -20,7 +20,7 @@ # Grok-assisted contribution review **Baseline:** `d89ccce01` (2026-09-27). **Status:** original-commit inventory -reviewed; two narrow replacements verified; remaining behavior reviews open. +reviewed; four narrow replacements verified locally; remaining behavior reviews open. This is a provenance and engineering inventory, not a legal classification or an assertion that these changes must be removed. The scope is merged PRs whose @@ -155,26 +155,31 @@ changes. It does not erase the original history or settle ASF legal questions. |---:|---|---| | #3082 | Removing the PTY exit reconciliation made the delayed-persist test return `running` instead of `completed`. | Reconcile the control reply after persistence against finalization, then mark terminal observations. Replaced the Grok-authored test with a fresh test that pauses storage and observes driver exit without monkeypatching the driver prototype. Runtime build, shell-run-manager suite (62 pass, 4 platform skips), and Biome passed. | | #5223 | Removing the directory check made the regular-file test fail with `Missing expected rejection`. | Check the canonical path's stat before Git discovery and reject non-directories with `TypeError`. Replaced the original test with Git/non-Git file cases, registration non-mutation, and a directory control. Storage build, project-catalog suite (23 pass), and Biome passed. | +| #3070 | Removing explicit nested-project selection made registration return the parent project's ID and broke relinking to a child directory. | Make resolution intent explicit: selected paths retain a nested folder identity, while historical paths and selected repository roots keep Git identity. Storage build, project-catalog suite (24 pass), Desktop nested-selection test, and Biome passed. Desktop full build remains blocked by seven unrelated implicit-`any` diagnostics. | +| #3099 | Disconnecting the migrated rail store's grouping read/write made a fresh store lose the selected grouping (`undefined` persisted value). | Let the rail layout store hydrate and write grouping directly, removing the old standalone read/write helpers. Rail layout tests (6 pass), navigation boundary tests (4 pass), and Biome passed. Desktop full build has the same seven unrelated implicit-`any` diagnostics. | The #3082 and #5223 revert-state commits are separate from the replacement commit to make the failure evidence inspectable. They are not safe to merge -without the following replacement. The remaining 23 entries have **not** been +without the following replacement. The remaining 21 entries have **not** been reverted or reimplemented; #3364 and #3123 require provenance decisions before any broad rollback. -Three narrow paths need a disposition rather than an identical revert/reapply: +Three narrow paths have been traced to a no-code disposition rather than an +identical revert/reapply. They are reviewed, **not** counted as replacements: - #3063 removed `history-compact-cleanup.ts`; the file and references to it - are absent now. Restoring and removing it again would not replace live code. + are absent now (a repository search finds only this review document). + Restoring and removing it again would not replace live code. - #3118 removed the `fallback-models` filter for xAI OAuth discovery, which is - still absent in the current registry. Its test change also removed a stale - snapshot assertion. Reintroducing the filter just to delete it again would - temporarily restore a model-discovery regression without changing the final - behavior. + still absent in `packages/core/src/provider-registry.ts`. The other changed + line removed an unused test import. Reintroducing the filter just to delete + it again would temporarily restore a model-discovery regression without + changing the final behavior. - #3119 moved Plan Mode types to core subpath imports. The imports survive, - with later UI-locale behavior added at the same boundary. Restoring the - removed root barrel import would break typecheck; an identical import fix is - not an independent implementation. + with later UI-locale lookup behavior added at the same boundary in + `apps/desktop/src/renderer/locales/plan-mode-copy.ts`. Restoring the removed + root barrel import would break typecheck; an identical import fix is not an + independent implementation. #3008 is only partly recognizable in today's Eval egress filter. Its CONNECT host classification remains, but #3017 subsequently replaced the raw-TCP @@ -183,15 +188,11 @@ mitmproxy tests. Reverting the #3008 squash against today's code would also unwind later security behavior. Treat the surviving CONNECT validation as a separate security review with unit and live-proxy regression checks. -#3070 remains material in `project-catalog.ts`: explicit registration and -relinking still use `resolveUserSelectedProjectLocation`, while session cwd -resolution keeps repository identity. Its behavior overlaps the #5223 path -validation, so a replacement must test nested registration, touch, relink, -session reassignment, and invalid paths together. #3099's original renderer -files moved under `features/session-navigation`; the successor -`session-rail-layout-store.ts` still hydrates and writes the grouping mode -through `session-list-layout.ts`. Review the current store and renderer reload -test, not the deleted AppShell persistence effect. +#3070 and #3099 were replaced in the current branch after reviewing their +migrated implementations. #3070's historical cwd resolution remains distinct +from explicit nested selection. #3099's renderer reload test survives at +`apps/desktop/e2e/sidebar-project-reload.spec.ts`; its direct store tests pass, +but that Electron E2E has not yet been rerun in this branch. ## Review protocol @@ -208,8 +209,9 @@ acceptability of any remediation need Apache project/legal review. - [x] Triage all 25 original changes by path, size, and current path presence. - [x] Read original commit trailers and compare branch history with the merged diff for the 25 candidate PRs. -- [ ] Trace current lines, behaviors, and tests for the remaining 23 PRs. +- [ ] Trace current lines, behaviors, and tests for 18 further PRs (three + no-code dispositions above are already traced). - [ ] Record which changes are still material, superseded, or mixed with other work. - [ ] Decide the appropriate action with the project and ASF legal discussion. -- [x] Implement and verify #3082 and #5223 in reviewable slices. +- [x] Implement and locally verify #3082, #5223, #3070, and #3099 in reviewable slices. - [ ] Finish the remaining scoped reviews and any agreed replacements. From 7b56b7b84dd1bb9cdeb8eb1fb8bf5d61712ea27d Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:00:54 +0800 Subject: [PATCH 015/110] docs: distinguish mixed attribution and removed prompt-rail test --- docs/grok-assisted-contribution-review.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 4d78a532d7a..bd1f8255bb6 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -181,6 +181,20 @@ identical revert/reapply. They are reviewed, **not** counted as replacements: root barrel import would break typecheck; an identical import fix is not an independent implementation. +Two mixed/uncertain entries require attribution decisions, not a wholesale +rollback: + +- #3364's original Grok-tagged commit (`e65809320`) added only the ASF header + to `packages/core/src/__tests__/relative-time.test.ts`. The relative-time + behavior and tests came from Maka-tagged commits, with a later untagged + follow-up. Keep the required license header and do not revert the formatter + under the squash commit's aggregate Grok trailer. +- #3123's three original PR commits and PR description do not disclose Grok, + despite the squash commit's trailer. Its only changed file, + `apps/desktop/e2e/prompt-rail.spec.ts`, was deleted by #4741. The original + provenance is unresolved; there is no surviving test file to revert and + reimplement mechanically. + #3008 is only partly recognizable in today's Eval egress filter. Its CONNECT host classification remains, but #3017 subsequently replaced the raw-TCP layer-closing path, added HTTP/TLS prefix handling, and introduced live @@ -209,8 +223,8 @@ acceptability of any remediation need Apache project/legal review. - [x] Triage all 25 original changes by path, size, and current path presence. - [x] Read original commit trailers and compare branch history with the merged diff for the 25 candidate PRs. -- [ ] Trace current lines, behaviors, and tests for 18 further PRs (three - no-code dispositions above are already traced). +- [ ] Trace current lines, behaviors, and tests for 16 further PRs (three + no-code dispositions and two mixed/uncertain entries above are traced). - [ ] Record which changes are still material, superseded, or mixed with other work. - [ ] Decide the appropriate action with the project and ASF legal discussion. - [x] Implement and locally verify #3082, #5223, #3070, and #3099 in reviewable slices. From 4902bf16ecee07e8f84689bdc98296f92f8fd90c Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:07:41 +0800 Subject: [PATCH 016/110] fix(eval): isolate framework selection per trial context --- packages/eval/harbor/eval_framework.py | 18 ++++++++---------- packages/eval/harbor/test_eval_framework.py | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/packages/eval/harbor/eval_framework.py b/packages/eval/harbor/eval_framework.py index c14666c4312..0d7bcd66b5c 100644 --- a/packages/eval/harbor/eval_framework.py +++ b/packages/eval/harbor/eval_framework.py @@ -15,26 +15,24 @@ # specific language governing permissions and limitations # under the License. -"""Authoritative Eval harness framework selection. - -`run_trial.py` validates the argv framework and installs it here before Harbor, -Pier, or the shared relay can import. The relay must not read the environment. -""" +"""Framework selection inherited by imports in the current trial context.""" from __future__ import annotations +from contextvars import ContextVar + _FRAMEWORKS = frozenset({"harbor", "pier"}) -_framework: str | None = None +_framework: ContextVar[str | None] = ContextVar("eval_framework", default=None) def install(framework: str) -> None: if framework not in _FRAMEWORKS: raise RuntimeError("framework must be harbor or pier") - global _framework - _framework = framework + _framework.set(framework) def selected() -> str: - if _framework not in _FRAMEWORKS: + framework = _framework.get() + if framework not in _FRAMEWORKS: raise RuntimeError("Eval framework selection is not installed") - return _framework + return framework diff --git a/packages/eval/harbor/test_eval_framework.py b/packages/eval/harbor/test_eval_framework.py index 0a84008f0dc..992505153ab 100644 --- a/packages/eval/harbor/test_eval_framework.py +++ b/packages/eval/harbor/test_eval_framework.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. +import asyncio import importlib.util import unittest from pathlib import Path @@ -40,6 +41,25 @@ def test_install_selects_harbor_and_pier(self) -> None: module.install("pier") self.assertEqual(module.selected(), "pier") + def test_concurrent_trials_do_not_share_framework_selection(self) -> None: + module = self._load_fresh() + + async def trial(framework: str, ready: asyncio.Event, peer: asyncio.Event) -> str: + module.install(framework) + ready.set() + await peer.wait() + return module.selected() + + async def run() -> list[str]: + harbor_ready = asyncio.Event() + pier_ready = asyncio.Event() + return await asyncio.gather( + trial("harbor", harbor_ready, pier_ready), + trial("pier", pier_ready, harbor_ready), + ) + + self.assertEqual(asyncio.run(run()), ["harbor", "pier"]) + @staticmethod def _load_fresh(): path = Path(__file__).with_name("eval_framework.py") From aae93d22fd7e665e74f9aad986b75494b12859fe Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:10:37 +0800 Subject: [PATCH 017/110] test(ci): independently verify Astryx inventory drift gate --- package.json | 2 +- scripts/check-astryx-surface-inventory.mjs | 80 ++++++++++--------- .../check-astryx-surface-inventory.test.mjs | 56 +++++++++++++ scripts/ci-test-plan.mjs | 3 +- scripts/ci-test-plan.test.mjs | 4 + 5 files changed, 104 insertions(+), 41 deletions(-) create mode 100644 scripts/check-astryx-surface-inventory.test.mjs diff --git a/package.json b/package.json index ba0b9adb456..c01b4e6f3f2 100644 --- a/package.json +++ b/package.json @@ -100,7 +100,7 @@ "astryx:theme": "node scripts/build-astryx-theme.mjs", "astryx:surface-inventory": "node scripts/check-astryx-surface-inventory.mjs", "astryx:surface-inventory:write": "node scripts/generate-astryx-surface-inventory.mjs", - "astryx:surface-inventory:test": "node --test scripts/generate-astryx-surface-inventory.test.mjs", + "astryx:surface-inventory:test": "node --test scripts/generate-astryx-surface-inventory.test.mjs scripts/check-astryx-surface-inventory.test.mjs", "sync:model-metadata": "node scripts/sync-model-metadata.mjs", "refresh:model-metadata": "node scripts/sync-model-metadata.mjs --refresh", "check:model-metadata": "node scripts/sync-model-metadata.mjs --check", diff --git a/scripts/check-astryx-surface-inventory.mjs b/scripts/check-astryx-surface-inventory.mjs index 94db4ed4624..9d4bf5d33c0 100644 --- a/scripts/check-astryx-surface-inventory.mjs +++ b/scripts/check-astryx-surface-inventory.mjs @@ -30,7 +30,7 @@ */ import { readFileSync, existsSync } from 'node:fs'; import { join } from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; import { assertNoAstryxBlockers, renderAstryxSurfaceInventory, @@ -49,39 +49,19 @@ const legacyBlockerBaseline = new Map([ ], ]); -function main() { - if (!existsSync(pathsFile) || !existsSync(mdFile)) { - console.error('missing inventory artifacts — run: npm run astryx:surface-inventory:write'); - process.exit(1); - } - - const rendered = renderAstryxSurfaceInventory(root); - assertNoAstryxBlockers(rendered, legacyBlockerBaseline); - const committedMd = readFileSync(mdFile, 'utf8'); - const committedPaths = readFileSync(pathsFile, 'utf8'); - const mdDrift = committedMd !== rendered.markdown; - const pathsDrift = committedPaths !== rendered.paths; - - if (!mdDrift && !pathsDrift) { - console.log( - `astryx surface inventory coverage: ok (${rendered.files.length} files, ${rendered.excluded.length} exclusions)`, - ); - return; - } - - const committedPathSet = new Set( - committedPaths - .split('\n') - .map((line) => line.trim()) - .filter(Boolean), - ); - const generatedPathSet = new Set(rendered.files); - const missing = rendered.files.filter((file) => !committedPathSet.has(file)); - const extra = [...committedPathSet].filter((file) => !generatedPathSet.has(file)); - +export function inventoryDrift(rendered, committed) { const details = []; - if (pathsDrift) { + if (committed.paths !== rendered.paths) { details.push('.paths does not match generator output'); + const committedPathSet = new Set( + committed.paths + .split('\n') + .map((line) => line.trim()) + .filter(Boolean), + ); + const generatedPathSet = new Set(rendered.files); + const missing = rendered.files.filter((file) => !committedPathSet.has(file)); + const extra = [...committedPathSet].filter((file) => !generatedPathSet.has(file)); if (missing.length) { details.push( `on disk but not in .paths (${missing.length}):\n ${missing.slice(0, 20).join('\n ')}`, @@ -93,19 +73,41 @@ function main() { ); } } - if (mdDrift) { + if (committed.markdown !== rendered.markdown) { details.push('docs/astryx-surface-file-inventory.md does not match generator output'); } + return details; +} + +function main() { + if (!existsSync(pathsFile) || !existsSync(mdFile)) { + throw new Error('missing inventory artifacts — run: npm run astryx:surface-inventory:write'); + } + + const rendered = renderAstryxSurfaceInventory(root); + assertNoAstryxBlockers(rendered, legacyBlockerBaseline); + const details = inventoryDrift(rendered, { + markdown: readFileSync(mdFile, 'utf8'), + paths: readFileSync(pathsFile, 'utf8'), + }); + if (details.length === 0) { + console.log( + `astryx surface inventory coverage: ok (${rendered.files.length} files, ${rendered.excluded.length} exclusions)`, + ); + return; + } console.error( `astryx surface inventory is stale; run: npm run astryx:surface-inventory:write\n${details.map((line) => `- ${line}`).join('\n')}`, ); - process.exit(1); + process.exitCode = 1; } -try { - main(); -} catch (error) { - console.error(error); - process.exit(1); +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + try { + main(); + } catch (error) { + console.error(error); + process.exitCode = 1; + } } diff --git a/scripts/check-astryx-surface-inventory.test.mjs b/scripts/check-astryx-surface-inventory.test.mjs new file mode 100644 index 00000000000..630dbf44219 --- /dev/null +++ b/scripts/check-astryx-surface-inventory.test.mjs @@ -0,0 +1,56 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { inventoryDrift } from './check-astryx-surface-inventory.mjs'; + +const rendered = { + files: ['packages/ui/src/first.tsx', 'packages/ui/src/second.tsx'], + paths: 'packages/ui/src/first.tsx\npackages/ui/src/second.tsx\n', + markdown: '# Surface inventory\n| first |\n| second |\n', +}; + +test('accepts both artifacts only when they match the generated bytes', () => { + assert.deepEqual(inventoryDrift(rendered, rendered), []); + assert.deepEqual(inventoryDrift(rendered, { ...rendered, paths: rendered.paths.trimEnd() }), [ + '.paths does not match generator output', + ]); +}); + +test('rejects a stale Markdown table even when the path list is current', () => { + assert.deepEqual( + inventoryDrift(rendered, { ...rendered, markdown: '# Surface inventory\n| first |\n' }), + ['docs/astryx-surface-file-inventory.md does not match generator output'], + ); +}); + +test('diagnoses missing and extra paths independently of Markdown drift', () => { + assert.deepEqual( + inventoryDrift(rendered, { + paths: 'packages/ui/src/first.tsx\npackages/ui/src/old.tsx\n', + markdown: rendered.markdown, + }), + [ + '.paths does not match generator output', + 'on disk but not in .paths (1):\n packages/ui/src/second.tsx', + 'in .paths but not on disk (1):\n packages/ui/src/old.tsx', + ], + ); +}); diff --git a/scripts/ci-test-plan.mjs b/scripts/ci-test-plan.mjs index 41820e568f7..01410ca7159 100644 --- a/scripts/ci-test-plan.mjs +++ b/scripts/ci-test-plan.mjs @@ -288,7 +288,8 @@ function isAstryxSurfaceInventoryPath(path) { path === 'docs/astryx-surface-file-inventory.md' || path === 'docs/astryx-surface-file-inventory.paths' || path === 'scripts/generate-astryx-surface-inventory.mjs' || - path === 'scripts/check-astryx-surface-inventory.mjs' + path === 'scripts/check-astryx-surface-inventory.mjs' || + path === 'scripts/check-astryx-surface-inventory.test.mjs' ) { return true; } diff --git a/scripts/ci-test-plan.test.mjs b/scripts/ci-test-plan.test.mjs index d2505845ab7..350667a0507 100644 --- a/scripts/ci-test-plan.test.mjs +++ b/scripts/ci-test-plan.test.mjs @@ -170,6 +170,10 @@ test('the Astryx inventory can run without selecting the code suite', () => { assert.equal(plan.code, false); assert.equal(plan.astryxSurface, true); + assert.equal( + planTests(['scripts/check-astryx-surface-inventory.test.mjs'], { graph }).astryxSurface, + true, + ); }); test('desktop renderer changes retain Electron and Storybook coverage', () => { From b9d8643b52f1178ba4b383cdf97983ddffd0b4b2 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:14:52 +0800 Subject: [PATCH 018/110] fix(eval): classify CONNECT by tunnel destination --- packages/eval/harbor/egress_filter.py | 2 +- packages/eval/harbor/test_egress_filter.py | 29 +++++++++++++++++++ .../eval/harbor/test_egress_filter_live.py | 16 ++++++++-- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/packages/eval/harbor/egress_filter.py b/packages/eval/harbor/egress_filter.py index 84c24956aba..4f60bb6c404 100644 --- a/packages/eval/harbor/egress_filter.py +++ b/packages/eval/harbor/egress_filter.py @@ -244,7 +244,7 @@ def apply_http_policy(flow: object, raw_url: str) -> None: def connect_target_url(flow: object) -> str: request = flow.request - host = (getattr(request, "pretty_host", None) or getattr(request, "host", "") or "").strip() + host = (getattr(request, "host", "") or "").strip() if not host: raise ValueError("empty CONNECT host") if ":" in host and not host.startswith("["): diff --git a/packages/eval/harbor/test_egress_filter.py b/packages/eval/harbor/test_egress_filter.py index 9e786997113..a9724c5683b 100644 --- a/packages/eval/harbor/test_egress_filter.py +++ b/packages/eval/harbor/test_egress_filter.py @@ -175,6 +175,35 @@ def make(status, body, headers): MODULE.http_connect(allowed) self.assertIsNone(allowed.response, host) + def test_connect_policy_uses_the_tunnel_target_not_a_spoofed_host_header(self) -> None: + class Response: + @staticmethod + def make(status, body, headers): + return {"status": status, "body": body, "headers": headers} + + with tempfile.TemporaryDirectory() as directory: + MODULE.http = SimpleNamespace(Response=Response) + MODULE.AUDIT_PATH = Path(directory) / "hits.jsonl" + flow = SimpleNamespace( + request=SimpleNamespace( + host="tbench.ai", pretty_host="example.com", port=443 + ), + response=None, + ) + + MODULE.http_connect(flow) + + self.assertEqual(flow.response["status"], 451) + self.assertEqual( + json.loads(MODULE.AUDIT_PATH.read_text().splitlines()[0])["host"], + "tbench.ai", + ) + + flow.request.host = "" + flow.response = None + MODULE.http_connect(flow) + self.assertEqual(flow.response["status"], 503) + def test_tcp_start_kills_raw_tunnels_and_records_them(self) -> None: with tempfile.TemporaryDirectory() as directory: MODULE.AUDIT_PATH = Path(directory) / "hits.jsonl" diff --git a/packages/eval/harbor/test_egress_filter_live.py b/packages/eval/harbor/test_egress_filter_live.py index a0b3734a38f..87686475a90 100644 --- a/packages/eval/harbor/test_egress_filter_live.py +++ b/packages/eval/harbor/test_egress_filter_live.py @@ -490,9 +490,14 @@ def http_via_proxy( return cls._recv_until_close(sock) @classmethod - def connect_via_proxy(cls, host: str, port: int, payload: bytes = b"CLIENT\n") -> tuple[bytes, bytes]: + def connect_via_proxy( + cls, host: str, port: int, payload: bytes = b"CLIENT\n", host_header: str | None = None + ) -> tuple[bytes, bytes]: with socket.create_connection(("127.0.0.1", cls.proxy_port), 5) as sock: - sock.sendall(f"CONNECT {host}:{port} HTTP/1.1\r\nHost: {host}:{port}\r\n\r\n".encode()) + authority = host_header or f"{host}:{port}" + sock.sendall( + f"CONNECT {host}:{port} HTTP/1.1\r\nHost: {authority}\r\n\r\n".encode() + ) header = b"" sock.settimeout(CLOSE_TIMEOUT_S) while b"\r\n\r\n" not in header: @@ -642,6 +647,13 @@ def test_connect_to_a_blocklisted_host_is_451(self) -> None: self.assertIn(b"451", header.split(b"\r\n", 1)[0]) self.assertIn(b"tbench_domain", header) + def test_connect_target_is_blocked_even_with_a_different_host_header(self) -> None: + header, _ = self.connect_via_proxy( + "tbench.ai", 443, b"", host_header="example.com:443" + ) + self.assertIn(b"451", header.split(b"\r\n", 1)[0]) + self.assertIn(b"tbench_domain", header) + def test_raw_connect_relays_no_bytes_and_is_audited(self) -> None: closed_before = self.origin_stats()["raw_closed"] header, body = self.connect_via_proxy("origin", 19081) From 13b008776a175e4d21c61980299c7ef24ef28fac Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:35:02 +0800 Subject: [PATCH 019/110] fix(desktop): fence live content readiness by observation generation --- .../main/__tests__/live-content-seed.test.ts | 30 +++++++++++-------- .../desktop/src/renderer/app-shell-effects.ts | 10 +++---- apps/desktop/src/renderer/app-shell.tsx | 7 +++-- .../desktop/src/renderer/live-content-seed.ts | 3 +- 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/apps/desktop/src/main/__tests__/live-content-seed.test.ts b/apps/desktop/src/main/__tests__/live-content-seed.test.ts index c12d5f52961..d7f5e60cf42 100644 --- a/apps/desktop/src/main/__tests__/live-content-seed.test.ts +++ b/apps/desktop/src/main/__tests__/live-content-seed.test.ts @@ -56,17 +56,15 @@ test('withholds live content until the current observation generation is ready', const first = beginLiveContentSeed(EMPTY_LIVE_CONTENT_SEED, 'session-a'); assert.equal(liveContentSeedRevision(first, 'session-a'), 0); - const ready = completeLiveContentSeed(first, 'session-a'); + const ready = completeLiveContentSeed(first, 'session-a', first.generation); assert.equal(liveContentSeedRevision(ready, 'session-a'), first.generation); assert.equal(liveContentSeedRevision(ready, 'session-b'), 0); }); test('A → B → A does not reuse the previous ready generation', () => { - const firstReady = completeLiveContentSeed( - beginLiveContentSeed(EMPTY_LIVE_CONTENT_SEED, 'session-a'), - 'session-a', - ); - assert.equal(liveContentSeedRevision(firstReady, 'session-a'), 1); + const first = beginLiveContentSeed(EMPTY_LIVE_CONTENT_SEED, 'session-a'); + const firstReady = completeLiveContentSeed(first, 'session-a', first.generation); + assert.equal(liveContentSeedRevision(firstReady, 'session-a'), first.generation); const pendingB = beginLiveContentSeed(firstReady, 'session-b'); assert.equal(liveContentSeedRevision(pendingB, 'session-b'), 0); @@ -76,18 +74,26 @@ test('A → B → A does not reuse the previous ready generation', () => { assert.equal(pendingA.generation, 3); assert.equal(liveContentSeedRevision(pendingA, 'session-a'), 0); - const recovered = completeLiveContentSeed(pendingA, 'session-a'); + const recovered = completeLiveContentSeed(pendingA, 'session-a', pendingA.generation); assert.equal(liveContentSeedRevision(recovered, 'session-a'), 3); }); test('a recovery generation only exposes live content after that generation completes', () => { - const firstReady = completeLiveContentSeed( - beginLiveContentSeed(EMPTY_LIVE_CONTENT_SEED, 'session-a'), - 'session-a', - ); + const first = beginLiveContentSeed(EMPTY_LIVE_CONTENT_SEED, 'session-a'); + const firstReady = completeLiveContentSeed(first, 'session-a', first.generation); const recovering = beginLiveContentSeed(firstReady, 'session-a'); assert.equal(liveContentSeedRevision(recovering, 'session-a'), 0); - const ready = completeLiveContentSeed(recovering, 'session-a'); + const ready = completeLiveContentSeed(recovering, 'session-a', recovering.generation); assert.equal(liveContentSeedRevision(ready, 'session-a'), 2); }); + +test('a stale ready signal cannot complete a newer observation of the same Session', () => { + const initial = beginLiveContentSeed(EMPTY_LIVE_CONTENT_SEED, 'session-a'); + const recovering = beginLiveContentSeed(initial, 'session-a'); + const stale = completeLiveContentSeed(recovering, 'session-a', initial.generation); + assert.equal(stale, recovering); + assert.equal(liveContentSeedRevision(stale, 'session-a'), 0); + const ready = completeLiveContentSeed(stale, 'session-a', recovering.generation); + assert.equal(liveContentSeedRevision(ready, 'session-a'), recovering.generation); +}); diff --git a/apps/desktop/src/renderer/app-shell-effects.ts b/apps/desktop/src/renderer/app-shell-effects.ts index aa75602df46..6280e131418 100644 --- a/apps/desktop/src/renderer/app-shell-effects.ts +++ b/apps/desktop/src/renderer/app-shell-effects.ts @@ -292,8 +292,8 @@ export function useActiveSessionEvents(options: { activeIdRef: RefBox; handleEvent: (sessionId: string, event: SessionEvent) => void; setExecution: import('./features/conversation/index.js').AppShellSessionUiStateController['setExecution']; - beginObservationSeed: (sessionId: string) => void; - completeObservationSeed: (sessionId: string) => void; + beginObservationSeed: (sessionId: string) => number; + completeObservationSeed: (sessionId: string, generation: number) => void; setMessageLoadErrorBySession: (updater: (current: Record) => Record) => void; clearMessageLoadError(sessionId: string): void; setMessageLoadPending: (pending: boolean) => void; @@ -399,7 +399,7 @@ export function useActiveSessionEvents(options: { applyTranscript(activeId, controller, () => !disposed)); const subscribeSessionEvents = () => { const attempt = ++observationAttempt; - beginObservationSeed(activeId); + let seedGeneration = beginObservationSeed(activeId); let unsubscribeRequested = false; let unsubscribeCurrent = () => { unsubscribeRequested = true; @@ -413,10 +413,10 @@ export function useActiveSessionEvents(options: { (phase) => { if (attempt !== observationAttempt) return; controller.observationChanged(phase); - if (phase === 'pending') beginObservationSeed(activeId); + if (phase === 'pending') seedGeneration = beginObservationSeed(activeId); else { observationFailures = 0; - completeObservationSeed(activeId); + completeObservationSeed(activeId, seedGeneration); } }, () => { diff --git a/apps/desktop/src/renderer/app-shell.tsx b/apps/desktop/src/renderer/app-shell.tsx index 72d3b5f384b..c77f5d891f3 100644 --- a/apps/desktop/src/renderer/app-shell.tsx +++ b/apps/desktop/src/renderer/app-shell.tsx @@ -1744,13 +1744,14 @@ function AppShellContent({ activeEventSeedRef.current = next; markDisplayPending(sessionId); setActiveEventSeed(next); + return next.generation; }; - const completeObservationSeed = (sessionId: string) => { + const completeObservationSeed = (sessionId: string, generation: number) => { const current = activeEventSeedRef.current; - if (current.sessionId !== sessionId) return; + if (current.sessionId !== sessionId || current.generation !== generation) return; flushDisplayEvents(sessionId); markDisplayReady(sessionId); - const next = liveContent.completeLiveContentSeed(current, sessionId); + const next = liveContent.completeLiveContentSeed(current, sessionId, generation); activeEventSeedRef.current = next; setActiveEventSeed(next); void retireCancelledTransientMessages(sessionId); diff --git a/apps/desktop/src/renderer/live-content-seed.ts b/apps/desktop/src/renderer/live-content-seed.ts index b83babfa8d4..4d8136105f0 100644 --- a/apps/desktop/src/renderer/live-content-seed.ts +++ b/apps/desktop/src/renderer/live-content-seed.ts @@ -78,8 +78,9 @@ export function beginLiveContentSeed( export function completeLiveContentSeed( current: LiveContentSeed, sessionId: string, + generation: number, ): LiveContentSeed { - if (current.sessionId !== sessionId) { + if (current.sessionId !== sessionId || current.generation !== generation) { return current; } return { From a15194b798a862f8c69e515bd81425abdc579be1 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:35:13 +0800 Subject: [PATCH 020/110] docs: record Grok contribution audit progress and onboarding contract --- .../bot-onboarding-runtime.zh-CN.md | 12 +- docs/grok-assisted-contribution-review.md | 122 +++++++++++++++++- 2 files changed, 122 insertions(+), 12 deletions(-) diff --git a/docs/architecture/bot-onboarding-runtime.zh-CN.md b/docs/architecture/bot-onboarding-runtime.zh-CN.md index 0a80efff4b0..0218a0f361d 100644 --- a/docs/architecture/bot-onboarding-runtime.zh-CN.md +++ b/docs/architecture/bot-onboarding-runtime.zh-CN.md @@ -122,11 +122,11 @@ Snapshot 字段固定为： - `sessionId`, `provider`, optional `brand` - `state` -- optional QR data URL、expiry、next poll delay +- optional QR data URL、expiry、next poll delay，以及短暂轮询故障期间的 `retryHealth` - `canOpenInBrowser` - optional non-sensitive identity `{ id, displayName }` -- redacted user-facing error -- optional `warning`：凭证已落盘，但 live bridge 未在 commit window 内达到 running/healthy；不是 onboarding 失败，也不携带 credential +- redacted user-facing `error`；终态错误可附带稳定的 `errorCode` +- optional `warningCode: 'saved_not_connected'` 与脱敏的 `warningDetail`：凭证已落盘，但 live bridge 未在 commit window 内达到 running/healthy；不是 onboarding 失败，也不携带 credential `poll()` 对同一 session 的并发调用共享一个 in-flight promise，避免 duplicate provider polling。新建同 provider session 会 abort 旧 session；不同 provider 可以独立进行。 @@ -138,7 +138,7 @@ confirmed 后执行一个受 current-session fence 保护的 commit sequence： 2. 把 provider credential 映射为 channel patch，并设置 `enabled: true`、`readiness: configured`。 3. 原子写入 settings。 4. 调用 `applySettingsRuntimeEffects`，让 `BotRegistry` reconcile channel。 -5. 读取 runtime identity，并把非敏感字段投影给 renderer。bridge 未 running 时快照带 `warning`，会话仍为 `connected`。 +5. 读取 runtime identity，并把非敏感字段投影给 renderer。bridge 未 running 时快照带 `warningCode`，可附带 `warningDetail`，会话仍为 `connected`。 每个 await 边界后都重新检查 `currentByProvider` 与 `AbortSignal`。如果用户在 settings write 或 runtime effect 期间取消，service 只回滚本次 onboarding 拥有的字段。 @@ -185,12 +185,12 @@ Rollback 使用 `SettingsStore.updateIf(predicate, patch)`：只有 current chan - Feishu channel 在快捷模式内提供飞书 / Lark brand 切换。 - Modal 明确区分生成中、等待扫码、已扫码、正在连接、已连接、过期、拒绝、取消和错误。 - 用户可刷新 QR、取消、在 HTTPS browser 中打开 verification URL；过期或失败后可重新生成。 -- 成功状态只展示 non-sensitive bot identity，不展示 credential。bridge 未起来时用 `warning`，不用失败态。 +- 成功状态只展示 non-sensitive bot identity，不展示 credential。bridge 未起来时通过 `warningCode` 呈现提示，不用失败态。 - QR frame 在 desktop viewport 居中，light/dark 与窄窗口保持同一 action hierarchy。 ## 9. 验收标准 -1. Unit tests 覆盖 input validation、poll dedupe、slow-down、expiry、cancel、runtime effect、credential rollback，以及 connected 后 bridge 未 running 的 `warning`。 +1. Unit tests 覆盖 input validation、poll dedupe、slow-down、expiry、cancel、runtime effect、credential rollback，以及 connected 后 bridge 未 running 的 `warningCode`。 2. 并发测试证明 cancel 能跨过 runtime-effect commit window，且不会覆盖 concurrent manual edit。 3. Runtime tests覆盖 Feishu/WeCom message mapping，以及 failed-handshake cleanup 顺序。 4. Electron E2E 通过 renderer → preload → IPC → main session 打开 Settings › 远程接入（`settings-bots-onboarding` fixture）。该场景只把 modal 固定在 `waiting` 以供界面校验；main unit tests 直接覆盖 `waiting → connected`，部分 dev/test adapters 能产生 `scanned → connected`，但 Playwright 当前没有运行完整 onboarding 行程的 spec。 diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index bd1f8255bb6..43e97ada312 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -19,8 +19,10 @@ # Grok-assisted contribution review -**Baseline:** `d89ccce01` (2026-09-27). **Status:** original-commit inventory -reviewed; four narrow replacements verified locally; remaining behavior reviews open. +**Baseline:** `d89ccce01` (2026-09-27). **Status:** all 25 original-commit +inventories reviewed; four narrow replacements verified locally, four further +PRs have scoped fixes, and a mixed-tool architecture document has been +corrected. Remaining behavior reviews and whole-PR dispositions are open. This is a provenance and engineering inventory, not a legal classification or an assertion that these changes must be removed. The scope is merged PRs whose @@ -164,7 +166,7 @@ without the following replacement. The remaining 21 entries have **not** been reverted or reimplemented; #3364 and #3123 require provenance decisions before any broad rollback. -Three narrow paths have been traced to a no-code disposition rather than an +Eight narrow paths have been traced to a no-code disposition rather than an identical revert/reapply. They are reviewed, **not** counted as replacements: - #3063 removed `history-compact-cleanup.ts`; the file and references to it @@ -180,6 +182,37 @@ identical revert/reapply. They are reviewed, **not** counted as replacements: `apps/desktop/src/renderer/locales/plan-mode-copy.ts`. Restoring the removed root barrel import would break typecheck; an identical import fix is not an independent implementation. +- #3102 removed unused Desktop bridge/IPC endpoints. The deleted channel names + (including `git-review:mutate`, the memory mutations, onboarding milestone + clearing, MCP reconnect, artifact get, and skill starter/details) have no + present Desktop callers or registered handlers in the source tree. Its + original branch also includes an untagged merge and an E2E follow-up. Do not + resurrect the discarded IPC authority merely to delete it again; the + surviving E2E changes remain a separate test review. +- #3104 removed a dead `runAgentGraphToQuiescence` loop and its test suite. A + current source search finds no call sites for that function; the migrated + `stream-graph-dispatch.ts` exports supervisor types still used by the live + coordinator and reconciliation code. Restoring an inactive second graph + driver would create another execution authority. Its remaining coordinator + and protocol test adjustments require focused verification, but the deleted + loop itself has no behavior to independently rewrite. +- #3117 deleted the legacy `llm-connections.json` store. Current storage and + Desktop main source has no production reference to that file or + `createConnectionStore`; the fixture writes to the Runtime Policy catalog + through its current storage authority. Its fixture test passes and asserts + the old JSON file is not created. The fixture and related migration edits + remain active and require their own review; restoring the removed legacy + store just to remove it again is not a remedy. +- #3069 only added local transcript-search tests; both original test paths + were subsequently deleted (#4877 and #5531). Search now uses the Recall + pipeline. The current multi-host Recall search tests pass (9 tests), but + restoring the old tests would target a retired local-scan implementation. +- #3106 originally sent `--desktop-e2e` through the production candidate CLI + and launcher. #3226 later separated the E2E execution entry into a + `test-only` module and made the production candidate reject that flag. The + current candidate/desktop tests pass (35 tests), including isolation from + test-only modules. Do not restore the old production flag. Other startup and + candidate wiring in that squash still needs its own attribution review. Two mixed/uncertain entries require attribution decisions, not a wholesale rollback: @@ -202,11 +235,88 @@ mitmproxy tests. Reverting the #3008 squash against today's code would also unwind later security behavior. Treat the surviving CONNECT validation as a separate security review with unit and live-proxy regression checks. +#3459 has two untagged substantive commits that introduced terminal query +handling. Its three Grok-tagged follow-ups suppress XTVERSION replies, preserve +cursor-position reports, and mark CLI `/transcript` as local while a turn runs. +The implementation moved to the workbar terminal feature; the three behaviors +and their unit tests survive. The Desktop query suite (6 tests) and CLI +mid-turn `/transcript` test pass. Reverting the squash would remove the +untagged implementation too. Review the tagged behaviors at their current +boundaries, not the terminal feature as a wholly Grok-authored change. + +#4345 is documentation-only. The first original commit is Grok-tagged and +describes bot onboarding; a later Codex-tagged commit corrected its claim about +test coverage. The surviving document was checked against the current +`BotOnboardingSnapshot`, main-process service, renderer, and tests: its stale +generic `warning` claim was corrected to `warningCode`/`warningDetail`, and +`retryHealth`/`errorCode` were documented. This is a scoped factual correction, +not an independently rewritten architecture or erasure of the original history. + +#3048's first two original commits (Grok-tagged) added catch-up seeding and +observation-generation gating; the next two (Codex-tagged) added retry and +ordering assertions. The live seed helper and observation hooks remain, with +later revisions. The current live-content-seed, observer, and streaming-handoff +test files passed before a scoped fix (88 tests). A new failing test showed +that a stale completion for the same Session could mark a newer generation +ready at the helper boundary. Completion now carries the exact generation +token through the observation hook and refuses to flush/display a newer seed +for an older signal. The three test files now pass (89 tests) and Biome passes. +This is a scoped independent hardening, **not** a replacement for the entire +two tagged implementations. The returning-to-live-conversation Electron E2E +passes; a separate Host-recovery E2E remains open. Desktop's full main build +still reports seven unrelated implicit-any errors. + +#3101's only squash change modified the streaming-remount E2E to sample on +animation frames instead of body mutations. The current test has since gained +other assertions; its returning-to-live-conversation case passes in a real +Electron window. This validates the existing assertion, not an independently +rewritten test or the rest of that E2E file. + +#3544's first six original commits are Grok-tagged and the last three are +Codex-tagged. The per-entry queue still has Host protocol, coordinator, and +Desktop UI behavior, though the original Desktop action module has since been +removed. The current Host message-coordinator and protocol suites pass (165 +tests). That baseline does not constitute a replacement for its 35-file mixed +change. The Host/protocol/UI slices and Electron workflow need separate +review; a direct squash revert previously conflicted in 31 paths. + +#3115's rate-limit classification, Host retry projection/continuity, and UI +countdown remain live. The current Runtime/Host suites pass (97 tests) and +Core/UI countdown suites pass (8 tests), including remaining-time projection +after reconnect and reduced-motion display. This is a baseline review only; +the active classification/Host/UI code has not been independently replaced. + +#3111's Daily Review fixture still writes through the interactive storage +authority with nested writer/owner cleanup. Its current archive-seeding test +passes, but there is no independent replacement for this live fixture writer; +the old Desktop archive store was deleted and has no direct current file. + #3070 and #3099 were replaced in the current branch after reviewing their migrated implementations. #3070's historical cwd resolution remains distinct from explicit nested selection. #3099's renderer reload test survives at `apps/desktop/e2e/sidebar-project-reload.spec.ts`; its direct store tests pass, -but that Electron E2E has not yet been rerun in this branch. +and the Electron renderer-reload E2E passes in this branch. + +### Further scoped work (not full PR dispositions) + +- #3066: a new concurrent-trial test failed under the process-global framework + selection (`harbor` observed `pier`). Selection now uses a Python `ContextVar`; + Python 3.13 Eval tests (88 pass, 12 skips), Eval TypeScript build, and 19 + lifecycle tests passed. The original TypeScript removal of the environment + selector remains in place and was verified by the lifecycle suite; it was + not reverted just to reproduce an obsolete selector. +- #3078: the inventory checker now exposes a pure drift comparison, with tests + for exact bytes, independently stale Markdown, and missing/extra paths. The + CI planner also recognizes the new test. The real 300-file inventory check, + 22 Astryx tests, 40 CI planner tests, and Biome passed. The generator and its + later fail-closed dependency parser remain unchanged. +- #3008: a new test exposed that CONNECT classification trusted `pretty_host` + over the actual tunnel destination, allowing a spoofed Host header to hide a + blocklisted target. It now classifies `request.host`; missing targets fail + closed. Python 3.13 Eval tests (90 pass, 13 skips) passed. A live mitmproxy + regression was added but not run locally because the Docker daemon did not + respond. #3017 owns the later raw-TCP closure behavior; do not roll that + implementation back as part of #3008. ## Review protocol @@ -223,8 +333,8 @@ acceptability of any remediation need Apache project/legal review. - [x] Triage all 25 original changes by path, size, and current path presence. - [x] Read original commit trailers and compare branch history with the merged diff for the 25 candidate PRs. -- [ ] Trace current lines, behaviors, and tests for 16 further PRs (three - no-code dispositions and two mixed/uncertain entries above are traced). +- [ ] Finish the behavior and test tracing for the remaining active changes; + no-code deletions and mixed-author PRs are tracked separately above. - [ ] Record which changes are still material, superseded, or mixed with other work. - [ ] Decide the appropriate action with the project and ASF legal discussion. - [x] Implement and locally verify #3082, #5223, #3070, and #3099 in reviewable slices. From d05db43f7069bf08605341063c584776bd3fd652 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:53:56 +0800 Subject: [PATCH 021/110] fix(eval): mark audit truncation before an oversized append --- docs/grok-assisted-contribution-review.md | 7 +++++++ packages/eval/harbor/egress_filter.py | 10 ++++++---- packages/eval/harbor/test_egress_filter.py | 12 ++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 43e97ada312..93e8f6543a8 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -310,6 +310,13 @@ and the Electron renderer-reload E2E passes in this branch. CI planner also recognizes the new test. The real 300-file inventory check, 22 Astryx tests, 40 CI planner tests, and Biome passed. The generator and its later fail-closed dependency parser remain unchanged. +- #2967: a new boundary test showed the audit writer could append a record + across `MAX_AUDIT_BYTES` without recording `audit_truncated` until another + event arrived. The writer now checks the encoded record length before + appending and emits the marker immediately on overflow. Python 3.13 Harbor + tests (91 pass, 13 skips) and 21 Eval audit artifact tests passed. This is a + scoped fix, not an independent replacement of all five original commits; + the first Grok-tagged commit predates the selected policy date. - #3008: a new test exposed that CONNECT classification trusted `pretty_host` over the actual tunnel destination, allowing a spoofed Host header to hide a blocklisted target. It now classifies `request.host`; missing targets fail diff --git a/packages/eval/harbor/egress_filter.py b/packages/eval/harbor/egress_filter.py index 4f60bb6c404..baaa7be2f6b 100644 --- a/packages/eval/harbor/egress_filter.py +++ b/packages/eval/harbor/egress_filter.py @@ -385,17 +385,19 @@ def blocked_response(rule_id: str): def append_audit(rule_id: str, host: str, normalized_path: str) -> None: AUDIT_PATH.parent.mkdir(parents=True, exist_ok=True) - if AUDIT_PATH.exists() and AUDIT_PATH.stat().st_size >= MAX_AUDIT_BYTES: - write_truncation_marker() - return record = { "ts": int(time.time() * 1000), "ruleId": rule_id, "host": host[:255], "normalizedPath": normalized_path[:4096], } + line = json.dumps(record, ensure_ascii=True, separators=(",", ":")) + "\n" + size = AUDIT_PATH.stat().st_size if AUDIT_PATH.exists() else 0 + if size + len(line.encode("utf-8")) > MAX_AUDIT_BYTES: + write_truncation_marker() + return with AUDIT_PATH.open("a", encoding="utf-8") as stream: - stream.write(json.dumps(record, ensure_ascii=True, separators=(",", ":")) + "\n") + stream.write(line) def write_truncation_marker() -> None: diff --git a/packages/eval/harbor/test_egress_filter.py b/packages/eval/harbor/test_egress_filter.py index a9724c5683b..4e1e857855f 100644 --- a/packages/eval/harbor/test_egress_filter.py +++ b/packages/eval/harbor/test_egress_filter.py @@ -493,6 +493,18 @@ def test_audit_writes_one_truncation_marker_when_the_byte_limit_is_reached(self) ["audit_truncated"], ) + def test_audit_marks_a_record_that_would_cross_the_byte_limit(self) -> None: + with tempfile.TemporaryDirectory() as directory: + MODULE.AUDIT_PATH = Path(directory) / "hits.jsonl" + MODULE.AUDIT_PATH.write_bytes(b"{}\n" * (MODULE.MAX_AUDIT_BYTES // 3)) + self.assertEqual(MODULE.AUDIT_PATH.stat().st_size, MODULE.MAX_AUDIT_BYTES - 1) + + MODULE.append_audit("tbench_domain", "tbench.ai", "/tasks") + + last_line = MODULE.AUDIT_PATH.read_bytes().splitlines()[-1] + self.assertEqual(json.loads(last_line)["ruleId"], "audit_truncated") + self.assertTrue(MODULE.audit_already_truncated()) + def test_truncation_probe_ignores_non_object_json_tails(self) -> None: with tempfile.TemporaryDirectory() as directory: MODULE.AUDIT_PATH = Path(directory) / "hits.jsonl" From d725d95bd55509c2e145e6334d520a19ea5f02a3 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 13:59:04 +0800 Subject: [PATCH 022/110] fix(runtime): honor retry delays in standard Headers --- docs/grok-assisted-contribution-review.md | 6 +++++- .../__tests__/provider-error-classification.test.ts | 11 +++++++++++ packages/runtime/src/provider-error-classification.ts | 6 ++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 93e8f6543a8..b9fae85cc2d 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -284,7 +284,11 @@ review; a direct squash revert previously conflicted in 31 paths. countdown remain live. The current Runtime/Host suites pass (97 tests) and Core/UI countdown suites pass (8 tests), including remaining-time projection after reconnect and reduced-motion display. This is a baseline review only; -the active classification/Host/UI code has not been independently replaced. +the active Host/UI code has not been independently replaced. A new failing +classification test showed that a standard `Headers` instance on a provider +error lost `Retry-After` even though retryability remained true. Header +extraction now accepts both `Headers` and plain records; the 20 classification +tests and Biome pass. This is a scoped parsing fix, not a full #3115 rewrite. #3111's Daily Review fixture still writes through the interactive storage authority with nested writer/owner cleanup. Its current archive-seeding test diff --git a/packages/runtime/src/__tests__/provider-error-classification.test.ts b/packages/runtime/src/__tests__/provider-error-classification.test.ts index ee864a1ab32..b09e318aa76 100644 --- a/packages/runtime/src/__tests__/provider-error-classification.test.ts +++ b/packages/runtime/src/__tests__/provider-error-classification.test.ts @@ -386,6 +386,17 @@ describe('Provider error classification', () => { assert.equal(providerModelFailure(refusedWithDelay).retryAfterMs, undefined); }); + test('reads Retry-After from a Headers instance without losing retryability', () => { + const error = Object.assign(new Error('provider rejected the request'), { + name: 'AI_APICallError', + statusCode: 429, + responseHeaders: new Headers({ 'Retry-After': '3' }), + }); + const failure = providerModelFailure(error); + assert.equal(failure.retryable, true); + assert.equal(failure.retryAfterMs, 3_000); + }); + test('abort and a spent Codex edge budget override the retryable kinds', () => { const aborted = new RetryError({ message: 'Retry stopped', diff --git a/packages/runtime/src/provider-error-classification.ts b/packages/runtime/src/provider-error-classification.ts index 310490ccaec..b66c7ef7f7d 100644 --- a/packages/runtime/src/provider-error-classification.ts +++ b/packages/runtime/src/provider-error-classification.ts @@ -244,6 +244,12 @@ function responseHeadersFromError(error: unknown): Record | unde const value = (error as { responseHeaders?: unknown }).responseHeaders; if (typeof value !== 'object' || value === null) return undefined; const headers: Record = {}; + if (value instanceof Headers) { + value.forEach((header, key) => { + headers[key.toLowerCase()] = header; + }); + return headers; + } for (const [key, header] of Object.entries(value)) { if (typeof header === 'string') headers[key.toLowerCase()] = header; } From 533085d48b323e1cf4fc0770227b6f0af87fa21f Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:23:24 +0800 Subject: [PATCH 023/110] fix(runtime-host): reject stale queue reorder revisions --- ...me-host-session-execution-ipc-main.test.ts | 7 +++- .../workbar-services-adapter.test.ts | 6 ++- .../__tests__/workhub-send-visibility.test.ts | 2 +- ...runtime-host-session-execution-ipc-main.ts | 3 +- apps/desktop/src/preload/bridge-contract.d.ts | 2 +- apps/desktop/src/preload/preload.ts | 4 +- .../controller/use-session-message-queue.ts | 10 ++++- .../src/renderer/features/workbar/ports.ts | 2 +- .../tools/side-chat/use-quote-companion.ts | 9 ++-- .../controller/use-workhub-controller.ts | 6 ++- .../src/renderer/features/workhub/ports.ts | 2 +- .../desktop/create-workbar-services.ts | 4 +- .../desktop/create-workhub-services.ts | 2 +- docs/grok-assisted-contribution-review.md | 41 ++++++++++++------- .../__tests__/execution-host-message.test.ts | 1 + .../src/__tests__/message-coordinator.test.ts | 21 ++++++++++ .../src/__tests__/protocol.test.ts | 11 +++++ packages/runtime-host/src/protocol/message.ts | 3 ++ .../src/server/message-coordinator.ts | 3 ++ 19 files changed, 108 insertions(+), 31 deletions(-) diff --git a/apps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.ts b/apps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.ts index bd5ad8eeae3..664981b2cb5 100644 --- a/apps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.ts +++ b/apps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.ts @@ -1912,7 +1912,7 @@ test("routes per-entry queue mutations to the Runtime Host", async () => { assert.equal(await ipc.invoke("sessions:retractQueueEntry", "session-1", "entry-1"), undefined); await ipc.invoke("sessions:promoteQueueEntry", "session-1", "entry-2"); - await ipc.invoke("sessions:reorderQueueEntries", "session-1", ["entry-3", "entry-2"]); + await ipc.invoke("sessions:reorderQueueEntries", "session-1", ["entry-3", "entry-2"], 5); assert.deepEqual(calls, [ { @@ -1931,6 +1931,7 @@ test("routes per-entry queue mutations to the Runtime Host", async () => { operation: "reorder", sessionId: "session-1", reorderId: "id-3", + expectedQueueRevision: 5, entryIds: ["entry-3", "entry-2"], }, ]); @@ -1943,6 +1944,10 @@ test("routes per-entry queue mutations to the Runtime Host", async () => { () => ipc.invoke("sessions:reorderQueueEntries", "session-1", ["entry-1", 42]), /Invalid queue entry order/, ); + await assert.rejects( + () => ipc.invoke("sessions:reorderQueueEntries", "session-1", ["entry-1"], -1), + /Invalid Queue sequence/, + ); }); test("binds steer and stop to Host-owned queue and active Turn identities", async () => { diff --git a/apps/desktop/src/main/__tests__/workbar-services-adapter.test.ts b/apps/desktop/src/main/__tests__/workbar-services-adapter.test.ts index 0a670e29d2b..9bfec6cc4ba 100644 --- a/apps/desktop/src/main/__tests__/workbar-services-adapter.test.ts +++ b/apps/desktop/src/main/__tests__/workbar-services-adapter.test.ts @@ -275,7 +275,7 @@ describe('createDesktopWorkbarServices', () => { await services.sideChat.queryMessageExecutions('fork', ['message-next']); await services.sideChat.retractQueueEntry('fork', 'entry-1'); await services.sideChat.promoteQueueEntry('fork', 'entry-2'); - await services.sideChat.reorderQueueEntries('fork', ['entry-3', 'entry-2']); + await services.sideChat.reorderQueueEntries('fork', ['entry-3', 'entry-2'], 5); await services.sideChat.setPermissionMode('fork', 'ask'); await services.sideChat.respondToSandboxBoundary('fork', {} as never); await services.sideChat.respondToClientCapability('fork', {} as never); @@ -376,5 +376,9 @@ describe('createDesktopWorkbarServices', () => { calls.find((call) => call.name === 'sessions.queryMessageExecutions')?.args, ['fork', ['message-next']], ); + assert.deepEqual( + calls.find((call) => call.name === 'sessions.reorderQueueEntries')?.args, + ['fork', ['entry-3', 'entry-2'], 5], + ); }); }); diff --git a/apps/desktop/src/main/__tests__/workhub-send-visibility.test.ts b/apps/desktop/src/main/__tests__/workhub-send-visibility.test.ts index ac5d80b03f9..195bac2e60f 100644 --- a/apps/desktop/src/main/__tests__/workhub-send-visibility.test.ts +++ b/apps/desktop/src/main/__tests__/workhub-send-visibility.test.ts @@ -689,7 +689,7 @@ test('WorkHub sends queue edits, withdrawal and both queue orders to the Host an }); assert.deepEqual(h.queueMutations, [ ['retract', h.controller.sessionId, 'second'], - ['reorder', h.controller.sessionId, ['second', 'first']], + ['reorder', h.controller.sessionId, ['second', 'first'], 7], ['retract', h.controller.sessionId, 'first'], ]); assert.deepEqual(h.restoredDrafts, [[h.controller.sessionId, 'second']], 'edit hands the retracted text back to the draft'); diff --git a/apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts b/apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts index 62fe6dbd302..94f28e357aa 100644 --- a/apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts +++ b/apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts @@ -676,7 +676,7 @@ export function registerRuntimeHostSessionExecutionIpc( ); ipcMain.handle( "sessions:reorderQueueEntries", - async (_event, sessionId: string, entryIds: unknown) => { + async (_event, sessionId: string, entryIds: unknown, expectedQueueRevision: unknown) => { if ( !Array.isArray(entryIds) || entryIds.some((entryId) => typeof entryId !== "string") @@ -686,6 +686,7 @@ export function registerRuntimeHostSessionExecutionIpc( await deps.client.reorderQueueEntries({ sessionId, reorderId: newId(), + expectedQueueRevision: requiredSequence(expectedQueueRevision, "Queue"), entryIds, }); }, diff --git a/apps/desktop/src/preload/bridge-contract.d.ts b/apps/desktop/src/preload/bridge-contract.d.ts index 3e9deaace28..2cfe4093a4e 100644 --- a/apps/desktop/src/preload/bridge-contract.d.ts +++ b/apps/desktop/src/preload/bridge-contract.d.ts @@ -1294,7 +1294,7 @@ export interface MakaBridge { ): Promise; retractQueueEntry(sessionId: string, entryId: string): Promise; promoteQueueEntry(sessionId: string, entryId: string): Promise; - reorderQueueEntries(sessionId: string, entryIds: readonly string[]): Promise; + reorderQueueEntries(sessionId: string, entryIds: readonly string[], expectedQueueRevision: number): Promise; readExecutionBoundary(sessionId: string): Promise; listActiveInteractions(sessionId: string): Promise; subscribeActiveInteractions( diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 72da9a55cab..bb3b965a0f1 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -2348,8 +2348,8 @@ const makaBridge = { promoteQueueEntry(sessionId: string, entryId: string): Promise { return invokeSessionRuntimeHost('sessions:promoteQueueEntry', sessionId, entryId); }, - reorderQueueEntries(sessionId: string, entryIds: readonly string[]): Promise { - return invokeSessionRuntimeHost('sessions:reorderQueueEntries', sessionId, [...entryIds]); + reorderQueueEntries(sessionId: string, entryIds: readonly string[], expectedQueueRevision: number): Promise { + return invokeSessionRuntimeHost('sessions:reorderQueueEntries', sessionId, [...entryIds], expectedQueueRevision); }, readExecutionBoundary(sessionId: string): Promise { return invokeSessionRuntimeHost('sessions:readExecutionBoundary', sessionId); diff --git a/apps/desktop/src/renderer/features/conversation/controller/use-session-message-queue.ts b/apps/desktop/src/renderer/features/conversation/controller/use-session-message-queue.ts index 0b079b217df..dff049d0f6b 100644 --- a/apps/desktop/src/renderer/features/conversation/controller/use-session-message-queue.ts +++ b/apps/desktop/src/renderer/features/conversation/controller/use-session-message-queue.ts @@ -128,6 +128,14 @@ export function useSessionMessageQueue(options: { restoreDraft: (draft) => restoreDraft(targetSessionId, draft), })), deleteQueuedEntry: (entryId) => runAction((targetSessionId) => services.sessions.retractQueueEntry(targetSessionId, entryId)), - reorderQueuedEntries: (entryIds) => runAction((targetSessionId) => services.sessions.reorderQueueEntries(targetSessionId, entryIds)), + reorderQueuedEntries: (entryIds) => { + const expectedQueueRevision = queue?.queueRevision; + if (expectedQueueRevision === undefined) { + return Promise.reject(new Error('Message queue is not ready for reordering')); + } + return runAction((targetSessionId) => + services.sessions.reorderQueueEntries(targetSessionId, entryIds, expectedQueueRevision), + ); + }, }; } diff --git a/apps/desktop/src/renderer/features/workbar/ports.ts b/apps/desktop/src/renderer/features/workbar/ports.ts index d8d79917f5e..440d36ea294 100644 --- a/apps/desktop/src/renderer/features/workbar/ports.ts +++ b/apps/desktop/src/renderer/features/workbar/ports.ts @@ -252,7 +252,7 @@ export interface SideChatSessionPort { ): Promise; retractQueueEntry(sessionId: string, entryId: string): Promise; promoteQueueEntry(sessionId: string, entryId: string): Promise; - reorderQueueEntries(sessionId: string, entryIds: readonly string[]): Promise; + reorderQueueEntries(sessionId: string, entryIds: readonly string[], expectedQueueRevision: number): Promise; setPermissionMode( sessionId: string, mode: PermissionMode, diff --git a/apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts b/apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts index 0803381ee8f..139359aa692 100644 --- a/apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts +++ b/apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts @@ -1639,9 +1639,12 @@ export function useQuoteCompanion(input: UseQuoteCompanionInput): UseQuoteCompan [dropOptimisticUserMessage, messageQueue.entries, runQueueEntryAction, sideChat], ); const reorderQueuedEntries = useCallback( - (entryIds: readonly string[]) => - runQueueEntryAction((id) => sideChat.reorderQueueEntries(id, entryIds)), - [runQueueEntryAction, sideChat], + (entryIds: readonly string[]) => runQueueEntryAction((id) => { + const revision = messageQueue.queueRevision; + if (revision === undefined) throw new Error('Message queue is not ready for reordering'); + return sideChat.reorderQueueEntries(id, entryIds, revision); + }), + [messageQueue.queueRevision, runQueueEntryAction, sideChat], ); const setPermissionMode = useCallback( diff --git a/apps/desktop/src/renderer/features/workhub/controller/use-workhub-controller.ts b/apps/desktop/src/renderer/features/workhub/controller/use-workhub-controller.ts index 2086c721d9a..6ea79e64b3d 100644 --- a/apps/desktop/src/renderer/features/workhub/controller/use-workhub-controller.ts +++ b/apps/desktop/src/renderer/features/workhub/controller/use-workhub-controller.ts @@ -747,7 +747,11 @@ export function useWorkHubController( : undefined, deleteQueuedEntry, promoteQueuedEntry: (entryId: string) => mutateQueue((target) => services.promoteQueueEntry(target, entryId)), - reorderQueuedEntries: (entryIds: readonly string[]) => mutateQueue((target) => services.reorderQueueEntries(target, entryIds)), + reorderQueuedEntries: (entryIds: readonly string[]) => mutateQueue((target) => { + const revision = messageQueue.revision; + if (revision === undefined) throw new Error('Message queue is not ready for reordering'); + return services.reorderQueueEntries(target, entryIds, revision); + }), viewportNavigation, liveTurn, liveTurns, diff --git a/apps/desktop/src/renderer/features/workhub/ports.ts b/apps/desktop/src/renderer/features/workhub/ports.ts index f2f0d8cceed..e874604f998 100644 --- a/apps/desktop/src/renderer/features/workhub/ports.ts +++ b/apps/desktop/src/renderer/features/workhub/ports.ts @@ -67,7 +67,7 @@ export interface WorkHubServices extends WorkHubWorkspaceServices { queryMessageExecutions(sessionId: string, messageIds: readonly string[]): Promise; retractQueueEntry(sessionId: string, entryId: string): Promise; promoteQueueEntry(sessionId: string, entryId: string): Promise; - reorderQueueEntries(sessionId: string, entryIds: readonly string[]): Promise; + reorderQueueEntries(sessionId: string, entryIds: readonly string[], expectedQueueRevision: number): Promise; configureModel( sessionId: string, input: OperationInput<'workhub.coordination.configureModel'>, diff --git a/apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts b/apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts index c6554dd9453..319fdb82e5a 100644 --- a/apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts +++ b/apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts @@ -205,8 +205,8 @@ export function createDesktopWorkbarServices( bridge.sessions.retractQueueEntry(sessionId, entryId), promoteQueueEntry: (sessionId, entryId) => bridge.sessions.promoteQueueEntry(sessionId, entryId), - reorderQueueEntries: (sessionId, entryIds) => - bridge.sessions.reorderQueueEntries(sessionId, entryIds), + reorderQueueEntries: (sessionId, entryIds, revision) => + bridge.sessions.reorderQueueEntries(sessionId, entryIds, revision), setPermissionMode: async (sessionId, mode) => expectSessionUpdate(await bridge.sessions.setPermissionMode(sessionId, mode)), respondToSandboxBoundary: (sessionId, response) => diff --git a/apps/desktop/src/renderer/platform/desktop/create-workhub-services.ts b/apps/desktop/src/renderer/platform/desktop/create-workhub-services.ts index 33c66b8f73a..cd264394c33 100644 --- a/apps/desktop/src/renderer/platform/desktop/create-workhub-services.ts +++ b/apps/desktop/src/renderer/platform/desktop/create-workhub-services.ts @@ -114,7 +114,7 @@ export function createDesktopWorkHubServices( bridge.sessions.queryMessageExecutions(sessionId, messageIds), retractQueueEntry: (sessionId, entryId) => bridge.sessions.retractQueueEntry(sessionId, entryId), promoteQueueEntry: (sessionId, entryId) => bridge.sessions.promoteQueueEntry(sessionId, entryId), - reorderQueueEntries: (sessionId, entryIds) => bridge.sessions.reorderQueueEntries(sessionId, entryIds), + reorderQueueEntries: (sessionId, entryIds, revision) => bridge.sessions.reorderQueueEntries(sessionId, entryIds, revision), configureModel: (sessionId, input) => bridge.workHub.configureModel(sessionId, input), getNewWorkDefaults: (sessionId) => bridge.workHub.getNewWorkDefaults(sessionId), setNewWorkDefaults: (sessionId, defaults) => diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index b9fae85cc2d..f6d060fa226 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -20,7 +20,7 @@ # Grok-assisted contribution review **Baseline:** `d89ccce01` (2026-09-27). **Status:** all 25 original-commit -inventories reviewed; four narrow replacements verified locally, four further +inventories reviewed; four narrow replacements verified locally, seven further PRs have scoped fixes, and a mixed-tool architecture document has been corrected. Remaining behavior reviews and whole-PR dispositions are open. @@ -187,22 +187,27 @@ identical revert/reapply. They are reviewed, **not** counted as replacements: clearing, MCP reconnect, artifact get, and skill starter/details) have no present Desktop callers or registered handlers in the source tree. Its original branch also includes an untagged merge and an E2E follow-up. Do not - resurrect the discarded IPC authority merely to delete it again; the - surviving E2E changes remain a separate test review. + resurrect the discarded IPC authority merely to delete it again. Its two + touched E2E files were later deleted (#5567 and #4752); the surviving + `/compact` Electron E2E passes (1 test), and the slash availability and + Skills controller tests pass (21 tests). The original E2E paths no longer + provide a live test target. - #3104 removed a dead `runAgentGraphToQuiescence` loop and its test suite. A current source search finds no call sites for that function; the migrated `stream-graph-dispatch.ts` exports supervisor types still used by the live coordinator and reconciliation code. Restoring an inactive second graph driver would create another execution authority. Its remaining coordinator - and protocol test adjustments require focused verification, but the deleted - loop itself has no behavior to independently rewrite. + and protocol test adjustments pass with the current Runtime read-model and + coordinator suites (31 tests). The deleted loop has no behavior to + independently rewrite. - #3117 deleted the legacy `llm-connections.json` store. Current storage and Desktop main source has no production reference to that file or `createConnectionStore`; the fixture writes to the Runtime Policy catalog through its current storage authority. Its fixture test passes and asserts - the old JSON file is not created. The fixture and related migration edits - remain active and require their own review; restoring the removed legacy - store just to remove it again is not a remedy. + the old JSON file is not created (rerun against the current Desktop build). + The fixture and related migration edits remain active and require their + own review; restoring the removed legacy store just to remove it again is + not a remedy. - #3069 only added local transcript-search tests; both original test paths were subsequently deleted (#4877 and #5531). Search now uses the Recall pipeline. The current multi-host Recall search tests pass (9 tests), but @@ -262,15 +267,14 @@ ready at the helper boundary. Completion now carries the exact generation token through the observation hook and refuses to flush/display a newer seed for an older signal. The three test files now pass (89 tests) and Biome passes. This is a scoped independent hardening, **not** a replacement for the entire -two tagged implementations. The returning-to-live-conversation Electron E2E -passes; a separate Host-recovery E2E remains open. Desktop's full main build +two tagged implementations. The full streaming-remount Electron E2E (4 tests) +and session-local recovery E2E (3 tests) pass. Desktop's full main build still reports seven unrelated implicit-any errors. #3101's only squash change modified the streaming-remount E2E to sample on animation frames instead of body mutations. The current test has since gained -other assertions; its returning-to-live-conversation case passes in a real -Electron window. This validates the existing assertion, not an independently -rewritten test or the rest of that E2E file. +other assertions; the full file passes in a real Electron window (4 tests). +This validates the existing assertions, not an independently rewritten test. #3544's first six original commits are Grok-tagged and the last three are Codex-tagged. The per-entry queue still has Host protocol, coordinator, and @@ -278,7 +282,15 @@ Desktop UI behavior, though the original Desktop action module has since been removed. The current Host message-coordinator and protocol suites pass (165 tests). That baseline does not constitute a replacement for its 35-file mixed change. The Host/protocol/UI slices and Electron workflow need separate -review; a direct squash revert previously conflicted in 31 paths. +review; a direct squash revert previously conflicted in 31 paths. A new +same-members concurrent-reorder test failed because `queue.entries.reorder` +had no expected queue revision, letting an old client's permutation overwrite +a newer one. The Host now checks a required revision, and Desktop main, +preload, WorkHub, and Side Chat pass through the observed revision. Host +message/protocol tests (175 pass), Desktop main/preload/renderer typechecks, +Desktop queue tests, and Side Chat/WorkHub Electron E2Es pass after rebuilding +the preload and renderer. This is a scoped concurrency fix, not a full rewrite +of the mixed 35-file PR. #3115's rate-limit classification, Host retry projection/continuity, and UI countdown remain live. The current Runtime/Host suites pass (97 tests) and @@ -294,6 +306,7 @@ tests and Biome pass. This is a scoped parsing fix, not a full #3115 rewrite. authority with nested writer/owner cleanup. Its current archive-seeding test passes, but there is no independent replacement for this live fixture writer; the old Desktop archive store was deleted and has no direct current file. +The current fixture test was rerun (1 pass). #3070 and #3099 were replaced in the current branch after reviewing their migrated implementations. #3070's historical cwd resolution remains distinct diff --git a/packages/runtime-host/src/__tests__/execution-host-message.test.ts b/packages/runtime-host/src/__tests__/execution-host-message.test.ts index 58b8ae5f705..36df142ce59 100644 --- a/packages/runtime-host/src/__tests__/execution-host-message.test.ts +++ b/packages/runtime-host/src/__tests__/execution-host-message.test.ts @@ -236,6 +236,7 @@ test('steering becomes durable and ordered followups automatically start the nex originHostEpoch: host.hostEpoch, sessionId: fixture.sessionId, reorderId: randomUUID(), + expectedQueueRevision: queueSubscription.snapshot.queue.queueRevision, entryIds: queuedFollowups.map((entry) => entry.entryId).reverse(), }); await queueSubscription.close(); diff --git a/packages/runtime-host/src/__tests__/message-coordinator.test.ts b/packages/runtime-host/src/__tests__/message-coordinator.test.ts index d1ca4bb4acb..fae4265ee47 100644 --- a/packages/runtime-host/src/__tests__/message-coordinator.test.ts +++ b/packages/runtime-host/src/__tests__/message-coordinator.test.ts @@ -2353,6 +2353,7 @@ test('entries reorder permutes the follow-up queue and rejects stale orders', as originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-1', + expectedQueueRevision: revisionBefore, entryIds: ['id-3', 'id-1', 'id-2'], }, operationContext(), @@ -2370,17 +2371,33 @@ test('entries reorder permutes the follow-up queue and rejects stale orders', as originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-1', + expectedQueueRevision: revisionBefore, entryIds: ['id-3', 'id-1', 'id-2'], }, operationContext(), ); assert.deepEqual(retry, reordered); + const staleMembers = { + originHostEpoch: 'epoch-1', + sessionId: ROOT.sessionId, + reorderId: 'reorder-stale-members', + entryIds: ['id-2', 'id-3', 'id-1'], + expectedQueueRevision: revisionBefore, + }; + const overwritten = await fixture.coordinator.handlers['queue.entries.reorder']( + staleMembers, + operationContext(), + ); + assert.equal(overwritten.ok, false); + if (!overwritten.ok) assert.equal(overwritten.error.code, 'operation_conflict'); + const stale = await fixture.coordinator.handlers['queue.entries.reorder']( { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-2', + expectedQueueRevision: revisionBefore, entryIds: ['id-2', 'id-1'], }, operationContext(), @@ -2393,6 +2410,7 @@ test('entries reorder permutes the follow-up queue and rejects stale orders', as originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-3', + expectedQueueRevision: revisionBefore + 1, entryIds: ['id-3', 'id-1', 'id-2'], }, operationContext(), @@ -2481,6 +2499,7 @@ test('steering edits and reorders settle before one complete batch is pulled; fo originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-steering', + expectedQueueRevision: fixture.coordinator.projection(ROOT.sessionId).queueRevision, entryIds: [entries[2]!.entryId, entries[0]!.entryId, entries[1]!.entryId], }, operationContext(), @@ -2552,6 +2571,7 @@ test('queued mutations reject a queue that is draining into the next Turn', asyn originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-draining', + expectedQueueRevision: fixture.coordinator.projection(ROOT.sessionId).queueRevision, entryIds: ['id-2', 'id-1'], }, operationContext(), @@ -2569,6 +2589,7 @@ test('queued mutations reject a queue that is draining into the next Turn', asyn originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, reorderId: 'reorder-after-commit', + expectedQueueRevision: fixture.coordinator.projection(ROOT.sessionId).queueRevision, entryIds: ['id-2'], }, operationContext(), diff --git a/packages/runtime-host/src/__tests__/protocol.test.ts b/packages/runtime-host/src/__tests__/protocol.test.ts index c56c194b590..ad2f3422876 100644 --- a/packages/runtime-host/src/__tests__/protocol.test.ts +++ b/packages/runtime-host/src/__tests__/protocol.test.ts @@ -1645,6 +1645,7 @@ describe('Runtime Host bootstrap protocol', () => { originHostEpoch: 'epoch-1', sessionId: 'session-1', reorderId: 'reorder-1', + expectedQueueRevision: 7, entryIds: ['entry-2', 'entry-1'], }, }; @@ -1652,6 +1653,16 @@ describe('Runtime Host bootstrap protocol', () => { assert.deepEqual(decodeClientFrame(entryPromote), entryPromote); assert.deepEqual(decodeClientFrame(entryUpdate), entryUpdate); assert.deepEqual(decodeClientFrame(entriesReorder), entriesReorder); + for (const expectedQueueRevision of [undefined, -1, 1.5]) { + assert.throws( + () => + decodeClientFrame({ + ...entriesReorder, + input: { ...entriesReorder.input, expectedQueueRevision }, + }), + isInvalidFrame, + ); + } assert.throws( () => decodeClientFrame({ diff --git a/packages/runtime-host/src/protocol/message.ts b/packages/runtime-host/src/protocol/message.ts index 21819e6417d..7f445b4d29f 100644 --- a/packages/runtime-host/src/protocol/message.ts +++ b/packages/runtime-host/src/protocol/message.ts @@ -188,6 +188,7 @@ export interface QueueEntriesReorderInput { readonly originHostEpoch: string; readonly sessionId: string; readonly reorderId: string; + readonly expectedQueueRevision: number; readonly entryIds: readonly string[]; } @@ -569,6 +570,7 @@ function decodeQueueEntriesReorderInput(value: unknown): QueueEntriesReorderInpu 'originHostEpoch', 'sessionId', 'reorderId', + 'expectedQueueRevision', 'entryIds', ]); const entryIds = requireBoundedArray(record.entryIds, 'reorder entry identities').map((entryId) => @@ -581,6 +583,7 @@ function decodeQueueEntriesReorderInput(value: unknown): QueueEntriesReorderInpu originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), sessionId: requireEntityId(record.sessionId, 'sessionId'), reorderId: requireEntityId(record.reorderId, 'reorderId'), + expectedQueueRevision: requireCount(record.expectedQueueRevision, 'expectedQueueRevision'), entryIds, }; } diff --git a/packages/runtime-host/src/server/message-coordinator.ts b/packages/runtime-host/src/server/message-coordinator.ts index e4e83acc49b..d4ccde46c9f 100644 --- a/packages/runtime-host/src/server/message-coordinator.ts +++ b/packages/runtime-host/src/server/message-coordinator.ts @@ -2029,6 +2029,9 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { if (state.transition) { return failure('operation_conflict', 'Message queue is draining into the next Turn'); } + if (state.revision !== input.expectedQueueRevision) { + return failure('operation_conflict', 'Message queue changed since the reorder was issued'); + } const steering = state.steering.some((entry) => entry.entryId === input.entryIds[0]); const current = steering ? state.steering : state.followup; if (input.entryIds.length !== current.length) { From d009564ac11408946b613c5cbbc2db4567ff6513 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:28:51 +0800 Subject: [PATCH 024/110] fix(runtime-host): bound retry wait after clock rollback --- docs/grok-assisted-contribution-review.md | 13 ++++++--- .../src/__tests__/session-projector.test.ts | 27 +++++++++++++++++++ .../src/adapter/session-projector.ts | 4 ++- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index f6d060fa226..8a6e6a6d4d6 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -226,12 +226,15 @@ rollback: to `packages/core/src/__tests__/relative-time.test.ts`. The relative-time behavior and tests came from Maka-tagged commits, with a later untagged follow-up. Keep the required license header and do not revert the formatter - under the squash commit's aggregate Grok trailer. + under the squash commit's aggregate Grok trailer. The current formatter and + refresh tests pass (8 tests). - #3123's three original PR commits and PR description do not disclose Grok, despite the squash commit's trailer. Its only changed file, `apps/desktop/e2e/prompt-rail.spec.ts`, was deleted by #4741. The original provenance is unresolved; there is no surviving test file to revert and - reimplement mechanically. + reimplement mechanically. #4741 removed the old layout E2E after repeated + compositor-dependent failures; that removal does not attribute its earlier + test lines to any model. #3008 is only partly recognizable in today's Eval egress filter. Its CONNECT host classification remains, but #3017 subsequently replaced the raw-TCP @@ -300,7 +303,11 @@ the active Host/UI code has not been independently replaced. A new failing classification test showed that a standard `Headers` instance on a provider error lost `Retry-After` even though retryability remained true. Header extraction now accepts both `Headers` and plain records; the 20 classification -tests and Biome pass. This is a scoped parsing fix, not a full #3115 rewrite. +tests and Biome pass. A second failing test showed a backward Host clock +adjustment projecting more remaining wait than the original scheduled delay; +the projector now caps elapsed time at zero. Projector/continuity tests +(79 pass) and Biome pass. These are scoped parsing and projection fixes, not +a full #3115 rewrite. #3111's Daily Review fixture still writes through the interactive storage authority with nested writer/owner cleanup. Its current archive-seeding test diff --git a/packages/runtime-host/src/__tests__/session-projector.test.ts b/packages/runtime-host/src/__tests__/session-projector.test.ts index fcdfef9c070..57e966f1878 100644 --- a/packages/runtime-host/src/__tests__/session-projector.test.ts +++ b/packages/runtime-host/src/__tests__/session-projector.test.ts @@ -586,6 +586,33 @@ test('reseeds a scheduled retry with remainingMs recomputed from the stored sche assert.equal(retry.remainingMs, 39_995); }); +test('a backward Host clock adjustment cannot lengthen a scheduled retry', () => { + const canonical = snapshot({ + rootTurn: { + sessionId: 'session-1', + turnId: 'turn-1', + runId: 'run-1', + status: 'running', + providerRetry: { + phase: 'scheduled' as const, + attempt: 1, + maxAttempts: 3, + delayMs: 40_000, + ts: 100, + reason: 'rate_limit' as const, + }, + }, + }); + const projector = new RuntimeHostSessionProjector( + canonical, + createRuntimeHostSessionProjectionSeed([], snapshot()), + () => 50, + ); + const retry = projector.seedActive(true)[0]; + assert.ok(retry && retry.type === 'provider_retry' && retry.phase === 'scheduled'); + assert.equal(retry.remainingMs, retry.delayMs); +}); + test('emits a live provider retry when the snapshot overlay appears, then drops it after content', () => { const projector = new RuntimeHostSessionProjector( snapshot(), diff --git a/packages/runtime-host/src/adapter/session-projector.ts b/packages/runtime-host/src/adapter/session-projector.ts index f535c7262f1..4364c010474 100644 --- a/packages/runtime-host/src/adapter/session-projector.ts +++ b/packages/runtime-host/src/adapter/session-projector.ts @@ -851,7 +851,9 @@ function providerRetryEvent( // the countdown. Snapshots from older runtimes lack `ts` and degrade to the // full delay. const remainingMs = - retry.ts === undefined ? retry.delayMs : Math.max(0, retry.delayMs - (ts - retry.ts)); + retry.ts === undefined + ? retry.delayMs + : Math.max(0, retry.delayMs - Math.max(0, ts - retry.ts)); return { type: 'provider_retry', id: `host-seed:${root.runId}:provider_retry`, From 7dacaf033045046662b819ab29527742aa93c455 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:36:09 +0800 Subject: [PATCH 025/110] docs: reconcile remaining Grok audit dispositions after upstream merge --- docs/grok-assisted-contribution-review.md | 29 +++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 8a6e6a6d4d6..2e012e0a9b3 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -19,7 +19,8 @@ # Grok-assisted contribution review -**Baseline:** `d89ccce01` (2026-09-27). **Status:** all 25 original-commit +**Baseline:** `c9a176e10` (2026-09-27; merged into the review branch after +the initial `d89ccce01` pass). **Status:** all 25 original-commit inventories reviewed; four narrow replacements verified locally, seven further PRs have scoped fixes, and a mixed-tool architecture document has been corrected. Remaining behavior reviews and whole-PR dispositions are open. @@ -331,9 +332,9 @@ and the Electron renderer-reload E2E passes in this branch. not reverted just to reproduce an obsolete selector. - #3078: the inventory checker now exposes a pure drift comparison, with tests for exact bytes, independently stale Markdown, and missing/extra paths. The - CI planner also recognizes the new test. The real 300-file inventory check, - 22 Astryx tests, 40 CI planner tests, and Biome passed. The generator and its - later fail-closed dependency parser remain unchanged. + CI planner also recognizes the new test. The real 301-file inventory check + (after merging upstream), 22 Astryx tests, 40 CI planner tests, and Biome + passed. The generator and its later fail-closed dependency parser remain unchanged. - #2967: a new boundary test showed the audit writer could append a record across `MAX_AUDIT_BYTES` without recording `audit_truncated` until another event arrived. The writer now checks the encoded record length before @@ -349,6 +350,26 @@ and the Electron renderer-reload E2E passes in this branch. respond. #3017 owns the later raw-TCP closure behavior; do not roll that implementation back as part of #3008. +### Remaining 21: current disposition + +These are the 25 candidate PRs minus the four narrow replacements above. A +scoped fix is **not** a whole-PR rewrite, and a removed feature is not a +reason to restore its old implementation just to revert it again. + +| Disposition | PRs | Next evidence or action | +|---|---|---| +| Deleted or superseded original path (8) | #3063, #3118, #3119, #3102, #3104, #3117, #3069, #3106 | Preserve the current replacement/removal; #3117's fixture and #3106's surviving startup behavior remain separately reviewable. | +| Mixed or uncertain attribution (2) | #3364, #3123 | Keep Maka-authored formatter and required ASF header; resolve #3123 squash-versus-original provenance with maintainers. | +| Active code with scoped fixes (7) | #3008, #2967, #3048, #3066, #3078, #3115, #3544 | Review the rest of each current behavior against later dependents; the #3008 live proxy test still needs Docker. | +| Active test, fixture, or mixed feature with passing regression only (3) | #3101, #3111, #3459 | Decide whether surviving behavior requires replacement, including the tagged follow-ups in #3459. | +| Documentation-only mixed-tool PR (1) | #4345 | Factual correction made; maintainers decide provenance remedy for the surviving architecture document. | + +The branch has been built against `c9a176e10`: `npm run build:with-deps` +passes, as do 214 Host/Runtime tests, 82 Desktop queue tests, and two real +Electron queue E2Es. These checks do not substitute for the unresolved +per-PR review or the live proxy test. No branch push or PR update has been +made for these local review commits. + ## Review protocol For each row, verify the original behavior and tests against current `main`, From 5e742ff2d0a5f9efebbb6a6ec12948fc90b7b2ec Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:37:24 +0800 Subject: [PATCH 026/110] test(desktop): verify Daily Review fixture releases root after failure --- .../__tests__/daily-review-fixture.test.ts | 23 +++++++++++++++++++ docs/grok-assisted-contribution-review.md | 9 ++++---- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/main/__tests__/daily-review-fixture.test.ts b/apps/desktop/src/main/__tests__/daily-review-fixture.test.ts index 101dd161f9e..6924437685b 100644 --- a/apps/desktop/src/main/__tests__/daily-review-fixture.test.ts +++ b/apps/desktop/src/main/__tests__/daily-review-fixture.test.ts @@ -58,3 +58,26 @@ test('Daily Review fixture seeds archives through the storage authority', async await rm(workspaceRoot, { recursive: true, force: true }); } }); + +test('Daily Review fixture releases its root owner after a rejected archive', async () => { + const workspaceRoot = await mkdtemp(join(tmpdir(), 'maka-daily-review-fixture-error-')); + try { + await assert.rejects(writeDailyReviewArchives(workspaceRoot, Number.NaN), /generatedAt/); + + const capability = await resolveStorageRoot({ path: workspaceRoot, kind: 'interactive' }); + const owner = await tryAcquireInteractiveRootOwner(capability); + assert.ok(owner, 'a failed fixture write must not retain the root owner'); + try { + const writer = await openInteractiveDailyReviewAuthorityForWrite(owner.lease); + try { + assert.deepEqual((await writer.listArchivePage(null, 180)).archives, []); + } finally { + writer.close(); + } + } finally { + await owner.close(); + } + } finally { + await rm(workspaceRoot, { recursive: true, force: true }); + } +}); diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 2e012e0a9b3..944cff1c001 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -311,10 +311,11 @@ the projector now caps elapsed time at zero. Projector/continuity tests a full #3115 rewrite. #3111's Daily Review fixture still writes through the interactive storage -authority with nested writer/owner cleanup. Its current archive-seeding test -passes, but there is no independent replacement for this live fixture writer; +authority with nested writer/owner cleanup. Its archive-seeding test passes; +a new failure-path test rejects an invalid archive, reacquires the same root +owner, and confirms that no partial archive was written (2 tests pass). This +validates cleanup rather than independently replacing the live fixture writer; the old Desktop archive store was deleted and has no direct current file. -The current fixture test was rerun (1 pass). #3070 and #3099 were replaced in the current branch after reviewing their migrated implementations. #3070's historical cwd resolution remains distinct @@ -361,7 +362,7 @@ reason to restore its old implementation just to revert it again. | Deleted or superseded original path (8) | #3063, #3118, #3119, #3102, #3104, #3117, #3069, #3106 | Preserve the current replacement/removal; #3117's fixture and #3106's surviving startup behavior remain separately reviewable. | | Mixed or uncertain attribution (2) | #3364, #3123 | Keep Maka-authored formatter and required ASF header; resolve #3123 squash-versus-original provenance with maintainers. | | Active code with scoped fixes (7) | #3008, #2967, #3048, #3066, #3078, #3115, #3544 | Review the rest of each current behavior against later dependents; the #3008 live proxy test still needs Docker. | -| Active test, fixture, or mixed feature with passing regression only (3) | #3101, #3111, #3459 | Decide whether surviving behavior requires replacement, including the tagged follow-ups in #3459. | +| Active test, fixture, or mixed feature with passing regression only (3) | #3101, #3111, #3459 | #3111 now covers owner release on failed publish; decide whether surviving behavior requires replacement, including the tagged follow-ups in #3459. | | Documentation-only mixed-tool PR (1) | #4345 | Factual correction made; maintainers decide provenance remedy for the surviving architecture document. | The branch has been built against `c9a176e10`: `npm run build:with-deps` From 920d9cb304b286947bc264508597f52df2005a41 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:41:05 +0800 Subject: [PATCH 027/110] docs: update Desktop build verification after upstream merge --- docs/grok-assisted-contribution-review.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 944cff1c001..2ebaec8c786 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -158,8 +158,8 @@ changes. It does not erase the original history or settle ASF legal questions. |---:|---|---| | #3082 | Removing the PTY exit reconciliation made the delayed-persist test return `running` instead of `completed`. | Reconcile the control reply after persistence against finalization, then mark terminal observations. Replaced the Grok-authored test with a fresh test that pauses storage and observes driver exit without monkeypatching the driver prototype. Runtime build, shell-run-manager suite (62 pass, 4 platform skips), and Biome passed. | | #5223 | Removing the directory check made the regular-file test fail with `Missing expected rejection`. | Check the canonical path's stat before Git discovery and reject non-directories with `TypeError`. Replaced the original test with Git/non-Git file cases, registration non-mutation, and a directory control. Storage build, project-catalog suite (23 pass), and Biome passed. | -| #3070 | Removing explicit nested-project selection made registration return the parent project's ID and broke relinking to a child directory. | Make resolution intent explicit: selected paths retain a nested folder identity, while historical paths and selected repository roots keep Git identity. Storage build, project-catalog suite (24 pass), Desktop nested-selection test, and Biome passed. Desktop full build remains blocked by seven unrelated implicit-`any` diagnostics. | -| #3099 | Disconnecting the migrated rail store's grouping read/write made a fresh store lose the selected grouping (`undefined` persisted value). | Let the rail layout store hydrate and write grouping directly, removing the old standalone read/write helpers. Rail layout tests (6 pass), navigation boundary tests (4 pass), and Biome passed. Desktop full build has the same seven unrelated implicit-`any` diagnostics. | +| #3070 | Removing explicit nested-project selection made registration return the parent project's ID and broke relinking to a child directory. | Make resolution intent explicit: selected paths retain a nested folder identity, while historical paths and selected repository roots keep Git identity. Storage build, project-catalog suite (24 pass), Desktop nested-selection test, and Biome passed. The full Desktop build initially had seven unrelated implicit-`any` diagnostics; it passed after the latest upstream merge. | +| #3099 | Disconnecting the migrated rail store's grouping read/write made a fresh store lose the selected grouping (`undefined` persisted value). | Let the rail layout store hydrate and write grouping directly, removing the old standalone read/write helpers. Rail layout tests (6 pass), navigation boundary tests (4 pass), and Biome passed. The full Desktop build initially had the same seven unrelated diagnostics; it passed after the latest upstream merge. | The #3082 and #5223 revert-state commits are separate from the replacement commit to make the failure evidence inspectable. They are not safe to merge @@ -272,8 +272,9 @@ token through the observation hook and refuses to flush/display a newer seed for an older signal. The three test files now pass (89 tests) and Biome passes. This is a scoped independent hardening, **not** a replacement for the entire two tagged implementations. The full streaming-remount Electron E2E (4 tests) -and session-local recovery E2E (3 tests) pass. Desktop's full main build -still reports seven unrelated implicit-any errors. +and session-local recovery E2E (3 tests) pass. Desktop's full build also +passes after merging the latest upstream; it initially reported seven +unrelated implicit-`any` diagnostics. #3101's only squash change modified the streaming-remount E2E to sample on animation frames instead of body mutations. The current test has since gained From 04e9fdf7e2a2df8f7412cab0ab8369c71aaa9d8b Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:44:19 +0800 Subject: [PATCH 028/110] test(desktop): verify terminal query parser routing --- .../__tests__/session-terminal-query.test.ts | 27 +++++++++++++++++++ docs/grok-assisted-contribution-review.md | 5 ++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/main/__tests__/session-terminal-query.test.ts b/apps/desktop/src/main/__tests__/session-terminal-query.test.ts index fdd580e4592..78b761af3e0 100644 --- a/apps/desktop/src/main/__tests__/session-terminal-query.test.ts +++ b/apps/desktop/src/main/__tests__/session-terminal-query.test.ts @@ -166,3 +166,30 @@ test('registers and disposes every xterm response-generating query handler', () queryReplies.dispose(); assert.equal(disposed, registered.length); }); + +test('registered handlers leave cursor reports to xterm while suppressing capability replies', () => { + const handlers = new Map boolean>(); + const disposable = (): IDisposable => ({ dispose() {} }); + const parser = { + registerOscHandler: () => disposable(), + registerCsiHandler: (id: unknown, callback: (params: (number | number[])[]) => boolean) => { + handlers.set(JSON.stringify(id), callback); + return disposable(); + }, + registerDcsHandler: () => disposable(), + } as unknown as IParser; + const registration = suppressTerminalQueryReplies({ parser, write() {} }); + try { + const status = handlers.get(JSON.stringify({ final: 'n' })); + const version = handlers.get(JSON.stringify({ prefix: '>', final: 'q' })); + assert.ok(status); + assert.ok(version); + assert.equal(status([5]), true); + assert.equal(status([6]), false); + assert.equal(handlers.has(JSON.stringify({ prefix: '?', final: 'n' })), false); + assert.equal(version([0]), true); + assert.equal(version([1]), false); + } finally { + registration.dispose(); + } +}); diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 2ebaec8c786..b2117dd6935 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -248,8 +248,9 @@ separate security review with unit and live-proxy regression checks. handling. Its three Grok-tagged follow-ups suppress XTVERSION replies, preserve cursor-position reports, and mark CLI `/transcript` as local while a turn runs. The implementation moved to the workbar terminal feature; the three behaviors -and their unit tests survive. The Desktop query suite (6 tests) and CLI -mid-turn `/transcript` test pass. Reverting the squash would remove the +and their unit tests survive. A new registered-handler test verifies status, +CPR, and XTVERSION routing at the parser boundary. The Desktop query suite +(7 tests) and CLI mid-turn `/transcript` test pass. Reverting the squash would remove the untagged implementation too. Review the tagged behaviors at their current boundaries, not the terminal feature as a wholly Grok-authored change. From 220396b7537550970c5a5cb108e8585409a78fee Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:56:59 +0800 Subject: [PATCH 029/110] fix(eval): scope trial framework selection to execution --- docs/grok-assisted-contribution-review.md | 11 +++++++---- packages/eval/harbor/eval_framework.py | 13 +++++++++++++ packages/eval/harbor/run_trial.py | 8 ++++++-- packages/eval/harbor/test_eval_framework.py | 9 +++++++++ packages/eval/harbor/test_run_trial_policy.py | 12 ++++++++++++ 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index b2117dd6935..1b98bf83da4 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -329,10 +329,13 @@ and the Electron renderer-reload E2E passes in this branch. - #3066: a new concurrent-trial test failed under the process-global framework selection (`harbor` observed `pier`). Selection now uses a Python `ContextVar`; - Python 3.13 Eval tests (88 pass, 12 skips), Eval TypeScript build, and 19 - lifecycle tests passed. The original TypeScript removal of the environment - selector remains in place and was verified by the lifecycle suite; it was - not reverted just to reproduce an obsolete selector. + the next ablation showed that a failed trial leaked its framework into the + caller's context. `run_trial` now scopes selection with a `ContextVar` token + and restores the caller's framework after success or error. Python 3.13 + Harbor tests (93 total, 13 skips), Eval TypeScript build, and 19 lifecycle + tests passed. The original TypeScript removal of the environment selector + remains in place and was verified by the lifecycle suite; it was not + reverted just to reproduce an obsolete selector. - #3078: the inventory checker now exposes a pure drift comparison, with tests for exact bytes, independently stale Markdown, and missing/extra paths. The CI planner also recognizes the new test. The real 301-file inventory check diff --git a/packages/eval/harbor/eval_framework.py b/packages/eval/harbor/eval_framework.py index 0d7bcd66b5c..2c669bb3d27 100644 --- a/packages/eval/harbor/eval_framework.py +++ b/packages/eval/harbor/eval_framework.py @@ -19,7 +19,9 @@ from __future__ import annotations +from contextlib import contextmanager from contextvars import ContextVar +from typing import Iterator _FRAMEWORKS = frozenset({"harbor", "pier"}) _framework: ContextVar[str | None] = ContextVar("eval_framework", default=None) @@ -31,6 +33,17 @@ def install(framework: str) -> None: _framework.set(framework) +@contextmanager +def selected_framework(framework: str) -> Iterator[None]: + if framework not in _FRAMEWORKS: + raise RuntimeError("framework must be harbor or pier") + token = _framework.set(framework) + try: + yield + finally: + _framework.reset(token) + + def selected() -> str: framework = _framework.get() if framework not in _FRAMEWORKS: diff --git a/packages/eval/harbor/run_trial.py b/packages/eval/harbor/run_trial.py index 9419ef67a16..cef98b8bf60 100644 --- a/packages/eval/harbor/run_trial.py +++ b/packages/eval/harbor/run_trial.py @@ -117,9 +117,13 @@ def publish_ready_task(namespace: Path, target: Path) -> None: async def run_trial(framework: str, expected_version: str, config_file: Path) -> None: - from eval_framework import install + from eval_framework import selected_framework - install(framework) + with selected_framework(framework): + await _run_selected_trial(framework, expected_version, config_file) + + +async def _run_selected_trial(framework: str, expected_version: str, config_file: Path) -> None: distribution = {"harbor": "harbor", "pier": "datacurve-pier"}[framework] if importlib.metadata.version(distribution) != expected_version: raise FrameworkVersionMismatch diff --git a/packages/eval/harbor/test_eval_framework.py b/packages/eval/harbor/test_eval_framework.py index 992505153ab..887f271f2b2 100644 --- a/packages/eval/harbor/test_eval_framework.py +++ b/packages/eval/harbor/test_eval_framework.py @@ -41,6 +41,15 @@ def test_install_selects_harbor_and_pier(self) -> None: module.install("pier") self.assertEqual(module.selected(), "pier") + def test_scoped_selection_restores_prior_framework_after_nested_failure(self) -> None: + module = self._load_fresh() + module.install("harbor") + with self.assertRaisesRegex(ValueError, "trial failed"): + with module.selected_framework("pier"): + self.assertEqual(module.selected(), "pier") + raise ValueError("trial failed") + self.assertEqual(module.selected(), "harbor") + def test_concurrent_trials_do_not_share_framework_selection(self) -> None: module = self._load_fresh() diff --git a/packages/eval/harbor/test_run_trial_policy.py b/packages/eval/harbor/test_run_trial_policy.py index 13e7d398149..4f32c205ea0 100644 --- a/packages/eval/harbor/test_run_trial_policy.py +++ b/packages/eval/harbor/test_run_trial_policy.py @@ -59,6 +59,18 @@ def test_invalid_framework_fails_before_framework_import(self) -> None: asyncio.run(MODULE.run_trial("other", "1.0.0", Path("missing.json"))) imported.assert_not_called() + def test_failed_trial_restores_the_callers_framework(self) -> None: + import eval_framework + + async def run() -> None: + with eval_framework.selected_framework("pier"): + with patch.object(MODULE.importlib.metadata, "version", return_value="different"): + with self.assertRaises(MODULE.FrameworkVersionMismatch): + await MODULE.run_trial("harbor", "1.0.0", Path("missing.json")) + self.assertEqual(eval_framework.selected(), "pier") + + asyncio.run(run()) + def test_main_installs_the_argv_framework_before_the_trial(self) -> None: import eval_framework From 2b82ea596b7a1fc8380abedd16aadb1a91c37292 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 14:58:39 +0800 Subject: [PATCH 030/110] fix(eval): reject malformed UTF-8 in egress audit records --- docs/grok-assisted-contribution-review.md | 11 +++++++---- packages/eval/src/__tests__/egress-audit.test.ts | 9 +++++++++ packages/eval/src/harness-executor.ts | 14 ++++++++++---- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 1b98bf83da4..5460882b23d 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -344,10 +344,13 @@ and the Electron renderer-reload E2E passes in this branch. - #2967: a new boundary test showed the audit writer could append a record across `MAX_AUDIT_BYTES` without recording `audit_truncated` until another event arrived. The writer now checks the encoded record length before - appending and emits the marker immediately on overflow. Python 3.13 Harbor - tests (91 pass, 13 skips) and 21 Eval audit artifact tests passed. This is a - scoped fix, not an independent replacement of all five original commits; - the first Grok-tagged commit predates the selected policy date. + appending and emits the marker immediately on overflow. Another ablation + showed permissive UTF-8 decoding counted a corrupt JSON string as a valid + `policy_error`; the artifact reader now decodes each raw line strictly, + preserving the existing score and missing-log rules. Python 3.13 Harbor + tests (93 total, 13 skips) and 22 Eval audit artifact tests passed. These + are scoped fixes, not an independent replacement of all five original + commits; the first Grok-tagged commit predates the selected policy date. - #3008: a new test exposed that CONNECT classification trusted `pretty_host` over the actual tunnel destination, allowing a spoofed Host header to hide a blocklisted target. It now classifies `request.host`; missing targets fail diff --git a/packages/eval/src/__tests__/egress-audit.test.ts b/packages/eval/src/__tests__/egress-audit.test.ts index 4d011a5ec7d..c62f550fb4a 100644 --- a/packages/eval/src/__tests__/egress-audit.test.ts +++ b/packages/eval/src/__tests__/egress-audit.test.ts @@ -122,6 +122,15 @@ test('invalid UTF-8 is counted as a malformed line, not judged', () => { assert.deepEqual(collectEgressAuditArtifact(audit, true), inventory(audit, false, 0, 1)); }); +test('invalid UTF-8 inside a valid JSON string is still a malformed audit line', () => { + const audit = Buffer.concat([ + Buffer.from('{"ruleId":"policy_error","host":"'), + Buffer.from([0xff]), + Buffer.from('"}\n{"ruleId":"audit_truncated"}\n'), + ]); + assert.deepEqual(collectEgressAuditArtifact(audit, true), inventory(audit, true, 0, 1)); +}); + test('trials without an egress proxy do not invent a missing-audit artifact', () => { assert.deepEqual(collectEgressAuditArtifact(undefined, false), { missing: false, diff --git a/packages/eval/src/harness-executor.ts b/packages/eval/src/harness-executor.ts index e5e19fcd9e3..1fbe09edf9a 100644 --- a/packages/eval/src/harness-executor.ts +++ b/packages/eval/src/harness-executor.ts @@ -763,12 +763,18 @@ function inspectEgressAudit(audit: Buffer): { let truncated = false; let policyErrorCount = 0; let malformedLineCount = 0; - for (const line of audit.toString('utf8').split(/\r?\n/)) { - const trimmed = line.trim(); - if (!trimmed) continue; + const decoder = new TextDecoder('utf-8', { fatal: true }); + for (let start = 0; start < audit.length; ) { + const newline = audit.indexOf(0x0a, start); + const end = newline === -1 ? audit.length : newline; + const line = audit.subarray(start, end); + start = newline === -1 ? audit.length : newline + 1; + if (line.length === 0) continue; let record: unknown; try { - record = JSON.parse(trimmed); + const text = decoder.decode(line).trim(); + if (!text) continue; + record = JSON.parse(text); } catch { malformedLineCount += 1; continue; From 66e1f42d09b3a0b678bd2a5da1ad75996550f433 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 15:00:37 +0800 Subject: [PATCH 031/110] fix(eval): validate CONNECT authority before egress classification --- docs/grok-assisted-contribution-review.md | 11 +++++++---- packages/eval/harbor/egress_filter.py | 14 ++++++++++--- packages/eval/harbor/test_egress_filter.py | 23 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 5460882b23d..92278cd1a9f 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -354,10 +354,13 @@ and the Electron renderer-reload E2E passes in this branch. - #3008: a new test exposed that CONNECT classification trusted `pretty_host` over the actual tunnel destination, allowing a spoofed Host header to hide a blocklisted target. It now classifies `request.host`; missing targets fail - closed. Python 3.13 Eval tests (90 pass, 13 skips) passed. A live mitmproxy - regression was added but not run locally because the Docker daemon did not - respond. #3017 owns the later raw-TCP closure behavior; do not roll that - implementation back as part of #3008. + closed. A further ablation showed malformed CONNECT authorities (userinfo + and path delimiters) and out-of-range ports were still accepted as benign + destinations. Host/port parsing now rejects those inputs while retaining + valid IPv6; Python 3.13 Harbor tests (94 total, 13 skips) passed. A live + mitmproxy regression was added but not run locally because the Docker daemon + did not respond. #3017 owns the later raw-TCP closure behavior; do not roll + that implementation back as part of #3008. ### Remaining 21: current disposition diff --git a/packages/eval/harbor/egress_filter.py b/packages/eval/harbor/egress_filter.py index baaa7be2f6b..37c5be8b859 100644 --- a/packages/eval/harbor/egress_filter.py +++ b/packages/eval/harbor/egress_filter.py @@ -23,6 +23,7 @@ import os import re import time +from ipaddress import IPv6Address from pathlib import Path from urllib.parse import unquote, urlsplit @@ -244,12 +245,19 @@ def apply_http_policy(flow: object, raw_url: str) -> None: def connect_target_url(flow: object) -> str: request = flow.request - host = (getattr(request, "host", "") or "").strip() - if not host: + host = getattr(request, "host", None) + if not isinstance(host, str) or not host or host != host.strip(): raise ValueError("empty CONNECT host") - if ":" in host and not host.startswith("["): + if host.startswith("[") and host.endswith("]"): + IPv6Address(host[1:-1]) + elif ":" in host: + IPv6Address(host) host = f"[{host}]" + elif re.search(r"[\s/@?#\\\[\]]", host): + raise ValueError("invalid CONNECT host") port = getattr(request, "port", None) + if port is not None and (type(port) is not int or not 1 <= port <= 65535): + raise ValueError("invalid CONNECT port") if port in (None, 443): return f"https://{host}/" if port == 80: diff --git a/packages/eval/harbor/test_egress_filter.py b/packages/eval/harbor/test_egress_filter.py index 4e1e857855f..e32bd3726c8 100644 --- a/packages/eval/harbor/test_egress_filter.py +++ b/packages/eval/harbor/test_egress_filter.py @@ -204,6 +204,29 @@ def make(status, body, headers): MODULE.http_connect(flow) self.assertEqual(flow.response["status"], 503) + def test_connect_rejects_malformed_authorities_and_ports(self) -> None: + self._install_response_stub() + with tempfile.TemporaryDirectory() as directory: + MODULE.AUDIT_PATH = Path(directory) / "hits.jsonl" + for host, port in ( + ("tbench.ai@safe.example", 443), + ("safe.example/path", 443), + ("safe.example", 0), + ("safe.example", 65536), + ): + with self.subTest(host=host, port=port): + flow = SimpleNamespace( + request=SimpleNamespace(host=host, port=port), response=None + ) + MODULE.http_connect(flow) + self.assertEqual(flow.response["status"], 503) + + valid = SimpleNamespace( + request=SimpleNamespace(host="2001:db8::1", port=8443), response=None + ) + MODULE.http_connect(valid) + self.assertIsNone(valid.response) + def test_tcp_start_kills_raw_tunnels_and_records_them(self) -> None: with tempfile.TemporaryDirectory() as directory: MODULE.AUDIT_PATH = Path(directory) / "hits.jsonl" From 7dc72ebe507b5572031a2b37d244db4e8ee42580 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 15:06:22 +0800 Subject: [PATCH 032/110] fix(runtime): fall back to valid retry-after header --- docs/grok-assisted-contribution-review.md | 12 ++++++++++- .../provider-error-classification.test.ts | 5 +++++ .../src/provider-error-classification.ts | 20 ++++++++++--------- scripts/ci-test-plan.test.mjs | 6 ++++++ 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 92278cd1a9f..7c30655b2da 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -340,7 +340,17 @@ and the Electron renderer-reload E2E passes in this branch. for exact bytes, independently stale Markdown, and missing/extra paths. The CI planner also recognizes the new test. The real 301-file inventory check (after merging upstream), 22 Astryx tests, 40 CI planner tests, and Biome - passed. The generator and its later fail-closed dependency parser remain unchanged. + passed. A further planner assertion confirms that edits to the generator's + test file select the general code lane, which always runs the Astryx gate; + no CI rule change was needed. The generator and its later fail-closed + dependency parser remain unchanged. +- #3115: another ablation found that an invalid `Retry-After-Ms` suppressed + an otherwise valid `Retry-After` on the same retryable response. The parser + now validates each candidate independently, preferring valid milliseconds + and falling back to valid seconds or an HTTP date. The new regression failed + before the change and passed afterward (20 classification tests); Runtime + build and Biome passed. This does not change retryability or constitute a + full #3115 replacement. - #2967: a new boundary test showed the audit writer could append a record across `MAX_AUDIT_BYTES` without recording `audit_truncated` until another event arrived. The writer now checks the encoded record length before diff --git a/packages/runtime/src/__tests__/provider-error-classification.test.ts b/packages/runtime/src/__tests__/provider-error-classification.test.ts index b09e318aa76..855ed2e98fa 100644 --- a/packages/runtime/src/__tests__/provider-error-classification.test.ts +++ b/packages/runtime/src/__tests__/provider-error-classification.test.ts @@ -363,6 +363,11 @@ describe('Provider error classification', () => { const headerCases: Array<[string, Record | undefined, number | undefined]> = [ ['seconds', { 'retry-after': '40' }, 40_000], ['milliseconds', { 'retry-after-ms': '1500' }, 1_500], + [ + 'invalid milliseconds with valid seconds', + { 'retry-after-ms': 'invalid', 'retry-after': '4' }, + 4_000, + ], ['malformed', { 'retry-after': 'not-a-delay' }, undefined], ['elapsed HTTP-date', { 'retry-after': 'Wed, 21 Oct 2015 07:28:00 GMT' }, undefined], ['absent', undefined, undefined], diff --git a/packages/runtime/src/provider-error-classification.ts b/packages/runtime/src/provider-error-classification.ts index b66c7ef7f7d..98691c98397 100644 --- a/packages/runtime/src/provider-error-classification.ts +++ b/packages/runtime/src/provider-error-classification.ts @@ -261,17 +261,19 @@ function parseRetryAfterMs(headers: Record): number | undefined const rawRetryAfter = headers['retry-after']; if (rawMilliseconds === undefined && rawRetryAfter === undefined) return undefined; - let delayMs: number; + const boundedDelay = (delayMs: number): number | undefined => + Number.isFinite(delayMs) && delayMs > 0 && delayMs <= MAX_SAFE_TIMER_DELAY_MS + ? Math.ceil(delayMs) + : undefined; if (rawMilliseconds !== undefined) { - delayMs = Number(rawMilliseconds); - } else { - const seconds = Number(rawRetryAfter); - delayMs = Number.isFinite(seconds) ? seconds * 1_000 : Date.parse(rawRetryAfter!) - Date.now(); + const milliseconds = boundedDelay(Number(rawMilliseconds)); + if (milliseconds !== undefined) return milliseconds; } - if (!Number.isFinite(delayMs) || delayMs <= 0 || delayMs > MAX_SAFE_TIMER_DELAY_MS) { - return undefined; - } - return Math.ceil(delayMs); + if (rawRetryAfter === undefined) return undefined; + const seconds = Number(rawRetryAfter); + return boundedDelay( + Number.isFinite(seconds) ? seconds * 1_000 : Date.parse(rawRetryAfter) - Date.now(), + ); } function retryMetadataFromFacts( diff --git a/scripts/ci-test-plan.test.mjs b/scripts/ci-test-plan.test.mjs index 350667a0507..e5e3a549d0c 100644 --- a/scripts/ci-test-plan.test.mjs +++ b/scripts/ci-test-plan.test.mjs @@ -174,6 +174,12 @@ test('the Astryx inventory can run without selecting the code suite', () => { planTests(['scripts/check-astryx-surface-inventory.test.mjs'], { graph }).astryxSurface, true, ); + // Generator tests use the normal code lane, which always invokes both + // inventory scripts even though this path is not an Astryx product surface. + assert.equal( + planTests(['scripts/generate-astryx-surface-inventory.test.mjs'], { graph }).code, + true, + ); }); test('desktop renderer changes retain Electron and Storybook coverage', () => { From ffaffb568d65726868a22cedb6c751c08b9281d1 Mon Sep 17 00:00:00 2001 From: likun Date: Sun, 27 Sep 2026 15:08:00 +0800 Subject: [PATCH 033/110] fix(ui): require queue revision before dragging entries --- docs/grok-assisted-contribution-review.md | 6 ++++++ .../src/__tests__/composer-message-queue.test.tsx | 13 +++++++++++++ packages/ui/src/composer-message-queue.tsx | 9 ++++++--- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 7c30655b2da..1f630caa795 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -351,6 +351,12 @@ and the Electron renderer-reload E2E passes in this branch. before the change and passed afterward (20 classification tests); Runtime build and Biome passed. This does not change retryability or constitute a full #3115 replacement. +- #3544: a UI ablation showed Host-admitted rows remained draggable when the + queue revision was unavailable, even though editing was disabled. The queue + component now gates both the drag affordance and drop submission on a known + revision. The new test failed before the fix and passed afterward (5 queue + component tests); UI build and Biome passed. This is not a complete rewrite + of the 35-file mixed-author feature. - #2967: a new boundary test showed the audit writer could append a record across `MAX_AUDIT_BYTES` without recording `audit_truncated` until another event arrived. The writer now checks the encoded record length before diff --git a/packages/ui/src/__tests__/composer-message-queue.test.tsx b/packages/ui/src/__tests__/composer-message-queue.test.tsx index c09679306a1..bec1fc8e513 100644 --- a/packages/ui/src/__tests__/composer-message-queue.test.tsx +++ b/packages/ui/src/__tests__/composer-message-queue.test.tsx @@ -172,3 +172,16 @@ test('dragging reorders the Host-owned id list', async () => { await view.close(); } }); + +test('queued entries cannot start a reorder without a Host queue revision', async () => { + const view = await mountQueue({ + queuedMessages: [queued('entry-1', 'first'), queued('entry-2', 'second')], + onReorderEntries: () => assert.fail('reorder must remain unavailable'), + }); + try { + assert.equal(view.document.querySelectorAll('[draggable="true"]').length, 0); + assert.equal(view.document.querySelectorAll('[data-maka-queue-drop-target="true"]').length, 0); + } finally { + await view.close(); + } +}); diff --git a/packages/ui/src/composer-message-queue.tsx b/packages/ui/src/composer-message-queue.tsx index 574191c06c2..fe1afdec3c5 100644 --- a/packages/ui/src/composer-message-queue.tsx +++ b/packages/ui/src/composer-message-queue.tsx @@ -95,9 +95,11 @@ export const ComposerMessageQueue = memo(function ComposerMessageQueue( function dropOn(targetEntryId: string) { const fromId = dragEntryId.current; dragEntryId.current = null; - if (!fromId || fromId === targetEntryId || !props.onReorderEntries) return; - if (!entries.some((entry) => entry.entryId === targetEntryId)) return; - const ids = entries.filter((entry) => entry.state === 'queued').map((entry) => entry.entryId); + if (!fromId || fromId === targetEntryId || !props.onReorderEntries || props.queueRevision === undefined) return; + const target = entries.find((entry) => entry.entryId === targetEntryId); + const source = entries.find((entry) => entry.entryId === fromId); + if (!target || source?.placement !== target.placement) return; + const ids = entries.filter((entry) => entry.placement === target.placement && entry.state === 'queued').map((entry) => entry.entryId); const from = ids.indexOf(fromId); const to = ids.indexOf(targetEntryId); if (from === -1 || to === -1) return; @@ -121,6 +123,7 @@ export const ComposerMessageQueue = memo(function ComposerMessageQueue( entry.state === 'queued' && !local && Boolean(props.onReorderEntries) + && props.queueRevision !== undefined && pendingEntryId === null; return (
Date: Sun, 27 Sep 2026 15:09:49 +0800 Subject: [PATCH 034/110] fix(ui): discard reorder drag after queue revision advances --- docs/grok-assisted-contribution-review.md | 5 +++- .../__tests__/composer-message-queue.test.tsx | 23 +++++++++++++++++++ packages/ui/src/composer-message-queue.tsx | 21 +++++++++++------ 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/docs/grok-assisted-contribution-review.md b/docs/grok-assisted-contribution-review.md index 1f630caa795..5f1c1c9041c 100644 --- a/docs/grok-assisted-contribution-review.md +++ b/docs/grok-assisted-contribution-review.md @@ -354,7 +354,10 @@ and the Electron renderer-reload E2E passes in this branch. - #3544: a UI ablation showed Host-admitted rows remained draggable when the queue revision was unavailable, even though editing was disabled. The queue component now gates both the drag affordance and drop submission on a known - revision. The new test failed before the fix and passed afterward (5 queue + revision. Another failing ablation showed an old drag could reorder a newer + projection after the Host revision changed mid-drag; the drag now retains + its starting revision and discards the drop when it differs. Both tests + failed before their respective fixes and passed afterward (6 queue component tests); UI build and Biome passed. This is not a complete rewrite of the 35-file mixed-author feature. - #2967: a new boundary test showed the audit writer could append a record diff --git a/packages/ui/src/__tests__/composer-message-queue.test.tsx b/packages/ui/src/__tests__/composer-message-queue.test.tsx index bec1fc8e513..7d4ac85cec0 100644 --- a/packages/ui/src/__tests__/composer-message-queue.test.tsx +++ b/packages/ui/src/__tests__/composer-message-queue.test.tsx @@ -185,3 +185,26 @@ test('queued entries cannot start a reorder without a Host queue revision', asyn await view.close(); } }); + +test('a drag from an older queue revision cannot reorder a newer projection', async () => { + const reordered: string[][] = []; + const entries = [queued('entry-1', 'first'), queued('entry-2', 'second')]; + const onReorderEntries = (ids: readonly string[]) => { reordered.push([...ids]); }; + const view = await mountQueue({ queuedMessages: entries, queueRevision: 1, onReorderEntries }); + try { + const source = view.document.querySelectorAll('[draggable="true"]')[1]!; + await act(async () => { + source.dispatchEvent(Object.assign(new window.Event('dragstart', { bubbles: true }), { + dataTransfer: { effectAllowed: '', setData: () => {} }, + })); + }); + await view.rerender({ queuedMessages: entries, queueRevision: 2, onReorderEntries }); + await act(async () => { + view.document.querySelectorAll('[data-maka-queue-drop-target="true"]')[0]! + .dispatchEvent(new window.Event('drop', { bubbles: true })); + }); + assert.deepEqual(reordered, []); + } finally { + await view.close(); + } +}); diff --git a/packages/ui/src/composer-message-queue.tsx b/packages/ui/src/composer-message-queue.tsx index fe1afdec3c5..b7c743281b8 100644 --- a/packages/ui/src/composer-message-queue.tsx +++ b/packages/ui/src/composer-message-queue.tsx @@ -69,7 +69,7 @@ export const ComposerMessageQueue = memo(function ComposerMessageQueue( props: ComposerMessageQueueProps, ) { const [pendingEntryId, setPendingEntryId] = useState(null); - const dragEntryId = useRef(null); + const dragSource = useRef<{ entryId: string; queueRevision: number } | null>(null); const mountedRef = useMountedRef(); const copy = props.copy; @@ -93,9 +93,15 @@ export const ComposerMessageQueue = memo(function ComposerMessageQueue( } function dropOn(targetEntryId: string) { - const fromId = dragEntryId.current; - dragEntryId.current = null; - if (!fromId || fromId === targetEntryId || !props.onReorderEntries || props.queueRevision === undefined) return; + const sourceDrag = dragSource.current; + dragSource.current = null; + if ( + !sourceDrag + || sourceDrag.entryId === targetEntryId + || !props.onReorderEntries + || props.queueRevision !== sourceDrag.queueRevision + ) return; + const fromId = sourceDrag.entryId; const target = entries.find((entry) => entry.entryId === targetEntryId); const source = entries.find((entry) => entry.entryId === fromId); if (!target || source?.placement !== target.placement) return; @@ -130,7 +136,7 @@ export const ComposerMessageQueue = memo(function ComposerMessageQueue( key={entry.entryId} data-maka-queue-drop-target={reorderable ? 'true' : undefined} onDragOver={(event) => { - if (reorderable && dragEntryId.current) event.preventDefault(); + if (reorderable && dragSource.current) event.preventDefault(); }} onDrop={reorderable ? () => dropOn(entry.entryId) : undefined} > @@ -147,13 +153,14 @@ export const ComposerMessageQueue = memo(function ComposerMessageQueue( draggable={reorderable} aria-label={copy.reorderQueuedEntry} onDragStart={(event) => { - dragEntryId.current = entry.entryId; + if (!reorderable || props.queueRevision === undefined) return; + dragSource.current = { entryId: entry.entryId, queueRevision: props.queueRevision }; event.dataTransfer.effectAllowed = 'move'; event.dataTransfer.setData('text/plain', entry.entryId); event.dataTransfer.setData('application/x-maka-queue-entry', entry.entryId); }} onDragEnd={() => { - dragEntryId.current = null; + dragSource.current = null; }} >
@@ -2606,7 +2596,6 @@ function AppShellContent({ onSelectedRuntimeHostProfileIdChange={setSettingsProfileId} /> - diff --git a/apps/desktop/src/renderer/styles/composer.css b/apps/desktop/src/renderer/styles/composer.css index 6549f9172f4..6c78eb8a435 100644 --- a/apps/desktop/src/renderer/styles/composer.css +++ b/apps/desktop/src/renderer/styles/composer.css @@ -18,15 +18,6 @@ */ /* Composer — outer container, controls, pickers, tool buttons, send/workspace/branch/streaming. Composer block merged in from tool-output.css (was misplaced there) — issue #546 PR3. Runtime interaction prompt styles live in interaction-prompts.css. */ -.maka-composer[data-drag-active="true"] .maka-composer-astryx { - border-color: oklch(from var(--focus-ring) l c h / 0.46); - background: oklch(from var(--focus-ring) 0.98 calc(c * 0.20) h / 0.35); - box-shadow: - 0 0 0 1px oklch(from var(--focus-ring) l c h / 0.22), - 0 0 0 4px oklch(from var(--focus-ring) l c h / 0.08), - 0 1px 3px oklch(from var(--foreground) l c h / 0.04); -} - .composer { flex: 0 0 auto; display: flex; @@ -86,33 +77,33 @@ } .maka-composer-queue { - width: 100%; - min-width: 0; - padding-bottom: var(--space-1); -} - -.maka-composer-queue-list li { - padding-right: 0; -} - -.maka-composer-drawer-divider { - width: calc(100% - (2 * var(--space-3))); - border-top: var(--border-width-hairline) solid var(--border); - margin: var(--space-0-5) var(--space-3); + box-sizing: border-box; + width: min(var(--maka-reading-measure), calc(100% - 2 * var(--space-6))); + max-width: var(--maka-reading-measure); + margin-inline: auto; + margin-block-end: var(--space-1); + padding-block: var(--space-1); + padding-inline: var(--space-1); + border-radius: var(--radius-chat, 8px); + background: var(--background-elevated); + box-shadow: inset 0 0 0 var(--border-width-hairline) var(--border); + --_button-radius: max( + var(--radius-element), + calc(var(--radius-chat) - var(--spacing-3)) + ); } .maka-composer-queue-list { - width: 100%; min-width: 0; max-height: 132px; - overflow-y: auto; + overflow: hidden auto; } .maka-composer-queue-text { display: block; min-width: 0; - overflow: hidden; white-space: nowrap; + overflow: hidden; text-overflow: ellipsis; } @@ -123,18 +114,32 @@ flex: 0 0 auto; } +.maka-composer-queue-edit { + width: 100%; + min-width: 0; + border: 0; + border-block-end: var(--border-width-hairline) solid var(--border-strong); + outline: 0; + background: transparent; + color: inherit; + font: inherit; + line-height: inherit; + resize: vertical; +} + .maka-composer-queue-grip { display: grid; - place-items: center; - width: 16px; + align-items: center; + justify-items: center; + inline-size: 16px; + flex: 0 0 auto; color: var(--muted-foreground); cursor: grab; opacity: 0; - transition: opacity 120ms ease; + transition: opacity 120ms ease-out; } -.maka-composer-queue-list li:hover .maka-composer-queue-grip, -.maka-composer-queue-grip:focus-visible { +.maka-composer-queue-list li:is(:hover, :focus-within) .maka-composer-queue-grip { opacity: 1; } @@ -143,6 +148,10 @@ } @media (max-width: 640px) { + .maka-composer-queue { + width: min(var(--maka-reading-measure), calc(100% - 2 * var(--space-3))); + } + .maka-composer-queue-actions { gap: 0; } @@ -684,6 +693,22 @@ gap: var(--space-3); } +.maka-composer-queue-status { + display: flex; + align-items: center; + justify-content: space-between; + gap: var(--space-2); + padding: var(--space-1) var(--space-2); + color: var(--muted-foreground); + font-size: 11px; + white-space: nowrap; +} + +.maka-composer-queue-delivery { + font-size: 11px; + color: var(--muted-foreground); +} + /* Share a grid cell so a wrapping suggestion reserves exactly the same height as the accepted draft, without measuring layout or mutating the editor. */ .maka-composer-input-suggestion-wrap { display: grid; min-width: 0; } diff --git a/apps/desktop/stories/composer-message-queue.stories.tsx b/apps/desktop/stories/composer-message-queue.stories.tsx index f673536bb4c..515ef24234e 100644 --- a/apps/desktop/stories/composer-message-queue.stories.tsx +++ b/apps/desktop/stories/composer-message-queue.stories.tsx @@ -17,309 +17,118 @@ * under the License. */ -import { useCallback, useMemo, useRef, useState } from 'react'; -import type { Meta, StoryObj } from '@storybook/react-vite'; -import { expect, userEvent, waitFor, within } from 'storybook/test'; -import type { ComponentProps, ReactElement, ReactNode } from 'react'; -import type { MessageQueueEntryProjection } from '@maka/core/events'; -import type { SessionSummary } from '@maka/core/session'; -import { Composer } from '@maka/ui'; -import type { ChatModelChoice, ComposerHandle, TransientUserMessageProjection } from '@maka/ui'; -import { - ConversationServicesProvider, - SessionLocalMessages, - type ConversationServices, -} from '../src/renderer/features/conversation'; -import { stubConversationServices } from '../src/renderer/features/conversation/testing'; -import type { DesktopLocalMessage } from '../src/shared/session-local-contract.js'; - -const NOW = Date.UTC(2026, 6, 1, 9, 30, 0); -const SESSION_ID = 's'; - -type DeliveryState = 'queued' | 'unconfirmed' | 'failed'; - -function localDeliveryMessages(state: DeliveryState): DesktopLocalMessage[] { - const base = { - sessionId: SESSION_ID, - attachments: [], - inlineReferences: [], - placement: 'next_turn' as const, - }; - if (state === 'unconfirmed') { - return [ - { - ...base, - messageId: 'local-unconfirmed', - createdAt: NOW - 60_000, - state: 'unknown', - canCancel: false, - text: 'Check whether the queue drained before retrying the compaction audit.', - error: 'Connection dropped before the receipt arrived', - }, - { - ...base, - messageId: 'local-waiting', - createdAt: NOW - 30_000, - state: 'saved', - canCancel: true, - text: 'Summarize the retry budget for the pending follow-ups.', - }, - ]; - } - if (state === 'failed') { - return [ - { - ...base, - messageId: 'local-failed', - createdAt: NOW - 60_000, - state: 'failed', - canCancel: true, - text: 'Attach the runtime.sqlite compaction log to the report.', - error: 'Message preparation failed. The local copy is retained.', - }, - ]; - } - return []; -} - -function localDeliveryServices(state: DeliveryState): ConversationServices { - let messages = localDeliveryMessages(state); - return stubConversationServices({ - listMessages: async () => messages, - cancelMessage: async (_sessionId, messageId) => { - messages = messages.filter((message) => message.messageId !== messageId); - }, - }); -} - -const meta = { - title: 'Product/Composer Message Queue', - component: QueuedComposer, - parameters: { layout: 'fullscreen' }, -} satisfies Meta; - -export default meta; - -type Story = StoryObj; -type ComposerProps = ComponentProps; - -function noop() { - return undefined; -} - -function session(): SessionSummary { - return { - id: 's', - name: '排查 Context summary failed', - isFlagged: false, - isArchived: false, - labels: [], - hasUnread: false, - lastMessageAt: NOW, - lastMessagePreview: '查一下 PR #3526 相关的 session。', - status: 'running', - backend: 'ai-sdk', - llmConnectionId: 'connection-anthropic-main', - llmConnectionSlug: 'anthropic-main', - connectionLocked: false, - model: 'claude-sonnet-4-5', - permissionMode: 'ask', - }; -} - -const modelChoices: ChatModelChoice[] = [ - { - connectionId: 'connection-anthropic-main', - connectionSlug: 'anthropic-main', - providerType: 'anthropic', - providerLabel: 'Anthropic', - model: 'claude-sonnet-4-5', - label: 'Claude Sonnet 4.5', - isDefault: true, - thinkingLevels: [], - }, +import { useReducer } from "react"; +import type { Meta, StoryObj } from "@storybook/react-vite"; +import type { MessageQueueEntryProjection } from "@maka/core/events"; +import type { SessionSummary } from "@maka/core/session"; +import { Composer, type ChatModelChoice } from "@maka/ui"; + +const SESSION: SessionSummary = { + id: "s", + name: "排查 Context summary failed", + isFlagged: false, + isArchived: false, + labels: [], + hasUnread: false, + lastMessageAt: Date.UTC(2026, 6, 1, 9, 30, 0), + lastMessagePreview: "查一下 PR #3526 相关的 session。", + status: "running", + backend: "ai-sdk", + llmConnectionId: "connection-anthropic-main", + llmConnectionSlug: "anthropic-main", + connectionLocked: false, + model: "claude-sonnet-4-5", + permissionMode: "ask", +}; +const MODELS: ChatModelChoice[] = [{ + connectionId: "connection-anthropic-main", + connectionSlug: "anthropic-main", + providerType: "anthropic", + providerLabel: "Anthropic", + model: "claude-sonnet-4-5", + label: "Claude Sonnet 4.5", + isDefault: true, + thinkingLevels: [], +}]; +const INITIAL_QUEUE: MessageQueueEntryProjection[] = [ + queueEntry("entry-steer", "先把刚才的判断改成只检查当前工作树。", "current_turn"), + queueEntry("entry-1", "先不要改协议。"), + queueEntry("entry-2", "查一下 PR #3526 相关的 session 及其 compaction 诊断记录。"), + queueEntry("entry-3", "把 runtime.sqlite 里的 compaction 日志也带上。"), ]; -function followUpEntry(entryId: string, text: string): MessageQueueEntryProjection { +type QueueAction = + | { type: "remove"; entryId: string } + | { type: "rename"; entryId: string; text: string } + | { type: "reorder"; entryIds: readonly string[] }; + +function queueEntry( + entryId: string, + text: string, + placement: MessageQueueEntryProjection["placement"] = "next_turn", +): MessageQueueEntryProjection { return { entryId, messageId: `message-${entryId}`, content: { text }, - placement: 'next_turn', - state: 'queued', + placement, + state: "queued", }; } -/** - * Local stand-in for the Runtime Host queue projection: promote hands an - * entry to the active Turn (it leaves the plate), edit retracts it back into - * the draft, retract drops it, and reorder applies the drag order. The component contract - * (projection in, mutations - * out) is the real one; only the authority is simulated. - */ -// A production queue snapshot also carries queued steering; the drawer filters -// it out — steering renders in the transcript instead (see the -// QueuedMessageLifecycleFlow story in app-shell). -const DEFAULT_QUEUE: MessageQueueEntryProjection[] = [ - { - entryId: 'entry-steer', - messageId: 'message-steer', - content: { text: '先把刚才的判断改成只检查当前工作树。' }, - placement: 'current_turn', - state: 'queued', - }, - followUpEntry('entry-1', '先不要改协议。'), - followUpEntry('entry-2', '查一下 PR #3526 相关的 session 及其 compaction 诊断记录。'), - followUpEntry('entry-3', '把 runtime.sqlite 里的 compaction 日志也带上。'), -]; - -function QueuedComposer({ - deliveryState, - stagedContext = true, - entries = DEFAULT_QUEUE, -}: { - deliveryState: DeliveryState; - stagedContext?: boolean; - entries?: MessageQueueEntryProjection[]; -}) { - const composerRef = useRef(null); - const [followup, setFollowup] = useState(entries); - - const base: ComposerProps = { - draftKey: 'storybook-composer-queue', - onSend: noop, - onStop: noop, - modelLabel: 'K3-256k', - activeSession: session(), - activeModel: 'claude-sonnet-4-5', - activeModelLabel: 'K3-256k', - modelChoices, - permissionMode: 'ask', - onPermissionModeChange: noop, - onPickAttachments: noop, - streaming: true, - // Real mid-turn state can stage context while follow-ups wait: the two - // sections stack inside the one staging drawer. - pendingQuotes: stagedContext - ? [{ text: 'queue.entries 表在 Host 端是唯一权威', label: '设计笔记' }] - : undefined, - pendingAttachments: stagedContext - ? [{ kind: 'other', displayName: 'compaction-audit.md', mimeType: 'text/markdown', size: 12_400 }] - : undefined, - }; - - const [published, setPublished] = useState(new Map()); - const publish = useCallback((_sessionId: string, message: TransientUserMessageProjection) => { - setPublished((current) => new Map(current).set(message.id, message)); - }, []); - const retire = useCallback((_sessionId: string, messageId: string) => { - setPublished((current) => { - const next = new Map(current); - next.delete(messageId); - return next; - }); - }, []); - const services = useMemo(() => localDeliveryServices(deliveryState), [deliveryState]); +function reduceQueue( + queue: readonly MessageQueueEntryProjection[], + action: QueueAction, +): MessageQueueEntryProjection[] { + if (action.type === "remove") { + return queue.filter((entry) => entry.entryId !== action.entryId); + } + if (action.type === "rename") { + return queue.map((entry) => entry.entryId === action.entryId + ? { ...entry, content: { ...entry.content, text: action.text, displayText: action.text } } + : entry); + } + const rank = new Map(action.entryIds.map((entryId, index) => [entryId, index])); + return [...queue].sort((left, right) => + (rank.get(left.entryId) ?? -1) - (rank.get(right.entryId) ?? -1), + ); +} +function PendingQueueExample() { + const [queue, dispatch] = useReducer(reduceQueue, INITIAL_QUEUE); return ( - - { - composerRef.current?.setText(draft.text); - composerRef.current?.focus(); - }} - /> +
{ - setFollowup((current) => current.filter((candidate) => candidate.entryId !== entryId)); - }} - onEditQueuedEntry={(entry) => { - setFollowup((current) => current.filter((candidate) => candidate.entryId !== entry.entryId)); - composerRef.current?.setText(entry.content.displayText ?? entry.content.text); - }} - onDeleteQueuedEntry={(entryId) => { - setFollowup((current) => current.filter((candidate) => candidate.entryId !== entryId)); - }} - onReorderQueuedEntries={(entryIds) => { - setFollowup((current) => - entryIds.flatMap((entryId) => - current.filter((candidate) => candidate.entryId === entryId), - ), - ); - }} + draftKey="storybook-composer-queue" + onSend={() => undefined} + onStop={() => undefined} + modelLabel="K3-256k" + activeSession={SESSION} + activeModel="claude-sonnet-4-5" + activeModelLabel="K3-256k" + modelChoices={MODELS} + permissionMode="ask" + onPermissionModeChange={() => undefined} + onPickAttachments={() => undefined} + streaming + queuedMessages={queue} + queuedMessageRevision={1} + onPromoteQueuedEntry={(entryId) => dispatch({ type: "remove", entryId })} + onUpdateQueuedEntry={(entryId, _revision, text) => + dispatch({ type: "rename", entryId, text })} + onDeleteQueuedEntry={(entryId) => dispatch({ type: "remove", entryId })} + onReorderQueuedEntries={(entryIds) => dispatch({ type: "reorder", entryIds })} /> - +
); } -function storyFrame(children: ReactNode): ReactElement { - return
{children}
; -} - -// Real path: mid-turn sends while a quote and a file are staged — the staging -// drawer holds the queue section above a hairline and the context chips below. -// Drag follow-ups to reorder; 直接发送 promotes one, 编辑 takes it back into the draft. -export const PendingPlate: Story = { - args: { deliveryState: 'queued' }, - argTypes: { - deliveryState: { - options: ['queued', 'unconfirmed', 'failed'], - control: { type: 'radio' }, - }, - }, - render: (args) => - storyFrame( - , - ), -}; +const meta = { + title: "Product/Composer Message Queue", + parameters: { layout: "fullscreen" }, +} satisfies Meta; -// Real path: the user folds the staging slab; every staged item collapses into -// the count strip until it is reopened. -export const StagingCollapsed: Story = { - args: { deliveryState: 'queued' }, - render: () => storyFrame(), - play: async ({ canvasElement }) => { - const canvas = within(canvasElement); - await userEvent.click(canvas.getByRole('button', { name: /收起|collapse/i })); - await waitFor(() => { - // Collapsed content stays mounted under the grid-row collapse — assert on - // the disclosure state, not DOM removal. - const toggle = canvas.getByRole('button', { name: /展开|expand/i }); - expect(toggle).toHaveAttribute('aria-expanded', 'false'); - expect(toggle).toHaveTextContent(/5|附加内容|staged/i); - }); - }, -}; +export default meta; +type Story = StoryObj; -// Real path: a long Turn while the user keeps queueing — the list scrolls -// inside the drawer instead of growing the composer. -export const OverflowingQueue: Story = { - args: { deliveryState: 'queued' }, - render: () => - storyFrame( - - followUpEntry(`entry-${index + 1}`, `排队跟进 ${index + 1}：检查 compaction 诊断记录的第 ${index + 1} 段。`), - )} - />, - ), - play: async ({ canvasElement }) => { - const canvas = within(canvasElement); - const list = canvasElement.querySelector('.maka-composer-queue-list'); - await waitFor(() => { - expect(list).not.toBeNull(); - expect(list!.scrollHeight).toBeGreaterThan(list!.clientHeight); - }); - await expect(canvas.getAllByText(/^排队跟进/).length).toBe(9); - }, -}; +export const PendingPlate: Story = { render: () => }; diff --git a/docs/desktop-message-queue.md b/docs/desktop-message-queue.md index 58210dd3261..c5b35a61fdf 100644 --- a/docs/desktop-message-queue.md +++ b/docs/desktop-message-queue.md @@ -21,33 +21,28 @@ ## Problem -Desktop used a rendered `streaming` prop to decide whether a composer submit started a root turn or steered the active turn. Immediately after the first send completed its IPC round trip, the Runtime Host could already own an active turn while React had not rendered the new `streaming` value yet. A second submit in that interval entered the root-turn path and could fail with `session_busy`. Multiple burst submissions also produced duplicate error toasts. +The renderer once selected root-turn versus active-turn submission from the last rendered `streaming` value. Runtime Host ownership can advance before React commits that projection, so a second message could take the stale root-turn path and fail with `session_busy`. Burst sends could also fan out the same failure through multiple toasts. Runtime Host already owns the durable message semantics: - `current_turn` queues steering for the next provider boundary. - `next_turn` queues one successor turn per accepted message. - queue projections are authoritative. -- queue projections carry the canonical queued message content; mutation results return only queue state. +- projections contain canonical queue entries; commands acknowledge only the committed queue revision. ## Desktop Behavior -- While a turn is active, a composer submit queues a follow-up. There is no mode switch: Send is always Send. +- Ordinary submission during an active turn creates a follow-up. Steering remains an explicit shortcut rather than a persistent composer mode. - `Cmd+Enter` on macOS (`Ctrl+Enter` on Windows/Linux) steers the draft into the active turn once; while idle it sends normally. - `Shift+Enter` and `Alt+Enter` always insert a line break, including during an active turn. -- Queued follow-ups render in a pending plate above the composer card, in send order (first at the top). Queued steering renders as transcript bubbles instead. Per plate entry: - - drag the hover grip to reorder the follow-up queue, - - promote (直接发送 / Send now) to steer the entry into the active turn, - - edit, which works the same as on a transcript bubble, - - delete to retract the entry. -- Edit, on a plate entry or a transcript bubble, retracts the entry and restores its full content (text, attachments, directory references, quotes) into the originating Session's composer draft, so a half-edited message can never run; resending queues it at the tail. Delete only retracts. -- Queue contents and mutations are Runtime Host operations (`turn.message.submit`, `queue.entry.promote`, `queue.entry.retract`, `queue.entries.reorder`); the renderer mirrors the authoritative projection. +- The pending plate renders the Host order above the composer. A queued row can be edited, reordered within its lane, promoted from follow-up to steering, or retracted. +- Runtime Host owns submission and mutation (`turn.message.submit`, `queue.entry.update`, `queue.entry.promote`, `queue.entry.retract`, and `queue.entries.reorder`). The renderer never invents a local queue order. - Identical active toasts reuse one toast instead of stacking duplicates. ## Race Fix -Submission routing reads the synchronous live-turn ref and the latest session catalog snapshot at the instant of submission. It does not rely only on the previous React render. +At submission time Desktop consults the live-turn reference together with the latest catalog projection. The decision therefore does not wait for another React commit. ## Deliberate Scope -Per-entry queue mutation is limited to reorder, promote-to-steering, edit, and retract. Pausing the queue and cross-session moves still require separate Runtime Host protocol and durability review. +Mutation remains lane-local. Pausing delivery or moving an entry between sessions would require new Host protocol and durability semantics. diff --git a/packages/runtime-host/src/__tests__/message-coordinator.test.ts b/packages/runtime-host/src/__tests__/message-coordinator.test.ts index fae4265ee47..6737d910eff 100644 --- a/packages/runtime-host/src/__tests__/message-coordinator.test.ts +++ b/packages/runtime-host/src/__tests__/message-coordinator.test.ts @@ -1788,33 +1788,28 @@ test('pull crosses the retract commit cut and only queued entries are retracted' assert.equal(fixture.liveResidencies(), 0); }); -test('entry retract removes one queued entry, replays its outcome, and rejects stale targets', async () => { +test('entry retract has one replayable cut and never crosses an in-flight lease', async () => { const fixture = createFixture(); fixture.coordinator.reserveRootTurn(ROOT); - await submit(fixture, 'steer-1', 'steer me', 'current_turn'); await submit(fixture, 'follow-1', 'first', 'next_turn'); await submit(fixture, 'follow-2', 'second', 'next_turn'); - const retracted = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - retractId: 'retract-entry-1', - }, - operationContext(), - ); - assert.equal(retracted.ok, true); - if (!retracted.ok) return; - assert.equal(retracted.result.queueRevision, 4); + const retract = (entryId: string, retractId: string) => + fixture.coordinator.handlers['queue.entry.retract']( + { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, entryId, retractId }, + operationContext(), + ); + const first = await retract('id-2', 'retract-entry-1'); + assert.deepEqual(first, { ok: true, result: { queueRevision: 4 } }); + const projection = fixture.coordinator.projection(ROOT.sessionId); assert.deepEqual( - fixture.coordinator.projection(ROOT.sessionId).followup.map((entry) => entry.messageId), - ['follow-2'], + projection.steering.map(({ messageId }) => messageId), + ['steer-1'], ); assert.deepEqual( - fixture.coordinator.projection(ROOT.sessionId).steering.map((entry) => entry.messageId), - ['steer-1'], + projection.followup.map(({ messageId }) => messageId), + ['follow-2'], ); assert.deepEqual( await fixture.coordinator.handlers['turn.message.execution.query']( @@ -1828,88 +1823,21 @@ test('entry retract removes one queued entry, replays its outcome, and rejects s }, }, ); + assert.deepEqual(await retract('id-2', 'retract-entry-1'), first); + assertQueueError(await retract('id-3', 'retract-entry-1'), 'operation_conflict'); + assertQueueError(await retract('id-2', 'retract-entry-2'), 'not_found'); - const retry = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - retractId: 'retract-entry-1', - }, - operationContext(), - ); - assert.deepEqual(retry, retracted); - - const conflict = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-3', - retractId: 'retract-entry-1', - }, - operationContext(), - ); - assert.equal(conflict.ok, false); - if (!conflict.ok) assert.equal(conflict.error.code, 'operation_conflict'); - - const missing = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - retractId: 'retract-entry-2', - }, - operationContext(), - ); - assert.equal(missing.ok, false); - if (!missing.ok) assert.equal(missing.error.code, 'not_found'); - - const steering = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-1', - retractId: 'retract-entry-3', - }, - operationContext(), - ); - assert.equal(steering.ok, true); - assert.deepEqual(fixture.coordinator.projection(ROOT.sessionId).steering, []); - assert.equal(fixture.liveResidencies(), 1); - - await fixture.coordinator.handlers['queue.retract']( - { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, retractId: 'cleanup-entry' }, - operationContext(), - ); - fixture.coordinator.abandonRootReservation(ROOT); - await fixture.coordinator.close(); -}); - -test('entry retract of an in-flight steering lease conflicts', async () => { - const fixture = createFixture(); - fixture.coordinator.reserveRootTurn(ROOT); const owner = fixture.coordinator.bindRun(ROOT); - - await submit(fixture, 'steer-1', 'steer me', 'current_turn'); const [lease] = await owner.pull(); assert.ok(lease); - - const outcome = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-1', - retractId: 'retract-in-flight', - }, - operationContext(), - ); - assert.equal(outcome.ok, false); - if (!outcome.ok) assert.equal(outcome.error.code, 'operation_conflict'); - + assertQueueError(await retract('id-1', 'retract-in-flight'), 'operation_conflict'); owner.ack([lease.id]); owner.release(); - const batch = fixture.coordinator.beginTerminalTransition(ROOT); - fixture.coordinator.completeIdle(batch); + await fixture.coordinator.handlers['queue.retract']( + { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, retractId: 'cleanup-entry' }, + operationContext(), + ); + fixture.coordinator.completeIdle(fixture.coordinator.beginTerminalTransition(ROOT)); assert.equal(fixture.liveResidencies(), 0); }); @@ -2124,72 +2052,48 @@ test('entry update keeps relocated inline references ordered and non-overlapping ]); }); -test('entry promote moves a follow-up into the steering queue', async () => { +test('promote changes placement once, replays exactly, and feeds the active owner', async () => { const fixture = createFixture(); fixture.coordinator.reserveRootTurn(ROOT); const owner = fixture.coordinator.bindRun(ROOT); - await submit(fixture, 'follow-1', 'first', 'next_turn'); await submit(fixture, 'follow-2', 'second', 'next_turn'); - const promoted = await fixture.coordinator.handlers['queue.entry.promote']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - promoteId: 'promote-1', - }, - operationContext(), - ); - assert.equal(promoted.ok, true); + const promote = (promoteId: string) => + fixture.coordinator.handlers['queue.entry.promote']( + { + originHostEpoch: 'epoch-1', + sessionId: ROOT.sessionId, + entryId: 'id-2', + promoteId, + }, + operationContext(), + ); + const promoted = await promote('promote-1'); + assert.deepEqual(promoted, { ok: true, result: { queueRevision: 3 } }); const projection = fixture.coordinator.projection(ROOT.sessionId); assert.deepEqual( - projection.steering.map((entry) => [entry.messageId, entry.placement]), + projection.steering.map(({ messageId, placement }) => [messageId, placement]), [['follow-2', 'current_turn']], ); assert.deepEqual( - projection.followup.map((entry) => entry.messageId), + projection.followup.map(({ messageId }) => messageId), ['follow-1'], ); - - const retry = await fixture.coordinator.handlers['queue.entry.promote']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - promoteId: 'promote-1', - }, - operationContext(), - ); - assert.deepEqual(retry, promoted); - - const again = await fixture.coordinator.handlers['queue.entry.promote']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - promoteId: 'promote-2', - }, - operationContext(), - ); - assert.equal(again.ok, false); - if (!again.ok) assert.equal(again.error.code, 'operation_conflict'); - + assert.deepEqual(await promote('promote-1'), promoted); + assertQueueError(await promote('promote-2'), 'operation_conflict'); const leases = await owner.pull(); assert.deepEqual( - leases.map((lease) => lease.messageId), + leases.map(({ messageId }) => messageId), ['follow-2'], ); owner.ack(leases.map((lease) => lease.id)); owner.release(); - - const retracted = await fixture.coordinator.handlers['queue.retract']( + await fixture.coordinator.handlers['queue.retract']( { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, retractId: 'cleanup-promote' }, operationContext(), ); - assert.equal(retracted.ok, true); - const batch = fixture.coordinator.beginTerminalTransition(ROOT); - fixture.coordinator.completeIdle(batch); + fixture.coordinator.completeIdle(fixture.coordinator.beginTerminalTransition(ROOT)); assert.equal(fixture.liveResidencies(), 0); }); @@ -2319,14 +2223,12 @@ test('editing a promoted entry preserves its original submitted placement', asyn ); }); -test('entry promote requires an active Turn', async () => { +test('promote is fenced when the Session has no active Turn', async () => { const fixture = createFixture(); fixture.coordinator.reserveRootTurn(ROOT); - await submit(fixture, 'follow-1', 'first', 'next_turn'); fixture.setRootState({ kind: 'idle' }); - - const promoted = await fixture.coordinator.handlers['queue.entry.promote']( + const outcome = await fixture.coordinator.handlers['queue.entry.promote']( { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, @@ -2335,89 +2237,45 @@ test('entry promote requires an active Turn', async () => { }, operationContext(), ); - assert.equal(promoted.ok, false); - if (!promoted.ok) assert.equal(promoted.error.code, 'operation_conflict'); + assertQueueError(outcome, 'operation_conflict'); }); -test('entries reorder permutes the follow-up queue and rejects stale orders', async () => { +test('reorder commits one exact permutation behind a queue-revision fence', async () => { const fixture = createFixture(); fixture.coordinator.reserveRootTurn(ROOT); - await submit(fixture, 'follow-1', 'first', 'next_turn'); await submit(fixture, 'follow-2', 'second', 'next_turn'); await submit(fixture, 'follow-3', 'third', 'next_turn'); - const revisionBefore = fixture.coordinator.projection(ROOT.sessionId).queueRevision; - - const reordered = await fixture.coordinator.handlers['queue.entries.reorder']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - reorderId: 'reorder-1', - expectedQueueRevision: revisionBefore, - entryIds: ['id-3', 'id-1', 'id-2'], - }, - operationContext(), - ); - assert.equal(reordered.ok, true); - if (!reordered.ok) return; - assert.equal(reordered.result.queueRevision, revisionBefore + 1); + const revision = fixture.coordinator.projection(ROOT.sessionId).queueRevision; + const reorder = (reorderId: string, expectedQueueRevision: number, entryIds: string[]) => + fixture.coordinator.handlers['queue.entries.reorder']( + { + originHostEpoch: 'epoch-1', + sessionId: ROOT.sessionId, + reorderId, + expectedQueueRevision, + entryIds, + }, + operationContext(), + ); + const intended = ['id-3', 'id-1', 'id-2']; + const reordered = await reorder('reorder-1', revision, intended); + assert.deepEqual(reordered, { ok: true, result: { queueRevision: revision + 1 } }); assert.deepEqual( - fixture.coordinator.projection(ROOT.sessionId).followup.map((entry) => entry.messageId), + fixture.coordinator.projection(ROOT.sessionId).followup.map(({ messageId }) => messageId), ['follow-3', 'follow-1', 'follow-2'], ); - - const retry = await fixture.coordinator.handlers['queue.entries.reorder']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - reorderId: 'reorder-1', - expectedQueueRevision: revisionBefore, - entryIds: ['id-3', 'id-1', 'id-2'], - }, - operationContext(), - ); - assert.deepEqual(retry, reordered); - - const staleMembers = { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - reorderId: 'reorder-stale-members', - entryIds: ['id-2', 'id-3', 'id-1'], - expectedQueueRevision: revisionBefore, - }; - const overwritten = await fixture.coordinator.handlers['queue.entries.reorder']( - staleMembers, - operationContext(), - ); - assert.equal(overwritten.ok, false); - if (!overwritten.ok) assert.equal(overwritten.error.code, 'operation_conflict'); - - const stale = await fixture.coordinator.handlers['queue.entries.reorder']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - reorderId: 'reorder-2', - expectedQueueRevision: revisionBefore, - entryIds: ['id-2', 'id-1'], - }, - operationContext(), - ); - assert.equal(stale.ok, false); - if (!stale.ok) assert.equal(stale.error.code, 'operation_conflict'); - - const unchanged = await fixture.coordinator.handlers['queue.entries.reorder']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - reorderId: 'reorder-3', - expectedQueueRevision: revisionBefore + 1, - entryIds: ['id-3', 'id-1', 'id-2'], - }, - operationContext(), - ); - assert.equal(unchanged.ok, true); - if (unchanged.ok) assert.equal(unchanged.result.queueRevision, revisionBefore + 1); - + assert.deepEqual(await reorder('reorder-1', revision, intended), reordered); + for (const [id, expected, entries] of [ + ['stale-members', revision, ['id-2', 'id-3', 'id-1']], + ['missing-member', revision, ['id-2', 'id-1']], + ] as const) { + assertQueueError(await reorder(id, expected, [...entries]), 'operation_conflict'); + } + assert.deepEqual(await reorder('unchanged', revision + 1, intended), { + ok: true, + result: { queueRevision: revision + 1 }, + }); await fixture.coordinator.handlers['queue.retract']( { originHostEpoch: 'epoch-1', sessionId: ROOT.sessionId, retractId: 'cleanup-reorder' }, operationContext(), @@ -2544,46 +2402,45 @@ test('steering edits and reorders settle before one complete batch is pulled; fo await fixture.coordinator.close(); }); -test('queued mutations reject a queue that is draining into the next Turn', async () => { +test('queue mutations stop at the transition cut and resume on the successor', async () => { const fixture = createFixture(); fixture.coordinator.reserveRootTurn(ROOT); const owner = fixture.coordinator.bindRun(ROOT); - await submit(fixture, 'follow-1', 'first', 'next_turn'); await submit(fixture, 'follow-2', 'second', 'next_turn'); owner.release(); const batch = fixture.coordinator.beginTerminalTransition(ROOT); - - const retracted = await fixture.coordinator.handlers['queue.entry.retract']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - entryId: 'id-2', - retractId: 'retract-draining', - }, - operationContext(), + assertQueueError( + await fixture.coordinator.handlers['queue.entry.retract']( + { + originHostEpoch: 'epoch-1', + sessionId: ROOT.sessionId, + entryId: 'id-2', + retractId: 'retract-draining', + }, + operationContext(), + ), + 'operation_conflict', ); - assert.equal(retracted.ok, false); - if (!retracted.ok) assert.equal(retracted.error.code, 'operation_conflict'); - - const reordered = await fixture.coordinator.handlers['queue.entries.reorder']( - { - originHostEpoch: 'epoch-1', - sessionId: ROOT.sessionId, - reorderId: 'reorder-draining', - expectedQueueRevision: fixture.coordinator.projection(ROOT.sessionId).queueRevision, - entryIds: ['id-2', 'id-1'], - }, - operationContext(), + assertQueueError( + await fixture.coordinator.handlers['queue.entries.reorder']( + { + originHostEpoch: 'epoch-1', + sessionId: ROOT.sessionId, + reorderId: 'reorder-draining', + expectedQueueRevision: fixture.coordinator.projection(ROOT.sessionId).queueRevision, + entryIds: ['id-2', 'id-1'], + }, + operationContext(), + ), + 'operation_conflict', ); - assert.equal(reordered.ok, false); - if (!reordered.ok) assert.equal(reordered.error.code, 'operation_conflict'); - - fixture.coordinator.commitNextRoot(batch, { + const successor = { sessionId: ROOT.sessionId, turnId: 'turn-2', runId: 'run-2', - }); + }; + fixture.coordinator.commitNextRoot(batch, successor); const after = await fixture.coordinator.handlers['queue.entries.reorder']( { originHostEpoch: 'epoch-1', @@ -2594,7 +2451,10 @@ test('queued mutations reject a queue that is draining into the next Turn', asyn }, operationContext(), ); - assert.equal(after.ok, true); + assert.deepEqual(after, { + ok: true, + result: { queueRevision: fixture.coordinator.projection(ROOT.sessionId).queueRevision }, + }); await fixture.coordinator.handlers['queue.retract']( { originHostEpoch: 'epoch-1', @@ -2603,11 +2463,7 @@ test('queued mutations reject a queue that is draining into the next Turn', asyn }, operationContext(), ); - fixture.coordinator.abandonRootReservation({ - sessionId: ROOT.sessionId, - turnId: 'turn-2', - runId: 'run-2', - }); + fixture.coordinator.abandonRootReservation(successor); await fixture.coordinator.close(); }); @@ -4310,6 +4166,17 @@ function operationContext(connectionId = 'connection-1') { acquireResidency: () => ({ release: () => undefined }), }; } + +function assertQueueError( + outcome: + | { readonly ok: true } + | { readonly ok: false; readonly error: { readonly code: string } }, + code: string, +): void { + assert.equal(outcome.ok, false); + if (!outcome.ok) assert.equal(outcome.error.code, code); +} + test('a lone folded steering entry leaves the queue projection decodable', async () => { // A steer the run never pulls is folded into `followup` at the terminal // transition. It has to arrive there as a followup entry: the wire decoder diff --git a/packages/runtime-host/src/__tests__/protocol.test.ts b/packages/runtime-host/src/__tests__/protocol.test.ts index 729eb417265..243af563c02 100644 --- a/packages/runtime-host/src/__tests__/protocol.test.ts +++ b/packages/runtime-host/src/__tests__/protocol.test.ts @@ -1582,111 +1582,6 @@ describe('Runtime Host bootstrap protocol', () => { assert.deepEqual(decodeClientFrame(submit), submit); assert.deepEqual(decodeClientFrame(retract), retract); assert.deepEqual(decodeClientFrame(interrupt), interrupt); - const entryRetract = { - requestId: 'entry-retract-request-1', - operation: 'queue.entry.retract' as const, - input: { - originHostEpoch: 'epoch-1', - sessionId: 'session-1', - entryId: 'entry-1', - retractId: 'retract-2', - }, - }; - const entryPromote = { - requestId: 'entry-promote-request-1', - operation: 'queue.entry.promote' as const, - input: { - originHostEpoch: 'epoch-1', - sessionId: 'session-1', - entryId: 'entry-1', - promoteId: 'promote-1', - }, - }; - const entryUpdate = { - requestId: 'entry-update-request-1', - operation: 'queue.entry.update' as const, - input: { - originHostEpoch: 'epoch-1', - sessionId: 'session-1', - entryId: 'entry-1', - updateId: 'update-1', - expectedQueueRevision: 7, - text: 'updated message', - }, - }; - const entriesReorder = { - requestId: 'entries-reorder-request-1', - operation: 'queue.entries.reorder' as const, - input: { - originHostEpoch: 'epoch-1', - sessionId: 'session-1', - reorderId: 'reorder-1', - expectedQueueRevision: 7, - entryIds: ['entry-2', 'entry-1'], - }, - }; - assert.deepEqual(decodeClientFrame(entryRetract), entryRetract); - assert.deepEqual(decodeClientFrame(entryPromote), entryPromote); - assert.deepEqual(decodeClientFrame(entryUpdate), entryUpdate); - assert.deepEqual(decodeClientFrame(entriesReorder), entriesReorder); - for (const expectedQueueRevision of [undefined, -1, 1.5]) { - assert.throws( - () => - decodeClientFrame({ - ...entriesReorder, - input: { ...entriesReorder.input, expectedQueueRevision }, - }), - isInvalidFrame, - ); - } - assert.throws( - () => - decodeClientFrame({ - ...entryUpdate, - input: { ...entryUpdate.input, text: ' ' }, - }), - isInvalidFrame, - ); - assert.throws( - () => - decodeClientFrame({ - ...entriesReorder, - input: { ...entriesReorder.input, entryIds: ['entry-1', 'entry-1'] }, - }), - isInvalidFrame, - ); - assert.throws( - () => - decodeClientFrame({ - ...entriesReorder, - input: { ...entriesReorder.input, entryIds: [] }, - }), - isInvalidFrame, - ); - assert.throws( - () => - decodeClientFrame({ - ...entriesReorder, - input: { ...entriesReorder.input, entryIds: ['not/a/semantic/id'] }, - }), - isInvalidFrame, - ); - assert.throws( - () => - decodeClientFrame({ - ...entryRetract, - input: { ...entryRetract.input, generation: 1 }, - }), - isInvalidFrame, - ); - assert.throws( - () => - decodeClientFrame({ - ...entryPromote, - input: { ...entryPromote.input, entryId: 'not/a/semantic/id' }, - }), - isInvalidFrame, - ); assert.throws( () => decodeClientFrame({ ...submit, input: { ...submit.input, originHostEpoch: undefined } }), @@ -2154,31 +2049,6 @@ describe('Runtime Host bootstrap protocol', () => { isInvalidFrame, ); } - for (const [operation, requestId] of [ - ['queue.entry.retract', 'entry-retract-response'], - ['queue.entry.promote', 'entry-promote-response'], - ['queue.entry.update', 'entry-update-response'], - ['queue.entries.reorder', 'entries-reorder-response'], - ] as const) { - assert.doesNotThrow(() => - decodeHostFrame({ - requestId, - operation, - ok: true, - result: { queueRevision: 8 }, - }), - ); - assert.throws( - () => - decodeHostFrame({ - requestId, - operation, - ok: true, - result: { queueRevision: 8, retracted: [] }, - }), - isInvalidFrame, - ); - } const retracted = [retractedMessage()]; assert.doesNotThrow(() => decodeHostFrame({ diff --git a/packages/runtime-host/src/__tests__/queue-mutation-protocol.test.ts b/packages/runtime-host/src/__tests__/queue-mutation-protocol.test.ts new file mode 100644 index 00000000000..2cdfa3911a4 --- /dev/null +++ b/packages/runtime-host/src/__tests__/queue-mutation-protocol.test.ts @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { RuntimeHostProtocolError } from '../protocol/errors.js'; +import { decodeClientFrame, decodeHostFrame } from '../protocol/index.js'; + +const BASE = { + originHostEpoch: 'epoch-1', + sessionId: 'session-1', + entryId: 'entry-1', +} as const; + +const REQUESTS = [ + { + requestId: 'retract-request', + operation: 'queue.entry.retract' as const, + input: { ...BASE, retractId: 'retract-1' }, + }, + { + requestId: 'promote-request', + operation: 'queue.entry.promote' as const, + input: { ...BASE, promoteId: 'promote-1' }, + }, + { + requestId: 'update-request', + operation: 'queue.entry.update' as const, + input: { ...BASE, updateId: 'update-1', expectedQueueRevision: 7, text: 'updated' }, + }, + { + requestId: 'reorder-request', + operation: 'queue.entries.reorder' as const, + input: { + originHostEpoch: BASE.originHostEpoch, + sessionId: BASE.sessionId, + reorderId: 'reorder-1', + expectedQueueRevision: 7, + entryIds: ['entry-2', 'entry-1'], + }, + }, +] as const; + +test('queue mutation requests decode as exact operation-specific records', () => { + for (const request of REQUESTS) { + assert.deepEqual(decodeClientFrame(request), request); + assert.deepEqual( + decodeHostFrame({ + requestId: request.requestId, + operation: request.operation, + ok: true, + result: { queueRevision: 8 }, + }), + { + requestId: request.requestId, + operation: request.operation, + ok: true, + result: { queueRevision: 8 }, + }, + ); + } +}); + +test('queue mutation requests reject stale, ambiguous, and non-semantic inputs', () => { + const retract = REQUESTS[0]; + const promote = REQUESTS[1]; + const update = REQUESTS[2]; + const reorder = REQUESTS[3]; + const invalid = [ + { ...retract, input: { ...retract.input, generation: 1 } }, + { ...promote, input: { ...promote.input, entryId: 'not/a/semantic/id' } }, + { ...update, input: { ...update.input, text: ' ' } }, + { ...update, input: { ...update.input, expectedQueueRevision: -1 } }, + { ...reorder, input: { ...reorder.input, expectedQueueRevision: 1.5 } }, + { ...reorder, input: { ...reorder.input, entryIds: [] } }, + { ...reorder, input: { ...reorder.input, entryIds: ['entry-1', 'entry-1'] } }, + { ...reorder, input: { ...reorder.input, entryIds: ['not/a/semantic/id'] } }, + ]; + for (const request of invalid) { + assert.throws(() => decodeClientFrame(request), isInvalidFrame); + } +}); + +test('queue mutation responses expose only the committed queue revision', () => { + for (const request of REQUESTS) { + assert.throws( + () => + decodeHostFrame({ + requestId: request.requestId, + operation: request.operation, + ok: true, + result: { queueRevision: 8, retracted: [] }, + }), + isInvalidFrame, + ); + } +}); + +function isInvalidFrame(error: unknown): boolean { + return error instanceof RuntimeHostProtocolError && error.code === 'invalid_frame'; +} diff --git a/packages/runtime-host/src/protocol/index.ts b/packages/runtime-host/src/protocol/index.ts index 0c4ab8c1d66..52d6521fa45 100644 --- a/packages/runtime-host/src/protocol/index.ts +++ b/packages/runtime-host/src/protocol/index.ts @@ -84,6 +84,7 @@ export * from './project-catalog-change.js'; export * from './execution-inspect.js'; export * from './external-session.js'; export * from './message.js'; +export * from './queue-mutation.js'; export * from './operations.js'; export * from './runtime-resource.js'; export * from './session-continuity.js'; @@ -453,8 +454,8 @@ export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 196 as const; // 41: Context compaction returns a typed terminal outcome on both Turn // snapshots and context.compact results. Epoch-40 peers reject these closed // shapes after admission, so mixed peers must fail during the handshake. -// 40: The message queue gains per-entry mutation operations -// (queue.entry.promote, queue.entry.retract, queue.entries.reorder). +// 40: Queue entries become independently addressable for promotion, +// retraction, and ordering. Epoch-39 peers do not recognize those commands. // 39: Client Capability tool descriptors carry trusted activity semantics and // invocations can stream bounded progress frames. // 38: `execute` is no longer a permission mode. Frame decoders reject it, so a diff --git a/packages/runtime-host/src/protocol/message-queue-limits.ts b/packages/runtime-host/src/protocol/message-queue-limits.ts new file mode 100644 index 00000000000..beda34aa551 --- /dev/null +++ b/packages/runtime-host/src/protocol/message-queue-limits.ts @@ -0,0 +1,22 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +export const MESSAGE_QUEUE_MAX_ENTRIES = 64; +export const MESSAGE_QUEUE_PROJECTION_MAX_BYTES = 52 * 1024; +export const MESSAGE_OPERATION_RESULT_MAX_BYTES = 56 * 1024; diff --git a/packages/runtime-host/src/protocol/message.ts b/packages/runtime-host/src/protocol/message.ts index dbfb0eccf4d..7b147c8c45c 100644 --- a/packages/runtime-host/src/protocol/message.ts +++ b/packages/runtime-host/src/protocol/message.ts @@ -26,7 +26,6 @@ import { requireId, requireRecord, requireShapedRecord, - requireUtf8String, } from './codec.js'; import { defineOperation } from './operation-spec.js'; import { @@ -36,16 +35,30 @@ import { decodeTurnOrchestration, decodeTurnSnapshot, type MessageContent, - TURN_MESSAGE_TEXT_MAX_BYTES, type TurnSnapshot, } from './turn.js'; import { decodeSkillInvocationResult } from '@maka/core/skill-invocation'; import type { SkillInvocationResult } from '@maka/core/skill-invocation'; import type { TurnOrchestration } from '@maka/core/runtime-inputs'; - -export const MESSAGE_QUEUE_MAX_ENTRIES = 64; -export const MESSAGE_QUEUE_PROJECTION_MAX_BYTES = 52 * 1024; -export const MESSAGE_OPERATION_RESULT_MAX_BYTES = 56 * 1024; +import { + MESSAGE_OPERATION_RESULT_MAX_BYTES, + MESSAGE_QUEUE_MAX_ENTRIES, + MESSAGE_QUEUE_PROJECTION_MAX_BYTES, +} from './message-queue-limits.js'; +import { QUEUE_MUTATION_OPERATION_SPECS } from './queue-mutation.js'; + +export { + MESSAGE_OPERATION_RESULT_MAX_BYTES, + MESSAGE_QUEUE_MAX_ENTRIES, + MESSAGE_QUEUE_PROJECTION_MAX_BYTES, +} from './message-queue-limits.js'; +export type { + QueueEntriesReorderInput, + QueueEntryPromoteInput, + QueueEntryRetractInput, + QueueEntryUpdateInput, + QueueMutationResult, +} from './queue-mutation.js'; export type MessagePlacement = 'current_turn' | 'next_turn'; @@ -157,41 +170,6 @@ export interface QueueRetractResult { readonly retracted: readonly RetractedMessageSnapshot[]; } -export interface QueueEntryRetractInput { - readonly originHostEpoch: string; - readonly sessionId: string; - readonly entryId: string; - readonly retractId: string; -} - -export interface QueueMutationResult { - readonly queueRevision: number; -} - -export interface QueueEntryPromoteInput { - readonly originHostEpoch: string; - readonly sessionId: string; - readonly entryId: string; - readonly promoteId: string; -} - -export interface QueueEntryUpdateInput { - readonly originHostEpoch: string; - readonly sessionId: string; - readonly entryId: string; - readonly updateId: string; - readonly expectedQueueRevision: number; - readonly text: string; -} - -export interface QueueEntriesReorderInput { - readonly originHostEpoch: string; - readonly sessionId: string; - readonly reorderId: string; - readonly expectedQueueRevision: number; - readonly entryIds: readonly string[]; -} - export interface TurnInterruptInput { readonly originHostEpoch: string; readonly sessionId: string; @@ -247,34 +225,7 @@ export const MESSAGE_OPERATION_SPECS = { decodeInput: decodeQueueRetractInput, decodeOutput: decodeQueueRetractResult, }), - 'queue.entry.retract': defineOperation({ - mode: 'command', - availability: 'ready', - errors: MESSAGE_OPERATION_ERRORS, - decodeInput: decodeQueueEntryRetractInput, - decodeOutput: decodeQueueMutationResult, - }), - 'queue.entry.promote': defineOperation({ - mode: 'command', - availability: 'ready', - errors: MESSAGE_OPERATION_ERRORS, - decodeInput: decodeQueueEntryPromoteInput, - decodeOutput: decodeQueueMutationResult, - }), - 'queue.entry.update': defineOperation({ - mode: 'command', - availability: 'ready', - errors: MESSAGE_OPERATION_ERRORS, - decodeInput: decodeQueueEntryUpdateInput, - decodeOutput: decodeQueueMutationResult, - }), - 'queue.entries.reorder': defineOperation({ - mode: 'command', - availability: 'ready', - errors: MESSAGE_OPERATION_ERRORS, - decodeInput: decodeQueueEntriesReorderInput, - decodeOutput: decodeQueueMutationResult, - }), + ...QUEUE_MUTATION_OPERATION_SPECS, 'turn.interrupt': defineOperation({ mode: 'control', availability: 'ready', @@ -509,85 +460,6 @@ function decodeQueueRetractResult(value: unknown): QueueRetractResult { return result; } -function decodeQueueEntryRetractInput(value: unknown): QueueEntryRetractInput { - const record = requireExactRecord(value, 'queue.entry.retract input', [ - 'originHostEpoch', - 'sessionId', - 'entryId', - 'retractId', - ]); - return { - originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), - sessionId: requireEntityId(record.sessionId, 'sessionId'), - entryId: requireEntityId(record.entryId, 'entryId'), - retractId: requireEntityId(record.retractId, 'retractId'), - }; -} - -function decodeQueueMutationResult(value: unknown): QueueMutationResult { - const record = requireExactRecord(value, 'queue mutation result', ['queueRevision']); - return { queueRevision: requireCount(record.queueRevision, 'queueRevision') }; -} - -function decodeQueueEntryPromoteInput(value: unknown): QueueEntryPromoteInput { - const record = requireExactRecord(value, 'queue.entry.promote input', [ - 'originHostEpoch', - 'sessionId', - 'entryId', - 'promoteId', - ]); - return { - originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), - sessionId: requireEntityId(record.sessionId, 'sessionId'), - entryId: requireEntityId(record.entryId, 'entryId'), - promoteId: requireEntityId(record.promoteId, 'promoteId'), - }; -} - -function decodeQueueEntryUpdateInput(value: unknown): QueueEntryUpdateInput { - const record = requireExactRecord(value, 'queue.entry.update input', [ - 'originHostEpoch', - 'sessionId', - 'entryId', - 'updateId', - 'expectedQueueRevision', - 'text', - ]); - const text = requireUtf8String(record.text, 'Message text', TURN_MESSAGE_TEXT_MAX_BYTES); - if (text.trim().length === 0) throw invalidProtocolFrame('Invalid Message text'); - return { - originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), - sessionId: requireEntityId(record.sessionId, 'sessionId'), - entryId: requireEntityId(record.entryId, 'entryId'), - updateId: requireEntityId(record.updateId, 'updateId'), - expectedQueueRevision: requireCount(record.expectedQueueRevision, 'expectedQueueRevision'), - text, - }; -} - -function decodeQueueEntriesReorderInput(value: unknown): QueueEntriesReorderInput { - const record = requireExactRecord(value, 'queue.entries.reorder input', [ - 'originHostEpoch', - 'sessionId', - 'reorderId', - 'expectedQueueRevision', - 'entryIds', - ]); - const entryIds = requireBoundedArray(record.entryIds, 'reorder entry identities').map((entryId) => - requireEntityId(entryId, 'entryId'), - ); - if (entryIds.length === 0 || new Set(entryIds).size !== entryIds.length) { - throw invalidProtocolFrame('Invalid reorder entry identities'); - } - return { - originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), - sessionId: requireEntityId(record.sessionId, 'sessionId'), - reorderId: requireEntityId(record.reorderId, 'reorderId'), - expectedQueueRevision: requireCount(record.expectedQueueRevision, 'expectedQueueRevision'), - entryIds, - }; -} - function decodeTurnInterruptInput(value: unknown): TurnInterruptInput { const record = requireExactRecord(value, 'turn.interrupt input', [ 'originHostEpoch', diff --git a/packages/runtime-host/src/protocol/queue-mutation.ts b/packages/runtime-host/src/protocol/queue-mutation.ts new file mode 100644 index 00000000000..0b10cc3d7b2 --- /dev/null +++ b/packages/runtime-host/src/protocol/queue-mutation.ts @@ -0,0 +1,187 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + requireCount, + requireEntityId, + requireExactRecord, + requireId, + requireUtf8String, +} from './codec.js'; +import { invalidProtocolFrame } from './errors.js'; +import { MESSAGE_QUEUE_MAX_ENTRIES } from './message-queue-limits.js'; +import { defineOperation } from './operation-spec.js'; +import { TURN_MESSAGE_TEXT_MAX_BYTES } from './turn.js'; + +interface QueueEntryCommand { + readonly originHostEpoch: string; + readonly sessionId: string; + readonly entryId: string; +} + +export interface QueueEntryRetractInput extends QueueEntryCommand { + readonly retractId: string; +} + +export interface QueueEntryPromoteInput extends QueueEntryCommand { + readonly promoteId: string; +} + +export interface QueueEntryUpdateInput extends QueueEntryCommand { + readonly updateId: string; + readonly expectedQueueRevision: number; + readonly text: string; +} + +export interface QueueEntriesReorderInput { + readonly originHostEpoch: string; + readonly sessionId: string; + readonly reorderId: string; + readonly expectedQueueRevision: number; + readonly entryIds: readonly string[]; +} + +export interface QueueMutationResult { + readonly queueRevision: number; +} + +const QUEUE_MUTATION_ERRORS = [ + 'host_not_ready', + 'host_draining', + 'operation_unavailable', + 'not_found', + 'session_archived', + 'session_busy', + 'operation_conflict', + 'outcome_unknown', + 'internal_failure', +] as const; + +export const QUEUE_MUTATION_OPERATION_SPECS = { + 'queue.entry.retract': defineOperation({ + mode: 'command', + availability: 'ready', + errors: QUEUE_MUTATION_ERRORS, + decodeInput: decodeQueueEntryRetractInput, + decodeOutput: decodeQueueMutationResult, + }), + 'queue.entry.promote': defineOperation({ + mode: 'command', + availability: 'ready', + errors: QUEUE_MUTATION_ERRORS, + decodeInput: decodeQueueEntryPromoteInput, + decodeOutput: decodeQueueMutationResult, + }), + 'queue.entry.update': defineOperation({ + mode: 'command', + availability: 'ready', + errors: QUEUE_MUTATION_ERRORS, + decodeInput: decodeQueueEntryUpdateInput, + decodeOutput: decodeQueueMutationResult, + }), + 'queue.entries.reorder': defineOperation({ + mode: 'command', + availability: 'ready', + errors: QUEUE_MUTATION_ERRORS, + decodeInput: decodeQueueEntriesReorderInput, + decodeOutput: decodeQueueMutationResult, + }), +} as const; + +export function decodeQueueEntryRetractInput(value: unknown): QueueEntryRetractInput { + const command = decodeEntryCommand(value, 'queue.entry.retract input', 'retractId'); + const { operationId, ...identity } = command; + return { ...identity, retractId: requireEntityId(operationId, 'retractId') }; +} + +export function decodeQueueEntryPromoteInput(value: unknown): QueueEntryPromoteInput { + const command = decodeEntryCommand(value, 'queue.entry.promote input', 'promoteId'); + const { operationId, ...identity } = command; + return { ...identity, promoteId: requireEntityId(operationId, 'promoteId') }; +} + +export function decodeQueueEntryUpdateInput(value: unknown): QueueEntryUpdateInput { + const record = requireExactRecord(value, 'queue.entry.update input', [ + 'originHostEpoch', + 'sessionId', + 'entryId', + 'updateId', + 'expectedQueueRevision', + 'text', + ]); + const text = requireUtf8String(record.text, 'Message text', TURN_MESSAGE_TEXT_MAX_BYTES); + if (text.trim().length === 0) throw invalidProtocolFrame('Invalid Message text'); + return { + ...decodeEntryIdentity(record), + updateId: requireEntityId(record.updateId, 'updateId'), + expectedQueueRevision: requireCount(record.expectedQueueRevision, 'expectedQueueRevision'), + text, + }; +} + +export function decodeQueueEntriesReorderInput(value: unknown): QueueEntriesReorderInput { + const record = requireExactRecord(value, 'queue.entries.reorder input', [ + 'originHostEpoch', + 'sessionId', + 'reorderId', + 'expectedQueueRevision', + 'entryIds', + ]); + if (!Array.isArray(record.entryIds) || record.entryIds.length > MESSAGE_QUEUE_MAX_ENTRIES) { + throw invalidProtocolFrame('Invalid reorder entry identities'); + } + const entryIds = record.entryIds.map((entryId) => requireEntityId(entryId, 'entryId')); + if (entryIds.length === 0 || new Set(entryIds).size !== entryIds.length) { + throw invalidProtocolFrame('Invalid reorder entry identities'); + } + return { + originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), + sessionId: requireEntityId(record.sessionId, 'sessionId'), + reorderId: requireEntityId(record.reorderId, 'reorderId'), + expectedQueueRevision: requireCount(record.expectedQueueRevision, 'expectedQueueRevision'), + entryIds, + }; +} + +export function decodeQueueMutationResult(value: unknown): QueueMutationResult { + const record = requireExactRecord(value, 'queue mutation result', ['queueRevision']); + return { queueRevision: requireCount(record.queueRevision, 'queueRevision') }; +} + +function decodeEntryCommand( + value: unknown, + label: string, + operationIdKey: 'retractId' | 'promoteId', +): QueueEntryCommand & { readonly operationId: unknown } { + const record = requireExactRecord(value, label, [ + 'originHostEpoch', + 'sessionId', + 'entryId', + operationIdKey, + ]); + return { ...decodeEntryIdentity(record), operationId: record[operationIdKey] }; +} + +function decodeEntryIdentity(record: Record): QueueEntryCommand { + return { + originHostEpoch: requireId(record.originHostEpoch, 'originHostEpoch'), + sessionId: requireEntityId(record.sessionId, 'sessionId'), + entryId: requireEntityId(record.entryId, 'entryId'), + }; +} diff --git a/packages/runtime-host/src/server/message-coordinator.ts b/packages/runtime-host/src/server/message-coordinator.ts index 93ebeaa1148..21c8fd054a8 100644 --- a/packages/runtime-host/src/server/message-coordinator.ts +++ b/packages/runtime-host/src/server/message-coordinator.ts @@ -27,7 +27,6 @@ import { normalizeMessageContent, type MessageContent, } from '@maka/core/events'; -import { exactQueueReorder } from '@maka/core/message-queue-order'; import type { RuntimeEvent } from '@maka/core/runtime-event'; import type { TurnOrchestration } from '@maka/core/runtime-inputs'; import type { SkillInvocationResult } from '@maka/core/skill-invocation'; @@ -49,7 +48,6 @@ import { type RootTurnSourceMessageReceipt, type SubmittedTurnIntent, } from '@maka/storage/execution-stores'; -import type { HostOperationErrorCode, OperationSpec } from '../protocol/operation-spec.js'; import { MESSAGE_QUEUE_MAX_ENTRIES, MESSAGE_QUEUE_PROJECTION_MAX_BYTES, @@ -84,6 +82,15 @@ import { } from './operation-dispatcher.js'; import { type SessionAdmissionLease, SessionAdmissionGate } from './session-admission-gate.js'; import type { LogicalRuntimeExecution } from '@maka/core/runtime-logical-execution'; +import { QueuedMutationExecutor, type QueuedMutationKind } from './queued-mutation-executor.js'; +import { + commitFollowupPromotion, + commitQueueReorder, + hasInFlightEntry, + locateQueuedEntry, + planQueueReorder, + removeQueuedEntry, +} from './message-queue-state.js'; type MessageOperationErrorCode = | 'host_draining' @@ -324,32 +331,13 @@ interface PendingSubmit { readonly result: Promise>; } -type QueuedMutationKind = 'retract' | 'retract_entry' | 'promote' | 'update_entry' | 'reorder'; - type MessageOperationKind = QueuedMutationKind | 'submit' | 'interrupt'; -interface PendingQueuedMutation { - readonly payload: { readonly sessionId: string }; - readonly result: Promise>; -} - interface CompletedOperation { readonly payloadIdentity: object; readonly result: object; } -interface QueuedMutationOptions< - I extends { readonly originHostEpoch: string; readonly sessionId: string }, - R, -> { - readonly spec: OperationSpec; - readonly operationKind: QueuedMutationKind; - readonly operationId: string; - readonly verb: string; - readonly input: I; - readonly execute: () => Promise>; -} - interface InterruptDeferred { readonly promise: Promise>; resolve(result: MessageOutcome): void; @@ -434,8 +422,8 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { readonly #preflightSessionSnapshot: CandidateSnapshotPreflight; readonly #sessions = new Map(); readonly #pendingSubmits = new Map(); - readonly #pendingQueuedMutations = new Map(); readonly #completedOperations = new Map(); + readonly #queueMutations: QueuedMutationExecutor; #draining = false; #failStopped = false; @@ -453,6 +441,13 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { this.#onProjectionChanged = options.onProjectionChanged ?? (() => undefined); this.#createId = options.createId ?? randomUUID; this.#preflightSessionSnapshot = options.preflightSessionSnapshot; + this.#queueMutations = new QueuedMutationExecutor({ + hostEpoch: this.#hostEpoch, + admissions: this.#sessionAdmission, + isFailStopped: () => this.#failStopped, + readCompleted: (kind, sessionId, operationId) => + this.#completedOperations.get(queuedMutationKey(kind, sessionId, operationId)), + }); } projection(sessionId: string): SessionMessageQueueProjection { @@ -1597,24 +1592,21 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { } private retract(input: QueueRetractInput): Promise> { - return this.#runQueuedMutation({ + return this.#queueMutations.run({ spec: MESSAGE_OPERATION_SPECS['queue.retract'], - operationKind: 'retract', - operationId: input.retractId, + kind: 'retract', + id: input.retractId, verb: 'Retract', input, + payloadIdentity: completedPayloadIdentity('retract', input), execute: () => this.#retractAdmitted(input), }); } async #retractAdmitted(input: QueueRetractInput): Promise> { - const header = await this.#root.readSessionHeader(input.sessionId); - if (this.#failStopped) { - return failure('host_draining', 'Runtime Host message authority has failed'); - } - if (!header) return failure('not_found', 'Session does not exist'); - if (header.isArchived) return failure('session_archived', 'Session is archived'); - const state = this.#state(input.sessionId); + const admitted = await this.#openQueueMutation(input.sessionId, true); + if (!admitted.ok) return admitted; + const state = admitted.result; if ( !retractionResultFits( state, @@ -1648,12 +1640,13 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { private retractQueuedEntry( input: QueueEntryRetractInput, ): Promise> { - return this.#runQueuedMutation({ + return this.#queueMutations.run({ spec: MESSAGE_OPERATION_SPECS['queue.entry.retract'], - operationKind: 'retract_entry', - operationId: input.retractId, + kind: 'retract_entry', + id: input.retractId, verb: 'Retract', input, + payloadIdentity: completedPayloadIdentity('retract_entry', input), execute: () => this.#retractQueuedEntryAdmitted(input), }); } @@ -1661,12 +1654,13 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { private promoteQueuedEntry( input: QueueEntryPromoteInput, ): Promise> { - return this.#runQueuedMutation({ + return this.#queueMutations.run({ spec: MESSAGE_OPERATION_SPECS['queue.entry.promote'], - operationKind: 'promote', - operationId: input.promoteId, + kind: 'promote', + id: input.promoteId, verb: 'Promote', input, + payloadIdentity: completedPayloadIdentity('promote', input), execute: () => this.#promoteQueuedEntryAdmitted(input), }); } @@ -1674,12 +1668,13 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { private updateQueuedEntry( input: QueueEntryUpdateInput, ): Promise> { - return this.#runQueuedMutation({ + return this.#queueMutations.run({ spec: MESSAGE_OPERATION_SPECS['queue.entry.update'], - operationKind: 'update_entry', - operationId: input.updateId, + kind: 'update_entry', + id: input.updateId, verb: 'Update', input, + payloadIdentity: completedPayloadIdentity('update_entry', input), execute: () => this.#updateQueuedEntryAdmitted(input), }); } @@ -1687,123 +1682,32 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { private reorderQueuedEntries( input: QueueEntriesReorderInput, ): Promise> { - return this.#runQueuedMutation({ + return this.#queueMutations.run({ spec: MESSAGE_OPERATION_SPECS['queue.entries.reorder'], - operationKind: 'reorder', - operationId: input.reorderId, + kind: 'reorder', + id: input.reorderId, verb: 'Reorder', input, + payloadIdentity: completedPayloadIdentity('reorder', input), execute: () => this.#reorderQueuedEntriesAdmitted(input), }); } - #runQueuedMutation( - options: QueuedMutationOptions, - ): Promise> { - const { input } = options; - const isCurrentEpoch = input.originHostEpoch === this.#hostEpoch; - const key = queuedMutationKey(options.operationKind, input.sessionId, options.operationId); - if (isCurrentEpoch) { - const pending = this.#pendingQueuedMutations.get(key); - if (pending) { - return samePayload(pending.payload, input) - ? (pending.result as Promise>) - : Promise.resolve( - failure('operation_conflict', `${options.verb} identity has a different payload`), - ); - } - } - if (this.#failStopped) { - return Promise.resolve(failure('host_draining', 'Runtime Host message authority has failed')); - } - if (!isCurrentEpoch) { - return Promise.resolve( - failure('outcome_unknown', `${options.verb} outcome is not durable across Host Epochs`), - ); - } - const result = this.#admitQueuedMutation(options); - this.#pendingQueuedMutations.set(key, { payload: input, result }); - void result.then( - () => this.#deletePendingQueuedMutation(key, result), - () => this.#deletePendingQueuedMutation(key, result), - ); - return result; - } - - #admitQueuedMutation< - I extends { readonly originHostEpoch: string; readonly sessionId: string }, - R, - >(options: QueuedMutationOptions): Promise> { - return this.#sessionAdmission.run(options.input.sessionId, async () => { - if (this.#failStopped) { - return failure('host_draining', 'Runtime Host message authority has failed'); - } - const receipt = await this.#readCompletedQueuedMutation(options); - if (receipt) { - return samePayload( - receipt.payloadIdentity, - completedPayloadIdentity(options.operationKind, options.input), - ) - ? success(receipt.result) - : failure('operation_conflict', `${options.verb} identity has a different payload`); - } - return options.execute(); - }); - } - - async #readCompletedQueuedMutation< - I extends { readonly originHostEpoch: string; readonly sessionId: string }, - R, - >( - options: QueuedMutationOptions, - ): Promise<{ readonly payloadIdentity: object; readonly result: R } | undefined> { - const receipt = this.#completedOperations.get( - queuedMutationKey(options.operationKind, options.input.sessionId, options.operationId), - ); - if (!receipt) return undefined; - try { - return { - payloadIdentity: receipt.payloadIdentity, - result: options.spec.decodeOutput(receipt.result), - }; - } catch (error) { - throw new RuntimeMessageAuthorityInvariantError( - `Invalid queued mutation replay outcome: ${ - error instanceof Error ? error.message : 'malformed' - }`, - ); - } - } - - #deletePendingQueuedMutation(key: string, result: Promise>): void { - if (this.#pendingQueuedMutations.get(key)?.result === result) { - this.#pendingQueuedMutations.delete(key); - } - } - async #retractQueuedEntryAdmitted( input: QueueEntryRetractInput, ): Promise> { - const header = await this.#root.readSessionHeader(input.sessionId); - if (this.#failStopped) { - return failure('host_draining', 'Runtime Host message authority has failed'); - } - if (!header) return failure('not_found', 'Session does not exist'); - if (header.isArchived) return failure('session_archived', 'Session is archived'); - const state = this.#state(input.sessionId); - if (state.transition) { - return failure('operation_conflict', 'Message queue is draining into the next Turn'); - } - const queued = findQueuedEntry(state, input.entryId); + const admitted = await this.#openQueueMutation(input.sessionId); + if (!admitted.ok) return admitted; + const state = admitted.result; + const queued = locateQueuedEntry(state, input.entryId); if (!queued) { - if ([...state.inFlight.values()].some((entry) => entry.entryId === input.entryId)) { + if (hasInFlightEntry(state, input.entryId)) { return failure('operation_conflict', 'Message entry is already being delivered'); } return failure('not_found', 'Message queue entry does not exist'); } await this.#admissions.cancelMessageAdmissions(input.sessionId, [queued.entry.messageId]); - queued.remove(); - this.#releaseEntry(queued.entry); + this.#releaseEntry(removeQueuedEntry(state, queued)); this.#mutated(state); this.#maybeReclaim(input.sessionId, state); const result = { queueRevision: state.revision }; @@ -1820,12 +1724,9 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { async #promoteQueuedEntryAdmitted( input: QueueEntryPromoteInput, ): Promise> { - const header = await this.#root.readSessionHeader(input.sessionId); - if (this.#failStopped) { - return failure('host_draining', 'Runtime Host message authority has failed'); - } - if (!header) return failure('not_found', 'Session does not exist'); - if (header.isArchived) return failure('session_archived', 'Session is archived'); + const admitted = await this.#openQueueMutation(input.sessionId); + if (!admitted.ok) return admitted; + const state = admitted.result; const rootState = await this.#root.readRootState(input.sessionId); if (this.#failStopped) { return failure('host_draining', 'Runtime Host message authority has failed'); @@ -1833,29 +1734,25 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { if (rootState.kind !== 'active') { return failure('operation_conflict', 'No active Turn can accept steering'); } - const state = this.#state(input.sessionId); if (state.phase !== 'open') { return failure('session_busy', 'Message admission is closed for the active generation'); } - if (state.transition) { - return failure('operation_conflict', 'Message queue is draining into the next Turn'); - } if (!state.reservedRoot || !sameRun(state.reservedRoot, rootState)) { throw new RuntimeMessageAuthorityInvariantError( 'Root state does not match message reservation', ); } - const index = state.followup.findIndex((entry) => entry.entryId === input.entryId); - const entry = index === -1 ? undefined : state.followup[index]; - if (!entry) { - if (state.steering.some((queued) => queued.entryId === input.entryId)) { + const queued = locateQueuedEntry(state, input.entryId); + if (!queued || queued.lane !== 'followup') { + if (queued?.lane === 'steering') { return failure('operation_conflict', 'Message entry already steers the active Turn'); } - if ([...state.inFlight.values()].some((queued) => queued.entryId === input.entryId)) { + if (hasInFlightEntry(state, input.entryId)) { return failure('operation_conflict', 'Message entry is already being delivered'); } return failure('not_found', 'Message queue entry does not exist'); } + const entry = queued.entry; const promotedSource = { ...sourceFromEntry(entry), placement: 'current_turn', @@ -1891,8 +1788,11 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { skillInvocation: entry.skillInvocation, admittedAt: entry.admittedAt, }); - state.followup.splice(index, 1); - state.steering.push({ ...entry, placement: 'current_turn', disposition: 'steering' }); + commitFollowupPromotion(state, queued, { + ...entry, + placement: 'current_turn', + disposition: 'steering', + }); this.#mutated(state); const result = { queueRevision: state.revision }; this.#rememberCompletedOperation('promote', input.sessionId, input.promoteId, input, result); @@ -1902,19 +1802,12 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { async #updateQueuedEntryAdmitted( input: QueueEntryUpdateInput, ): Promise> { - const header = await this.#root.readSessionHeader(input.sessionId); - if (this.#failStopped) { - return failure('host_draining', 'Runtime Host message authority has failed'); - } - if (!header) return failure('not_found', 'Session does not exist'); - if (header.isArchived) return failure('session_archived', 'Session is archived'); - const state = this.#state(input.sessionId); - if (state.transition) { - return failure('operation_conflict', 'Message queue is draining into the next Turn'); - } - const queued = findQueuedEntry(state, input.entryId); + const admitted = await this.#openQueueMutation(input.sessionId); + if (!admitted.ok) return admitted; + const state = admitted.result; + const queued = locateQueuedEntry(state, input.entryId); if (!queued) { - if ([...state.inFlight.values()].some((entry) => entry.entryId === input.entryId)) { + if (hasInFlightEntry(state, input.entryId)) { return failure('operation_conflict', 'Message entry is already being delivered'); } return failure('not_found', 'Message queue entry does not exist'); @@ -1980,7 +1873,7 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { } if ( state.revision !== currentRevision || - findQueuedEntry(state, input.entryId)?.entry !== queued.entry + locateQueuedEntry(state, input.entryId)?.entry !== queued.entry ) { return failure('session_busy', 'Message queue changed during update'); } @@ -2020,22 +1913,13 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { async #reorderQueuedEntriesAdmitted( input: QueueEntriesReorderInput, ): Promise> { - const header = await this.#root.readSessionHeader(input.sessionId); - if (this.#failStopped) { - return failure('host_draining', 'Runtime Host message authority has failed'); - } - if (!header) return failure('not_found', 'Session does not exist'); - if (header.isArchived) return failure('session_archived', 'Session is archived'); - const state = this.#state(input.sessionId); - if (state.transition) { - return failure('operation_conflict', 'Message queue is draining into the next Turn'); - } + const admitted = await this.#openQueueMutation(input.sessionId); + if (!admitted.ok) return admitted; + const state = admitted.result; if (state.revision !== input.expectedQueueRevision) { return failure('operation_conflict', 'Message queue changed since the reorder was issued'); } - const steering = state.steering.some((entry) => entry.entryId === input.entryIds[0]); - const current = steering ? state.steering : state.followup; - const reorder = exactQueueReorder(current, input.entryIds); + const reorder = planQueueReorder(state, input.entryIds); if (!reorder) { return failure('operation_conflict', 'Message queue changed since the reorder was issued'); } @@ -2043,10 +1927,9 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { await this.#admissions.reorderMessageAdmissions( input.sessionId, reorder.entries.map((entry) => entry.messageId), - steering ? 'steering' : 'followup', + reorder.lane, ); - if (steering) state.steering = [...reorder.entries]; - else state.followup = [...reorder.entries]; + commitQueueReorder(state, reorder.lane, reorder.entries); this.#mutated(state); } const result = { queueRevision: state.revision }; @@ -2054,6 +1937,23 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { return success(result); } + async #openQueueMutation( + sessionId: string, + allowTransition = false, + ): Promise> { + const header = await this.#root.readSessionHeader(sessionId); + if (this.#failStopped) { + return failure('host_draining', 'Runtime Host message authority has failed'); + } + if (!header) return failure('not_found', 'Session does not exist'); + if (header.isArchived) return failure('session_archived', 'Session is archived'); + const state = this.#state(sessionId); + if (!allowTransition && state.transition) { + return failure('operation_conflict', 'Message queue is draining into the next Turn'); + } + return success(state); + } + private async interrupt(input: TurnInterruptInput): Promise> { if (input.originHostEpoch !== this.#hostEpoch) { return failure('outcome_unknown', 'Interrupt outcome is not durable across Host Epochs'); @@ -2369,9 +2269,7 @@ export class HostMessageCoordinator implements RuntimeMessageAuthority { ({ payload }) => payload.sessionId === run.sessionId && payload.placement === 'current_turn', ), - ...[...this.#pendingQueuedMutations.values()].filter( - ({ payload }) => payload.sessionId === run.sessionId, - ), + ...this.#queueMutations.pendingResults(run.sessionId).map((result) => ({ result })), ]; if (pending.length === 0) break; await Promise.all(pending.map(({ result }) => result)); @@ -2626,19 +2524,6 @@ function queuedMutationKey( return `${kind}\0${sessionId}\0${operationId}`; } -function findQueuedEntry( - state: SessionState, - entryId: string, -): { readonly entry: LiveEntry; remove(): void } | undefined { - for (const queue of [state.steering, state.followup]) { - const index = queue.findIndex((entry) => entry.entryId === entryId); - const entry = index === -1 ? undefined : queue[index]; - if (!entry) continue; - return { entry, remove: () => queue.splice(index, 1) }; - } - return undefined; -} - function relocateInlineReferences( references: MessageContent['inlineReferences'], text: string, diff --git a/packages/runtime-host/src/server/message-queue-state.ts b/packages/runtime-host/src/server/message-queue-state.ts new file mode 100644 index 00000000000..6d1b77d4f64 --- /dev/null +++ b/packages/runtime-host/src/server/message-queue-state.ts @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { exactQueueReorder } from '@maka/core/message-queue-order'; + +interface QueueEntryIdentity { + readonly entryId: string; + readonly messageId: string; +} + +interface QueueCollections { + steering: Entry[]; + followup: Entry[]; + readonly inFlight: ReadonlyMap; +} + +export interface QueuedEntryLocation { + readonly lane: 'steering' | 'followup'; + readonly index: number; + readonly entry: Entry; +} + +export function locateQueuedEntry( + state: QueueCollections, + entryId: string, +): QueuedEntryLocation | undefined { + for (const lane of ['steering', 'followup'] as const) { + const index = state[lane].findIndex((entry) => entry.entryId === entryId); + const entry = index < 0 ? undefined : state[lane][index]; + if (entry) return { lane, index, entry }; + } + return undefined; +} + +export function hasInFlightEntry( + state: QueueCollections, + entryId: string, +): boolean { + return [...state.inFlight.values()].some((entry) => entry.entryId === entryId); +} + +export function removeQueuedEntry( + state: QueueCollections, + location: QueuedEntryLocation, +): Entry { + const [removed] = state[location.lane].splice(location.index, 1); + if (removed !== location.entry) throw new Error('Queued entry location changed before commit'); + return removed; +} + +export function commitFollowupPromotion( + state: QueueCollections, + location: QueuedEntryLocation, + promoted: Entry, +): void { + if (location.lane !== 'followup') throw new Error('Only follow-up entries can be promoted'); + removeQueuedEntry(state, location); + state.steering.push(promoted); +} + +export function planQueueReorder( + state: QueueCollections, + entryIds: readonly string[], +): + | { + readonly lane: 'steering' | 'followup'; + readonly entries: readonly Entry[]; + readonly changed: boolean; + } + | undefined { + const lane = state.steering.some((entry) => entry.entryId === entryIds[0]) + ? 'steering' + : 'followup'; + const reorder = exactQueueReorder(state[lane], entryIds); + return reorder ? { lane, entries: reorder.entries, changed: reorder.changed } : undefined; +} + +export function commitQueueReorder( + state: QueueCollections, + lane: 'steering' | 'followup', + entries: readonly Entry[], +): void { + state[lane] = [...entries]; +} diff --git a/packages/runtime-host/src/server/queued-mutation-executor.ts b/packages/runtime-host/src/server/queued-mutation-executor.ts new file mode 100644 index 00000000000..6bce1eca6a6 --- /dev/null +++ b/packages/runtime-host/src/server/queued-mutation-executor.ts @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { isDeepStrictEqual } from 'node:util'; +import { RuntimeMessageAuthorityInvariantError } from '@maka/runtime/message-authority'; +import type { HostOperationErrorCode, OperationSpec } from '../protocol/operation-spec.js'; +import type { SessionAdmissionGate } from './session-admission-gate.js'; + +export type QueuedMutationKind = + | 'retract' + | 'retract_entry' + | 'promote' + | 'update_entry' + | 'reorder'; + +type MutationInput = { + readonly originHostEpoch: string; + readonly sessionId: string; +}; + +type QueuedMutationErrorCode = + | 'host_draining' + | 'operation_unavailable' + | 'not_found' + | 'session_archived' + | 'session_busy' + | 'operation_conflict' + | 'outcome_unknown'; + +type MutationOutcome = + | { readonly ok: true; readonly result: T } + | { + readonly ok: false; + readonly error: { readonly code: QueuedMutationErrorCode; readonly message: string }; + }; + +export interface CompletedQueuedMutation { + readonly payloadIdentity: object; + readonly result: object; +} + +export interface QueuedMutationRequest { + readonly spec: OperationSpec; + readonly kind: QueuedMutationKind; + readonly id: string; + readonly verb: string; + readonly input: I; + readonly payloadIdentity: object; + readonly execute: () => Promise>; +} + +interface PendingMutation { + readonly payload: MutationInput; + readonly result: Promise>; +} + +interface QueuedMutationExecutorOptions { + readonly hostEpoch: string; + readonly admissions: SessionAdmissionGate; + readonly isFailStopped: () => boolean; + readonly readCompleted: ( + kind: QueuedMutationKind, + sessionId: string, + operationId: string, + ) => CompletedQueuedMutation | undefined; +} + +/** Epoch-local idempotency and Session admission for all queue mutations. */ +export class QueuedMutationExecutor { + readonly #hostEpoch: string; + readonly #admissions: SessionAdmissionGate; + readonly #isFailStopped: () => boolean; + readonly #readCompleted: QueuedMutationExecutorOptions['readCompleted']; + readonly #pending = new Map(); + + constructor(options: QueuedMutationExecutorOptions) { + this.#hostEpoch = options.hostEpoch; + this.#admissions = options.admissions; + this.#isFailStopped = options.isFailStopped; + this.#readCompleted = options.readCompleted; + } + + run( + request: QueuedMutationRequest, + ): Promise> { + const currentEpoch = request.input.originHostEpoch === this.#hostEpoch; + const key = mutationKey(request.kind, request.input.sessionId, request.id); + if (currentEpoch) { + const pending = this.#pending.get(key); + if (pending) { + return isDeepStrictEqual(pending.payload, request.input) + ? (pending.result as Promise>) + : Promise.resolve(conflict(`${request.verb} identity has a different payload`)); + } + } + if (this.#isFailStopped()) return Promise.resolve(draining()); + if (!currentEpoch) { + return Promise.resolve( + failed('outcome_unknown', `${request.verb} outcome is not durable across Host Epochs`), + ); + } + + const result = this.#admit(request); + this.#pending.set(key, { payload: request.input, result }); + void result.then( + () => this.#settle(key, result), + () => this.#settle(key, result), + ); + return result; + } + + pendingResults(sessionId: string): readonly Promise>[] { + return [...this.#pending.values()] + .filter(({ payload }) => payload.sessionId === sessionId) + .map(({ result }) => result); + } + + #admit( + request: QueuedMutationRequest, + ): Promise> { + return this.#admissions.run(request.input.sessionId, async () => { + if (this.#isFailStopped()) return draining(); + const completed = this.#readCompleted(request.kind, request.input.sessionId, request.id); + if (!completed) return request.execute(); + if (!isDeepStrictEqual(completed.payloadIdentity, request.payloadIdentity)) { + return conflict(`${request.verb} identity has a different payload`); + } + try { + return { ok: true, result: request.spec.decodeOutput(completed.result) }; + } catch (error) { + throw new RuntimeMessageAuthorityInvariantError( + `Invalid queued mutation replay outcome: ${ + error instanceof Error ? error.message : 'malformed' + }`, + ); + } + }); + } + + #settle(key: string, result: Promise>): void { + if (this.#pending.get(key)?.result === result) this.#pending.delete(key); + } +} + +function mutationKey(kind: QueuedMutationKind, sessionId: string, operationId: string): string { + return `${kind}\0${sessionId}\0${operationId}`; +} + +function draining(): MutationOutcome { + return failed('host_draining', 'Runtime Host message authority has failed'); +} + +function conflict(message: string): MutationOutcome { + return failed('operation_conflict', message); +} + +function failed(code: QueuedMutationErrorCode, message: string): MutationOutcome { + return { ok: false, error: { code, message } }; +} diff --git a/packages/ui/src/__tests__/composer-message-queue.test.tsx b/packages/ui/src/__tests__/composer-message-queue.test.tsx index 9b0986888f7..32b3298336a 100644 --- a/packages/ui/src/__tests__/composer-message-queue.test.tsx +++ b/packages/ui/src/__tests__/composer-message-queue.test.tsx @@ -212,3 +212,63 @@ test('a drag from an older queue revision cannot reorder a newer projection', as await view.close(); } }); + +test('promote and delete dispatch only for Host-owned queued entries', async () => { + const promoted: string[] = []; + const deleted: string[] = []; + const view = await mountQueue({ + queuedMessages: [ + queued('entry-1', 'first'), + { ...queued('entry-2', 'second'), state: 'in_flight' }, + ], + queueRevision: 4, + onPromoteEntry: (entryId) => { promoted.push(entryId); }, + onDeleteEntry: (entryId) => { deleted.push(entryId); }, + }); + try { + assert.equal(actionButton(view.document, copy.promoteQueuedEntry, 1).disabled, true); + assert.equal( + actionButton(view.document, copy.deleteQueuedEntry, 1).getAttribute('aria-disabled'), + 'true', + ); + await click(actionButton(view.document, copy.promoteQueuedEntry, 0)); + await click(actionButton(view.document, copy.deleteQueuedEntry, 0)); + assert.deepEqual(promoted, ['entry-1']); + assert.deepEqual(deleted, ['entry-1']); + } finally { + await view.close(); + } +}); + +test('one render cannot dispatch two mutations for the same pending queue', async () => { + let settlePromotion!: () => void; + const promotion = new Promise((resolve) => { + settlePromotion = resolve; + }); + const calls: string[] = []; + const view = await mountQueue({ + queuedMessages: [queued('entry-1', 'first')], + queueRevision: 4, + onPromoteEntry: () => { + calls.push('promote'); + return promotion; + }, + onDeleteEntry: () => { + calls.push('delete'); + }, + }); + try { + const promote = actionButton(view.document, copy.promoteQueuedEntry); + const remove = actionButton(view.document, copy.deleteQueuedEntry); + await act(async () => { + promote.dispatchEvent(new window.Event('click', { bubbles: true })); + remove.dispatchEvent(new window.Event('click', { bubbles: true })); + }); + assert.deepEqual(calls, ['promote']); + settlePromotion(); + await act(async () => promotion); + } finally { + settlePromotion(); + await view.close(); + } +}); diff --git a/packages/ui/src/__tests__/composer-send-toggle.test.tsx b/packages/ui/src/__tests__/composer-send-toggle.test.tsx index 0e3047a4145..9e65d4794d0 100644 --- a/packages/ui/src/__tests__/composer-send-toggle.test.tsx +++ b/packages/ui/src/__tests__/composer-send-toggle.test.tsx @@ -17,24 +17,16 @@ * under the License. */ -/** - * The composer's send slot holds ONE control (Astryx's send/stop toggle), and - * mid-turn it reads Stop while the draft is empty. This is the shape the slot - * drifted out of more than once — a Stop button and a Steer button side by - * side, then a Queue/Steer mode switch beside Send — so the count is asserted, - * not just the label. Queue affordances live in the pending plate above the - * card, never in the send slot. - */ +/** The send slot is a single Send/Stop control; queue actions live above it. */ import assert from 'node:assert/strict'; import test from 'node:test'; -import { act, createRef } from 'react'; +import { act } from 'react'; import { createRoot } from 'react-dom/client'; import { renderToStaticMarkup } from 'react-dom/server'; import { parseHTML } from 'linkedom'; -import { Composer, type ComposerHandle } from '../composer.js'; +import { Composer } from '../composer.js'; import { LocaleProvider } from '../locale-context.js'; -import { ICON_SIZE, Search } from '../icons.js'; function renderComposer(streaming: boolean): string { return renderToStaticMarkup( @@ -45,7 +37,10 @@ function renderComposer(streaming: boolean): string { } function sendSlotControls(markup: string): string[] { - return markup.match(/aria-label="(?:Send|Stop)"/g) ?? []; + const document = parseHTML(`${markup}`).document; + return [...document.querySelectorAll('button[aria-label="Send"], button[aria-label="Stop"]')] + .map((button) => button.getAttribute('aria-label')) + .filter((label): label is string => label !== null); } /** The Send control's own `aria-disabled` value — asserted directly, not by a @@ -59,7 +54,7 @@ function sendButtonAriaDisabled(markup: string): string | null { test('an idle composer offers Send alone', () => { const controls = sendSlotControls(renderComposer(false)); - assert.deepEqual(controls, ['aria-label="Send"']); + assert.deepEqual(controls, ['Send']); }); test('a host-owned send gate disables Send without an inline notice', () => { @@ -79,12 +74,12 @@ test('a host-owned send gate disables Send without an inline notice', () => { test('a turn in flight turns the same single control into Stop', () => { const controls = sendSlotControls(renderComposer(true)); - assert.deepEqual(controls, ['aria-label="Stop"']); + assert.deepEqual(controls, ['Stop']); }); -test('a running composer keeps Send alone — no mode switch in the send slot', () => { +test('a running composer adds no queue mode switch beside Stop', () => { const markup = renderComposer(true); - assert.deepEqual(sendSlotControls(markup), ['aria-label="Stop"']); + assert.deepEqual(sendSlotControls(markup), ['Stop']); assert.doesNotMatch(markup, /Follow-up behavior/); assert.doesNotMatch(markup, /SegmentedControl/); }); @@ -132,7 +127,7 @@ test('a staged quote enables Send without any host opt-in (#4804)', () => { ); // The toggle and the disabled state agree: a quote-only draft is a live // Send, and it needs no per-host decision the way attachments do. - assert.deepEqual(sendSlotControls(staged), ['aria-label="Send"']); + assert.deepEqual(sendSlotControls(staged), ['Send']); assert.equal(sendButtonAriaDisabled(staged), null); // The same empty draft with nothing staged is what disabled looks like, so // the assertion above pins the staged quote as the enabling reason. @@ -173,7 +168,7 @@ test('the three send gates agree about a staged quote while streaming (#4804)', )); // Gate 1 — the send/stop toggle: mid-turn the slot stays on Send because // the staged quote is handable content, not an empty draft. - assert.deepEqual(sendSlotControls(container.innerHTML), ['aria-label="Send"']); + assert.deepEqual(sendSlotControls(container.innerHTML), ['Send']); const button = container.querySelector('button[aria-label="Send"]'); assert.ok(button); // Gate 2 — sendDisabled: the control is live. @@ -231,181 +226,26 @@ test('the actual submit waits for Session references and keeps the draft on refu } }); -test('a send completing after navigation keeps the newer draft it left behind', async () => { - const original = { document: globalThis.document, window: globalThis.window, Node: globalThis.Node, HTMLElement: globalThis.HTMLElement, - IS_REACT_ACT_ENVIRONMENT: (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT }; - const { document, window } = parseHTML('
'); - window.getComputedStyle = () => ({ direction: 'ltr', writingMode: 'horizontal-tb', getPropertyValue: () => '' }) as unknown as CSSStyleDeclaration; - Object.assign(window, { getSelection: () => null }); - Object.assign(document, { getSelection: () => null }); - Object.assign(globalThis, { document, window, Node: window.Node, HTMLElement: window.HTMLElement, IS_REACT_ACT_ENVIRONMENT: true }); - const root = createRoot(document.querySelector('#root')!); - const handle = createRef(); - let finish!: (sent: boolean) => void; - const render = (draftKey: string) => root.render( - - new Promise((resolve) => { finish = resolve; })} - onStop={() => undefined} /> - , - ); - try { - await act(() => render('a')); - await act(() => handle.current!.setText('first request')); - await act(async () => { - document.querySelector('form')!.dispatchEvent(new window.Event('submit', { bubbles: true, cancelable: true })); - await Promise.resolve(); - }); - await act(() => handle.current!.setText('second unsent draft')); - await act(() => render('b')); - await act(async () => { finish(true); await Promise.resolve(); }); - assert.equal(handle.current!.getDraft('a'), 'second unsent draft'); - } finally { - await act(() => root.unmount()); - Object.assign(globalThis, original); - } -}); - -test('keeps Host order visible until the reordered projection arrives', async () => { - const original = { - document: globalThis.document, - window: globalThis.window, - IS_REACT_ACT_ENVIRONMENT: (globalThis as typeof globalThis & { - IS_REACT_ACT_ENVIRONMENT?: boolean; - }).IS_REACT_ACT_ENVIRONMENT, - }; - const { document, window } = parseHTML('
'); - window.getComputedStyle = () => ({ - direction: 'ltr', - writingMode: 'horizontal-tb', - getPropertyValue: () => '', - }) as unknown as CSSStyleDeclaration; - Object.assign(globalThis, { document, window, IS_REACT_ACT_ENVIRONMENT: true }); - const container = document.querySelector('#root'); - assert.ok(container); - const root = createRoot(container); - let requestedOrder: readonly string[] | undefined; - const editedEntryIds: string[] = []; - const deletedEntryIds: string[] = []; - - try { - await act(() => root.render( - - ({ - entryId, - messageId: `message-${entryId}`, - content: { text: entryId }, - placement: 'next_turn' as const, - state: 'queued' as const, - })), - ]} - onPromoteQueuedEntry={() => undefined} - onEditQueuedEntry={(entry) => { - editedEntryIds.push(entry.entryId); - }} - onDeleteQueuedEntry={(entryId) => { - deletedEntryIds.push(entryId); - }} - onReorderQueuedEntries={(entryIds) => { - requestedOrder = entryIds; - return new Promise(() => undefined); - }} - onSend={() => undefined} - onStop={() => undefined} - /> - , - )); - const editButtons = [ - ...container.querySelectorAll('[aria-label="Edit"]'), - ]; - const deleteButtons = [ - ...container.querySelectorAll('[aria-label="Delete"]'), - ]; - // Queued steering lives in the transcript, not the follow-up plate. - assert.equal(container.querySelectorAll('[aria-label="Send now"]').length, 2); - assert.equal(editButtons.length, 2); - assert.equal(deleteButtons.length, 2); - assert.equal( - [...container.querySelectorAll('.maka-composer-queue-text')] - .every((row) => !row.textContent?.includes('steering')), - true, - ); - await act(async () => { - editButtons[0]?.dispatchEvent(new window.Event('click', { bubbles: true })); - await Promise.resolve(); - }); - await act(async () => { - container - .querySelector('[aria-label="Delete"]') - ?.dispatchEvent(new window.Event('click', { bubbles: true })); - await Promise.resolve(); - }); - assert.deepEqual(editedEntryIds, ['first']); - assert.deepEqual(deletedEntryIds, ['first']); - const grips = [...container.querySelectorAll('.maka-composer-queue-grip')]; - assert.equal(grips.length, 2); - const dragStart = new window.Event('dragstart', { bubbles: true }); - Object.defineProperty(dragStart, 'dataTransfer', { - value: { effectAllowed: '', setData() {} }, - }); - await act(() => grips[1]?.dispatchEvent(dragStart)); - const firstRow = grips[0]?.closest('li')?.parentElement; - assert.ok(firstRow); - const drop = new window.Event('drop', { bubbles: true }); - Object.defineProperty(drop, 'dataTransfer', { value: { types: [], files: [] } }); - await act(() => firstRow.dispatchEvent(drop)); - - assert.deepEqual(requestedOrder, ['second', 'first']); - assert.deepEqual( - [...container.querySelectorAll('.maka-composer-queue-text')].map((row) => row.textContent), - ['first', 'second'], - ); - } finally { - await act(() => root.unmount()); - Object.assign(globalThis, original); +test('deduplicates pending steering against Host queue entries and keeps the plate through an empty queue snapshot', () => { + const pending = { id: 'steer', text: 'new direction', ts: 1, transientPlacement: 'steering' as const }; + const queued = { entryId: 'host-entry', messageId: pending.id, placement: 'current_turn' as const, state: 'queued' as const, content: { text: pending.text } }; + for (const entries of [[queued], []]) { + const markup = renderToStaticMarkup( undefined} onStop={() => undefined} + queuedMessages={entries} pendingMessages={[pending]} />); + const document = parseHTML(`${markup}`).document; + assert.equal(document.querySelectorAll('.maka-composer-queue-text').length, 1); + assert.equal(document.querySelector('.maka-composer-queue-text')?.textContent, pending.text); + assert.equal(document.querySelector('.maka-composer-queue-status')?.textContent, 'Steering · Applied together'); } }); -test('local sends share the flat staging list with icon-only delivery actions', () => { +test('a locally saved follow-up keeps its delivery status and recovery actions in the pending list', () => { const markup = renderToStaticMarkup( undefined} onStop={() => undefined} - queuedMessages={[{ - entryId: 'admitted', messageId: 'message-admitted', - content: { text: 'admitted follow-up' }, placement: 'next_turn', state: 'queued', - }]} - pendingMessages={[ - { id: 'local', text: 'offline follow-up', ts: 1, transientPlacement: 'follow_up', - deliveryStatus: 'Delivery uncertain', deliveryDetail: 'Connection interrupted', - deliveryActions: [{ label: 'Check delivery', icon: ); + pendingMessages={[{ id: 'local', text: 'offline follow-up', ts: 1, transientPlacement: 'follow_up', + deliveryStatus: 'Delivery uncertain', deliveryDetail: 'Connection interrupted', + deliveryActions: [{ label: 'Check delivery', onClick() {} }] }]} />); const document = parseHTML(`${markup}`).document; - assert.deepEqual( - [...document.querySelectorAll('.maka-composer-queue-list li')].map((row) => row.textContent), - ['admitted follow-up', 'offline follow-up', 'still sending'], - 'admitted entries and in-flight sends share one flat list', - ); - assert.ok(document.querySelector('.maka-composer-queue-actions button[aria-label="Check delivery"]'), - 'the delivery action stays an accessible labelled icon control'); - const gripRows = [...document.querySelectorAll('li')] - .filter((row) => row.querySelector('.maka-composer-queue-grip') !== null); - assert.equal(gripRows.length, 1, 'only the admitted row carries a drag grip'); - const sendingRow = [...document.querySelectorAll('li')] - .find((row) => row.textContent?.includes('still sending')); - assert.ok(sendingRow); - assert.equal(sendingRow.querySelectorAll('.maka-composer-queue-actions button').length, 0, - 'a local row without delivery actions offers no Host edit/steer/delete operations'); + assert.equal(document.querySelector('.maka-composer-queue-delivery')?.textContent, 'Delivery uncertain'); + assert.ok([...document.querySelectorAll('.maka-composer-queue-actions button')].some((button) => button.textContent === 'Check delivery')); }); diff --git a/packages/ui/src/composer-message-queue-controller.ts b/packages/ui/src/composer-message-queue-controller.ts new file mode 100644 index 00000000000..d7a1525eea9 --- /dev/null +++ b/packages/ui/src/composer-message-queue-controller.ts @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useRef, useState } from "react"; +import type { MessageQueueEntryProjection } from "@maka/core/events"; +import { moveQueueEntryId } from "@maka/core/message-queue-order"; +import type { TransientUserMessageProjection } from "./chat-view.js"; +import { useMountedRef } from "./use-mounted-ref.js"; + +export type ComposerQueueEntry = Omit & { + state: MessageQueueEntryProjection["state"] | "local"; + localMessage?: TransientUserMessageProjection; +}; + +export interface ComposerQueueActions { + queueRevision?: number; + onUpdateEntry?(entryId: string, expectedQueueRevision: number, text: string): void | Promise; + onReorderEntries?(entryIds: readonly string[], expectedQueueRevision: number): void | Promise; +} + +export function useComposerMessageQueueController( + entries: readonly ComposerQueueEntry[], + actions: ComposerQueueActions, +) { + const [pendingEntryId, setPendingEntryId] = useState(null); + const [editing, setEditing] = useState<{ + entryId: string; + queueRevision: number; + text: string; + } | null>(null); + const pending = useRef(null); + const drag = useRef<{ entryId: string; queueRevision: number } | null>(null); + const mountedRef = useMountedRef(); + + const groups = (["current_turn", "next_turn"] as const) + .map((placement) => ({ + placement, + entries: entries.filter((entry) => entry.placement === placement), + })) + .filter((group) => group.entries.length > 0); + + async function runAction( + entryId: string, + action: (() => void | Promise) | undefined, + ): Promise { + if (!action || pending.current) return false; + pending.current = entryId; + setPendingEntryId(entryId); + try { + await action(); + return true; + } catch { + return false; + } finally { + if (pending.current === entryId) pending.current = null; + if (mountedRef.current) setPendingEntryId(null); + } + } + + function beginEdit(entry: ComposerQueueEntry): void { + if (pendingEntryId || !actions.onUpdateEntry || actions.queueRevision === undefined) return; + setEditing({ + entryId: entry.entryId, + queueRevision: actions.queueRevision, + text: entry.content.displayText ?? entry.content.text, + }); + } + + function cancelEdit(): void { + setEditing(null); + } + + async function commitEdit(): Promise { + if (!editing) return; + const text = editing.text.trim(); + if (!text) return; + const updated = await runAction(editing.entryId, () => + actions.onUpdateEntry?.(editing.entryId, editing.queueRevision, text), + ); + if (updated && mountedRef.current) setEditing(null); + } + + function canReorder(entry: ComposerQueueEntry): boolean { + return ( + entry.state === "queued" && + editing?.entryId !== entry.entryId && + Boolean(actions.onReorderEntries) && + actions.queueRevision !== undefined && + pendingEntryId === null + ); + } + + function beginDrag(entryId: string): number | undefined { + if (actions.queueRevision === undefined) return undefined; + drag.current = { entryId, queueRevision: actions.queueRevision }; + return actions.queueRevision; + } + + function endDrag(): void { + drag.current = null; + } + + function hasDrag(): boolean { + return drag.current !== null; + } + + function dropOn(targetEntryId: string): void { + const source = drag.current; + drag.current = null; + if ( + !source || + source.entryId === targetEntryId || + !actions.onReorderEntries || + actions.queueRevision !== source.queueRevision + ) { + return; + } + const target = entries.find((entry) => entry.entryId === targetEntryId); + const sourceEntry = entries.find((entry) => entry.entryId === source.entryId); + if (!target || sourceEntry?.placement !== target.placement) return; + const entryIds = entries + .filter((entry) => entry.placement === target.placement && entry.state === "queued") + .map((entry) => entry.entryId); + const reordered = moveQueueEntryId(entryIds, source.entryId, targetEntryId); + if (!reordered) return; + void runAction(source.entryId, () => + actions.onReorderEntries?.(reordered, source.queueRevision), + ); + } + + return { + beginDrag, + beginEdit, + cancelEdit, + canReorder, + commitEdit, + dropOn, + editing, + endDrag, + groups, + hasDrag, + pendingEntryId, + runAction, + setEditingText(text: string) { + setEditing((current) => current ? { ...current, text } : null); + }, + }; +} diff --git a/packages/ui/src/composer-message-queue.tsx b/packages/ui/src/composer-message-queue.tsx index 88450e5b47c..485bdfeb9ce 100644 --- a/packages/ui/src/composer-message-queue.tsx +++ b/packages/ui/src/composer-message-queue.tsx @@ -17,107 +17,59 @@ * under the License. */ -import { memo, useRef, useState } from 'react'; +import { memo } from 'react'; import type { TransientUserMessageProjection } from './chat-view.js'; import type { MessageQueueEntryProjection } from '@maka/core/events'; -import { moveQueueEntryId } from '@maka/core/message-queue-order'; -import { IconButton } from '@astryxdesign/core'; +import { Button, IconButton, Tooltip } from '@astryxdesign/core'; import { List, ListItem } from '@astryxdesign/core/List'; import type { ConversationCopy } from './conversation-copy.js'; -import { CornerDownLeft, GripVertical, ICON_SIZE, Pencil, Trash2 } from './icons.js'; -import { useMountedRef } from './use-mounted-ref.js'; +import { Check, GripVertical, HelpCircle, ICON_SIZE, Trash2, X } from './icons.js'; +import { PlatformShortcutText } from './platform-shortcut-text.js'; +import { + type ComposerQueueEntry, + useComposerMessageQueueController, +} from './composer-message-queue-controller.js'; -type ComposerQueueEntry = Omit & { - state: MessageQueueEntryProjection['state'] | 'local'; - localMessage?: TransientUserMessageProjection; -}; +export type { ComposerQueueEntry } from './composer-message-queue-controller.js'; /** - * The queued follow-up section of the composer staging drawer. It lists - * follow-up entries — Host-queued and still in flight — so a queued message - * stays promotable, reorderable and deletable until a Turn consumes it; edit - * hands it back to the composer. - * Steering targets the active Turn and lives in the transcript instead, where - * its delivery state is message metadata rather than a queue row. + * The pending plate above the composer card. It lists both pending steering + * and follow-up entries so a submitted message stays editable, reorderable and + * deletable while it waits for the active Turn to reach a steering boundary. + * Steering enters the transcript only when Runtime actually consumes it. */ export interface ComposerMessageQueueProps { queuedMessages: readonly ComposerQueueEntry[]; queueRevision?: number; copy: ConversationCopy['composer']; onPromoteEntry?(entryId: string): void | Promise; - onEditEntry?(entry: Pick): void | Promise; + onUpdateEntry?(entryId: string, expectedQueueRevision: number, text: string): void | Promise; onDeleteEntry?(entryId: string): void | Promise; onReorderEntries?( entryIds: readonly string[], expectedQueueRevision: number, ): void | Promise; } -/** The plate owns next-turn sends only; steering renders in the transcript. */ +/** Host entries own queue actions; local sends remain visible before a receipt. */ export function projectComposerMessageQueue( queued: readonly MessageQueueEntryProjection[], transient: readonly TransientUserMessageProjection[], ): readonly ComposerQueueEntry[] { - const followups = queued.filter((entry) => entry.placement === 'next_turn'); - // Dedup against the whole queue: a Host-admitted steering entry renders as a - // transcript bubble, so its lingering local copy must not stay a plate row. const ids = new Set(queued.map((entry) => entry.messageId)); const pending = transient.filter((message) => message.transientPlacement !== 'transcript' && !ids.has(message.id)); - if (pending.length === 0) return followups; - return [...followups, ...pending.map((message): ComposerQueueEntry => ({ + if (pending.length === 0) return queued; + return [...queued, ...pending.map((message): ComposerQueueEntry => ({ entryId: message.id, messageId: message.id, content: { text: message.text }, - placement: 'next_turn', state: 'local', localMessage: message, + placement: message.transientPlacement === 'steering' ? 'current_turn' : 'next_turn', state: 'local', localMessage: message, }))]; } export const ComposerMessageQueue = memo(function ComposerMessageQueue( props: ComposerMessageQueueProps, ) { - const [pendingEntryId, setPendingEntryId] = useState(null); - const dragSource = useRef<{ entryId: string; queueRevision: number } | null>(null); - const mountedRef = useMountedRef(); const copy = props.copy; - const entries = props.queuedMessages; - - async function runEntryAction( - entryId: string, - action: (() => void | Promise) | undefined, - ): Promise { - if (!action || pendingEntryId) return; - setPendingEntryId(entryId); - try { - // The caller (app shell) surfaces failures itself; the projection is - // unchanged on failure, so there is nothing to settle here. - await action(); - } catch { - // surfaced by the caller - } finally { - if (mountedRef.current) setPendingEntryId(null); - } - } - - function dropOn(targetEntryId: string) { - const sourceDrag = dragSource.current; - dragSource.current = null; - if ( - !sourceDrag - || sourceDrag.entryId === targetEntryId - || !props.onReorderEntries - || props.queueRevision !== sourceDrag.queueRevision - ) return; - const fromId = sourceDrag.entryId; - const target = entries.find((entry) => entry.entryId === targetEntryId); - const source = entries.find((entry) => entry.entryId === fromId); - if (!target || source?.placement !== target.placement) return; - const ids = entries.filter((entry) => entry.placement === target.placement && entry.state === 'queued').map((entry) => entry.entryId); - const reordered = moveQueueEntryId(ids, fromId, targetEntryId); - if (!reordered) return; - // The Host projection is the only rendered order. Keep other queue actions - // pending until this request settles instead of maintaining a local overlay. - void runEntryAction(fromId, () => - props.onReorderEntries?.(reordered, sourceDrag.queueRevision) - ); - } + const controller = useComposerMessageQueueController(entries, props); return (
- - {entries.map((entry) => { - const local = entry.localMessage; - const reorderable = - entry.state === 'queued' - && !local - && Boolean(props.onReorderEntries) - && props.queueRevision !== undefined - && pendingEntryId === null; + {controller.groups.map((group, index) =>
+
+ {group.placement === 'current_turn' ? copy.steeringPending : copy.followupPending} + {index === 0 && }> + } +
+ + {group.entries.map((entry) => { + const editing = controller.editing?.entryId === entry.entryId; + const reorderable = controller.canReorder(entry); return (
{ - if (reorderable && dragSource.current) event.preventDefault(); + if (reorderable && controller.hasDrag()) event.preventDefault(); }} - onDrop={reorderable ? () => dropOn(entry.entryId) : undefined} + onDrop={reorderable ? () => controller.dropOn(entry.entryId) : undefined} > - {entry.content.displayText ?? entry.content.text} - + label={editing ? ( +