Conversation
Module Scorecard Check
|
| Theme | Before | After |
|---|---|---|
| Presentation & Onboarding | 6 / 17 | 6 / 17 |
| Integration | 15 / 25 | 7.5 / 25 |
| Credential Hygiene | 20 / 20 | 12 / 20 |
| Restricted-Environment | 17 / 20 | 12 / 20 |
| Engineering Quality | 8 / 10 | 10 / 10 |
| Overall | 72 / 100 | 52 / 100 |
Full scorecard for this PR
| Presentation & Onboarding | Agent Integration | Credential Hygiene | Restricted-Environment Readiness | Engineering Quality | Overall |
|---|---|---|---|---|---|
| 6 / 17 | 7.5 / 25 | 12 / 20 | 12 / 20 | 10 / 10 | 52 / 100 |
Drilldown
Presentation & Onboarding — 6 / 17
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Configuration-mode examples | 12 | 6 | Only one full code example is shown (install_cli = true path). The install_cli = false bake-in mode and env-var passthrough (SELF_HOSTED_RUNNER_HOOKS_DIR) are described in prose/comments but never given their own worked example block, so major modes aren't each demonstrated. |
| Visual preview | 5 | 0 | No image, GIF, or video anywhere in the README. |
Credential Hygiene — 12 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Secrets marked sensitive | 16 | 8 | agent_relay_credential uses ephemeral = true and styling.mask_input = true, and README examples never inline a raw key — but no coder_parameter/coder_env in main.tf uses the literal sensitive = true attribute, so the rubric's explicit marking is not present. |
| Non-hardcoded auth path | 4 | 4 | README/main.tf explicitly document that Agent Relay stamps agent_relay_credential on dispatch and the field is disabled/masked in the UI, so a human never pastes a raw key. |
Restricted-Environment Readiness — 12 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Mirrorable artifact source | 5 | 0 | install.sh.tftpl hardcodes curl https://claude.ai/install.sh -fsSL | bash. cli_binary only overrides the binary name/path used post-install, not the download URL — no variable overrides the install source. |
| Bring-your-own binary | 10 | 10 | install_cli = false plus cli_binary is documented in the README's Requirements section ("bake it into the image for the fastest start"), and the install script explicitly no-ops when the binary is already present. |
| Egress transparency | 3 | 0 | No dedicated network/egress/air-gapped section in the README. The claude.ai/install.sh endpoint only appears in script comments, not enumerated in README prose. |
| Runs without sudo | 2 | 2 | install.sh.tftpl, start.sh.tftpl, stop.sh.tftpl, and status.sh.tftpl contain no sudo invocations anywhere; all operations (curl install, mkdir, setsid, kill -TERM) run as the workspace user. |
Engineering Quality — 10 / 10
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Input quality | 6 | 6 | Every variable has a detailed description and sensible default; cli_binary, exit_if_unused_min, and drain_wait_sec carry explicit validation blocks guarding shell-injection and fractional inputs. |
| Test coverage | 4 | 4 | main.tftest.hcl covers parameter contract, script rendering, injection safety, and shutdown budget arithmetic in depth; main.test.ts exercises end-to-end lifecycle (idle/working/serving/done/failed, drain, SIGTERM handling) inside a real container. |
Agent Integration — 7.5 / 25
| Criterion | Max | Score | Notes |
|---|---|---|---|
| AI governance | 10 | 0 | No mention anywhere of Coder AI Gateway or Agent Firewall; the module's "governance" is Agent Relay's own dispatch contract, which is a different, unrelated product surface. |
| Dashboard entry point | 5 | 0 | No coder_app resource or example; the module is a headless runner with only agent metadata (agent_relay_status), no dashboard-launchable app. |
| Session continuity | 5 | 2.5 | README documents push_outcome_on_release pushing a session's outcome branch so "a resumed session continues from them" — a form of continuity, but it's git-branch state preservation rather than a native resume/session-ID mechanism or a persistent session manager (tmux/screen/boo), so only partial credit. |
| Managed configuration | 5 | 5 | base_dir documents a configurable workdir, and the README shows a worked example of injecting SELF_HOSTED_RUNNER_HOOKS_DIR via coder_env to manage hooks configuration passed through to the CLI. |
Overall — 52 / 100
Raw 47.5 / 92 → round(47.5 / 92 × 100) = 52
Tip
You can run this locally by telling your agent: "review this module against .github/scorecard/SCORECARD.md".
Scored against SCORECARD.md with solstice-1. Language-model scores are advisory.
30fd43f to
e257dfc
Compare
The start step detaches the supervisor with setsid, so it lives in its own session and never receives the SIGTERM the container's init gets on shutdown. The runner was therefore killed outright: its session was never released server-side, the post-session hook never ran, and in-flight commits were lost. Add a run_on_stop script that relays the signal to the runner and waits for the supervisor to record its exit. coder-utils has no stop hook, so it is a standalone coder_script beside it. That half only works if the platform grants the time to use it, which a module cannot arrange because it owns no compute resource. Export shutdown_grace_seconds for the template to wire into docker_container.destroy_grace_seconds or a pod's termination_grace_period_seconds. Measured, not guessed: the runner prints its budget at startup, 80s bare, 110s with --push-outcome-on-release and 170s with --drain-wait-sec 60, and the local adds the 20s in-flight release and the 5s the agent spends on SSH first. Also expose drain_wait_sec and push_outcome_on_release, each omitting its flag at the neutral value so the paired env var keeps working, and client_label, defaulting to <owner>/<workspace> so a runner is identifiable in the Anthropic console without the template setting a hostname. Fix two doc errors found on the way: the lifecycle section listed a runner command line missing --base-dir and --exec-path, and exit_if_unused_min's description cited dispatch_deadline, which is the Cursor pools' setting. Claude Code pools use the work order's issue time plus 300s.
e257dfc to
64d87b2
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 918629f15b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # defeat the point, and the platform SIGKILLs us soon enough anyway. | ||
| kill -TERM "$pid" 2>/dev/null || true | ||
|
|
||
| SECONDS=0 |
There was a problem hiding this comment.
When SIGTERM makes the runner exit, /proc/$pid can disappear before the separate supervisor resumes from wait and writes done <code>. Because this loop only waits on runner_alive, the stop resource can return during that gap, allowing the platform to terminate the container while the state still says working; the metadata script then classifies the clean shutdown as orphaned, which can turn it into a nack. Keep waiting, within the existing budget, until the state file leaves working rather than treating runner disappearance as completion.
Useful? React with 👍 / 👎.
| variable "push_outcome_on_release" { | ||
| type = bool | ||
| default = true | ||
| description = "Push the session's outcome branch to origin before deleting it when the runner ends a session it could not complete (the CLI's --push-outcome-on-release), so commits survive an ephemeral workspace being torn down and a resumed session continues from them. Fires on every runner-initiated incomplete end, which includes idle-release and failed sessions, so it needs git auth in the workspace and it creates branches for those too. Adds 30s to shutdown_grace_seconds. false leaves the flag off, so a template can set SELF_HOSTED_RUNNER_PUSH_OUTCOME_ON_RELEASE itself." |
There was a problem hiding this comment.
Existing consumers upgrading from 0.1.1 will not set this new variable, so this default begins pushing branches for every incomplete, idle-released, or failed session even though the previous module emitted no such flag. As the new documentation notes, this requires git authentication and leaves branches behind, making the upgrade behavior-breaking for existing templates; default this to false and let users opt in.
AGENTS.md reference: AGENTS.md:L137-L139
Useful? React with 👍 / 👎.
| | `drain_wait_sec` | `push_outcome_on_release` | `shutdown_grace_seconds` | | ||
| | ---------------- | ------------------------- | ------------------------ | | ||
| | `0` (default) | `true` (default) | 135 | | ||
| | `0` | `false` | 105 | | ||
| | `60` | `true` | 195 | |
There was a problem hiding this comment.
This table directly enumerates the drain_wait_sec and push_outcome_on_release inputs alongside the shutdown_grace_seconds output, duplicating documentation that the registry generates from Terraform. Convert the combinations to prose or a usage example instead of maintaining an input/output table.
AGENTS.md reference: AGENTS.md:L110-L111
Useful? React with 👍 / 👎.
| variable "drain_wait_sec" { | ||
| type = number | ||
| default = 0 | ||
| description = "Seconds the runner waits for the session's in-flight turn and background tasks to finish before stopping the session process, once it is asked to shut down (the CLI's --drain-wait-sec). 0 leaves the flag off, so a template can set SELF_HOSTED_RUNNER_DRAIN_WAIT_MS itself; note that one is milliseconds. Every second here is added to shutdown_grace_seconds, which the template must honor." |
There was a problem hiding this comment.
The three new variable blocks—drain_wait_sec, push_outcome_on_release, and client_label—place type and default before description. Reorder each block to description → type → default → validation to follow the repository's required variable convention.
AGENTS.md reference: AGENTS.md:L113-L123
Useful? React with 👍 / 👎.
| # session release already in flight. The two optional behaviors extend | ||
| # it by exactly what their flags document. This is what the stop script | ||
| # waits for, because it starts counting when it runs. | ||
| runner_budget = 100 + var.drain_wait_sec + (var.push_outcome_on_release ? 30 : 0) |
There was a problem hiding this comment.
When drain_wait_sec remains 0 but a template uses the documented SELF_HOSTED_RUNNER_DRAIN_WAIT_MS escape hatch, the runner may drain for longer than this budget, while the stop script still waits only 100 baseline seconds and the advertised platform grace remains 105/135 seconds. Likewise, setting push_outcome_on_release = false and enabling its environment variable removes the required 30 seconds from the budget. In those supported configurations the platform can kill the runner before its configured shutdown work completes, so expose additional grace or otherwise include environment-driven behavior in the budget.
Useful? React with 👍 / 👎.
Wait for the supervisor to record the exit, not just for the runner to go. The supervisor is a separate process, so /proc/<pid> can disappear before it resumes from `wait` and writes "done <code>". Returning in that gap left the state file saying "working" with a dead pid, which the status script reports as orphaned -- a clean drain graded as a failure. Match the runner's argv in the pid guard rather than the bare basename. A basename alone matches any command line merely containing it, which is exactly the recycled-pid case the guard exists for. Order the new variables description -> type -> default -> validation, and replace the budget table in the README with prose, both per AGENTS.md. Document that the budget only counts what the module passes: a template using SELF_HOSTED_RUNNER_DRAIN_WAIT_MS or the push-outcome env var instead of the inputs makes the runner spend time the number does not know about.
1 participant
Problem
The start step detaches the supervisor with
setsid, so it lives in its own session and never receives the SIGTERM the container's init gets on shutdown. The runner is therefore killed outright: its session is never released server-side, the post-session hook never runs, and in-flight commits are lost.The runner asks for this explicitly at startup:
Nothing in the module or the templates using it grants that today.
Changes
stop.sh.tftpl+ arun_on_stopcoder_script. It reads the runner PID from the state file'sworking <pid>line, sends SIGTERM only, never escalates, and waits for the supervisor to recorddone <code>. It deliberately does not signal the supervisor: killing that skips itswait, so terminal state is never written and the status reportsorphaned— turning a clean drain into a nack.coder-utils0.0.1 has no stop hook, so this is a standalone resource beside it.shutdown_grace_secondsoutput. A module owns no compute resource, so the template wires this intodocker_container.destroy_grace_secondsor a pod'stermination_grace_period_seconds.drain_wait_secandpush_outcome_on_releaseinputs. Each omits its flag at the neutral value, so the pairedSELF_HOSTED_RUNNER_*env var keeps working — a flag always beats its env var.client_labelinput, defaulting to<owner>/<workspace>so a runner is identifiable in the Anthropic console without the template having to set a hostname. Base64-encoded across the template boundary.--base-dirand--exec-path, andexit_if_unused_min's description citeddispatch_deadline, which is the Cursor pools' setting. Claude Code pools use the work order's issue time plus 300s (serviceclaudecoderunners/dispatch.go:107).The budget is measured, not read off the help text
The runner prints its computed budget at startup. Against CLI 2.1.280 on
linux/amd64:--push-outcome-on-release--push-outcome-on-release --drain-wait-sec 60shutdown_grace_secondsis that plus 20s for a session release already in flight and the 5s the agent spends shutting down SSH before it runs stop scripts at all — so 135 / 105 / 195 for the three input combinations.Why the grace period is not optional
A
docker_containerdestroy callsContainerStopwith a zero timeout unlessdestroy_grace_secondsis set. Verified with a probe container that traps SIGTERM and writes to a bind mount:destroy_grace_secondsUPonly — never even records the signal20UP,TERM,DRAINEDNote the second row: destroy returns as soon as the container exits, so the value is a ceiling rather than a fixed wait and costs nothing when the runner has already finished.
Testing
terraform test: 14 pass.bun test main.test.ts: 18 pass.New coverage includes the SIGTERM relay end to end against a stub CLI that traps and exits 7, asserting the supervisor records
done 7and that the script actually waited; four no-op paths (never dispatched, alreadydone, no state file, dead PID); and argv assertions for the new flags.Sequencing
Minor bump,
0.1.1→0.2.0. Two follow-ups depend on the release tag: the reference template incoder/agent-relayand the dogfood template both pin 0.2.0 to wire the grace period.Not covered here:
agent-relay-cursorshares the same dogfood template and therefore the same grace period, but has no stop script yet.Generated with Claude Code using Claude Opus 4.5.