refactor(sandbox): migrate to local-path providers and profiles - #211
Conversation
|
🤖 Finished Review · ✅ Success · Started 3:48 PM UTC · Completed 4:08 PM UTC |
PR Summary by QodoRefactor sandbox harnesses to use portable URL-based providers & profiles
AI Description
Diagram
High-Level Assessment
Files changed (22)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (5)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (6)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (7)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (8)ReviewFindingsMedium
Low
Next steps:
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsHigh
Medium
Low
Labels: PR modifies sandbox security policies, credential delivery mechanisms, and permission scoping across all agents Next steps:
Previous run (11)ReviewFindingsMedium
Low
Verified
Security assessment
Delta since prior review (SHA
|
| Finding | Status |
|---|---|
Medium [api-contract] — refs/pull/211/head/ URLs |
Unresolved — all 36 references still use the ephemeral ref |
Low [api-contract] — api.anthropic.com dropped |
Documented as intentional; verified via CLAUDE_CODE_USE_VERTEX=1 |
Low [data-exposure] — broad allowed_remote_resources |
Unchanged — practical risk low |
Low [design-smell] — _NOOP_* credentials |
Unchanged |
Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.Protected files in this PR:
.github/scripts/select-eval-agents-test.sh.github/scripts/select-eval-agents.shharness/code.yamlharness/fix.yamlharness/prioritize.yamlharness/retro.yamlharness/review.yamlharness/scribe.yamlharness/triage.yamlpolicies/base.yamlpolicies/code.yamlpolicies/fix.yamlpolicies/prioritize.yamlpolicies/retro.yamlpolicies/review.yamlpolicies/scribe.yamlpolicies/triage.yamlskills/pr-review/SKILL.md
Previous run (12)
Review
Findings
Medium
- [api-contract]
harness/code.yaml(and all 6 other harness files) — All 36 profile and provider URL references across all 7 harness files still use the ephemeral refrefs/pull/211/head/. These URLs resolve only while PR refactor(sandbox): migrate to local-path providers and profiles #211's branch exists and will return 404 after the branch is deleted post-merge. The pinned sha256 integrity hashes are coupled to the current file contents at this ref. Depending on OpenShell's error handling, a 404 on profile/provider fetch could either fail-closed (agents cannot start) or fail-open (sandbox starts without network policy enforcement). This finding was raised in the prior review (SHAb300555) and remains unresolved.
Remediation: Before merging, update all profile and provider URLs to reference a stable ref (e.g.,refs/heads/main) and recompute sha256 hashes against the post-merge file contents.
Low
-
[api-contract]
profiles/fullsend-vertex-ai.yaml— The new profile dropsapi.anthropic.com(present in 6 of 7 old per-agent policies). The PR body documents this as intentional (all agents use Vertex AI viaCLAUDE_CODE_USE_VERTEX=1). Verify no sandbox code path makes direct Anthropic API calls. -
[data-exposure]
harness/code.yaml(and all harness files) — Theallowed_remote_resourcesprefixhttps://raw.githubusercontent.com/fullsend-ai/agents/permits fetching any file from this repository, not just profiles and providers. Scope is broader than strictly necessary, though the practical risk is low (read-only access to a public repo). -
[design-smell]
providers/gitleaks.yaml,providers/package-registries.yaml,providers/vertex-ai.yaml— These providers use_NOOP_*placeholder credentials. Reasonable workaround for an upstream schema requirement but worth documenting.
Verified
- SHA256 integrity hashes: all 12 profile and provider file hashes verified against file contents at HEAD — every hash in every harness config matches ✓
- Provider→profile type bindings: all 6 providers correctly reference their corresponding profile IDs ✓
- Env var migration completeness: all variables from the 6 deleted
.envfiles are accounted for in the new structuredenv.sandbox,forge.github.env.sandbox, and provider credentials sections ✓ - GH_TOKEN delivery chain: GitHub providers (github-code, github-ro, github-artifacts) inject
GH_TOKENvia provider credentials, replacing explicit sandbox env entries ✓ - No stale references: zero references to deleted files (
env/code.env,policies/code.yaml,common/env/, etc.) remain in any harness or config file ✓ - README updated:
profiles/,providers/, and revisedpolicies/andenv/descriptions added; stalecommon/entry removed ✓ - Test fixtures updated:
select-eval-agents-test.shfixtures and assertions aligned with new file structure ✓ - SKILL.md reference updated:
skills/pr-review/SKILL.mdnow referencesforge.github.env.sandboxinstead of deletedenv/review.env✓ - Usage comment updated:
select-eval-agents.shusage example now referencesenv/gcp-vertex.envinstead of deletedenv/triage.env✓ - Commit messages: both commits follow Conventional Commits format (
feat(sandbox):,refactor(harness):) ✓
Security assessment
| Change | Assessment |
|---|---|
| Triage/prioritize: read-write → read-only GitHub | Improvement — sandbox only reads; writes are post-script-only |
| GH_TOKEN removed from sandbox env vars | Improvement — provider delivers credentials via OpenShell gateway; raw token no longer exposed inside sandbox |
GraphQL /graphql endpoint added to github-ro and github-code profiles |
Controlled expansion — read-only access, enables gh --json commands (per #173) |
api.anthropic.com removed from vertex-ai profile |
Improvement — unused with CLAUDE_CODE_USE_VERTEX=1 |
_NOOP_* credentials in gitleaks/package-registries/vertex-ai providers |
Neutral — placeholder credentials for providers that don't require auth |
Delta since prior review (SHA b300555)
The branch was force-pushed with a reorganized commit history (previously 3+ commits, now 2 clean commits). The changes are functionally equivalent to the prior review's version. The medium finding about ephemeral refs/pull/211/head/ URLs remains unresolved — no commits addressed this. All low findings from the prior review are also unchanged.
Prior review resolution
| Finding | Status |
|---|---|
Medium [api-contract] — refs/pull/211/head/ URLs |
Unresolved — all 36 references still use the ephemeral ref |
| Medium [scope-exceeded] — PR breadth | Observational — no code change expected |
Low [api-contract] — api.anthropic.com dropped |
Documented as intentional in PR body |
Low [data-exposure] — broad allowed_remote_resources |
Acknowledged — practical risk low |
Low [design-smell] — _NOOP_* credentials |
Acknowledged |
| Low [design-direction] — ADR 0065 reference | Addressed — base.yaml comment references ADR |
| Low [field-ordering] — scribe.yaml ordering | Resolved — post_script moved next to pre_script |
Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.Protected files in this PR:
.github/scripts/select-eval-agents-test.sh.github/scripts/select-eval-agents.shharness/code.yamlharness/fix.yamlharness/prioritize.yamlharness/retro.yamlharness/review.yamlharness/scribe.yamlharness/triage.yamlpolicies/base.yamlpolicies/code.yamlpolicies/fix.yamlpolicies/prioritize.yamlpolicies/retro.yamlpolicies/review.yamlpolicies/scribe.yamlpolicies/triage.yamlskills/pr-review/SKILL.md
Previous run (13)
Review
Findings
Medium
-
[api-contract]
harness/code.yaml:24— All profile and provider URLs across all 7 harness files use the ephemeral refrefs/pull/211/head/. These URLs resolve only while PR refactor(sandbox): migrate to local-path providers and profiles #211's branch exists and will return 404 after the branch is deleted post-merge. The pinned sha256 integrity hashes are also coupled to the current file contents at this ref. Depending on OpenShell's error handling, a 404 on profile/provider fetch could either fail-closed (safe) or fail-open (sandbox starts without network policy enforcement).
Remediation: Before merging, update all profile and provider URLs to reference a stable ref (commit SHA on main, versioned tag, orrefs/heads/main) and recompute sha256 hashes. -
[scope-exceeded] — PR title claims
refactor(sandbox)but the scope encompasses multiple architecturally related concerns: provider/profile migration (ADR 0070), env file restructuring (ADR 0055), GitHub access restrictions, endpoint changes, and GH_TOKEN delivery. While these changes are interconnected parts of the same portability model, the breadth (400+ lines, 39 files) warrants careful review attention.
Low
-
[api-contract]
profiles/fullsend-vertex-ai.yaml:7— The new profile dropsapi.anthropic.com(present in 6 of 7 old per-agent policies). The PR body documents this as intentional ("all agents use Vertex AI viaCLAUDE_CODE_USE_VERTEX=1"), and the old scribe policy already operated without it. Verify no sandbox code path makes direct Anthropic API calls. -
[data-exposure]
harness/code.yaml:20— Theallowed_remote_resourcesprefixhttps://raw.githubusercontent.com/fullsend-ai/agents/permits fetching any file from this repository, not just profiles and providers. The scope is broader than strictly necessary for loading profiles and providers, though the practical risk is low (read-only access to a public repo). -
[design-smell]
providers/gitleaks.yaml— Providers for gitleaks, package-registries, and vertex-ai use_NOOP_*placeholder credentials. This is a reasonable workaround for an upstream schema requirement but worth documenting. -
[design-direction]
policies/base.yaml— The base policy references ADR 0065 for network policy architecture but the ADR is not in this repository. The README is updated with directory descriptions, which partially addresses the documentation gap. -
[field-ordering]
harness/scribe.yaml:39— Field ordering placesskills: []beforepre_script/post_script, inconsistent with the majority pattern (4 of 7 harness files place script fields before extension fields).
Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.Protected files in this PR:
.github/scripts/select-eval-agents-test.sh.github/scripts/select-eval-agents.shharness/code.yamlharness/fix.yamlharness/prioritize.yamlharness/retro.yamlharness/review.yamlharness/scribe.yamlharness/triage.yamlpolicies/base.yamlpolicies/code.yamlpolicies/fix.yamlpolicies/prioritize.yamlpolicies/retro.yamlpolicies/review.yamlpolicies/scribe.yamlpolicies/triage.yamlskills/pr-review/SKILL.md
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
056ac36 to
3be6d7f
Compare
|
🤖 Finished Review · ✅ Success · Started 10:16 AM UTC · Completed 10:23 AM UTC |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
f988a8f to
52f89eb
Compare
|
🤖 Finished Review · ✅ Success · Started 12:28 PM UTC · Completed 12:34 PM UTC |
|
Blocked on fullsend-ai/fullsend#5240 The harness files reference profiles and providers via URLs pointing to This is a chicken-and-egg problem inherent to self-referencing URL patterns. Once this PR merges the files will exist on |
52f89eb to
b300555
Compare
|
🤖 Finished Review · ❌ Failure · Started 1:01 PM UTC · Completed 1:22 PM UTC |
|
Updated blocker: fullsend-ai/fullsend#5241 — fullsend-ai/fullsend#5240 (local-path resolution for profiles/providers) remains a separate design gap tracked independently. |
b300555 to
0c216ed
Compare
|
🤖 Finished Review · ✅ Success · Started 2:15 PM UTC · Completed 2:24 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Ran 3 independent reviews (Claude x2, Grok) plus my own verification. Summary:
Confirmed findings:
- [MEDIUM]
.github/scripts/select-eval-agents.sh'sextract_refs()doesn't track.openshell.profiles[]/.providers[]— filed as #278, not blocking this PR itself. - [MEDIUM] PR body cites #2758–#2763 as completed scaffold migrations; #2760 (code agent) was actually closed, not merged — minor doc-accuracy note, doesn't affect this PR's own correctness.
- [LOW] Fix agent gains broader package-registry access via the shared profile (yarn/pnpm/npx + registry.yarnpkg.com) — likely a correction of pre-existing drift (old fix policy claimed parity with code's policy but wasn't actually identical), not a regression.
Rejected as false positive: a claim that profiles need their own credentials: block — verified against ADR 0025 and the actual provider/profile file pairs: credentials correctly belong at the provider level (what openshell provider create consumes), profiles are pure L7 policy. Confirmed no raw GH_TOKEN in any env:sandbox: block across all 7 harnesses.
Independently verified: all 36 sha256 hashes match, the harness→provider→profile resolution chain is sound and will fix the original missing-profiles/-directory bug once merged, and the read-only downgrade for triage/prioritize is real and enforced at the endpoint level (a genuine security fix, not just convention — the old policies claimed read-only but were actually read-write).
Known blocker (already tracked, not this PR's fault): functional-tests(triage) fails because this is the first harness anywhere to exercise the URL-referenced single-profile import path (ImportProfile), which hits a pre-existing bug in fullsend-ai/fullsend — filed as #5241, already has a fix PR open (fullsend-ai/fullsend#5245, which I'll review next). This PR's CI will go green once that lands and a release picks it up.
Also still open from prior rounds: the ephemeral refs/pull/211/head/ URLs need to become a real commit SHA before/at merge.
Approving on the substance — the design and implementation are sound.
24a0c4a to
f9c840b
Compare
|
🤖 Finished Review · ✅ Success · Started 4:39 PM UTC · Completed 4:55 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 4 findings below (2 inline, 2 in this body because they don't map to a single file/line).
HIGH — Nondeterministic vertex-ai provider-creation failure (files: providers/vertex-ai.yaml, profiles/fullsend-vertex-ai.yaml)
All 7 triage eval cases in CI run 31721624303 share byte-identical harness config (harness/triage.yaml), yet produce two different fatal errors: 6 of 7 fail at the GitHub 403 pre-flight check, while case 002-needs-info-vague-crash instead fails earlier at provider setup:
Error: ensuring provider "vertex-ai": provider create "vertex-ai" failed: exit status 1 (output: Error: × unsupported provider type or profile: fullsend-vertex-ai)
Confirmed by downloading eval-results-triage and reading all 7 cases/*/stderr.log directly — code's and fix's single eval cases (which also use providers/vertex-ai.yaml, identical type: fullsend-vertex-ai) did not hit this error, only reaching the 403. Since the harness config is identical across the 7 triage cases, this points to a race/nondeterminism in the provider 'ensure' lifecycle when eval cases run concurrently on the same runner and share local provider/profile state — a new failure class this PR introduces by moving from static inline network_policies to dynamically-'ensured' local-path provider files. Not previously raised in any existing review comment.
Suggestion: Investigate whether concurrent fullsend run invocations on the same runner can race on provider/profile 'ensure' state (e.g. a shared local cache or lockfile for OpenShell provider creation). If so, serialize provider-ensure calls per runner or make the ensure step safe for concurrent invocation before relying on this provider/profile model at scale.
MEDIUM — GitHub API 403 pre-flight connectivity failure blocks all 4 functional-tests jobs (file: providers/github-code.yaml, no single-line anchor)
At current head, functional-tests (code), (fix), and (triage, 6 of 7 cases) all fail with the identical error, confirmed by downloading eval-results-code/eval-results-fix/eval-results-triage and reading stderr.log:
Error: pre-flight connectivity check: GitHub API unreachable from sandbox (HTTP 403 — proxy allowlist issue): Get "https://api.github.com/rate_limit": Forbidden
This reproduces across every case in code/fix/triage (review fails earlier on the separate PRIOR_REVIEW_* validation bug, see inline comment on harness/review.yaml). No existing PR comment or review thread mentions this connectivity/403 failure at all — it has not been surfaced or acknowledged anywhere despite mergeStateStatus being BLOCKED specifically on these required checks.
Suggestion: Do not merge until at least one functional-tests job passes end-to-end against the new provider/profile files. Compare the OpenShell gateway's resolved proxy allowlist for a working top-level provider (vertex-ai succeeds past this point) against the forge-scoped github-* providers/profiles to find where the network policy or credential binding is being dropped at runtime.
f9c840b to
a09df55
Compare
|
🤖 Review · Commit: |
a09df55 to
0eca12f
Compare
|
🤖 Finished Review · ✅ Success · Started 10:10 AM UTC · Completed 10:32 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0eca12f to
f7a0528
Compare
|
🤖 Finished Review · ✅ Success · Started 8:53 AM UTC · Completed 9:14 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace inline openshell config blocks with portable provider and
profile YAML files that live alongside the harness configs. This makes
sandbox credentials and tool definitions reusable across agents and
forks without duplicating openshell stanzas.
- Add providers/ and profiles/ directories with vertex-ai, github-ro,
github-code, github-artifacts, and github-community configs.
- Reference them from harness/*.yaml via providers: and
openshell.profiles: keys (top-level and per-forge).
- Update select-eval-agents.sh to extract provider/profile refs from
harness YAML (generic .forge[]? iterator for multi-forge support).
- Update select-eval-agents-test.sh fixtures and expectations.
- Fix env.sandbox ${VAR:-} syntax: fullsend's env validator doesn't
understand bash-style defaults — remove :- suffixes and supply empty
defaults in the eval runner instead.
Signed-off-by: Marta Anon <marta@fullsend.ai>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Marta Anon <manon@redhat.com>
f7a0528 to
253f382
Compare
|
🤖 Finished Review · ✅ Success · Started 9:56 AM UTC · Completed 10:17 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 12:15 PM UTC · Completed 12:29 PM UTC Commit: |
Retro: PR #211 — refactor(sandbox): migrate to local-path providers and profilesPR: #211 by maruiz93 (human-authored) Review quality assessmentThe review bot ran 13+ iterations across 14 commits. The final review (run 32017692283) found 10 issues (2 high, 4 medium, 4 low) and issued Bot strengths (16 unique findings humans missed): The bot excelled at static-analysis breadth — stale file references across docs/configs (5 findings), missing CI trigger paths, duplicate YAML keys, missing Human strengths (12 unique findings bot missed): The human reviewer (waynesun09) caught every critical and high-severity runtime failure:
Most concerning gap: The bot produced unsubstantiated verification claims ("zero references to deleted files remain," "GH_TOKEN delivery chain verified") at commits where both were provably false. These false verifications are more dangerous than missing findings because they create false confidence. Existing issues with new evidence from this PR
Token cost note13+ review iterations over a month is significant token spend. Each iteration used claude-opus-4-6 with ~21 minutes execution time. Most iterations re-flagged the same stale-reference and permission-expansion findings that persisted across intermediate commits. This is inherent to the re-review-on-push workflow and is partially addressed by agents#511 (coverage-stability check on re-review). Proposals filed
|
3 participants
Summary
policies/*.yamlwith duplicatednetwork_policies) with local-path providers and profiles (ADR 0070, feat(resolve): portable provider and profile definitions via URL references fullsend#3062)profiles/*.yaml) defining network endpoints and binary allowlists, 6 providers (providers/*.yaml) binding profile types to credentials, and a sharedpolicies/base.yaml(filesystem/landlock/process only)forge.githubblocks (harness: add providers and openshell to ForgeConfig for per-forge provider/profile selection fullsend#5970)env/*.envhost files to structuredenv: runner:/env: sandbox:sections per ADR 0055 (feat(harness): unified env var delivery (ADR 0055) fullsend#2582), aligning with the scaffold migrations done in refactor(harness): migrate review agent to env.runner/env.sandbox (ADR 0055) fullsend#2758–#2763common/env/gcp-vertex.env→env/gcp-vertex.envand extract GIT_SSL_CAINFO workaround to sharedenv/ssl-cainfo.env/graphqlendpoint to GitHub profiles (from fix(policies): allow POST /graphql in agent sandbox policies #173)api.anthropic.comfrom vertex-ai profile (all agents use Vertex AI viaCLAUDE_CODE_USE_VERTEX=1)GH_TOKEN) from sandbox env vars to provider definitions — the provider delivers credentials through OpenShell's gateway, so the raw token no longer enters the sandboxMotivation
The current per-agent policy files duplicate ~60 lines of network rules each. When an endpoint changes (e.g., adding GraphQL), every policy file must be updated independently. The portable provider/profile model (ADR 0070) lets each harness compose its network policy from reusable profiles — the same pattern already used for skills and other portable harness components.
Providers and profiles use local filesystem paths, referenced directly in harness YAML. Forge-agnostic entries (vertex-ai, package-registries, gitleaks) live at the top level so they apply regardless of forge. Forge-specific entries (github-code, github-ro, github-artifacts) live under
forge.githubblocks, so they're only activated when the harness runs on GitHub — making the harnesses portable across forges (fullsend-ai/fullsend#5970).The env migration follows ADR 0055 (fullsend-ai/fullsend#2582), which introduced structured
env: runner:/env: sandbox:sections to replace scatter-shot.envhost files. The scaffold agents were already migrated in fullsend-ai/fullsend#2758–#2763; this PR brings the agents repo in line.Provider/profile mapping
Security changes
/graphqlendpointgh --jsoncommands that use GraphQL internallyapi.anthropic.comfrom vertex-aiCLAUDE_CODE_USE_VERTEX=1Files
Created (14): 6 profiles, 6 providers,
policies/base.yaml,env/ssl-cainfo.envModified (7): all harness files
Deleted (13): 7 old policy files, 6 old env files
Moved (1):
common/env/gcp-vertex.env→env/gcp-vertex.envTest plan
fullsend harness lintpasses on all 7 harness filesRelated
🤖 Generated with Claude Code