[waynesun09]

Summary

Two OpenShell 0.1.x regressions from #7233 that CI could not catch before merge.

1. behaviour fails on main (required check). Since #7233, every scenario that runs a test-defined custom harness fails (15–16 of 33 on main run 36621388462 and on rebased PRs). The fleet triage/review harnesses in the same suite pass. OpenShell 0.1.2 refuses both policy shapes the custom harnesses reach:

• No policy (most scenarios): the sandbox ends in phase Error, with or without providers attached:

  Configuration: ConfigurationInvalid: Effective configuration could not be activated; replace the policy or repair attached providers
  Policy source: sandbox
  

• A policy file with no fields: the steps committed # Minimal policy for <name> for every relative policy: a harness names, and 0.1.2 fails at create with failed to decode sandbox policy fields.

Reproduced on a local 0.1.2 gateway with a runtime: dummy harness and no providers, as the scenarios run: no policy → ConfigurationInvalid; the comment-only placeholder → decode failure; the base policy below → sandbox created, agent exit 0. The fleet harnesses pass because they resolve a real policy from the agents repo. (codex-openai/pi-openai name a policy but are capability-gated and were skipped, so they were never evidence either way. They would have hit the decode failure once enabled.)

fullsend ships no default policy (#7266), so a harness must name one, and the feature files now show it: every standalone and base harness heredoc declares policy: policies/base.yaml, and base-composed children inherit it from their base. The resource helpers commit the base policy fullsend agent new writes (agentnew.BasePolicy, exported for this) for any relative policy a harness names, in the config repo or the hosting repo. It has no network section, so egress still comes only from the harness's providers.

2. Parallel runs fail with provider profile '<id>' not found. Parallel runs on one gateway reimport shared profiles; the provider-create retry for that only matched OpenShell 0.0.x's "unsupported provider type or profile". Three of four parallel functional-test cases failed on it (run 36623063153). isTransientProviderErr now also matches 0.1's wording, across the CLI's line wrap.

User custom harnesses without a policy break the same way on 0.1.x; a clear early error for that is #7849.

Independent of the WIF HTTP 429 behaviour flake (#7843, #7844): this PR's previous behaviour run had no 429s and still failed 15 scenarios.

Testing

• go test ./pkg/behaviourtest/... ./internal/agentnew/ ./internal/sandbox/, and go vet -tags behaviour ./e2e/behaviour/. Tests: every committed policy resource is the base policy (version: 1, non-empty filesystem_policy), including one committed next to a URL-sourced base for its children to inherit; the 0.1 error text (plain and line-wrapped) is retried and a missing provider is not.
• Mutation checks: restoring the comment-only placeholder at either commit site (dispatch.go, url_dispatch.go) fails the step tests; without the new matcher the retry test fails.
• Local 0.1.2 run of a dummy-runtime custom harness with the committed base policy: sandbox created, exit 0.
• behaviour 33/33 on c05749e (the same base policy, then injected by the steps). This head moves the policy into the feature files and needs its own run.

The previous e2e failure (triage pre-flight Get "https://api.github.com/rate_limit": EOF from a Ready sandbox) is a different shape: it has a real policy and providers. It passed on #7233's re-run, so it is treated as a flake for now and re-checked on this run.

[qodo-code-review]

PR Summary by Qodo

Restore behaviour tests and provider retries on OpenShell 0.1

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Give policy-less behaviour harnesses a dedicated default policy so their sandboxes can activate on
 OpenShell 0.1.
• Retry OpenShell 0.1 profile-not-found errors caused by concurrent profile reimports.
• Test local and hosted policy commits, explicit policies, YAML edge cases, and wrapped CLI errors.

Diagram

graph TD
  Steps["Harness steps"] --> Check{"Policy present?"} -->|No| Default["Default policy"] --> Resources["Resource helpers"] --> Repos["Test repositories"] --> Sandbox["OpenShell sandbox"] --> Matcher["Provider error matcher"] --> Retry["Bounded retry"]
  Check -->|Yes| Resources

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Default policies in production

• ➕ Would also address user-created harnesses without policies.
• ➖ Changes product and security semantics beyond the behaviour-test regression.

2. Serialize shared profile imports

• ➕ Could prevent the profile deletion-and-reimport race altogether.
• ➖ Requires coordination across parallel runs sharing a gateway.

Recommendation: Keep the test-only default and bounded provider retry for these regressions. Consider production policy semantics and cross-run profile coordination separately.

Files changed (9) +164 / -8

Bug fix (5) +48 / -3

sandbox.goRecognize OpenShell 0.1 missing-profile errors as transient +10/-3

Recognize OpenShell 0.1 missing-profile errors as transient

• Extends provider error classification to match profile-not-found messages, including CLI line wraps, so the existing bounded retry handles concurrent profile reimports.

internal/sandbox/sandbox.go

base_dispatch.goDefault policies across base-harness steps +4/-0

Default policies across base-harness steps

• Applies the policy fallback to local and URL-sourced base-harness creation paths before committing their documents.

pkg/behaviourtest/steps/base_dispatch.go

dispatch.goDefault policies for local custom harnesses +2/-0

Default policies for local custom harnesses

• Adds the policy fallback to enabled and disabled custom-harness steps before they commit harness YAML.

pkg/behaviourtest/steps/dispatch.go

harness_policy.goAdd a dedicated behaviour-harness policy fallback +31/-0

Add a dedicated behaviour-harness policy fallback

• Adds a default policy path only when parsed harness YAML has no effective top-level policy. Preserves explicit policies and leaves unparseable documents unchanged.

pkg/behaviourtest/steps/harness_policy.go

url_dispatch.goDefault policies for hosted custom harnesses +1/-0

Default policies for hosted custom harnesses

• Applies the fallback before committing URL-sourced custom harnesses, allowing the existing resource helper to commit their policy to the hosting repository.

pkg/behaviourtest/steps/url_dispatch.go

Tests (4) +116 / -5

sandbox_test.goCover plain and wrapped missing-profile errors +8/-0

Cover plain and wrapped missing-profile errors

• Tests both OpenShell 0.1 message layouts and confirms an unrelated missing-provider error is not treated as transient.

internal/sandbox/sandbox_test.go

base_dispatch_test.goVerify hosted base-harness policy commitment +6/-2

Verify hosted base-harness policy commitment

• Updates the expected hosted harness content and hash, and checks that its default policy file is committed.

pkg/behaviourtest/steps/base_dispatch_test.go

harness_policy_test.goTest policy fallback and local resource commits +93/-0

Test policy fallback and local resource commits

• Covers absent and explicit policies, trailing YAML block scalars, and invalid YAML. Verifies local harness steps commit the policy file when defaulted.

pkg/behaviourtest/steps/harness_policy_test.go

url_dispatch_test.goAdjust hosted-harness hashes and verify policy file +9/-3

Adjust hosted-harness hashes and verify policy file

• Updates expected hashes for defaulted harness content and checks that a hosted custom harness has its policy file committed alongside it.

pkg/behaviourtest/steps/url_dispatch_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:28 PM UTC · Completed 8:43 PM UTC

Commit: 027350b · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.08

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Moderate risk score of 2 preserved from prior assessment: elevated regression history and change coupling in internal/sandbox, offset by zero protected paths, no dependency changes, solid test coverage, and alignment with tracking issue #7751; the additional commit since the prior review is a small, unrelated, test-only fix that does not change the tier signals.

Previous run

Risk Assessment: moderate (2/5)

Details

Moderate risk driven by regression history and change coupling in internal/sandbox, offset by narrow scope, zero protected path changes, strong test coverage, and clear alignment with tracking issue #7751. Unchanged from the prior assessment on this PR (Tier 1 signals stable across the one additional commit since the last review).

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Moderate risk driven by regression history and change coupling in internal/sandbox, offset by narrow scope across 11 files, zero protected path changes, 45% test ratio, easily revertible changes, and continuity with the closed OpenShell 0.1 migration tracking issue; consistent with the prior assessment on this PR.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Moderate risk driven by regression history and change coupling in internal/sandbox, offset by narrow scope, zero protected path changes, strong test coverage, and clear alignment with the tracking issue.

[fullsend-ai-review]

Review

Findings

Low

• [scope-creep] e2e/admin/admin_test.go:237 — The most recent commit (ci(e2e): retry enrollment merge after concurrent head update) adds retry handling for a GitHub API 422 race in the admin E2E test's enrollment-PR merge loop. This is unrelated to linked issue Move to OpenShell 0.1.1 #7751 (OpenShell 0.1 migration regressions) and to the rest of this PR's stated purpose (sandbox policy handling and transient-provider-error retry matching). It is test-only and covered by the accompanying admin_test_helpers_test.go unit test, so it is not blocking — but bundling an unrelated fix into an issue-scoped PR loses traceability for anyone auditing Move to OpenShell 0.1.1 #7751's changes later.
  Remediation: Consider splitting unrelated CI/e2e fixes into their own PR, even when low-risk and test-only.

Previous run

Looks good to me

Previous run (2)

Looks good to me

Previous run (3)

Review

Findings

Low

• [logic-error] pkg/behaviourtest/steps/harness_policy.go:28 — withDefaultPolicy still injects policy: policies/behaviour-default.yaml whenever the parsed document has no non-empty policy key. The child-composition steps (givenCustomHarnessWithLocalBase at base_dispatch.go:44, givenCustomHarnessWithURLBase at :191, givenURLSourcedCustomHarnessWithURLBase at :279) call it on the child YAML before prepending base:. internal/harness/compose.go:698-699 confirms LoadWithBase copies base.Policy only when child.Policy == "", so a policy-less child of a base that names a real policy keeps the injected placeholder instead of inheriting. givenURLSourcedCustomHarnessWithURLBase has a second trap: after prepending base: it delegates to givenURLSourcedCustomHarness, which calls withDefaultPolicy again (url_dispatch.go:228); skipping only the pre-prepend calls would still inject unless withDefaultPolicy also ignores docs that already have base. This is unchanged from the prior review (not addressed by this PR's commits) — current e2e scenarios (e2e/behaviour/features/dispatch/base-dispatch.feature) still don't name an explicit policy on base or child, so both layers independently get the same placeholder and OpenShell 0.1 stays unblocked, but the helper still cannot exercise "child inherits base's real policy" (e.g. policies/base.yaml).
  Remediation: Don't default the policy on a harness that has (or will have) a base: field — let composition inherit it instead. Skip injection in withDefaultPolicy when the parsed doc already has a base key, and stop calling withDefaultPolicy in givenCustomHarnessWithLocalBase, givenCustomHarnessWithURLBase, and givenURLSourcedCustomHarnessWithURLBase on the pre-composition child doc. Keep defaulting on the standalone/base steps (givenCustomHarness, givenDisabledCustomHarness, givenURLSourcedBaseHarness, givenURLSourcedCustomHarness). Add a test asserting a policy-less child with a base that names policies/base.yaml does not commit or set policies/behaviour-default.yaml.

Previous run (4)

Review

Findings

Low

• [logic-error] pkg/behaviourtest/steps/harness_policy.go:27 — withDefaultPolicy injects policy: policies/behaviour-default.yaml whenever the parsed document has no non-empty policy key. The child-composition steps (givenCustomHarnessWithLocalBase, givenCustomHarnessWithURLBase, givenURLSourcedCustomHarnessWithURLBase) call it on the child YAML before prepending base:. LoadWithBase only copies base.Policy when child.Policy is empty (internal/harness/compose.go:698-699, :2378-2379), so a policy-less child of a base that names a real policy would keep the injected placeholder instead of inheriting the base's policy. The existing e2e/behaviour/features/dispatch/base-dispatch.feature scenarios don't currently expose this — neither base nor child names an explicit policy there today, so both layers independently get the same placeholder — but it's a latent test-helper correctness gap: base-composed behaviour-test scenarios can never actually exercise "child inherits base's real policy" (e.g. policies/base.yaml, used by the fleet triage harnesses per the PR description) once a scenario needs one. No test covers a policy-less child correctly inheriting a base's non-default policy.
  Remediation: Don't default the policy on a harness that has (or will have) a base: field — let composition inherit it instead. Concretely: skip injection in withDefaultPolicy when the parsed doc already has a base key, and/or move the withDefaultPolicy calls in givenCustomHarnessWithLocalBase, givenCustomHarnessWithURLBase, and givenURLSourcedCustomHarnessWithURLBase so they don't run on the pre-composition child doc. Keep defaulting on the standalone/base steps (givenCustomHarness, givenDisabledCustomHarness, givenURLSourcedBaseHarness, givenURLSourcedCustomHarness). Add a test asserting a policy-less child with a base that names policies/base.yaml does not commit or set policies/behaviour-default.yaml.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Flow-style harnesses fail to parse 🐞 Bug ≡ Correctness

Description

withDefaultPolicy appends a block-style policy: key after the original YAML instead of adding it
within a flow-style root mapping. A valid policy-less document such as `{agent: agents/triage.md,
role: triage}` therefore becomes invalid when a custom-harness step passes the modified document to
its resource parser.

Code

pkg/behaviourtest/steps/harness_policy.go[30]

+	return strings.TrimRight(doc, "\n") + "\npolicy: " + defaultHarnessPolicy

Relevance

●●● Strong

Valid flow-style YAML becomes unparsable after block-key concatenation; this is a deterministic
correctness bug.

PR-#6367

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper accepts a flow-style mapping through yaml.Unmarshal but concatenates a separate
block-mapping entry after its closing brace. The custom-harness step passes those changed bytes to
commitLocalHarnessResources, which unmarshals them again and returns a parsing error.

pkg/behaviourtest/steps/harness_policy.go[22-30]
pkg/behaviourtest/steps/dispatch.go[164-181]
pkg/behaviourtest/steps/dispatch.go[224-242]

Agent prompt

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

## Issue description
Appending a block key after a flow-style YAML mapping makes a previously valid harness document invalid.
## Fix Focus Areas
- pkg/behaviourtest/steps/harness_policy.go[22-30]
- pkg/behaviourtest/steps/dispatch.go[224-242]
- pkg/behaviourtest/steps/harness_policy_test.go[15-50]
## Recommended Fix
Add the default policy to the parsed YAML mapping and serialize a valid document, preserving existing explicit policies. Add a test using a flow-style harness document.

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

Dismiss ↗ | View ↗

---

2. Child harnesses lose inherited policies 🐞 Bug ≡ Correctness

Description

withDefaultPolicy assigns a policy to a child harness that omits one, so mergeBaseIntoChild no
longer copies the base harness’s policy into that child. This occurs in the base-composed harness
steps whenever a base defines its own policy and its child leaves policy unspecified.

Code

pkg/behaviourtest/steps/base_dispatch.go[44]

+	doc = withDefaultPolicy(doc)

Relevance

●●● Strong

Clear inheritance regression in base-composed harnesses; closely related base-dispatch correctness
feedback was accepted.

PR-#6367

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new call adds a nonempty policy before the child is committed. Composition copies base.Policy
only when child.Policy is empty, so an explicit base policy is bypassed.

pkg/behaviourtest/steps/base_dispatch.go[41-50]
pkg/behaviourtest/steps/harness_policy.go[22-30]
internal/harness/compose.go[698-700]

Agent prompt

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

## Issue description
Defaulting a base-composed child’s policy prevents it from inheriting the base policy.
## Fix Focus Areas
- pkg/behaviourtest/steps/base_dispatch.go[41-50]
- pkg/behaviourtest/steps/base_dispatch.go[188-198]
- pkg/behaviourtest/steps/harness_policy.go[19-30]
- internal/harness/compose.go[698-700]
## Recommended Fix
Do not inject a default into a child that can inherit its base’s policy. Resolve whether the composed harness lacks a policy before supplying the default, and test a child that inherits an explicit base policy.

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

Dismiss ↗ | View ↗

---

Context sources

✅ Compliance rules (platform): 72 rules

✅ Skills: writing-user-docs, writing-adrs, check-patch-coverage

✅ Cross-repo context — repo relationships

  Explored: repo: fullsend-ai/agents (sha: a05e2b35) — View relationship

Review mode: ⚖️ Balanced: This behavioral change spans sandbox retry logic and multiple harness/resource paths, with meaningful runtime and test-integration risk, but is not dense enough to require redundant extended review.

Tip of the day

💡 Did you know, you can route each severity your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:00 PM UTC · Completed 9:18 PM UTC

Commit: c05749e · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.31

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:33 PM UTC · Completed 11:46 PM UTC

Commit: c583177 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.63

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:48 PM UTC · Completed 12:05 AM UTC

Commit: 24d5e1a · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.98

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:45 AM UTC · Completed 6:58 AM UTC

Commit: 33bae11 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.10

Superseded by updated review