fix: remove allowed_ips from host-side API server experiment

[maruiz93]

Summary

• Remove allowed_ips from both full-access and restricted policies — per NVIDIA/OpenShell#1560, allowed_ips is not required when endpoints are explicitly declared with host+port
• Remove HOST_IP resolution and policy template rendering from run.sh (no longer needed)
• Point harness files to raw policy files instead of rendered copies
• Add required role field to all harness files (new fullsend requirement since PR Host-side API server experiment #28)

Validation

Tested on OpenShell v0.0.83, rootless Podman + pasta, Fedora 44:

1. Created sandbox with updated policy (no allowed_ips)
2. curl from inside sandbox to host.openshell.internal:9090/tools.json → 200 OK
3. curl from inside sandbox to host.openshell.internal:9091/tools.json → 200 OK
4. curl to undeclared endpoint (example.com) → blocked

Refs: NVIDIA/OpenShell#1633

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 3:29 PM UTC · Ended 3:31 PM UTC
Commit: 12bd957 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Remove allowed_ips templating from host-side API server experiment policies

🐞 Bug fix ⚙️ Configuration changes 🕐 10-20 Minutes

AI Description

• Drop allowed_ips from host-side API server policies since host+port endpoints are explicit.
• Simplify run.sh by removing HOST_IP resolution and policy rendering.
• Update harness configs to use raw policies and include required role: experiment.

Diagram

graph TD
R["run.sh"] --> S["Host API servers :9090/:9091"]
H["harness/*.yaml"] --> P["policies/*.yaml"] --> OS["OpenShell sandbox"] --> S

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep rendering as an optional fallback for older OpenShell versions

• ➕ Maintains compatibility if older policy engines still require allowed_ips
• ➕ Allows gradual rollout across environments with mixed versions
• ➖ Adds ongoing complexity (HOST_IP resolution + rendered file management)
• ➖ More moving parts in the experiment script; harder to debug networking issues

2. Pin experiment to a minimum OpenShell version that supports endpoint-only policies

• ➕ Avoids runtime branching while making compatibility expectations explicit
• ➖ Requires documentation/enforcement (CI check or runtime version check)

Recommendation: The PR’s approach is the right simplification if the experiment targets OpenShell versions where explicit host+port endpoints make allowed_ips redundant. If this experiment is expected to run in mixed-version environments, consider either (a) documenting/enforcing a minimum OpenShell version or (b) adding a simple fallback mode that renders policies only when required.

Files changed (9) +14 / -43

Bug fix (3) +2 / -37

full-access.yamlRemove allowed_ips from full-access network policies +0/-4

Remove allowed_ips from full-access network policies

• Removes 'allowed_ips: {{HOST_IP}}/32' constraints from the declared endpoints, relying on explicit host+port endpoint declarations instead.

host-side-api-server/policies/full-access.yaml

restricted.yamlRemove allowed_ips from restricted network policies +0/-4

Remove allowed_ips from restricted network policies

• Removes 'allowed_ips: {{HOST_IP}}/32' constraints from restricted endpoints, eliminating the need for HOST_IP templating.

host-side-api-server/policies/restricted.yaml

run.shDrop HOST_IP resolution and policy rendering from experiment runner +2/-29

Drop HOST_IP resolution and policy rendering from experiment runner

• Removes host IP discovery logic and the rendered policy file lifecycle (creation + cleanup). Updates messaging to reflect that policies are used directly without templating.

host-side-api-server/run.sh

Other (6) +12 / -6

baked-instructions-full.yamlAdd required role and reference raw full-access policy +2/-1

Add required role and reference raw full-access policy

• Adds 'role: experiment' and switches the policy path from the rendered policy to 'policies/full-access.yaml'.

host-side-api-server/harness/baked-instructions-full.yaml

baked-instructions-restricted.yamlAdd required role and reference raw restricted policy +2/-1

Add required role and reference raw restricted policy

• Adds 'role: experiment' and switches the policy path from the rendered policy to 'policies/restricted.yaml'.

host-side-api-server/harness/baked-instructions-restricted.yaml

openapi-discovery-full.yamlAdd required role and reference raw full-access policy +2/-1

Add required role and reference raw full-access policy

• Adds 'role: experiment' and updates the policy reference to use the non-rendered full-access policy file.

host-side-api-server/harness/openapi-discovery-full.yaml

openapi-discovery-restricted.yamlAdd required role and reference raw restricted policy +2/-1

Add required role and reference raw restricted policy

• Adds 'role: experiment' and updates the policy reference to use the non-rendered restricted policy file.

host-side-api-server/harness/openapi-discovery-restricted.yaml

tooluse-discovery-full.yamlAdd required role and reference raw full-access policy +2/-1

Add required role and reference raw full-access policy

• Adds 'role: experiment' and points the harness at 'policies/full-access.yaml' instead of a rendered copy.

host-side-api-server/harness/tooluse-discovery-full.yaml

tooluse-discovery-restricted.yamlAdd required role and reference raw restricted policy +2/-1

Add required role and reference raw restricted policy

• Adds 'role: experiment' and points the harness at 'policies/restricted.yaml' instead of a rendered copy.

host-side-api-server/harness/tooluse-discovery-restricted.yaml

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 3:32 PM UTC · Ended 3:34 PM UTC
Commit: 12bd957 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 19 rules

✅ Skills: writing-how-to

1. Host allowlist removed ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The host-side API server policies removed allowed_ips, but repo documentation states OpenShell
blocks RFC1918/private IPs by default and requires allowed_ips (CIDR) to allow host access, so
calls to host.openshell.internal:9090/9091 can be denied even though the endpoint is declared.
run.sh also removed HOST_IP resolution and policy rendering, eliminating the previous mechanism
that produced the required CIDR allowlist.

Code

host-side-api-server/policies/restricted.yaml[L37-38]

-        allowed_ips:
-          - "{{HOST_IP}}/32"

Relevance

⭐⭐⭐ High

PR#28 docs say host SSRF requires allowed_ips CIDR; policies used {{HOST_IP}}/32 with run.sh
rendering.

PR-#28

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Internal experiment documentation and other policies in this repo explicitly describe the SSRF guard
behavior and the need for allowed_ips CIDR allowlisting for host access; the updated host-side
policies still target host.openshell.internal but now contain no allowed_ips, and run.sh no
longer performs HOST_IP substitution/rendering.

host-side-api-server/findings.md[68-81]
agent-scoped-tools-triage/README.md[275-291]
agent-scoped-tools-triage/policies/readonly.yaml[20-45]
host-side-api-server/policies/full-access.yaml[19-74]
host-side-api-server/policies/restricted.yaml[17-54]
host-side-api-server/run.sh[137-142]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`allowed_ips` was removed from the host-side API server policies, but repo docs indicate OpenShell blocks private IPs by default (SSRF guard) and requires an explicit CIDR allowlist to reach host-side services. The PR also removed HOST_IP resolution and policy rendering, so even if `allowed_ips` is re-added, it will not be populated.

## Issue Context
The policies still target `host.openshell.internal` (which resolves to a private bridge IP in typical container setups). Without a CIDR allowlist, OpenShell may deny these requests.

## Fix Focus Areas
- host-side-api-server/policies/full-access.yaml[19-74]
- host-side-api-server/policies/restricted.yaml[17-54]
- host-side-api-server/run.sh[93-167]

## Suggested fix
1. Reintroduce `allowed_ips:` for the `host.openshell.internal` endpoints in both policies (builder and provisioner), using the `{{HOST_IP}}/32` placeholder.
2. Reintroduce safe HOST_IP resolution in `run.sh` and render the policies (to temp files or `policies/rendered-*.yaml`).
3. Point harness policy paths back to the rendered outputs (or implement an equivalent substitution step before `openshell policy set`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 3:35 PM UTC · Ended 3:37 PM UTC
Commit: 12bd957 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:37 PM UTC · Completed 3:41 PM UTC
Commit: d6ec691 · View workflow run →

[fullsend-ai-review]

Review — approve

PR: fix: remove allowed_ips from host-side API server experiment

Re-review note

Re-reviewing at ac52e71. The prior review (d6ec691) approved with one low finding: stale usage text in run.sh line 13 still referencing "renders policies." This has been fixed — the usage text now reads "Starts the host-side API servers and runs fullsend with the specified harness."

Summary

Clean, well-scoped simplification of the host-side API server experiment (0010). Removes allowed_ips from network policies, eliminates HOST_IP resolution and policy template rendering from the orchestrator, updates harness files to reference raw policies directly, and adds the now-required role: experiment field. Documentation in README.md and findings.md is thorough, with proper strikethrough of outdated content and a detailed validation section.

What I verified

• Policy files (full-access.yaml, restricted.yaml): All four allowed_ips: ["{{HOST_IP}}/32"] entries removed — two endpoint declarations per file, both policies consistent.
• Harness files (6 files): All updated from policies/rendered-* → policies/* and role: experiment added uniformly.
• run.sh: HOST_IP resolution block (multi-platform fallback via getent and podman network inspect) completely removed. Policy rendering (sed template substitution) and rendered file cleanup removed. Usage text updated (fixes prior review finding). Remaining script flow is coherent — servers start, health-check, env file generated, fullsend runs, cleanup tears down.
• findings.md: Strikethrough on stale allowed_ips text, updated orchestrator lifecycle (8 steps instead of 10), allowed_ips removed from example YAML and key-elements list, cleanup description updated, comprehensive validation section added with test matrix.
• README.md: Changelog section documents the upstream justification (OpenShell #1560, maintainer confirmation on #1633).
• No secrets or sensitive data introduced. The 0.0.0.0 bind is pre-existing and tracked upstream (#1633).

Findings

Severity	File	Description
low	0010-host-side-api-server/.gitignore	Line 1 (policies/rendered-*.yaml) is now stale — rendered policies are no longer generated after this change. Harmless but could be cleaned up.

Verdict

The change is safe. The prior review's finding has been addressed. Upstream justification is sound (OpenShell PR #1560, maintainer confirmation on #1633), validation evidence is documented, and the simplification reduces operational fragility by eliminating the platform-dependent HOST_IP resolution and sed templating. Approved.

Previous run

Review — approve

PR: fix: remove allowed_ips from host-side API server experiment

Summary

Clean, well-scoped simplification of the host-side API server experiment (0010). The PR removes allowed_ips from network policies, removes the HOST_IP resolution and policy template rendering from the orchestrator, updates harness files to reference raw policies directly, and adds the now-required role: experiment field. Documentation updates in README.md and findings.md are thorough, with proper strikethrough of outdated content and detailed validation results.

What I verified

• Policy files (full-access.yaml, restricted.yaml): All four allowed_ips: ["{{HOST_IP}}/32"] entries removed consistently across both endpoint declarations in both files.
• Harness files (6 files): All correctly updated from policies/rendered-* → policies/* (raw files), and role: experiment added uniformly.
• run.sh: HOST_IP resolution block (multi-platform fallback logic), policy rendering (sed template substitution), and cleanup of rendered files all removed. The remaining script flow is coherent — servers start, health-check, env file generated, fullsend runs, cleanup tears down.
• findings.md: Strikethrough on old allowed_ips text, updated orchestrator lifecycle numbering (8 steps instead of 10), removed allowed_ips from example YAML and key-elements list, added comprehensive validation section at the bottom with test matrix.
• README.md: Changelog section correctly positioned, documents the upstream justification (OpenShell #1560, maintainer confirmation on #1633).
• No secrets or sensitive data introduced. The 0.0.0.0 bind is pre-existing and tracked upstream.

Findings

Severity	File	Description
low	0010-host-side-api-server/run.sh	Usage text (line 13) still says "renders policies" — this is now stale since policy rendering was removed. Consider updating to match the new behavior.

Verdict

The change is safe. The upstream justification is sound (OpenShell PR #1560 confirmed by maintainer), validation evidence is documented, and the simplification reduces operational fragility by eliminating the most platform-dependent part of the setup (HOST_IP resolution and sed templating). Approved.

[maruiz93]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:48 PM UTC · Completed 3:54 PM UTC
Commit: 12bd957 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:59 PM UTC · Completed 4:08 PM UTC
Commit: ac52e71 · View workflow run →

[fullsend-ai-retro]

PR #42 was a human-authored fix removing allowed_ips from OpenShell policies in experiment 0010, merged in ~29 minutes. The workflow had 5 review runs: 3 cancelled by rapid force-pushes (~9 min wasted compute), 1 completed review but failed at the post-review script (422 on inline comment outside diff hunk — already tracked as agents#193), and 1 final successful run after /fs-review. The fullsend review agent performed well — it correctly approved the change and found a genuine low-severity issue (stale usage text) that the author fixed before merge. The third-party qodo bot produced a false positive that required human dismissal. Two existing issues cover the main gaps: agents#193 (422 on inline comments outside diff hunks) and fullsend#4960 (debounce review on rapid force-pushes). One new proposal for a stale .gitignore entry left behind.

Proposals filed

• Evidence for agents#193: PR experiments#42 post-review 422 on inline comment outside diff hunk (in fullsend-ai/agents)
• Evidence for fullsend#4960: PR experiments#42 had 4 force-pushes in 16 min, 3 wasted review runs (in fullsend-ai/fullsend)
• chore(0010): remove stale .gitignore entry for rendered policies (in fullsend-ai/experiments)