Phase 4b: add Go local runner - #46
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Go as a supported runtime to the local test runner, extends sandboxing to accept writable cache paths, forwards Go-specific env into spawned processes, updates config/docs to list python3 and go as runnable (java remains unimplemented), and broadens unit, integration, and e2e tests to cover Go paths. ChangesGo Runtime Implementation and Integration
✨ Finishing Touches✨ Simplify code
Comment |
SPerekrestova
left a comment
SPerekrestova
left a comment
There was a problem hiding this comment.
Reviewed the diff. Two concrete findings — one reliability issue worth addressing, one stale-comment fix.
1. Reliability — Go inherits a fresh empty HOME on every run, so GOCACHE is cold every time
src/runner/subprocess-runner.ts (lines 224, 266–270) sets cwd to a freshly-mkdtemp'd dir and forwards only PATH, HOME, LANG, with HOME pointed at that same temp dir (which is removed in the finally). go run resolves GOCACHE from $HOME/.cache/go-build when unset, so every invocation:
- creates an empty build cache under the temp dir,
- cold-compiles every stdlib package the snippet touches (e.g.
fmt), - and throws the cache away at the end of the run.
A trivial fmt.Println snippet that takes ~50 ms warm can easily take 1–3 s cold, and on slow CI hosts that pushes uncomfortably close to the default DEFAULT_TIMEOUT_MS = 5_000. It also makes every Go run noticeably slower than Python, which is contrary to the "turns around in seconds" promise in the tool description.
Two fixes either of which would address it:
- Forward a stable, process-scoped
GOCACHE(and ideallyGOMODCACHE) inspawnAndCapture's env so the build cache survives across runs — e.g., a directory undertmpdir()created once per process and reused. - Or, only for
go, overrideHOMEto a stable per-process dir (still distinct from the user's real$HOME) so Go's defaults land in a persistent location.
A unit/e2e assertion that two consecutive Go runs of the same trivial program complete well under the timeout would catch regressions here.
2. Stale comment — go registry entry is now described as an unimplemented stub
src/runner/subprocess-runner.ts lines 76–78:
// Phase 4c stub — present in the registry so the type system
// requires they stay in sync with `RunnerLanguage`. The runner
// refuses to use it until we actually wire the harness.
go: {
extension: "go",
probe: { cmd: "go", args: ["version"] },
buildArgs: (sourcePath) => ({
cmd: "go",
args: ["run", sourcePath]
})
},
java: {The "Phase 4c stub … refuses to use it" comment now sits directly above the fully-implemented go entry, so a reader will conclude go is still a stub. Move the comment block so it lives immediately above java: (the only remaining stub).
Otherwise LGTM — the registry-driven extension, the runtime-gated skipIf test pattern, and the symmetry with the existing Python path all look clean.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runner/subprocess-runner.ts`:
- Around line 76-78: The "Phase 4c stub" comment is misplaced before the Go
entry; move that comment so it precedes the java: entry instead, keeping its
text about staying in sync with RunnerLanguage and the runner refusing to use it
until the harness is wired; locate the comment string "Phase 4c stub" and the
java: enum/registry entry and cut/paste the comment immediately above the java:
label (leaving the Go entry and its surrounding comments unchanged) so the
comment accurately documents the stub behavior for Java.
In `@tests/runner/subprocess-runner.test.ts`:
- Around line 115-134: The Go happy-path test omits timeoutMs causing flaky
timeouts; update the runner.run call in the test (the invocation of runner.run
in tests/runner/subprocess-runner.test.ts inside the "executes a happy-path Go
program" spec) to include timeoutMs: 20_000 in its options object so the
SubprocessRunner has a 20s timeout for this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b889ff1-9c49-4679-8209-c654fa1f1227
📒 Files selected for processing (6)
src/mcp/tools/runner-tools.tssrc/runner/runner.tssrc/runner/subprocess-runner.tstests/e2e/runner.test.tstests/integration/runner-tools-integration.test.tstests/runner/subprocess-runner.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runner/subprocess-runner.ts`:
- Around line 81-95: The cached promise goRuntimePathsPromise can stay rejected
if mkdtemp or mkdir throws, causing all future calls to getGoRuntimePaths() to
fail; wrap the async initializer inside getGoRuntimePaths() in a try/catch, and
if the inner async IIFE rejects, reset goRuntimePathsPromise to undefined before
rethrowing the error so transient failures don't permanently pollute the cache
(update the logic inside getGoRuntimePaths and touch goRuntimePathsPromise and
the anonymous initializer accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78677c9d-6118-4351-955d-ab464e8f3adc
📒 Files selected for processing (6)
src/mcp/tools/runner-tools.tssrc/runner/sandbox.tssrc/runner/subprocess-runner.tssrc/types/runner.tstests/runner/sandbox.test.tstests/runner/subprocess-runner.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/mcp/tools/runner-tools.ts
- src/types/runner.ts
1 participant
Summary
Adds
gotoIMPLEMENTED_LANGUAGESand executes Go submissions via the same isolated subprocess path as Python:Go runs keep the per-invocation temp
HOMEisolation, but now receive stable process-scopedGOCACHE/GOMODCACHEdirectories that are also whitelisted in OS sandbox wrappers. This avoids cold-compiling the Go stdlib on everyrun_local_testsinvocation without exposing the user's real home or shell env.Keeps
javaas the only still-unimplemented supported runner language, updatesrun_local_testsmessaging, and adds runtime-gated unit/e2e coverage for Go success, absent-runtime behavior, and stable Go cache reuse. Verified locally withnpm run build,npm run test:types,npx prettier --check .,npm test,npm run test:e2e, andnpm run test:all; this VM does not have Go installed, so real Go execution tests are present but skipped locally unless a Go runtime is available.Link to Devin session: https://app.devin.ai/sessions/3e6f3e63f6e54852b9bf6da1c792774f
Requested by: @SPerekrestova