fix(#6452): propagate org-level allowed_remote_resources to URL resolution - #6454
fix(#6452): propagate org-level allowed_remote_resources to URL resolution#6454fullsend-ai-coder[bot] wants to merge 5 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 3:30 PM UTC · Completed 3:47 PM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
|
…ution The org-level allowed_remote_resources from config.yaml was only used during base: composition (via ComposeOpts.OrgAllowlist) but not during URL resolution of policy:, agent:, skills:, plugins:, profiles:, and providers: fields. This caused confusing behavior where base: URLs resolved successfully but policy: URLs with the same prefix failed. Add OrgAllowlist field to ResolveOpts and check it as a fallback in resolveFileURL and resolveSkillDirURL when the harness-level AllowedRemoteResources check fails. Pass the org allowlist from both run.go and lock.go callers. Also update resolveFromLock to accept and check the org allowlist for lock-file validation. Closes #6452
d6db2a5 to
bc8e4ba
Compare
|
/fs-fix |
|
🤖 Review · Commit: |
|
🤖 Finished Fix · ✅ Success · Started 9:14 PM UTC · Completed 9:23 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.95 |
- Update layered-config-reference.md to describe allowed_remote_resources as covering all remote resource types, not just agent sources and base composition - Add TestResolveFromLock_OrgAllowlistFallback to exercise the org-level allowlist in the lock-file resolution path - Update bring-your-own-agent.md note to mention org-level config.yaml fallback for URL resolution - Reorder OrgAllowlist field in ResolveOpts to match ComposeOpts ordering - Fix 4 additional resolveFromLock call sites missing the orgAllowlist parameter Note: pre-commit hooks were not run. pre-commit could not complete (infrastructure failure), and hooks were run directly instead (gofmt, go vet). Addresses #6454
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 6 review findings: fixed 4 (doc updates for layered-config-reference and bring-your-own-agent, added resolveFromLock org-allowlist test, reordered ResolveOpts fields), disagreed with 2 (both ADR findings were acknowledged by the reviewer as not requiring changes). Fixed (4):
Disagreed (2):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:26 PM UTC · Completed 9:43 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.28 |
|
Risk Assessment: moderate (2/5) DetailsBot-authored targeted bug fix with no protected paths, CI, security, or dependency changes; high fix/revert churn across core files sustains moderate caution — score unchanged at 2, consistent with prior assessment. Previous runRisk Assessment: moderate (2/5) DetailsTargeted bot-authored bug fix with no protected paths, CI, security, or dependency changes; test ratio is mid-band; high-churn files with 17 distinct recent authors sustain moderate caution, consistent with prior assessment — score unchanged at 2. Previous run (2)Risk Assessment: moderate (2/5) DetailsTargeted bug fix propagating an org-level allowlist to all URL resolution paths; change is well-scoped and test-accompanied, but internal/resolve/resolve.go and internal/cli/lock.go remain high-churn files with six-to-seven distinct recent authors, warranting moderate caution. Signals are unchanged from prior assessment — score preserved at 2. Previous run (3)Risk Assessment: moderate (2/5) DetailsTargeted bug fix propagating an org-level allowlist to all URL resolution paths; change is well-scoped and test-accompanied, but internal/resolve/resolve.go is a high-churn file with six distinct recent authors, warranting moderate caution. |
|
🤖 Finished Fix · ✅ Success · Started 9:45 PM UTC · Completed 9:55 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.46 |
Update four docs to reflect that the org-level allowed_remote_resources from config.yaml now acts as a fallback for all URL resolution types, not just base composition: - customizing-agents.md: add org-level fallback caveat to inheritance note - bring-your-own-agent.md: separate base-inheritance note from org-level fallback into distinct blockquotes for clarity - harness-reference.md: add fallback qualifier to merge-rules table - ADR 0038: add annotation noting the union resolution model Addresses #6454
🔧 Fix agent — iteration 2 (bot-triggered)Updated four documentation files to reflect that the org-level allowed_remote_resources now acts as a fallback for all URL resolution. Fixed stale notes in customizing-agents.md and harness-reference.md, added an ADR annotation to 0038, and improved clarity in bring-your-own-agent.md by separating base-inheritance from org-level fallback concepts. Two informational findings (authorization scope confirmation and default trust expansion) were acknowledged as intentional design. Fixed (4):
Disagreed (2):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:56 PM UTC · Completed 10:13 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.98 |
|
🤖 Finished Fix · ✅ Success · Started 10:14 PM UTC · Completed 10:21 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.15 |
Add a minor annotation to the Security section of ADR 0070 noting that profile/provider URLs now also pass validation against the org-level allowed_remote_resources from config.yaml, consistent with the changes in this PR. Note: pre-commit hooks were not run. pre-commit could not complete (infrastructure failure), and the direct-execution fallback ran the relevant local hooks (lint-adr-status, lint-adr-numbers, lint-docs-links, lychee, trailing-whitespace, end-of-file-fixer) manually. Addresses #6454
🔧 Fix agent — iteration 3 (bot-triggered)Added org-level allowlist fallback annotation to ADR 0070 Security section. Two informational findings (authorization scope confirmation and default trust expansion) were acknowledged as intentional design with no code change needed. Fixed (1):
Disagreed (2):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:23 PM UTC · Completed 10:40 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.85 |
|
🤖 Finished Fix · ✅ Success · Started 10:41 PM UTC · Completed 10:50 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.92 |
….go comment Move inline Note (PR #6454) blockquotes in ADR 0038 and ADR 0070 to dated entries under Amendments sections, following the established YYYY-MM-DD Title (#issue) convention used by existing amendments. Clarify the ambiguous allowlist-skip comment in lock.go to specify that both harness-level and org-level checks are skipped for agent_source entries. Addresses #6454
🔧 Fix agent — iteration 4 (bot-triggered)Moved ADR inline annotations to Amendments sections (strategy change from prior iterations), clarified ambiguous lock.go comment. Two informational findings acknowledged as intentional design with no code change needed. Fixed (3):
Disagreed (2):
Tests: passed
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:52 PM UTC · Completed 11:09 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.56 |
Superseded by updated review
Summary
Propagate the org-level
allowed_remote_resourcesfromconfig.yamlto all URL resolution paths, not justbase:composition. Previously,policy:,agent:,skills:,plugins:,profiles:, andproviders:URLs were only checked against the harness-levelAllowedRemoteResources, causing confusing failures when users expected org-level trust to apply uniformly.Related Issue
Fixes #6452
Changes
OrgAllowlist []stringfield toresolve.ResolveOptsresolveFileURLandresolveSkillDirURLto fall back to the org allowlist when the harness-level check failsorgAllowlistinrun.goandlock.gocallers when callingresolve.ResolveHarnessresolveFromLockto accept and check the org allowlist during lock-file validationTesting
TestResolveHarness_*tests pass (no regressions)TestResolveFromLock_*tests pass (no regressions)go vetpassesgo build ./...passesCloses #6452
Post-script verification
agent/6452-org-allowlist-policy-resolve)18792930cd964497a98a4ba99d4543be13ece8a3..HEAD)