Feat/interactive wizard#228
Conversation
WalkthroughChanges to src/commands/test.ts add an interactive plan-path prompt (via new src/lib/prompt.ts helper ChangesTest CLI UX changes
Update-check debug logging
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant runCreateFromPlan
participant promptForPlanPath
participant promptText
User->>runCreateFromPlan: test create (no --plan-from, non-JSON, non-dry-run)
runCreateFromPlan->>promptForPlanPath: request plan path
promptForPlanPath->>promptForPlanPath: check TTY status and CI env var
promptForPlanPath->>promptText: prompt user for input
promptText-->>promptForPlanPath: raw answer
promptForPlanPath-->>runCreateFromPlan: trimmed path or fallback ("plan.json")
runCreateFromPlan->>runCreateFromPlan: requireNonEmpty('plan-from', ...)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/lib/update-check.ts (2)
137-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated debug-log-on-catch pattern.
The
isDebugLoggingEnabled(...) → resolved.stderr(...)block is repeated verbatim (only the prefix string differs) in bothreadUpdateCheckCacheandwriteUpdateCheckCache. Consider extracting a small helper (e.g.,logDebug(resolved, label, err)) to avoid the duplication as more call sites are added.♻️ Suggested helper
+function logDebugError(resolved: ResolvedUpdateCheckDeps, label: string, err: unknown): void { + if (isDebugLoggingEnabled(resolved.argv)) { + resolved.stderr(`[debug] ${label}: ${err instanceof Error ? err.message : String(err)}`); + } +}Then in each catch block:
- } catch (err) { - if (isDebugLoggingEnabled(resolved.argv)) { - resolved.stderr(`[debug] readUpdateCheckCache: ${err instanceof Error ? err.message : String(err)}`); - } + } catch (err) { + logDebugError(resolved, 'readUpdateCheckCache', err);Also applies to: 155-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/update-check.ts` around lines 137 - 140, The same debug-on-catch logging pattern is duplicated in readUpdateCheckCache and writeUpdateCheckCache. Extract a small helper near these functions, such as logDebug(resolved, label, err), that checks isDebugLoggingEnabled(resolved.argv) and writes the formatted message to resolved.stderr. Then replace both catch blocks to call the helper with the appropriate label so the prefix stays specific while the shared logic is centralized.
96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing doc comment on new
argvfield.Every other field in
UpdateCheckDepshas a one-line JSDoc comment describing its purpose (e.g., Line 92/** Sink for the single advisory line. */);argvlacks one, which is a minor inconsistency for an otherwise well-documented public interface.📝 Suggested addition
+ /** Process arguments, used to detect --debug/--verbose for diagnostic logging. */ argv?: string[];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/update-check.ts` at line 96, Add a one-line JSDoc comment for the new argv field in UpdateCheckDeps to match the documentation style used by the other fields in update-check.ts. Locate the interface containing the existing documented members like sink and other deps, and add a concise description of what argv represents so the public API stays consistently documented.src/commands/test.ts (1)
1986-1992: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew interactive branch isn't wired through
depsfor testability.Every other side-effecting call in this file (stderr, sleep, client) goes through the
depsinjection pattern, butpromptForPlanPath()here hitsprocess.stdin/process.stderrdirectly with no way for a caller/test to supply thePromptStreamsit already accepts. As written, this branch has no path to deterministic unit-test coverage (short of mockingprocess.stdin.isTTYglobally), and the duplicate-flags test cited in context usesoutput: 'json'specifically to avoid it.🔧 Suggested direction
- if (!opts.planFrom && !opts.dryRun && opts.output !== 'json') { - const interactivePath = await promptForPlanPath(); + if (!opts.planFrom && !opts.dryRun && opts.output !== 'json') { + const interactivePath = await promptForPlanPath(undefined, undefined, { + input: deps.stdin, + output: deps.stderrStream, + });(requires adding optional
stdin/stderrStreamfields toTestDeps)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/test.ts` around lines 1986 - 1992, The new interactive plan-path branch in test command handling bypasses the existing deps injection pattern, making it hard to test deterministically. Update the test flow around the `promptForPlanPath()` call to source `stdin`/`stderrStream` from `TestDeps` (adding those optional fields if needed) and pass them into `promptForPlanPath` so this side effect is fully injectable like the other dependencies in `src/commands/test.ts`.src/lib/prompt.ts (1)
169-187: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCI-detection only matches literal
'true'/'1'.
process.env.CI !== 'true' && process.env.CI !== '1'will treat any other truthyCIvalue (e.g.'True','yes', or a CI system that just setsCI=<pipeline-id>) as "interactive," letting the prompt block on stdin in an unattended job. Since this is explicitly meant to guard against exactly that per the path instructions ("enable only for real TTY runs (not CI...)"), consider a case-insensitive/truthy check (Boolean(process.env.CI)) instead of an exact-match allowlist.🔧 Proposed fix
const isInteractive = (input as { isTTY?: boolean }).isTTY === true && (output as { isTTY?: boolean }).isTTY === true && - process.env.CI !== 'true' && - process.env.CI !== '1'; + !process.env.CI;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/prompt.ts` around lines 169 - 187, The CI gate in promptForPlanPath only blocks when process.env.CI is exactly 'true' or '1', so other CI values can still be treated as interactive. Update the isInteractive check in promptForPlanPath to use a truthy CI test instead of exact string comparisons, while keeping the existing TTY checks and fallback behavior unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commands/test.ts`:
- Around line 1986-1992: The new interactive plan-path branch in test command
handling bypasses the existing deps injection pattern, making it hard to test
deterministically. Update the test flow around the `promptForPlanPath()` call to
source `stdin`/`stderrStream` from `TestDeps` (adding those optional fields if
needed) and pass them into `promptForPlanPath` so this side effect is fully
injectable like the other dependencies in `src/commands/test.ts`.
In `@src/lib/prompt.ts`:
- Around line 169-187: The CI gate in promptForPlanPath only blocks when
process.env.CI is exactly 'true' or '1', so other CI values can still be treated
as interactive. Update the isInteractive check in promptForPlanPath to use a
truthy CI test instead of exact string comparisons, while keeping the existing
TTY checks and fallback behavior unchanged.
In `@src/lib/update-check.ts`:
- Around line 137-140: The same debug-on-catch logging pattern is duplicated in
readUpdateCheckCache and writeUpdateCheckCache. Extract a small helper near
these functions, such as logDebug(resolved, label, err), that checks
isDebugLoggingEnabled(resolved.argv) and writes the formatted message to
resolved.stderr. Then replace both catch blocks to call the helper with the
appropriate label so the prefix stays specific while the shared logic is
centralized.
- Line 96: Add a one-line JSDoc comment for the new argv field in
UpdateCheckDeps to match the documentation style used by the other fields in
update-check.ts. Locate the interface containing the existing documented members
like sink and other deps, and add a concise description of what argv represents
so the public API stays consistently documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dcf9e9ca-95e1-4fc5-a6e2-c14e66340a5c
📒 Files selected for processing (3)
src/commands/test.tssrc/lib/prompt.tssrc/lib/update-check.ts
1 participant
What does this PR do?
This PR enhances the CLI user experience by introducing an interactive wizard. If a user runs a test command without specifying the required
--plan-frompath, instead of throwing a hard error, the CLI now gracefully prompts the user to enter the path using Node's nativereadline.🖥️ CLI Experience (Before vs. After)
❌ Before:
$ testsprite test run Error: Missing required argument: plan-from✅ After:
$ testsprite test run Enter path to plan JSON file [default: plan.json]: _Key Highlights:
--dry-runand--output jsonto ensure automated pipelines remain unaffected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Chores