feat(compose): resolve local profiles and providers in base harness (#5240)

[maruiz93]

Summary

• Add resolveBaseProfiles and resolveBaseProviders to harness composition so local paths in base harnesses are fetched and cached, matching existing behavior for skills, agent, policy, and scripts
• Extract IsProviderPath helper to distinguish bare provider names (e.g. fullsend-github) from file paths, consolidating a heuristic previously duplicated in 3 locations
• Extract parseProviderDef helper to deduplicate provider YAML validation between local-path and URL branches
• Add Warnings field to ResolveResult so credential warnings from local providers surface to the user
• Wire local-only resolution path in run.go with result merging to preserve lock-file deps/providers
• Add ValidateFilesExist checks for profiles and provider paths

Closes #5240

Test plan

• TestResolveBaseProfiles — 5 cases: URL, relative, absolute, mixed, empty
• TestResolveBaseProviders — 6 cases: URL, bare name, relative path, absolute, mixed, empty
• TestIsProviderPath — 9 cases covering bare names, slashes, YAML extensions
• TestParseProviderDef — 7 cases: valid, missing name/type, invalid chars, credential warning
• TestResolveHarness_LocalProviderWarnings — end-to-end warning propagation
• TestValidateFilesExist_Missing{Profile,ProviderPath} and bare-name skip
• Consolidated overlapping traversal/unchanged tests into table-driven patterns
• All harness and resolve tests pass (go test ./internal/harness/... ./internal/resolve/...)

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Resolve local profiles/providers when composing and running base harnesses

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Fetch and cache relative profile/provider paths from URL base harnesses.
• Treat local profiles and absolute provider paths as resolvable resources.
• Surface provider credential warnings and strengthen path/file validation.

Diagram

graph TD
  A["CLI runAgent"] --> B["compose.LoadWithBase"] --> C["resolveBaseProfiles/Providers"] --> D["fetchBaseFile"] --> E[("Cache")]
  A --> F["resolve.ResolveHarness"] --> G["Warn + merge results"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Require file:// URLs (with optional integrity) for local profiles/providers

• ➕ Single URL-based codepath; fewer special cases for local paths
• ➕ Could reuse existing integrity-hash validation model for all resources
• ➖ More verbose authoring and a breaking UX change for existing local configs
• ➖ Still needs careful handling of relative paths in base composition

2. Resolve all local resources during composition only (no CLI fallback)

• ➕ Keeps resolution concerns in one layer (compose), simplifying run-time logic
• ➕ Would avoid the local-only ResolveHarness invocation/merge in CLI
• ➖ Composition doesn’t naturally parse provider YAML/profile IDs (resolve currently does)
• ➖ Risk of duplicating parsing/validation logic between compose and resolve

3. Extend lockfile to capture local provider/profile digests and warnings

• ➕ More reproducible builds; local paths could be pinned by content hash
• ➕ Warnings could be tracked deterministically as part of the resolved state
• ➖ Additional design and migration work; larger change surface than needed for feat(compose): support local-path resolution for profiles and providers during base composition #5240
• ➖ Still needs runtime handling when lockfile is absent/outdated

Recommendation: Keep the PR’s approach: it aligns profiles/providers with existing base-resource behavior (fetch+cache on URL bases) while minimally expanding ResolveHarness to support local paths. The added CLI fallback and result merging is pragmatic to preserve lockfile/URL-resolved data without requiring a larger lockfile redesign.

Files changed (9) +751 / -40

Enhancement (2) +160 / -29

compose.goFetch/cache base profiles and provider file paths from URL bases +100/-0

Fetch/cache base profiles and provider file paths from URL bases

• Introduces resolveBaseProfiles and resolveBaseProviders to fetch relative paths referenced by URL base harnesses, caching them content-addressed and rewriting harness entries to local cache paths. Wires this into both LoadWithBase and base-chain loading to match existing agent/skill/script/host_file behavior.

internal/harness/compose.go

resolve.goResolve local profiles and absolute provider paths; propagate warnings +60/-29

Resolve local profiles and absolute provider paths; propagate warnings

• Updates ResolveHarness to accept local profile paths (read directly) alongside URL-based profiles (fetch+cache). Adds absolute-path provider resolution by parsing local YAML into ProviderDef while preserving bare names for later provider loading; introduces parseProviderDef helper and surfaces local-provider credential warnings via a new ResolveResult.Warnings field.

internal/resolve/resolve.go

Bug fix (2) +68 / -9

run.goRun local-only profile/provider resolution and print warnings +31/-0

Run local-only profile/provider resolution and print warnings

• Adds a local-only resolution path when profiles/providers are present as local paths without URL references, ensuring they are parsed into resolved structures. Merges newly resolved deps/providers into any existing resolution result and emits ResolveResult warnings to the user.

internal/cli/run.go

harness.goAllow local profiles, resolve provider/profile relpaths, and validate file existence +37/-9

Allow local profiles, resolve provider/profile relpaths, and validate file existence

• Extends relative-path resolution to openshell profiles and provider file paths (while leaving bare provider names unchanged). Updates validation to accept local profile paths (URLs still require integrity hashes), improves HasURLReferences to check profiles individually, and adds ValidateFilesExist checks for profiles and provider paths.

internal/harness/harness.go

Refactor (1) +8 / -0

url.goExtract IsProviderPath helper for name-vs-path detection +8/-0

Extract IsProviderPath helper for name-vs-path detection

• Adds IsProviderPath heuristic to distinguish bare provider names from file-path-like provider entries (slash or .yaml/.yml suffix). Centralizes logic previously duplicated across resolution and validation paths.

internal/harness/url.go

Tests (4) +515 / -2

compose_test.goAdd URL-base composition tests for profile/provider resolution +230/-0

Add URL-base composition tests for profile/provider resolution

• Adds end-to-end tests verifying relative profile/provider fetching, cache rewrites, and dependency recording when base harnesses are URL-sourced. Includes a regression test ensuring bare provider names are skipped (not fetched as relative paths).

internal/harness/compose_test.go

harness_test.goCover profile/provider relative resolution and new file-existence checks +123/-2

Cover profile/provider relative resolution and new file-existence checks

• Adds tests for profile relpath resolution, provider relpath vs bare-name handling, and traversal rejection. Adds ValidateFilesExist tests for missing profile/provider paths and verifies bare provider names are skipped; updates URL-reference and profile validation expectations.

internal/harness/harness_test.go

url_test.goAdd IsProviderPath unit tests +23/-0

Add IsProviderPath unit tests

• Introduces table-driven coverage for bare names, slashes, YAML extensions, absolute paths, and empty string behavior.

internal/harness/url_test.go

resolve_test.goAdd ResolveHarness tests for local profiles/providers and warning propagation +139/-0

Add ResolveHarness tests for local profiles/providers and warning propagation

• Adds coverage for resolving a local profile path, resolving an absolute provider YAML while retaining bare provider names, parsing/validating provider YAML via parseProviderDef, and ensuring literal-credential warnings from local providers surface in ResolveResult.Warnings.

internal/resolve/resolve_test.go

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:39 PM UTC · Ended 1:41 PM UTC
Commit: 84d4a79 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:43 PM UTC · Completed 2:01 PM UTC
Commit: 1daf9b1 · View workflow run →

[github-actions]

Site preview

Preview: https://ab98e092-site.fullsend-ai.workers.dev

Commit: 893ec298fd02e96e64693a7ffc0e10019d1b120a

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Lock skips local providers ✓ Resolved 🐞 Bug ≡ Correctness

Description

In runAgent, the local-only ResolveHarness fallback is gated by len(result.Profiles)==0 and only
merges deps/providers, so when lock-file resolution already produced profiles, any remaining local
provider file paths in h.Providers are never parsed into result.Providers. This can prevent provider
creation because LoadProviderDefs filters by provider name (def.Name), not file paths, and it
suppresses local-provider credential warnings.

Code

internal/cli/run.go[R481-498]

+	// When profiles or providers use local paths (from ResolveRelativeTo or
+	// base composition), ResolveHarness must still run to parse them into
+	// ResolvedProfile/ResolvedProvider — even without URL references.
+	// Merge into any existing result to avoid discarding deps/providers
+	// already resolved by the lock-file or URL-resolution path.
+	if len(result.Profiles) == 0 && (len(h.OpenShellProfiles()) > 0 || hasLocalProviders(h)) {
+		prevDeps := result.Deps
+		prevProviders := result.Providers
+		var resolveErr error
+		result, resolveErr = resolve.ResolveHarness(ctx, h, resolve.ResolveOpts{
+			WorkspaceRoot: absFullsendDir,
+		})
+		if resolveErr != nil {
+			return fmt.Errorf("resolving local profiles/providers: %w", resolveErr)
+		}
+		result.Deps = append(prevDeps, result.Deps...)
+		result.Providers = append(prevProviders, result.Providers...)
+	}

Relevance

⭐⭐ Medium

No prior review evidence on ResolveHarness fallback gating with lock results; related lock/resolve
work in #2082/#3062.

PR-#2082
PR-#3062

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new fallback in runAgent is skipped when lock resolution produces any profiles, even if local
provider paths still exist in h.Providers; later provider loading filters by provider *name*
(def.Name), so those path entries won’t load any provider YAMLs unless ResolveHarness converts them
to ResolvedProvider first and removes them from h.Providers.

internal/cli/run.go[481-501]
internal/cli/run.go[711-819]
internal/cli/lock.go[656-785]
internal/harness/harness.go[53-102]
internal/resolve/resolve.go[293-347]

Agent prompt

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

### Issue description
`runAgent`’s local-only resolution block is guarded by `len(result.Profiles) == 0` and only merges `Deps`/`Providers`. This means that when the lock-file path already populated `result.Profiles`, local provider file paths (absolute cache paths from base composition or ResolveRelativeTo) can remain in `h.Providers` and never get converted into `ResolvedProvider` entries.

### Issue Context
- Lock resolution (`resolveFromLock`) strips URL providers from `h.Providers` but keeps non-URL entries, so absolute provider YAML paths can remain in `h.Providers`.
- Provider creation later relies on `result.Providers` (resolved defs) + `LoadProviderDefs(providersDir, declaredNames)` where `declaredNames` is built from `h.Providers` entries; passing absolute paths there will not match `def.Name` and can lead to providers not being loaded/created.

### Fix Focus Areas
- internal/cli/run.go[481-501]
- internal/cli/run.go[728-819]
- internal/cli/lock.go[656-785]
- internal/harness/harness.go[53-102]
- internal/resolve/resolve.go[266-347]

### What to change
1. Change the fallback condition to run whenever *unresolved local* profiles/providers may exist (e.g., `len(h.OpenShellProfiles()) > 0 || hasLocalProviders(h)`), not based on `len(result.Profiles)`.
2. When invoking `ResolveHarness` for local-only parsing, **merge all fields**, not just deps/providers:
  - Preserve `prevProfiles := result.Profiles` and `prevWarnings := result.Warnings`.
  - After local resolve, do `result.Profiles = append(prevProfiles, result.Profiles...)`, `result.Warnings = append(prevWarnings, result.Warnings...)`, and keep the existing deps/providers merge.
3. Update `resolveFromLock` to avoid wiping non-URL local profiles:
  - Instead of unconditional `h.OpenShell.Profiles = nil`, filter out only URL profile entries and keep local-path entries so the local-only `ResolveHarness` pass can parse them into `ResolvedProfile`.

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

---

2. Windows provider paths misdetected ✗ Dismissed 🐞 Bug ☼ Reliability

Description

IsProviderPath only checks for "/" separators, so Windows-style relative paths using "\\" can be
misclassified as bare provider names, skipping ResolveRelativeTo/ValidateFilesExist path handling.
This can leave provider file paths unresolved/unchecked and later treated as provider names.

Code

internal/harness/url.go[R25-30]

+// IsProviderPath returns true if s looks like a file path rather than a bare
+// provider name. A provider string is a path if it contains a directory
+// separator or ends with a YAML extension.
+func IsProviderPath(s string) bool {
+	return strings.Contains(s, "/") || strings.HasSuffix(s, ".yaml") || strings.HasSuffix(s, ".yml")
+}

Relevance

⭐⭐ Medium

No historical suggestions about Windows '\\' path handling in IsProviderPath; related URL helper
work exists but not OS-specific.

PR-#1095

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The heuristic only checks for forward slashes, but it’s used as the gate for resolving provider
paths and for skipping provider-name validation; on Windows, backslash-separated paths may not be
handled as intended.

internal/harness/url.go[25-30]
internal/harness/harness.go[415-422]
internal/harness/harness.go[585-592]
internal/harness/harness.go[710-715]

Agent prompt

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

### Issue description
`IsProviderPath` detects paths by checking for `"/"` or a `.yaml/.yml` suffix. On Windows, relative paths can be written/constructed with `"\\"`, which may not contain `"/"`.

### Issue Context
`IsProviderPath` is now used to decide whether a provider entry should be treated as a file path (resolved/checked) or as a bare provider name.

### Fix Focus Areas
- internal/harness/url.go[25-30]
- internal/harness/harness.go[415-422]
- internal/harness/harness.go[585-592]
- internal/harness/harness.go[710-715]

### What to change
- Make `IsProviderPath` recognize both separators, e.g. `strings.ContainsAny(s, "/\\")` (and keep the `.yaml/.yml` suffix checks).
- Consider adding/adjusting tests to include a Windows-style path like `providers\\custom.yaml` and/or `providers\\custom` (if extensionless paths should be treated as paths).

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

---

3. Profile errors lose URL ✓ Resolved 🐞 Bug ◔ Observability

Description

ResolveHarness now reports profile parse failures as coming from the local cache path even for URL
profiles, losing the original remote URL context. This makes malformed remote profile failures
harder to diagnose and trace to the actual source reference.

Code

internal/resolve/resolve.go[R266-289]

+	// Resolve profiles: URL entries are fetched and cached; local paths
+	// (from ResolveRelativeTo or base composition cache) are used directly.
	var profiles []ResolvedProfile
	for i, p := range h.OpenShellProfiles() {
-		if !harness.IsURL(p) {
-			return ResolveResult{}, fmt.Errorf("openshell.profiles[%d]: expected URL, got local path %q", i, p)
-		}
-		dep, localPath, err := resolveFileURL(ctx, fmt.Sprintf("openshell.profiles[%d]", i), p, h, opts, state)
-		if err != nil {
-			return ResolveResult{}, fmt.Errorf("resolving openshell.profiles[%d]: %w", i, err)
+		var localPath string
+		if harness.IsURL(p) {
+			dep, lp, err := resolveFileURL(ctx, fmt.Sprintf("openshell.profiles[%d]", i), p, h, opts, state)
+			if err != nil {
+				return ResolveResult{}, fmt.Errorf("resolving openshell.profiles[%d]: %w", i, err)
+			}
+			localPath = lp
+			state.appendDependency(dep)
+		} else {
+			localPath = p
		}

		content, err := os.ReadFile(localPath)
		if err != nil {
-			return ResolveResult{}, fmt.Errorf("reading resolved profile %s: %w", localPath, err)
+			return ResolveResult{}, fmt.Errorf("reading profile %s: %w", localPath, err)
		}
		id, err := ParseProfileID(content)
		if err != nil {
-			return ResolveResult{}, fmt.Errorf("openshell.profiles[%d]: %w (from %s)", i, err, dep.URL)
+			return ResolveResult{}, fmt.Errorf("openshell.profiles[%d]: %w (from %s)", i, err, localPath)
		}
-		state.appendDependency(dep)

Relevance

⭐⭐ Medium

No direct historical evidence on preserving remote URL context in profile parse errors; related
profile-resolution changes in #3062/#5397.

PR-#3062
PR-#5397

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In the URL branch, resolveFileURL returns dep and lp, but the error message always references
localPath, which is the cache path for URL profiles.

internal/resolve/resolve.go[266-290]

Agent prompt

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

### Issue description
For URL-backed profiles, `ResolveHarness` fetches the profile via `resolveFileURL` (which returns `dep.URL`) but then reports `ParseProfileID` errors as `(from %s)` where `%s` is the local cache path.

### Issue Context
The resolver has both the cache path and the source URL; retaining the URL in the error is more actionable.

### Fix Focus Areas
- internal/resolve/resolve.go[266-290]

### What to change
- Track a `source` string for error reporting:
 - If `harness.IsURL(p)`, set `source = dep.URL` (or the original `p`).
 - Else set `source = localPath`.
- Use `source` in the `ParseProfileID` error message, optionally including both URL and cache path if desired.

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

---

[fullsend-ai-review]

Review

Findings

Medium

• [logic-error] internal/cli/lock.go:800 — resolveFromLock strips ALL absolute-path profiles from h.OpenShell.Profiles via the !filepath.IsAbs(p) guard, and all provider paths via !harness.IsProviderPath(p). However, local-path profiles and providers resolved by ResolveRelativeTo are also absolute after resolution. When a harness combines URL references (triggering the lock-file path) with local-path profiles or providers, the local entries are stripped from h.OpenShell.Profiles / h.Providers without appearing in the lock file’s ResolvedProfile / ResolvedProvider list. They cannot be recovered by the second ResolveHarness call either, because the guards (len(h.OpenShellProfiles()) > 0 and hasLocalProviders(h)) find no remaining entries. Example: a harness with a URL-referenced skill and a local profile path (profiles/network.yaml) would silently lose the local profile when running with a lock file.
  Remediation: Track which profile/provider entries correspond to lock-file dependencies (e.g., by collecting the field names from lock deps before filtering), and only strip entries that were actually reconstructed from the lock file. Alternatively, preserve remaining absolute-path entries that have no corresponding lock dep so the second ResolveHarness pass can process them.

Low

• [arbitrary-file-read] internal/resolve/resolve.go:321 — In ResolveHarness, when a provider entry is an absolute path (filepath.IsAbs(p) is true), os.ReadFile(p) is called without a containment check against WorkspaceRoot. The same pattern exists for local profiles. In practice, upstream guards (ResolveRelativeTo containment check, validateBaseRelPath traversal rejection) prevent untrusted absolute paths from reaching here, but the defense-in-depth is absent at this layer.
  Remediation: Consider adding a containment check verifying the path starts with WorkspaceRoot or the cache directory before calling os.ReadFile.

• [stale-doc] docs/guides/dev/cli-internals.md:395 — The flowchart describes ImportProfile() as importing profiles "(from URL-resolved openshell.profiles)". Profiles can now also come from local paths via base composition or ResolveRelativeTo. The parenthetical is incomplete but not incorrect.
  Remediation: Update to "(from resolved openshell.profiles)" or "(from URL-resolved or local openshell.profiles)".

• [function-comment-consistency] internal/harness/url.go:27 — IsProviderPath function comment uses "looks like a file path" while other predicate functions in the same file (IsURL, IsAbsPath, IsRelPath) use direct "returns true if s is" phrasing. The heuristic nature of IsProviderPath makes "looks like" semantically accurate, but the asymmetry is notable.

Previous run

Review

Findings

High

• [stale-doc] docs/guides/user/customizing-agents.md:62 — The user guide states "Openshell provider profiles (URL-only, with integrity hash)" for the openshell.profiles field, but this PR modifies ValidateResourceTypes to accept local paths for profiles (no longer URL-only). A user reading this guide would be misled into thinking local profile paths are not supported.
  Remediation: Update the field description to reflect that profiles can now be local paths (from base composition or ResolveRelativeTo) as well as URLs with integrity hashes.

Medium

• [consumer-completeness] internal/cli/lock.go:784 — resolveFromLock strips URL entries from h.Providers but does not strip absolute-path entries introduced by base composition. When resolveFromLock produces non-empty result.Profiles (from lock file profile deps), the new second ResolveHarness call's guard condition (len(result.Profiles) == 0) is false, so abs-path providers remain in h.Providers. These abs-path strings then flow into sandboxProviderNames as provider "names" instead of bare names like fullsend-github.
  Remediation: Also strip absolute-path entries from h.Providers in resolveFromLock, mirroring how ResolveHarness filters them via the remaining slice: if !harness.IsURL(p) && !filepath.IsAbs(p) { remainingProviders = append(...) }.

• [scope-authorization-mismatch] PR title uses fix(compose) but this is a feature addition, not a bug fix. The linked issue feat(compose): support local-path resolution for profiles and providers during base composition #5240 correctly uses feat(compose). Per AGENTS.md, "Getting the prefix right is not optional — GoReleaser uses PR titles to build release notes." Using fix hides this feature from the Features section of release notes.
  Remediation: Change the PR title to feat(compose): resolve local profiles and providers in base harness (#5240).

• [stale-doc] docs/ADRs/0070-portable-provider-profile-resolution.md:59 — ADR 0070 states profiles accept "only HTTPS URLs with #sha256=... integrity hashes" and "No local-path form." This PR adds local-path support for profiles and extends provider resolution to support relative paths from base composition. Since ADR 0070 has status Accepted, it should not be substantially rewritten (per AGENTS.md). The resolution flow documentation also does not cover base composition for profiles/providers.
  Remediation: Write a new superseding ADR documenting the decision to extend profile and provider resolution to support local paths from base composition, and add a cross-reference annotation to ADR 0070.

Low

• [edge-case] internal/harness/url.go:31 — IsProviderPath only checks for / as a directory separator, not \. On the Linux-only runtime this is fine, but creates an asymmetry with filepath.IsAbs (which handles both separators). No immediate action needed.

---

Labels: PR modifies harness composition and resolve logic (internal/harness/, internal/resolve/, internal/cli/)

[rh-hemartin]

Didn't read anything yet: make sure you allow for overriding by the same name. We detected that problem with skills, in which if you wanted to override an existing skill you got an error.

[maruiz93]

make sure you allow for overriding by the same name. We detected that problem with skills, in which if you wanted to override an existing skill you got an error.

Already handled — child entries override base entries by name/ID using last-wins semantics:

• dedupResolvedProfiles deduplicates profiles by ID
• dedupResolvedProviders deduplicates providers by Name
• mergeProviderDefs gives local defs precedence over URL-resolved ones

Tests covering the compose override scenario:

• TestDedupResolvedProfiles_ComposeScenario
• TestDedupResolvedProviders_ComposeScenario

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:45 PM UTC · Completed 3:26 PM UTC
Commit: 25258f1 · View workflow run →

[waynesun09]

Multi-agent review — MEDIUM+ findings

Parallel review (Claude + Gemini agents). Posting only MEDIUM-and-above findings that are not already covered by existing comments. The already-flagged local providers dropped in the lock path issue (qodo bot at run.go:481) is still unresolved — note that the mechanism it describes (gated by len(result.Profiles)==0) no longer matches the current code, but the underlying bug persists in the reworked gate; see the inline notes below for the current form.

Inline comments cover: duplicate base-composed profiles (High), the Accepted-ADR rewrite policy violation (High), the untested merge design (Medium), and the hasLocalProviders absolute-path assumption (Medium).

[MEDIUM] test-coverage — No internal/cli test for the lock ↔ second-pass merge seam

The added tests exercise ResolveHarness, IsProviderPath, parseProviderDef, and ValidateFilesExist in isolation, but the diff touches no internal/cli/run_test.go or internal/cli/lock_test.go. The correctness bugs in the inline comments all live specifically in the resolveFromLock → hasLocalProviders → merge composition, which has zero coverage. Please add internal/cli integration tests over the matrix {lock, no-lock} × {base-composed, child-local} × {profile, provider}, asserting exactly one resolved entry per resource and no dropped local providers — these would fail today and pin the fixes.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:52 PM UTC · Completed 4:12 PM UTC
Commit: 893ec29 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] logic-error

resolveFromLock strips ALL absolute-path profiles via !filepath.IsAbs(p) and all provider paths via !harness.IsProviderPath(p), but local-path profiles/providers resolved by ResolveRelativeTo are also absolute. When a harness combines URL references (triggering the lock-file path) with local-path profiles/providers, the local entries are stripped without appearing in the lock file. They cannot be recovered by the second ResolveHarness call because the guards find no remaining entries. A harness with a URL-referenced skill and a local profile path would silently lose the local profile when running with a lock file.

Suggested fix: Track which entries correspond to lock-file dependencies and only strip those. Alternatively, preserve remaining absolute-path entries that have no corresponding lock dep so the second ResolveHarness pass can process them.

[fullsend-ai-review]

[low] arbitrary-file-read

In ResolveHarness, when a provider entry is an absolute path, os.ReadFile(p) is called without a containment check against WorkspaceRoot. Upstream guards prevent untrusted paths from reaching here, but the defense-in-depth is absent at this layer.

Suggested fix: Consider adding a containment check verifying the path starts with WorkspaceRoot or the cache directory before calling os.ReadFile.

[fullsend-ai-review]

[low] function-comment-consistency

IsProviderPath comment uses 'looks like' while other predicate functions in url.go (IsURL, IsAbsPath, IsRelPath) use direct 'returns true if s is' phrasing.

[waynesun09]

Re-review at 893ec298 — the round of fixes doesn't close the blockers

Verified the pushed fixes against the current code (multi-agent, Claude + Gemini). One new top-level finding below; I've also replied in-thread to the ADR, profile-strip, and second-pass comments where the fix is incomplete or the premise is contested. Out of scope but worth noting: dedupResolvedProfiles/Providers (pre-existing, not in this diff) silently collapse two distinct entries sharing an id/name with no warning — unlike mergeProviderDefs, which surfaces a shadowed list. Consider a warning on content mismatch in a follow-up.

[waynesun09]

[HIGH] correctness — H1 is not actually fixed: local providers are still dropped in the lock path

The hasLocalProviders change (now !IsURL && IsProviderPath) can't take effect here, because this strip runs first and removes exactly the entries the gate looks for.

Trace: runAgent calls ResolveRelativeTo unconditionally at run.go:386, so a locally-declared providers/custom.yaml is already absolute by the time resolveFromLock runs. This loop then drops it (IsProviderPath is true for /abs/.../custom.yaml). Genuinely-local providers are never fetched, so ResolveHarness records no dependency for them — they're absent from the lock file and are not reconstructed. Back at run.go:486, hasLocalProviders(h) inspects the now-stripped h.Providers, returns false, and the compensating second pass never runs. Net: a harness with any URL reference (so a lock entry exists) plus a local provider drops that provider silently — no error, no warning. The same root cause hits the profile strip below (see my reply on that thread).

The strip is also asymmetric: providers strip on IsProviderPath (relative + absolute), profiles on filepath.IsAbs (absolute only). Post-ResolveRelativeTo both only ever hold absolute paths so they converge today, but the discriminator should be whether the entry was consumed by a lock dep, not its path shape.

Suggestion: In resolveFromLock, strip only entries backed by a reconstructed lock dep (URL-resolved); leave genuinely-local paths for the second pass to resolve — or evaluate the run.go:486 gate on a snapshot of h.Providers/h.OpenShell.Profiles taken before stripping. Please add a lock-path test: harness with a URL skill + providers/custom.yaml, locked, asserting the provider survives into result.Providers (no lock-path test currently covers a local path-form provider).