feat(resolve): portable provider and profile definitions via URL references

[maruiz93]

Summary

Closes #2672

Implements portable provider and profile definitions that can be referenced by URL with SHA-256 integrity hashing, extending the existing resource resolution system (ADRs 0038, 0045) to cover the full harness surface.

• Add profiles field (URL-only) and extend providers to accept mixed local/URL entries in harness schema
• Resolve URL-referenced profiles and providers during fullsend run, with ParseProfileID validation and WarnLiteralCredentials for credential hygiene
• Import resolved profiles via sandbox.ImportProfile and create/update providers via sandbox.EnsureProvider with reserved credential key validation
• Merge profiles and providers during base harness composition (base + child, child wins)
• Extend lock file resolution to reconstruct profiles and providers from cache
• Validate referential integrity between provider types and declared profile IDs
• Add ADR 0066 documenting the design decisions

Test plan

• go test ./internal/resolve/ — profile/provider resolution, ParseProfileID, WarnLiteralCredentials
• go test ./internal/harness/ — profiles field validation, HasURLReferences, base composition merge
• go test ./internal/sandbox/ — ImportProfile, reserved credential key rejection
• go test ./internal/cli/ — dedupResolvedProfiles, mergeProviderDefs, checkProviderProfileIntegrity, lock file reconstruction
• make lint passes
• 5 rounds of code review (42+ findings triaged across rounds)

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:39 AM UTC · Ended 10:40 AM UTC
Commit: 0a95cac · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Portable provider/profile definitions via URL references (sha256-pinned)

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add URL-resolved profiles and mixed local/URL providers to the harness schema.
• Resolve, validate, and cache remote profile/provider YAML during fullsend run and lock
 resolution.
• Import profiles and ensure providers on the gateway with credential hygiene checks and integrity
 validation.

Diagram

graph TD
  H[/"Harness YAML"/] --> C["compose.go merge"] --> R["resolve.ResolveHarness"] --> Cache[(".fullsend-cache")]
  R --> Run["cli/run.go"] --> OS{{"openshell gateway"}}
  Lock[/"lock.yaml"/] --> LockRes["lock.go resolveFromLock"] --> Run
  LockRes --> Cache

  subgraph Legend
    direction LR
    _doc[/"Document"/] ~~~ _proc["Process"] ~~~ _db[("Cache/DB")] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fetch provider/profile directories as a git tree (skill-style)

• ➕ Single fetch per repo subtree; potentially fewer network round-trips
• ➕ Keeps provider/profile files colocated in a repo without per-file URLs
• ➖ More complex semantics: directory layout conventions, name/id discovery, and dedup rules
• ➖ Harder integrity pinning at the individual definition level; tree hashes are coarser-grained

2. Allow local-path profiles/providers relative to harness repo

• ➕ Simpler authoring for local development and monorepos
• ➕ Avoids remote fetching for common cases
• ➖ Undermines portability guarantees when composing remote base harnesses
• ➖ Increases ambiguity around search paths and precedence vs .fullsend/* directories

3. Introduce a unified `resources:` map with typed entries

• ➕ More extensible long-term (future resource types)
• ➕ Can encode richer metadata per entry (type, url, hash, name overrides)
• ➖ Schema churn and migration cost vs incremental extension
• ➖ Heavier authoring burden for a targeted portability gap

Recommendation: The PR’s approach (explicit profiles URL list + mixed providers list, both sha256-pinned) is the best tradeoff for portability and security: it preserves existing local-provider workflows, keeps integrity pinning per definition, and cleanly integrates with existing resolver/cache/lock mechanisms. The main alternatives either add significant complexity (tree fetch + discovery) or weaken the portability/security goals (local profiles).

Files changed (13) +1530 / -145

Enhancement (6) +417 / -42

lock.goReconstruct resolved profiles/providers from lock cache +70/-13

Reconstruct resolved profiles/providers from lock cache

• Updates lock-time harness resolution to return a 'ResolveResult' (deps + resolved profiles/providers). Rehydrates profile IDs and provider defs from cached YAML content, attaches credential-hygiene warnings to dependency entries, and strips URL providers from 'h.Providers' to mirror normal resolution behavior.

internal/cli/lock.go

run.goImport resolved profiles and merge URL/local provider defs at runtime +114/-12

Import resolved profiles and merge URL/local provider defs at runtime

• Threads 'ResolveResult' through the run flow (including lock-file path), prints non-fatal resolution warnings, imports resolved profiles via 'sandbox.ImportProfile', merges local provider defs with URL-resolved defs deterministically, and validates provider->profile referential integrity before ensuring providers.

internal/cli/run.go

compose.goMerge 'profiles' during base harness composition +6/-0

Merge 'profiles' during base harness composition

• Extends 'mergeBaseIntoChild' to concatenate base and child profile URL lists (base first), matching existing merge patterns for providers/skills.

internal/harness/compose.go

harness.goAdd 'Profiles' field and URL/hash validation for profiles/providers +27/-3

Add 'Profiles' field and URL/hash validation for profiles/providers

• Adds 'Profiles []string' to the harness schema, enforces that profiles are URL-only with required '#sha256=' integrity hashes, and tightens provider validation so URL providers must include integrity hashes. Extends 'HasURLReferences' to include profiles and URL providers.

internal/harness/harness.go

resolve.goResolve URL profiles/providers and return structured 'ResolveResult' +144/-9

Resolve URL profiles/providers and return structured 'ResolveResult'

• Introduces 'ResolveResult', 'ResolvedProfile', and 'ResolvedProvider' outputs; adds profile ID parsing/validation and provider YAML parsing/validation for URL entries. Adds 'WarnLiteralCredentials' to flag non-${VAR} credential values, strips URL providers from 'h.Providers', and updates error handling to return the new result type.

internal/resolve/resolve.go

sandbox.goAdd profile import and harden provider creation against reserved keys +56/-5

Add profile import and harden provider creation against reserved keys

• Adds 'ImportProfile' to invoke 'openshell provider profile import' with idempotent handling for already-existing profiles. Updates 'EnsureProvider'/'updateProvider' to accept context and reject reserved credential keys that could influence process/shell/proxy behavior before executing openshell.

internal/sandbox/sandbox.go

Tests (6) +881 / -103

lock_test.goExpand lock resolution tests for profiles/providers reconstruction +213/-37

Expand lock resolution tests for profiles/providers reconstruction

• Refactors existing tests to use 'ResolveResult' and adds coverage for profile/provider reconstruction from cache, including missing id/name/type errors and literal-credential warnings.

internal/cli/lock_test.go

run_test.goAdd unit tests for profile dedup, provider merge, and integrity checks +188/-0

Add unit tests for profile dedup, provider merge, and integrity checks

• Adds focused tests for 'dedupResolvedProfiles', 'mergeProviderDefs' precedence/ordering rules, and 'checkProviderProfileIntegrity' warning/error behavior.

internal/cli/run_test.go

compose_test.goTest base+child profile list concatenation +66/-0

Test base+child profile list concatenation

• Adds tests covering profile inheritance from base harnesses and child-only behavior when base has or lacks profiles.

internal/harness/compose_test.go

harness_test.goTest profiles/providers URL detection and validation rules +82/-0

Test profiles/providers URL detection and validation rules

• Expands 'HasURLReferences' coverage for profiles and provider URL/local cases and adds 'ValidateResourceTypes' tests for profile URL-only + hash requirements and provider URL hash enforcement.

internal/harness/harness_test.go

resolve_test.goAdd resolver tests for profile/provider URL resolution and warnings +285/-63

Add resolver tests for profile/provider URL resolution and warnings

• Refactors existing tests to use 'ResolveResult' and adds new coverage for profile fetching/id validation, provider fetching/validation, provider URL stripping behavior, and credential warning detection/sorting.

internal/resolve/resolve_test.go

sandbox_test.goTest profile import failure and reserved credential key rejection +47/-3

Test profile import failure and reserved credential key rejection

• Adds tests for 'ImportProfile' error behavior when openshell is unavailable and for 'EnsureProvider' rejecting reserved credential env-var keys (case-insensitive), while leaving allowed keys unaffected.

internal/sandbox/sandbox_test.go

Documentation (1) +232 / -0

0066-portable-provider-profile-resolution.mdAdd ADR 0066 for portable profile/provider URL resolution +232/-0

Add ADR 0066 for portable profile/provider URL resolution

• Introduces an accepted ADR describing new harness fields ('profiles', extended 'providers'), composition/merge semantics, resolution and runtime flows, validation rules, and security posture (hash pinning, SSRF hardening, credential hygiene warnings).

docs/ADRs/0066-portable-provider-profile-resolution.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:41 AM UTC · Completed 10:56 AM UTC
Commit: dcf40a7 · View workflow run →

[github-actions]

Site preview

Preview: https://98ff1f10-site.fullsend-ai.workers.dev

Commit: f839bdf96d2f660a8f856868d4f5abdfd7a4f9fb

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

[fullsend-ai-review]

Review

The branch was rebased since the prior review (SHA e9f8f39); commit 031dedd ("fix: address review findings for portable providers (round 3)") is a rebase with the ADR renumbered from 0069 to 0070 (accommodating a new ADR merged to main). All prior security fixes remain in place and all cross-references are correctly updated to 0070.

The security posture is comprehensive and unchanged from the prior review: validIdentifier regex is correctly applied in both resolve.go and lock.go; fromURL correctly gates os.ExpandEnv() on config values; reservedCredentialKeys blocklist (30 entries including GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, GIT_ASKPASS) is correctly gated on fromURL; ImportProfile wraps both delete and import in context.WithTimeout; resolveFromLock checks MatchingAllowedPrefix unconditionally before any cache access or mutation.

One prior high finding and one prior medium finding persist unchanged. Both require resolution before merge.

Findings

High

• [missing-breaking-change-marker] PR title feat(resolve): portable provider and profile definitions via URL references — The ImportProfiles function change (from filename-based ID to YAML id field extraction via profileIDFromFile) constitutes a breaking change: profile YAML files in .fullsend/profiles/ without an id field will now fail with a hard error where they previously succeeded using the filename as the ID. Per COMMITS.md: "Validation is added that rejects previously accepted input" is a breaking change. The PR title must carry the ! suffix: feat(resolve)!: portable provider and profile definitions via URL references. Commit messages should include a BREAKING CHANGE: trailer explaining that ImportProfiles now requires the id field in profile YAML files.

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement in the BREAKING CHANGE trailer.

Low

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in multiple locations across 3 files (phase1 line 147, phase2 lines 102/134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [naming-consistency] internal/resolve/resolve.go — The validIdentifier comment states it "matches the same format as harness.validProviderName" but the patterns differ: validIdentifier requires an alphanumeric first character (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) while validProviderName allows underscore/hyphen starts (^[a-zA-Z0-9_-]+$). The stricter pattern is correct for URL-fetched content (prevents flag injection), but the comment should clarify this intentional difference rather than claiming equivalence.

Previous run

Review

The branch was rebased since the prior review (SHA c846e51); commit e9f8f39 ("fix: address review findings for portable providers (round 3)") addresses the remaining ADR renumbering and content alignment finding from the prior iteration:

• [doc-inconsistency] FIXED — ADR renumbered from 0068 to 0069. Title, heading, and filename are consistent. All cross-references in ADRs 0024, 0038, architecture.md, and customizing-agents.md updated. The "Integration in run.go" section now matches the actual implementation ordering. No stale references to the old portable-provider ADR 0068 remain — existing 0068 references are for 0068-public-community-mint-architecture.md, a different ADR.

The prior [behavior-change] low finding is also resolved: the "Provider declared but no definition found" warning was re-added in run.go (lines ~783–790), restoring the diagnostic message that helps debug misspelled provider names.

All five prior security findings (missing-input-validation, env-var-exfiltration-via-config, incomplete-blocklist, missing-context-timeout, doc-inconsistency) remain fixed. The validIdentifier regex is correctly applied in both resolve.go and lock.go. The fromURL flag is correctly threaded through urlProviderNames → mergeProviderDefs → EnsureProvider → buildProviderArgs/buildProviderUpdateArgs. The reservedCredentialKeys blocklist (30 entries including GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, GIT_ASKPASS) is correctly gated on fromURL. ImportProfile wraps both delete and import in context.WithTimeout.

One new high finding was identified regarding the PR title. One prior medium finding persists unchanged. The medium finding and the PR title finding require resolution before merge.

Findings

High

• [missing-breaking-change-marker] PR title feat(resolve): portable provider and profile definitions via URL references — The ImportProfiles function change (from filename-based ID to YAML id field extraction via profileIDFromFile) constitutes a breaking change: profile YAML files in .fullsend/profiles/ without an id field will now fail with a hard error where they previously succeeded using the filename as the ID. Per COMMITS.md: "Validation is added that rejects previously accepted input" is a breaking change. The PR title must carry the ! suffix: feat(resolve)!: portable provider and profile definitions via URL references. Commit messages should include a BREAKING CHANGE: trailer explaining that ImportProfiles now requires the id field in profile YAML files.

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement in the BREAKING CHANGE trailer.

Low

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 6 locations across 3 files (phase1 line 147, phase2 lines 134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [naming-consistency] internal/resolve/resolve.go — The validIdentifier comment states it "matches the same format as harness.validProviderName" but the patterns differ: validIdentifier requires an alphanumeric first character (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) while validProviderName allows underscore/hyphen starts (^[a-zA-Z0-9_-]+$). The stricter pattern is correct for URL-fetched content (prevents flag injection), but the comment should clarify this intentional difference rather than claiming equivalence.

Previous run

Review

The branch was rebased since the prior review (SHA c846e51); commit e9f8f39 ("fix: address review findings for portable providers (round 3)") addresses the remaining ADR renumbering and content alignment finding from the prior iteration:

• [doc-inconsistency] FIXED — ADR renumbered from 0068 to 0069. Title, heading, and filename are consistent. All cross-references in ADRs 0024, 0038, architecture.md, and customizing-agents.md updated. The "Integration in run.go" section now matches the actual implementation ordering. No stale references to the old portable-provider ADR 0068 remain — existing 0068 references are for 0068-public-community-mint-architecture.md, a different ADR.

The prior [behavior-change] low finding is also resolved: the "Provider declared but no definition found" warning was re-added in run.go (lines ~783–790), restoring the diagnostic message that helps debug misspelled provider names.

All five prior security findings (missing-input-validation, env-var-exfiltration-via-config, incomplete-blocklist, missing-context-timeout, doc-inconsistency) remain fixed. The validIdentifier regex is correctly applied in both resolve.go and lock.go. The fromURL flag is correctly threaded through urlProviderNames → mergeProviderDefs → EnsureProvider → buildProviderArgs/buildProviderUpdateArgs. The reservedCredentialKeys blocklist (30 entries including GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, GIT_ASKPASS) is correctly gated on fromURL. ImportProfile wraps both delete and import in context.WithTimeout.

One new high finding was identified regarding the PR title. One prior medium finding persists unchanged. The medium finding and the PR title finding require resolution before merge.

Findings

High

• [missing-breaking-change-marker] PR title feat(resolve): portable provider and profile definitions via URL references — The ImportProfiles function change (from filename-based ID to YAML id field extraction via profileIDFromFile) constitutes a breaking change: profile YAML files in .fullsend/profiles/ without an id field will now fail with a hard error where they previously succeeded using the filename as the ID. Per COMMITS.md: "Validation is added that rejects previously accepted input" is a breaking change. The PR title must carry the ! suffix: feat(resolve)!: portable provider and profile definitions via URL references. Commit messages should include a BREAKING CHANGE: trailer explaining that ImportProfiles now requires the id field in profile YAML files.

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement in the BREAKING CHANGE trailer.

Low

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 6 locations across 3 files (phase1 line 147, phase2 lines 134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [naming-consistency] internal/resolve/resolve.go — The validIdentifier comment states it "matches the same format as harness.validProviderName" but the patterns differ: validIdentifier requires an alphanumeric first character (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) while validProviderName allows underscore/hyphen starts (^[a-zA-Z0-9_-]+$). The stricter pattern is correct for URL-fetched content (prevents flag injection), but the comment should clarify this intentional difference rather than claiming equivalence.

Previous run (2)

Review

Commit c846e51 since the prior review (SHA ec1b1ed) addresses five prior findings:

1. [missing-input-validation] FIXED — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) and exported ValidIdentifier() function are now applied to URL-fetched provider name and type in both resolve.go (ResolveHarness) and lock.go (resolveFromLock), and to profile id via ParseProfileID. The regex requires an alphanumeric first character, preventing leading-dash flag injection, and rejects path traversal, shell metacharacters, and whitespace. New tests confirm rejection of -h, foo;rm -rf /, ../traversal, and my profile (spaces).

2. [env-var-exfiltration-via-config] FIXED — buildProviderArgs and buildProviderUpdateArgs now skip os.ExpandEnv() on config values when fromURL=true. The fromURL flag is correctly threaded: urlProviderNames map starts from all URL-resolved providers, removes entries shadowed by local providers via mergeProviderDefs, then passes the boolean to EnsureProvider. New tests verify no secret leaking for URL providers (TestBuildProviderArgs_ConfigNotExpandedForURL) and correct expansion for local providers (TestBuildProviderArgs_ConfigExpandedForLocal). The update path is also covered (TestBuildProviderUpdateArgs_ConfigNotExpandedForURL).

3. [incomplete-blocklist] FIXED — GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, and GIT_ASKPASS added to reservedCredentialKeys, closing the git command execution vector via credential keys from URL-fetched providers. The blocklist remains case-insensitive via strings.ToUpper(k).

4. [missing-context-timeout] FIXED — ImportProfile now accepts an id parameter for delete-before-import idempotency and wraps both the delete and import commands in context.WithTimeout(ctx, providerTimeout), matching the timeout pattern used by EnsureProvider and directory-based ImportProfiles.

5. [doc-inconsistency] FIXED — ADR renumbered from 0068 to 0069. Title, heading (now # 69.), and filename are consistent. All cross-references in ADRs 0024, 0038, architecture.md, and customizing-agents.md updated. No stale references to the old 0068 portable-provider ADR remain.

Additionally:

• ADR content updated — The "Integration in run.go" section now matches the actual implementation ordering: fail-fast referential integrity check at step 2 (before gateway mutations), then EnableProvidersV2, profile import, provider creation.
• Profile import moved inside providers v2 block — Correctly sequencing after EnableProvidersV2, since profiles are a providers-v2 concept. The gate condition now includes result.Profiles.
• New integration test TestRunAgent_ProviderProfileOrchestration exercises the full orchestration flow (CheckGateway → integrity check → EnableProvidersV2 → ImportProfile → EnsureProvider → CreateWithRetry) using a providers-stub.
• Lock-file reconstruction test TestResolveFromLock_ProfileAndProviderReconstruction validates combined profile + provider round-trip through the lock file.
• Compose scenario tests verify last-wins dedup semantics for both profiles and providers in base+child composition.

The resolveFromLock allowlist enforcement is correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation, with tests confirming deny-by-default when AllowedRemoteResources is empty/nil.

The security review confirms all trust-boundary controls are comprehensive: validIdentifier is applied at every code path where URL-fetched names flow into CLI arguments (resolve.go, lock.go, and ParseProfileID); the fromURL flag is correctly threaded through urlProviderNames → mergeProviderDefs → EnsureProvider → buildProviderArgs/buildProviderUpdateArgs; reservedCredentialKeys is correctly gated on fromURL; and ImportProfile uses exec.CommandContext (no shell) with a validated id.

One prior medium finding persists unchanged. The remaining low findings do not block the feature.

Findings

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 7 locations across 3 files (phase1 line 147, phase2 lines 102/134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [naming-consistency] internal/resolve/resolve.go — The validIdentifier comment states it "matches the same format as harness.validProviderName" but the patterns differ: validIdentifier requires an alphanumeric first character (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) while validProviderName allows underscore/hyphen starts (^[a-zA-Z0-9_-]+$). The stricter pattern is correct for URL-fetched content (prevents flag injection), but the comment should clarify this intentional difference rather than claiming equivalence.

Previous run (3)

Review

Commit ec1b1ed since the prior review (SHA bc91a1e) addresses four prior findings:

1. [missing-input-validation] FIXED — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) and exported ValidIdentifier() function are now applied to URL-fetched provider name and type in both resolve.go (ResolveHarness) and lock.go (resolveFromLock), and to profile id via ParseProfileID. The regex requires an alphanumeric first character, preventing leading-dash flag injection, and rejects path traversal, shell metacharacters, and whitespace. New tests confirm rejection of -h, foo;rm -rf /, ../traversal, and my profile (spaces).

2. [env-var-exfiltration-via-config] FIXED — buildProviderArgs and buildProviderUpdateArgs now skip os.ExpandEnv() on config values when fromURL=true. The fromURL flag is correctly threaded: urlProviderNames map starts from all URL-resolved providers, removes entries shadowed by local providers via mergeProviderDefs, then passes the boolean to EnsureProvider. New tests verify no secret leaking for URL providers (TestBuildProviderArgs_ConfigNotExpandedForURL) and correct expansion for local providers (TestBuildProviderArgs_ConfigExpandedForLocal). The update path is also covered (TestBuildProviderUpdateArgs_ConfigNotExpandedForURL).

3. [incomplete-blocklist] FIXED — GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, and GIT_ASKPASS added to reservedCredentialKeys, closing the git command execution vector via credential keys from URL-fetched providers. The blocklist remains case-insensitive via strings.ToUpper(k).

4. [missing-context-timeout] FIXED — ImportProfile now accepts an id parameter for delete-before-import idempotency and wraps both the delete and import commands in context.WithTimeout(ctx, providerTimeout), matching the timeout pattern used by EnsureProvider and directory-based ImportProfiles.

Additionally:

• [doc-inconsistency] FIXED — ADR renumbered from 0068 to 0069. Title, heading (now # 69.), and filename are consistent. All cross-references in ADRs 0024, 0038, architecture.md, and customizing-agents.md updated. No stale references to the old 0068 portable-provider ADR remain.
• ADR content updated — The "Integration in run.go" section now matches the actual implementation ordering: fail-fast referential integrity check at step 2 (before gateway mutations), then EnableProvidersV2, profile import, provider creation.
• Profile import moved inside providers v2 block — Correctly sequencing after EnableProvidersV2, since profiles are a providers-v2 concept.
• New integration test TestRunAgent_ProviderProfileOrchestration exercises the full orchestration flow (CheckGateway → integrity check → EnableProvidersV2 → ImportProfile → EnsureProvider → CreateWithRetry) using a providers-stub.
• Lock-file reconstruction test TestResolveFromLock_ProfileAndProviderReconstruction validates combined profile + provider round-trip through the lock file.
• Compose scenario tests verify last-wins dedup semantics for both profiles and providers in base+child composition.

The resolveFromLock allowlist enforcement is correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation, with tests confirming deny-by-default when AllowedRemoteResources is empty/nil.

One prior medium finding persists unchanged. The remaining low findings do not block the feature.

Findings

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 7 locations across 3 files (phase1 line 147, phase2 lines 102/134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

Previous run (4)

Review

Commit ec1b1ed since the prior review (SHA bc91a1e) addresses four prior findings:

1. [missing-input-validation] FIXED — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) and exported ValidIdentifier() function are now applied to URL-fetched provider name and type in both resolve.go (ResolveHarness) and lock.go (resolveFromLock), and to profile id via ParseProfileID. The regex requires an alphanumeric first character, preventing leading-dash flag injection, and rejects path traversal, shell metacharacters, and whitespace. New tests confirm rejection of -h, foo;rm -rf /, ../traversal, and my profile (spaces).

2. [env-var-exfiltration-via-config] FIXED — buildProviderArgs and buildProviderUpdateArgs now skip os.ExpandEnv() on config values when fromURL=true. The fromURL flag is correctly threaded: urlProviderNames map starts from all URL-resolved providers, removes entries shadowed by local providers via mergeProviderDefs, then passes the boolean to EnsureProvider. New tests verify no secret leaking for URL providers (TestBuildProviderArgs_ConfigNotExpandedForURL) and correct expansion for local providers (TestBuildProviderArgs_ConfigExpandedForLocal). The update path is also covered (TestBuildProviderUpdateArgs_ConfigNotExpandedForURL).

3. [incomplete-blocklist] FIXED — GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, and GIT_ASKPASS added to reservedCredentialKeys, closing the git command execution vector via credential keys from URL-fetched providers. The blocklist remains case-insensitive via strings.ToUpper(k).

4. [missing-context-timeout] FIXED — ImportProfile now accepts an id parameter for delete-before-import idempotency and wraps both the delete and import commands in context.WithTimeout(ctx, providerTimeout), matching the timeout pattern used by EnsureProvider and directory-based ImportProfiles.

Additionally:

• [doc-inconsistency] FIXED — ADR renumbered from 0068 to 0069. Title, heading (now # 69.), and filename are consistent. All cross-references in ADRs 0024, 0038, architecture.md, and customizing-agents.md updated. No stale references to the old 0068 portable-provider ADR remain.
• ADR content updated — The "Integration in run.go" section now matches the actual implementation ordering: fail-fast referential integrity check at step 2 (before gateway mutations), then EnableProvidersV2, profile import, provider creation.
• Profile import moved inside providers v2 block — Correctly sequencing after EnableProvidersV2, since profiles are a providers-v2 concept.
• New integration test TestRunAgent_ProviderProfileOrchestration exercises the full orchestration flow (CheckGateway → integrity check → EnableProvidersV2 → ImportProfile → EnsureProvider → CreateWithRetry) using a providers-stub.
• Lock-file reconstruction test TestResolveFromLock_ProfileAndProviderReconstruction validates combined profile + provider round-trip through the lock file.
• Compose scenario tests verify last-wins dedup semantics for both profiles and providers in base+child composition.

The resolveFromLock allowlist enforcement is correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation, with tests confirming deny-by-default when AllowedRemoteResources is empty/nil.

One prior medium finding persists unchanged. The remaining low findings do not block the feature.

Findings

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 7 locations across 3 files (phase1 line 147, phase2 lines 102/134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

Previous run (5)

Review

The branch was rebased since the prior review (SHA bb0d3eb); commit bc91a1e addresses four prior findings:

1. [missing-input-validation] FIXED — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) is now applied to URL-fetched provider name and type in both resolve.go and lock.go, and to profile id via ParseProfileID. The regex correctly requires an alphanumeric first character, preventing leading-dash flag injection, and rejects path traversal, shell metacharacters, and whitespace. Tests confirm rejection of -h, foo;rm -rf /, ../traversal, and my profile (spaces). The exported ValidIdentifier function enables consistent validation at both the resolve and lock-file reconstruction trust boundaries.

2. [env-var-exfiltration-via-config] FIXED — buildProviderArgs and buildProviderUpdateArgs now skip os.ExpandEnv() on config values when fromURL=true. URL-fetched provider config values containing ${SECRET_VAR} patterns are preserved as literal strings and passed to openshell without host environment expansion. Local provider config values continue to expand as before. Tests verify no secret leaking for URL providers (TestBuildProviderArgs_ConfigNotExpandedForURL) and correct expansion for local providers (TestBuildProviderArgs_ConfigExpandedForLocal). The update path is also covered (TestBuildProviderUpdateArgs_ConfigNotExpandedForURL).

3. [incomplete-blocklist] FIXED — GIT_SSH_COMMAND, GIT_TEMPLATE_DIR, and GIT_ASKPASS added to reservedCredentialKeys, closing the git command execution vector via credential keys from URL-fetched providers.

4. [missing-context-timeout] FIXED — ImportProfile now wraps both the delete and import commands in context.WithTimeout(ctx, providerTimeout), matching the timeout pattern used by EnsureProvider and the directory-based ImportProfiles.

The fromURL trust boundary is correctly enforced end-to-end: urlProviderNames is built from all URL-resolved providers, then entries shadowed by local providers are removed. The resulting map is passed to EnsureProvider as fromURL, which gates both the reservedCredentialKeys check and the os.ExpandEnv skip on config values. TestEnsureProvider_AllowsReservedKeysForLocalProviders confirms local providers bypass the reserved key check (they are org-authored and trusted).

The ADR was renumbered from 0068 to 0069 with all cross-references updated (ADRs 0024, 0038, architecture.md, customizing-agents.md). No stale references to the old portable-provider ADR 0068 remain — the existing references to "ADR 0068" in the codebase are for 0068-public-community-mint-architecture.md, a different ADR.

One prior medium finding persists unchanged. Neither it nor the remaining low findings block the feature.

Findings

Medium

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 7 locations across 3 files (phase1 line 147, phase2 lines 102/134/137, main plan lines 316/318/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [doc-inconsistency] docs/ADRs/0069-portable-provider-profile-resolution.md — The frontmatter title says "69. Portable provider and profile resolution" but the markdown heading reads # 68. Portable provider and profile resolution. The heading number does not match the filename (0069) or the title.

Previous run (6)

Review

The branch was rebased since the prior review (SHA 1dc0e89); commit bb0d3eb is a rebase with no production code changes relative to the prior review. All prior findings persist unchanged. The [missing-input-validation] medium finding from earlier iterations remains fixed — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) is correctly applied to URL-fetched provider name and type fields in both resolve.go and lock.go.

The resolveFromLock allowlist enforcement remains correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation. Tests confirm deny-by-default semantics. The reservedCredentialKeys blocklist (27 entries) correctly gates on fromURL — local org-authored providers bypass the check. Context propagation is correct: EnsureProvider and updateProvider use the passed-in ctx as the timeout parent. ImportProfile uses exec.CommandContext (no shell), so the id parameter is safe from injection. Profile and provider dedup logic (dedupResolvedProviders, dedupResolvedProfiles) correctly implements last-wins semantics with comprehensive test coverage including compose scenarios. mergeProviderDefs correctly shadows URL-resolved providers with local defs. checkProviderProfileIntegrity validates all provider types against profile IDs and reports all mismatches in a single error. sandboxProviderNames correctly deduplicates via a seen map.

One prior medium finding persists unchanged. One medium finding regarding ImportProfiles backward compatibility persists. Neither blocks the feature.

Findings

Medium

• [env-var-exfiltration-via-config] internal/sandbox/sandbox.go — ProviderDef.Config map values undergo os.ExpandEnv() in buildProviderArgs and are passed as --config KEY=VALUE CLI arguments on the process command line. The fromURL gating added in this PR only covers reservedCredentialKeys for credential keys — config keys and config values from URL-fetched providers are not validated or restricted. A URL-fetched provider could include config values containing ${SECRET_VAR} references to expand host environment variables into openshell config flags, where they appear in /proc/PID/cmdline. WarnLiteralCredentials only inspects credential values, not config values. The same gap exists in buildProviderUpdateArgs.
  Remediation: Either (1) do not call os.ExpandEnv on config values from URL-fetched providers — require them to be literal values, or (2) apply the reservedCredentialKeys check to config keys when fromURL is true, or (3) apply equivalent ${VAR} pattern warnings to config values in URL-fetched providers.

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [incomplete-blocklist] internal/sandbox/sandbox.go — reservedCredentialKeys includes GIT_CONFIG_GLOBAL and GIT_EXEC_PATH but omits GIT_SSH_COMMAND (executes arbitrary command when git uses SSH), GIT_TEMPLATE_DIR (can inject hooks via template directories), and GIT_ASKPASS (runs a program to capture credentials). Attack surface is narrow since exploitation requires control over a SHA256-pinned provider YAML.

• [missing-context-timeout] internal/sandbox/sandbox.go — ImportProfile's main import command (openshell provider profile import) uses the parent ctx directly without a timeout wrapper. The delete command above correctly uses context.WithTimeout(ctx, providerTimeout), EnsureProvider also wraps its commands in a timeout, and the directory-based ImportProfiles wraps each import in context.WithTimeout(context.Background(), providerTimeout). The single-profile ImportProfile does not, meaning it could hang indefinitely if the caller passes a context without a deadline.
  Remediation: Wrap the import command in context.WithTimeout(ctx, providerTimeout) to match the pattern used elsewhere.

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 5 locations across 3 files (phase1 line 147, phase2 lines 134/137, main plan lines 316/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [doc-inconsistency] docs/ADRs/0069-portable-provider-profile-resolution.md — The frontmatter title says "69. Portable provider and profile resolution" but the markdown heading reads # 68. Portable provider and profile resolution. The heading number does not match the filename (0069) or the title.

Previous run (7)

Review

The branch was rebased since the prior review (SHA 1dc0e89); commit bb0d3eb is a rebase with no production code changes relative to the prior review. All prior findings persist unchanged. The [missing-input-validation] medium finding from earlier iterations remains fixed — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) is correctly applied to URL-fetched provider name and type fields in both resolve.go and lock.go.

The resolveFromLock allowlist enforcement remains correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation. Tests confirm deny-by-default semantics. The reservedCredentialKeys blocklist (27 entries) correctly gates on fromURL — local org-authored providers bypass the check. Context propagation is correct: EnsureProvider and updateProvider use the passed-in ctx as the timeout parent. ImportProfile uses exec.CommandContext (no shell), so the id parameter is safe from injection. Profile and provider dedup logic (dedupResolvedProviders, dedupResolvedProfiles) correctly implements last-wins semantics with comprehensive test coverage including compose scenarios. mergeProviderDefs correctly shadows URL-resolved providers with local defs. checkProviderProfileIntegrity validates all provider types against profile IDs and reports all mismatches in a single error. sandboxProviderNames correctly deduplicates via a seen map.

One prior medium finding persists unchanged. One medium finding regarding ImportProfiles backward compatibility persists. Neither blocks the feature.

Findings

Medium

• [env-var-exfiltration-via-config] internal/sandbox/sandbox.go — ProviderDef.Config map values undergo os.ExpandEnv() in buildProviderArgs and are passed as --config KEY=VALUE CLI arguments on the process command line. The fromURL gating added in this PR only covers reservedCredentialKeys for credential keys — config keys and config values from URL-fetched providers are not validated or restricted. A URL-fetched provider could include config values containing ${SECRET_VAR} references to expand host environment variables into openshell config flags, where they appear in /proc/PID/cmdline. WarnLiteralCredentials only inspects credential values, not config values. The same gap exists in buildProviderUpdateArgs.
  Remediation: Either (1) do not call os.ExpandEnv on config values from URL-fetched providers — require them to be literal values, or (2) apply the reservedCredentialKeys check to config keys when fromURL is true, or (3) apply equivalent ${VAR} pattern warnings to config values in URL-fetched providers.

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [incomplete-blocklist] internal/sandbox/sandbox.go — reservedCredentialKeys includes GIT_CONFIG_GLOBAL and GIT_EXEC_PATH but omits GIT_SSH_COMMAND (executes arbitrary command when git uses SSH), GIT_TEMPLATE_DIR (can inject hooks via template directories), and GIT_ASKPASS (runs a program to capture credentials). Attack surface is narrow since exploitation requires control over a SHA256-pinned provider YAML.

• [missing-context-timeout] internal/sandbox/sandbox.go — ImportProfile's main import command (openshell provider profile import) uses the parent ctx directly without a timeout wrapper. The delete command above correctly uses context.WithTimeout(ctx, providerTimeout), EnsureProvider also wraps its commands in a timeout, and the directory-based ImportProfiles wraps each import in context.WithTimeout(context.Background(), providerTimeout). The single-profile ImportProfile does not, meaning it could hang indefinitely if the caller passes a context without a deadline.
  Remediation: Wrap the import command in context.WithTimeout(ctx, providerTimeout) to match the pattern used elsewhere.

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 5 locations across 3 files (phase1 line 147, phase2 lines 134/137, main plan lines 316/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [doc-inconsistency] docs/ADRs/0069-portable-provider-profile-resolution.md — The frontmatter title says "69. Portable provider and profile resolution" but the markdown heading reads # 68. Portable provider and profile resolution. The heading number does not match the filename (0069) or the title.

Previous run (8)

Review

The branch was rebased since the prior review (SHA 9c364d9); commit 1dc0e89 addresses the prior [missing-input-validation] finding — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) is now applied to URL-fetched provider name and type fields in both resolve.go and lock.go (the resolveFromLock path). Test coverage confirms rejection of leading-hyphen flag injection (-h), shell metacharacters (foo;rm -rf /), and path traversal (../traversal).

The resolveFromLock allowlist enforcement is correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation. TestResolveFromLock_RejectsDisallowedURL and TestResolveFromLock_EmptyAllowlistDeniesURLs confirm deny-by-default semantics. The reservedCredentialKeys blocklist (27 entries) correctly gates on fromURL — local org-authored providers bypass the check (TestEnsureProvider_AllowsReservedKeysForLocalProviders). Context propagation is fixed: EnsureProvider and updateProvider now use the passed-in ctx as the timeout parent. ImportProfile uses exec.CommandContext (no shell), so the id parameter is safe from injection. Profile and provider dedup logic (dedupResolvedProviders, dedupResolvedProfiles) correctly implements last-wins semantics with comprehensive test coverage including compose scenarios. mergeProviderDefs correctly shadows URL-resolved providers with local defs, logging warnings for shadowed names. checkProviderProfileIntegrity validates all provider types against profile IDs and reports all mismatches in a single error. sandboxProviderNames correctly deduplicates via a seen map.

One prior medium finding persists unchanged. One new medium finding was identified regarding ImportProfiles backward compatibility. Neither blocks the feature.

Findings

Medium

• [env-var-exfiltration-via-config] internal/sandbox/sandbox.go — ProviderDef.Config map values undergo os.ExpandEnv() in buildProviderArgs and are passed as --config KEY=VALUE CLI arguments on the process command line. The fromURL gating added in this PR only covers reservedCredentialKeys for credential keys — config keys and config values from URL-fetched providers are not validated or restricted. A URL-fetched provider could include config values containing ${SECRET_VAR} references to expand host environment variables into openshell config flags, where they appear in /proc/PID/cmdline. WarnLiteralCredentials only inspects credential values, not config values. The same gap exists in buildProviderUpdateArgs.
  Remediation: Either (1) do not call os.ExpandEnv on config values from URL-fetched providers — require them to be literal values, or (2) apply the reservedCredentialKeys check to config keys when fromURL is true, or (3) apply equivalent ${VAR} pattern warnings to config values in URL-fetched providers.

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [incomplete-blocklist] internal/sandbox/sandbox.go — reservedCredentialKeys includes GIT_CONFIG_GLOBAL and GIT_EXEC_PATH but omits GIT_SSH_COMMAND (executes arbitrary command when git uses SSH), GIT_TEMPLATE_DIR (can inject hooks via template directories), and GIT_ASKPASS (runs a program to capture credentials). Attack surface is narrow since exploitation requires control over a SHA256-pinned provider YAML.

• [missing-context-timeout] internal/sandbox/sandbox.go — ImportProfile's main import command (openshell provider profile import) uses the parent ctx directly without a timeout wrapper. The delete command above correctly uses context.WithTimeout(ctx, providerTimeout), EnsureProvider also wraps its commands in a timeout, and the directory-based ImportProfiles wraps each import in context.WithTimeout(context.Background(), providerTimeout). The single-profile ImportProfile does not, meaning it could hang indefinitely if the caller passes a context without a deadline.
  Remediation: Wrap the import command in context.WithTimeout(ctx, providerTimeout) to match the pattern used elsewhere.

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 5 locations across 3 files (phase1 line 147, phase2 lines 134/137, main plan lines 316/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [doc-inconsistency] docs/ADRs/0068-portable-provider-profile-resolution.md — The ADR describes referential integrity validation at step 4 (after loading local defs and merging), but the implementation checks it at step 2b (before loading local defs). The implementation's fail-fast ordering is better, but diverges from the documented flow.

Previous run (9)

Review

The branch was rebased since the prior review (SHA 9c364d9); commit 1dc0e89 addresses the prior [missing-input-validation] finding — validIdentifier regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) is now applied to URL-fetched provider name and type fields in both resolve.go and lock.go (the resolveFromLock path). Test coverage confirms rejection of leading-hyphen flag injection (-h), shell metacharacters (foo;rm -rf /), and path traversal (../traversal).

The resolveFromLock allowlist enforcement is correct — h.MatchingAllowedPrefix(lockDep.URL) is checked unconditionally at the top of the loop before any cache lookup or mutation. TestResolveFromLock_RejectsDisallowedURL and TestResolveFromLock_EmptyAllowlistDeniesURLs confirm deny-by-default semantics. The reservedCredentialKeys blocklist (27 entries) correctly gates on fromURL — local org-authored providers bypass the check (TestEnsureProvider_AllowsReservedKeysForLocalProviders). Context propagation is fixed: EnsureProvider and updateProvider now use the passed-in ctx as the timeout parent. ImportProfile uses exec.CommandContext (no shell), so the id parameter is safe from injection. Profile and provider dedup logic (dedupResolvedProviders, dedupResolvedProfiles) correctly implements last-wins semantics with comprehensive test coverage including compose scenarios. mergeProviderDefs correctly shadows URL-resolved providers with local defs, logging warnings for shadowed names. checkProviderProfileIntegrity validates all provider types against profile IDs and reports all mismatches in a single error. sandboxProviderNames correctly deduplicates via a seen map.

One prior medium finding persists unchanged. One new medium finding was identified regarding ImportProfiles backward compatibility. Neither blocks the feature.

Findings

Medium

• [env-var-exfiltration-via-config] internal/sandbox/sandbox.go — ProviderDef.Config map values undergo os.ExpandEnv() in buildProviderArgs and are passed as --config KEY=VALUE CLI arguments on the process command line. The fromURL gating added in this PR only covers reservedCredentialKeys for credential keys — config keys and config values from URL-fetched providers are not validated or restricted. A URL-fetched provider could include config values containing ${SECRET_VAR} references to expand host environment variables into openshell config flags, where they appear in /proc/PID/cmdline. WarnLiteralCredentials only inspects credential values, not config values. The same gap exists in buildProviderUpdateArgs.
  Remediation: Either (1) do not call os.ExpandEnv on config values from URL-fetched providers — require them to be literal values, or (2) apply the reservedCredentialKeys check to config keys when fromURL is true, or (3) apply equivalent ${VAR} pattern warnings to config values in URL-fetched providers.

• [behavior-change-regression-risk] internal/sandbox/sandbox.go — ImportProfiles (the directory-based import for local profiles/ YAML files) now reads each profile's id from the YAML content via profileIDFromFile instead of deriving it from the filename. If any existing profile YAML in the profiles/ directory is missing an id field, ImportProfiles will return a hard error where it previously succeeded (using the filename minus extension as the id). This function is called unconditionally in the provider orchestration block, so a missing id field will fail the entire agent run. The change is correct (YAML id is more reliable than filename), but is a breaking change for existing users.
  Remediation: Consider falling back to the filename-based id when the YAML id field is absent, or document this as a migration requirement.

Low

• [incomplete-blocklist] internal/sandbox/sandbox.go — reservedCredentialKeys includes GIT_CONFIG_GLOBAL and GIT_EXEC_PATH but omits GIT_SSH_COMMAND (executes arbitrary command when git uses SSH), GIT_TEMPLATE_DIR (can inject hooks via template directories), and GIT_ASKPASS (runs a program to capture credentials). Attack surface is narrow since exploitation requires control over a SHA256-pinned provider YAML.

• [missing-context-timeout] internal/sandbox/sandbox.go — ImportProfile's main import command (openshell provider profile import) uses the parent ctx directly without a timeout wrapper. The delete command above correctly uses context.WithTimeout(ctx, providerTimeout), EnsureProvider also wraps its commands in a timeout, and the directory-based ImportProfiles wraps each import in context.WithTimeout(context.Background(), providerTimeout). The single-profile ImportProfile does not, meaning it could hang indefinitely if the caller passes a context without a deadline.
  Remediation: Wrap the import command in context.WithTimeout(ctx, providerTimeout) to match the pattern used elsewhere.

• [behavior-change] internal/cli/run.go — The "Provider declared but no definition found" warning was removed. If a user declares providers: [misspelled-name] and no matching YAML exists in providers/, the name still appears in sandboxProviderNames and is passed to CreateWithRetry, which may fail at the openshell level. The loss of the explicit warning makes debugging harder.

• [stale-doc] docs/plans/universal-harness-access*.md — Plan documents reference the old ResolveHarness return type ([]Dependency, error) in 5 locations across 3 files (phase1 line 147, phase2 lines 134/137, main plan lines 316/1066). The return type changed to (ResolveResult, error) in this PR. These are historical plan documents, not user-facing API docs.

• [doc-inconsistency] docs/ADRs/0068-portable-provider-profile-resolution.md — The ADR describes referential integrity validation at step 4 (after loading local defs and merging), but the implementation checks it at step 2b (before loading local defs). The implementation's fail-fast ordering is better, but diverges from the documented flow.

[maruiz93]

Keeping warn-only as designed for WarnLiteralCredentials. A hard error would produce false positives for compound ${VAR} expressions (e.g., ${HOST}:${PORT}) and brace-less $VAR syntax — both are legitimate credential value patterns. The current regex requires the entire value to be a single ${VAR} reference, which is intentionally strict as a warning but would be too aggressive as a hard stop.

Regarding checkProviderProfileIntegrity warn vs error: this is intentional per ADR 0066 §Referential Integrity. Providers can exist without profiles (e.g., local providers whose types are validated by the gateway at creation time). The warning covers the case where URL-resolved providers exist but no URL-resolved profiles are declared — integrity can't be verified ahead of time, but this isn't necessarily an error.

All other findings addressed in 5bcc974:

• Renamed profiles → openshell-profiles (YAML field, errors, tests, ADR 0066) to disambiguate the overloaded term
• Extended reservedCredentialKeys with IFS, CDPATH, DYLD_INSERT_LIBRARIES, JAVA_TOOL_OPTIONS, RUBYOPT, PERL5OPT
• Added TestResolveHarness_ProviderURLMissingType
• Fixed ImportProfile duplicate check to use "profile already exists" instead of broad "duplicate"
• Added ADR cross-references from 0024 and 0038 to 0066
• Clarified "policy" as "openshell policy" in ADR 0066
• Documented openshell-profiles and URL-based providers in user guide
• Added ImportProfile step to sandbox lifecycle in cli-internals.md

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:45 PM UTC · Completed 4:00 PM UTC
Commit: 5bcc974 · View workflow run →

[waynesun09]

Review Squad Report — #3062

Agents dispatched: 5 (claude-coder ×2, claude-researcher, gemini-code-review, cursor-code-review)
Models used: Claude, Gemini, Codex
Total findings: 14 (after dedup + verification) — 6 posted inline (MEDIUM+)
False positives removed: 5

Summary

Solid, well-structured PR that cleanly extends the existing resolution system. The strongest finding is the incomplete reserved credential key blocklist (missing LD_AUDIT and TLS trust chain variables), which 4/5 agents flagged independently. The h.Profiles not being cleared after resolution is a real asymmetry with h.Providers that should be fixed for consistency. Remaining findings are hardening opportunities around error message clarity and warning-vs-error enforcement for literal credentials.

Assisted-by: Claude (review), Gemini (review), Codex (review)

[maruiz93]

Addressed findings from this review in 401384a:

• [logic-error] critical — URL-resolved provider names now passed to CreateWithRetry via collectProviderNames(allDefs). Added TestCollectProviderNames covering local+URL, URL-only, local-only, and empty cases.
• [inconsistent-blocklist] — Noted; reservedSandboxKeys synchronization deferred (separate concern from this PR's scope).
• [missing-doc] architecture.md — Will add ADR 0066 reference.

Remaining low findings are either already addressed (ImportProfile duplicate check, ADR cross-refs) or intentional per ADR 0066 (WarnLiteralCredentials warn-only, checkProviderProfileIntegrity warn for no-profiles case, lock deps[len-1] pattern).

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:50 AM UTC · Ended 10:06 AM UTC
Commit: e8381e3 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:06 AM UTC · Completed 10:20 AM UTC
Commit: 5a56f20 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:22 PM UTC · Ended 9:34 PM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:34 PM UTC · Completed 9:46 PM UTC
Commit: 1dc0e89 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:47 PM UTC · Ended 9:56 PM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:56 PM UTC · Completed 10:09 PM UTC
Commit: bb0d3eb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:20 PM UTC · Completed 10:32 PM UTC
Commit: bc91a1e · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:42 PM UTC · Ended 10:57 PM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:58 PM UTC · Completed 11:08 PM UTC
Commit: ec1b1ed · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 11:16 PM UTC · Ended 11:19 PM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:20 PM UTC · Completed 11:31 PM UTC
Commit: c846e51 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:33 PM UTC · Completed 11:43 PM UTC
Commit: e9f8f39 · View workflow run →

[waynesun09]

Reviewed at HEAD e9f8f39d. All 7 findings from my last review-squad pass (3 HIGH, 4 MEDIUM) are fixed and verified via the reply threads — unvalidated profile id now goes through validIdentifier, URL-resolved provider Name/Type get the same validation as local ones, the new orchestration block now has an integration test (TestRunAgent_ProviderProfileOrchestration) exercising the real CheckGateway → integrity check → EnableProvidersV2 → ImportProfile → EnsureProvider path, and the lock/compose test-coverage gaps are filled. fullsend-ai-review[bot]'s subsequent rounds independently confirmed the same fixes plus closed additional gaps (config-value env exfiltration, blocklist completeness, context timeouts).

All CI checks are green (build, test, e2e, functional-tests, commit-lint, DCO).

One open item: the bot's CHANGES_REQUESTED review asks for a ! breaking-change marker on the PR title, since ImportProfiles now requires a profile YAML's id field instead of falling back to the filename. I agree with the author's rebuttal — openshell itself already hard-requires id (ProfileError::MissingId), so a profile lacking it was never functional; this only moves the failure earlier with a clearer message. Treating this as a non-blocking labeling/process disagreement rather than a functional or security issue.

No other major issues found. Approving.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:45 PM UTC · Completed 1:56 PM UTC
Commit: 031dedd · View workflow run →

[fullsend-ai-review]

See the review comment for full details.