Skip to content

test(cli): cover auth, connection status, proxy startup, and replay shard formatting - #3437

Merged
thymikee merged 3 commits into
mainfrom
crap/3420-cli-commands
Oct 11, 2026
Merged

thymikee merged 3 commits into
mainfrom
crap/3420-cli-commands

Conversation

@thymikee

@thymikee thymikee commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Summary

Burns down the five coverage-based CRAP findings in #3420 with tests only — no production changes. Each finding is driven through its owning command's real entry point (CLI capture for auth/connection, the proxyCommand handler, and the format.ts display-name exports).

Function Before After
auth.ts:44 renderAuthStatus 42 (0%) 6 (100%)
auth.ts:7 authCommand 30 (0%) 5 (100%)
connection.ts:353 status render arrow 42 (0%) 6 (100%)
proxy.ts:32 startProxy 30 (0%) 5 (92.3%)
format.ts:139 replayTestShardDeviceName 30 (0%) 5 (100%)

New tests: cli/commands/__tests__/{auth,connection,proxy}.test.ts and replay-test/reporters/__tests__/format.test.ts (575 gross lines, test-only). Mutation-checked: removing the deviceId fallback fails the format test; hardcoding the proxy bind fails the flag test.

Closes #3420. Part of #3414.

Validation

Tested SHA: af9efc5b2. pnpm check:affected --run passes locally under the machine lock on this head. pnpm check:coverage-crap --all on the partial map over the four owned files: no function at or above 30 (was 5). check:quick and pnpm format clean; full CI (including Coverage and Integration Tests) green on the previous head f78c2a197, re-running on this one. No provider-facing code touched.

…hard formatting

Drives the five CRAP findings in #3420 through their owning command entry
points: auth status/login/logout rendering, the connection status human
block, the proxy command's daemon-backed startup and loopback bind, and
replay-test shard device-name suffixes.
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.17 MB 5.17 MB 0 B
Package (unpacked) 5.17 MB 5.17 MB 0 B
Package (download) 1.55 MB 1.55 MB -12 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 22.1 ms 22.5 ms +0.5 ms
CLI --help 58.4 ms 59.2 ms +0.7 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files

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

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread src/cli/commands/__tests__/proxy.test.ts Outdated
Comment thread src/cli/commands/__tests__/connection.test.ts
Review follow-up: record the arguments the command passes to the proxy
server's listen so the --proxy-host/--proxy-port test cannot pass on
defaults, and pin the absence of the deferred Metro notice in the
bound-lease connection status case.
@thymikee

Copy link
Copy Markdown
Member Author

The PR is ready at f78c2a1. The code is correct, all 18 checks pass, and the diff only adds test files, so no production route changes. There are no conflicts, and nothing else needs to happen before merge.

Not blocking, and you can take or leave these: the proxy test mocks writeCommandOutput, so it never runs the real JSON and human output route, and you could also assert render() against renderProxyStartup(data). The auth test lives in src/__tests__/ while the connection and proxy tests live in src/cli/commands/__tests__/, so you could move it to src/cli/commands/__tests__/auth.test.ts or keep the cli-* pattern from #3420 for all three. The client: {} as never casts feed an input the CLI cannot produce, and the reserved-port test has a small rebind race on a busy host. A typed client stub would fix the casts, but the current approach is acceptable.

The two cubic-dev-ai threads no longer apply, so you can resolve them. The proxy flags test now asserts the exact listen args and the URL port suffix (#3437 (comment)). The bound-lease test now has the doesNotMatch check for the Metro message (#3437 (comment)).

I did not run the tests or the coverage tool, so I have not reproduced the CRAP score drops or the mutation-check claim. I also did not check how long the login test takes, since auth-session sleeps between polls. The first poll returns approved, and CI is green.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 11, 2026
Move the auth command test beside connection and proxy, assert the
command's render closure against the exported renderProxyStartup
payload, and replace the proxy test's blind client cast with a typed
stub that throws on any access.
@thymikee

Copy link
Copy Markdown
Member Author

Review items addressed in af9efc5:

  1. Cubic threads — already resolved (both discussion_r4242129409 and r4242129414 show resolved=true with the fixing commit cited).
  2. Auth test layout — taken: moved to src/cli/commands/__tests__/auth.test.ts (rename at 96% similarity), so auth/connection/proxy share one layout.
  3. render() vs renderProxyStartup(data) — taken: the supplied-token case now asserts the emitted render closure equals renderProxyStartup(data as unknown as Parameters<typeof renderProxyStartup>[0]), no unmocking needed (the payload is read from the mocked writeCommandOutput call, and the renderer's color decision goes through supportsColor() as before).
  4. Typed client stub — taken in the proxy test: one module-level Proxy({} as AgentDeviceClient) whose get throws, used at all three call sites; it also proves the command never dereferences the client. Left the {} as never casts in daemon.test.ts alone — that file predates this PR and is outside the CRAP: CLI auth, connection, proxy and replay-test formatting #3420 owned-file scope.

Reserved-port rebind race left as agreed. Still test-only: 4 files, 575 gross lines. pnpm check:affected --run passes on af9efc5 under the machine lock; partial-map CRAP re-check still shows no function at or above 30.

@thymikee
thymikee merged commit ed516ac into main Oct 11, 2026
18 checks passed
@thymikee
thymikee deleted the crap/3420-cli-commands branch October 11, 2026 17:29
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-11 17:29 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CRAP: CLI auth, connection, proxy and replay-test formatting

1 participant