build agent: language-agnostic rules against orphaned background processes #3

New issue

Open

opened 2026-11-12 21:25:12 +00:00 by hermes_forgeron · 0 comments

hermes_forgeron commented

2026-11-12 21:25:12 +00:00

Collaborator

Part of #1 (finding 2). Three consecutive build runs were lost this way (test_factory_ai issue #3, runs 895 / 898 / 901) even though each implementation was correct.

Evidence

Every run followed the same pattern: the agent implemented the spec, its runtime smoke test PASSED (server answered HTTP/1.1 200 OK with {"test":true}), and then:

<shell_metadata> shell tool terminated command after exceeding timeout 30000 ms …
[agent  67.3s] ⚠️ Agent run failed: opencode exited with status null

Root cause (from the job logs):

the agent starts the server with npm start & — $! is the npm PID, not the node child;
kill $SERVER_PID kills npm; the node child survives, keeps listening and holds the shell's stdout/stderr pipes open;
opencode's shell tool waits on those pipes, hits its timeout (30 s, 60 s on the retry), and opencode is killed (status null) → run failed, work lost.

This is NOT node-specific: any wrapper toolchain (mvn, gradle, cargo run, sbt, lein, uv run, …) spawns the real server as a grandchild with exactly the same result. The factory serves Java / Node.js / Rust / other projects, so the guard must be language-agnostic.

Proposed change

glue/src/agents.ts — extend the BUILD_AGENT operating rules. That markdown block is where the agent's standing instructions live (buildBuildPrompt in prompt.ts merely says "Follow your operating rules"). Add, as hard rules:

Prefer static or in-process validation whenever the spec allows it: compiler/syntax checks (node --check, mvn -DskipTests compile, cargo check, python -m py_compile, go vet, …), linters, and in-process test harnesses (supertest, Spring Boot tests/MockMvc, axum::test, go httptest, pytest + httpx TestClient, …) — they cannot orphan anything.
If a real server must be exercised, do it in ONE command shaped like this:
- start the actual executable directly (never through a wrapper that spawns children); resolve it in an earlier command if needed (npm exec, the mvn-generated classpath, the cargo build binary under target/, …);
- redirect its stdout/stderr to files (>/tmp/srv.log 2>&1) — never leave them attached to the terminal pipes;
- capture its exact PID; poll readiness with a bounded retry loop (curl/wget/nc);
- exercise the endpoint(s);
- kill -9 the exact PID, then confirm the port is released;
- wrap the whole thing in an explicit timeout guard so nothing can outlive the command.
Never end a command with a listening process still running.

Keep the rules short and imperative — the agent demonstrably adopts explicit patterns (attempt 3 adopted the polling loop and git check-ignore from a hint) but reverted on the one detail that mattered when the rule was left implicit.

Deployment note

Same image-tag repin as the other glue issues (build a new tag, update the agent-image default in .forgejo/workflows/agent.yml); the image build runs npm run check itself.

Acceptance criteria

BUILD_AGENT in glue/src/agents.ts contains the language-agnostic rule set (static/in-process first; direct executable + output redirection + exact-PID kill -9 + timeout guard when a server is truly needed)
e2e on test_factory_ai: in issue #3's thread, replace the "static checks only" validation section of the spec block with one that REQUIRES a runtime smoke test, dispatch mode=build with the new image → the run succeeds, the job log contains no shell tool terminated command after exceeding timeout and no exited with status null, and the PR is opened/updated
The runtime smoke test really ran: the job log shows a live HTTP response from the started server (not a skipped check)
No regression: an issue whose spec allows static-only validation still builds green

Part of #1 (finding 2). Three consecutive build runs were lost this way (test_factory_ai issue #3, runs 895 / 898 / 901) even though each implementation was correct. ## Evidence Every run followed the same pattern: the agent implemented the spec, its runtime smoke test PASSED (server answered `HTTP/1.1 200 OK` with `{"test":true}`), and then: ``` <shell_metadata> shell tool terminated command after exceeding timeout 30000 ms … [agent 67.3s] ⚠️ Agent run failed: opencode exited with status null ``` Root cause (from the job logs): - the agent starts the server with `npm start &` — `$!` is the **npm** PID, not the **node** child; - `kill $SERVER_PID` kills npm; the node child survives, keeps listening **and holds the shell's stdout/stderr pipes open**; - opencode's shell tool waits on those pipes, hits its timeout (30 s, 60 s on the retry), and opencode is killed (`status null`) → run failed, work lost. This is NOT node-specific: any wrapper toolchain (`mvn`, `gradle`, `cargo run`, `sbt`, `lein`, `uv run`, …) spawns the real server as a grandchild with exactly the same result. The factory serves Java / Node.js / Rust / other projects, so the guard must be language-agnostic. ## Proposed change `glue/src/agents.ts` — extend the `BUILD_AGENT` operating rules. That markdown block is where the agent's standing instructions live (`buildBuildPrompt` in `prompt.ts` merely says "Follow your operating rules"). Add, as hard rules: 1. **Prefer static or in-process validation** whenever the spec allows it: compiler/syntax checks (`node --check`, `mvn -DskipTests compile`, `cargo check`, `python -m py_compile`, `go vet`, …), linters, and in-process test harnesses (supertest, Spring Boot tests/MockMvc, axum::test, go httptest, pytest + httpx TestClient, …) — they cannot orphan anything. 2. If a real server must be exercised, do it in ONE command shaped like this: - start the **actual executable directly** (never through a wrapper that spawns children); resolve it in an earlier command if needed (`npm exec`, the mvn-generated classpath, the `cargo build` binary under `target/`, …); - **redirect its stdout/stderr to files** (`>/tmp/srv.log 2>&1`) — never leave them attached to the terminal pipes; - capture its exact PID; poll readiness with a bounded retry loop (`curl`/`wget`/`nc`); - exercise the endpoint(s); - **`kill -9` the exact PID**, then confirm the port is released; - wrap the whole thing in an explicit `timeout` guard so nothing can outlive the command. 3. Never end a command with a listening process still running. Keep the rules short and imperative — the agent demonstrably adopts explicit patterns (attempt 3 adopted the polling loop and `git check-ignore` from a hint) but reverted on the one detail that mattered when the rule was left implicit. ## Deployment note Same image-tag repin as the other glue issues (build a new tag, update the `agent-image` default in `.forgejo/workflows/agent.yml`); the image build runs `npm run check` itself. ## Acceptance criteria - [ ] `BUILD_AGENT` in `glue/src/agents.ts` contains the language-agnostic rule set (static/in-process first; direct executable + output redirection + exact-PID `kill -9` + timeout guard when a server is truly needed) - [ ] e2e on test_factory_ai: in issue #3's thread, replace the "static checks only" validation section of the spec block with one that REQUIRES a runtime smoke test, dispatch `mode=build` with the new image → the run succeeds, the job log contains **no** `shell tool terminated command after exceeding timeout` and no `exited with status null`, and the PR is opened/updated - [ ] The runtime smoke test really ran: the job log shows a live HTTP response from the started server (not a skipped check) - [ ] No regression: an issue whose spec allows static-only validation still builds green

hermes_forgeron referenced this issue

2026-11-12 21:25:13 +00:00

Dogfood findings: reliability improvements for the agent workflow #1