Resolve SSH relay review annotations and document the connect-token round trip - #322
Merged
dbmikus merged 3 commits intoAug 9, 2026
Merged
Conversation
Add top-of-file comments to norelay/token.go and operations.go explaining how the sandbox-side verifier and writer fit into the full connect-token lifecycle (generation, hashing, delivery, and rotation), with cross-references to the control-plane repo's spec doc and to each other. Plain file-header comments, not package docstrings -- handler.go and command.go already own this package's doc comment.
Work through the `#amika/annotate` questions and TODOs left across the diff during review, one per touched spot: - `amikad serve`: MaxConnections was hardcoded to 64 in the no-relay bridge's Config. Add `ServeOptions.MaxConnections` and a `--max-connections` flag, defaulting to the newly exported `norelay.DefaultMaxConnections` (still 64) rather than a duplicated literal. - `SensitiveStore`/scrub manifest: answer "does scrubbing actually find and clean up these files?" by documenting the amika-mono side of the contract in package state's doc comment (manifest format, register-before-write guarantee, and the control plane's fail-closed sudo scrub at snapshot time), and pointing SetConnectToken's call site at it. - `/healthz`: answer why it isn't named `/health` (Kubernetes/Google liveness convention; the control plane's own e2e checks already probe this exact path) and leave it as-is. - `scpv2`: chase "are the v2 alias test strings still right?" back to its answer -- yes for the test (its stub alias is an independent fixture), but scpv2.go's own doc comments still described the pre-per-environment `<name>.<id>.amika` alias shape. Update them to `<name>.<id>.<environment>.amika` to match `ssh.BuildSessionAlias`. - managed sshd: make the default account configurable via a new `AMIKA_SSHD_USER` environment variable (default "amika"), covering both the UID/GID lookup and the authorized_keys home path. Multi-user support stays out of scope, per the annotation. - `state` package tests: answer "does this need more red-teaming?" by adding coverage for a symlinked ancestor directory (not just the leaf path), a corrupt or hostile on-disk manifest (invalid JSON, JSON `null`, duplicate/relative/self-referential entries), the sensitive file size cap, non-permission mode bits, out-of-range ownership, and a pre-cancelled context. - `plumbing`: drop the redundant `Hidden: true` on `ssh-stdio-proxy`; the parent `plumbing` command is already hidden, so nothing under it surfaces via `--help` regardless of its own Hidden field. - `secrets.go`: replace the "split this file up" annotation with `TODO(KAPRO-747)`, tracking the split as a standalone follow-up instead of folding it into this diff.
- amika-annotations: add `annotate` to the built-in imperative-types table. It showed up nine times in one diff as a free-form question or request and was handled by inference every time; codify it so future runs don't re-derive the same behavior. - AGENTS.md: note that comments/docstrings here sometimes point at amika-mono (the private control-plane monorepo) for protocol details owned by that side. That content isn't in this repo's own specs/ directory, which is numbered separately and OSS-only; look for amika-mono as a sibling worktree instead of grepping for those paths here.
![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: 1a64e1bd3d
ℹ️ 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 on lines
+177
to
+180
| &serveOptions.MaxConnections, | ||
| "max-connections", | ||
| norelay.DefaultMaxConnections, | ||
| "maximum concurrent no-relay bridge sessions", |
There was a problem hiding this comment.
When serve is invoked with both --bg and a non-default --max-connections, Serve branches to serveBackground before constructing the handler, and that function launches the child with only --port and --beta-no-relay. The child therefore silently uses the default limit of 64, so this newly advertised flag has no effect in background mode; include the selected limit in the child arguments.
Useful? React with 👍 / 👎.
| } | ||
| uid, gid := -1, -1 | ||
| if account, err := user.Lookup("amika"); err == nil { | ||
| if account, err := user.Lookup(username); err == nil { |
There was a problem hiding this comment.
When AMIKA_SSHD_USER is set to anything other than amika, this lookup and the authorized-keys ownership switch to the custom account, but RenderConfig still emits AllowUsers amika. The managed daemon consequently denies SSH logins as the selected account, making the override unusable; carry the managed username into the rendered policy instead of retaining the hard-coded allowlist.
Useful? React with 👍 / 👎.
| HostPrivateKey: "/var/lib/amikad/ssh_host_ed25519_key", | ||
| HostPublicKey: "/var/lib/amikad/ssh_host_ed25519_key.pub", | ||
| AuthorizedKeys: "/home/amika/.ssh/authorized_keys", | ||
| AuthorizedKeys: "/home/" + username + "/.ssh/authorized_keys", |
There was a problem hiding this comment.
For a managed account whose home is not /home/<username> (for example a service account with a home under /var/lib), user.Lookup succeeds but this constructs an unrelated authorized-keys path. prepareAuthorizedKeysDirectory then either fails because that directory is absent or installs keys outside the account's home; build the path from the looked-up account's HomeDir.
Useful? React with 👍 / 👎.
This was referenced Aug 9, 2026
dbmikus
changed the title
dbmikus
added a commit
that referenced
this pull request
Aug 10, 2026
Release amika@v0.14.0. Changes since amika@v0.13.0: Direct (no-relay) SSH transport: - Define and tighten the no-relay SSH contracts and vertical-slice spec (fc01f30, 650b433, dac6b6d) - Implement the local no-relay SSH path and fix its runtime lifecycle (d4bb156, 05c6df1) - Add `amika scpv2` over the direct WebSocket transport (9dccbfd) - Bind the managed sshd to a reserved loopback port (2e90dc6) - Scope v2 SSH session config per control plane (c7063c7) - Resolve SSH relay review annotations and document the connect-token round trip (#322) SSH key handling: - Harden authorized key handling and close an ownership race (b565213, f448546, 3fde13c) - Add an amikad authorized-keys clear subcommand (0e2fa59) - Add the amika secret ssh-key command group (#324) Sandbox images: - Build and package amikad into the base images (6279f28, f336617) - Repin the reviewed amikad source as fixes landed (862cbbd, 4777f5c, f864496, a6a4c04) - Add sandbox codev2 editor support (#325) Testing and CI: - Test snapshot scrubbing end to end (#323) - Publish linux-vm Daytona snapshots from CI, with retries and paged state verification (930a99f, 5379037, 4e713a7) Agent tooling: - Add and narrow the amika-cli skill for agents (9b6be25, 8cfc32a) - Add [release <symbol>] prefix and release trailers to the CLI release skill (59526b2) Release-Component: amika Release-Version: v0.14.0
dbmikus
added a commit
that referenced
this pull request
Aug 10, 2026
Releases the `amika` CLI at v0.14.0 and rolls that release into the sandbox base image. Two commits: 1. **`[release amika] Bump install.sh DEFAULT_VERSION to 0.14.0`** — the public CLI release. Carries the `Release-Component` / `Release-Version` trailers. 2. **`Set Amika base snapshot to install amika CLI v0.14.0 and repin amikad`** — bumps `AMIKA_VERSION` to 0.14.0 in `go/internal/sandbox/presets/base/Dockerfile` and repins `AMIKAD_SOURCE_REF` from `9dccbfd` to `4c7b647` (current `main`). The amikad repin matters on its own: the old pin predated the no-relay connect-token work, so sandbox images were building an amikad without the connect-token round trip, the sshd-manager fixes, or the state contract additions. The new ref resolves through the module proxy the builder stage's `go install` uses (`github.com/gofixpoint/amika/go v0.0.0-20260810034344-4c7b647bbeb8`). ## Changes since amika@v0.13.0 **Direct (no-relay) SSH transport** - Define and tighten the no-relay SSH contracts and vertical-slice spec (fc01f30, 650b433, dac6b6d) - Implement the local no-relay SSH path and fix its runtime lifecycle (d4bb156, 05c6df1) - Add `amika scpv2` over the direct WebSocket transport (9dccbfd) - Bind the managed sshd to a reserved loopback port (2e90dc6) - Scope v2 SSH session config per control plane (c7063c7) - Resolve SSH relay review annotations and document the connect-token round trip (#322) **SSH key handling** - Harden authorized key handling and close an ownership race (b565213, f448546, 3fde13c) - Add an amikad `authorized-keys clear` subcommand (0e2fa59) - Add the `amika secret ssh-key` command group (#324) **Sandbox images** - Build and package amikad into the base images (6279f28, f336617) - Repin the reviewed amikad source as fixes landed (862cbbd, 4777f5c, f864496, a6a4c04) - Add sandbox codev2 editor support (#325) **Testing and CI** - Test snapshot scrubbing end to end (#323) - Publish linux-vm Daytona snapshots from CI, with retries and paged state verification (930a99f, 5379037, 4e713a7) **Agent tooling** - Add and narrow the amika-cli skill for agents (9b6be25, 8cfc32a) - Add `[release <symbol>]` prefix and release trailers to the CLI release skill (59526b2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes out the
#amika/annotatereview annotations left on the SSHrelay diff (no-relay connect-token bridge, scpv2, managed sshd, and
statepackage), documents the connect-token round trip for futurereaders, and folds this session's annotation-handling learnings back
into the agent docs.
Changes
Connect-token documentation
norelay/token.goandoperations.goexplaining how the sandbox-side verifier and writer fit into the
full connect-token lifecycle (generation, hashing, delivery,
rotation), cross-referencing each other and the control-plane spec.
Review annotation resolutions
amikad serve: replace the hardcodedMaxConnections: 64with aconfigurable
ServeOptions.MaxConnectionsand--max-connectionsflag, defaulting to the newly exported
norelay.DefaultMaxConnections.statepackage: document the amika-mono side of the scrub-manifestcontract (manifest format, register-before-write guarantee, the
control plane's fail-closed sudo scrub at snapshot time), and point
SetConnectToken's call site at it./healthz: document why it isn't named/health(Kubernetes/Googleliveness convention; the control plane's e2e checks already probe
this path) and leave it as-is.
scpv2: updatescpv2.go's doc comments to the current<name>.<id>.<environment>.amikaalias shape(
ssh.BuildSessionAlias), and note inscpv2_test.gowhy the test'sstub alias fixture doesn't need to track that format.
AMIKA_SSHD_USER(defaultamika) to make themanaged account configurable for both the UID/GID lookup and the
authorized_keyshome path; multi-user support stays out of scope.statepackage tests: add coverage for a symlinked ancestordirectory, a corrupt or hostile on-disk manifest (invalid JSON,
JSON
null, duplicate/relative/self-referential entries), thesensitive file size cap, non-permission mode bits, out-of-range
ownership, and a pre-cancelled context.
plumbing: drop the redundantHidden: trueonssh-stdio-proxysince the parent
plumbingcommand is already hidden.secrets.go: replace the "split this file up" annotation withTODO(KAPRO-747)to track it as a standalone follow-up.Agent docs
amika-annotationsskill: addannotateto the built-inimperative-types table for free-form review questions/requests.
AGENTS.md: note that comments here sometimes point atamika-mono(the private control-plane monorepo) for protocol details owned by
that side, and that it should be looked up as a sibling worktree
rather than searched for in this repo.
Stack
dylan/amika-cli-skillAdd amika-cli skill documenting the CLI for agents #315dylan/ssh-impl-no-relayAdd theamikadsandbox daemon and the no-relay WebSocket SSH path #316dylan/no-ssh-keygen-yet-fixFix the managed sshd port, addamika scpv2, and scope SSH sessions per control plane #321dylan/ssh-websocket-code-review#THIS← you are here