Fix Codex SDK shell command hydration
This commit is contained in:
@@ -1660,23 +1660,39 @@ function finalizeRolloutFunctionCall(
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const result = parseJsonOrObject<CommandExecutionResult>(payload.output);
|
const result = parseJsonOrObject<CommandExecutionResult>(payload.output);
|
||||||
const exitCode = result?.metadata?.exit_code;
|
const parsedOutput =
|
||||||
|
typeof payload.output === "string"
|
||||||
|
? parseCommandOutputText(payload.output)
|
||||||
|
: null;
|
||||||
|
const exitCode = result?.metadata?.exit_code ?? parsedOutput?.exitCode;
|
||||||
const status =
|
const status =
|
||||||
exitCode === undefined || exitCode === 0 ? "completed" : "failed";
|
exitCode === undefined || exitCode === 0 ? "completed" : "failed";
|
||||||
|
const stdout =
|
||||||
|
result?.stdout ??
|
||||||
|
(typeof result?.output === "string" ? result.output : undefined) ??
|
||||||
|
parsedOutput?.stdout;
|
||||||
|
const metadata =
|
||||||
|
result?.metadata ??
|
||||||
|
(typeof exitCode === "number" ? { exit_code: exitCode } : undefined);
|
||||||
|
|
||||||
// Build structured command output
|
// Build structured command output
|
||||||
const output = result?.stdout
|
let output: unknown;
|
||||||
? {
|
if (stdout !== undefined) {
|
||||||
type: "command" as const,
|
output = {
|
||||||
command: command.command,
|
type: "command" as const,
|
||||||
output: result.stdout,
|
command: command.command,
|
||||||
exitCode,
|
output: stdout,
|
||||||
cwd: command.cwd,
|
exitCode,
|
||||||
metadata:
|
cwd: command.cwd,
|
||||||
result?.metadata ??
|
metadata,
|
||||||
(typeof exitCode === "number" ? { exit_code: exitCode } : undefined),
|
};
|
||||||
}
|
} else if (result && typeof result === "object") {
|
||||||
: result;
|
output = { ...result, metadata: metadata ?? result.metadata };
|
||||||
|
} else if (metadata) {
|
||||||
|
output = { metadata };
|
||||||
|
} else {
|
||||||
|
output = result;
|
||||||
|
}
|
||||||
|
|
||||||
events.push({
|
events.push({
|
||||||
type: "timeline",
|
type: "timeline",
|
||||||
@@ -2116,6 +2132,32 @@ function parseJsonOrObject<T = unknown>(value: unknown): T | null {
|
|||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function parseCommandOutputText(
|
||||||
|
output: string
|
||||||
|
): { exitCode?: number; stdout?: string } | null {
|
||||||
|
if (!output.trim()) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
const exitCodeMatch = output.match(/Exit code:\s*(\d+)/i);
|
||||||
|
const exitCode =
|
||||||
|
exitCodeMatch && exitCodeMatch[1]
|
||||||
|
? Number.parseInt(exitCodeMatch[1], 10)
|
||||||
|
: undefined;
|
||||||
|
|
||||||
|
const outputMatch = output.match(/\nOutput:\s*\n([\s\S]*)$/);
|
||||||
|
const stdout = outputMatch ? outputMatch[1].replace(/\s+$/, "") : undefined;
|
||||||
|
|
||||||
|
if (exitCode === undefined && stdout === undefined) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
return {
|
||||||
|
exitCode: Number.isNaN(exitCode as number) ? undefined : exitCode,
|
||||||
|
stdout,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
function formatCommand(args: unknown): string | null {
|
function formatCommand(args: unknown): string | null {
|
||||||
if (!args || typeof args !== "object") {
|
if (!args || typeof args !== "object") {
|
||||||
return null;
|
return null;
|
||||||
|
|||||||
@@ -175,29 +175,29 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov
|
|||||||
- Decide: keep MCP for tools, or drop it and use SDK only?
|
- Decide: keep MCP for tools, or drop it and use SDK only?
|
||||||
- **Done (2025-12-24 20:04)**: Concluded MCP provider is only necessary for external MCP tool integration; for permissions and core workflows, SDK provider is simpler and already supports approvals. Recommend dropping/parking MCP unless external MCP tool usage is a requirement.
|
- **Done (2025-12-24 20:04)**: Concluded MCP provider is only necessary for external MCP tool integration; for permissions and core workflows, SDK provider is simpler and already supports approvals. Recommend dropping/parking MCP unless external MCP tool usage is a requirement.
|
||||||
|
|
||||||
- [ ] **CRITICAL FINDING**: `codex exec` (SDK) is non-interactive - cannot prompt!
|
|
||||||
|
|
||||||
- Ran `scripts/codex-sdk-permission-test.mjs` with workspace-write + untrusted
|
|
||||||
- NO approval events fire - commands just run or refuse
|
|
||||||
- `codex exec --experimental-json` mode is NON-INTERACTIVE by design
|
|
||||||
- It either refuses ("approvals are disabled") or runs in sandbox
|
|
||||||
- The interactive `codex` CLI is what prompts - but SDK uses `codex exec`
|
|
||||||
- This explains why SDK never emits `exec_approval_request` events
|
|
||||||
- MCP server (`codex mcp-server`) may work differently via MCP elicitation
|
|
||||||
- The SDK provider CANNOT support interactive approvals by design
|
|
||||||
|
|
||||||
- [x] **Plan**: Re-audit based on test results.
|
- [x] **Plan**: Re-audit based on test results.
|
||||||
|
|
||||||
- **Done (2025-12-24 20:06)**: Reviewed latest E2E failures/hang; added fix tasks for Codex SDK hydration and Codex MCP abort hang, plus a retest task.
|
- **Done (2025-12-24 20:06)**: Reviewed latest E2E failures/hang; added fix tasks for Codex SDK hydration and Codex MCP abort hang, plus a retest task.
|
||||||
|
|
||||||
- [ ] **Fix**: Codex SDK persistence hydration should emit completed shell_command tool entries.
|
- [x] **Fix**: Codex SDK persistence hydration should emit completed shell_command tool entries.
|
||||||
|
|
||||||
- Capture the failing rollout entry and ensure hydrated tool calls include completed status + exit code metadata.
|
- Capture the failing rollout entry and ensure hydrated tool calls include completed status + exit code metadata.
|
||||||
|
- **Done (2025-12-24 20:12)**: Parsed rollout command output strings to extract exit codes/stdout and attach metadata to hydrated shell tool results.
|
||||||
|
|
||||||
- [ ] **Fix**: Codex MCP E2E hang in long-running command abort test.
|
- [ ] **Fix**: Codex MCP E2E hang in long-running command abort test.
|
||||||
|
|
||||||
- Add deterministic abort/timeout handling and ensure the session closes even if the sleep tool call is never surfaced.
|
- Add deterministic abort/timeout handling and ensure the session closes even if the sleep tool call is never surfaced.
|
||||||
|
|
||||||
|
- [ ] **CRITICAL FINDING (VERIFIED IN SOURCE)**: `codex exec` IGNORES approval events!
|
||||||
|
|
||||||
|
- In `codex-rs/exec/src/event_processor_with_human_output.rs:568`:
|
||||||
|
- `ExecApprovalRequest` and `ApplyPatchApprovalRequest` are in ignore match arm `=> {}`
|
||||||
|
- `codex exec` receives approval events but DOESN'T emit them as JSON
|
||||||
|
- SDK uses `codex exec` → CANNOT support approvals BY DESIGN
|
||||||
|
- MCP server DOES handle them → sends `ElicitRequest` (see `exec_approval.rs:107`)
|
||||||
|
- CONCLUSION: MCP provider is the ONLY path to real permissions, not SDK
|
||||||
|
- The agent's earlier conclusion to "park MCP" was WRONG
|
||||||
|
|
||||||
- [ ] **Test (E2E)**: Rerun server vitest after fixes.
|
- [ ] **Test (E2E)**: Rerun server vitest after fixes.
|
||||||
|
|
||||||
- If failures: add follow-up fix tasks immediately after this item.
|
- If failures: add follow-up fix tasks immediately after this item.
|
||||||
|
|||||||
Reference in New Issue
Block a user