I read through all the changed files carefully: the new registerWithEnrollToken function, the pkg/registration additions, the unit tests in cli/commands/server_register_enroll_test.go and pkg/registration/registration_test.go, the blackbox harness, and the CI workflow changes.
The design is sound. The unattended path is cleanly separated from the interactive one — Register dispatches to registerWithEnrollToken at the top before touching any of the interactive state, so the two flows can't accidentally contaminate each other. The no-fallback guarantee is correctly implemented: the code checks result.Status != StatusRegistered and returns a terminal error rather than falling through to polling. The comment explaining why (no human at a browser) is exactly the kind of reasoning that pays off when someone is tempted to change it later.
The idempotency strategy is deliberate and well-considered. Saving the key before the HTTP request, then reusing a leftover key on retry, is the right approach for a machine that might crash mid-flight. The tests in TestRegisterWithEnrollTokenReusesSavedKeypairOnRetry directly assert the key identity that makes this work. PublicKeyFromPrivateKeyPEM is tested round-trip and against bad input.
A few things I noticed but that don't block merging:
registerWithEnrollToken skips the pending-record resume path. If a prior interactive enrollment left a pending record and someone then calls register --enroll-token, the enroll path only guards against approved, so it happily clobbers the initializing state with the same-named cluster without noticing the pending record. This is probably fine in practice (pending records are transient, and the two flows are intended to be mutually exclusive), but it is a subtle asymmetry. It doesn't represent a correctness bug for the unattended case.
server_install.go passes EnrollToken to Register even when a pending record was found (line ~401). That means the install command would silently enter the unattended path when re-run with --enroll-token on a box that already had a pending interactive registration. This is a mild interaction, but the failure mode is terminal and user-visible, so it's self-correcting.
The CI routing for TestServerEnrollWithToken is correct: it's excluded from the sharded blackbox runner and pinned to test-blackbox-pop (where the cloud repo and CLOUD_REPO_TOKEN are available), and it skips gracefully when the cloud build predates the mint route. That's the same pattern the other cloud-backed tests use and it's appropriate here.
The test coverage is solid: happy path, three rejection codes, the interactive-fallback guard, and the key-reuse retry. The blackbox test adds end-to-end confidence including the spent-token replay check. This is ready to merge.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1102,
"verdict": "ready",
"event": "approve",
"summary": "I read through all the changed files carefully: the new `registerWithEnrollToken` function, the `pkg/registration` additions, the unit tests in `cli/commands/server_register_enroll_test.go` and `pkg/registration/registration_test.go`, the blackbox harness, and the CI workflow changes.\n\nThe design is sound. The unattended path is cleanly separated from the interactive one — `Register` dispatches to `registerWithEnrollToken` at the top before touching any of the interactive state, so the two flows can't accidentally contaminate each other. The no-fallback guarantee is correctly implemented: the code checks `result.Status != StatusRegistered` and returns a terminal error rather than falling through to polling. The comment explaining *why* (no human at a browser) is exactly the kind of reasoning that pays off when someone is tempted to change it later.\n\nThe idempotency strategy is deliberate and well-considered. Saving the key before the HTTP request, then reusing a leftover key on retry, is the right approach for a machine that might crash mid-flight. The tests in `TestRegisterWithEnrollTokenReusesSavedKeypairOnRetry` directly assert the key identity that makes this work. `PublicKeyFromPrivateKeyPEM` is tested round-trip and against bad input.\n\nA few things I noticed but that don't block merging:\n\n- **`registerWithEnrollToken` skips the `pending`-record resume path**. If a prior *interactive* enrollment left a `pending` record and someone then calls `register --enroll-token`, the enroll path only guards against `approved`, so it happily clobbers the initializing state with the same-named cluster without noticing the pending record. This is probably fine in practice (pending records are transient, and the two flows are intended to be mutually exclusive), but it is a subtle asymmetry. It doesn't represent a correctness bug for the unattended case.\n\n- **`server_install.go` passes `EnrollToken` to `Register` even when a `pending` record was found** (line ~401). That means the install command would silently enter the unattended path when re-run with `--enroll-token` on a box that already had a pending interactive registration. This is a mild interaction, but the failure mode is terminal and user-visible, so it's self-correcting.\n\n- **The CI routing for `TestServerEnrollWithToken` is correct**: it's excluded from the sharded blackbox runner and pinned to `test-blackbox-pop` (where the cloud repo and `CLOUD_REPO_TOKEN` are available), and it skips gracefully when the cloud build predates the mint route. That's the same pattern the other cloud-backed tests use and it's appropriate here.\n\nThe test coverage is solid: happy path, three rejection codes, the interactive-fallback guard, and the key-reuse retry. The blackbox test adds end-to-end confidence including the spent-token replay check. This is ready to merge.",
"posted_to_pr": true
}