Conversation
Module Scorecard Check
|
| Theme | Before | After |
|---|---|---|
| Presentation & Onboarding | 6 / 17 | 6 / 17 |
| Integration | 15 / 25 | 7.5 / 25 |
| Credential Hygiene | 20 / 20 | 20 / 20 |
| Restricted-Environment | 17 / 20 | 12 / 20 |
| Engineering Quality | 8 / 10 | 10 / 10 |
| Overall | 72 / 100 | 60 / 100 |
Full scorecard for this PR
| Presentation & Onboarding | Agent Integration | Credential Hygiene | Restricted-Environment Readiness | Engineering Quality | Overall |
|---|---|---|---|---|---|
| 6 / 17 | 7.5 / 25 | 20 / 20 | 12 / 20 | 10 / 10 | 60 / 100 |
Drilldown
Presentation & Onboarding — 6 / 17
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Configuration-mode examples | 12 | 6 | One primary tf example (install_cli toggle) plus a small coder_env override snippet and a docker_container.destroy_grace_seconds wiring example. Options like base_dir, client_label, drain_wait_sec, push_outcome_on_release are described in prose only, with no code example per mode. Narrow-purpose module so "modes" are limited, but coverage is incomplete/prose-only for most knobs. |
| Visual preview | 5 | 0 | No image, GIF, or video anywhere in the README. |
Credential Hygiene — 20 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Secrets marked sensitive | 16 | 16 | agent_relay_credential parameter is ephemeral = true with styling.mask_input = true (the coder_parameter data source has no literal sensitive field), and README states "the credential is masked." No inline secrets appear anywhere in README examples. |
| Non-hardcoded auth path | 4 | 4 | Credential is stamped entirely by Agent Relay via an ephemeral, disabled, masked parameter ("a human never fills it in"); README documents this explicitly as the only path, with no manual key entry. |
Restricted-Environment Readiness — 12 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Mirrorable artifact source | 5 | 0 | install.sh.tftpl hardcodes curl https://claude.ai/install.sh. No variable overrides this download URL (cli_binary only overrides the installed binary's path, not the source). |
| Bring-your-own binary | 10 | 10 | install_cli = false fully disables the download, and cli_binary points at a baked-in binary; documented clearly in the intro example and Requirements section ("bake the CLI into the image and set this to false"). |
| Egress transparency | 3 | 0 | No dedicated README network/egress/air-gapped section. claude.ai/install.sh is only mentioned inside script comments (not README), which per rubric doesn't count. |
| Runs without sudo | 2 | 2 | install.sh.tftpl, start.sh.tftpl, stop.sh.tftpl, status.sh.tftpl never invoke sudo; verified from script bodies. |
Engineering Quality — 10 / 10
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Input quality | 6 | 6 | All variables have clear descriptions and sensible defaults; validation blocks guard cli_binary (shell-safe regex), exit_if_unused_min, and drain_wait_sec (whole-number bounds). |
| Test coverage | 4 | 4 | main.tftest.hcl extensively covers parameter contract, script rendering, shutdown budget math, and injection safety; main.test.ts runs real container end-to-end lifecycle tests (idle/working/serving/done/orphaned states, SIGTERM drain, stub CLI). |
Agent Integration — 7.5 / 25
| Criterion | Max | Score | Notes |
|---|---|---|---|
| AI governance | 10 | 0 | No mention of Coder AI Gateway or Agent Firewall anywhere in README; this module concerns Anthropic's Agent Relay, a distinct system. |
| Dashboard entry point | 5 | 0 | No coder_app resource or documented dashboard entry; the only surfaced UI element is the agent_relay_status agent metadata script (not a coder_app). |
| Session continuity | 5 | 2.5 | push_outcome_on_release documents that "a resumed session continues from" the pushed outcome branch after a workspace is torn down — a git-based continuation mechanism, not native resume/session-id or a persistent session manager (tmux/screen). Documented but partial/unconventional. |
| Managed configuration | 5 | 5 | base_dir variable gives explicit, documented control over the session working directory ("Directory the runner checks sessions out under... Created at start"). |
Overall — 60 / 100
Raw 55.5 / 92 → round(55.5 / 92 × 100) = 60
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.
Adding the input with a true default changed behaviour for every template upgrading from 0.1.1: they would start pushing outcome branches for incomplete, idle-released and failed sessions without asking, and that path needs git auth in the workspace. AGENTS.md asks new variables to keep backward compatibility, so it is off unless a template opts in. The default shutdown budget drops to 105s accordingly; turning the push on puts its 30s back.
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.