fix(#3396): propagate default allowlist to harness composition - #3425
Conversation
tryAgentsRepoFallback defaults OrgAllowlist to config.DefaultAllowedRemoteResources() for its own fetch, but didn't propagate that default back into composeOpts. Downstream, LoadWithBase → resolveBaseScripts → fetchBaseFile rejected pre_script/post_script URLs because OrgAllowlist was still nil. Add propagateDefaultAllowlist() to fill the default when no config.yaml supplied an allowlist, called in runAgent between resolveAgentSource and LoadWithBase. Closes #3396 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoFix agents-repo fallback by propagating default allowlist into ComposeOpts
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Site previewPreview: https://16449ba1-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1. propagateDefaultAllowlist ignores empty allowlist
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Verified the root-cause analysis against the code and the CI history — the diagnosis is correct and the fix is the right minimal change: tryAgentsRepoFallback defaulted a nil allowlist only for its own fetch, while LoadWithBase → resolveBaseScripts saw the still-nil OrgAllowlist and rejected the harness's own script URLs.
Three inline comments:
- Keep the
nil-only check (the bot's suggestion to also default empty slices would break the deny-all contract), but document the semantic change for configs that omit the key. - The integration test overwrites the propagated allowlist, so nothing asserts the real default list matches the fallback's URL shape — one extra assertion closes that.
- The green
functional-testscheck on this PR is a path-filter skip, not a pass —internal/cli/isn't in the relevance filter (the same gap that let #2947 breakmainsilently). Addinginternal/scaffold/fullsend-repo/config.yaml(fix 2 from #3396) to this PR would both harden CI's dogfood path and make the evals actually run pre-merge.
The e2e failure is unrelated infra flake — apt-get install podman in the test-org workflow hit a bad packages.microsoft.com repo signature before the fullsend binary was exercised.
| func propagateDefaultAllowlist(opts *harness.ComposeOpts) { | ||
| if opts.OrgAllowlist == nil { | ||
| opts.OrgAllowlist = config.DefaultAllowedRemoteResources() | ||
| } |
There was a problem hiding this comment.
[HIGH — review-squad consensus update] A 4-agent review squad (Claude, Gemini, Codex reviewers) independently converged on this spot, elevating my earlier note: the unconditional propagation is a behavior change to a deny-by-default security boundary, shipped untested.
Concretely: when config.yaml loads successfully but omits allowed_remote_resources (YAML-omitted → nil), the orgCfg == nil recheck below is skipped, so a URL-base: disk harness pointing at the fullsend-ai/* prefixes now fetches and executes its remote pre/post scripts where LoadWithBase previously hard-rejected with "URL base requires org-level allowed_remote_resources" (the ADR 0058 anti-self-authorization guard). The no-config.yaml-at-all case is still protected (requireOrgConfig hard-errors), and the injected default is compile-time first-party constants — so this is a policy-control loss, not a new execution primitive. But it deserves an explicit decision rather than riding along:
- Either scope the propagation to the path that needs it — e.g. only when the harness came from
tryAgentsRepoFallback/fetchDepssetSourceURL— keeping URL-base disk harnesses strict; - or ratify the broader semantics: document at the
AllowedRemoteResourcesfield that omitting the key means "first-party defaults allowed" and explicit[]is the supported deny-all, and add a regression test for the config-present-key-omitted + URL-base case (there is currently none, which is how this shipped silently).
Two adjacent consistency gaps the squad also flagged if the broad behavior is kept:
propagateDefaultAllowlistonly checks== nil— good — but no test pins the explicit-[]-stays-deny-all contract; a futurelen()==0refactor would silently flip deny-all to first-party-allow with every test green.- The post-load check
h.ValidateAllowedRemoteResources(orgCfg.AllowedRemoteResources)still uses the raw config value, so base scripts and URL skills see two different effective allowlists in one run (fails closed, but contradicts this helper's "consistent allowlist" doc comment).
Previous review comment (superseded)
The nil-only check is correct — please keep it that way despite the bot suggestion to also default len == 0. An explicitly empty allowed_remote_resources: [] means deny-all, and tryAgentsRepoFallback has the same contract (TestTryAgentsRepoFallback_ExplicitlyEmptyAllowlist). Defaulting the empty slice here would silently re-enable remote fetches for orgs that opted out.
One behavior change worth spelling out in this doc comment: this propagation is unconditional, not scoped to the fallback path. A config.yaml that exists but omits the allowed_remote_resources key previously meant "no remote resources anywhere in composition"; after this change it means "first-party defaults allowed". That's consistent with the policy #2947 already established (and it softens the #3172/#3173 outage class), but orgs that want deny-all must now write an explicit empty list — worth a sentence here so the next reader knows it's intentional.
There was a problem hiding this comment.
Expanded the doc comment in a844d3f. Kept the nil-only check and added a sentence about the semantic change for configs omitting the key.
There was a problem hiding this comment.
Went with option 1 in f251f09 — scoped the propagation inside the fetchDeps/SourceURL block so it only fires on the agents-repo fallback path. Config-present harnesses that omit allowed_remote_resources keep the deny-all behavior they had before.
Also added a test pinning the explicit-empty-slice contract ([]string{} stays empty, not replaced with defaults).
Point 2 (ValidateAllowedRemoteResources using the raw config value) — that's a pre-existing inconsistency unrelated to this fix. Worth a follow-up issue?
There was a problem hiding this comment.
Agreed — the ValidateAllowedRemoteResources inconsistency predates this PR and a follow-up issue is the right home for it; with the propagation now scoped it's back to a narrow, pre-existing wart rather than something this PR widens.
The scoping in f251f09 looks right to me, and it resolves the run/lock divergence I raised in the review body as well — with propagation limited to the fetch-deps path, both commands agree again on URL-base harnesses under a config that omits the key. The only residual there is pre-existing too (fullsend lock can't lock fallback-resolved agents at all since it only uses resolveHarnessPath), which could ride along in the same follow-up issue or its own.
With the explicit-empty test pinning the deny-all contract and the evals running for real on this PR now, this looks ready from my side once functional-tests completes.
| // Override allowlist to match test server (default points at github.com). | ||
| opts.OrgAllowlist = []string{srv.URL + "/"} | ||
|
|
There was a problem hiding this comment.
The propagateDefaultAllowlist(&opts) call here is immediately overwritten on the next line, so this test never verifies the property the bug was about: that the actual default list prefix-matches the script URLs the fallback constructs (https://raw.githubusercontent.com/fullsend-ai/agents/<sha>/scripts/...). The override is unavoidable for the httptest server, but consider adding one assertion before it:
propagateDefaultAllowlist(&opts)
assert.Equal(t, config.DefaultAllowedRemoteResources(), opts.OrgAllowlist)plus a cheap static check that the default list covers the fallback's URL shape, e.g. assert.NotEmpty(t, harness.MatchingAllowedPrefixInList(defaultAgentsRepoURLPrefix+strings.Repeat("a", 40)+"/scripts/pre-triage.sh", config.DefaultAllowedRemoteResources())). Otherwise a future edit to DefaultAllowedRemoteResources() could regress #3396 with every test here still green.
There was a problem hiding this comment.
Added both assertions in a844d3f — the assert.Equal before the override, and the static MatchingAllowedPrefixInList check against the real defaultAgentsRepoURLPrefix.
| // Propagate the default allowlist when no config.yaml provided one. | ||
| // tryAgentsRepoFallback defaults internally, but that local default | ||
| // wasn't reaching LoadWithBase → resolveBaseScripts. See #3396. | ||
| propagateDefaultAllowlist(&composeOpts) |
There was a problem hiding this comment.
Placement checks out: the orgCfg == nil strict-load block below can still overwrite OrgAllowlist with the real config's value for URL-base: harnesses, so the "URL base requires org-level allowed_remote_resources" self-authorization guard keeps working — the default only survives when there's genuinely no config-supplied allowlist.
Heads-up on the test plan though: the green functional-tests check on this PR is a path-filter skip, not a pass — the relevance filter only matches eval/, internal/scaffold/, .github/scripts/, and the workflow file, and this PR touches only internal/cli/. Same blind spot that let #2947 land without running the evals. The tidy way to close both gaps at once is to include fix (2) from #3396 in this PR: add internal/scaffold/fullsend-repo/config.yaml declaring allowed_remote_resources explicitly. That hardens CI's dogfooding path independently of this code fix and matches internal/scaffold/**, so the triage evals actually run against this branch before merge instead of us finding out on main again.
There was a problem hiding this comment.
I looked into the functional-tests path filter (.github/workflows/functional-tests.yml:13-17) — it triggers on eval/**, internal/scaffold/**, .github/workflows/functional-tests.yml, and .github/scripts/**. Adding a scaffold config.yaml would only make the filter match PRs that touch that file, not PRs that touch internal/cli/. The real gap is that internal/cli/ isn't in the path filter at all — same blind spot for any future CLI change that affects agent resolution.
Do you think we should widen the filter to include internal/cli/**? That would be a separate commit, and it would make functional tests run on every CLI PR (which might be broader than intended). Or file a follow-up issue for the filter gap?
There was a problem hiding this comment.
Good call scoping it — and glad the workflow-file touch made the evals actually run on this PR (4/4 OK confirms the fix end-to-end).
On widening: I'd suggest keep internal/cli/run.go and add internal/harness/**, rather than all of internal/cli/**. Numbers from main since May 1: internal/cli/** would trigger the evals on ~18% of commits (472/2633), mostly flags/printers/unrelated subcommands — which largely defeats the point of having a filter. run.go + internal/harness/** lands at ~8% and covers both halves of this exact bug class: #3396 needed run.go (resolution didn't propagate the allowlist) and internal/harness/compose.go (composition enforced it) conspiring. A regression introduced purely in LoadWithBase/resolveBaseScripts — where the guard actually lives — wouldn't touch run.go at all, and internal/harness/ changes rarely enough (~2.5% of commits) that the added eval cost is small.
The more durable fix, though, is making this class not depend on LLM evals at all: a hermetic smoke test in the always-run test job that drives resolveAgentSource → LoadWithBase against an httptest server with no config.yaml present. Your new TestAgentsRepoFallback_LoadWithBase_NilAllowlist is already ~90% of that — it just calls the two stages separately instead of driving the real runAgent wiring, which is exactly where the propagation was missing. If that's a follow-up issue, the filter choice here becomes second-order; happy either way.
Expand propagateDefaultAllowlist doc comment to clarify the nil-vs-empty semantic: nil means "no config supplied" and gets the default; an explicit empty slice preserves deny-all. Add two assertions to the integration test: - Verify propagateDefaultAllowlist actually sets the default list before the test overrides it for the httptest server - Static check that DefaultAllowedRemoteResources() prefix-matches the real agents-repo fallback URL shape, so a future edit to the default list can't silently regress #3396 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · |
|
🤖 Review · |
The functional-tests path filter and relevance check only covered eval/, internal/scaffold/, .github/scripts/, and the workflow file itself. Changes to internal/cli/ (like the harness resolution fix in this PR) skipped functional tests entirely — same blind spot that let #2947 land without running the evals. Add internal/cli/ to both the push.paths filter and the PR relevance grep so that CLI changes affecting agent resolution get evaluated before merge. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
f4940af to
bb07ba8
Compare
|
🤖 Finished Review · ✅ Success · Started 9:57 PM UTC · Completed 10:09 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up from a 4-agent review squad (Claude, Gemini, Codex reviewers) — one MEDIUM finding that can't be anchored inline because the file isn't in this diff:
[MEDIUM] fullsend lock doesn't get the propagation → run/lock divergence
internal/cli/lock.go:208 — lockOneAgent builds harness.ComposeOpts with the same nil-derived orgAllowlist pattern this PR fixes in runAgent, but without a propagateDefaultAllowlist call. Today lock never sets SourceURL (disk-only via resolveHarnessPath), so it can't reproduce #3396 itself — but for a URL-base: harness under a config.yaml that omits allowed_remote_resources, the two commands now disagree: run succeeds with the propagated first-party default while lock still hard-fails with "URL base requires org-level allowed_remote_resources". That breaks the lock-then-offline-run workflow for exactly the configs this PR aims to unblock. Suggest applying one policy at a single shared point (e.g. where orgAllowlist is derived from orgCfg, or at ComposeOpts construction) so run and lock can't drift — or at minimum a comment at lock.go's ComposeOpts noting the "no SourceURL here" invariant so a future extension revisits this.
The squad's remaining findings were already covered by the existing review comments (the propagateDefaultAllowlist scope/semantics comment has been updated in place with the consensus details, including the dual-allowlist inconsistency with h.ValidateAllowedRemoteResources and the missing explicit-empty-list test).
ReviewFindingsMedium
Low
Previous runReviewVerdict: Approve SummaryClean, well-scoped bug fix for #3396. The PR correctly addresses the allowlist-default asymmetry introduced by PR #2947: Security assessment: No security concerns. The fix does not create a fail-open condition — it aligns the behavior so the same defaults used for the initial agent fetch are also available during harness composition. The nil-vs-empty distinction is correctly handled: nil (no config) gets the default allowlist; explicitly empty Scope: The three commits are well-structured — the core fix, review feedback improvements, and the CI trigger update are properly separated with correct conventional commit types ( FindingsTwo low-severity findings, neither blocking: 1. Missing test case for empty-slice edge case (
Suggested addition: t.Run("explicitly empty allowlist preserved", func(t *testing.T) {
empty := []string{}
opts := harness.ComposeOpts{OrgAllowlist: empty}
propagateDefaultAllowlist(&opts)
assert.Equal(t, empty, opts.OrgAllowlist)
})2. Ordering comment (
Dimensions reviewed
Labels: PR fixes a harness composition bug and updates CI trigger paths |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
Addresses review feedback: propagateDefaultAllowlist was called unconditionally, which changed behavior for config-present harnesses that omit allowed_remote_resources. A config.yaml that exists but omits the key previously meant deny-all in LoadWithBase; the unconditional propagation would have silently allowed first-party URLs in that case. Scope the call to inside the fetchDeps/SourceURL block so it only fires when tryAgentsRepoFallback actually set SourceURL. This preserves the deny-all contract for config-present harnesses with URL bases. Also add a test pinning the explicit-empty-slice-stays-deny-all contract, so a future len==0 refactor can't silently flip it. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · ❌ Terminated · Started 10:13 PM UTC · Ended 10:27 PM UTC |
|
Re: the [MEDIUM] lock/run divergence — looked at it. Since |
| paths: | ||
| - 'eval/**' | ||
| - 'internal/scaffold/**' | ||
| - 'internal/cli/run.go' |
There was a problem hiding this comment.
[low] scope-coherence
The workflow trigger adds all of internal/cli/run.go (a 2700+ line file) to the push paths filter, but the bug fix only affects the tryAgentsRepoFallback / propagateDefaultAllowlist code path. Any future change to run.go will now trigger functional tests, which is overly broad.
Suggested fix: Consider adding a comment explaining the breadth trade-off, or evaluate whether functional test runtime makes the extra triggers acceptable.
fullsend-ai-review
Bot
removed
the
requires-manual-review
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 10:13 PM UTC · Completed 10:27 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
All review findings are addressed:
- Root-cause fix verified:
propagateDefaultAllowlistcloses the exact #3396 asymmetry, now scoped to the fetch-deps/fallback path (f251f09) so config-present harnesses that omitallowed_remote_resourceskeep deny-all — which also resolves therun/lockdivergence concern. nil-vs-explicit-[]semantics documented and pinned by tests, including the new deny-all contract test.- Integration test now asserts the propagated default and statically verifies
DefaultAllowedRemoteResources()covers the agents-repo fallback URL shape. internal/cli/run.goadded to the functional-tests relevance filter, and — decisively — the triage evals ran for real on this PR and pass 4/4 on the final head, proving the fix end-to-end against the exact failure from #3396.
Remaining items are pre-existing and agreed as follow-ups: the ValidateAllowedRemoteResources raw-config inconsistency, and fullsend lock not supporting fallback-resolved agents. Optionally consider widening the eval filter to internal/harness/** per the thread discussion, but that's not blocking.
|
🤖 Finished Retro · ✅ Success · Started 10:39 PM UTC · Completed 10:47 PM UTC |
|
PR #3425 fixed a regression from PR #2947 where Proposals filed
|
2 participants
Summary
tryAgentsRepoFallbackdefaultedOrgAllowlistinternally for its own fetch but didn't propagate it intocomposeOpts, causingLoadWithBase→resolveBaseScripts→fetchBaseFileto reject pre_script/post_script URLs with "not in allowed_remote_resources"propagateDefaultAllowlist()inrunAgentbetweenresolveAgentSourceandLoadWithBaseto fill the default when noconfig.yamlsupplied an allowlistconfig.yaml(or one missingallowed_remote_resources) that relies on the agents-repo fallback — including fullsend's own CIRelated Issue
Closes #3396
Changes
internal/cli/run.go: AddpropagateDefaultAllowlist()and call it inrunAgentafter settingSourceURLinternal/cli/run_test.go: Unit test forpropagateDefaultAllowlist(nil → default, non-nil → preserved) and integration test confirmingLoadWithBasesucceeds with propagated allowlist but fails withoutTesting
TestPropagateDefaultAllowlist,TestAgentsRepoFallback_LoadWithBase_NilAllowlistgo vetcleanmake lintcleanfunctional-testsjob should go green — the 4 triage eval cases that have been failing since PR feat(cli): add runtime fallback to agents repo for unconfigured agents #2947 mergedChecklist
🤖 Generated with Claude Code