Conversation
Module Scorecard Check
|
| Theme | Before | After |
|---|---|---|
| Presentation & Onboarding | 6 / 17 | 6 / 17 |
| Integration | 10 / 25 | 5 / 25 |
| Credential Hygiene | 12 / 20 | 20 / 20 |
| Restricted-Environment | 13.5 / 20 | 13.5 / 20 |
| Engineering Quality | 10 / 10 | 10 / 10 |
| Overall | 56 / 100 | 59 / 100 |
Full scorecard for this PR
| Presentation & Onboarding | Agent Integration | Credential Hygiene | Restricted-Environment Readiness | Engineering Quality | Overall |
|---|---|---|---|---|---|
| 6 / 17 | 5 / 25 | 20 / 20 | 13.5 / 20 | 10 / 10 | 59 / 100 |
Drilldown
Presentation & Onboarding — 6 / 17
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Configuration-mode examples | 12 | half | One well-annotated primary example (module "cursor_worker" with agent_id, install_cli) plus a second docker_container.destroy_grace_seconds snippet. There are no distinct "modes" (e.g. computer_use = true, the insecure_shared_token/api_key credential path, Kubernetes grace-period wiring) shown as worked code examples — only prose. |
| Visual preview | 5 | 0 | No image, GIF, or video anywhere in the README. |
Agent Integration — 5 / 25
| Criterion | Max | Score | Notes |
|---|---|---|---|
| AI governance | 10 | 0 | README documents "Agent Relay" dispatch/credential exchange but never mentions Coder AI Gateway or Agent Firewall, which are the specific governance surfaces the criterion requires. |
| Dashboard entry point | 5 | full | coder_app.cursor_web and coder_app.cursor_desktop are declared and documented in the README ("Open in Cursor Web" / "Open in Cursor Desktop"), gated correctly on agent_relay_session_id. |
| Session continuity | 5 | 0 | README explicitly states the opposite of continuity: "It is not refreshed: a worker that has to reconnect after expiry fails and Cursor re-queues the request for a fresh workspace." No resume/session-manager support documented. |
| Managed configuration | 5 | 0 | No documented MCP, settings, or policy management. agent_relay_cursor_repo_url is a contract parameter cloned by the template, not configuration the module manages. |
Credential Hygiene — 20 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Secrets marked sensitive | 16 | full | agent_relay_credential is ephemeral = true and rendered with mask_input = true styling; the "Worker credential" README section documents exactly how the token flows (env var only, never written to disk, expires in 1h, visible only via /proc/<pid>/cmdline). No inline secrets appear in any README example. |
| Non-hardcoded auth path | 4 | full | README documents a token-exchange flow: the service-account key never leaves Agent Relay, which mints a short-lived per-user sub-token exported as AGENT_RELAY_CURSOR_TOKEN — a documented non-hardcoded auth path. |
Restricted-Environment Readiness — 13.5 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Mirrorable artifact source | 5 | 0 | install.sh.tftpl hardcodes `curl https://cursor.com/install -fsSL |
| Bring-your-own binary | 10 | full | install_cli = false fully skips the download, and cli_binary plus the documented ~/.local/bin lookup let a baked-in CLI be used; README explicitly recommends this ("Bake the CLI into the image and set this to false for faster workspaces"). |
| Egress transparency | 3 | half | No dedicated network/egress/air-gapped section. Endpoints (cursor.com/install, cursor.com/agents/<id>, cursor://... deeplink) are only mentioned scattered across the install-flow and app descriptions, capping this at half per rubric. |
| Runs without sudo | 2 | full | None of install.sh.tftpl, start.sh.tftpl, stop.sh.tftpl, or status.sh.tftpl invoke sudo; all run as the workspace user. |
Engineering Quality — 10 / 10
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Input quality | 6 | full | Every variable has a clear description and sensible default; cli_binary, shutdown_grace_seconds, and serving_log_pattern all carry validation blocks guarding against shell injection or degenerate values. |
| Test coverage | 4 | full | main.tftest.hcl thoroughly covers parameter contract, credential-kind branching, injection safety, and script wiring; main.test.ts runs true end-to-end container tests exercising install/start/stop lifecycle, restart handling, and shutdown draining. |
Overall — 59 / 100
Raw 54.5 / 92 → round(54.5 / 92 × 100) = 59
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.
|
@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: dd4421987b
ℹ️ 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".
| install_cli = var.install_cli | ||
| }) | ||
|
|
||
| stop_script = templatefile("${path.module}/stop.sh.tftpl", { |
There was a problem hiding this comment.
The new template is loaded from the module root, but the repository convention requires script templates to live under scripts/ and be rendered at plan time. Move it to scripts/stop.sh.tftpl and update this templatefile() path so module tooling and maintainers can rely on the required layout.
AGENTS.md reference: AGENTS.md:L56-L69
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one, and I want to be explicit about why rather than silently ignore it.
The rule is real, but this module's three existing templates — install.sh.tftpl, start.sh.tftpl, status.sh.tftpl — all live at the module root, as do the Claude module's. Moving only the new file would leave one script in scripts/ and three beside it, which is worse for a maintainer than either consistent layout.
Happy to move all four (in both modules) as its own PR, where the diff is a pure relocation and easy to review. Doing it here would mix a layout change into a shutdown fix.
| type = number | ||
| default = 60 | ||
| description = <<-EOT |
There was a problem hiding this comment.
This new variable block places type and default before description, contrary to the repository's required description → type → default → validation → sensitive ordering. Reorder these attributes to keep generated module source consistent with the documented convention.
AGENTS.md reference: AGENTS.md:L113-L124
Useful? React with 👍 / 👎.
| [ -n "$${1:-}" ] && kill -0 "$1" 2>/dev/null && | ||
| tr '\0' ' ' <"/proc/$1/cmdline" 2>/dev/null | grep -qF -- "$(basename "${cli_binary}")" |
There was a problem hiding this comment.
When a stale working <pid> entry points to a recycled PID, this substring test accepts any command line containing the generic basename agent—including a Coder process invoked with an agent argument—and line 47 then sends SIGTERM to that unrelated process. This defeats the PID-reuse protection precisely in the recovery case it is meant to handle; verify the executable or the expected agent worker ... argv structure rather than grepping for the basename anywhere.
Useful? React with 👍 / 👎.
dd44219 to
2dbb057
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 worker was therefore killed outright: it never deregistered from its pool, its sessionEnd hook never ran, and work in flight was lost. This is the same gap the Claude Code module has, in the same place. Add a run_on_stop script that relays the signal and waits for the supervisor to record the exit -- not merely for the pid to disappear, which can happen before the supervisor resumes from `wait` and writes "done <code>", leaving a state file that reads as orphaned. Tighten the module's worker_alive guard while reusing it. Matching the bare basename accepted any command line containing "agent", including the workspace agent's own, which defeats the pid-reuse protection exactly in the recovery case it exists for; it now matches the argv the supervisor launches. The stop step is the one caller that signals, so the copies in start and status move with it. Export shutdown_grace_seconds for the template to wire into docker_container.destroy_grace_seconds or a pod's termination_grace_period_seconds, since a module owns no compute resource. Unlike the Claude Code runner, this one is an input rather than a computed value. The Cursor CLI advertises no shutdown budget and exposes no drain, retire or push-outcome flag -- verified against `agent worker --help` -- so there is nothing to derive it from. It defaults to 60 seconds and is the operator's to tune.
2dbb057 to
19d7579
Compare
1 participant
The same gap as #1143, in the same place, for the Cursor module. Independent of it — different files, no conflict.
Problem
start.sh.tftpldetaches the supervisor withsetsid, so it lives in its own session and never receives the SIGTERM the container's init gets. The worker is killed outright: it never deregisters from its pool, itssessionEndhook never runs, and work in flight is lost.What is different from the Claude module
I checked the Cursor CLI's surface rather than assuming it mirrors Claude's, and it does not:
setsiddetachment gapworking <pid>/done <code>--drain-wait-sec,--retire-at,--push-outcome-on-releaseVerified against
agent worker --helpon the CLI inghcr.io/coder/agent-relay-workspace(build2026.09.23-86fc751).So
shutdown_grace_secondshere is an input with a default of 60, not a value computed from the CLI as in #1143. There is nothing to derive it from, and the README says so rather than implying a number that came from somewhere.What is shared
The stop script is the same shape: SIGTERM only, never escalates, never writes the state file, always exits 0, and stays out of the
coder exp syncchain.One improvement over #1143, taken from this module: it reuses the existing
worker_alivehelper, which confirms the pid is still the worker via/proc/<pid>/cmdlinebefore signalling. A barekill -0would trust a recycled pid, and the stop step is the one caller that actually sends a signal. I have backported that guard to #1143.Not included
agent workerhas several flags this module does not expose, which I looked at and deliberately left out of a shutdown PR:--name— display name, defaults to hostname. The analogue ofclient_label.--label key=value— repeatable, unlike Claude's single label. Whether it affects routing is undocumented, so I make no claim.--mint-github-token— short-lived GitHub tokens during claimed runs, pool workers only, requires a team admin to enable. The analogue of the Anthropic git proxy in feat(agent-relay-claude-code): support Anthropic-managed git #1144, and the same answer for a service-account owner that cannot completecoder_external_auth.--management-addr—/healthz,/readyzand a Prometheus/metricsscrape.Each is worth its own PR.
Testing
terraform test: 13 pass, including the pid guard, SIGTERM-only, the budget matching between the output and the script, an override, and rejection of0.bun test: 18 pass.Not verified: whether the Cursor worker traps SIGTERM internally and drains, or just dies. That needs a valid pool credential — the worker exits immediately on a bad key. I tried reading the binary for signal handling and it is packed, so
stringsreturns nothing even for known flag names; that check proves nothing either way. The stop script is correct regardless, since without it the worker gets no signal at all.Minor bump,
0.2.0→0.3.0.Generated with Claude Code using Claude Opus 4.5.