test(cli): cover unknown-profile diagnostics and clarify help - #306
Conversation
Claude Code independent reviewReviewer model: claude-opus-5 (modelUsage asserted); PR head: 054bd45; main: 93b65ee; reviewed result: 054bd45; invoked approximately 2026-09-10T14:27Z. Review — PR #306, head SHA
|
| Check | Exit | Notes |
|---|---|---|
pnpm build |
0 | |
pnpm typecheck |
0 | |
pnpm test |
0 | 252 files, 3840 passed, 4 skipped |
pnpm test:preflight fails on the default TMPDIR here (/tmp/.git sandbox marker) — an environment fact, not a PR defect. I re-ran the suite under an isolated TMPDIR outside Git ancestry, which I created and removed myself.
CI on the exact head SHA — all four green: test (ubuntu-latest) success, test (macos-latest) success, windows-sandbox-none success, scripted-structure success. (Windows was still in_progress when I started; I polled to completion.)
Findings
MEDIUM — packages/cli/src/program.ts:650 (and :184): doctor's --profile help promises a listing doctor never produces
doctor's option now reads "…to diagnose; unknown names list available profiles (names only)", but the doctor action never reaches the throwing resolver. doctor.ts:386-391 pre-checks the profile, sets configInvalid = true, and doctor.ts:399 skips the resolveConfig call at :403 that produces the listing.
Verified against the built CLI, user config holding profiles alpha and fast:
run --profile typo → unknown config profile "typo"; available profiles: alpha, fast
doctor --profile typo → fail config:profile — active profile "typo" does not exist —
add it under profiles or remove --profile "typo"
Same environment, same flag, and the two names exist — doctor lists nothing. The root-level --profile (:184) carries the identical clause and PROFILE_AWARE (:186) routes doctor through the same path, so agentrig --profile typo doctor is false too. Failure scenario: a user with a typo'd profile reads the help, runs doctor (the diagnostic command — the natural choice), gets no candidate names, and has to open the config by hand.
Fix (preferred): make the behavior match the promise. doctor.ts:390 already has user and project in scope, so build the same sorted-unique list and append it:
const available = [...new Set([...Object.keys(user?.profiles ?? {}), ...Object.keys(project?.profiles ?? {})])].sort();
checks.push(line("fail", "config:profile", `active profile ${display(profile)} does not exist — add it under profiles or remove --profile ${display(profile)}; available profiles: ${available.length === 0 ? "(none)" : available.join(", ")}`));doctor.test.ts:235 uses toContain, so it keeps passing. Cheaper alternative: drop the clause from :650 and exempt doctor in the help test.
LOW — packages/cli/test/config.test.ts:492: the help test asserts text, not behavior, and locks the false claim in
check() walks every command and asserts each --profile description contains the phrase. It can never detect a command whose runtime diverges — and by requiring the phrase on every option it is precisely what makes the doctor claim mandatory. Failure scenario: exactly the one above; the test is green while the help is wrong. Fix: pair it with a behavioral assertion — drive doctor and one configured() command through an unknown profile and assert the emitted diagnostic contains the available names, rather than asserting the help string alone.
LOW — packages/cli/src/config.ts:371: --profile recommended is exempt from the whole diagnostic (pre-existing)
When neither layer defines a recommended profile, :371 omits profile from the resolveConfig call, so an unknown name silently succeeds. Verified: run --profile recommended against a config with only alpha/fast proceeds straight to the credential check, no error and no listing — contradicting the blanket clause the PR just added to :184/:216. Related pre-existing inconsistency: doctor --profile recommended reports fail config:profile … does not exist and then precedence cannot be trusted, for a config run accepts. Fix: out of scope to change the behavior here, but the help clause should not read as unconditional, and recommended deserves a word as a built-in.
LOW — packages/cli/src/config.ts:278: available names interpolated unescaped (pre-existing, now pinned)
The requested name is JSON.stringify'd; the available names go through raw names.join(", "). Profile keys are unconstrained (z.record(ConfigValuesSchema), :158). A trusted project config with a profile named with embedded newlines/ANSI can shape that stderr line. Trust-gated, so LOW — but this PR adds a test that pins the format, so it is the moment to fix it. Fix: names.map(n => JSON.stringify(n)).join(", "), updating the two new expectations and the regex at :172.
§5 Mutation probes — 4 run, each restored before the next, none overlapped
| # | Mutant | Result |
|---|---|---|
| 1 | config.ts:275 drop new Set() dedup |
killed, exit 1 — new test only (alpha, shared, shared, zebra) |
| 2 | config.ts:275 drop .sort() |
killed, exit 1 — 2 tests (zebra, shared, alpha) |
| 3 | program.ts:342 drop process.exitCode = 1 in configured()'s catch |
killed, exit 1 — both new CLI cases |
| 4 | program.ts:650 revert doctor help clause |
killed, exit 1 — 1 failed / 63 passed |
Mutants 1 and 4 re-run the PR's own claims (§5 requirement). Mutant 4 reproduces the stated fail-first result exactly (1 failed, 63 passed). No mutant survived. The pre-existing regex test at :172 survived mutant 1 alone — the new exact-Error assertion is what kills it, so the added coverage is real, not redundant.
Restored state: both files back to recorded SHA-256 (fa8d1398… config.ts, c1152581… program.ts); HEAD still 054bd45…; git status --porcelain and git diff HEAD both empty; no vitest/node children left; my temp dirs removed and the shared temp dir left with only its pre-existing entries. No sibling or author tree touched, nothing pushed, committed, or merged.
PR body claims
Verified: resolver was already implemented (git show origin/main:packages/cli/src/config.ts is byte-identical); no runtime resolver behavior changed (diff is help strings + tests + STATUS only); build/typecheck exit 0; pnpm test exit 0 with 252 files / 3840 passed / 4 skips — matching the stated numbers exactly; fail-first help regression; dedup mutant; ROADMAP untouched, no row advanced.
Not verified: the "expose full config values" and "omit (none)" mutants — I substituted .sort() and the CLI exit-status mutant to widen coverage rather than repeat all three. The builder session id 023c3a5d is not checkable from here.
Contract fidelity: no docs/ROADMAP.md edit, no docs/plans/R1b.md exists, no new event type, no memory/supervisor import changes, CLI stays thin, raw/ untouched. ## Deviations: None holds — no unapproved deviation found.
The human authorization quoted in the PR body is the author's transcription; I make no merge decision and took no landing action.
Codex independent reviewPR head: 054bd45; main: 93b65ee; reviewed result: 054bd45; invoked approximately 2026-09-10T14:27Z. Codex session 01a08bb7-13b2-76e3-bba2-f365c1ea9ae6. The changes update profile help text and add regression coverage without modifying configuration behavior. No actionable defects were found; local test validation was blocked by fixture-directory trust detection affecting both existing and new tests. |
Claude Code delta reviewModel claude-opus-5 asserted from modelUsage. OLD 054bd45; PR head/reviewed result c6480c3; main 93b65ee; invoked 2026-09-10T14:52Z. Delta review — PR #306, head
|
| Check | Exit | Notes |
|---|---|---|
pnpm build |
0 | |
pnpm typecheck |
0 | |
pnpm test |
0 | 252 files, 3846 passed, 4 skipped — matches the PR body exactly |
test:preflight fails on the default TMPDIR (/tmp/.git marker), an environment fact. I ran the suite under an isolated TMPDIR outside Git ancestry (/home/amit/agentrig-review306-delta-tmp), created and removed by me — the documented fixture remedy, no check disabled.
CI on the exact head SHA: test (ubuntu-latest), test (macos-latest), windows-sandbox-none, scripted-structure — all success.
Prior findings (comment 5620425662)
MEDIUM — doctor's help promised a listing doctor never produced → CLOSED. doctor.ts:390-391 now builds the same sorted-unique set and appends it. Verified against the built CLI with user profiles alpha/fast/a hostile name:
doctor --profile typo → fail config:profile — active profile "typo" does not exist — add it
under profiles or remove --profile "typo"; available profiles: "alpha", …, "fast"
--profile typo doctor → identical (root flag path)
LOW — help test asserted text, not behavior → CLOSED. doctor.test.ts:238-258 now drives buildProgram().parseAsync for both flag placements and asserts the exact emitted line, exitCode === 1, and absence of config values; config.test.ts:509 still covers a configured() command. The behavioral pairing the finding asked for exists.
LOW — recommended exemption → fixed as specified, with a residual (below). Help is no longer unconditional and names the built-in.
LOW — raw available names → CLOSED as specified, with a residual (below). config.ts:278 uses names.map(name => JSON.stringify(name)); doctor uses display. Newlines, quotes and ANSI ESC are neutralized on both paths (verified end-to-end: "ev\u001b[31mil\nsecond…").
Residual findings
LOW — packages/cli/src/config.ts:278: bidi overrides still reach stderr raw on the resolver path
JSON.stringify escapes control chars below 0x20 but leaves U+202A–202E / U+2066–2069 literal. The same PR's doctor path uses display (doctor.ts:85), which explicitly escapes them — "so terminal controls and bidi marks are never executable output". Same data, two different guarantees. Verified on the built CLI with a profile named ev<ESC>[31mil\nsecond<U+202E>line:
run --profile typo→… "ev\u001b[31mil\nsecondline", "fast"(rawU+202E, byte-confirmed)doctor --profile typo→… "ev\u001b[31mil\nsecond\u202eline", "fast"(escaped)
Scenario: a user trusts a repo, whose .agentrig/config.json names a profile with an embedded U+202E; a later --profile <typo> prints a list whose tail renders right-to-left, letting the repo shape what the operator believes their available profiles are. Trust-gated, hence LOW.
Fix: export doctor's display (or its bidi replace) and use it at config.ts:276-279 for both the requested and the available names.
LOW — packages/cli/test/config.test.ts:185, packages/cli/test/doctor.test.ts:249: escaping tests compute the expectation with the SUT's own primitive
Both new tests build the expected string from JSON.stringify(name), so they pin "output equals JSON.stringify" rather than "output carries no raw terminal-control codepoints" — which is exactly why the bidi gap above survives a green suite. Not vacuous (both die under mutation), just blind to the class they exist to guard.
Fix: add expect(message).not.toMatch(/[\u0000-\u001f\u202a-\u202e\u2066-\u2069]/) alongside the equality assertion, and include a bidi character in the fixture name.
LOW — packages/cli/src/program.ts:380 and :544: the new recommended clause is inaccurate for review and mcp login
Both carry "run commands also accept built-in recommended; other unknown names list available profiles (names only)", but config.ts:371 exempts recommended for every configured() command, not just the seven at :353. Verified: review --profile recommended prints note: recommended run defaults do not apply to 'review'; using this command's baseline config and proceeds — no error, no listing — while review --profile typo does list. mcp login shares that path. A review user reading its own --help concludes recommended is rejected there; it is accepted silently.
Fix (wording only): on those two options, "built-in recommended is accepted with a note and no run defaults; other unknown names list available profiles (names only)".
§5 Mutation probes — 7 run, sequential, each restored before the next
| # | Mutant | Test file | Result |
|---|---|---|---|
| 1 | doctor.ts:391 drop the appended available-name guidance |
doctor | killed, exit 1 (5 failed / 46 passed) |
| 2 | doctor.ts:390 names.map(display) → raw names |
doctor | killed, exit 1 (2 failed) |
| 3 | doctor.ts:390 append each profile's config values |
doctor | killed, exit 1 — leaked private-model; the not.toContain("private-") guard is load-bearing |
| 4 | config.ts:278 names.map(JSON.stringify) → raw names |
config | killed, exit 1 (5 failed / 60 passed) |
| 5 | program.ts:650 revert doctor's help clause |
config | killed, exit 1 (1 failed / 64 passed) |
| 6 | program.ts:184 revert the root recommended clause |
config | killed, exit 1 (1 failed / 64 passed) |
| 7 | doctor.ts:390 drop new Set() dedup |
doctor | killed, exit 1 (2 failed) |
Four of the PR's five claimed mutants re-run independently (raw resolver names, raw doctor names, duplicate doctor names, omitted doctor guidance), plus value-leakage and two help mutants. No survivors.
PR body claims
Verified: all four prior findings addressed; five mutation claims (four re-run directly, the fifth — value leakage — reproduced by my own variant); build/test/typecheck each exit 0 with the stated 252/3846/4 counts; exact-head CI green on all three platforms plus scripted-structure; no roadmap row advanced (docs/ROADMAP.md untouched in the delta); no event-type, memory/supervisor import, or raw/ changes; CLI stays thin (logic sits in config.ts/doctor.ts, not the command wiring); docs/STATUS.md updated truthfully.
Not verified: the fixer session id 69654553 and the fail-first count of 10 (the pre-fix tree is not reconstructable here without rewriting the branch); the author's transcription of human merge authorization.
Restored state
All three touched sources verify against their pre-mutation SHA-256 (sha256sum -c OK ×3); git status --porcelain empty; HEAD still c6480c3…. No vitest/node children of mine remain (the one live packages/cli/dist/index.js process belongs to /home/amit/agentrig, the parent checkout — not spawned or touched by me). My TMPDIR was removed; /home/amit/agentrig-fix306-tmp (author's) untouched. Nothing pushed, committed, merged, or approved; no permissions changed, no children or auxiliary models spawned. Worktree cleanup left to the conductor.
Codex delta reviewOLD 054bd45; PR head/reviewed result c6480c3; main 93b65ee; invoked 2026-09-10T14:52Z. No actionable regressions were identified. Doctor tests passed; config test failures matched the documented sandbox temporary-directory trust-boundary limitation rather than the changed behavior. |
Bounded repair round 2 — all three residuals closedReviewed full Claude delta comment #306 (comment).
Fail-first exit 1: two resolver safety failures and help failure. Restored focused config/doctor/recommended suite exit 0, 142 tests. Five meaningful mutations each exited 1 and were restored: requested escaping, available escaping, doctor bidi escaping, review help, login help. Full restored trio: Old: |
|
Round-two final-head CI verified green on |
Claude Code final delta reviewModel claude-opus-5 asserted from modelUsage. OLD c6480c3; PR head/reviewed result 8a7c1dd; main 93b65ee; invoked 2026-09-10T15:15Z. Delta review — PR #306, reviewed SHA
|
| Check | Exit | Notes |
|---|---|---|
pnpm build |
0 | |
pnpm typecheck |
0 | |
pnpm test |
0 | 252 files, 3847 passed / 4 skipped — exactly the STATUS.md claim |
test:preflight rejects the default TMPDIR (/tmp/.git marker). Ran under a fresh isolated TMPDIR outside Git ancestry (/home/amit/agentrig-review306-r2-tmp, git rev-parse --show-toplevel → fatal/not-a-repo), created and removed by me. Documented remedy; no check disabled.
CI on the exact head: test (ubuntu-latest), test (macos-latest), windows-sandbox-none, scripted-structure — all success.
Residuals — each confirmed closed
LOW #1 — bidi overrides reached stderr raw on the resolver path → CLOSED. config.ts:268-273 adds displayProfileName, byte-identical in logic to doctor's display (doctor.ts:85-89), applied to both the requested name and each available name at :285. Verified end-to-end on the built CLI with a user profile named ev<ESC>[31mil\nsecond<U+202E>line<U+2066>iso written as raw UTF-8 bytes (e2 80 ae, e2 81 a6 confirmed via od -c):
run --profile typo→… "ev\u001b[31mil\nsecond\u202eline\u2066iso", "fast"— codepoint scan: no raw C0/bidireview --profile typo→ identicaldoctor --profile typo→ identical string
The two paths now emit the same bytes for the same hostile name. Grep confirms no other profile-name emission path in config.ts (:396 interpolates only fixed text and the enum-validated resolved.sandbox). The .replace callback form means $-patterns are inert, and a name containing literal ASCII \u202e stringifies to \\u202e — unambiguous against a real override.
LOW #2 — escaping tests computed the expectation with the SUT's own primitive → CLOSED. config.test.ts:185-195 and doctor.test.ts:240-254 now use hardcoded literal expected strings (escaped / escapedName / escapedRequested) instead of JSON.stringify(name), add class-level expect(...).not.toMatch(/[\u0000-\u001f\u202a-\u202e\u2066-\u2069]/u), and widen the fixtures to the full bidi range (\u202a–\u202e, \u2066–\u2069). Non-vacuous: if resolveConfig failed to throw, message stays "" and the toBe assertion fails.
LOW #3 — recommended clause inaccurate for review and mcp login → CLOSED. Both program.ts:380 and :544 now read "built-in recommended is accepted with a note and no run defaults". Verified against the built CLI:
review --profile recommended→note: recommended run defaults do not apply to \review`; using this command's baseline config`, then proceedsmcp login --profile recommended→ same note naminglogin, then proceedsreview --profile typo→ still lists available names--helpfor both renders the new wording
I enumerated every --profile declaration to check the fix is complete: withRunOptions (:216) reaches only run/acp/web/mcp-serve/resume/tui, and tick has no own option — all six are in the seven-name recommended list at config.ts:360, so review and mcp login were the only inaccurate sites. Nothing was missed.
§5 Mutation probes — 5 run, strictly sequential, each restored and hash-verified before the next
| # | Mutant | Test file | Result |
|---|---|---|---|
| 1 | config.ts:270 drop the bidi .replace (revert residual #1) |
config | killed, exit 1 (2 failed / 64 passed) |
| 2 | config.ts:270 narrow the set to [\u202e] only |
config | killed, exit 1 (2 failed / 64 passed) |
| 3 | doctor.ts:86 drop the bidi .replace |
doctor | killed, exit 1 (2 failed / 49 passed) |
| 4 | program.ts:380 revert review help wording |
config | killed, exit 1 (1 failed / 65 passed) |
| 5 | program.ts:544 revert mcp login help wording |
config | killed, exit 1 (1 failed / 65 passed) |
Mutants 1–3 were killed by the new not.toMatch guard specifically (failure text: AssertionError: expected … not to match /[\u0000-\u001f\u202a-\u202e\u2066-\u2069]/u) — the residual-#2 fix is what catches the residual-#1 class, which is exactly the coupling the prior review asked for. Mutant 2 confirms the guard is sensitive to a narrowed escape set, not just a removed one. This independently re-runs all five of the PR's claimed mutants. No survivors. Restored suites: config + doctor exit 0, 117 passed.
PR body / STATUS.md claims
Verified: trio exit codes and the exact 252/3847/4 counts; exact-head CI green on all four checks; all five claimed mutants killed (re-run independently); all three residuals closed as specified; docs/ROADMAP.md untouched in the delta, so no ## Deviations/arbiter record is owed; no event-type, memory/supervisor import, or raw/ changes; CLI stays thin (escaping lives in config.ts, not command wiring); nothing pushed or merged by the repair child.
Not verified: the fixer session id 52fd8b99; the "fail-first exited 1" claim (the pre-fix tree isn't reconstructable without rewriting the branch). The "restored focused suite … (142 tests)" figure names no file set — config+doctor is 117 and config+doctor+recommended-defaults+recommended-runtime is 143, so the claim is plausible but not exactly reproducible.
Out of bounded scope — noted, not a finding
The root --profile option (program.ts:184, unchanged) still carries the old "run commands also accept built-in recommended" phrasing, and config.test.ts:513-517's else-branch now pins it. agentrig --profile recommended review takes the same note-and-proceed path, so the root wording has the same imprecision the delta fixed on the two subcommand options. Unchanged line, LOW at most, and the prior review deliberately scoped its fix to :380/:544 — reporting it only so the human has it, not as a residual.
The duplication of doctor's display into config.ts (rather than exporting it, as the prior review's fix suggested) is deliberate and commented; both copies are independently guarded by class-level assertions, so drift is test-detectable.
Restored state
All three touched sources verify against their pre-mutation SHA-256 (sha256sum -c OK ×3); git status --porcelain empty; HEAD still 8a7c1ddc4373ea5f71a265174a050580b2e6ba79. No vitest/node children of mine remain (pgrep clean — one earlier hit was the pgrep shell matching its own pattern, gone on re-check). My TMPDIR was removed; the author's /home/amit/agentrig-fix306-tmp and /home/amit/agentrig-fix306-round2-tmp were never read or written. Nothing pushed, committed, merged, or approved; no permission changes, no children or auxiliary models spawned. Worktree cleanup left to the conductor.
Codex final delta reviewOLD c6480c3; PR head/reviewed result 8a7c1dd; main 93b65ee; invoked 2026-09-10T15:15Z. No actionable regressions were identified. The escaping change matches doctor's existing formatter, and the help text matches current behavior. Test execution encountered fixture trust-boundary failures unrelated to the diff. |
|
Landed via squash as Post-merge verification on that actual main commit:
No linked closing issues and no other open PRs at verification. No branches deleted or review worktrees removed. Land child session: |
1 participant
Contract and final scope
User-directed unknown-profile CLI guidance, bound exclusively to PR #306: requested name and available names without configuration values, resolver regression/help coverage, doctor consistency, and safe profile-name escaping. No roadmap row advanced; no scope expansion.
Resolver diagnostics quote requested and available names and escape terminal/bidi controls. Doctor lists sorted, unique, escaped available names and
(none)for absent/empty maps. Configuration values, precedence, trust boundaries, and built-in recommended semantics remain unchanged. Help distinguishes run defaults, review/login note-and-proceed behavior, and doctor's file-defined profiles. Behavioral regressions cover both flag placements, failure status, names-only output, sorting/deduplication, empty maps, recommended semantics, and literal escaping assertions.Independent review resolution
All concrete findings from original review 5620425662 and delta review 5620769274 are fixed: doctor discovery, behavioral test pairing, recommended help distinctions, raw-name injection, bidi escaping, and independent expected strings. Final reviews on
8a7c1ddc4373ea5f71a265174a050580b2e6ba79report no remaining findings / no actionable regressions:The optional root-help wording observation is outside the bounded task and explicitly not a residual.
Final verification
Independent final review: build and typecheck exit 0; test exit 0, 252 files, 3,847 passed / 4 existing skips; five meaningful mutants killed and restored. Full suite used the documented isolated TMPDIR remedy for the pre-existing /tmp Git fixture boundary; no checks disabled. The other reviewer candidly records its fixture trust-boundary test limitation; its run is not claimed as a passing full suite.
Lander freshly checked current head
8a7c1ddc4373ea5f71a265174a050580b2e6ba79, mergeable/CLEAN, and exact-head success:Builder session
023c3a5d; repair sessions69654553and52fd8b99.Human authorization — task bound to PR #306
Verbatim human authorization handed to the land child for this task's PR #306 (no subsequent revocation):
Landing is restricted to PR #306 and the scope above. Squash merge, verify actual merge-commit main CI, then safely update local main and run pnpm build. No branch deletion or review worktree cleanup.