feat: support Codex local goal sync and remote approvals#663
feat: support Codex local goal sync and remote approvals#663SpartaGeorge wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major] Permission hooks now write raw tool payloads to logs —
readJsonBodylogs the complete request body for both hook paths, and the new/hook/permission-requestpath can contain shell commands, MCP arguments, and pasted secrets insidetool_input; HAPI debug logs are persisted locally and can be forwarded when remote debug logging is enabled. Evidencecli/src/claude/utils/startHookServer.ts:86
Suggested fix:async function readJsonBody( req: IncomingMessage, res: ServerResponse, timeoutMs: number, options: { logBody?: boolean } = {} ): Promise<SessionHookData | null> { // ... const body = Buffer.concat(chunks).toString('utf-8'); if (options.logBody) { logger.debug('[hookServer] Received hook:', body); } else { logger.debug('[hookServer] Received hook body'); } // ... } const data = await readJsonBody(req, res, timeoutMs, { logBody: requestPath === '/hook/session-start' });
-
[Major] Remote approvals lose Codex tool details and stable ids — the handler ignores Codex's per-invocation id fields and sends only
description,command,cwd, and hook metadata to HAPI; for MCP approval requests, and any non-command input, the web/mobile approval card cannot show what is being approved, and repeated same-command requests can collide inagentState.requests. Evidencecli/src/codex/codexLocalLauncher.ts:212
Suggested fix:const toolCallId = typeof data.tool_use_id === 'string' && data.tool_use_id.length > 0 ? data.tool_use_id : typeof data.tool_call_id === 'string' && data.tool_call_id.length > 0 ? data.tool_call_id : [ typeof data.session_id === 'string' ? data.session_id : null, typeof data.turn_id === 'string' ? data.turn_id : null, toolName, typeof toolInput.command === 'string' ? toolInput.command : null ].filter(Boolean).join(':') || `${toolName}:${Date.now()}`; const requestInput = { ...toolInput, ...(typeof toolInput.description === 'string' ? { message: toolInput.description } : {}), ...(typeof data.cwd === 'string' ? { cwd: data.cwd } : {}), hookEventName: data.hook_event_name, permissionMode: data.permission_mode };
-
[Minor] Duplicate package entry in lockfile —
bun.locknow contains two identical@twsxtd/hapi-win32-x64package keys, which makes the generated lockfile nondeterministic/noisy and can be collapsed differently by tooling that treats duplicate object keys as last-write-wins. Evidencebun.lock:1070
Suggested fix:- "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="], -
Questions
- None.
Summary
- Review mode: initial
- The permission forwarding path needs two fixes before it is safe/useful for remote approvals: avoid logging raw approval payloads, and preserve the full Codex tool input plus stable tool id. The lockfile also has an accidental duplicate entry.
Testing
- Not run (automation);
bunis not installed in this runner, so I could not verify the PR's Bun test/typecheck commands.
HAPI Bot
| } | ||
|
|
||
| const body = Buffer.concat(chunks).toString('utf-8'); | ||
| logger.debug('[hookServer] Received hook:', body); |
There was a problem hiding this comment.
[MAJOR] This now logs the full request body for /hook/permission-request, which can include shell commands, MCP args, and pasted secrets in tool_input. Please keep full-body logging only for non-sensitive hooks or redact tool_input before writing debug logs.
Suggested fix:
const data = await readJsonBody(req, res, timeoutMs, {
logBody: requestPath === '/hook/session-start'
});| ? data.tool_name | ||
| : 'CodexTool'; | ||
| const toolInput = asRecord(data.tool_input); | ||
| const toolCallId = [ |
There was a problem hiding this comment.
[MAJOR] The HAPI request id is synthesized from session/turn/tool/command and the approval payload later keeps only a few fields from tool_input. That drops MCP/non-command arguments from the web/mobile approval card and can collide for repeated same-command approvals. Prefer Codex's per-invocation id when present and pass the full tool_input through.
Suggested fix:
const toolCallId = typeof data.tool_use_id === 'string' && data.tool_use_id.length > 0
? data.tool_use_id
: typeof data.tool_call_id === 'string' && data.tool_call_id.length > 0
? data.tool_call_id
: fallbackToolCallId;
const requestInput = {
...toolInput,
...(typeof toolInput.description === 'string' ? { message: toolInput.description } : {}),
...(typeof data.cwd === 'string' ? { cwd: data.cwd } : {}),
hookEventName: data.hook_event_name,
permissionMode: data.permission_mode
};|
|
||
| "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="], | ||
|
|
||
| "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="], |
There was a problem hiding this comment.
[MINOR] This duplicates the existing @twsxtd/hapi-win32-x64 package key two lines above. Please regenerate the lockfile or remove the duplicate entry so the lockfile stays deterministic.
Suggested fix:
- "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="],
-
There was a problem hiding this comment.
Findings
-
[Major] Permission hooks log raw approval payloads —
readJsonBodynow logs the complete body for both hook routes, and/hook/permission-requestcan include shell commands, MCP arguments, file paths, or pasted secrets insidetool_input. These CLI debug logs are persisted under~/.hapi/logs, so remote approvals can leak sensitive approval data to local log files. Evidencecli/src/claude/utils/startHookServer.ts:86
Suggested fix:async function readJsonBody( req: IncomingMessage, res: ServerResponse, timeoutMs: number, options: { logBody?: boolean } = {} ): Promise<SessionHookData | null> { // ... const body = Buffer.concat(chunks).toString('utf-8'); if (options.logBody) { logger.debug('[hookServer] Received hook:', body); } else { logger.debug('[hookServer] Received hook body'); } // ... } const data = await readJsonBody(req, res, timeoutMs, { logBody: requestPath === '/hook/session-start' });
-
[Major] Remote approvals drop tool details and use unstable ids — the request id is synthesized from session/turn/tool/command, while the approval payload keeps only
message,command,cwd, and hook metadata. That means MCP/non-command approvals lose their arguments in the web/mobile card, and repeated same-command approvals can overwrite each other inagentState.requests. Evidencecli/src/codex/codexLocalLauncher.ts:212
Suggested fix:const fallbackToolCallId = [ typeof data.session_id === 'string' ? data.session_id : null, typeof data.turn_id === 'string' ? data.turn_id : null, toolName, typeof toolInput.command === 'string' ? toolInput.command : null ].filter(Boolean).join(':') || `${toolName}:${Date.now()}`; const toolCallId = typeof data.tool_use_id === 'string' && data.tool_use_id.length > 0 ? data.tool_use_id : typeof data.tool_call_id === 'string' && data.tool_call_id.length > 0 ? data.tool_call_id : fallbackToolCallId; const requestInput = { ...toolInput, ...(typeof toolInput.description === 'string' ? { message: toolInput.description } : {}), ...(typeof data.cwd === 'string' ? { cwd: data.cwd } : {}), hookEventName: data.hook_event_name, permissionMode: data.permission_mode };
-
[Minor] Duplicate package entry in lockfile —
bun.locknow has two identical@twsxtd/hapi-win32-x64keys. Duplicate object keys are last-write-wins for many parsers and make the lockfile nondeterministic/noisy. Evidencebun.lock:1070
Suggested fix:- "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="], -
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The follow-up commits only touched TLS CN array handling and missing session id validation; I did not find a new high-confidence issue there. The current PR still contains the unresolved remote approval payload/logging issues and duplicate lockfile entry from the full diff.
Testing
- Not run (automation);
bunis not installed in this runner, so I could not runbun typecheckor the PR's test command.
HAPI Bot
| } | ||
|
|
||
| const body = Buffer.concat(chunks).toString('utf-8'); | ||
| logger.debug('[hookServer] Received hook:', body); |
There was a problem hiding this comment.
[MAJOR] This logs the full request body for /hook/permission-request, which can include shell commands, MCP arguments, paths, or pasted secrets in tool_input. Please avoid full-body logging for permission hooks or redact tool_input before writing debug logs.
Suggested fix:
const data = await readJsonBody(req, res, timeoutMs, {
logBody: requestPath === '/hook/session-start'
});| ? data.tool_name | ||
| : 'CodexTool'; | ||
| const toolInput = asRecord(data.tool_input); | ||
| const toolCallId = [ |
There was a problem hiding this comment.
[MAJOR] The HAPI request id is synthesized from session/turn/tool/command, and the approval payload later keeps only a few fields from tool_input. That drops MCP/non-command arguments from the web/mobile approval card and can collide for repeated same-command approvals. Prefer Codex's per-invocation id when present and pass the full tool_input through.
Suggested fix:
const toolCallId = typeof data.tool_use_id === 'string' && data.tool_use_id.length > 0
? data.tool_use_id
: typeof data.tool_call_id === 'string' && data.tool_call_id.length > 0
? data.tool_call_id
: fallbackToolCallId;
const requestInput = {
...toolInput,
...(typeof toolInput.description === 'string' ? { message: toolInput.description } : {}),
...(typeof data.cwd === 'string' ? { cwd: data.cwd } : {}),
hookEventName: data.hook_event_name,
permissionMode: data.permission_mode
};|
|
||
| "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="], | ||
|
|
||
| "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="], |
There was a problem hiding this comment.
[MINOR] This duplicates the existing @twsxtd/hapi-win32-x64 package key two lines above. Please regenerate the lockfile or remove the duplicate entry so the lockfile stays deterministic.
Suggested fix:
- "@twsxtd/hapi-win32-x64": ["@twsxtd/hapi-win32-x64@0.18.3", "", { "os": "win32", "cpu": "x64", "bin": { "hapi": "bin/hapi.exe" } }, "sha512-kEG8tH5OcY/dRBpqbLmb3q2CS0UiM9L3OQX0LWvCn1aI3JQKaaZvzXElCXuwiIml0AI4fBAfofzUB/E31lxffw=="],
-1 participant
Summary
This PR improves Codex local mode integration:
SessionStarthook command invocationPermissionRequesthook forwarding so local mode approvals can be handled from HAPI web/mobileWhy
In Codex local mode,
/goalprogress and conversation details were not fully visible in HAPI web/mobile. Also, when Codex needed userapproval, the approval had to be handled in the local TUI.
This change lets HAPI keep local Codex sessions visible remotely and forwards Codex permission requests through the existing HAPI
approval flow.
Test
bun run typecheck:clibun run test:win -- src/codex/codexLocal.test.ts src/codex/codexLocalLauncher.test.ts src/codex/utils/codexMcpConfig.test.ts src/ claude/utils/startHookServer.test.ts