feat: add prompt-free session control workflow - #1771
Conversation
There was a problem hiding this comment.
Findings
-
[Major] Make
spawn-with-remitidempotent across ambiguous transport failures - the Hub can finish creating the child and delivering the remit, then lose the HTTP response. The CLI converts that into an ordinary failure and a retry generates a new remit ID, leaving the first child running and potentially creating a duplicate. Evidencehub/src/sync/syncEngine.ts:2007,cli/src/modules/spawnPeer/spawnPeer.ts:153,cli/src/modules/spawnPeer/spawnPeer.ts:178.Suggested fix:
const key = `${namespace}:${request.remitId}` const previous = this.spawnRemitResults.get(key) if (previous) return previous const outcome = await this.performSpawnSessionWithRemit(machineId, namespace, request) this.spawnRemitResults.set(key, outcome) return outcome
Retain the terminal outcome durably or with a bounded TTL, and retry/reconcile from the CLI using the same
remitId. -
[Major] Bound
wait-peerresults to the requested turn - result extraction skips every user message after the remit and collects all later assistant messages. If another prompt is queued or sent before polling completes, the globallive.thinkingflag can settle only after that later turn, andwait-peerreturns both turns as the original remit's result. Evidencecli/src/modules/pingPeer/pingPeer.ts:583,cli/src/modules/pingPeer/pingPeer.ts:662.Suggested fix:
const role = typeof row.content.role === 'string' ? row.content.role : '' if (role === 'user') break if (role !== 'agent' && role !== 'assistant') continue
-
[Major] Preserve OpenCode plan-mode semantics - plan-mode prompts are now forwarded unchanged, while the only remaining enforcement happens after OpenCode requests a tool. Nothing in the repo signals to the model that it should produce a plan, so Plan Mode can attempt ordinary work, receive denials, or answer without an implementation plan. No native OpenCode plan-mode signal was found in repo/docs. Evidence
cli/src/opencode/opencodeRemoteLauncher.ts:593,cli/src/opencode/utils/permissionHandler.ts:95,cli/src/opencode/opencodeRemoteLauncher.test.ts:1861.Suggested fix:
const text = batch.mode.permissionMode === 'plan' ? `You are in plan mode. Analyze the request and return an implementation plan only.\n\n${batch.message}` : batch.message
Use that text for the prompt until a native OpenCode mode can be set; otherwise disable the advertised Plan Mode for OpenCode.
-
[Major] Reject relative directories before remote spawn - the new CLI deliberately preserves a remote path such as
./runner/path, and the Hub schema only trims it. The runner then resolves/checks it relative to the runner daemon's process cwd and may create or start the child in an unintended directory, despite the session-control contract requiring an explicit absolute working directory. Evidencecli/src/modules/spawnPeer/spawnPeer.ts:150,shared/src/apiTypes.ts:628,hub/src/web/routes/machines.ts:172,cli/skills/hapi-session-control/SKILL.md:19.Suggested fix:
const pathApi = machine.metadata?.platform === 'win32' ? path.win32 : path.posix if (!pathApi.isAbsolute(parsed.data.directory)) { return c.json({ error: 'directory must be absolute' }, 400) }
Summary
Review mode: initial. Four Major correctness/safety issues found in the new session-control workflow.
Testing
Not run (automation; PR code was not executed during review). GitHub test, integration, and drift-gate checks pass. Add focused tests for response-loss retry/reconciliation, a later user-turn boundary in waitPeer, OpenCode plan-mode prompt behavior, and relative remote directory rejection.
HAPI Bot
| namespace: string, | ||
| request: SpawnSessionWithRemitRequest | ||
| ): Promise<SpawnSessionWithRemitResult> { | ||
| const existingIds = new Set(this.getSessions().map((session) => session.id)) |
There was a problem hiding this comment.
[MAJOR] This starts a fresh child without first checking whether the same namespace/remit request already completed. If the Hub delivers the remit but the HTTP response is lost, the caller sees spawn_failed; its next invocation generates another UUID and can create a duplicate while the original remains running.
Suggested fix:
const key = `${namespace}:${request.remitId}`
const previous = this.spawnRemitResults.get(key)
if (previous) return previous
const outcome = await this.performSpawnSessionWithRemit(machineId, namespace, request)
this.spawnRemitResults.set(key, outcome)
return outcomeRetain the outcome durably or with a bounded TTL, and make the CLI retry/reconcile with the same remitId.
| if (!isObject(row)) continue | ||
| if (!isObject(row.content)) continue | ||
| const role = typeof row.content.role === 'string' ? row.content.role : '' | ||
| if (role !== 'agent' && role !== 'assistant') continue |
There was a problem hiding this comment.
[MAJOR] Skipping user rows means this collector crosses turn boundaries. When another prompt follows the requested remit, waitPeer can wait for the later turn's global thinking state to clear and then attribute both assistant replies to the first remit.
Suggested fix:
const role = typeof row.content.role === 'string' ? row.content.role : ''
if (role === 'user') break
if (role !== 'agent' && role !== 'assistant') continueAdd a test with remit -> assistant reply -> next user prompt -> next assistant reply and assert only the first reply is returned.
| const promptContent: PromptContent[] = [{ | ||
| type: 'text', | ||
| text: messageText, | ||
| text: batch.message, |
There was a problem hiding this comment.
[MAJOR] Forwarding the prompt unchanged removes the only signal telling OpenCode to produce a plan. The permission handler only denies tools after they are requested; it does not establish plan-mode response semantics, so the advertised mode can attempt normal work or return non-plan prose.
Suggested fix:
const text = batch.mode.permissionMode === 'plan'
? `You are in plan mode. Analyze the request and return an implementation plan only.\n\n${batch.message}`
: batch.messageUse text here until a native OpenCode plan-mode setting is available; otherwise do not expose Plan Mode for OpenCode.
| } | ||
|
|
||
| const body = await c.req.json().catch(() => null) | ||
| const parsed = SpawnSessionWithRemitRequestSchema.safeParse(body) |
There was a problem hiding this comment.
[MAJOR] The schema accepts relative directories, and spawnPeer intentionally sends remote paths unchanged. A value like ./runner/path is then resolved and potentially created relative to the runner daemon's cwd, not an explicit user-selected workspace.
Suggested fix:
const pathApi = machine.metadata?.platform === 'win32' ? path.win32 : path.posix
if (!pathApi.isAbsolute(parsed.data.directory)) {
return c.json({ error: 'directory must be absolute' }, 400)
}Validate at the Hub using the target machine platform, and cover POSIX/Windows absolute paths plus relative-path rejection.
There was a problem hiding this comment.
Findings
-
[Major] Keep native OpenCode mode catalogs in sync — the TypeScript source of truth now removes OpenCode Plan Mode, but both native ports still advertise it. The current iOS
package-testscheck fails on this exact mismatch, and Android's hard-coded catalog is stale too. Evidenceshared/src/modes.ts:46,ios/Packages/HapiKit/Sources/HapiProtocol/Catalog/PermissionModes.swift:96,android/core/protocol/src/main/kotlin/app/hapi/protocol/catalog/Modes.kt:70.Suggested fix:
case .opencode: return [.default, .yolo]
val OPENCODE = listOf(PermissionMode.Default, PermissionMode.Yolo)
-
[Major] Do not report cleanup for an unexpected returned session ID — this branch calls
fail(), which only stops/archives the reservedsessionId, notresult.sessionId. If the runner actually launched the unexpected ID, the response can still saycleanedUp: true, so the CLI suppresses its orphan warning while that process may remain running. Evidencehub/src/sync/syncEngine.ts:2173,hub/src/sync/syncEngine.ts:2130.Suggested fix:
if (result.sessionId !== sessionId) { await this.cleanupSpawnedSession(machineId, namespace, sessionId) return { type: 'error', code: 'spawn_not_fresh', message: 'Runner returned an unexpected session id; it was not stopped', childSessionId: result.sessionId, cleanedUp: false } }
-
[Major] Preserve remote Grok plan semantics or stop exposing the mode — the prompt is now forwarded unchanged, while the ACP backend only switches between
defaultandauto. The permission handler can reject tool requests after the fact, but nothing tells Grok to produce a plan, despite the mode remaining advertised. Evidencecli/src/grok/grokRemoteLauncher.ts:297,shared/src/modes.ts:43,cli/src/grok/utils/permissionHandler.ts:87.Suggested fix:
// Until remote ACP exposes a native plan signal, do not advertise it. export const GROK_PERMISSION_MODES = ['default', 'auto', 'bypassPermissions'] as const
-
[Major] Normalize the target path before spawning and verifying it — absolute paths with a trailing separator are accepted and passed through unchanged, but a compiled runner records the child's OS-normalized cwd. The later strict equality check then produces
spawn_selection_mismatchand cleans up an otherwise valid child. Evidencehub/src/web/routes/machines.ts:191,hub/src/sync/syncEngine.ts:2197.Suggested fix:
const directory = targetPath.resolve(parsed.data.directory) const result = await engine.spawnSessionWithRemit( machineId, c.get('namespace'), { ...parsed.data, directory } )
Summary
Review mode: follow-up after new commits. Four Major correctness/conformance issues remain on the latest head.
Testing
Not run (review automation). GitHub test, integration, fixture drift-gate, Android build, and iOS app-build pass. iOS package-tests fails because OpenCode Plan Mode remains in the native catalog. Add focused tests for unexpected-ID cleanup reporting, remote Grok Plan Mode behavior, and POSIX/Windows trailing-separator paths.
HAPI Bot
| export type GrokPermissionMode = typeof GROK_PERMISSION_MODES[number] | ||
|
|
||
| export const OPENCODE_PERMISSION_MODES = ['default', 'plan', 'yolo'] as const | ||
| export const OPENCODE_PERMISSION_MODES = ['default', 'yolo'] as const |
There was a problem hiding this comment.
[MAJOR] Removing OpenCode Plan Mode here leaves the native protocol catalogs stale. The current iOS package-tests job fails because PermissionModes.swift still returns [.default, .plan, .yolo]; Android's Modes.kt has the same old list.
Suggested fix:
case .opencode:
return [.default, .yolo]val OPENCODE = listOf(PermissionMode.Default, PermissionMode.Yolo)Update the corresponding native spot-check expectations as well.
| } | ||
| if (result.type === 'error') return await fail(result.code ?? 'spawn_failed', result.message) | ||
| if (result.sessionId !== sessionId) { | ||
| return await fail('spawn_not_fresh', 'Runner returned an unexpected session id; remit was not delivered') |
There was a problem hiding this comment.
[MAJOR] fail() cleans only the reserved sessionId. In this branch the runner returned a different ID, so an actually launched unexpected child is not stopped, yet the response can report cleanedUp: true for the reservation. The CLI then suppresses the warning that a child may still be running.
Suggested fix:
if (result.sessionId !== sessionId) {
await this.cleanupSpawnedSession(machineId, namespace, sessionId)
return {
type: 'error',
code: 'spawn_not_fresh',
message: 'Runner returned an unexpected session id; it was not stopped',
childSessionId: result.sessionId,
cleanedUp: false
}
}| this.instructionsSent = true | ||
| } | ||
| const promptContent: PromptContent[] = [{ type: 'text', text }] | ||
| const promptContent: PromptContent[] = [{ type: 'text', text: batch.message }] |
There was a problem hiding this comment.
[MAJOR] This removes the only signal that made remote Grok produce a plan, but plan remains advertised. The ACP backend maps every non-auto mode to default; rejecting tools afterward does not establish plan-only response semantics.
Suggested fix:
// Until remote ACP exposes a native plan signal, do not advertise it.
export const GROK_PERMISSION_MODES =
['default', 'auto', 'bypassPermissions'] as constAlternatively gate Plan Mode to local Grok sessions with a real native mode signal.
| return c.json({ error: `${flavor.toUpperCase()} only supports remote mode` }, 400) | ||
| } | ||
|
|
||
| const result = await engine.spawnSessionWithRemit(machineId, c.get('namespace'), parsed.data) |
There was a problem hiding this comment.
[MAJOR] The route validates absolute paths but passes their original spelling through. A valid path such as /runner/project/ is recorded by a compiled child as the OS-normalized cwd /runner/project; the strict selection check then fails and cleans up the valid session.
Suggested fix:
const directory = targetPath.resolve(parsed.data.directory)
const result = await engine.spawnSessionWithRemit(
machineId,
c.get('namespace'),
{ ...parsed.data, directory }
)Cover trailing separators for both POSIX and Windows paths.
There was a problem hiding this comment.
Findings
-
[Blocker] Reserved HAPI session IDs are not forwarded for fresh Claude, Kimi, or Copilot spawns —
spawnSessionWithRemitreserves one exact row and rejects any returned ID that differs, but the runner only adds--existing-session-idfor other flavors/Claude forks. The new default agent is Claude, so the defaultspawn_peercall creates a second row and ends asspawn_not_fresh; Kimi and Copilot also lack command parsing for the flag. Evidence:cli/src/modules/spawnPeer/spawnPeer.ts:136,cli/src/runner/run.ts:1543,hub/src/sync/syncEngine.ts:2160.
Suggested fix:const existingSessionId = options.existingSessionId ?? options.sessionId if (existingSessionId) { args.push('--existing-session-id', existingSessionId) } // In kimi.ts and copilot.ts: } else if (arg === '--existing-session-id') { const sessionId = commandArgs[++i] if (!sessionId) throw new Error('Missing --existing-session-id value') options.existingSessionId = sessionId }
-
[Major] Concurrent conflicting requests with the same remit ID are coalesced instead of rejected — the in-flight key contains only namespace/remit ID, so a second request with different machine, directory, message, or options receives the first request's promise. The durable request-hash conflict check is never reached while that promise is active. Evidence:
hub/src/sync/syncEngine.ts:2012.
Suggested fix:const requestHash = hashSpawnRemitRequest(machineId, request) const existing = this.spawnRemitTails.get(tailKey) if (existing) { if (existing.requestHash !== requestHash) { return { type: 'error', code: 'remit_conflict', message: 'Remit ID was reused with different inputs' } } return await existing.task } this.spawnRemitTails.set(tailKey, { requestHash, task })
-
[Major] Remote Grok Plan Mode no longer tells the model to plan — Grok still advertises
plan, but this changed prompt now sends the user's message unchanged. Backend mode synchronization maps every non-automode to default; denying tool requests after they occur is not equivalent to instructing the model to produce a plan. Evidence:cli/src/grok/grokRemoteLauncher.ts:297,shared/src/modes.ts:43.
Suggested fix:const isSlashCommand = batch.message.trimStart().startsWith('/') const text = batch.mode.permissionMode === 'plan' && !isSlashCommand ? `Work in plan-only mode. Analyze and propose a plan, but do not execute commands or modify files.\n\n${batch.message}` : batch.message const promptContent: PromptContent[] = [{ type: 'text', text }]
-
[Major] Remote target paths are validated but not normalized before exact selection matching — spellings such as
/project/, lexical.., or Windows trailing separators can be normalized by the target CLI before metadata is reported. The Hub then compares that normalized metadata with the original request string and cleans up a valid child asspawn_selection_mismatch. Evidence:hub/src/web/routes/machines.ts:191,hub/src/sync/syncEngine.ts:2197.
Suggested fix:const directory = targetPath.resolve(parsed.data.directory) const result = await engine.spawnSessionWithRemit( machineId, c.get('namespace'), { ...parsed.data, directory } )
Summary
Review mode: follow-up after new commits
Testing: Not run (review automation; PR code was not executed). Current GitHub checks pass, but coverage is missing for reserved-ID spawning across every creatable flavor, conflicting concurrent remit reuse, remote Grok Plan Mode prompt semantics, and POSIX/Windows path normalization.
HAPI Bot
| throw new SpawnPeerError('bad_args', `name must be at most ${SESSION_NAME_MAX_LENGTH} characters`) | ||
| } | ||
|
|
||
| const agent = options.agent ?? 'claude' |
There was a problem hiding this comment.
[BLOCKER] Reserved session ID is lost for the default agent
This defaults atomic remit spawning to Claude, but buildCliArgs() only emits --existing-session-id for Codex/Cursor/Pi/OpenCode/AGY/DSH/Grok and Claude forks. Fresh Claude therefore creates another HAPI row; Kimi and Copilot have the same runner omission and their command parsers also ignore this flag. The Hub reserved sessionId is passed at syncEngine.ts:2160 and a different result is rejected at :2172, so these flavors cannot complete the new atomic path.
Suggested fix:
const existingSessionId = options.existingSessionId ?? options.sessionId
if (existingSessionId) {
args.push('--existing-session-id', existingSessionId)
}Apply this to every fresh creatable flavor, and parse/pass existingSessionId in the Kimi and Copilot commands. Add a runner integration matrix over CREATABLE_AGENT_FLAVORS.
| request: SpawnSessionWithRemitRequest | ||
| ): Promise<SpawnSessionWithRemitResult> { | ||
| const tailKey = `${namespace}:${request.remitId}` | ||
| const existing = this.spawnRemitTails.get(tailKey) |
There was a problem hiding this comment.
[MAJOR] Conflicting concurrent remit requests bypass conflict detection
The in-flight key only includes namespace and remit ID. If a second request reuses that ID with a different machine/directory/message/options while the first is active, it receives the first promise/result instead of remit_conflict; the request hash in spawnSessionWithRemitOnce() is never checked.
Suggested fix:
const requestHash = hashSpawnRemitRequest(machineId, request)
const existing = this.spawnRemitTails.get(tailKey)
if (existing) {
if (existing.requestHash !== requestHash) {
return { type: 'error', code: 'remit_conflict', message: 'Remit ID was reused with different inputs' }
}
return await existing.task
}
this.spawnRemitTails.set(tailKey, { requestHash, task })The concurrent-retry test should also cover the same remit ID with different inputs.
| this.instructionsSent = true | ||
| } | ||
| const promptContent: PromptContent[] = [{ type: 'text', text }] | ||
| const promptContent: PromptContent[] = [{ type: 'text', text: batch.message }] |
There was a problem hiding this comment.
[MAJOR] Remote Grok Plan Mode no longer has plan semantics
This changed line removes the plan-only instruction while GROK_PERMISSION_MODES still advertises plan. syncBackendPermissionMode() resets every non-auto mode to backend default, and the permission handler only denies tool calls after Grok requests them; neither tells Grok to analyze and return a plan.
Suggested fix:
const isSlashCommand = batch.message.trimStart().startsWith('/')
const text = batch.mode.permissionMode === 'plan' && !isSlashCommand
? `Work in plan-only mode. Analyze and propose a plan, but do not execute commands or modify files.\n\n${batch.message}`
: batch.message
const promptContent: PromptContent[] = [{ type: 'text', text }]Alternatively, gate remote Grok Plan Mode while retaining local Grok's native plan support.
| return c.json({ error: `${flavor.toUpperCase()} only supports remote mode` }, 400) | ||
| } | ||
|
|
||
| const result = await engine.spawnSessionWithRemit(machineId, c.get('namespace'), parsed.data) |
There was a problem hiding this comment.
[MAJOR] Normalize the target path before exact selection matching
The route validates with the target OS path implementation but forwards the original spelling. The target CLI can normalize /project/, lexical .., or Windows trailing separators before publishing metadata; spawnSessionWithRemit() then compares childMetadata.path === request.directory and cleans up the valid child as spawn_selection_mismatch.
Suggested fix:
const directory = targetPath.resolve(parsed.data.directory)
const result = await engine.spawnSessionWithRemit(
machineId,
c.get('namespace'),
{ ...parsed.data, directory }
)Add cross-machine POSIX and Windows cases with trailing separators.
There was a problem hiding this comment.
Findings
-
[Blocker] Fresh Claude, Kimi, and Copilot spawns do not bind the Hub-reserved session ID — the atomic flow passes the reserved ID into
spawnSession, butbuildCliArgsonly emits--existing-session-idfor Codex/Cursor/Pi/OpenCode/AGY/DSH/Grok (and Claude forks), while the Kimi and Copilot command parsers have no field for it. Since Claude is the default, a defaultspawn-peercreates a second row, returnsspawn_not_fresh, and the returned process is deliberately left unstopped because its identity is untrusted. Evidencehub/src/sync/syncEngine.ts:2167; related contextcli/src/runner/run.ts:1545,cli/src/commands/kimi.ts:14,cli/src/commands/copilot.ts:15.
Suggested fix:const existingSessionId = options.existingSessionId ?? options.sessionId if (existingSessionId) args.push('--existing-session-id', existingSessionId) // Kimi/Copilot option shape + parser: existingSessionId?: string // ... } else if (arg === '--existing-session-id') { const id = commandArgs[++i] if (!id) throw new Error('Missing --existing-session-id value') options.existingSessionId = id }
-
[Major] Concurrent conflicting requests with the same remit ID are coalesced — the in-flight map stores only a promise under
namespace:remitId, so a second request with different machine, directory, message, or selectors receives the first request's result. The durable request-hash conflict check is bypassed until the first promise finishes. Evidencehub/src/sync/syncEngine.ts:2012.
Suggested fix:const requestHash = hashSpawnRemitRequest(machineId, request) const existing = this.spawnRemitTails.get(tailKey) if (existing) { if (existing.requestHash !== requestHash) { return { type: 'error', code: 'remit_conflict', message: 'Remit ID was reused with different inputs' } } return await existing.task } this.spawnRemitTails.set(tailKey, { requestHash, task })
Summary
Review mode: follow-up after new commits
The current head still contains two issues from the previous bot pass: one blocks the default and two selectable agent flavors from completing atomic spawn, and one breaks the remit conflict contract under concurrency.
Testing
Not run (review automation; PR code was not executed). Missing coverage: reserved-ID fresh spawn through every creatable flavor, and concurrent same-remit requests with different inputs.
HAPI Bot
| request.effort, | ||
| request.permissionMode, | ||
| request.serviceTier, | ||
| sessionId, |
There was a problem hiding this comment.
[BLOCKER] Fresh Claude/Kimi/Copilot spawns do not bind this reserved ID.
This argument is passed as existingSessionId, but cli/src/runner/run.ts:1545 excludes fresh Claude, Kimi, and Copilot from emitting --existing-session-id; Kimi/Copilot also do not parse that flag. Claude is the default, so the runner creates a different Hub row, this flow returns spawn_not_fresh, and the actual process is left running because the returned ID cannot safely be trusted.
Suggested fix:
const existingSessionId = options.existingSessionId ?? options.sessionId
if (existingSessionId) {
args.push('--existing-session-id', existingSessionId)
}
// Also add existingSessionId to the Kimi/Copilot option shapes and parse:
} else if (arg === '--existing-session-id') {
const id = commandArgs[++i]
if (!id) throw new Error('Missing --existing-session-id value')
options.existingSessionId = id
}| request: SpawnSessionWithRemitRequest | ||
| ): Promise<SpawnSessionWithRemitResult> { | ||
| const tailKey = `${namespace}:${request.remitId}` | ||
| const existing = this.spawnRemitTails.get(tailKey) |
There was a problem hiding this comment.
[MAJOR] Reject conflicting in-flight reuse before coalescing.
The map key contains only namespace/remit ID and stores only the first promise. A simultaneous second request with a different machine, directory, message, or selector therefore receives the first request's result; spawnSessionWithRemitOnce never reaches the durable requestHash conflict check for that request.
Suggested fix:
const requestHash = hashSpawnRemitRequest(machineId, request)
const existing = this.spawnRemitTails.get(tailKey)
if (existing) {
if (existing.requestHash !== requestHash) {
return { type: 'error', code: 'remit_conflict', message: 'Remit ID was reused with different inputs' }
}
return await existing.task
}
this.spawnRemitTails.set(tailKey, { requestHash, task })There was a problem hiding this comment.
Findings
-
[Major]
wait-peertreats uninvoked user rows as turn boundaries - the Hub pins queued and future-scheduled user messages into the latest message page. If one is created while the remit is running, it sorts before the later assistant reply; the unconditional break then hides that reply andwait-peertimes out even though the result exists. Evidence:cli/src/modules/pingPeer/pingPeer.ts:583.Suggested fix:
if (role === 'user') { if (typeof row.invokedAt === 'number') break continue }
-
[Minor] Codex still advertises
/agentafter its handler was removed - the changed test requires passthrough, whileshared/src/slashCommands.ts:15still describes the command as toggling proactive delegation. In this app-server path, passthrough is queued as a normal prompt, so the exposed command no longer performs its advertised state change. Evidence:cli/src/codex/utils/slashCommands.test.ts:13.Suggested fix:
codex: [ { name: 'clear', description: 'Clear current Codex thread context', source: 'builtin' }, // ... ],
Summary
Review mode: follow-up after new commits
Two issues remain: remit-result delimiting can block result collection in the presence of queued or scheduled messages, and the Codex command catalog exposes behavior removed by this diff.
Testing
- Not run (automation; PR code was not executed).
HAPI Bot
| if (!isObject(row)) continue | ||
| if (!isObject(row.content)) continue | ||
| const role = typeof row.content.role === 'string' ? row.content.role : '' | ||
| if (role === 'user') break |
There was a problem hiding this comment.
[MAJOR] Ignore uninvoked user rows when delimiting the remit result
The Hub includes uninvoked queued and future-scheduled user messages in the latest page. If such a message is created while this remit is running, its createdAt position can fall before the later assistant output. This unconditional break then returns no result messages, so wait-peer keeps polling until timeout even though the assistant reply exists.
Suggested fix:
if (role === 'user') {
if (typeof row.invokedAt === 'number') break
continue
}| message: 'Codex proactive multi-agent mode: on' | ||
| }); | ||
| it('does not intercept /agent with a HAPI prompt mode', () => { | ||
| expect(resolveCodexSlashCommand('/agent', state)).toEqual({ kind: 'passthrough' }); |
There was a problem hiding this comment.
[MINOR] Remove the stale /agent catalog entry when removing its handler
This now deliberately passes /agent through, but shared/src/slashCommands.ts:15 still exposes it as "Toggle proactive Codex multi-agent delegation." The app-server path queues passthrough text as a normal turn, so users are offered a command that no longer performs the advertised toggle.
Suggested fix:
codex: [
{ name: 'clear', description: 'Clear current Codex thread context', source: 'builtin' },
// ...
],There was a problem hiding this comment.
Findings
-
[Major]
wait-peertreats uninvoked queued rows as result boundaries — queued or future-scheduled user messages can appear after the remit withoutinvokedAt; breaking on them hides the later assistant response and causes a false timeout. Evidence:cli/src/modules/pingPeer/pingPeer.ts:583.Suggested fix:
if (role === "user") { if (typeof row.invokedAt === "number") break continue }
-
[Major] Atomic spawn does not verify every accepted runtime selector — the request accepts service tier, Codex collaboration mode, Copilot agent mode, and starting mode, but the ready check ignores all four. A dropped plan-mode selector can therefore still deliver the remit to a default execution session. Evidence:
hub/src/sync/syncEngine.ts:2223, related request fieldsshared/src/apiTypes.ts:619.Suggested fix:
&& (request.serviceTier === undefined || child.serviceTier === request.serviceTier) && (request.collaborationMode === undefined || child.collaborationMode === request.collaborationMode) && (request.copilotAgentMode === undefined || child.copilotAgentMode === request.copilotAgentMode) && (request.startingMode === undefined || childMetadata.startingMode === request.startingMode)
-
[Minor] Codex still advertises
/agentafter removing its handler — the changed test requires passthrough, while the catalog still describes/agentas toggling proactive delegation. The command is now submitted as a normal prompt instead of performing the advertised state change. Evidence:cli/src/codex/utils/slashCommands.test.ts:13, related catalogshared/src/slashCommands.ts:15.Suggested fix:
codex: [ { name: "clear", description: "Clear current Codex thread context", source: "builtin" }, // ... ],
Summary
Review mode: follow-up after new commits
Three issues remain: remit-result delimiting can miss valid output, atomic spawn can deliver work despite ignored plan/runtime selectors, and the Codex command catalog exposes removed behavior.
Testing
- Not run (automation; PR code was not executed).
- Add coverage for uninvoked queued rows during
wait-peer, plus mismatch cleanup for service tier, collaboration mode, Copilot agent mode, and starting mode.
HAPI Bot
| if (!isObject(row)) continue | ||
| if (!isObject(row.content)) continue | ||
| const role = typeof row.content.role === 'string' ? row.content.role : '' | ||
| if (role === 'user') break |
There was a problem hiding this comment.
[MAJOR] Uninvoked queued/future user rows are not execution boundaries. Breaking on every user row can hide the later assistant response and make wait-peer time out.
Suggested fix:
if (role === "user") {
if (typeof row.invokedAt === "number") break
continue
}| ?? (request.yolo ? resolveHapiYoloPermissionMode(expectedAgent) : undefined) | ||
| const directoryMatches = childMetadata.path === request.directory | ||
| || (request.sessionType === 'worktree' && childMetadata.worktree?.basePath === request.directory) | ||
| const selectionMatches = childMetadata.flavor === expectedAgent |
There was a problem hiding this comment.
[MAJOR] This ready check omits accepted runtime selectors (serviceTier, collaborationMode, copilotAgentMode, and startingMode). If the runner drops one, the remit can still be delivered under unintended execution behavior.
Suggested fix:
&& (request.serviceTier === undefined || child.serviceTier === request.serviceTier)
&& (request.collaborationMode === undefined || child.collaborationMode === request.collaborationMode)
&& (request.copilotAgentMode === undefined || child.copilotAgentMode === request.copilotAgentMode)
&& (request.startingMode === undefined || childMetadata.startingMode === request.startingMode)| message: 'Codex proactive multi-agent mode: on' | ||
| }); | ||
| it('does not intercept /agent with a HAPI prompt mode', () => { | ||
| expect(resolveCodexSlashCommand('/agent', state)).toEqual({ kind: 'passthrough' }); |
There was a problem hiding this comment.
[MINOR] This test makes /agent pass through as a normal prompt, but the shared Codex catalog still advertises it as a built-in delegation toggle.
Suggested fix:
codex: [
{ name: "clear", description: "Clear current Codex thread context", source: "builtin" },
// ...
],There was a problem hiding this comment.
Findings
-
[Major] Worktree selection can be falsely accepted as a simple session —
childMetadata.path === request.directorypasses even when the runner dropssessionType: "worktree"; the remit can then run in the base checkout instead of the requested isolated worktree.worktreeNameis also accepted but never verified. Evidence:hub/src/sync/syncEngine.ts:2221.Suggested fix:
// Persist the runner-applied values in child metadata, including Cursor native worktrees. const sessionTypeMatches = request.sessionType === undefined || childMetadata.sessionType === request.sessionType const worktreeNameMatches = request.worktreeName === undefined || childMetadata.worktreeName === request.worktreeName const selectionMatches = childMetadata.flavor === expectedAgent && directoryMatches && sessionTypeMatches && worktreeNameMatches // existing selector checks...
-
[Major] A later turn can make
wait-peertime out an already-completed remit — the next invoked user row is a definitive boundary for the requested remit, but completion still depends on the session-widethinkingflag. If that later turn is running, the earlier result waits for unrelated work and may time out. Evidence:cli/src/modules/pingPeer/pingPeer.ts:666.Suggested fix:
const { messages, boundaryReached } = extractResultMessages(result.rows, -1) if (result.invoked && messages.length > 0 && (boundaryReached || !live.thinking)) { return buildCompletedResult(messages) }
Summary
Review mode: follow-up after new commits
The three prior findings are fixed. Two Major issues remain: atomic worktree selection is not actually verified, and remit waiting can still depend on a later turn's state.
Testing
- Not run (automation; PR code was not executed).
- Add a spawn test where
sessionType: "worktree"returns a simple-session metadata snapshot, plus a wait test where the next invoked turn hasthinking: true.
HAPI Bot
| const expectedAgent = request.agent ?? 'claude' | ||
| const expectedPermissionMode = request.permissionMode | ||
| ?? (request.yolo ? resolveHapiYoloPermissionMode(expectedAgent) : undefined) | ||
| const directoryMatches = childMetadata.path === request.directory |
There was a problem hiding this comment.
[MAJOR] Worktree requests can pass this check after the runner starts a simple session: for sessionType: "worktree", a child at the base path satisfies the first branch even when no worktree exists. worktreeName is also never checked, so the remit can be delivered under a different isolation/layout than requested.
Suggested fix:
// Persist the runner-applied values in child metadata, including Cursor native worktrees.
const sessionTypeMatches =
request.sessionType === undefined || childMetadata.sessionType === request.sessionType
const worktreeNameMatches =
request.worktreeName === undefined || childMetadata.worktreeName === request.worktreeName
const selectionMatches = childMetadata.flavor === expectedAgent
&& directoryMatches
&& sessionTypeMatches
&& worktreeNameMatches
// existing selector checks...| const result = await getMessagesFromRemit(apiUrl, jwt, sessionId, remitId, http) | ||
| if (result.found) { | ||
| const messages = extractResultMessages(result.rows, -1) | ||
| if (result.invoked && !live.thinking && messages.length > 0) { |
There was a problem hiding this comment.
[MAJOR] This uses the session-wide thinking state even after extractResultMessages has encountered the next invoked user row. That row definitively bounds the requested remit, so if the later turn is still running, waiting for the earlier remit is delayed by unrelated work and can falsely time out.
Suggested fix:
const { messages, boundaryReached } = extractResultMessages(result.rows, -1)
if (result.invoked && messages.length > 0 && (boundaryReached || !live.thinking)) {
return buildCompletedResult(messages)
}There was a problem hiding this comment.
Findings
-
[Blocker] Atomic spawn waits for a readiness event most creatable flavors never emit — after the child becomes active, the new path always waits on
sessionReadyIds. That set is populated only by thesession-readysocket event; production emitters exist for AGY, Pi, and Cursor, while emitters for Claude, Codex, DSH, Copilot, Kimi, Grok, and OpenCode were not found in repo/docs. The defaulthapi spawn-peertherefore waits until the deadline, reportsspawn_timeout, and archives the fresh child without delivering its remit. Evidence:hub/src/sync/syncEngine.ts:2204,cli/src/api/apiSession.ts:1207.Suggested fix:
const flavor = this.getSessionByNamespace(sessionId, namespace)?.metadata?.flavor const requiresReadySignal = flavor === 'agy' || flavor === 'pi' || flavor === 'cursor' const ready = requiresReadySignal ? await this.waitForSessionReady( sessionId, Math.max(1, waitDeadline - Date.now()) ).catch(() => 'timeout' as const) : 'ready'
-
[Major]
stop-peerreports completion before the CLI process has stopped — the RPC handler startsstopAndExit()without awaiting it and immediately acknowledges; this code then marks the session inactive itself. During the CLI's asynchronous cleanup/flush window, a caller can immediately delete or reopen the supposedly stopped row, creating an orphan or duplicate process. Evidence:hub/src/sync/syncEngine.ts:1726,cli/src/claude/registerKillSessionHandler.ts:39,cli/src/agent/runnerLifecycle.ts:153.Suggested fix:
try { await this.rpcGateway.stopSessionProcess(sessionId) } catch (error) { if (!(error instanceof RpcTargetMissingError)) throw error this.handleSessionEnd({ sid: sessionId, time: Date.now(), reason: 'error' }) return { alreadyStopped: false } } if (!await this.waitForSessionInactive(sessionId)) { throw new Error('Timed out waiting for session process to stop') } return { alreadyStopped: false }
Summary
Review mode: follow-up after new commits
The two findings from review 5121194412 are fixed. The latest full diff still has one Blocker in the default atomic spawn path and one Major lifecycle race in process-only stop.
Testing
- Not run (automation; PR code was not executed).
- Add a spawn test using an active Claude/Codex child with no
session-readyevent, and a stop test where the RPC acknowledges before the delayedsession-endevent.
HAPI Bot
| const waitDeadline = Date.now() + (request.waitActiveSecs ?? 60) * 1000 | ||
| const active = await this.waitForSessionActive(sessionId, Math.max(1, waitDeadline - Date.now())).catch(() => false) | ||
| if (!active) return await fail('spawn_timeout', 'New session did not become active') | ||
| const ready = await this.waitForSessionReady(sessionId, Math.max(1, waitDeadline - Date.now())).catch(() => 'timeout' as const) |
There was a problem hiding this comment.
[BLOCKER] This readiness fence is unconditional, but waitForSessionReady can only succeed after a CLI emits session-ready. Production emitSessionReady() callers exist for AGY, Pi, and Cursor; callers for the default Claude path, Codex, DSH, Copilot, Kimi, Grok, and OpenCode were not found in repo/docs. Those flavors become active, then always time out here and get cleaned up before the remit is sent.
Suggested fix:
const flavor = this.getSessionByNamespace(sessionId, namespace)?.metadata?.flavor
const requiresReadySignal = flavor === 'agy' || flavor === 'pi' || flavor === 'cursor'
const ready = requiresReadySignal
? await this.waitForSessionReady(
sessionId,
Math.max(1, waitDeadline - Date.now())
).catch(() => 'timeout' as const)
: 'ready'Alternatively, add a real post-initialization emitSessionReady() boundary to every creatable flavor before keeping this unconditional wait.
| if (!(error instanceof RpcTargetMissingError)) throw error | ||
| reason = 'error' | ||
| } | ||
| this.handleSessionEnd({ sid: sessionId, time: Date.now(), reason }) |
There was a problem hiding this comment.
[MAJOR] The CLI kill handler invokes void stopAndExit() and returns its RPC acknowledgement immediately. This line therefore marks the row inactive while the CLI is still running its asynchronous cleanup and flush. An automated caller can chain stop-peer with delete/reopen and race the live process, producing an orphan or two processes for one session.
Suggested fix:
try {
await this.rpcGateway.stopSessionProcess(sessionId)
} catch (error) {
if (!(error instanceof RpcTargetMissingError)) throw error
this.handleSessionEnd({ sid: sessionId, time: Date.now(), reason: 'error' })
return { alreadyStopped: false }
}
if (!await this.waitForSessionInactive(sessionId)) {
throw new Error('Timed out waiting for session process to stop')
}
return { alreadyStopped: false }There was a problem hiding this comment.
Findings
-
[Major]
stop-peerstill confirms Hub inactivity rather than OS process exit —waitForSessionInactive()can complete as soon as the CLI emitssession-end, but cleanup sends that event before flushing, closing the socket, and callingprocess.exit(cli/src/agent/runnerLifecycle.ts:70-72,153-160). The command can therefore report success while the child is still alive, preserving the delete/reopen race. Evidence:hub/src/sync/syncEngine.ts:1726.
Suggested fix:if (!await this.waitForSessionInactive(sessionId)) { throw new Error('Timed out waiting for session shutdown') } const machineId = session.metadata?.machineId if (!machineId) throw new Error('Cannot confirm session process exit') const status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) if (status === 'still_alive') { throw new Error('Session process is still running') }
-
[Major] Generic
--effortis silently ignored for Codex/OpenCode and unsupported flavors while the response reports it as applied — the new client always sendseffort, whereas the runner only forwards that field for Claude/Grok/Pi/AGY and requiresmodelReasoningEffortfor Codex/OpenCode (cli/src/runner/run.ts:1564-1568). Because the Hub reserves the row with the requestedeffortbefore the child connects (hub/src/sync/syncEngine.ts:2069-2071), the later equality check cannot detect the dropped setting. Evidence:cli/src/modules/spawnPeer/spawnPeer.ts:169.
Suggested fix:const usesModelReasoningEffort = agent === 'codex' || agent === 'opencode' const supportsEffort = ['claude', 'grok', 'pi', 'agy'].includes(agent) if (options.effort && !usesModelReasoningEffort && !supportsEffort) { throw new SpawnPeerError('bad_args', `effort is not supported by ${agent}`) } const body = { // ... ...(options.effort ? usesModelReasoningEffort ? { modelReasoningEffort: options.effort } : { effort: options.effort } : {}) }
-
[Major] Automatic skill installation overwrites user-owned skill configuration/data — a differing regular
SKILL.mdis replaced via rename, and a directory symlink is displaced and then unlinked. Since session bootstrap invokes this installer automatically, merely starting an agent can destroy a user-managed skill path. Evidence:cli/src/modules/common/hapiSessionControlSkill.ts:69-100.
Suggested fix:if (targetDirStat?.isSymbolicLink()) { throw new Error(`Refusing to replace user-managed skill symlink at ${targetDir}`) } const markerPath = join(targetDir, '.hapi-managed') const managed = await readFile(markerPath, 'utf8').catch(() => null) === HAPI_SESSION_CONTROL_SKILL_NAME if (targetStat && installed !== source && !managed) { throw new Error(`Refusing to overwrite existing user skill at ${targetPath}`) } // After installing/updating a HAPI-owned copy: await writeFile(markerPath, HAPI_SESSION_CONTROL_SKILL_NAME, { mode: 0o600 })
Summary
Review mode: follow-up after new commits
Three Major issues remain at head ab334533da857176e19b693835354ddc56ca5009. The previous unconditional readiness-wait issue is no longer present in the current diff. The prior stop-process finding remains unresolved because Hub inactivity is still used as the completion boundary.
Testing
- Not run (automation; PR code was not executed).
- Current package-tests, app-build, test, build, drift-gate, and integration checks are passing.
- Add a stop test that keeps the child PID alive after
session-endand asserts the API does not succeed until the runner observes exit. - Add spawn mapping tests for Codex/OpenCode, supported
effortflavors, and rejection of unsupported flavors. - Add installer tests proving unmanaged regular files and directory/file symlinks are preserved and rejected without mutation.
HAPI Bot
| this.handleSessionEnd({ sid: sessionId, time: Date.now(), reason: 'error' }) | ||
| return { alreadyStopped: false } | ||
| } | ||
| if (!await this.waitForSessionInactive(sessionId)) { |
There was a problem hiding this comment.
[MAJOR] This wait only proves that Hub received session-end; it does not prove the child process exited. runnerLifecycle.close() emits session death before flush/socket close, and process.exit happens later, so stop-peer can still return while the old process is alive.
Suggested fix:
if (!await this.waitForSessionInactive(sessionId)) {
throw new Error('Timed out waiting for session shutdown')
}
const machineId = session.metadata?.machineId
if (!machineId) throw new Error('Cannot confirm session process exit')
const status = await this.rpcGateway.stopRunnerSession(machineId, sessionId)
if (status === 'still_alive') {
throw new Error('Session process is still running')
}Please extend the test to hold the PID alive after session-end; the current inactive-polling test does not cover the OS process-exit boundary.
| agent, | ||
| ...(name ? { name } : {}), | ||
| ...(options.model ? { model: options.model } : {}), | ||
| ...(options.effort ? { effort: options.effort } : {}), |
There was a problem hiding this comment.
[MAJOR] This always sends effort, but the runner ignores that field for Codex/OpenCode (they require modelReasoningEffort) and for unsupported flavors. The Hub's pre-seeded session row then makes the requested value appear successfully applied.
Suggested fix:
const usesModelReasoningEffort = agent === 'codex' || agent === 'opencode'
const supportsEffort = ['claude', 'grok', 'pi', 'agy'].includes(agent)
if (options.effort && !usesModelReasoningEffort && !supportsEffort) {
throw new SpawnPeerError('bad_args', `effort is not supported by ${agent}`)
}
// in body:
...(options.effort
? usesModelReasoningEffort
? { modelReasoningEffort: options.effort }
: { effort: options.effort }
: {})| const installed = targetStat?.isFile() && !targetStat.isSymbolicLink() | ||
| ? await readFile(targetPath, 'utf8').catch(() => null) | ||
| : null | ||
| if (installed !== source) { |
There was a problem hiding this comment.
[MAJOR] An existing differing SKILL.md is overwritten here, while the preceding branch also replaces and unlinks a user-created directory symlink. Because this installer runs during session bootstrap, starting HAPI can destroy user-managed skill configuration/data.
Suggested fix:
if (targetDirStat?.isSymbolicLink()) {
throw new Error(`Refusing to replace user-managed skill symlink at ${targetDir}`)
}
const markerPath = join(targetDir, '.hapi-managed')
const managed = await readFile(markerPath, 'utf8').catch(() => null)
=== HAPI_SESSION_CONTROL_SKILL_NAME
if (targetStat && installed !== source && !managed) {
throw new Error(`Refusing to overwrite existing user skill at ${targetPath}`)
}
// write marker only after HAPI creates/updates its own copyAdd fail-closed tests for unmanaged regular files plus directory/file symlinks, asserting no mutation.
There was a problem hiding this comment.
Findings
-
[Major] Restart recovery loses the Cursor/Pi readiness fence —
spawnRemitOperationis durable, but readiness is kept only in the in-memorysessionReadyIdsset. If the Hub restarts after a fresh Cursor/Pi child emits its one-shotsession-readyand before remit delivery completes, an identical retry finds the active pending child, waits for an event that will not be re-emitted, times out, and compensating cleanup terminates that valid child. Evidencehub/src/sync/syncEngine.ts:2219.
Suggested fix:const hasDurableReady = (): boolean => { const metadata = this.getSessionByNamespace(sessionId, namespace)?.metadata return expectedAgent === 'cursor' ? Boolean(metadata?.cursorSessionId) : expectedAgent === 'pi' ? Boolean(metadata?.piSessionId) : true } const ready = requiresReadySignal ? await this.waitForSessionReady( sessionId, Math.max(1, waitDeadline - Date.now()), hasDurableReady ) : 'ready'
Extend
waitForSessionReadyto accept the durable predicate and check it on each poll. -
[Major] Concurrent first-time skill installs can fail one session — directory creation and ownership-marker creation are separate operations. When two same-flavor children start with no installed skill, the loser of
mkdir(targetDir)can observe the winner's directory before.hapi-managedexists; it then computesmanaged === falseand rejects the launch as user-managed. Every bootstrap now awaits this installer, so a valid concurrent session fails before registration. Evidencecli/src/modules/common/hapiSessionControlSkill.ts:84.
Suggested fix:const lockPath = join(skillRoot, `.${HAPI_SESSION_CONTROL_SKILL_NAME}.install.lock`) await withExclusiveFileLock(lockPath, async () => { // Re-read targetDir, markerPath, and targetPath only after acquiring // the cross-process lock, then run the existing ownership checks/install. await installOrVerifyManagedSkill() })
Reuse/adapt the token-owned cross-process lock pattern already used by
cursorMcpOverlay.ts.
Summary
Review mode: follow-up after new commits
Two Major issues remain at head c6f31b74babb11aef2fca0d1637b77a248e736d4. The prior stop-process, effort-mapping, and unmanaged-skill overwrite findings are no longer present; restart durability and concurrent installer coverage remain incomplete.
Testing
- Not run (automation; PR code was not executed).
- Current package-tests, app-build, test, build, drift-gate, and integration checks pass.
- Add a restart-recovery test with an active pending Cursor/Pi child, durable native session metadata, and an empty
sessionReadyIdsset; the retry must deliver the remit without cleanup. - Add a two-caller installer test that pauses the first caller between target-directory and marker creation; both calls must succeed and resolve the canonical skill.
HAPI Bot
| // so waiting for it here would deadlock the initial remit. | ||
| const requiresReadySignal = expectedAgent === 'pi' || expectedAgent === 'cursor' | ||
| const ready = requiresReadySignal | ||
| ? await this.waitForSessionReady(sessionId, Math.max(1, waitDeadline - Date.now())).catch(() => 'timeout' as const) |
There was a problem hiding this comment.
[MAJOR] This readiness fence is only stored in the in-memory sessionReadyIds set. After a Hub restart, an active pending Cursor/Pi child has durable native identity metadata but will not re-emit its one-shot initial session-ready; the retry times out here and cleanup kills a valid child.
Suggested fix:
const hasDurableReady = (): boolean => {
const metadata = this.getSessionByNamespace(sessionId, namespace)?.metadata
return expectedAgent === 'cursor'
? Boolean(metadata?.cursorSessionId)
: expectedAgent === 'pi' && Boolean(metadata?.piSessionId)
}
const ready = requiresReadySignal
? await this.waitForSessionReady(sessionId, remainingMs, hasDurableReady)
: 'ready'Have waitForSessionReady poll both the live set and this durable predicate. Add a Hub-restart retry test with an empty readiness set.
| await mkdir(targetDir, { mode: 0o700 }) | ||
| created = true | ||
| } catch (error) { | ||
| targetDirStat = await lstat(targetDir).catch(() => null) |
There was a problem hiding this comment.
[MAJOR] The mkdir winner has not written .hapi-managed yet. A concurrent same-flavor bootstrap can lose mkdir, observe this directory, then read no marker and reject the canonical install as user-managed. Because every session bootstrap awaits this function, one valid child can fail nondeterministically.
Suggested fix:
const lockPath = join(skillRoot, `.${HAPI_SESSION_CONTROL_SKILL_NAME}.install.lock`)
await withExclusiveFileLock(lockPath, async () => {
// Re-read directory, marker, and skill state under the cross-process lock.
await installOrVerifyManagedSkill()
})Reuse/adapt the token-owned lock pattern in cursorMcpOverlay.ts, and test two callers with the first paused before marker creation.
There was a problem hiding this comment.
Findings
-
[Major] Process-only stop reports failure after successfully stopping terminal sessions — once the session RPC emits
session-end, the Hub still unconditionally requires a runnerstop-sessionRPC. A normal terminal-started HAPI session can have no runner daemon/handler, so/stopreturns an error after the CLI process has already accepted termination. Evidencehub/src/sync/syncEngine.ts:1727.
Suggested fix:const runnerBacked = session.metadata?.startedBy === 'runner' || session.metadata?.startedFromRunner === true if (inactive && !runnerBacked) return { alreadyStopped: false } // Runner confirmation/fallback is required only for runner-backed children. const status = await this.rpcGateway.stopRunnerSession(machineId, sessionId)
-
[Major] Valid worktree spawns are rejected after runner canonicalization — the runner resolves a source subdirectory to the repository root, slugifies
worktreeName, and can add a collision suffix, but the Hub requires both values to equal the original request. For example,directory=/repo/pkgorworktreeName="Feature X"produces a valid child and thenspawn_selection_mismatch, triggering compensating archive. Evidencehub/src/sync/syncEngine.ts:2248andhub/src/sync/syncEngine.ts:2261; runner behaviorcli/src/runner/worktree.ts:123.
Suggested fix:const directoryMatches = request.sessionType === 'worktree' ? childMetadata.sessionType === 'worktree' && Boolean(childMetadata.worktree?.basePath) : childMetadata.path === request.directory // worktreeName is a runner-normalized hint; do not exact-match it.
-
[Minor] Atomic spawn strips remit boundary whitespace — Zod
.trim()transforms the message before hashing and delivery, so a message file beginning with intentional indentation or ending with a significant newline is not delivered byte-for-byte. Evidenceshared/src/apiTypes.ts:629.
Suggested fix:message: z.string().refine((value) => value.trim().length > 0, { message: 'message must contain non-whitespace' }),
Summary
Review mode: follow-up after new commits
Three issues remain at head c35cc8d3b77f1f9d17120ea25403f785ba4099d9. The two prior findings—restart-ready recovery and concurrent skill installation—are addressed by the new durable predicate and cross-process lock. Residual risk centers on lifecycle behavior outside runner-managed sessions and worktree normalization across the Hub/runner boundary.
Testing
- Not run (automation; PR code was not executed).
- Current package-tests, app-build, test, build, drift-gate, and integration checks pass.
- Add: active terminal session with successful stop RPC and missing machine RPC; worktree source subdirectory plus normalized/colliding name hint; remit with leading indentation and trailing newline.
HAPI Bot
|
|
||
| const machineId = session.metadata?.machineId | ||
| if (!machineId) throw new Error('Cannot confirm session process exit without a machine id') | ||
| const status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) |
There was a problem hiding this comment.
[MAJOR] After the CLI accepts a process-only stop and emits session-end, this path still unconditionally calls the machine runner. Terminal-started HAPI sessions commonly have no runner daemon/handler, so the request returns lifecycle_failed after the process has already been told to exit.
Suggested fix:
const runnerBacked = session.metadata?.startedBy === 'runner'
|| session.metadata?.startedFromRunner === true
if (inactive && !runnerBacked) return { alreadyStopped: false }
// Only runner-backed children require runner confirmation/fallback.
const status = await this.rpcGateway.stopRunnerSession(machineId, sessionId)Add a test with an active terminal session, successful stopSessionProcess + inactive confirmation, and no machine RPC target.
| && (request.copilotAgentMode === undefined || child.copilotAgentMode === request.copilotAgentMode) | ||
| && (request.startingMode === undefined || childMetadata.startingMode === request.startingMode) | ||
| && (request.sessionType === undefined || childMetadata.sessionType === request.sessionType) | ||
| && (request.worktreeName === undefined || childMetadata.worktreeName === request.worktreeName) |
There was a problem hiding this comment.
[MAJOR] worktreeName is a hint, not the resulting identity. The runner slugifies it ("Feature X" → feature-x) and may add a collision suffix; it also resolves a source subdirectory to the repository root. These exact comparisons therefore reject valid children and invoke compensating cleanup.
Suggested fix:
const directoryMatches = request.sessionType === 'worktree'
? childMetadata.sessionType === 'worktree' && Boolean(childMetadata.worktree?.basePath)
: childMetadata.path === request.directory
// Remove the exact request.worktreeName === childMetadata.worktreeName check.Return the resolved child worktree name if callers need it, and test a subdirectory plus a normalized/colliding name hint.
|
|
||
| export const SpawnSessionWithRemitRequestSchema = SpawnSessionRequestSchema.extend({ | ||
| directory: z.string().trim().min(1), | ||
| message: z.string().trim().min(1), |
There was a problem hiding this comment.
[MINOR] .trim() transforms the remit before request hashing and delivery. Message-file input with intentional leading indentation or a significant trailing newline is silently changed, unlike normal session messaging.
Suggested fix:
message: z.string().refine((value) => value.trim().length > 0, {
message: 'message must contain non-whitespace'
}),Add a schema/route test asserting leading and trailing whitespace is preserved.
There was a problem hiding this comment.
Findings
-
[Major] Codex
spawn_peeris not forced through manual approval — the bridge registers this new cross-session write tool but does not assign it a per-tool approval mode. It therefore inherits the server default, which can be configured asauto/otherwise non-prompting, despite the comment claiming it is not auto-approved. Evidencecli/src/codex/utils/buildHapiMcpBridge.ts:108.
Suggested fix:tools.spawn_peer = { approval_mode: 'prompt' }
-
[Major] Runner preflight failures are reported as uncleaned live children — every returned spawn error enters compensating stop, but errors such as an unavailable agent, an out-of-root directory, or a missing worktree base happen before any PID/session is registered. The runner then answers
still_alivefor the reserved ID because it has no verified-exit record, so an ordinary validation failure becomescleanup_failedand leaves an unarchived reservation. Evidencehub/src/sync/syncEngine.ts:2204; related runner pathscli/src/runner/run.ts:522andcli/src/runner/run.ts:1063.
Suggested fix:const failBeforeChild = (errorMessage: string, code?: string): SpawnSessionResult => { if (options.existingSessionId) { rememberVerifiedExit(options.existingSessionId) } return { type: 'error', errorMessage, code } }
Summary
Review mode: follow-up after new commits
Two Major issues remain at head 08034dd4119fcd827172508315f8323e66b4ced1. The three findings from the prior HAPI Bot pass are addressed by the new commits. Residual risk is concentrated in the new cross-session control boundary and failure reconciliation before a child process exists.
Testing
- Not run (automation; PR code was not executed).
- Current integration, test, Android, fixture drift, iOS app-build, and iOS package checks pass.
- Add: Codex config with a non-prompting MCP server default still forces
spawn_peerto prompt; atomic spawn rejected before PID creation returns the original error withcleanedUp: trueand leaves no unarchived reservation.
HAPI Bot
| // ping_peer / inspect_peer are registered on the HTTP MCP server / stdio | ||
| // bridge, but are not auto-approved: they target another session (resume + | ||
| // inject, or read peer histories). | ||
| // ping_peer / inspect_peer / spawn_peer are registered on the HTTP MCP |
There was a problem hiding this comment.
[MAJOR] Omitting spawn_peer from tools does not guarantee a prompt; it inherits mcp_servers.hapi.default_tools_approval_mode. With a non-prompting server default, Codex can create a session and inject its remit without the manual boundary claimed here.
Suggested fix:
tools.spawn_peer = {
approval_mode: 'prompt'
}Add a config test that sets a non-prompting server default and verifies the generated per-tool override.
| error instanceof Error ? error.message : 'Runner failed to create a session' | ||
| ) | ||
| } | ||
| if (result.type === 'error') return await fail(result.code ?? 'spawn_failed', result.message) |
There was a problem hiding this comment.
[MAJOR] A returned runner error can occur before any child PID exists (agent preflight, workspace-root rejection, missing worktree base). Sending that through fail() calls stopRunnerSession, but the runner has neither a tracked PID nor verified-exit evidence and returns still_alive. The CLI then reports cleanup_failed for a validation error and the reserved row remains unarchived.
Suggested fix:
const failBeforeChild = (errorMessage: string, code?: string): SpawnSessionResult => {
if (options.existingSessionId) {
rememberVerifiedExit(options.existingSessionId)
}
return { type: 'error', errorMessage, code }
}Use this for every runner return before process creation, or carry an explicit childStarted: false result to the Hub. Add an integration test for an out-of-root atomic spawn.
There was a problem hiding this comment.
Findings
- [Major] Preserve worktree metadata when reopening — resume calls omit the selection, so the new runner environment reports
simple; existing-session bootstrap does not preservesessionTypeorworktreeName, rewriting archived worktree sessions on reopen. Evidence:cli/src/runner/run.ts:691,cli/src/agent/sessionFactory.ts:127.
Suggested fix:if (metadata.sessionType !== undefined) preserved.sessionType = metadata.sessionType if (metadata.worktreeName !== undefined) preserved.worktreeName = metadata.worktreeName
- [Major] Return the generated remit ID after ambiguous transport exhaustion — after both POST responses are lost, the Hub may have completed the operation, but the error omits the only key that permits an idempotent retry. Re-running the command generates a new key and can create a duplicate child. Evidence:
cli/src/modules/spawnPeer/spawnPeer.ts:195,cli/src/commands/spawnPeer.ts:333.
Suggested fix:throw new SpawnPeerError("spawn_failed", message, remitId) // JSON error output: { ok: false, remitId: error.remitId, error: { code: error.code, message: error.message } }
Summary
Review mode: follow-up after new commits
Two Major issues remain. Current tests do not cover worktree metadata across reopen or two consecutive lost spawn responses.
HAPI Bot
| ?? (cursorNativeWorktree ? worktreeName?.trim() || undefined : undefined); | ||
| extraEnv = { | ||
| ...extraEnv, | ||
| [HAPI_RUNNER_SESSION_TYPE_ENV]: sessionType, |
There was a problem hiding this comment.
[MAJOR] Preserve worktree identity across resume
Resume requests do not pass sessionType or worktreeName, so this exports simple. bootstrapExistingSession() rebuilds metadata from that environment, while pickExistingSessionMetadata() preserves worktree but not sessionType or worktreeName. Reopening an archived worktree therefore persists sessionType: "simple" and drops its name; Cursor-native worktrees may have no HAPI worktree record to recover it from.
Suggested fix:
if (metadata.sessionType !== undefined) preserved.sessionType = metadata.sessionType
if (metadata.worktreeName !== undefined) preserved.worktreeName = metadata.worktreeNameAdd a reopen test with existing worktree metadata and assert both fields survive bootstrap.
| break | ||
| } catch (error) { | ||
| if (attempt === 1) { | ||
| throw new SpawnPeerError('spawn_failed', error instanceof Error ? error.message : String(error)) |
There was a problem hiding this comment.
[MAJOR] Return the generated remit ID on ambiguous failure
If both POST attempts throw, the generated remitId is discarded. The Hub may already have created and delivered the child, but JSON output exposes only the error code/message, so automation cannot retry the same operation; rerunning normally creates a new UUID and can duplicate the child.
Suggested fix:
export class SpawnPeerError extends Error {
constructor(
readonly code: SpawnPeerErrorCode,
message: string,
readonly remitId?: string
) {
super(message)
}
}
throw new SpawnPeerError("spawn_failed", message, remitId)
// command JSON error
{ ok: false, remitId: error.remitId, error: { code: error.code, message: error.message } }Add coverage for two consecutive transport failures with an auto-generated remit ID.
There was a problem hiding this comment.
Findings
- [Major] Preserve retry identity when a ping response is lost —
pingPeergenerates the correlation ID immediately before the POST, but a transport rejection escapes without that ID; the CLI also has no--remit-idretry input. If the Hub stored the message and only the response was lost, rerunning creates a second UUID and can deliver the message twice. Evidence:cli/src/modules/pingPeer/pingPeer.ts:381,cli/src/commands/pingPeer.ts:29,cli/src/commands/pingPeer.ts:115.
Suggested fix:const remitId = options.remitId ?? randomUUID() try { await sendMessage(apiUrl, jwt, matched.id, message, remitId, http) } catch (error) { throw new PingPeerError('send_failed', messageFrom(error), remitId) } // Parse/pass --remit-id and include error.remitId in JSON output.
- [Major] Expose the retryable remit through MCP
spawn_peer— the module now attachesremitIdafter ambiguous spawn failure, but this tool neither accepts a caller-supplied remit nor includeserror.remitIdin its error result. A lost successful response therefore still leaves MCP callers unable to retry idempotently and may create a duplicate child. Evidence:cli/src/claude/utils/startHappyServer.ts:124,cli/src/claude/utils/startHappyServer.ts:367,cli/src/claude/utils/startHappyServer.ts:387.
Suggested fix:remitId: z.string().uuid().optional() // ... const result = await spawnPeer({ ...args, remitId: args.remitId }) // On SpawnPeerError, return error.remitId and instruct retry with that same ID.
Summary
Review mode: follow-up after new commits
The two prior findings are resolved. Two Major correlation failures remain in the newly added peer-control paths.
Testing
- Not run (automation). Current GitHub checks pass. Missing coverage: lost
/messagesresponse retaining a retryable remit, and MCPspawn_peerexposing/reusing the remit after ambiguous failure.
HAPI Bot
| onProgress?.(`sending message (${message.length} chars)...`) | ||
| await sendMessage(apiUrl, jwt, matched.id, message, http) | ||
| const remitId = options.remitId ?? randomUUID() | ||
| await sendMessage(apiUrl, jwt, matched.id, message, remitId, http) |
There was a problem hiding this comment.
[MAJOR] Preserve the remit ID on ambiguous ping failure
The generated ID is created immediately before the POST, but any Axios transport rejection escapes as an unstructured error. The Hub may already have stored this localId; JSON output then omits it, and hapi ping-peer has no --remit-id option, so retrying generates a new UUID and can deliver the same message twice.
Suggested fix:
const remitId = options.remitId ?? randomUUID()
try {
await sendMessage(apiUrl, jwt, matched.id, message, remitId, http)
} catch (error) {
throw new PingPeerError(
'send_failed',
error instanceof Error ? error.message : String(error),
remitId
)
}Add remitId to PingPeerError, parse/pass --remit-id, include it in JSON errors, and cover a lost POST response followed by an idempotent retry.
| const result = await inspectPeer({ | ||
| sessionIdPrefix: args.sessionIdPrefix, | ||
| messageLimit: args.messageLimit, | ||
| const result = await spawnPeer({ |
There was a problem hiding this comment.
[MAJOR] Keep MCP spawn retries on the same remit
spawnPeer() now returns SpawnPeerError.remitId when both responses are lost, but the MCP handler drops that field, and its schema offers no remitId input. If the Hub completed the spawn and only the responses were lost, the model can only invoke the tool again with a fresh generated ID, potentially creating a duplicate child.
Suggested fix:
const spawnPeerInputSchema = z.object({
// existing fields...
remitId: z.string().uuid().optional()
})
const result = await spawnPeer({
// existing fields...
remitId: args.remitId
})When catching SpawnPeerError, include error.remitId in the tool result and tell the caller to reuse it. Add a handler test for ambiguous transport exhaustion.
There was a problem hiding this comment.
Findings
-
[Major] Stdio MCP clients cannot reuse retry IDs - the HTTP MCP tools accept
remitId, but the bridge schemas expose only the other arguments, so Codex/OpenCode retries cannot preserve the correlation key. Evidence:cli/src/codex/happyMcpStdioBridge.ts:207,cli/src/codex/happyMcpStdioBridge.ts:236.Suggested fix:
const remitId = z.string().uuid().optional() // Add to both pingPeerInputSchema and spawnPeerInputSchema: remitId,
-
[Major] Persisted spawn cleanup is not reconciled after a Hub restart -
cleanup-neededis saved before the compensating stop/archive, but only another request with the same remit re-enters that path. A crash in between can leave the child running indefinitely. Evidence:hub/src/sync/syncEngine.ts:2148,cli/src/modules/spawnPeer/spawnPeer.ts:226.Suggested fix:
for (const row of this.store.sessions.listSpawnRemits('cleanup-needed')) { const key = `${row.namespace}:${row.operation.remitId}` if (!this.spawnRemitTails.has(key)) await this.reconcileSpawnRemitCleanup(row) } throw new SpawnPeerError( 'cleanup_failed', `${detail}; child ${data.childSessionId} may still be running`, data.remitId ?? remitId )
-
[Major] A ping retry ID is not bound to its message payload - callers can reuse one
remitIdwith different text; the Hub treats the existinglocalIdas an idempotent retry without comparing content, reports success, and keeps/re-emits the old message. Evidence:cli/src/modules/pingPeer/pingPeer.ts:348, related handling athub/src/sync/messageService.ts:897.Suggested fix:
if (!inserted.inserted) { const stored = isObject(msg.content) && isObject(msg.content.content) ? msg.content.content : null const samePayload = stored?.type === 'text' && stored.text === payload.text && JSON.stringify(stored.attachments ?? []) === JSON.stringify(payload.attachments ?? []) if (!samePayload) throw new Error('localId is already bound to a different message') }
Summary
Review mode: follow-up after new commits
The prior retry-related work is present at the HTTP MCP layer, but the stdio bridge still blocks retry-key reuse. Three Major correctness gaps remain around idempotency and compensating cleanup.
CI test, package-test, integration, build, app-build, and drift-gate checks pass. I did not execute PR code during this automation review. Missing targeted coverage: bridge remitId schema/forwarding; restart from cleanup-needed; cleanup-error JSON retaining remitId; and reuse of one ping key with changed text.
HAPI Bot
|
|
||
| const pingPeerInputSchema: z.ZodTypeAny = z.object({ | ||
| sessionIdPrefix: z.string().trim().min(1).describe(SESSION_ID_PREFIX_PARAM_DESCRIPTION), | ||
| sessionId: z.string().uuid().describe(SESSION_ID_PARAM_DESCRIPTION), |
There was a problem hiding this comment.
[MAJOR] Stdio MCP drops the retry key. The HTTP ping_peer and spawn_peer schemas now accept optional remitId, but neither bridge schema exposes it (the spawn schema starts at line 236). Codex/OpenCode callers therefore cannot reuse the ID returned after an uncertain request, defeating the new idempotent retry contract.
Suggested fix:
const remitId = z.string().uuid().optional()
const pingPeerInputSchema = z.object({
sessionId: z.string().uuid().describe(SESSION_ID_PARAM_DESCRIPTION),
message: z.string().min(1),
remitId
})
// Also add remitId to spawnPeerInputSchema.Add a bridge test that validates and forwards the same UUID for both tools.
| ): Promise<SpawnSessionWithRemitResult> => { | ||
| const cleanupOperation: SpawnRemitOperation = { | ||
| ...operation, | ||
| state: 'cleanup-needed', |
There was a problem hiding this comment.
[MAJOR] cleanup-needed has no restart reconciler. This durable marker is written before stopping/archiving the child, but the only reader is another spawnSessionWithRemit call for the same ID. If the Hub exits after this write, no startup or maintenance path scans the persisted operation, so an active child can remain without its remit. The CLI also omits the retry ID from the cleanup_failed error at cli/src/modules/spawnPeer/spawnPeer.ts:226, making recovery harder.
Suggested fix:
for (const row of this.store.sessions.listSpawnRemits('cleanup-needed')) {
const key = `${row.namespace}:${row.operation.remitId}`
if (!this.spawnRemitTails.has(key)) await this.reconcileSpawnRemitCleanup(row)
}Run this after cache initialization and from maintenance, and preserve data.remitId ?? remitId when constructing SpawnPeerError. Add a restart test with a persisted cleanup-needed child.
| if (options.remitId !== undefined && !providedRemitId) { | ||
| throw new PingPeerError('bad_args', 'an exact remit UUID is required') | ||
| } | ||
| const remitId = providedRemitId ?? randomUUID() |
There was a problem hiding this comment.
[MAJOR] The retry ID is not bound to the ping payload. A caller can retry this remitId with different text. In hub/src/sync/messageService.ts:897, duplicate localId rows are accepted and delivered from the stored row without checking the incoming text/attachments, so the request succeeds while the old message is retained or re-emitted.
Suggested fix:
if (!inserted.inserted) {
const stored = isObject(msg.content) && isObject(msg.content.content)
? msg.content.content
: null
const samePayload = stored?.type === 'text'
&& stored.text === payload.text
&& JSON.stringify(stored.attachments ?? []) === JSON.stringify(payload.attachments ?? [])
if (!samePayload) throw new Error('localId is already bound to a different message')
}Map that conflict to a stable 409-style API error. Compare only user text/attachments, not transient delivery metadata, and add a changed-text retry test.
There was a problem hiding this comment.
Findings
-
[Major] Restart reconciliation skips persisted
pendingspawn operations — the operation is written before spawning and remainspendinguntil after remit storage, but the reconciler handles onlycleanup-neededand retryablefailed. A Hub restart between child creation and completion can therefore leave a live child without its initial remit indefinitely; a restart after remit storage can leave a delivered operation permanently marked pending. Evidence:hub/src/sync/syncEngine.ts:982, with the pending window athub/src/sync/syncEngine.ts:2081andhub/src/sync/syncEngine.ts:2273.Suggested fix:
if (operation.state === 'pending') { const state = this.getQueuedState(session.id, [operation.remitId]) const delivered = state.queuedLocalIds.includes(operation.remitId) || state.indeterminateLocalIds?.includes(operation.remitId) === true || state.invokedLocalMessages.some((item) => item.localId === operation.remitId) const next = delivered ? { ...operation, state: 'completed' as const, updatedAt: Date.now() } : { ...operation, state: 'cleanup-needed' as const, updatedAt: Date.now(), code: 'spawn_interrupted', error: 'Hub restarted before remit delivery completed', cleanedUp: false } if (!this.persistSpawnRemitOperation(session.id, session.namespace, operation, next)) continue if (delivered) continue operation = next }
-
[Major] Inactive lease state is treated as proof that a runner process exited —
activeexpires after 30 seconds without heartbeat, so a temporary Hub/socket outage can leave a live runner child represented by an inactive row. The new early return skips the authoritative machine-levelstopRunnerSessionRPC and reportsalreadyStopped: true. Evidence:hub/src/sync/syncEngine.ts:1741; lease expiry ishub/src/sync/sessionCache.ts:627.Suggested fix:
const session = this.getSession(sessionId) if (!session) return { alreadyStopped: true } const runnerBacked = session.metadata?.startedBy === 'runner' || session.metadata?.startedFromRunner === true if (!session.active && !runnerBacked) return { alreadyStopped: true } if (session.active) { try { await this.rpcGateway.stopSessionProcess(sessionId) inactive = await this.waitForSessionInactive(sessionId) } catch (error) { if (!(error instanceof RpcTargetMissingError)) throw error } } // Runner-backed sessions must still be confirmed through stopRunnerSession.
-
[Minor] A reused
localIdwith a different payload becomes a generic retryable 500 — the new payload check throwsError, while the messages route has no conflict mapping. Peer callers classify every failure assend_failedwith the same remit ID and tell MCP callers to retry, so this deterministic conflict can loop indefinitely. Evidence:hub/src/sync/messageService.ts:904,hub/src/web/routes/messages.ts:144, andcli/src/modules/pingPeer/pingPeer.ts:303.Suggested fix:
export class MessageLocalIdConflictError extends Error {} throw new MessageLocalIdConflictError( 'localId is already bound to a different message payload' ) // In the route: if (error instanceof MessageLocalIdConflictError) { return c.json({ error: error.message, code: 'local_id_conflict' }, 409) }
-
[Minor] Grok documentation still advertises the removed
planmode — the modified support matrix now lists three modes, but the same page still says “four permission modes” and documentsplanatdocs/guide/agents.md:200-205. This leaves conflicting launch guidance.Suggested fix:
Grok exposes three permission modes: - `default` — ... - `auto` — ... - `bypassPermissions` — ...
Summary
Review mode: follow-up after new commits. Two major correctness gaps remain in restart-safe spawning and process termination; two minor consistency/error-contract issues remain.
Testing
Not run (automation; PR code was not executed). Current GitHub build/test checks pass. Add targeted restart tests for pending operations both with and without a stored remit, an inactive-but-live runner stop test, and an HTTP 409 conflict route test.
HAPI Bot
| const retryableFailure = operation.state === 'failed' | ||
| && operation.cleanedUp !== true | ||
| && !operation.orphanSessionId | ||
| if (operation.state !== 'cleanup-needed' && !retryableFailure) continue |
There was a problem hiding this comment.
[MAJOR] Restart reconciliation skips pending operations.
The operation is persisted as pending before spawning and remains so until after remit storage. A Hub restart in that window is ignored here, leaving either a live child without its initial remit or a delivered remit whose operation never reaches completed.
Suggested fix:
if (operation.state === 'pending') {
const state = this.getQueuedState(session.id, [operation.remitId])
const delivered = state.queuedLocalIds.includes(operation.remitId)
|| state.indeterminateLocalIds?.includes(operation.remitId) === true
|| state.invokedLocalMessages.some((item) => item.localId === operation.remitId)
const next = delivered
? { ...operation, state: 'completed' as const, updatedAt: Date.now() }
: { ...operation, state: 'cleanup-needed' as const, updatedAt: Date.now(),
code: 'spawn_interrupted', error: 'Hub restarted before remit delivery completed',
cleanedUp: false }
if (!this.persistSpawnRemitOperation(session.id, session.namespace, operation, next)) continue
if (delivered) continue
operation = next
}Add restart coverage for pending-with-remit completion and pending-without-remit cleanup.
|
|
||
| async stopSession(sessionId: string): Promise<{ alreadyStopped: boolean }> { | ||
| const session = this.getSession(sessionId) | ||
| if (!session?.active) return { alreadyStopped: true } |
There was a problem hiding this comment.
[MAJOR] Inactive does not prove a runner child has exited.
SessionCache.expireInactive flips active after 30 seconds without heartbeat. A live runner process can therefore reach this return during a Hub/socket outage, causing stop-peer to report success without invoking the authoritative machine-level stop RPC.
Suggested fix:
const session = this.getSession(sessionId)
if (!session) return { alreadyStopped: true }
const runnerBacked = session.metadata?.startedBy === 'runner'
|| session.metadata?.startedFromRunner === true
if (!session.active && !runnerBacked) return { alreadyStopped: true }
if (session.active) {
try {
await this.rpcGateway.stopSessionProcess(sessionId)
inactive = await this.waitForSessionInactive(sessionId)
} catch (error) {
if (!(error instanceof RpcTargetMissingError)) throw error
}
}
// Always call stopRunnerSession for runnerBacked sessions and fail on still_alive.The inactive runner-backed test should expect stopRunnerSession to be called and fail closed on still_alive.
| if (storedContent?.type !== 'text' | ||
| || storedContent.text !== payload.text | ||
| || JSON.stringify(storedContent.attachments ?? []) !== JSON.stringify(payload.attachments ?? [])) { | ||
| throw new Error('sendMessage: localId is already bound to a different message payload') |
There was a problem hiding this comment.
[MINOR] Return a stable conflict instead of a retryable 500.
This is a deterministic idempotency-key conflict, but the generic error escapes the messages route as a 500. Peer/MCP callers preserve the remit ID and recommend retrying all send failures, so a changed-payload reuse can retry forever.
Suggested fix:
export class MessageLocalIdConflictError extends Error {}
throw new MessageLocalIdConflictError(
'localId is already bound to a different message payload'
)
// Route:
if (error instanceof MessageLocalIdConflictError) {
return c.json({ error: error.message, code: 'local_id_conflict' }, 409)
}Add a route test asserting HTTP 409 and local_id_conflict.
| | Codex | `hapi codex` | TUI wrapper (local) + `codex app-server` JSON-RPC (remote) | ✓ | ✓ | `default` `read-only` `safe-yolo` `yolo` (+ `plan` collaboration mode) | ✓ | | ||
| | Cursor Agent | `hapi cursor` | ACP (`agent acp`); legacy stream-json resume | ✓ | ✓ | `default` `plan` `ask` `debug` `autoReview` `yolo` | ✓ | | ||
| | Grok Build | `hapi grok` | ACP (`grok agent stdio`) | ✓ | ✓ | `default` `auto` `plan` `bypassPermissions` | ✓ | | ||
| | Grok Build | `hapi grok` | ACP (`grok agent stdio`) | ✓ | ✓ | `default` `auto` `bypassPermissions` | ✓ | |
There was a problem hiding this comment.
[MINOR] The Grok section still documents the removed plan mode.
This matrix now lists three modes, but docs/guide/agents.md:200-205 still says Grok exposes four and includes plan, leaving the page internally inconsistent.
Suggested fix:
Grok exposes three permission modes:
- `default` — ...
- `auto` — ...
- `bypassPermissions` — ...There was a problem hiding this comment.
Findings
No findings.
Summary
Review mode: follow-up after new commits. The prior restart reconciliation, runner-stop confirmation, local-ID conflict handling, and Grok documentation findings are addressed. No new blocker, major, minor, or nit issue was found in the latest full diff. Residual risk remains concentrated in the large cross-runtime session-control rollout and real runner/process integration boundaries.
Testing
Not run (automation; PR code was not executed).
HAPI Bot
Closes #1769.
Summary
hapi-session-controlskill through every creatable flavor's native skill root, with a metadata-onlyskill_lookupfallback for generic ACPThe implementation reuses the safe spawn/remit groundwork from #1511, but moves atomicity and cleanup into one Hub operation and does not adopt its later global peer defaults.
Verification
bun typecheck && bun run test— 6,928 passed, 4 skipped, 0 failedbun run test:cli:integration— 13 passed, 1 skipped; final audit found zero test-owned processesbunx playwright test e2e/composer-copy.spec.ts— 4 passedcd cli && bun run build:exe— single executable built; bundled canonical skill found in the binaryAI disclosure: Codex assisted with source inspection, implementation, testing, and PR preparation. The architecture and acceptance requirements came from #1769 and its operator follow-up.