Milestone 6: Tauri desktop shell over the headless engine#9
Merged
Conversation
…gine Adds the desktop delivery layer (tasks 25 & 26) without reimplementing any loop logic (Req 13.3). The engine is reused through a thin Node HTTP/SSE server compiled to a single binary and run as a Tauri sidecar. Engine server (src/server/): - createServer wraps runGoal + buildTrace: POST /api/runs, SSE /api/runs/:id/stream (live transitions, attempts, outcomes, runner calls, log lines), GET /api/sessions, GET /api/sessions/:id/trace, /api/health. - RunHub: ordered in-memory pub/sub with replay buffer + Last-Event-ID resume. - tapStoreEvents bridges the store's event stream to live subscribers. - Optional static serving so the UI also runs in a plain browser. - 7 new tests; full suite 132 passing, tsc clean. Sidecar build: - scripts/build-sidecar.mjs compiles src/server/index.ts via `bun --compile` into desktop/src-tauri/binaries/loopwright-engine-<target-triple>. - npm scripts: `serve`, `build:sidecar`. Desktop app (desktop/): - Vite + vanilla TS frontend: Start / Monitor (live SSE) / Results (trace) / Sessions / Secrets views. Resolves the engine URL same-origin in a browser or via the `engine_url` Tauri command in the app. - Tauri v2 shell (src-tauri): spawns the engine sidecar, discovers its port from the readiness line, and stores runner API keys in the OS keychain (keyring), injecting them into the sidecar env so `apiKeyEnv` resolves with no plaintext on disk. Also syncs tasks.md to reality (M2–M5 were complete on main; the prior "current position" note was stale). Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Node HTTP/SSE engine server with RunHub, a Tauri desktop shell that runs a bundled engine sidecar with keychain-backed secrets, a Vite/TypeScript desktop UI with SSE monitoring/control, sidecar build tooling, and Vitest tests covering runtime and streaming behaviors. ChangesDesktop Delivery (Milestone 6)
Sequence Diagram: High-level flowsequenceDiagram
participant User as User
participant Desktop as Desktop UI
participant Tauri as Tauri Shell
participant EngMgr as EngineManager
participant Sidecar as Sidecar Server
participant Hub as RunHub
participant Store as Store
User->>Desktop: Click "Start Run"
Desktop->>Tauri: invoke("engine_url")
Tauri->>EngMgr: url()
EngMgr-->>Desktop: http://127.0.0.1:PORT
Desktop->>Sidecar: POST /api/runs {goal, env}
Sidecar->>Store: persist session/events/transitions/outcomes
Store-->>Hub: tap -> publish events
Desktop->>Sidecar: GET /api/runs/:id/stream
Sidecar->>Hub: subscribe(sessionId, afterId)
Hub-->>Desktop: replay + live SSE events
Desktop->>Desktop: Update monitor view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
desktop/src-tauri/tauri.conf.json (1)
22-22: ⚡ Quick winTighten
connect-srcto the sidecar host actually used.Line 22 allows both
http://127.0.0.1:*andhttp://localhost:*; the sidecar is pinned to127.0.0.1, so droppinglocalhost:*reduces unnecessary local-service exposure.Proposed config change
- "csp": "default-src 'self'; connect-src 'self' http://127.0.0.1:* http://localhost:* ipc: http://ipc.localhost; style-src 'self' 'unsafe-inline'" + "csp": "default-src 'self'; connect-src 'self' http://127.0.0.1:* ipc: http://ipc.localhost; style-src 'self' 'unsafe-inline'"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src-tauri/tauri.conf.json` at line 22, Update the "csp" value to remove the unnecessary "http://localhost:*" entry from the connect-src list so that only the pinned sidecar host (http://127.0.0.1:*) and the existing ipc/http://ipc.localhost entries remain; edit the "csp" property string (the JSON "csp" key currently containing "connect-src 'self' http://127.0.0.1:* http://localhost:* ipc: http://ipc.localhost; ...") and delete the "http://localhost:*" token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.kiro/specs/loop-engine/tasks.md:
- Around line 82-85: Update the stale test count text that currently reads "125
tests" to the correct, current number "132 tests" in the tasks description
(search for the string "125 tests" in the milestone summary and replace it with
"132 tests") so the reported test count matches the PR objective.
In `@desktop/src-tauri/src/engine.rs`:
- Around line 67-77: The readiness detection call to read_listening_url can fail
after spawn(), leaving the spawned sidecar process (child) running; update the
start() flow so that if tauri::async_runtime::block_on(read_listening_url(&mut
rx)) returns an Err you first ensure the spawned child is cleaned up (call
child.kill() and/or child.wait() and handle or log any error) before returning
the Err; specifically modify the code around the variables rx and child so the
Err path for read_listening_url invokes child.kill().map_err(|_| /* optional log
*/).ok(); (or equivalent) and then returns the original error, ensuring no
orphaned sidecar remains.
In `@desktop/src-tauri/src/lib.rs`:
- Around line 26-39: The set_secret and delete_secret tauri commands currently
just call secrets::set/delete (functions set_secret and delete_secret) and
return without triggering restart_engine; change them to invoke the engine
restart path after a successful secrets::set or secrets::delete (e.g., call the
existing restart_engine handler or its internal restart function) so that secret
changes take effect immediately, or alternatively return a clear status
indicating a required restart so the frontend can call restartEngine(); leave
list_secret_keys unchanged. Ensure the restart is only triggered on successful
set/delete and propagate any errors through the Result<String, String>.
In `@desktop/src-tauri/src/secrets.rs`:
- Around line 26-30: The read_index function incorrectly maps any
fs::read_to_string error to an empty Vec, conflating "file not found" with
permission or I/O failures; change read_index (and keep using index_path) to
match on the fs::read_to_string result and only return Ok(Vec::new()) when the
error is a NotFound (std::io::ErrorKind::NotFound), otherwise return Err with a
descriptive error (propagate the ioe.to_string() or wrap it) so permission/IO
errors are surfaced instead of being treated as an empty index.
- Around line 44-50: The set() function in secrets.rs must reject reserved or
invalid secret names so user secrets cannot override internal env vars; add a
validation at the top of secrets::set that (1) rejects any key starting with the
reserved prefix "LOOPWRIGHT_" (return Err with a clear message) and (2) enforces
a safe env-var name format (e.g. only A–Z, 0–9 and underscore, non-empty)
returning Err on violation; if the key passes, proceed to call Entry::new,
read_index and write_index as before. Ensure the error strings match the
function's Result<String> error path and reference the SERVICE constant and the
helper functions read_index/write_index so reviewers can find the change.
In `@desktop/src/api.ts`:
- Around line 29-33: The current try/catch around the tauriInvoke call that
assigns cachedBase (calling tauriInvoke("engine_url")) swallows errors and falls
back to a hardcoded "http://127.0.0.1:4317"; remove that hardcoded fallback and
fail fast instead: either remove the catch or change the catch to rethrow the
caught error (or throw a new Error that includes the caught error) so the real
invoke/startup failure surfaces; update any callers expecting a non-null
cachedBase if necessary so they handle the propagated error.
In `@desktop/src/main.ts`:
- Around line 239-243: The teardown is registered only after openStream()
resolves which can leak EventSource; fix by capturing the openStream() promise
immediately, set teardown synchronously to an async closer that awaits that
promise and calls the real close once available, and then replace teardown with
the real close when the promise resolves. Concretely: call const openPromise =
openStream(sessionId, onMessage, () => {/*...*/}); set teardown = async () => {
const close = await openPromise; close?.(); }; and then openPromise.then(close
=> { teardown = close; }); so teardown is valid synchronously and will close the
stream even if navigation happens before openStream resolves.
- Around line 393-397: The form submit handler currently accepts any text as a
secret key; update the listener attached via form.addEventListener("submit",
...) to validate the key before calling setSecret: read and trim the key string
from FormData, ensure it matches a strict env-var regex (e.g.
/^[A-Z_][A-Z0-9_]*$/ or your chosen policy) and if it fails, prevent saving and
surface an error to the user (e.g. show a validation message or alert) instead
of calling setSecret(String(data.get("key")), ...); only call setSecret and
form.reset() when validation passes. Ensure you reference the existing handler,
the setSecret invocation, and form.reset so the change is local to that submit
flow.
In `@desktop/src/styles.css`:
- Line 12: The CSS custom property --mono uses mixed-case family names that
trigger stylelint value-keyword-case; update the declaration so mixed-case
families are quoted: change the value for --mono to include quoted names for
SFMono-Regular, Menlo, and Consolas (e.g. --mono: ui-monospace,
"SFMono-Regular", "Menlo", "Consolas", monospace;), leaving generic keywords
like ui-monospace and monospace unquoted.
In `@src/server/hub.ts`:
- Around line 97-103: The subscribe method currently calls
this.channel(sessionId) which creates a new session channel; change
subscribe(sessionId, ...) to first check for an existing channel without
creating one (e.g., use the internal map/get method that holds channels instead
of this.channel) and if no channel exists, do not create it—return a no-op
unsubscribe or throw a clear "session not found" error so callers won't
implicitly create a running session; update the SSE route handler to likewise
check for an existing channel and return a 404 response for unknown sessionIds
instead of calling the channel-creating accessor so that nonexistent sessions
are reported as 404 rather than creating a "running" session.
In `@src/server/index.ts`:
- Around line 28-32: The environment-provided staticDir may be relative, causing
the server's path-traversal guard to compare an absolute resolved request path
against a relative base and reject requests; normalize staticDir to an absolute
path before passing it into createServer. Update the code around the staticDir
variable (the const staticDir = process.env.LOOPWRIGHT_STATIC_DIR; and the
createServer({ ..., ...(staticDir ? { staticDir } : {}), }) call) to: if
staticDir is set, replace it with an absolute path (e.g., use
path.isAbsolute/static path.resolve(process.cwd(), staticDir) or
path.resolve(staticDir)) and pass that absolute value into createServer; also
add the required import for path if not already present.
In `@src/server/server.ts`:
- Around line 279-282: The path-traversal guard compares an absolute `resolved`
path to `dir` which may be a relative `process.env.LOOPWRIGHT_STATIC_DIR`;
normalize `dir` first so both sides are absolute and comparable — e.g., replace
the raw `dir` with `path.resolve(dir)` (and optionally `path.normalize`) before
computing `resolved` or performing the startsWith check so the existing guard
(the `resolved !== dir && !resolved.startsWith(dir + path.sep)` logic) works
correctly for relative env values.
In `@test/server.test.ts`:
- Around line 252-259: The test incorrectly assumes event ids are zero-based and
gapless by computing expected replay count as all.length - (cutoff + 1); instead
calculate the expected count by comparing ids. Replace the arithmetic assertion
with a computed expected value like: const expected = all.filter((e) => e.id >
cutoff).length and then assert expect(rest.length).toBe(expected). Update the
assertion that references cutoff/all/rest/sessionId/readSse accordingly.
- Around line 164-173: The test's polling loop may exit without setting the
local variable trace, causing an uncaught property access when asserting
trace.trace.session.*; modify the test to fail fast on timeout by asserting that
trace was set (e.g., expect(trace).toBeDefined() or throw a clear error)
immediately after the loop and before dereferencing, or alternatively capture
the final response and assert its status is 200 and its JSON populated; update
the polling block that uses fetch(.../api/sessions/${sessionId}/trace) and the
subsequent assertions to first verify trace is defined and/or that the response
indicated completion, then proceed to check trace.trace.session.goal and
trace.trace.session.status.
---
Nitpick comments:
In `@desktop/src-tauri/tauri.conf.json`:
- Line 22: Update the "csp" value to remove the unnecessary "http://localhost:*"
entry from the connect-src list so that only the pinned sidecar host
(http://127.0.0.1:*) and the existing ipc/http://ipc.localhost entries remain;
edit the "csp" property string (the JSON "csp" key currently containing
"connect-src 'self' http://127.0.0.1:* http://localhost:* ipc:
http://ipc.localhost; ...") and delete the "http://localhost:*" token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 678b6da5-8c73-4e3d-a2a0-c1b71482dc8f
⛔ Files ignored due to path filters (5)
desktop/package-lock.jsonis excluded by!**/package-lock.jsondesktop/src-tauri/Cargo.lockis excluded by!**/*.lockdesktop/src-tauri/icons/128x128.pngis excluded by!**/*.pngdesktop/src-tauri/icons/32x32.pngis excluded by!**/*.pngdesktop/src-tauri/icons/icon.pngis excluded by!**/*.png
📒 Files selected for processing (26)
.gitignore.kiro/specs/loop-engine/tasks.mddesktop/index.htmldesktop/package.jsondesktop/src-tauri/Cargo.tomldesktop/src-tauri/build.rsdesktop/src-tauri/capabilities/default.jsondesktop/src-tauri/src/engine.rsdesktop/src-tauri/src/lib.rsdesktop/src-tauri/src/main.rsdesktop/src-tauri/src/secrets.rsdesktop/src-tauri/tauri.conf.jsondesktop/src/api.tsdesktop/src/main.tsdesktop/src/styles.cssdesktop/src/types.tsdesktop/src/vite-env.d.tsdesktop/tsconfig.jsondesktop/vite.config.tspackage.jsonscripts/build-sidecar.mjssrc/server/hub.tssrc/server/index.tssrc/server/server.tssrc/server/store-tap.tstest/server.test.ts
Replace the placeholder solid-color icons with a real branded icon: a white circular loop-arrow (the actor-critic loop motif) on the brand-blue gradient rounded square. Generated the full platform set from a 1024x1024 source (desktop/app-icon.png) via `tauri icon` — desktop PNGs, icon.icns, icon.ico, Windows Square/Store logos, and android/ios assets. Point tauri.conf.json bundle.icon at the standard set (32/128/128@2x png + icns + ico).
Engine server: - hub.subscribe no longer creates a channel for an unknown session (prevented a stray monitor connection from reserving an id and causing spurious 409s); the SSE route now 404s unknown sessions (+ test). - Normalize LOOPWRIGHT_STATIC_DIR to an absolute path (in index.ts and the serveStatic guard) so a relative value no longer 403s every asset. Desktop shell (Rust): - engine.rs: kill the spawned sidecar if readiness detection fails, instead of leaking the process. - secrets.rs: surface non-NotFound index read errors instead of treating them as an empty key list; validate secret key names and reject the reserved LOOPWRIGHT_ prefix before persisting/injecting. Desktop frontend: - api.ts: drop the hardcoded loopback fallback; fail fast on engine_url errors. - main.ts: register the SSE teardown synchronously (no leaked EventSource on fast navigation); validate secret keys as env-var names; keep the restart button resilient on failure; surface an explicit "restart engine to apply" gate after secret changes. - styles.css: quote mixed-case font family names. Docs/tests: - tasks.md: correct the test count (132). - server.test.ts: fail fast if trace polling times out; compute SSE replay count by id comparison rather than assuming a gapless zero-based scheme. Full suite green (132 tests), tsc clean, desktop frontend builds, Rust formatted.
…atch deps Addresses post-merge review findings P1–P2. P1 (security) — the engine API could start runs (and thus spawn local cli processes / forward stored secrets to arbitrary endpoints) from any browser origin via wildcard CORS and token-less per-request runner config: - Require an unguessable per-process bearer token on every /api route except health (constant-time check; accepted via Authorization header or, for EventSource, a token query param). - Replace `Access-Control-Allow-Origin: *` with an origin allowlist (loopback + the Tauri webview); the specific origin is echoed, never `*`. - Token is delivered out-of-band: a Tauri `engine_token` command, or injected into the served index.html (so only a page loaded from loopback can read it), and reported on the engine readiness line. Frontend attaches it to all requests; restart re-resolves it. P1 (build) — the Tauri shell did not compile / package: - Declare `tokio` (time feature) directly; it was used via tokio::time::timeout but only present transitively. - Wire the sidecar build into Tauri: desktop `build:sidecar` script + run it from beforeDevCommand/beforeBuildCommand so binaries/loopwright-engine exists. P2 (deps) — upgrade vulnerable dev tooling: vitest 2 -> 4 (root) and vite 5 -> 8 (desktop). Both `npm audit` now report 0 vulnerabilities. Validated: 134 engine tests pass (incl. new auth-boundary tests), tsc clean, desktop frontend builds on vite 8, sidecar compiles and enforces the token end-to-end (401 for unauthenticated/cross-origin requests; cli runner never executes), Rust formatted and dependency graph (incl. tokio) resolves.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/api.ts`:
- Line 70: getJson currently always awaits authHeaders()/authToken causing
health() to fail when auth is unavailable; make health() bypass token resolution
by either adding an opt-out to getJson (e.g., add an optional parameter skipAuth
= false and, when true, avoid calling await authHeaders()) or by changing
health() to call fetch((await apiBase()) + path) directly without using
authHeaders()/getJson; update the getJson signature and callers if you choose
the opt-out approach, and ensure health() uses the skipAuth flag (or the direct
fetch) so /api/health remains unauthenticated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e635494-8af7-47d7-8e01-55e770a60068
⛔ Files ignored due to path filters (3)
desktop/package-lock.jsonis excluded by!**/package-lock.jsondesktop/src-tauri/Cargo.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
desktop/package.jsondesktop/src-tauri/Cargo.tomldesktop/src-tauri/src/engine.rsdesktop/src-tauri/src/lib.rsdesktop/src-tauri/tauri.conf.jsondesktop/src/api.tspackage.jsonsrc/server/index.tssrc/server/server.tstest/server.test.ts
✅ Files skipped from review due to trivial changes (1)
- desktop/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- package.json
- desktop/src-tauri/Cargo.toml
- desktop/src-tauri/tauri.conf.json
- desktop/src-tauri/src/engine.rs
- desktop/src-tauri/src/lib.rs
- src/server/index.ts
- test/server.test.ts
- src/server/server.ts
| } | ||
|
|
||
| async function getJson<T>(path: string): Promise<T> { | ||
| const res = await fetch((await apiBase()) + path, { headers: await authHeaders() }); |
There was a problem hiding this comment.
Keep /api/health independent from token resolution.
getJson() now always awaits authToken(), so health() can report false when the token bridge/global is unavailable even though the server is up. src/server/server.ts deliberately leaves /api/health unauthenticated so the UI can probe readiness before resolving auth, so this helper needs an opt-out or health() should bypass it.
Suggested fix
-async function getJson<T>(path: string): Promise<T> {
- const res = await fetch((await apiBase()) + path, { headers: await authHeaders() });
+async function getJson<T>(path: string, authenticated = true): Promise<T> {
+ const init = authenticated ? { headers: await authHeaders() } : undefined;
+ const res = await fetch((await apiBase()) + path, init);
if (!res.ok) throw new Error(`${res.status} ${await res.text()}`);
return res.json() as Promise<T>;
}
@@
export async function health(): Promise<boolean> {
try {
- const { ok } = await getJson<{ ok: boolean }>("/api/health");
+ const { ok } = await getJson<{ ok: boolean }>("/api/health", false);
return ok === true;
} catch {
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await fetch((await apiBase()) + path, { headers: await authHeaders() }); | |
| async function getJson<T>(path: string, authenticated = true): Promise<T> { | |
| const init = authenticated ? { headers: await authHeaders() } : undefined; | |
| const res = await fetch((await apiBase()) + path, init); | |
| if (!res.ok) throw new Error(`${res.status} ${await res.text()}`); | |
| return res.json() as Promise<T>; | |
| } | |
| export async function health(): Promise<boolean> { | |
| try { | |
| const { ok } = await getJson<{ ok: boolean }>("/api/health", false); | |
| return ok === true; | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/api.ts` at line 70, getJson currently always awaits
authHeaders()/authToken causing health() to fail when auth is unavailable; make
health() bypass token resolution by either adding an opt-out to getJson (e.g.,
add an optional parameter skipAuth = false and, when true, avoid calling await
authHeaders()) or by changing health() to call fetch((await apiBase()) + path)
directly without using authHeaders()/getJson; update the getJson signature and
callers if you choose the opt-out approach, and ensure health() uses the
skipAuth flag (or the direct fetch) so /api/health remains unauthenticated.
…control Second round of review findings. P1 — token leak in browser/static mode: static asset responses no longer carry CORS headers, so a page on another loopback origin can no longer fetch and read the token injected into index.html (the standalone UI loads same-origin and needs no CORS; the Tauri webview gets the token via the engine_token command). P1 — loopback-only enforcement: the entrypoint now refuses to bind a non-loopback LOOPWRIGHT_HOST unless LOOPWRIGHT_ALLOW_NON_LOOPBACK is set, so the token-authenticated API + injected token can't be exposed over the network by accident. Adds an exported isLoopbackHost helper. P2 — bounded, released event buffers: RunHub now caps retained messages per run (monotonic ids via a seq counter, so Last-Event-ID resume is unaffected), and the server releases a finished run's buffer after a grace period (hub.forget on a TTL) instead of holding it forever. P2 — admission control: a configurable cap on concurrent active runs returns 429 once exceeded, so repeated clicks / a buggy UI / a leaked token can't kick off unbounded background work. P3 — query-token scope: the ?token= query param is now accepted only on the SSE /stream route (EventSource can't set headers); every other route requires the Authorization header, keeping tokens out of URLs/logs. Validated: 140 tests pass (new hub buffer/forget tests + server tests for query-token restriction, the 429 cap, and static-without-CORS), tsc clean, and verified against the compiled sidecar (non-loopback refused; no ACAO on static; query token rejected off /stream).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/server.ts (1)
282-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel stale retention cleanup when a session id is reused.
settle()scheduleshub.forget(sessionId)(Line [315]) unconditionally. If a new run starts with the samesessionIdbefore that timer fires, the stale timer can delete the active channel/listeners mid-run, causing dropped SSE delivery and state reset.💡 Suggested fix
export function createServer(opts: CreateServerOptions): LoopwrightServer { @@ const hub = new RunHub(opts.maxBufferPerRun); let activeRuns = 0; + const forgetTimers = new Map<string, ReturnType<typeof setTimeout>>(); @@ async function startRun(req: IncomingMessage, res: ServerResponse, cors: Record<string, string>): Promise<void> { @@ const sessionId = body.sessionId ?? randomUUID(); + + // If this session is being reused, cancel any stale retention cleanup from + // a previous completed run so it can't delete the new active channel. + const pendingForget = forgetTimers.get(sessionId); + if (pendingForget) { + clearTimeout(pendingForget); + forgetTimers.delete(sessionId); + } + if (hub.has(sessionId) && hub.phase(sessionId) === "running") { return send(res, 409, { error: `session ${sessionId} is already running` }, cors); } @@ const settle = (): void => { activeRuns = Math.max(0, activeRuns - 1); - const timer = setTimeout(() => hub.forget(sessionId), retainMs); + const timer = setTimeout(() => { + forgetTimers.delete(sessionId); + if (hub.phase(sessionId) !== "running") hub.forget(sessionId); + }, retainMs); + forgetTimers.set(sessionId, timer); if (typeof timer.unref === "function") timer.unref(); };Also applies to: 328-338
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 282 - 317, The scheduled cleanup timer in settle() unconditionally calls hub.forget(sessionId) which can race with a new run reusing the same sessionId; to fix, track the per-session forget timer (e.g., a Map<string, Timeout> or attach a property keyed by sessionId), clear/cleanup any existing timer when a new run starts (just after computing sessionId and before hub.publish(..., "running")), and when creating the timer in settle() store it in that map and remove it after it fires; also call clearTimeout(existingTimer) and if present call existingTimer.unref() where supported to avoid orphaned timers, and ensure settle() removes the map entry when scheduling hub.forget so a resumed run won't be forgotten.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/server.ts`:
- Around line 282-317: The scheduled cleanup timer in settle() unconditionally
calls hub.forget(sessionId) which can race with a new run reusing the same
sessionId; to fix, track the per-session forget timer (e.g., a Map<string,
Timeout> or attach a property keyed by sessionId), clear/cleanup any existing
timer when a new run starts (just after computing sessionId and before
hub.publish(..., "running")), and when creating the timer in settle() store it
in that map and remove it after it fires; also call clearTimeout(existingTimer)
and if present call existingTimer.unref() where supported to avoid orphaned
timers, and ensure settle() removes the map entry when scheduling hub.forget so
a resumed run won't be forgotten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de8c198b-46ad-4ec3-b871-b13ec5ff5dd4
📒 Files selected for processing (5)
src/server/hub.tssrc/server/index.tssrc/server/server.tstest/hub.test.tstest/server.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/index.ts
… cache Third round of review findings. P1 — reused session id replayed stale live events: the hub now starts a fresh channel per run (RunHub.start) that discards the previous run's buffer and listeners, so reusing a finished session id can't replay the old run's events. A generation token guards the deferred cleanup timer so an old run's TTL forget can't drop a newer run on the same id. P1 — integration failure left the session "completed": final durable status is now decided AFTER integration via finalSessionStatus(), which downgrades to needs_human when integration.ok is false (conflicts / failed verification). The integration result is also recorded as an event for the trace/UI. P2 — corrupt/unreadable store silently discarded: JsonFileStore.open now treats only ENOENT as "start fresh"; permission/I/O errors propagate, and invalid JSON fails loud with the file left untouched for recovery (instead of being clobbered by the next write). P2 — token-bearing index.html was cacheable: it is now served with Cache-Control: no-store so a stale token can't linger in browser disk cache (hashed assets still cache normally). Validated: 145 tests pass (new: hub reuse stream isolation, finalSessionStatus matrix, corrupt-store refusal, static no-store + no-CORS), tsc clean, and the recompiled sidecar serves index.html with Cache-Control: no-store.
…on in UI Fourth round of review findings. P1 — engine failures left sessions stuck "running": runGoal's body is now wrapped in try/catch/finally. On any throw (planning, runner execution, worktree setup, integration, cleanup) it records a structured session_failed event and persists status "failed", then rethrows so the server's SSE error status still fires. P1 — worktrees leaked on thrown errors: per-task worktree release now happens in a finally that runs regardless of whether integration succeeded or threw, so a failed run can't leave .loopwright worktrees/branches behind. (Uses a const handle inside try for closure narrowing and mirrors it for the finally.) P2 — integration failures were easy to miss in the desktop UI: - the live "done" banner no longer always says "completed"; it reflects integration failure / tasks needing a human; - the results view now has a first-class Integration & verification card (merged/conflicts/verification, highlighted when blocking) plus a Run failed card sourced from the durable event log. Validated: 146 tests pass (new: failed-run persistence + session_failed event), tsc clean, desktop frontend builds.
Fifth round of review findings. P1 — sidecar kill/restart left sessions stuck "running": startup now calls reconcileInterruptedSessions(), marking any still-"running" session "failed" with a session_interrupted event, so a crash/restart can't leave durable sessions running forever. P1 — one rejected parallel task could fail the whole session and abandon siblings: the scheduler wraps each task run so it never rejects (an unexpected throw becomes a NEEDS_HUMAN result for that task only). Promise.race no longer rejects, all in-flight siblings drain, and worktree cleanup runs after they settle rather than racing live tasks. P2 — actor build/model failures were session-fatal: runTask now catches a thrown actor build and degrades that task to NEEDS_HUMAN (like a malformed critic) instead of throwing out of the run. P2 — no cancellation path: added cooperative cancellation via AbortSignal. runGoal/scheduler/loop honor it (stop launching, bail between steps, drain, then throw AbortError so the session is persisted failed), the mechanical gate kills in-flight subprocesses on abort, the server exposes POST /api/runs/:id/cancel backed by a per-run AbortController, and the desktop monitor gains a "Stop run" button. Validated: 151 tests pass (new: scheduler fault-isolation + cancel, gate subprocess kill, session reconcile, server cancel), tsc clean, desktop builds, and the compiled sidecar serves the cancel route (404 unknown, 401 unauth).
# Conflicts: # src/engine/mechanicalGate.ts
Cancel now reaches the model runners and the integration step, not just the loop and the mechanical gate. - RunRequest carries an optional AbortSignal; HttpRunner aborts its in-flight fetch and CliRunner kills its subprocess tree (reusing killProcessTree so descendants die too) when the signal fires. - Thread the run's signal through createRoles into RunnerActor and RunnerCritic so every actor/critic runner.run call is cancellable. - Make integrate() cancel-aware: stop merging further branches once aborted and pass the signal into the verification gate. So clicking Stop during a long HTTP/CLI model call or during integration returns promptly instead of waiting out the runner/request timeout. Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
The existing matrix only exercised the headless engine (npm ci, typecheck, test). Add a dedicated desktop job that proves the full desktop shell builds on Linux/macOS/Windows: - Bun + Rust toolchains and the Tauri v2 Linux system deps (incl. libsecret for the keyring backend). - desktop npm ci + npm audit (high), build the engine sidecar, build the Vite frontend, cargo test the Rust crate, then a full tauri build. This closes the gap where a break in the Tauri shell or sidecar could ship unnoticed.
The desktop ubuntu job built the binary and bundled .deb and .rpm, but AppImage bundling failed with 'failed to run linuxdeploy': the bundling tools are AppImages and GitHub runners lack FUSE. Set APPIMAGE_EXTRACT_AND_RUN=1 so they extract-and-run instead of mounting. macOS and Windows builds already pass.
APPIMAGE_EXTRACT_AND_RUN did not resolve the linuxdeploy failure on the GitHub Ubuntu runner. Restrict the Linux bundle to deb + rpm (which build reliably and exercise the full compile-and-bundle pipeline) and keep full all-target bundling on macOS and Windows.
Address PR review findings:
- Graceful server shutdown: stop() (and a new token-protected
POST /api/shutdown) now aborts in-flight runs — whose cooperative
cancel handlers synchronously kill detached subprocess trees — ends
tracked SSE streams, then closes the HTTP server, force-closing any
lingering sockets after a grace period. Previously stop() only called
http.close(), which could hang on long-lived SSE streams and orphan work.
- Tauri restart/quit now calls the engine's graceful shutdown endpoint
before child.kill(), so detached subprocess trees aren't orphaned by a
hard sidecar kill. lib.rs builds the app and handles ExitRequested/Exit
to stop the engine on quit.
- Validate caller-supplied sessionId (^[A-Za-z0-9_-]{1,64}$) at the
server boundary before it becomes a worktree path or branch ref; slug
the session id in GitWorktreeManager as defense in depth.
- Pin CI Bun to 1.2.14 so the shipped sidecar binary builds reproducibly.
… tests Address PR review findings: - closeServer() now tracks in-flight run promises and awaits them (bounded by a single shutdownGraceMs deadline) after aborting, so a shutdown no longer races ahead of durable failure writes and worktree cleanup. New runs are refused once shutdown has begun. - Tauri shutdown() polls the engine's loopback port until the listener closes (up to 6s, comfortably above the server's grace window) instead of a fixed 400ms sleep, so it waits for the graceful path to finish before force-killing the sidecar. - CI: add root 'npm audit --audit-level=moderate' (the sidecar engine's dependency tree), alongside the existing desktop audit. - Tests: cover POST /api/shutdown, stop() waiting for an aborted run's cleanup to complete, and rejection of unsafe session ids (../x, a/b, empty, >64 chars).
Address remaining production-readiness findings: - Set a real release version 0.1.0 across the root and desktop package manifests, Cargo.toml/Cargo.lock, and tauri.conf.json (was 0.0.0). - Add production bundle metadata to tauri.conf.json (publisher, homepage, category, copyright, short/long description) so installers carry proper identity. - CI: upload the built installers as artifacts from every desktop run so QA has a downloadable deliverable, not just a green check. - Add a release workflow that builds per-OS and publishes installers to a draft GitHub Release on a version tag, with macOS notarization and updater signing wired through repository secrets (activate when configured). - Add targeted Rust tests for the sidecar shutdown path: request_shutdown posts an authorized POST /api/shutdown, and wait_for_listener_close returns on refusal, waits for the listener to close, and respects its timeout. Remaining team-owned items (documented in release.yml): real signing/ notarization secrets, confirming the bundle identifier's domain, and an auto-update endpoint.
Windows loopback connection-refused can take ~1s (vs instant on Linux/macOS), so the <1s bound was brittle. Assert the function returns well before its timeout instead, which still proves it returns on refusal rather than hanging.
…ain namespace Address release-readiness findings: - release.yml now runs a 'gate' job (typecheck, root+desktop audits, JS tests, cargo test) that the publish job depends on, so a bad tagged commit can no longer produce a draft release without passing the same checks as main. - gate also verifies, on tag builds, that the vX.Y.Z tag exactly matches the version in package.json, desktop/package.json, tauri.conf.json, and Cargo.toml — failing fast before any build if they diverge. - secrets.rs derives the keychain service namespace from the app bundle identifier (app.config().identifier) instead of a second hardcoded 'dev.loopwright.desktop' copy, so the keychain namespace tracks the app identity automatically when it is finalized for production. Signing/notarization, Windows signing, the updater, and confirming the bundle identifier's domain remain team-owned (documented in release.yml).
- engine.rs: guard url/token/child as one Lifecycle behind a single mutex held across start/restart/shutdown (via *_locked helpers). Three separate mutexes let a concurrent restart_engine, or a restart racing app exit, interleave - spawning a second sidecar, overwriting the child handle, or clearing the URL/token of a freshly-started process. One lock makes the lifecycle operations mutually exclusive. - release.yml: manual (workflow_dispatch) dev builds are now prerelease:true (ref_type != 'tag'); real version tags stay non-prerelease, so publishing a dev draft can't be treated as a normal release. - release.yml: run the gate across ubuntu/macos/windows so direct tag pushes exercise the JS + Rust suites on every release platform. Tag/version match, dependency audits, and Linux deps stay Linux-only.
- release.yml: default the workflow to permissions: contents: read so a compromised dependency install/test step in the gate has no repo write authority. Grant contents: write only to the publish (release) job, and set persist-credentials: false on both checkouts so the GITHUB_TOKEN isn't left in .git/config for npm ci / test steps to read. tauri-action publishes via the GITHUB_TOKEN env var, not persisted git creds. - Guard the Secrets-view 'Restart engine' action against silently killing active runs (backend restart = shutdown + start, which aborts in-flight work). The engine now reports a process-scoped activeRuns count on /api/health; the desktop UI checks it and, when runs are active, shows an inline confirmation naming how many will be cancelled before restarting. Using the in-memory count (not stale store status) avoids false warnings after a crash. typecheck + 154 tests pass (root); desktop tsc + vite build clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request was created by @kiro-agent on behalf of @inhaq 👻
Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro Web
Summary
Implements Milestone 6 — Desktop delivery (tasks 25 & 26). The desktop app reuses the headless engine without reimplementing any loop logic (Req 13.3): the engine runs as a Node HTTP/SSE server compiled to a single binary and shipped as a Tauri sidecar.
What's included
Engine server (
src/server/) — a thin transport over existing entry points, no new orchestration policy:POST /api/runsstart a run ·GET /api/runs/:id/streamlive SSE (transitions, attempts, outcomes, runner calls, log lines) ·GET /api/sessions·GET /api/sessions/:id/trace·/api/healthRunHub: ordered in-memory pub/sub with a replay buffer +Last-Event-IDresume so late/reconnecting monitors see the full runtapStoreEvents: bridges the store's event stream to live subscribers while the store stays the single source of truthSidecar build —
scripts/build-sidecar.mjscompiles the server viabun --compiletodesktop/src-tauri/binaries/loopwright-engine-<target-triple>; new npm scriptsserveandbuild:sidecar.Desktop app (
desktop/)src-tauri/): spawns the sidecar, discovers its loopback port from the readiness line, and stores runner API keys in the OS keychain (keyring), injecting them into the sidecar env so a profile'sapiKeyEnvresolves with no plaintext on diskAlso syncs
tasks.mdto reality — M2–M5 were already complete onmain; the old "current position" note was stale.Testing
tsc --noEmitclean · 132 tests pass (7 new server tests over real HTTP/SSE with a simulated engine)/api/health,/api/sessions, run validation, and static+SPA serving against the built binarytsc+vite build)cargo generate-lockfile)Not verified here (local-only)
The full Tauri GUI build could not run in the sandbox: WebKitGTK dev libraries aren't available on Amazon Linux 2023, and there's no display. The Rust code is idiomatic Tauri v2 and its dependencies resolve, but
cargo build/tauri buildand a live GUI smoke test need a local machine.How to run
Desktop app:
Requires the Tauri prerequisites for your OS (WebKitGTK + build tools on Linux).
Browser-only (no desktop toolchain):
Provide runner profiles in the Start form (
LOOPWRIGHT_RUNNERS) and reference API keys by env-var name viaapiKeyEnv.Summary by CodeRabbit
New Features
Tests
Chores