Repository navigation
T4 — Stop min login minting certificates and remove min ssh-forward from the client - #1673
Conversation
The HTTPS reverse proxy and its daemon-issued client certificates are retired (NET-109), and with them the client half of the surface: - `min ssh-forward` is gone from the command tree, the dispatch, and the connection inventory; the daemon's direct-tcpip handler stays, per the same ruling. - `min login` no longer calls the certificate RPC, spawns a daemon, or writes a key or CA; it prints the one line saying there is nothing to mint, and `--cert-dir` is refused by the parser. - new tests: `login_mints_no_certificate` and `cli_reference_has_no_retired_commands` (NET-111), guarding that the CLI reference documents neither retired surface.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Reviewed against T4's plan block on #1437 (layers, two test lines, proof).
- Scope: exactly the four layer files, nothing else.
SshForward,SshForwardArgs,cmd_ssh_forwardand the dispatch arm are gone;LoginArgslostcert_dir;cmd_loginmakes no RPC and writes nothing. No helper was orphaned:connect_daemon,ensure_daemon,resolve_session_version_gatedandresolve_minimal_config_dirall keep other callers. - Tests: both plan lines pass on the runner and in CI.
login_mints_no_certificateruns the verb with no daemon, checks the three files are absent, refuses--cert-dir, and guards the function body against re-minting.cli_reference_has_no_retired_commandsguardsdocs/reference/cli-min.mdagainst the retired names and asserts the parser refusesssh-forwardwith usage. The connection inventory drops the ssh-forward row. - Proof matches the task's proof line.
- Draft cause:
agent-verifytimed out at 600 s with clippy alone taking 9m10s in a cold clone. Not a code defect. Every CI lane is green on the head (ci, linux-native, linux-kvm, kani, shell-installer, commitlint; macOS in progress).
Approving. The stale ssh-forward mention in crates/minimal/Cargo.toml's feature comment is out of this task's layers, as the PR notes; it can ride with T5.
|
Approved: every review thread resolved at @norrietaylor resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Verification timed out, and the login-output test does not exercise the command end to end.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Retires client-side certificate minting from min login and removes the ssh-forward command.
Changes:
- Makes
min logina no-op without daemon or filesystem access. - Removes SSH forwarding CLI definitions and dispatch.
- Adds regression coverage for retired surfaces.
| File | Summary |
|---|---|
crates/minimal/src/tests.rs |
Updates inventories and adds retirement tests. |
crates/minimal/src/cmd/mod.rs |
Removes SSH-forward dispatch. |
crates/minimal/src/cmd/admin.rs |
Removes forwarding and certificate-minting logic. |
crates/minimal/src/cli.rs |
Removes retired CLI definitions and certificate options. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/revise Address the open Copilot thread on crates/minimal/src/tests.rs (login_mints_no_certificate): the test asserts the helper's string, not that cmd_login prints it. Make the test prove the verb emits the line — capture what the handler writes, or at least assert cmd_login's body calls login_nothing_to_mint_line and prints it — and answer the thread (👍 and fix if valid, 👎 with the reason if not). Touch only the task's layer files. |
A cold workspace clippy alone ran 9 m 10 s of the gate's 600 s on #1673 and the gate was stopped before nextest began, so a green tree opened as a draft. Clippy now takes the same -p flags nextest already does; with no touched crate it still runs over the workspace. -p checks every dependency of the touched crates, so a warning there is still caught.
|
/revise Judge the open Copilot thread at crates/minimal/src/tests.rs:350: fix it if it is right, otherwise answer it with the reason so the thread can be resolved and the runtime's approval released. |
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 6c365fd79a09 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
|
/revise Judge the open Copilot thread at crates/minimal/src/tests.rs:350 (round 1 died before the coder ran): fix it if it is right, otherwise answer it with the reason so the thread can be resolved. |
…ogin-minting-certificates-and-remove-m
The login_mints_no_certificate test asserted on the string the login_nothing_to_mint_line helper returns, so removing the print or printing a different line still passed (Copilot, PR #1673). cmd_login now writes its notice to a caller-supplied writer (the dispatch hands it a locked stdout) and the test reads the captured output back: it must contain 'Nothing to mint' and be exactly the required line plus one newline.
…ogin-minting-certificates-and-remove-m
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 3d1ae4b89716 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
T4 — Stop min login minting certificates and remove min ssh-forward from the client
slice: S1
kind: task
covers: NET-111
size: M
est: 180 code, 120 tests
executor: agent
difficulty: routine
model: standard
diagnostics: min login prints that it minted no certificate, and the ssh-forward verb is refused by the argument parser with its usage
observability: min login prints one line saying it has nothing to mint; the removed verb logs nothing because it no longer exists
layers:
- cli:
- crates/minimal/src/cli.rs: removes SshForward and SshForwardArgs; LoginArgs loses cert_dir and the Login doc no longer mentions certificates
- crates/minimal/src/cmd/admin.rs: removes cmd_ssh_forward; cmd_login stops calling the certificate RPC and writes no key or certificate
- crates/minimal/src/cmd/mod.rs: drops the ssh-forward dispatch arm
- crates/minimal/src/tests.rs: drops the ssh-forward row from the connection inventory and adds the tests
test: cargo nextest run -p minimal cli_reference_has_no_retired_commands
test: cargo nextest run -p minimal login_mints_no_certificate
produces: none
consumes: none
proof:
min ssh-forwardno longer exists,min loginwrites no key or certificate, and the CLI reference names neither.Proof
min ssh-forwardno longer exists: the argument parser refuses it with "error: unrecognized subcommand 'ssh-forward'" plus its usage (observed on the built binary), and the dispatch arm, args struct and connection-inventory row are gone.min loginconnects to no daemon and writes no key or certificate; it prints the single line "Nothing to mint: the HTTPS reverse proxy is retired, so no client certificate is issued and no key is written." and--cert-diris refused by the parser. docs/reference/cli-min.md names neither retired surface, and the newcli_reference_has_no_retired_commandstest holds that guard.Notes from the agent: All four layer files only; no other file touched. Left out of scope on purpose: crates/minimal/Cargo.toml's comment still says remote-access "gates the
meshandssh-forwardCLI subcommands" — the ssh-forward half of that comment is now stale, the mesh half is true, and the file is not in this task's layers. Daemon-side IssueClientCert and the direct-tcpip handler remain, per NET-110 (direct-tcpip is kept; daemon-side retirement is not this task's client-only layer list). diag/collect.rs still lists client.pem/ca.pem/client.key as bundle candidates — harmless, they no longer exist on disk, and tests/bug.rs's client.key skip assertions are unaffected. The reference test deliberately does not grep for the bare word "login": the verb survives (NET-109 retires the surface, not the verb) and will be documented again when the sign-in lands, so the guard is on ssh-forward and the minted file names (--cert-dir, client.pem/key, ca.pem). Extra runs besides the two named line 8000 s: every_daemon_connection_is_classified and cli_command_tree_stays_renderable both pass; cargo clippy -p minimal --all-targets is clean with and without --features remote-access; cargo fmt -p minimal clean.Tests
cargo nextest run -p minimal cli_reference_has_no_retired_commandscargo nextest run -p minimal login_mints_no_certificateVerify
min task run agent-verifytimed out after 600s, so this PR is a draft.Closes #1613