Skip to content

RedHatProductSecurity/ai-guardian#2460: [OpenShell] Pin and qualify v0.1.2 compatibility - #2474

Merged
itdove merged 10 commits into
mainfrom
2460
Oct 1, 2026
Merged

itdove merged 10 commits into
mainfrom
2460

Conversation

@itdove

@itdove itdove commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Description

#2460: [OpenShell] Pin and qualify v0.1.2 compatibility

Changes in this branch:

Related issue (if applicable)

Closes #2460 (issue)

Type of change

  • Bug fix
  • New feature
  • Security fix or detection change
  • Documentation or workflow change
  • Refactoring or maintenance

Security impact

  • This change has no security impact.
  • This change affects security behavior; describe the threat, protection, and user-visible behavior below.

Security behavior is implicated by the session context; review the threat and user-visible impact before merging.

Testing

  • Tests added or updated where needed.
  • Related tests pass locally. (test results not captured in session context)
  • Manual verification completed, if applicable. (manual verification not captured in session context)

Evidence from session context:

  • Test files changed: container/tests/README.md, tests/unit/test_context_tokens.py, tests/unit/test_daemon_discovery.py, tests/unit/test_daemon_multi_client.py, tests/unit/test_daemon_traces.py, tests/unit/test_hook_traces.py, tests/unit/test_opencode_support.py, tests/unit/test_opencode_transcript.py, tests/unit/test_sandbox_command.py, tests/unit/test_sandbox_tray.py
  • Test command results were not captured in the session metadata.

Commands run

No command evidence was captured in the session context.

Checklist

  • The change is limited to this PR's purpose.
  • Documentation is updated where needed.
  • CHANGELOG.md is updated for notable changes, or no update is needed.
  • Backward compatibility was considered for the public SDK and configuration. (not independently verified in session context)
  • New dependencies have compatible licenses, if applicable. (no new dependencies identified)
  • UX contract tests are included when user-visible security behavior changes.

itdove and others added 2 commits September 30, 2026 08:47
Remove tracked project hooks with host-specific paths; keep shared Codex feature settings.
🤖 Generated with [OpenCode](https://opencode.ai)

Co-Authored-By: OpenCode (openai/gpt-5.6-luna) <noreply@opencode.ai>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: e9ce0c3a-8545-4778-a307-e52009588eb2

📥 Commits

Reviewing files that changed from the base of the PR and between e109be8 and 4c2cf62.

📒 Files selected for processing (45)
  • .codex/hooks.json
  • .cursor/rules/openwolf.mdc
  • .gitignore
  • .opencode/plugin/openwolf.ts
  • .opencode/plugin/openwolf/anatomy.ts
  • .opencode/plugin/openwolf/fs.ts
  • .opencode/plugin/openwolf/index.ts
  • .opencode/plugin/openwolf/post-read.ts
  • .opencode/plugin/openwolf/post-write.ts
  • .opencode/plugin/openwolf/pre-read.ts
  • .opencode/plugin/openwolf/pre-write.ts
  • .opencode/plugin/openwolf/session.ts
  • .opencode/plugin/openwolf/stop.ts
  • .opencode/plugin/openwolf/types.ts
  • .wolf/STATUS.md
  • .wolf/anatomy.md
  • .wolf/cerebrum.md
  • .wolf/config.json
  • AGENTS.md
  • CHANGELOG.md
  • GEMINI.md
  • README.md
  • container/Dockerfile.openshell
  • container/README.md
  • container/entrypoint.sh
  • container/policies/agents/claude.yaml
  • container/policies/agents/codex.yaml
  • container/policies/agents/pi.yaml
  • container/tests/README.md
  • docs/Sandbox.md
  • src/ai_guardian/daemon/discovery.py
  • src/ai_guardian/daemon/multi_client.py
  • src/ai_guardian/sandbox.py
  • src/ai_guardian/scanners/secret_scanning.py
  • src/ai_guardian/setup/hooks.py
  • src/ai_guardian/tray/menu_builder.py
  • src/ai_guardian/tray/sandbox_dialog.py
  • src/ai_guardian/tui/ask_dialog.py
  • tests/unit/test_container_scripts.py
  • tests/unit/test_daemon_discovery.py
  • tests/unit/test_daemon_multi_client.py
  • tests/unit/test_pi_support.py
  • tests/unit/test_preferred_ui.py
  • tests/unit/test_sandbox_command.py
  • tests/unit/test_sandbox_tray.py
 ________________________________________________________________________________________________________________________
< Don't assume it - prove it. Prove your assumptions in the actual environment - with real data and boundary conditions. >
 ------------------------------------------------------------------------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@read-the-docs-community

read-the-docs-community Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Documentation build overview

📚 ai-guardian | 🛠️ Build #34865438 | 📁 Comparing 4c2cf62 against latest (e109be8)

  🔍 Preview build  

4 files changed
± index.html
± Sandbox/index.html
± project/changelog/index.html
± project/container/index.html

Update stale container expectations, prevent test UI prompts, and apply required Black formatting.\n\nRefs #2460
@itdove

itdove commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review: #2474 — merge-base e109be88..HEAD (branch 2460, commit 4f8dc0b)

Verdict: NEEDS_CHANGES

Scores

Lens Score Findings
Functionality 9.0/10 1 Major
Security 5.0/10 1 Critical, 3 Major
Quality 8.0/10 4 Minor
Overall 7.3/10

Note: the Security sub-agent self-reported "6/10"; recomputed here from its own surviving deductions (max(1, 10 - (2+1+1+1))), which is 5.0/10, not 6.0. Overall = arithmetic mean of the three corrected lens scores, rounded to one decimal.

Scope note

This review covers git diff e109be88 HEAD — the actual diff of PR #2474 (42 files, +1285/-1590), i.e. the OpenShell v0.1.2 pin/qualification work plus the small .codex/hooks.json cleanup commit. It does not re-review the large prior-release-cycle diff (v1.18.0..HEAD, 396 files) covered by this repo's only earlier stored review on this branch (2026-09-30-edba298) — that review's scored findings are all in files untouched by this PR and are not re-verified here. One loose thread from that session's scope note (a narrower, unpersisted review of the then-in-progress OpenShell migration reportedly flagging duplicate Vertex env vars, a CLI-scoped provider-readiness race, and over-broad Vertex policy binding in sandbox.py) was handed to both agents as a hint to verify fresh against the current diff — all three were independently confirmed as real, and are scored below (Quality Minor #2, Functionality Major #1, Security Major #3 respectively).

Findings

Critical

  1. Security container/entrypoint.sh:368 Plaintext provider API key passed as a command-line argument to a subprocess in _configure_pi_openshell_models, exposing it via /proc/<pid>/cmdline/ps to any co-resident process in the sandbox.
    • Evidence: api_key="${OPENAI_API_KEY:-unused}" (or the Anthropic equivalent) then python3 - "$models_path" "$provider_name" "$base_url" "$api_key" <<'PY'
    • Confidence: HIGH
    • Fix: Export the value as an environment variable consumed by the heredoc (e.g. PI_MODEL_API_KEY="$api_key" python3 - "$models_path" "$provider_name" "$base_url" <<'PY', read via os.environ.pop("PI_MODEL_API_KEY")), mirroring the argv-safe pattern already used for Codex credentials in sandbox.py's _ensure_openshell_cli_provider (credential name only in argv, value via env=).
    • Points: 2

Major

  1. Functionality src/ai_guardian/sandbox.py:2362,2554-2565 The post-creation Vertex/Google-Cloud provider readiness wait is gated on the literal selected CLI (args.cli == "claude"), not on whether a Vertex provider was actually attached (vertex_provider_required, which can also be true for cli == "opencode"/cli == "pi", e.g. opencode --agent claude or Pi's Anthropic-Vertex path). Those CLIs get the same GCE_METADATA_HOST=127.0.0.1:8174 emulator environment but skip _wait_for_openshell_provider's --wait.

    • Evidence: vertex_provider_required = inference_cli == "claude" and bool(project) sets up the shared Vertex environment for any cli, but the wait is gated by if (runtime == OPENSHELL_RUNTIME and vertex_project and getattr(args, "cli", None) == "claude"): ... _wait_for_openshell_provider(...).
    • Confidence: HIGH for the mismatch; MEDIUM for the exact runtime failure mode (depends on OpenShell's own gateway timing).
    • Fix: Gate the wait on whether _openshell_create actually attached the Google Cloud/Vertex provider (return that flag alongside name), not on the literal cli value, so opencode/pi sessions on the same provider path also wait.
    • Points: 1
  2. Security container/policies/agents/claude.yaml:34-36 curl and bash were added to the single flat claude_code network policy's binaries allowlist, granting both binaries permanent (not install-scoped) access to all five endpoints in that policy — including read-write access to api.anthropic.com/sentry.io — for the sandbox's whole lifetime, not just the one-time TOS-gated curl | bash install in entrypoint.sh.

    • Evidence: binaries: [{ path: /usr/bin/curl }, { path: /usr/bin/bash }, { path: /bin/bash }, ...]
    • Confidence: MEDIUM (relies on the OpenShell gateway enforcing network access per-binary-per-policy; inferred from schema layout, not independently runtime-verified)
    • Fix: Scope the install-time fetch to a dedicated network policy limited to claude.ai/downloads.claude.ai (already read-only) bound only to curl; do not grant bash (or curl) access to the read-write api.anthropic.com/sentry.io endpoints — bash doesn't need network egress for a curl | bash pipe.
    • Points: 1
  3. Security container/policies/agents/codex.yaml tls: terminate was removed from all four Codex network endpoints (api.openai.com, auth.openai.com, chatgpt.com, ab.chatgpt.com) with no CHANGELOG entry, no test coverage, and no matching update to two sibling OpenShell policy files (container/openshell-github-readonly-policy.yaml, container/openshell-github-readwrite-policy.yaml) that still use tls: terminate and are untouched by this PR — evidence the key remains valid in v0.1.2 and this is a deliberate, undocumented narrowing of gateway TLS inspection/enforcement for Codex egress rather than a forced compatibility fix.

    • Evidence: - host: api.openai.com\n port: 443\n protocol: rest\n- tls: terminate\n enforcement: enforce\n access: read-write (repeated ×4)
    • Confidence: MEDIUM (no documentation found anywhere in the repo explaining what tls: terminate controls or why only codex.yaml lost it)
    • Fix: Restore tls: terminate on the four endpoints unless a documented v0.1.2-specific reason requires dropping it for this policy shape; if it must be removed, add a CHANGELOG/docs note on the enforcement trade-off and a regression test.
    • Points: 1
  4. Security src/ai_guardian/sandbox.py:1636-1637 _add_openshell_vertex_policy_binding unconditionally adds two hardcoded Claude-specific binary paths to the Vertex credential-bound network policy regardless of the cli parameter, so opencode/pi Vertex-attached sandboxes also get Vertex read-write network-credential binding on Claude-specific paths unrelated to the selected CLI.

    • Evidence: "binaries": [{"path": f"/usr/bin/{cli}"}, {"path": f"/usr/local/bin/{cli}"}, {"path": f"/sandbox/.local/bin/{cli}"}, {"path": "/sandbox/.claude/downloads/**"}, {"path": "/sandbox/.local/share/claude/**"}]
    • Confidence: HIGH for the code defect; LOW for present-day exploitability — per container/tests/README.md's qualification table, both Vertex-dependent rows (claude-vertex, opencode-claude-vertex) are currently blocked upstream (fix(providers): restore supervisor-backed GCP metadata discovery NVIDIA/OpenShell#3973), and this binding is unreachable for the two combinations that did pass qualification (Codex, OpenCode+OpenAI).
    • Fix: Only append the two Claude-specific path entries when cli == "claude".
    • Points: 1

Minor

  1. Quality src/ai_guardian/setup/hooks.py:4353-4546 (embedded Pi extension TS) isProviderCredentialField and providerPayloadForScan were both actively extended in this diff, but their only call site — pi.on("before_provider_request", ...) — was gutted to a bare return event.payload; passthrough in the same diff, leaving both functions (and restoreProviderCredentials) dead code that was edited as if still live.

    • Evidence: isProviderCredentialField gains normalized.endsWith("apikey") || normalized.endsWith("token") || [...]; providerPayloadForScan gains a system/developer role-skip branch; but before_provider_request becomes pi.on("before_provider_request", async (event, ctx) => { /* ... */ return event.payload; });
    • Confidence: HIGH
    • Fix: Either delete the now-dead helpers/branches, or state explicitly (comment + CHANGELOG) that provider-request scanning is intentionally disabled. (Security impact of disabling the scan is covered by the Security lens's Needs Human Judgment item below.)
    • Points: 0.5
  2. Quality src/ai_guardian/sandbox.py:2398-2399,2454-2455,2465-2466 When vertex_provider_required is true and cli is opencode or pi, ANTHROPIC_VERTEX_PROJECT_ID/CLOUD_ML_REGION are appended to environment twice — once in the shared Vertex block, again in the CLI-specific blocks. Values are identical, so no divergence bug, but it's redundant and relies on OpenShell tolerating duplicate --env flags.

    • Evidence: environment.extend([f"ANTHROPIC_VERTEX_PROJECT_ID={project}", f"CLOUD_ML_REGION={region}", ...]) (shared block) then again inside if cli == "opencode" ... and if cli == "pi" ... blocks.
    • Confidence: HIGH for the duplication; MEDIUM for whether OpenShell's CLI rejects duplicate --env flags (unverifiable from this repo — if it does, this becomes Major).
    • Fix: Drop the redundant environment.extend(...) calls in the CLI-specific blocks since the shared block already adds them whenever vertex_provider_required is true.
    • Points: 0.5
  3. Quality README.md:299-330,357-358 Root README.md's OpenShell integration section still documents the retired inference.local virtual-routing mechanism as current, tested behavior — directly contradicting this diff's own code and its sibling doc updates (container/README.md, docs/Sandbox.md) in the same PR. It even instructs "Do not set CLAUDE_CODE_USE_VERTEX=1 inside an OpenShell sandbox," while this diff's sandbox.py now does exactly that for cli == "claude".

    • Evidence: configures the workspace's inference.local route, and supplies Claude only the non-secret client settings... / Do not set CLAUDE_CODE_USE_VERTEX=1 inside an OpenShell sandbox... vs. the diff's if cli == "claude": environment.append("CLAUDE_CODE_USE_VERTEX=1").
    • Confidence: HIGH
    • Fix: Update README.md's OpenShell/Vertex sections to match container/README.md's v0.1.2 rewrite (native CLAUDE_CODE_USE_VERTEX=1/ANTHROPIC_VERTEX_PROJECT_ID/CLOUD_ML_REGION, no inference.local).
    • Points: 0.5
  4. Quality tests/unit/test_pi_support.py:512-527 (untouched by this diff, but now stale because of it) test_pi_extension_template_contains_all_managed_events still asserts "providerPayloadForScan", "restoreProviderCredentials", and 'pi.on("before_provider_request"' are present in the extension source — all still pass (the dead code/registration is still textually present) but no longer verify that before_provider_request actually scans/blocks anything, giving false confidence of coverage for that behavior.

    • Evidence: assert "providerPayloadForScan" in _PI_EXTENSION_TS / assert "restoreProviderCredentials" in _PI_EXTENSION_TS unchanged while the diff turned the sole caller into return event.payload;.
    • Confidence: HIGH
    • Fix: Assert on before_provider_request's actual (now passthrough) behavior instead of substring presence of dead helpers.
    • Points: 0.5

Nit

(none)

Score Trajectory

(Run 1 reviewed a different, much larger diff range on this branch — see Scope note above — so this trajectory compares across scope changes, not iterations of the same fix set.)

Run 1 (different scope): █████████████████████████████░░░░░░░░░░░░░░░░░░░░░  3.0/10
Run 2 (this PR's diff):  █████████████████████████████████████░░░░░░░░░░░░░  7.3/10
Target:                  ██████████████████████████████████████████████████  10/10

Needs Human Judgment

  • tls: terminate semantics (container/policies/agents/codex.yaml, Security Major Add CLI command to setup IDE hooks with remote config support #3): no documentation found in this repo for what this key controls in OpenShell's policy schema, or why v0.1.2 required dropping it only for Codex. Needs confirmation against OpenShell 0.1.2 release notes/schema docs — is this a real enforcement narrowing, or a forced schema-compatibility change?
  • before_provider_request no-op + providerPayloadForScan's system/developer-role skip (Quality Minor Multi-IDE Support: Expand ai-guardian to GitHub Copilot, VS Code, Aider, and MCP #1): does any other still-active Pi hook scan the user-authored portion of the outbound provider request specifically (as opposed to the user's original input before assembly into a provider payload)? If the aggregate payload scan was the only place catching secrets/PII introduced between user input and the provider call, this may be a real coverage gap, not just dead code — could not be fully traced from the diff alone.
  • Vertex/Claude readiness race and over-broad binary binding (Functionality Major Multi-IDE Support: Expand ai-guardian to GitHub Copilot, VS Code, Aider, and MCP #1, Security Major itdove/ai-guardian#1: Multi-IDE Support: Expand ai-guardian to GitHub Copilot, VS Code, Aider, and MCP #4): both are currently gated behind an admittedly-broken upstream Vertex/Claude path (fix(providers): restore supervisor-backed GCP metadata discovery NVIDIA/OpenShell#3973 per container/tests/README.md). Confirm whether that upstream issue blocks all reachable paths into this code, or only some — if a Vertex + non-Claude-CLI path is reachable today despite the tracked bug, both findings' urgency should be raised.
  • Whether OpenShell's own sandbox create CLI fails loudly or silently reuses/attaches to an existing sandbox when two invocations race on the same auto-derived name, now that _resolve_sandbox_name returns the raw name unconditionally for OpenShell with no local collision check. ai-guardian's own side propagates a nonzero exit loudly; OpenShell's own collision semantics aren't visible in this repo.

Verification Results

Check Command Output Result
Deleted-file sweep: .codex/hooks.json (repo-tracked file removed by this PR) grep -rn "\.codex/hooks\.json" ... All remaining hits are the distinct, still-supported ~/.codex/hooks.json (global Codex CLI hooks config referenced by setup/hooks.py, tools/patterns.py, tools/policy.py, scanners/supply_chain.py) PASS — no stale references to the deleted repo file
New subprocess.run in discovery.py has timeout/list-form/rc handling sed -n '1016,1061p' src/ai_guardian/daemon/discovery.py list-form argv, timeout=timeout, check=False, explicit except (OSError, subprocess.TimeoutExpired) and returncode == 0 check PASS
Codex credential passed via env, not argv (contrast with entrypoint.sh Critical #1) git diff ... -- src/ai_guardian/sandbox.py --credential OPENAI_API_KEY (name only) in argv; value passed via env=provider_environment PASS
tls: terminate still used elsewhere, unmodified by this PR grep -n tls container/openshell-github-read*-policy.yaml; diff of those files Both still contain tls: terminate; diff against them is empty Supports Security Major #3
No test coverage for tls removal or curl/bash binary scope grep -n tls tests/unit/test_container_scripts.py tests/unit/test_sandbox_command.py no output Confirms untested change
_add_openshell_vertex_policy_binding writes only to its own temp policy file sed -n '1051,1093p' src/ai_guardian/sandbox.py policy_path = temporary_dir / "policy.yaml" from tempfile.mkdtemp, os.chmod(policy_path, 0o600) PASS — no arbitrary-path write
YAML parsing in new Vertex/GCP functions uses safe load/dump grep -n "yaml.safe_load|yaml.safe_dump" src/ai_guardian/sandbox.py both present in _add_openshell_vertex_policy_binding PASS
Proxy-bypass in multi_client.py scoped to .openshell.localhost only sed -n '1432,1472p' src/ai_guardian/daemon/multi_client.py if check_host.lower().endswith(".openshell.localhost") PASS — narrowly scoped
ask_dialog.py headless-before-tray reorder matches test, no regression git diff ... -- src/ai_guardian/tui/ask_dialog.py tests/unit/test_preferred_ui.py headless check now precedes tray forwarding; test adds mock_tray.assert_not_called() PASS
claude.yaml grants match entrypoint.sh's new curl | bash install flow git diff ... -- container/policies/agents/claude.yaml container/entrypoint.sh new claude.ai/downloads.claude.ai (read-only) + curl/bash binaries added alongside the install line PASS for functional consistency (Security still flags the scope as Major #2)
test_container_scripts.py's stale AI_GUARDIAN_OPEN_SHELL_INFERENCE env lines are pre-existing, not new git diff e109be88..HEAD -- tests/unit/test_container_scripts.py filtered for that string no +/- matches Confirmed out of scope (Incidental Finding)
Sandbox-name collision tests document the new behavior accurately (not trivially passing) git diff ... -- tests/unit/test_sandbox_command.py (naming tests) test_resolve_sandbox_name_preserves_openshell_name_on_collision/_preserves_long_openshell_name patch _sandbox_name_exists and assert exists.assert_not_called() PASS — accurately asserts new behavior
No prompt-injection strings found in diff/commit messages git diff ... | grep -niE "ignore (all|previous|the above)|you are now|score this|do not report|reviewed by the security team|no findings|score 10" no output PASS

Incidental Findings (out of scope — not scored)

  • src/ai_guardian/sandbox.py's pre-existing _run() helper (unmodified by this diff) has no timeout on any subprocess.run/Popen call. This diff adds several new call sites through it (_wait_for_openshell_provider's --wait call, the new Google Cloud provider create/update calls) — a hung OpenShell gateway could block sandbox create indefinitely. Pre-existing infrastructure reused by new code; caller is a trusted local operator CLI, not attacker-reachable.
  • tests/unit/test_container_scripts.py:943,996,1057,1123,1183,1240 still set "AI_GUARDIAN_OPEN_SHELL_INFERENCE": "true" in test environment dicts — confirmed unchanged context in this diff, now referencing a variable entrypoint.sh no longer reads. Dead test setup, should be cleaned up in a follow-up.
  • src/ai_guardian/tray/menu_builder.py:806,1263 contain two other pre-existing "claude" fallback defaults, unmodified by this diff, that are now inconsistent with the new primary "codex" default established elsewhere in the same file. Reachability (whether the required cli tray field can ever actually be empty) is uncertain from static inspection; flagged for follow-up.
  • AGENTS.md line ~1615 (pre-existing, unchanged in this diff) is flagged by AI Guardian's own secret scanner as secret_detected/critical — a documentation paragraph listing credential-prefix format examples, not a real secret. Out of this diff's scope.

Observations

  • Duplicate Vertex env vars, CLI-scoped readiness race, and over-broad Vertex policy binding — all three items hinted at by the prior (differently-scoped) review session were independently re-verified fresh against this diff by both sub-agents and are scored above (Quality Minor Add AI-Guardian logo images to project and README #2, Functionality Major Multi-IDE Support: Expand ai-guardian to GitHub Copilot, VS Code, Aider, and MCP #1, Security Major itdove/ai-guardian#1: Multi-IDE Support: Expand ai-guardian to GitHub Copilot, VS Code, Aider, and MCP #4 respectively) rather than carried over as unverified claims.
  • Neither sub-agent found any prompt injection attempt in the diff, PR description, or commit messages.
  • container/entrypoint.sh's curl -fsSL https://claude.ai/install.sh | sh → | bash change has no stated rationale in the diff/commit messages; plausibly a shell-compatibility fix given the base-image swap to raw Ubuntu (whose /bin/sh is dash). The pre-existing pattern of piping remote content to a shell is unchanged by this diff and was not additionally flagged.
  • docs/notebooklm-export.md contains the same stale inference.local passages as README.md but is explicitly an auto-generated combined doc export — it will pick up the fix once README.md is corrected and the export regenerated; not treated as a separate finding.
  • src/ai_guardian/daemon/multi_client.py's new .openshell.localhost host check is not explicitly guarded against None, but every current DaemonTarget construction site makes that unreachable today (dataclass default host="127.0.0.1", OpenShell targets set an explicit URL) — investigated and not scored (materiality fails).
  • pi.yaml's new registry.npmjs.org (read-only) + npm binaries addition is appropriately scoped, in contrast with claude.yaml's broader curl/bash grant (Security Major Add AI-Guardian logo images to project and README #2).

Limitations

This review was performed by two AI sub-agents (one Security-lens, one Functionality+Quality-lens) plus a coordinator merge pass, against PR #2474's actual diff (42 files). It does not understand OpenShell's own gateway-side enforcement semantics for network policies (tls: terminate, per-binary binding) or CLI-side collision/exit-code behavior for sandbox create — several findings and Needs-Human-Judgment items depend on that external, unverifiable-from-this-repo behavior. It does not understand business context, domain intent, organizational constraints, or deployment environment specifics. LOW/MEDIUM-confidence findings and items in "Needs Human Judgment" require human verification. This is a first pass, not a final approval — final approval belongs to human maintainers.

Validation Summary

Lens Proposed Removed Converted Surviving
Security 4 (self-validated by sub-agent) 0 0 4 (1 Critical, 3 Major)
Functionality 1 (self-validated by sub-agent) 0 0 1 (Major)
Quality 4 (self-validated by sub-agent) 0 0 4 (Minor)

Path to 10/10

  1. [Security] container/entrypoint.sh:368 (+2 points)

    Stop passing "$api_key" as a positional argument to the python3 - subprocess in _configure_pi_openshell_models. Export it as an environment variable (e.g. PI_MODEL_API_KEY) right before the heredoc call and read it via os.environ.pop("PI_MODEL_API_KEY", "unused") instead of sys.argv.

  2. [Security] container/policies/agents/claude.yaml:34-36 (+1 point)

    Remove bash from the claude_code network policy's binaries list; split the claude.ai/downloads.claude.ai install-time fetch into its own network policy scoped only to curl, so curl's access doesn't extend to the read-write api.anthropic.com/sentry.io endpoints.

  3. [Security] container/policies/agents/codex.yaml (+1 point)

    Restore tls: terminate on the four Codex endpoints to match the still-active usage in the sibling openshell-github-read*-policy.yaml files, unless OpenShell 0.1.2 docs confirm this key must be dropped for this schema shape — in which case add a CHANGELOG entry documenting the enforcement trade-off.

  4. [Functionality] src/ai_guardian/sandbox.py:2554-2565 (+1 point)

    Change the post-creation provider-wait gate in _create() from getattr(args, "cli", None) == "claude" to a signal reflecting whether _openshell_create actually attached the Google Cloud/Vertex provider, so opencode/pi sessions on the same Vertex path also wait.

  5. [Security] src/ai_guardian/sandbox.py:1636-1637 (+1 point)

    In _add_openshell_vertex_policy_binding, wrap the two hardcoded /sandbox/.claude/downloads/** and /sandbox/.local/share/claude/** binary entries in if cli == "claude": so non-Claude CLIs don't get Claude-specific paths bound to their Vertex credential.

  6. [Quality] src/ai_guardian/setup/hooks.py:4353-4546 (+0.5 points)

    Remove the now-dead providerPayloadForScan/restoreProviderCredentials/unused isProviderCredentialField branches, or restore a real scan in before_provider_request if disabling it was unintentional; document the decision in CHANGELOG.md either way.

  7. [Quality] src/ai_guardian/sandbox.py:2454-2455,2465-2466 (+0.5 points)

    Remove the redundant ANTHROPIC_VERTEX_PROJECT_ID/CLOUD_ML_REGION environment.extend(...) calls in the opencode/pi Vertex blocks — the shared block already adds them whenever vertex_provider_required is true.

  8. [Quality] README.md:290-360 (+0.5 points)

    Update the OpenShell/Vertex sections to match container/README.md's v0.1.2 rewrite (native CLAUDE_CODE_USE_VERTEX=1/ANTHROPIC_VERTEX_PROJECT_ID/CLOUD_ML_REGION, no inference.local).

  9. [Quality] tests/unit/test_pi_support.py:512-527 (+0.5 points)

    Replace the presence-only assertions for the dead helpers with an assertion on before_provider_request's actual (passthrough) behavior.

Final Score

Overall: 7.3/10 — NEEDS_CHANGES

Keep provider credentials in the OpenShell gateway, tighten policy bindings, wait for all Vertex provider paths, remove dead Pi payload code, and align documentation and tests.\n\nRefs #2460
Require explicit gateway provider attachment for gateway-only OpenCode credentials and document existing-sandbox recovery.\n\nRefs #2460
Automatically select ai-guardian-codex for OpenShell OpenCode sessions using OpenAI-shaped models when no provider is entered.\n\nRefs #2460
OpenShell v0.1.2 rejects tls: terminate; omit unsupported TLS fields while retaining gateway-managed Codex credentials.\n\nRefs #2460
Remove unqualified OpenCode from OpenShell Tray choices and document Codex as the only currently supported OpenShell scenario.\n\nRefs #2460
Default OpenCode model/provider selection dynamically and normalize missing config source to Host/default.\n\nRefs #2460
@itdove
itdove marked this pull request as ready for review October 1, 2026 02:38
@itdove
itdove merged commit 6737a8c into main Oct 1, 2026
45 of 47 checks passed
@itdove
itdove deleted the 2460 branch October 1, 2026 02:38
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.

[OpenShell] Pin and qualify v0.1.2 compatibility

1 participant