fix(#6471): validate config-driven runtimes against ValidRuntimes() - #6472
Conversation
|
🤖 Finished Review · ✅ Success · Started 12:42 AM UTC · Completed 12:54 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Looks good to me |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
resolveBackendFromConfigData() passes parsed config straight into ResolveFromConfig/ResolveFromPerRepoConfig → Resolve() without checking ValidRuntimes(). This allows stub runtimes like "opencode" (registered in Resolve() for dev/testing but absent from ValidRuntimes()) to pass through config-driven resolution, burning sandbox/bootstrap side effects before failing in Bootstrap(). Add a validateConfigRuntime() guard in both ResolveFromConfig() and ResolveFromPerRepoConfig() that checks the runtime name against config.ValidRuntimes() before calling Resolve(). Direct Resolve() calls remain unaffected, preserving dev/testing access to stubs. Update registry_test.go: the existing test asserting opencode resolves via config is replaced with tests verifying stub runtimes are rejected through config paths while remaining accessible via direct Resolve(). Closes #6471
38a5330 to
f047ad7
Compare
|
Rebased onto The conflict, and how I resolved itThe conflict was in // opencode is not in ValidRuntimes() but is resolvable via Resolve().
// A hand-written config bypassing validation can reach the stub.
ocCfg.SetRuntime("opencode")
ocBackend, err := ResolveFromPerRepoConfig(ocCfg)
require.NoError(t, err)
assert.Equal(t, "opencode", ocBackend.Runtime.Name())
// pi is user-selectable (#6464).
piCfg.SetRuntime("pi")
...The first half asserts exactly the behaviour this PR removes — it pinned the bug #6471 describes — so it had to go; your ReviewThe change is correct and minimal: One correction to the framing, not the code. The PR body and #6471 both say an invalid runtime currently burns "sandbox/bootstrap side effects" before failing. Half of that stays true after this fix: runtime resolution happens at So this correctly prevents the wasted Ready for approval and the merge queue; say the word and I will take it the rest of the way. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving. Rebased onto 9a184a9e by me; the conflict was in TestResolveFromPerRepoConfig, where main had gained an assertion that opencode resolves through per-repo config — the exact behaviour this PR removes — next to an unrelated pi assertion. I dropped the first (superseded by the new …_RejectsStubRuntimes tests) and kept the second, so pi coverage survives the rebase.
The change itself is correct and minimal: validateConfigRuntime gates both config entry points against config.ValidRuntimes() while direct Resolve() keeps stub runtimes available for dev and tests. gofmt/go vet clean; internal/config and internal/cli pass in full, internal/runtime passes apart from two DummyRuntime tests that fail on this machine regardless of branch (a real openshell is installed here, so their "call against a nonexistent sandbox must fail" assertion does not hold).
No blocking findings. The one correction — recorded in the thread above rather than as a change here — is that this stops the wasted Bootstrap, not the wasted sandbox: runtime resolution runs at run.go:1202 while CreateWithRetry is at 1045, so a typo'd runtime: still creates and deletes a sandbox before failing. Follow-up material, not a reason to hold this.
|
🤖 Finished Review · ✅ Success · Started 12:46 AM UTC · Completed 12:57 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 12:56 AM UTC · Completed 1:08 AM UTC Commit: |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
Retro: PR #6472 — validate config-driven runtimes against ValidRuntimes()Workflow outcome: Successful. The code agent produced correct, minimal code in one shot (zero rework), tests achieved 100% function coverage on changed functions, and the PR merged cleanly after human review. Timeline: Issue #6471 opened → triage (~6 min) → code agent (~11 min) → review agent approved (~14 min, $2.99) → human rebased and reviewed (~24h later) → second review agent ran post-merge (~13 min, $2.56) → merged. Key human reviewer contribution: Wayne resolved a meaningful merge conflict (main had gained a test asserting the exact behavior this PR removes, alongside an unrelated Evidence for existing open issues
What went well
Proposals filed
|
1 participant
Summary
Add a
ValidRuntimes()guard to config-driven runtime resolution so that stub runtimes (e.g.opencode) registered inResolve()for dev/testing are rejected before sandbox bootstrap when selected through org or per-repo config. Previously,resolveBackendFromConfigData()skipped.Validate()and passed parsed config straight toResolve(), which accepted stub runtimes and let the run proceed through sandbox creation before failing inBootstrap().Changes
validateConfigRuntime()helper ininternal/runtime/registry.gothat checks the runtime name againstconfig.ValidRuntimes()before resolutionvalidateConfigRuntime()in bothResolveFromConfig()andResolveFromPerRepoConfig()Resolve()calls remain unaffected — stub runtimes are still accessible for dev/testingregistry_test.go: replace test asserting opencode resolves via config with tests verifying stub runtimes are rejected through config paths while remaining accessible via directResolve()Testing
TestResolveFromPerRepoConfig_RejectsStubRuntimesverifies stub runtimes are rejected via per-repo config and still resolvable via directResolve()TestResolveFromConfig_RejectsStubRuntimesverifies stub runtimes are rejected via org configgo test -race ./internal/runtime/...)registry.gogo vetpassesNote:
golangci-lintwas not available in the sandbox environment.pre-commitcould not initialize due to network restrictions. Manual verification of these is required.Closes #6471
Post-script verification
agent/6471-validate-config-runtimes)6fa6cb907b700abaace905bf2cfd8ffb5693dc71..HEAD)