docs(skill): correct verified command drift in the agent skill - #40
Conversation
Four corrections where the skill recommended commands or semantics that do not match the CLI. Each verified against the current source, not assumed. `deploy --wait` does not wait outside a terminal. `Envelope#WantsJSON()` is `JSONMode || !IsTTY` (internal/output/envelope.go:51-56), and the JSON branch in deploy returns as soon as the deployment is queued (internal/commands/deploy.go:588), so the `if wait` block below it is unreachable. In any pipe, CI job, agent invocation or redirect, `--wait` is inert and the command exits 0 while the deployment is still running. Note the trigger is the missing TTY, not the `--json` flag — passing it changes nothing. Confirmed by control test on WantsJSON. The skill documented `--wait --json` as the way to block, in four places. Those now use the safe composition — create, `deployments watch`, then assert `status == "completed"` via `deployments show` — including the two-environment worked example in servers.md. A cancelled deployment exits 0. internal/commands/watch.go:108-111 treats `cancelled` as a clean terminal state and returns nil; only `failed` is non-zero, and `dhq rollback` shares the watcher. The blanket "non-zero = failure" claim in SKILL.md was therefore false, and is now qualified. `ssh-commands create` cannot express a usable release hook: the CLI sends only command, description and timing (internal/commands/ssh_commands.go:229-231), so the callback phase, server targeting, timeout and halt-on-error all fall to backend defaults — before_changes, every server, no failure on error. Documented as a limitation, with a post_deploy ssh deployment check as the first-class alternative, including atomic ordering, variable expansion, failure semantics, the 600-second cap and the beta gate. Reference count in CLAUDE.md said 8; the directory has 9. Adds three evals: automation must not reach for `--wait`, a cancelled deploy must be detected by status rather than exit code, and post-release reconcile must use a deployment check rather than ssh-commands. Suite unchanged at 879 (documentation and evals only). Refs DHQ-695 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
WalkthroughDeployHQ documentation now distinguishes interactive and automated deployment behavior. It requires explicit completion checks, documents cancelled deployment handling, and adds guidance for post-deployment SSH deployment checks. Evaluation cases cover these requirements. ChangesDeployHQ verification guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a9aca8f33
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@CLAUDE.md`:
- Line 45: Update the evaluation-suite total documented in CLAUDE.md from 49 to
52, after verifying that skill-evals/deployhq/evals.json contains the three
newly added cases.
In `@skill-evals/deployhq/evals.json`:
- Around line 653-664: Update the workflow-deploy-wait-timeout eval case in
evals.json to remove the stale dhq deploy --wait --timeout expectation. Require
the agent workflow to create the deployment, watch it, and use deployments show
to explicitly verify status == completed, consistent with the
deploy-and-verify-in-automation case and SKILL.md behavior.
In `@skills/deployhq/references/deployments.md`:
- Around line 88-89: Update the `dhq deployments show` example to request the
`servers` JSON field instead of `server`, matching the serialized
`Deployment.Servers` property while preserving the other selected fields.
In `@skills/deployhq/references/servers.md`:
- Around line 264-266: Update the manual production deploy example to capture
the deployment identifier from dhq deploy, wait for completion with deployments
watch, and perform a final status == "completed" check. Keep the existing
project and Production server parameters while replacing the standalone deploy
--json command with the full verification sequence.
🪄 Autofix
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
Run ID: d8783982-2ef9-4488-a677-01843c466ba1
📒 Files selected for processing (6)
CLAUDE.mdskill-evals/deployhq/evals.jsonskills/deployhq/SKILL.mdskills/deployhq/references/configuration.mdskills/deployhq/references/deployments.mdskills/deployhq/references/servers.md
Review follow-up on #40. `--json=<fields>` unwraps the response envelope: `filterFields` (internal/output/envelope.go:266-270) returns the inner Data, and WriteJSON encodes that directly. So `--json` alone emits `{"ok":true,"data":{...,"status":"completed"}}` while `--json=status` emits `{"status":"completed"}`. The verification recipe added in the previous commit read `.data.status` after `--json=status`, which is always null — so the "safe composition" written to stop automation trusting a bad deploy would instead have failed the job after every *successful* one. Confirmed by control test on WriteJSON with and without field selection. Two call sites fixed (deployments.md, servers.md). The two `.data.identifier` reads are correct and unchanged — they follow a bare `--json`, where the envelope is present. Added a note documenting the shape difference so the two forms are not conflated again. Also strengthens the deploy-and-verify eval: it previously passed for a response containing only `dhq deployments show`, without creating, watching, or asserting the status — the case could not fail for the behaviour it guards. Now requires watch, show and the literal `completed`. Note `check_eval` word-splits `expected.flags` before substring-matching, so the tokens are single words; multi-word entries would silently become separate assertions. Caught by Codex on #40 (P1 and P2). Refs DHQ-695 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
Review follow-up on #40. `Deployment` serialises its targets as `servers` (pkg/sdk/types.go, `Servers []Server json:"servers"`); there is no `server` field on it — that belongs to `DeploymentStep`. The verification example selected `server`, which `pickFields` silently drops, so the field the operator asked for would simply be absent. The same silent-drop behaviour this branch documents two sections earlier. CLAUDE.md's eval-suite count said 49; the suite now has 69. The previous commit corrected the reference-file count two lines above and missed this one. Note the count is 69, not the 52 suggested in review — that assumed this PR's three cases were the only additions, but the stacked branches for DHQ-691 and DHQ-692 added five and four respectively. Verified against `jq '.evals|length'`. Caught by CodeRabbit on #40. Refs DHQ-695 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
skills/ is go:embed-ed into the binary (skills/embed.go:15), so a skill correction changes what ships and belongs in the changelog. #40 had no entry. Also regroups the Unreleased section. The DHQ-692 changelog edit inserted a `### Fixed` heading ahead of the DHQ-691 entries, which left seven Added items sitting under Fixed. Entries are now grouped by kind rather than by the order they were appended. Refs DHQ-695 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
Summary
Four places where the agent skill recommended commands or semantics that don't match the CLI. Each verified against source, not assumed.
Stacked on #39 (base is that branch, not
main) — both touchSKILL.mdandevals.json. Retarget tomainonce #39 merges.Closes DHQ-695.
What was wrong
deploy --waitdocumented as blockingWantsJSON()isJSONMode || !IsTTY(envelope.go:51-56) and the JSON branch returns atdeploy.go:588, soif waitat:598is unreachable. Confirmed by control test onWantsJSON.watch.go:108-111—case "cancelled": … return nil. Cancelled exits 0.dhq rollbackshares the watcher.ssh-commands createshown as a usable release hookssh_commands.go:229-231— only--command,--description,--timing. No callback phase, targeting, timeout, or halt-on-error.The one worth reading
dhq deploy --waitis inert whenever stdout isn't a terminal — the trigger is the missing TTY, not--json. So in every CI job, pipe, redirect and agent invocation,--waitdoes nothing and the command exits 0 while the deployment is still running. Four skill examples taught exactly that pattern.This PR documents the behaviour and replaces those examples with the safe composition. It deliberately does not change the CLI — that was a conscious call, so the flag remains silently inert for existing users. Worth a separate decision.
The
status == "completed"assertion is required rather than belt-and-braces: cancelled exits 0 too. Note.status, not.data.status—--json=<fields>unwraps the envelope and emits the selected fields at the top level.Review round
Four findings, all fixed, all threads resolved.
--json=statusunwraps the envelope, so.data.statusisnull— the verification recipe would exit 1 after every successful deploy785ed0c, both call sites; added a note on the shape differencedeployments show785ed0c— now requires watch, show andcompleted--json=…,server—Deploymenthas noserverfield (that'sDeploymentStep), so it is silently dropped592956f→serversCLAUDE.mdstale592956f→ 69 (not the suggested 52; the stacked branches add 5 and 4 that aren't visible from this diff)The P1 is worth flagging to a reviewer: the recipe added here to stop automation trusting a bad deploy would, as first written, have failed the job after every good one.
Blast radius
Docs and evals only — no Go source touched.
deployments.md,SKILL.md,configuration.md,servers.md,CLAUDE.md,evals.json.Test plan
go build ./cmd/dhq/,go vet ./...,go test ./...— 879 passing, unchanged (docs-only)--wait --jsonleft anywhere inskills/, reference count matches the directory, cancelled documented as unsuccessful, safe composition present, ssh-commands limitation documented, post-deploy check documented with ordering/variables/failure/timeout/beta-gate, evals addeddeployment-checksflags in the recommended snippet verified to exist (deployment_checks.go:106-118)./skill-evals/deployhq/run-evals.shfor the three new cases (needs an API key)3 new evals: automation must not reach for
--wait; a cancelled deploy must be detected by status not exit code; post-release reconcile must use a deployment check rather thanssh-commands.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
Summary by CodeRabbit
Documentation
Tests