Skip to content

feat(servers): select an existing account SSH key for Managed VPS creation - #39

Merged
thdurante merged 2 commits into
mainfrom
thiago/dhq-692-allow-managed-vps-creation-with-an-existing-ssh-key
Aug 6, 2026
Merged

feat(servers): select an existing account SSH key for Managed VPS creation#39
thdurante merged 2 commits into
mainfrom
thiago/dhq-692-allow-managed-vps-creation-with-an-existing-ssh-key

Conversation

@thdurante

@thdurante thdurante commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds --key-pair-identifier to dhq servers create, so a Managed VPS can be provisioned with an SSH key the operator already holds rather than the shared key DeployHQ auto-creates. That matters for deterministic recovery access — without it there is no guarantee you hold the private key for a droplet you own.

The value is the public identifier from dhq ssh-keys list. The backend resolves it against the account's own keys; unknown or foreign identifiers return 422 and create nothing.

Companion API PR: deployhq/deployhq#1107 (also carries the DHQ-696 security fix this relies on)

Part of DHQ-692.

Deviation from the ticket — please sanity-check

The ticket says to reuse --global-key-pair-id. I didn't, because it cannot work as written:

  • --global-key-pair-id serialises inside the server object
  • Servers::ManagedVps's permit list omits it, and action_on_unpermitted_parameters is unset in production
  • So today dhq servers create --protocol-type managed_vps --global-key-pair-id … is a silent no-op: 200 OK, param dropped, default key used

Making it work would mean serialising one field into two different envelope positions depending on protocol — no precedent in this SDK, and it breaks the day someone permits the field on ManagedVps. A distinct top-level flag matches how the provisioner actually reads params. The two are mutually exclusive and both misuses are rejected locally.

Review round

All five threads resolved. One fix, four skips with rationale.

Reviewer Finding Outcome
Codex --json <fields> parses the field list as a positional arg and hard-fails Fixed in c7b4792 — all 11 occurrences across the three skill files
CodeRabbit Use httptest.NewServer at the command layer Skipped — not implementable: client.go:101 hardcodes https://%s.deployhq.com, so a command-level test resolves to https://x.127.0.0.1:PORT. The SDK tests here already use httptest
CodeRabbit Use a recorded golden fixture, not inline JSON Skipped — repo convention is inline JSON (integration_test.go:16-45), and ssh_key isn't in production yet so there is nothing to record
CodeRabbit Add structured Recovery actions SkippedUserError has no such field; would mean threading one through 155 call sites in 49 files. Both errors set Hint, the mechanism this repo uses
CodeRabbit Strengthen the eval's flag assertions Skippedexpected.flags is a subset assertion by design; the case guards flag choice, not completeness

The --json fix is worth noting beyond the one line Codex flagged: the same broken form shipped in six other places in the previous commit, and SKILL.md:44 was teaching it repo-wide. Two occurrences remain in deployments.md and projects.md — silent-drop class, pre-existing, files outside this PR.

Blast radius

Area Change
pkg/sdk/types.go ServerCreateRequest.KeyPairIdentifier (json:"-", hoisted); ManagedVPSInfo.SSHKey + ManagedVPSSSHKey
pkg/sdk/servers.go one hoist in CreateServer, beside region/size/os_image
internal/commands/servers.go flag + two local guards
skills/, skill-evals/, CHANGELOG.md docs, 4 new eval cases

Additive throughout — omitting the flag leaves every existing path byte-identical.

Also fixes a wrong agent-skill doc

references/global-resources.md documented dhq ssh-keys create --name --public-key. Neither flag exists, and it implied you supply your own public key — keys are generated server-side. Real interface is --title (required) and --type (ED25519 default / RSA). Also documents ssh-keys download -o and ssh-keys delete, and warns against writing a private key to stdout.

This overlaps DHQ-695 (agent-skill command drift) — flagging in case that ticket should own it instead.

Test plan

  • go build ./cmd/dhq/, go vet ./...
  • go test ./...879 passing (was 871)
  • go test -race ./... — 879 passing

New coverage:

  • SDK: identifier is a top-level sibling, explicitly asserting it is not nested inside server and that key_pair_id is never sent
  • SDK: omitted identifier stays off the wire; read-back parses, and an absent ssh_key stays nil rather than a zero-valued struct
  • CLI: flag registered on create and not on update; non-managed_vps protocol and --global-key-pair-id conflict both rejected with zero HTTP (network tripwire)
  • CLI: end-to-end capture proving the identifier lands top-level

Staging, once #1107 is deployed:

dhq ssh-keys list --json title,identifier,fingerprint
dhq servers create -p <project> --name ops --protocol-type managed_vps \
  --region lon1 --size s-1vcpu-1gb --accept-cost \
  --key-pair-identifier <identifier> --json
dhq servers show <server-id> -p <project> --json managed_vps   # expect managed_vps.ssh_key

# must fail, locally, with no request:
dhq servers create -p <project> --name x --protocol-type ssh --key-pair-identifier <identifier>
dhq servers create -p <project> --name x --protocol-type managed_vps --accept-cost \
  --key-pair-identifier <identifier> --global-key-pair-id other

🤖 Generated with Claude Code

https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz

Summary by CodeRabbit

  • New Features
    • Managed VPS server creation now supports selecting an existing account SSH key with --key-pair-identifier.
    • Server details expose the SSH key used, including its identifier, title, and fingerprint.
  • Bug Fixes
    • Added validation for incompatible protocols, conflicting SSH key options, and invalid key identifiers.
  • Documentation
    • Updated SSH key and Managed VPS guides, including key creation, download, deletion, and verification workflows.

…ation

Adds --key-pair-identifier to `dhq servers create`, so a Managed VPS can be
provisioned with an SSH key the operator already has instead of the shared key
DeployHQ auto-creates. That matters for deterministic recovery access: without
it there is no way to guarantee you hold the private key for a droplet.

The value is the PUBLIC identifier from `dhq ssh-keys list`. The backend
resolves it against the account's own keys and rejects an unknown or foreign
identifier with 422, creating neither a server nor a hosted resource. The
internal database id is never accepted from a client.

Sent as a top-level provisioning param, a sibling of `server`, next to
region/size/os_image — the backend reads params[:key_pair_identifier], not
params[:server][...]. Nesting it would be a silent no-op, since Rails' permit
list for Servers::ManagedVps drops it and still returns 2xx.

Note this deliberately does NOT reuse --global-key-pair-id as the ticket
originally proposed. That flag serialises inside `server` and is not permitted
for Servers::ManagedVps, so it is a silent no-op there today; making it work
would need one field serialised into two different envelope positions depending
on protocol, which the SDK has no precedent for. The two are mutually exclusive
and both misuses are rejected locally, before any request.

Read-back adds ManagedVPSInfo.SSHKey so `dhq servers show --json managed_vps`
confirms which key was applied — identifier, title and fingerprint only, never
key material or an internal id.

Also corrects the agent skill: global-resources.md documented
`ssh-keys create --name --public-key`, and neither flag exists. Keys are
generated server-side; the flags are --title and --type. Documents
`ssh-keys download -o` and `ssh-keys delete` too, and warns against writing a
private key to stdout.

Requires the matching API change (deployhq/deployhq).

Suite 871 -> 879, clean under -race and go vet.

Refs DHQ-692

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
@linear

linear Bot commented Aug 5, 2026

Copy link
Copy Markdown

DHQ-692

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The CLI supports servers create --key-pair-identifier for Managed VPS resources. The SDK sends the identifier at the top level and exposes the provisioned SSH key. Validation, serialization, read-back, documentation, and evaluation coverage were added.

Changes

Managed VPS SSH key selection

Layer / File(s) Summary
SDK key contract and serialization
pkg/sdk/types.go, pkg/sdk/servers.go, pkg/sdk/managed_vps_key_test.go
The SDK adds KeyPairIdentifier, serializes it at the request top level, and exposes optional SSH key identifier, title, and fingerprint fields. Tests cover omission and response decoding.
CLI flag, validation, and request wiring
internal/commands/servers.go, internal/commands/servers_test.go
servers create accepts --key-pair-identifier. The command validates Managed VPS usage and rejects conflicts with --global-key-pair-id.
Documentation and evaluation coverage
CHANGELOG.md, skills/deployhq/references/*, skill-evals/deployhq/evals.json
Documentation and evaluations cover SSH key creation, selection, download, deletion, provisioning, and verification.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant ServersCommand
  participant SDK
  participant ManagedVPSAPI
  User->>ServersCommand: Run servers create with --key-pair-identifier
  ServersCommand->>ServersCommand: Validate protocol and conflicting flags
  ServersCommand->>SDK: CreateServer with KeyPairIdentifier
  SDK->>ManagedVPSAPI: Send top-level key_pair_identifier
  ManagedVPSAPI-->>SDK: Return ManagedVPSInfo with ssh_key
  SDK-->>User: Show created server
Loading

Possibly related PRs

Suggested reviewers: facundofarias

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: selecting an existing account SSH key for Managed VPS creation.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch thiago/dhq-692-allow-managed-vps-creation-with-an-existing-ssh-key

Comment @coderabbitai help to get the list of available commands.

@thdurante
thdurante marked this pull request as ready for review August 5, 2026 08:09
@thdurante thdurante self-assigned this Aug 5, 2026
@thdurante thdurante added the WIP Work in Progress label Aug 5, 2026
@thdurante thdurante changed the title WIP: feat(servers): select an existing account SSH key for Managed VPS creation feat(servers): select an existing account SSH key for Managed VPS creation Aug 5, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@internal/commands/servers_test.go`:
- Around line 612-657: Update the three tests in
TestServersCreate_KeyPairIdentifierRejectedForNonVPS_NoHTTP,
TestServersCreate_KeyPairIdentifierConflictsWithGlobalKeyPairID_NoHTTP, and
TestServersCreate_ManagedVPSSendsKeyPairIdentifierTopLevel to use a recorded
httptest.NewServer instead of blockNetwork and captureRequest. Configure the
command host to the test server, return the appropriate recorded response,
assert zero handler calls for validation failures, and record/assert the
successful request body in the handler.

In `@internal/commands/servers.go`:
- Around line 443-459: Update both validation errors in the keyPairIdentifier
checks to include structured Recovery actions describing how to correct the
invalid flag usage, while preserving their existing messages and hints. Ensure
the recovery data is emitted for JSON and represented appropriately in non-JSON
non-interactive output, and add tests covering both protocol-type and mutually
exclusive flag failures.

In `@pkg/sdk/managed_vps_key_test.go`:
- Around line 62-87: Update TestServer_ManagedVPS_SSHKeyReadBack to load and
unmarshal the recorded API JSON golden fixture instead of using the inline raw
payload. Preserve the existing assertions for ManagedVPS.SSHKey fields and keep
TestServer_ManagedVPS_SSHKeyAbsent validating that an omitted ssh_key remains
nil.

In `@skill-evals/deployhq/evals.json`:
- Around line 635-651: Update the `create-managed-vps-with-existing-key`
expected flags to include `--name ops`, `--region lon1`, and `--size
s-1vcpu-1gb` alongside the existing flags. Update `verify-managed-vps-key` so
its expected flags place `--json` before `managed_vps`, keeping `managed_vps` as
the command’s mode argument rather than an extra positional argument.
🪄 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: c0a30148-d0ab-4920-9663-ee0e9541c9b6

📥 Commits

Reviewing files that changed from the base of the PR and between 59d9855 and f5dbe41.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • internal/commands/servers.go
  • internal/commands/servers_test.go
  • pkg/sdk/managed_vps_key_test.go
  • pkg/sdk/servers.go
  • pkg/sdk/types.go
  • skill-evals/deployhq/evals.json
  • skills/deployhq/references/global-resources.md
  • skills/deployhq/references/servers.md

Comment thread internal/commands/servers_test.go
Comment thread internal/commands/servers.go
Comment thread pkg/sdk/managed_vps_key_test.go
Comment thread skill-evals/deployhq/evals.json
@thdurante

Copy link
Copy Markdown
Contributor Author

@codex review this

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f5dbe41bb7

ℹ️ 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".

Comment thread skills/deployhq/references/servers.md Outdated
`--json` has NoOptDefVal="true" (internal/commands/root.go:160), so pflag never
consumes a following token. The space form therefore leaves the field list as a
positional argument:

  dhq servers show srv-001 -p my-app --json managed_vps
  → "error": "accepts 1 arg(s), received 2"     (servers show is ExactArgs(1))

  dhq servers show srv-001 -p my-app --json=managed_vps
  → reaches the API

Verified against a built binary, both forms.

Codex flagged the one line this PR added, but the same broken form shipped in
six other places in the previous commit, and on commands without an Args
validator it fails more quietly still — the field list is silently discarded and
every field is returned. SKILL.md:44 was teaching the wrong form repo-wide.

Fixes all 11 occurrences in the three skill files this PR already touches: 8 in
references/servers.md, 2 in SKILL.md, 1 in references/global-resources.md. Bare
`--json` is untouched.

Two occurrences remain in references/deployments.md:62 and
references/projects.md:10 — both the silent-drop class, both pre-existing, and
both in files outside this PR. Left for a follow-up rather than widening the
diff.

Caught by Codex on #39.

Refs DHQ-692

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hibw5xsRNGDkYzr1hXsDQz
@thdurante
thdurante merged commit 8bb020d into main Aug 6, 2026
25 checks passed
@thdurante
thdurante deleted the thiago/dhq-692-allow-managed-vps-creation-with-an-existing-ssh-key branch August 6, 2026 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants