Harden Codex CUA facade isolation, evidence, and tool logging - #2907
Open
miguelg719 wants to merge 1 commit into
Open
Harden Codex CUA facade isolation, evidence, and tool logging#2907miguelg719 wants to merge 1 commit into
miguelg719 wants to merge 1 commit into
Conversation
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 23 files
Confidence score: 3/5
- In
packages/integrations/core/src/facade/logging.ts, invalid facade logging settings raise a genericErrorduring stdio startup, weakening the expected configuration-error handling; use the existing facade configuration error type. - In
packages/integrations/core/src/facade/logging.ts,redactToolLogleaves image payloads of 256 characters or fewer unchanged, allowing image bytes into debug and error records; omit everydatapayload before applying length-based redaction. - In
packages/integrations/codex-sdk/src/isolation.ts, duplicated environment filtering letsbuildIsolatedCodexEnvand eval callers diverge, creating inconsistent isolated environments; centralize the filtering or have evals consume the returned environment. - In
packages/integrations/codex/README.md, the raw CLI pipeline omits the task prompt, so the documented command cannot reproduce a run whose events are captured; include the instruction argument.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/integrations/core/src/facade/logging.ts">
<violation number="1" location="packages/integrations/core/src/facade/logging.ts:11">
P2: When an image payload is 256 characters or shorter, `redactToolLog` returns it unchanged, so debug and error records can write the image bytes despite the facade logging contract. Omit every `data` payload before applying the preview limit.</violation>
<violation number="2" location="packages/integrations/core/src/facade/logging.ts:34">
P2: Custom agent: **Exception and error message sanitization**
When either facade logging setting is invalid, `createFacadeLogger` raises a generic `Error` during stdio startup. Use the existing `StagehandFacadeConfigError` for both validation failures so callers can identify facade configuration errors consistently.</violation>
</file>
<file name="packages/integrations/codex-sdk/src/isolation.ts">
<violation number="1" location="packages/integrations/codex-sdk/src/isolation.ts:24">
P2: The new environment-filtering logic duplicates `buildIsolatedCodexEnv`, while eval callers discard this function's returned environment and reconstruct it separately. Centralize the filtering or have evals use the returned environment so future isolation changes cannot diverge between the SDK example and evals.</violation>
</file>
<file name="packages/integrations/codex/README.md">
<violation number="1" location="packages/integrations/codex/README.md:104">
P2: This raw CLI pipeline supplies no task prompt, unlike the complete `codex exec` example above, so it cannot reproduce a run whose events are being captured. Include the instruction argument in the command.</violation>
</file>
Architecture diagram
sequenceDiagram
participant C as Codex CLI/Agent
participant SDK as Codex SDK Session
participant FS as File System
participant M as MCP Server (Stagehand Facade)
participant TL as Tool Logger
participant B as Stagehand Browser
participant O as Observation Recorder
participant E as Evals Harness
Note over C,E: Isolated Codex Environment
C->>SDK: runCodexSession(env, allowedMcpServers, diagnosticDir)
SDK->>FS: create isolated HOME/CODEX_HOME
SDK->>FS: copy auth.json only (no plugins/config/thread)
SDK->>M: Connect MCP server (allowlist: stagehand)
SDK->>SDK: Initialize thread (read-only sandbox, on-failure approval)
Note over SDK,M: Tool Call Flow with Logging
C->>SDK: Run streamed events
SDK->>M: CallToolRequest (run/snapshot/screenshot)
M->>TL: tool.start (requestId, name, args)
TL->>TL: Redact credentials, omit images, bound size
M->>B: Execute browser tool
B-->>M: Result
M->>TL: tool.end (durationMs, status, preview)
TL->>TL: Write JSONL to file or stderr (never MCP stdout)
M-->>SDK: Text result (escaped Unicode separators)
Note over SDK,O: Event Handling & Policy
SDK->>SDK: Check mcp_tool_call item
alt Unexpected MCP server
SDK->>SDK: Abort run, throw policy error
SDK->>FS: Save diagnostic artifact (bounded)
else Allowed MCP server
SDK->>O: recordObservation(toolCallId) - await completion
O->>O: Capture probe evidence (screenshot/url/ariaTree)
O-->>SDK: Observation with toolCallId
end
Note over SDK,E: Evidence Matching
SDK-->>E: Session result (events, observations)
E->>E: Key evidence by toolCallId
alt Tool call ID matches observation
E->>E: Attach evidence to specific step
else Missing/unrelated ID
E->>E: Leave step without evidence (no misattribution)
end
Note over SDK,FS: Failure Handling
SDK->>FS: Save diagnostics (best-effort, never masks original error)
SDK-->>C: result (status, diagnosticPath)
C->>FS: Optional diagnostics directory cleanup
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const file = env.STAGEHAND_FACADE_LOG_FILE; | ||
| const level = env.STAGEHAND_FACADE_LOG_LEVEL ?? (file ? "calls" : "off"); | ||
| if (!["off", "calls", "debug"].includes(level)) | ||
| throw new Error("STAGEHAND_FACADE_LOG_LEVEL must be off, calls, or debug."); |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Exception and error message sanitization
When either facade logging setting is invalid, createFacadeLogger raises a generic Error during stdio startup. Use the existing StagehandFacadeConfigError for both validation failures so callers can identify facade configuration errors consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/core/src/facade/logging.ts, line 34:
<comment>When either facade logging setting is invalid, `createFacadeLogger` raises a generic `Error` during stdio startup. Use the existing `StagehandFacadeConfigError` for both validation failures so callers can identify facade configuration errors consistently.</comment>
<file context>
@@ -0,0 +1,115 @@
+ const file = env.STAGEHAND_FACADE_LOG_FILE;
+ const level = env.STAGEHAND_FACADE_LOG_LEVEL ?? (file ? "calls" : "off");
+ if (!["off", "calls", "debug"].includes(level))
+ throw new Error("STAGEHAND_FACADE_LOG_LEVEL must be off, calls, or debug.");
+ const parsedLimit = Number(env.STAGEHAND_FACADE_LOG_MAX_CHARS ?? 16_000);
+ if (!Number.isSafeInteger(parsedLimit) || parsedLimit < 256 || parsedLimit > 1_000_000)
</file context>
| const env = Object.fromEntries( | ||
| Object.entries(source).filter( | ||
| ([key, value]) => | ||
| value !== undefined && (!key.startsWith("CODEX_") || key === "CODEX_API_KEY"), |
Contributor
There was a problem hiding this comment.
P2: The new environment-filtering logic duplicates buildIsolatedCodexEnv, while eval callers discard this function's returned environment and reconstruct it separately. Centralize the filtering or have evals use the returned environment so future isolation changes cannot diverge between the SDK example and evals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/codex-sdk/src/isolation.ts, line 24:
<comment>The new environment-filtering logic duplicates `buildIsolatedCodexEnv`, while eval callers discard this function's returned environment and reconstruct it separately. Centralize the filtering or have evals use the returned environment so future isolation changes cannot diverge between the SDK example and evals.</comment>
<file context>
@@ -0,0 +1,28 @@
+ const env = Object.fromEntries(
+ Object.entries(source).filter(
+ ([key, value]) =>
+ value !== undefined && (!key.startsWith("CODEX_") || key === "CODEX_API_KEY"),
+ ),
+ ) as Record<string, string>;
</file context>
| JSON.stringify(value, (key, item: unknown) => { | ||
| if (/^(?:authorization|cookie|password|secret|token|api[_-]?key|signingKey)$/i.test(key)) | ||
| return "[redacted]"; | ||
| if (key === "data" && typeof item === "string" && item.length > 256) |
Contributor
There was a problem hiding this comment.
P2: When an image payload is 256 characters or shorter, redactToolLog returns it unchanged, so debug and error records can write the image bytes despite the facade logging contract. Omit every data payload before applying the preview limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/core/src/facade/logging.ts, line 11:
<comment>When an image payload is 256 characters or shorter, `redactToolLog` returns it unchanged, so debug and error records can write the image bytes despite the facade logging contract. Omit every `data` payload before applying the preview limit.</comment>
<file context>
@@ -0,0 +1,115 @@
+ JSON.stringify(value, (key, item: unknown) => {
+ if (/^(?:authorization|cookie|password|secret|token|api[_-]?key|signingKey)$/i.test(key))
+ return "[redacted]";
+ if (key === "data" && typeof item === "string" && item.length > 256)
+ return `[binary omitted: ${item.length} characters]`;
+ if (typeof item === "string")
</file context>
| but code, page text and typed values may remain sensitive. Review before sharing | ||
| and rotate files yourself; preview limits do not bound total file size. | ||
|
|
||
| Raw `codex exec --json | tee /tmp/codex-events.jsonl` only saves Codex events. |
Contributor
There was a problem hiding this comment.
P2: This raw CLI pipeline supplies no task prompt, unlike the complete codex exec example above, so it cannot reproduce a run whose events are being captured. Include the instruction argument in the command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/codex/README.md, line 104:
<comment>This raw CLI pipeline supplies no task prompt, unlike the complete `codex exec` example above, so it cannot reproduce a run whose events are being captured. Include the instruction argument in the command.</comment>
<file context>
@@ -65,3 +65,48 @@ service worker — browser-side, never on your machine. Browserbase is the recom
+but code, page text and typed values may remain sensitive. Review before sharing
+and rotate files yourself; preview limits do not bound total file size.
+
+Raw `codex exec --json | tee /tmp/codex-events.jsonl` only saves Codex events.
+It does not enable facade logging or the SDK example's isolated profile. Pass
+the `STAGEHAND_FACADE_LOG_*` variables in `mcp_servers.stagehand.env` when using
</file context>
Suggested change
| Raw `codex exec --json | tee /tmp/codex-events.jsonl` only saves Codex events. | |
| Raw `codex exec --json "your instruction" | tee /tmp/codex-events.jsonl` only saves Codex events. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack
Top-of-stack child of #2906 (
evals/consolidation-17-gemini-cua).Review against that immediate parent, not
main.Summary
Port the remaining focused Codex/Stagehand facade hardening from the experimental
Codex worktree without replacing the consolidation stack's newer shared runtime.
paired starts/ends, timing, errors, bounded result previews and browser readiness.
Never write logs to MCP stdout; redact known credentials and omit image payloads.
parsing failure observed in the Hostelworld benchmark traces.
do not inherit operator plugins/config/thread context. Preserve file-based auth.
tool-call ID so unrelated calls/missing final captures do not shift screenshots.
telemetry without mutating original events.
limitations, authentication, and runnable logging commands.
This remains the existing Codex SDK harness with the Stagehand facade, not a new
native OpenAI computer-use adapter. Preserve the parent's terminal session-loss
semantics, shared facade API, HardBench dataset, verifier and usage contracts.
The older experimental
state/nodeRepl/resetsurface and reconnect implementationare not copied wholesale into the newer shared runtime. The original dirty
worktree remains untouched.
Validation
request IDs and error results while the MCP client remains responsive.
stringification.
git diff --checkpasses.Operational notes
Logging remains opt-in. Logs/error artifacts may contain sensitive model/page
content despite best-effort redaction and must be reviewed before sharing.
Unexpected-server detection aborts observed calls; it is not a security sandbox.
Keychain-only login is not copied into isolated profiles.
Summary by cubic
Hardens Codex facade runs with isolated
HOME/CODEX_HOMEprofiles, opt-in tool logging, and evidence keyed to tool-call IDs so unmatched or unrelated calls no longer misplace screenshots.New Features
STAGEHAND_CODEX_DIAGNOSTICS_DIRorEVAL_CODEX_DIAGNOSTICS_DIR; a failed diagnostic write never masks the original SDK failure.HOME/CODEX_HOMEcreation between the SDK example and evals; only file-basedauth.jsonis copied, never plugins, config, or inherited thread context.Bug Fixes
Written for commit 024b0ed. Summary will update on new commits.