ssh: shorter connection reuse, and say why an expired certificate was not renewed by CarmenDou · Pull Request #348 · InsForge/instacloud-cli

CarmenDou · 2026-10-04T15:22:11Z

What

There are two changes to the SSH aliases insta compute ssh sets up.

Shorter connection reuse. The generated ssh config keeps a multiplexed connection for 60s instead of 10m. The gateway pins each connection to one pod. With 10m, a connection outlived a pod that scaled to zero after 300s idle, and every later ssh over it failed with runtime object not found (14 of 14 in a 2026-10-03 prod repro). Existing installs are rewritten automatically on their next ssh, by the drift repair the renewal hook already runs.
An expired certificate that could not be renewed now says why, in one line. Agent renewal already follows the project's agent policy, which staging confirmed on 2026-10-04. A full_access project renews from the project directory. A restricted project raises one deduplicated approval and renews once it is approved. What failed was the silence: the agent only ever saw Permission denied (publickey).

The gateway half (the connection re-pins to the current pod) is InsForge/instacloud-compute branch fix/ssh-stale-pin, with spec docs/superpowers/specs/2026-10-04-ssh-stale-pin-design.md. The two ship independently.

How

ssh-config.ts renders ControlPersist 60s. Windows still renders no multiplexing lines. The old comment said 10m kept a developer under the per-service session cap, but the gateway now counts sessions, not connections, so that reason no longer holds.
ensureCertForAlias keeps its flow and its silence, with one exception. For an alias this CLI manages, if any exit leaves the certificate missing or expired, so this ssh will fail, it writes one stderr line naming the cause. That covers a thrown error, a lost renewal lock, a commit abandoned after the mint, and a stale-stanza repair that failed. The causes:
- no agent session for the project in this directory
- approval needed, with the platform's review text stripped of control characters (C0, C1, DEL) through config.ts's existing safeText
- agent policy denies it, matched on the platform's "denied by agent policy" error
- another ssh may be renewing it right now, for the lock loser
- otherwise, run insta compute ssh <service>
The exit code stays 0, and nothing goes to stdout or the console. A certificate that still works stays silent and costs no extra check.
mintCert treated a 202 approval as a malformed certificate. It now throws AgentApprovalRequired, which also gives insta compute ssh a real approval message under a restricted policy.
loadAgentSession throws AgentSessionMissing, a subclass of Error, with the same message, so the notice can name that cause.

Verify

npm run typecheck && npm test passes: 101 files and 2015 tests.
Every new test was watched failing before its change. Each change also had a negative control that failed only its own test:
- the 10m value
- the expired-only margin
- the 202 line
- the agent-session type
- the whitespace collapse that keeps the notice to one line
Two existing tests pinned the old behavior and were updated on purpose. One asserted the ControlPersist 10m text. The other asserted total silence for a missing certificate with the platform unreachable, which is exactly the case the spec now makes say one line. It still asserts that nothing reaches stdout or the console.
No command or flag changed, so skills/insta/cli-reference.md needs no update.
Codex review, four rounds. Each round's Critical was fixed with a test that failed first:
- Round 1: unsanitized approval text. Fixed in 9a2723c.
- Round 2: abandoned renewals stayed silent. Fixed in b6b054d.
- Round 3: Codex reviewed a stale checkout, so nothing new.
- Round 4: a failed stale-stanza repair stayed silent. Fixed in a5d5149, together with the softer lock-loser wording Codex suggested.
cubic: three of four comments fixed. Declined: narrowing AgentSessionMissing to ENOENT, since loadAgentSession already gave every read failure the same guidance and narrowing it would change other callers.

🤖 Generated with Claude Code

The SSH gateway pins each connection to one pod. With ControlPersist 10m a master outlived any pod replaced in the meantime, and a scale-to-zero service suspends after 300s idle, so a developer who left and came back five to ten minutes later had every ssh over the old master fail with "runtime object not found" (14 of 14 in a 2026-10-03 prod repro). Each retry also reset the 10 minute timer, so retrying kept it broken. 60s still collapses a burst (an IDE's connections, scp after ssh) onto one connection. The old reason for 10m, the per-service session cap, no longer holds: the gateway counts sessions, not connections. Existing installs are rewritten by the drift repair on their next ssh, which the new orchestration test pins. The gateway half (re-pinning the connection) is InsForge/instacloud-compute branch fix/ssh-stale-pin.

…e line The renewal hook swallowed every failure, so a certificate that had already expired ended in a bare "Permission denied (publickey)". An agent had no way to learn why. Staging on 2026-10-04 showed agent renewal does follow the project's agent policy: full_access renews from the project directory, and a restricted project raises one deduplicated approval that renews once approved. The only problem was the silence. An agent could not tell it was in the wrong directory, or that an approval was waiting. Now, when renewal fails and the certificate has already expired (so this ssh will fail), the hook writes one stderr line naming the cause: - no agent session for the project in this directory - approval needed, with the platform's review text - the agent policy denies it - or, for a person, run "insta compute ssh <service>" It stays silent while the certificate still works and exits 0 every time. mintCert treated a 202 approval as a malformed certificate. It now throws AgentApprovalRequired, which also gives `insta compute ssh` a real approval message under a restricted policy. A missing agent session gets its own error type, AgentSessionMissing, with the same message. The "says nothing when the platform cannot be reached" test asserted the old silence for a missing certificate. It now asserts the one stderr line and still checks that nothing reaches stdout or the console.

cubic-dev-ai

2 issues found across 5 files

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/commands/compute.ts">

<violation number="1" location="src/commands/compute.ts:1693">
P3: `certNeedsRenewal(..., { marginMs: 0 })` also returns true for a missing or unreadable certificate, so this can claim a certificate expired when its expiry was never established. Emit this notice only for a parseable expiry at or before now, or use wording that covers an unusable certificate too.</violation>
</file>

<file name="src/agent.ts">

<violation number="1" location="src/agent.ts:68">
P2: This also turns `EACCES` and other I/O failures into `AgentSessionMissing`, so an expired-certificate renewal falsely reports that no project session exists. Translate expected missing or invalid-session cases only; let unrelated I/O errors reach the generic retry notice.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

cubic-dev-ai · 2026-10-04T15:25:36Z

      || !Number.isFinite(Date.parse(session.expiresAt)) || Date.parse(session.expiresAt) <= Date.now()) throw new Error()
    return session
-  } catch { throw new Error(guidance) }
+  } catch { throw new AgentSessionMissing(guidance) }


P2: This also turns EACCES and other I/O failures into AgentSessionMissing, so an expired-certificate renewal falsely reports that no project session exists. Translate expected missing or invalid-session cases only; let unrelated I/O errors reach the generic retry notice.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/agent.ts, line 68: <comment>This also turns `EACCES` and other I/O failures into `AgentSessionMissing`, so an expired-certificate renewal falsely reports that no project session exists. Translate expected missing or invalid-session cases only; let unrelated I/O errors reach the generic retry notice.</comment> <file context> @@ -62,7 +65,7 @@ export async function loadAgentSession(apiUrl: string, projectId: string, cwd = || !Number.isFinite(Date.parse(session.expiresAt)) || Date.parse(session.expiresAt) <= Date.now()) throw new Error() return session - } catch { throw new Error(guidance) } + } catch { throw new AgentSessionMissing(guidance) } } </file context>

cubic-dev-ai · 2026-10-04T15:25:36Z

-    // Deliberately swallowed. See above.
+  } catch (err) {
+    // Swallowed unless this ssh is about to fail on it: then one line says why.
+    if (certNeedsRenewal(instaCertPath(alias), { marginMs: 0 })) {


P3: certNeedsRenewal(..., { marginMs: 0 }) also returns true for a missing or unreadable certificate, so this can claim a certificate expired when its expiry was never established. Emit this notice only for a parseable expiry at or before now, or use wording that covers an unusable certificate too.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/compute.ts, line 1693: <comment>`certNeedsRenewal(..., { marginMs: 0 })` also returns true for a missing or unreadable certificate, so this can claim a certificate expired when its expiry was never established. Emit this notice only for a parseable expiry at or before now, or use wording that covers an unusable certificate too.</comment> <file context> @@ -1671,8 +1688,11 @@ export async function ensureCertForAlias(alias: string, timeoutMs = RENEWAL_REQU - // Deliberately swallowed. See above. + } catch (err) { + // Swallowed unless this ssh is about to fail on it: then one line says why. + if (certNeedsRenewal(instaCertPath(alias), { marginMs: 0 })) { + process.stderr.write(renewalFailureNotice(alias, err, agentMode() !== null) + '\n') + } </file context>

…ired From the cubic review on #348: - A 403 is reported as a policy denial only when its error ends in "denied by agent policy", which is how the platform's agent governance words it (agent-routes.ts). Other 403s fall through to the generic line: not a member, or an agent credential key refused at "a shell needs an interactive login". That makes the agent flag unnecessary, so renewalFailureNotice takes the alias and the error only. - The notice says "is missing or expired", because the trigger also fires for a certificate file that is absent or unreadable. - The near-expiry fixture is minted inside the test that uses it. Minted at module load with a 2 minute life, a slow run could reach the test after it expired and fail the silence assertion. - renewalFailureNotice moves above ensureCertForAlias's doc comment, which it had been inserted under. Not changed: loadAgentSession still reports every read or parse failure as AgentSessionMissing (cubic suggested only ENOENT). It already gave every such failure the same "agent session missing" guidance before this branch, and narrowing it would change what its other callers see, so that is left as is.

jwfing

Summary

The implementation correctly shortens SSH connection reuse and adds targeted renewal-failure notices, but the approval notice must sanitize untrusted platform text before being written to the terminal.

Requirements context

I assessed the change against the PR description, the repository’s agent/development guidance, and the existing SSH renewal/configuration architecture. The referenced compute-repository design was not present in this checkout and could not be retrieved, so the CLI half was evaluated against the PR description alone.

Findings

Critical

Platform-provided approval text can inject terminal control sequences. AgentApprovalRequired.message comes from the API response, but the formatter only collapses whitespace before writing it directly to stderr. Characters such as ESC (U+001B) and C1 CSI (U+009B) survive replace(/\s+/g, ' '), allowing the response to repaint or spoof terminal output; escape sequences can also move the cursor and break the promised one-line presentation. This conflicts with the repository’s established treatment of API/file-derived terminal text as untrusted. Strip C0/C1 controls before interpolation and add regression coverage for both ESC and C1 sequences, not only \n. (src/commands/compute.ts:1534-1535, test/ssh-orchestration.test.ts:1867-1872)

Suggestion

(none)

Information

Software engineering/functionality: The 60-second Unix multiplexing value, Windows omission, automatic stale-config repair, expired-versus-near-expiry behavior, typed missing-session path, approval response, policy denial, and stderr-only behavior all have focused coverage and fit existing conventions. (src/commands/ssh-config.ts:189-210, src/agent.ts:26-68, src/commands/compute.ts:1691-1697, test/ssh-config.test.ts:480-482, test/ssh-orchestration.test.ts:1837-1892)
Performance: No blocking performance issue found. The shorter persistence interval intentionally trades occasional extra SSH handshakes for avoiding stale pod-pinned connections; renewal remains bounded and only emits the extra notice on an unusable certificate. (src/commands/ssh-config.ts:205-210, src/commands/compute.ts:1691-1695)
Automated verification could not run because this checkout lacks installed development dependencies: npm run typecheck stopped with tsc: not found. No files were modified.

Verdict

Request changes because the newly introduced automatic terminal output includes an unsanitized API-controlled string.

…otice The approval notice interpolates AgentApprovalRequired.message, which is platform text, and wrote it to stderr after only collapsing whitespace. ESC and the single-byte C1 CSI (U+009B) survive that, so a response could repaint or spoof the terminal, or break the one-line notice (Codex review on #348, Critical). The notice now runs that text through config.ts's safeText, the repo's existing helper that removes C0 and C1 controls and DEL. It is exported for this rather than copied. Whitespace is collapsed first, so a newline still becomes a space instead of joining two words. The new test sends ESC and a C1 CSI in the approval message, and checks that neither reaches stderr and that the link survives on a single line.

jwfing

Summary

The change implements the intended SSH configuration and renewal notices, but some expected renewal races still bypass the new notice entirely.

Requirements context

I assessed the change against the PR description, the existing renewal-hook invariants and comments, and the repository tests. The referenced gateway design belongs to the separate instacloud-compute branch and was not available in this checkout, so no additional CLI requirements were inferred from it.

Findings

Critical

src/commands/compute.ts:1592-1593, src/commands/compute.ts:1635-1661 — The notice is emitted only from the exception handler, but renewal can be abandoned through several normal returns while the installed certificate remains expired. Most notably, losing the deliberately non-blocking renewal-lock race returns immediately; with the documented burst of IDE/SSH connections, that invocation can proceed with the expired certificate before the winner finishes and still show only Permission denied (publickey). Post-mint record changes and a moved endpoint without a CA have the same silent outcome. This contradicts the stated guarantee that an expired certificate which could not be renewed says why. Please make these abandonment outcomes observable when the certificate is still expired and add a regression test using an expired certificate plus a held renewal lock.

Suggestion

(none)

Information

src/commands/ssh-config.ts:189-210, test/ssh-config.test.ts:480-483, test/ssh-orchestration.test.ts:1492-1505 — The 60-second ControlPersist value, effective OpenSSH behavior, and automatic repair of existing 10-minute stanzas are covered.
src/commands/compute.ts:1529-1541, src/config.ts:300-303, test/ssh-orchestration.test.ts:1856-1900 — Exception-based failure cases cover generic errors, approval-required responses, missing agent sessions, policy denials, one-line output, and terminal-control sanitization. No security-relevant regression, authorization weakening, secret exposure, or new dependency was found.
src/commands/compute.ts:1692-1698 — The additional certificate check occurs only on the failure path; no material performance regression, unbounded work, or new blocking network behavior was found.
I could not independently execute the test suite because dependencies are absent from the read-only checkout (tsc: not found); git diff --check passed.

Verdict

Request changes: one critical functionality/race gap remains; no separate security or performance blockers were found.

cubic-dev-ai

All reported issues were addressed across 3 files (changes from recent commits).

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

The notice only fired from the catch block, but renewal can also give up through normal returns while the installed certificate stays expired (Codex review on #348, Critical): - losing the non-blocking renewal lock, the common case when an IDE opens several connections at once - the alias record changing while the mint was in flight - a moved host with no CA in the response Each of those still ended in a bare "Permission denied (publickey)". The check now sits in finally. Once renewal has been attempted for an alias this CLI manages, any exit that leaves the certificate missing or expired prints the one line, with the cause if there is one. The lock loser says another ssh is renewing it right now and to retry in a moment. If the winner already finished, the certificate is fresh and nothing is printed. Aliases the CLI does not manage, and certificates that still work, stay silent as before. Two new tests: an expired certificate plus a held renewal lock (no request is sent, the in-progress line is printed), and a record removed during the mint (the certificate is left as it was, the generic line is printed). Both failed before this change.

jwfing

Summary

The change correctly shortens SSH connection reuse and improves several renewal errors, but the checked-out implementation does not report all failed renewals of expired certificates.

Requirements context

I assessed the change against the PR description: Unix SSH aliases should use ControlPersist 60s, existing stanzas should be repaired, and any attempted renewal that leaves a missing or expired certificate should emit exactly one explanatory stderr line while preserving exit code 0. I also inspected the repository development guidance and surrounding SSH orchestration tests; no repository-local behavioral design for this change was found, and the referenced gateway design is outside this repository.

The supplied diff does not match the clean checked-out head at 9a2723c: it includes additional lock-contention and post-mint-abandonment handling/tests that are absent from the workspace. Per the review instructions, this verdict is based on the checked-out files.

Findings

Critical

Expired certificates can still fail silently on normal-return renewal failures. The notice is emitted only from catch, but losing the nonblocking renewal lock returns normally. If two SSH processes encounter an expired certificate, the loser immediately continues toward authentication with that expired credential and no explanation—the exact silent Permission denied (publickey) behavior this PR claims to eliminate. Similarly, after minting, the transaction can return without committing when the alias changed/disappeared or a moved host lacks a CA; because these are normal returns, an expired certificate left in place also produces no notice. Check the certificate state after every attempted renewal, including normal-return paths, and add regression coverage for lock loss and abandoned post-mint commits. (src/commands/compute.ts:1592-1593, src/commands/compute.ts:1635-1661, src/commands/compute.ts:1692-1696)

Suggestion

(none)

Information

Software engineering: Tests cover the 60-second rendered value, drift repair, network failures, near-expiry silence, approval responses, missing agent sessions, policy denial, and terminal-control stripping. The critical normal-return paths above are the material coverage gap. (test/ssh-config.test.ts:477-481, test/ssh-orchestration.test.ts:1837-1901)
Security: No authorization weakening or new dependency was introduced. Aliases remain narrowly validated, and platform-provided approval text is stripped of terminal control characters before reaching stderr. (src/commands/ssh-config.ts:16-23, src/commands/compute.ts:1529-1541, src/config.ts:299-304)
Performance: No N+1 work, unbounded loop, or large allocation was added. The healthy-certificate path still returns before API work, while ControlPersist 60s intentionally trades some additional handshakes for avoiding stale pod-pinned connections. (src/commands/compute.ts:1550-1581, src/commands/ssh-config.ts:186-209)
git diff --check main...HEAD passed. I could not execute typechecking or tests because dependencies are not installed in the review workspace (tsc: not found), and read-only constraints prevented installing them. (package.json:23-27)

Verdict

Request changes: the checked-out implementation violates the explicit expired-certificate notice requirement on reachable concurrency and transaction-abandonment paths.

cubic-dev-ai

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/commands/compute.ts">

<violation number="1" location="src/commands/compute.ts:1604">
P2: `acquireRenewalLock` can return `undefined` when creating the lock fails, not only when another SSH holds it. This then falsely tells users to wait; distinguish contention from lock errors or use the generic renewal-failure notice for ambiguous failures.</violation>
</file>

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

cubic-dev-ai · 2026-10-04T18:44:50Z

    release = acquireRenewalLock(alias)
-    if (!release) return
+    if (!release) {
+      cause = new RenewalInProgress()


P2: acquireRenewalLock can return undefined when creating the lock fails, not only when another SSH holds it. This then falsely tells users to wait; distinguish contention from lock errors or use the generic renewal-failure notice for ambiguous failures.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/compute.ts, line 1604: <comment>`acquireRenewalLock` can return `undefined` when creating the lock fails, not only when another SSH holds it. This then falsely tells users to wait; distinguish contention from lock errors or use the generic renewal-failure notice for ambiguous failures.</comment> <file context> @@ -1590,7 +1600,10 @@ export async function ensureCertForAlias(alias: string, timeoutMs = RENEWAL_REQU release = acquireRenewalLock(alias) - if (!release) return + if (!release) { + cause = new RenewalInProgress() + return + } </file context>

jwfing

Summary

The core behavior is sound, but one control-flow gap still suppresses the required notice for some expired certificates.

Requirements context

I assessed the change against the PR description and the existing SSH renewal invariants in the repository. The referenced gateway design is not present in this checkout, and no linked CLI issue or additional implementation spec was found, so the PR description is the primary requirement source.

Findings

Critical

A stale-config repair failure still leaves an expired certificate silent. Config drift repair can throw before attempted is set. In that case control reaches finally with attempted === false, so no notice is written even when the managed certificate is already expired or missing. This is especially relevant to this PR because every existing stanza containing ControlPersist 10m is initially stale; lock timeout, permissions, or an SSH-config rewrite failure can therefore reproduce the original bare Permission denied (publickey) behavior. Establish the managed/expired state before the potentially failing repair, or otherwise base the final notice on that state, and add a regression test for this path. (src/commands/compute.ts:1583-1591, src/commands/compute.ts:1705-1712, test/ssh-orchestration.test.ts:1837-1915)

Suggestion

Do not always describe lock acquisition failure as another SSH renewing. acquireRenewalLock() returns undefined for both genuine contention and filesystem failures such as an unwritable directory or a symlinked lock path, but the notice unconditionally says another SSH is renewing. That can send users into an ineffective retry loop; consider distinguishing contention from lock I/O failure or falling back to the generic explicit-renewal guidance. (src/commands/compute.ts:1602-1605, src/commands/compute.ts:1729-1775)

Information

Software-engineering coverage is otherwise strong: tests cover effective OpenSSH parsing, automatic replacement of the old 10-minute stanza, expired versus near-expiry behavior, approval text sanitization, missing sessions, policy denial, contention, and abandoned commits. (test/ssh-config.test.ts:480-483, test/ssh-orchestration.test.ts:1489-1505, test/ssh-orchestration.test.ts:1837-1923)
No additional security issue found. Aliases remain narrowly validated, and untrusted approval text has terminal controls removed before reaching stderr. (src/commands/compute.ts:1532-1547, src/commands/compute.ts:1575-1575)
No material performance regression found. The valid-certificate fast path still returns before API work, while the shorter multiplexing lifetime intentionally trades occasional extra handshakes for avoiding stale pod pins. (src/commands/compute.ts:1589-1589, src/commands/ssh-config.ts:189-210)
git diff --check passed, but typecheck and tests could not be run in this checkout because dependencies are absent (tsc: not found). (package.json:21-29)

Verdict

Request changes because the expired-certificate notice is still skipped on a reachable failure path, contrary to the stated behavior.

…rtificate The config-drift repair runs before the renewal gate, and it was outside the "explain on exit" window. So a repair that threw (an alias-store lock timeout, permissions, a failed ~/.ssh/config rewrite) left an expired certificate silent. This PR makes every existing ControlPersist 10m stanza stale, so that path is reachable on the first ssh after an upgrade (Codex review on #348, Critical). Whether this CLI manages the alias is now decided first, from the same store read the repair uses. The flag is set before the repair can throw. It is cleared only once the certificate is confirmed to work, so the healthy path still costs one ssh-keygen read and no second check on the way out. The lock-loser line now reads "another ssh may be renewing it right now. Retry in a moment, or run ...", because acquireRenewalLock also returns nothing on lock I/O failures, not only on contention (Codex suggestion). New test: ~/.ssh made read-only with a 10m stanza and an expired certificate. The repair is blocked (the stanza still says 10m), and the generic line is printed. It failed before this change.

cubic-dev-ai

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/commands/compute.ts">

<violation number="1" location="src/commands/compute.ts:1576">
P3: This store read now precedes the certificate check, and the following stale-block check can inspect or rewrite SSH config even for a fresh certificate. Update the fast-path comment above to describe the new ordering.</violation>
</file>

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

cubic-dev-ai · 2026-10-04T18:52:17Z

+  let cause: unknown
  try {
    if (!isSafeAlias(alias)) return
+    const managed = readAliasStore()


P3: This store read now precedes the certificate check, and the following stale-block check can inspect or rewrite SSH config even for a fresh certificate. Update the fast-path comment above to describe the new ordering.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/compute.ts, line 1576: <comment>This store read now precedes the certificate check, and the following stale-block check can inspect or rewrite SSH config even for a fresh certificate. Update the fast-path comment above to describe the new ordering.</comment> <file context> @@ -1569,26 +1569,30 @@ export function renewalFailureNotice(alias: string, err: unknown): string { let cause: unknown try { if (!isSafeAlias(alias)) return + const managed = readAliasStore() + if (!managed[alias]) return // not an alias this CLI manages: stay silent + explainOnExit = true // set before the repair, which can throw too </file context>

…tests From cubic's second pass on #348: - safeText also removes bidi marks, embeddings, overrides and isolates (U+200E, U+200F, U+202A-U+202E, U+2066-U+2069). It already removed C0, C1 and DEL. An override in the platform's approval text could otherwise reorder the notice or the approval link in a bidi-aware terminal. The change is in the shared helper, so every terminal line built through it gets the same protection. The new test sends U+202E and the isolates, failed before, and passes now. - The abandoned-commit test asserts that the mint request was made, so an ordinary renewal failure cannot satisfy it. - The blocked-repair test is skipped for uid 0, because root can write into the 0500 directory the test relies on. CI and dev machines run as normal users. Not changed: the lock-failure wording was already made non-committal in a5d5149, and the fast-path doc comment's ordering (P3) predates this branch.

jwfing

Summary

The changes correctly shorten Unix SSH connection reuse and provide a safe, single-line explanation when a managed alias has an unusable certificate that renewal could not replace.

Requirements context

I assessed the change against the PR title and description, the existing SSH renewal/config invariants in the repository, and the developer guidance in .claude/skills/developing-insta-cli/SKILL.md. No local CLI-side design document was found; the referenced gateway design belongs to the separate instacloud-compute repository and was not accessible, so gateway behavior was treated as out of scope. No command or flag changes were introduced, so the CLI reference does not require updating.

Findings

Critical

(none)

Suggestion

(none)

Information

Software engineering: Coverage exercises effective OpenSSH parsing, Windows omission, automatic drift repair, missing/expired versus near-expiry certificates, approval responses, missing agent sessions, lock contention, abandoned commits, and terminal sanitization. The patterns are consistent with the surrounding test suite (test/ssh-config.test.ts:480-482, test/ssh-orchestration.test.ts:1857-1950). git diff --check main...HEAD passed. I could not execute typechecking or tests because this read-only checkout has no installed dependencies and npm run typecheck failed with tsc: not found.
Functionality: Unix-like configurations now render ControlPersist 60s while Windows remains unmultiplexed, and the existing drift-repair path rewrites old managed blocks (src/commands/ssh-config.ts:189-210, src/commands/compute.ts:1586-1590). Renewal notices are emitted only when a managed certificate is actually missing or expired after the attempted renewal; still-valid certificates remain silent (src/commands/compute.ts:1575-1595, src/commands/compute.ts:1709-1716). Approval-required responses and missing agent sessions retain distinct error types, enabling the claimed cause-specific messages (src/commands/compute.ts:1071-1075, src/agent.ts:24-68).
Security: Platform-supplied approval text is stripped of terminal controls and bidi reordering characters before reaching stderr, while aliases remain constrained by the existing narrow allowlist (src/config.ts:300-305, src/commands/compute.ts:1532-1547, src/commands/ssh-config.ts:16-23). No authentication or authorization checks are weakened, no secrets are newly printed, and there are no dependency changes.
Performance: The successful-certificate path disables the final validity recheck, while the additional zero-margin certificate inspection is limited to renewal/failure paths (src/commands/compute.ts:1592-1595, src/commands/compute.ts:1712-1715). The shorter persistence interval intentionally trades some additional SSH handshakes for avoiding stale pod-pinned masters; no unbounded work, N+1 behavior, or new network call was introduced.

Verdict

Approved: no Critical findings. Per team policy, this is a bot comment verdict rather than a GitHub green-check approval.

jwfing

LGTM - approved.

CarmenDou added 2 commits October 4, 2026 01:50

cubic-dev-ai Bot reviewed Oct 4, 2026

View reviewed changes

jwfing requested changes Oct 4, 2026

View reviewed changes

cubic-dev-ai Bot reviewed Oct 4, 2026

View reviewed changes

Comment thread src/commands/compute.ts

jwfing requested changes Oct 4, 2026

View reviewed changes

cubic-dev-ai Bot reviewed Oct 4, 2026

View reviewed changes

jwfing requested changes Oct 4, 2026

View reviewed changes

cubic-dev-ai Bot reviewed Oct 4, 2026

View reviewed changes

jwfing reviewed Oct 4, 2026

View reviewed changes

jwfing approved these changes Oct 4, 2026

View reviewed changes

CarmenDou merged commit 777fe26 into main Oct 4, 2026
3 checks passed

Conversation

CarmenDou commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Verify

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cubic-dev-ai Bot Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cubic-dev-ai Bot Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

jwfing left a comment

Choose a reason for hiding this comment

Critical

Suggestion

Information

Uh oh!

jwfing left a comment

Choose a reason for hiding this comment

Critical

Suggestion

Information

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jwfing left a comment

Choose a reason for hiding this comment

Critical

Suggestion

Information

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jwfing left a comment

Choose a reason for hiding this comment

Critical

Suggestion

Information

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cubic-dev-ai Bot Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

jwfing left a comment

Choose a reason for hiding this comment

Uh oh!

jwfing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CarmenDou commented Oct 4, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading