Repository navigation
Release v0.30.7.6: Claude mid-turn Steer (#2008) and 9 new audited carries - #30
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This PR is too large for Bugbot to review. It changes 72,056 lines and 4,043,672 characters. Split the change into smaller pull requests to get a review. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new HAPI Desktop launcher, which uses Electron to manage the HAPI Hub and Runner by launching a bundled platform-specific CLI binary. It also includes significant updates to the Codex session management, specifically adding support for mid-turn steering and conversation rewinding, along with various improvements to Android text paging, permission modes, and WxPusher notification support. My review identified a potential runtime error in the claudeRemote.ts cleanup logic where response.waitForExit() could be called on an undefined object if initialization fails, and I have provided a fix for this.
| if (pendingSteers.size > 0) { | ||
| // An early return leaves the process running, and it could still | ||
| // run these prompts while their restored copies run again. | ||
| processController.abort(); | ||
| await response.waitForExit(); | ||
| } |
There was a problem hiding this comment.
In the finally block, response.waitForExit() is called if pendingSteers.size > 0. However, if an error occurs during initialization (for example, inside applyInitialTurn or before response is assigned), response will be undefined. Since pendingSteers can be populated as soon as setTurnActive(true) is called (which happens inside applyInitialTurn), pendingSteers.size > 0 can be true while response is still undefined. This will result in a TypeError: Cannot read properties of undefined (reading 'waitForExit') (or a ReferenceError if block-scoped), masking the original initialization error.
Please add a safety check to ensure response is defined before calling waitForExit().
| if (pendingSteers.size > 0) { | |
| // An early return leaves the process running, and it could still | |
| // run these prompts while their restored copies run again. | |
| processController.abort(); | |
| await response.waitForExit(); | |
| } | |
| if (pendingSteers.size > 0 && response) { | |
| // An early return leaves the process running, and it could still | |
| // run these prompts while their restored copies run again. | |
| processController.abort(); | |
| await response.waitForExit(); | |
| } |
![swear-review[bot]](https://avatars.githubusercontent.com/u/49574926?s=60&v=4){kind=small}
| cd ../release-artifacts | ||
| sign-macos: | ||
| if: github.repository == 'swear01/hapi' | ||
| runs-on: macos-14 |
There was a problem hiding this comment.
None of the jobs in this pipeline declare timeout-minutes. The sign-macos steps run security import / codesign, which can block indefinitely waiting for keychain unlock or an interactive prompt. Without a timeout a hung job can occupy a runner until the (default) hard limit, delaying the whole multi-stage release. Consider adding timeout-minutes to each job.
| runs-on: macos-14 | |
| runs-on: macos-14 | |
| timeout-minutes: 60 |
low · maintainability
— Swear Review
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: 1.4.0 |
There was a problem hiding this comment.
oven-sh/setup-bun is a third-party action pinned to a mutable tag (@v2). A moved/retagged release would run arbitrary code with this job's token, so it should be pinned to a full commit SHA (same applies to the other oven-sh/setup-bun@v2 occurrences in this file and in fixtures.yml).
| - uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: 1.4.0 | |
| - uses: oven-sh/setup-bun@<commit-sha> # v2.x.y | |
| with: | |
| bun-version: 1.4.0 |
low · security
— Swear Review
| build: | ||
| if: github.repository == 'tiann/hapi' |
There was a problem hiding this comment.
Putting the repository guard on build (and not only on deploy) makes the whole workflow a no-op on this fork (release.yml targets swear01/hapi), so the web production build (bun run --cwd web build) is no longer validated anywhere in CI here. If the intent is only to avoid deploying GitHub Pages from the fork, consider guarding just deploy; otherwise please confirm dropping web build coverage is intended.
| build: | |
| if: github.repository == 'tiann/hapi' | |
| build: | |
| runs-on: ubuntu-latest |
low · maintainability
— Swear Review
| with open(output, "w") as handle: | ||
| handle.write("upstream_ref\tauthor\thead_sha\tdecision\tstatus\tmaintainer_signal\tbot_verdict\tbot_sha\tscope\tbehavior\treason\n") | ||
| for pr in json.load(sys.stdin): | ||
| author = pr["author"]["login"] |
There was a problem hiding this comment.
author is a nullable GraphQL field (GitHub returns author: null for PRs opened by deleted/ghost accounts). Since this loops over every open PR in a long-lived repo, a single such PR raises TypeError here, the pipe fails and set -euo pipefail aborts the whole audit, after audit.md has already been written (leaving a partial release record).
| author = pr["author"]["login"] | |
| author = (pr.get("author") or {}).get("login") or "ghost" |
medium · bug
— Swear Review
| pkill -KILL -f "$SMOKE_PATTERN" 2>/dev/null || true |
There was a problem hiding this comment.
This SIGKILLs every process whose command line matches the broad SMOKE_PATTERN, not just this harness's children. The startup stray check only rejects processes that existed before the run, so a developer's own bun hub/src/index.ts / bun cli/src/index.ts runner start-sync (started after the check, or on a shared box with HAPI_SMOKE_FORCE=1) is hard-killed on exit, losing their session state. Prefer killing only this run's descendants instead of a global pattern match.
| pkill -KILL -f "$SMOKE_PATTERN" 2>/dev/null || true | |
| pkill -KILL -P "$HUB_PID" 2>/dev/null || true | |
| pkill -KILL -P "$RUNNER_PID" 2>/dev/null || true |
medium · bug
— Swear Review
| await this.rewindBarrier.load(); | ||
| this.rewindBarrier.assertOpen(); |
There was a problem hiding this comment.
SharedRewindBarrier.load() sets the token to the 'reconciliation-required' sentinel whenever the rewind file exists, and assertOpen() then throws, so prepare() fails for any session with a leftover rewind file. Because CompleteConversationRewind is only registered in registerControls() (called from activate(), which runs after bind()/prepare()), a root that fails prepare() never registers the recovery handler. The hub also only calls completeConversationRewind after a successful rewind(), so a crash (or a failed complete RPC) with the barrier file on disk leaves the session permanently unresumable — only manual deletion of the file recovers it. Note rewind.test.ts even asserts a replacement barrier cannot complete() the persisted state. Consider letting startup proceed in a blocked-but-recoverable state (register controls, then accept CompleteConversationRewind), or persisting/restoring the real token so the hub can reconcile in-band.
high · bug
— Swear Review
| } | ||
| /** Only before native dispatch: rejection is proven, so no reconciliation is needed. */ | ||
| async cancel(): Promise<void> { | ||
| await unlink(this.file); this.token = undefined; |
There was a problem hiding this comment.
cancel() awaits unlink before clearing this.token. If the unlink rejects (e.g. the file was already removed, or an FS error), this.token stays set, leaving the barrier permanently blocked even though the rejection was proven, and the throw propagates out of rewind()'s catch — turning the intended rejected(...) result into an RPC-level error. Clear the token first and tolerate a missing file:
this.token = undefined;
await unlink(this.file).catch(() => {});medium · bug
— Swear Review
| if (this.closed || this.stopping || this.reconnecting || !this.threadId) return; | ||
| if (this.closed || this.stopping || this.reconnecting || !this.threadId || this.rewindBarrier.blocked) return; | ||
| // The new connection replays authoritative pending native requests. | ||
| this.deferredRequests = []; |
There was a problem hiding this comment.
completeRewind discards buffered native server requests (this.deferredRequests = []) without replaying them, whereas the rejection path in rewind() explicitly calls replayDeferredRequests(). Any native server request (e.g. an approval prompt) that arrived while the barrier was closed is silently dropped, and the app-server may then wait indefinitely for a response that is never sent. replayDeferredRequests()'s own comment claims replay is safe because retired requests are ignored — please reuse it here (or document why dropping is correct after a successful revert).
medium · bug
— Swear Review
| ...(sandbox ? { permissionMode: sandbox === 'dangerFullAccess' ? 'yolo' : sandbox === 'readOnly' ? 'read-only' | ||
| : this.settingsNative.approvalsReviewer === 'auto_review' ? 'autoReview' : 'default' } : {}), |
There was a problem hiding this comment.
This is a three-level nested ternary, which the project checklist forbids (hard to read and easy to mis-edit when a mode is added). Extract a helper, e.g. permissionModeFromSandbox(sandbox, approvalsReviewer):
...(sandbox ? { permissionMode: permissionModeFromSandbox(sandbox, this.settingsNative.approvalsReviewer) } : {}),low · style
— Swear Review
| send(downstream, { id: message.id, result: await options.hooks.control(message.method, message.params) }); | ||
| return; | ||
| } | ||
| release = await options.hooks.admit?.(message, connectionId); |
There was a problem hiding this comment.
admit can now reject id-less notifications (runtime.ts throws 'Native mutations require a request ID'), which lands in the catch below. That branch always sends { id: message.id, error }; for a notification message.id is undefined, so JSON.stringify emits a malformed {"error":...} frame with no id, and the notification is dropped instead of forwarded upstream. Only emit the error envelope when message.id !== undefined (otherwise just drop/report the notification).
medium · bug
— Swear Review
Swear ReviewMode: Full PR Findings:
25 findings total Status: ❌ Failed |
1276614 to
af29626
Compare
Same official base as v0.30.7.5 (
5d8deb20). Rebuilt from the audited v0.30.7.5 queue; publication replacesmainwith--force-with-leaseagainst6c3e23c4.Changes vs v0.30.7.5
4c22ab47, plus overlayfix(claude): keep mid-turn steers recoverable until Claude takes them. The overlay fixes the 3 open HAPI Bot findings on that head and 3 Swear Review findings:command_lifecyclestay tracked, so an abort restores them and a follow-up turn doesn't send an early ready.in-flight) keeps the prompt's echo marker.AbortSignal.any) no longer leaks on early returns; a started steer is committed even if the output flush throws; the cancel log no longer labels non-removed results as "removed".command_lifecyclequeued/started beforeresult.fix(codex): fail closed when a shared rewind cannot complete, from Swear Review findings on fix(codex): restore Web Rewind for shared sessions tiann/hapi#2092):{ error }reply to CompleteConversationRewind, so the session is quarantined instead of stuck behind a closed barrier.a29b6496.hapi set-mode), fix(codex): include inherited fork history when syncing Codex transcripts tiann/hapi#2096, fix(web): give the rich composer its own undo history tiann/hapi#2097, fix(web): load one older page when the cold tail is tool-only tiann/hapi#2098, fix(codex): do not re-insert legacy history on shared resume tiann/hapi#2099, fix(web): ignore pending requests on archived sessions in the sidebar tiann/hapi#2101, fix: show Claude sessions waiting on background tasks (#2100) tiann/hapi#2102.webhookTimeoutMs, which carried fix(runner): add webhook timeout configuration hint tiann/hapi#2066 still uses. The hint now reports the per-agent startup timeout.Added after re-inventory (2026-10-23)
Operator-selected PRs, replayed at their audited heads:
ps axewwin test-process audit tiann/hapi#1869: already covered on the fork.hapi.dbbackup.inspect_peer/ping_peer/spawn_peer; the fork has nolist_peersand no peer prompts.hapi spawn-peeralso refuses when disabled.Swear Review triage (d58ec32, 25 findings)
meta.jsonrace: it falls back to prompt matching, and holding the cursor would stop forwarding whenmeta.jsonnever appears.command_lifecyclestate union.Verification (oracle, 2026-10-23)
bun typecheck,bun run test: CLI 3,656 / 19 skip, hub 1,642, web 3,846, shared 346, relay 118, desktop 5; 0 failures. Frozen install: no changes.REPLAY_TREE_MATCHES=47dee910,REHEARSAL_OK; the replayed tree passes the same suites. Maintenance tests: 43 pass.gen:fixtures: no drift.Release record:
tools/maintenance/releases/v0.30.7.6/.🤖 Generated with Claude Code