Skip to content

fix(pi): fail closed on oversized post-tool hook errors#1037

Merged
internet-dot merged 6 commits into
mainfrom
fix/pi-posttool-timeout
Jun 21, 2026
Merged

fix(pi): fail closed on oversized post-tool hook errors#1037
internet-dot merged 6 commits into
mainfrom
fix/pi-posttool-timeout

Conversation

@internet-dot

Copy link
Copy Markdown
Collaborator

Summary

  • fail closed when the Pi managed hook subprocess errors or times out instead of allowing the action
  • truncate Pi post-tool output payloads before invoking Guard so oversized tool output cannot bypass review
  • add regression coverage for the generated managed extension source

Testing

  • python3 -m pytest tests/test_pi_adapter.py -q
  • python3 -m ruff check src/codex_plugin_scanner/guard/adapters/pi_support.py tests/test_pi_adapter.py

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@greptile-apps

greptile-apps Bot commented Jun 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR hardens the Pi harness managed extension to fail closed when the Guard subprocess errors or times out, and adds payload bounding before Guard is invoked so oversized post-tool output cannot cause the hook to stall or be bypassed.

  • Fail-closed on subprocess errors: replaces the old return { decision: \"allow\" } fallback in runGuard with an explicit deny response, including correct detection of ETIMEDOUT error codes and TimeoutError names from Node's spawnSync.
  • Payload bounding for post-tool hooks: adds boundValue, boundedOutputText, and related helpers that cap strings at 12K chars, arrays at 24 items, and objects at 24 keys before the payload reaches hol-guard; also adds a serialized-size cap inside runGuard with a stdout-drop fallback.
  • Silent _coalesce_string bug fixed: commands_support_runtime_artifacts.py previously called _coalesce_string(payload.get(\"stdout\")) for a single-argument call, which returns the sentinel \"unknown-artifact\" when stdout is absent — that string was then appended to response_text and analyzed by classify_secret_content. The replacement with _optional_string returns None when stdout is missing, preventing this false concatenation.

Confidence Score: 4/5

The fail-closed and payload bounding changes are sound and correct; the remaining concern is the Pi hook model using a patched tool result rather than a native abort, which is an API-level limitation that neither this PR nor previous comments have resolved.

The core fail-closed logic in runGuard is correctly implemented: serialization errors, subprocess timeouts (ETIMEDOUT and TimeoutError name are both checked), and non-zero exits all now deny instead of allow. The bounding helpers (boundValue, boundedOutputText) are well-structured — bigint conversion, cycle detection via WeakSet with RAII delete, undefined treated as non-truncated, and unknown types treated as truncated are all handled correctly. The _coalesce_string → _optional_string fix removes a subtle bug where a missing stdout field would cause the literal string 'unknown-artifact' to be appended to the text analyzed by classify_secret_content. The blockedToolResults map correctly uses toolCallIdKey validation and deletes entries in message_end after rewriting. The one open structural gap is that tool_call (PreToolUse) and input (UserPromptSubmit) handlers still send raw unbounded payloads to runGuard — oversized pre-tool arguments will time out and deny (fail closed via the new error handler), but there is no proactive bounding before serialization as there is for tool_result.

src/codex_plugin_scanner/guard/adapters/pi_support.py — the generated TypeScript extension is the main change and warrants the most scrutiny; the tool_call handler has no bounding symmetry with the new tool_result handler.

Important Files Changed

Filename Overview
src/codex_plugin_scanner/guard/adapters/pi_support.py Core change: adds payload bounding helpers (boundValue, boundedOutputText, collectOutputText) and hardens runGuard to deny on subprocess errors/timeouts; fail-closed logic is correct; architectural limitation (patch vs. native Pi abort) remains but is pre-existing.
src/codex_plugin_scanner/guard/cli/commands_support_runtime_artifacts.py One-line fix: replaces _coalesce_string with _optional_string for stdout, preventing the 'unknown-artifact' sentinel from being silently appended to response_text when stdout is absent in the payload.
tests/test_pi_adapter.py Adds two new snapshot-style tests that assert key strings are present in the generated TypeScript extension source; tests verify the fail-closed error handling and payload bounding constants/functions are emitted, but do not test runtime behavior of the generated code.
docs/guard/harness-support.md Documentation update only: adds tool_result to the event surfaces list for the codex, claude-code, and pi harnesses.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Pi as Pi Runtime
    participant Ext as Managed Extension (hol-guard.ts)
    participant Guard as hol-guard subprocess

    Pi->>Ext: tool_result event (event.content, event.toolCallId)
    Ext->>Ext: boundValue(toolInput) → boundedToolInput
    Ext->>Ext: boundValue(event.content) → boundedContent
    Ext->>Ext: boundedOutputText(event.content) → boundedStdout

    alt "any .truncated === true"
        Ext->>Ext: store reason in blockedToolResults[toolCallId]
        Ext-->>Pi: "blockedToolResult(reason) isError=true"
    else all within limits
        Ext->>Ext: build PostToolUse payload (tool_response, stdout)
        Ext->>Ext: JSON.stringify(payload)
        alt serialization fails
            Ext-->>Pi: deny
        else "payload > 24K and stdout removable"
            Ext->>Ext: drop stdout, re-serialize
        end
        alt "still > 24K"
            Ext-->>Pi: deny (size cap)
        else
            Ext->>Guard: "spawnSync(hol-guard, payload, timeout=10s)"
            alt result.error (ETIMEDOUT / other)
                Guard-->>Ext: error
                Ext-->>Pi: deny (fail closed)
            else non-zero exit
                Guard-->>Ext: stderr / status
                Ext-->>Pi: deny
            else allow
                Guard-->>Ext: allow
                Ext-->>Pi: undefined (continue)
            else deny
                Guard-->>Ext: deny + reason
                Ext->>Ext: store reason in blockedToolResults[toolCallId]
                Ext-->>Pi: "blockedToolResult(reason) isError=true"
            end
        end
    end

    Pi->>Ext: "message_end (role=toolResult, toolCallId)"
    alt toolCallId in blockedToolResults
        Ext->>Ext: blockedToolResults.delete(toolCallId)
        Ext-->>Pi: "rewrite message with block reason, isError=true"
    else
        Ext-->>Pi: undefined (no change)
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Pi as Pi Runtime
    participant Ext as Managed Extension (hol-guard.ts)
    participant Guard as hol-guard subprocess

    Pi->>Ext: tool_result event (event.content, event.toolCallId)
    Ext->>Ext: boundValue(toolInput) → boundedToolInput
    Ext->>Ext: boundValue(event.content) → boundedContent
    Ext->>Ext: boundedOutputText(event.content) → boundedStdout

    alt "any .truncated === true"
        Ext->>Ext: store reason in blockedToolResults[toolCallId]
        Ext-->>Pi: "blockedToolResult(reason) isError=true"
    else all within limits
        Ext->>Ext: build PostToolUse payload (tool_response, stdout)
        Ext->>Ext: JSON.stringify(payload)
        alt serialization fails
            Ext-->>Pi: deny
        else "payload > 24K and stdout removable"
            Ext->>Ext: drop stdout, re-serialize
        end
        alt "still > 24K"
            Ext-->>Pi: deny (size cap)
        else
            Ext->>Guard: "spawnSync(hol-guard, payload, timeout=10s)"
            alt result.error (ETIMEDOUT / other)
                Guard-->>Ext: error
                Ext-->>Pi: deny (fail closed)
            else non-zero exit
                Guard-->>Ext: stderr / status
                Ext-->>Pi: deny
            else allow
                Guard-->>Ext: allow
                Ext-->>Pi: undefined (continue)
            else deny
                Guard-->>Ext: deny + reason
                Ext->>Ext: store reason in blockedToolResults[toolCallId]
                Ext-->>Pi: "blockedToolResult(reason) isError=true"
            end
        end
    end

    Pi->>Ext: "message_end (role=toolResult, toolCallId)"
    alt toolCallId in blockedToolResults
        Ext->>Ext: blockedToolResults.delete(toolCallId)
        Ext-->>Pi: "rewrite message with block reason, isError=true"
    else
        Ext-->>Pi: undefined (no change)
    end
Loading

Reviews (7): Last reviewed commit: "docs(guard): sync harness support surfac..." | Re-trigger Greptile

Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
@internet-dot internet-dot force-pushed the fix/pi-posttool-timeout branch from e6f2f38 to 524f5f0 Compare June 21, 2026 17:27
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Signed-off-by: internet-dot <207546839+internet-dot@users.noreply.github.com>
@internet-dot internet-dot force-pushed the fix/pi-posttool-timeout branch from 524f5f0 to 9357c4d Compare June 21, 2026 17:35
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Signed-off-by: internet-dot <207546839+internet-dot@users.noreply.github.com>
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Comment thread src/codex_plugin_scanner/guard/adapters/pi_support.py Outdated
Signed-off-by: internet-dot <207546839+internet-dot@users.noreply.github.com>
Signed-off-by: internet-dot <207546839+internet-dot@users.noreply.github.com>
Signed-off-by: internet-dot <207546839+internet-dot@users.noreply.github.com>
Signed-off-by: internet-dot <207546839+internet-dot@users.noreply.github.com>
@internet-dot internet-dot merged commit 35a6705 into main Jun 21, 2026
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant