test(cli-app): close the branch-coverage gap and add the package to CI

[aryanku-dev]

Adds @percy/cli-app to CI. It was the only one of 18 packages missing from both the test.yml and windows.yml matrices, so its 81 specs have never run in CI on any platform.

Adding it as-is would have turned CI red — the package sits at 98.44% branch coverage against the repo's 100% threshold:

maestro-inject.js | 100% stmts | 98.44% branch | uncovered: 157, 273
ERROR: Coverage for branches (98.44%) does not meet global threshold (100%)

The uncovered branch is not the one it looks like

Both lines contain a log?. optional call, which is the obvious suspect. It isn't that. The gap is err.code || err.message, interpolated into the warning at maestro-inject.js:157 and the debug line at :273. Every existing spec throws an error carrying a code (EACCES, EROFS, EEXIST, ENOENT), so the err.message arm is unreachable.

Worth stating explicitly because it is a trap for the next fallback spec: adding another coded-error case moves no coverage at all.

Two specs throw codeless errors to cover it, and cli-app joins both matrices in the same commit so CI never observes a failing job.

Verification

Run the way these workflows currently do — Node 14:

Executed 83 of 83 specs SUCCESS
All files          | 100 | 100 | 100 | 100
maestro-inject.js  | 100 | 100 | 100 | 100
EXIT=0

Found via, but deliberately separate from, the Node 20 work

Surfaced while auditing package coverage for #2386. It is unrelated to that migration — the gap is version-agnostic and would fail identically on any Node — so it is kept off that branch rather than widening a release-bound PR.

One note for reviewers of #2386: running this same suite on master + Node 20 reports All files | 0 | 0 | 0 | 0 and still exits 0. That is the vacuous-coverage failure mode #2386 fixes, reproduced here incidentally. It is why the verification above was run on Node 14.

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code PR Review

PR: #2402 • Head: fd29784 • Reviewers: stack-code-reviewer

Summary

Adds @percy/cli-app to the Linux (test.yml) and Windows (windows.yml) CI test matrices, and adds two specs to packages/cli-app/test/exec.test.js covering the err.code || err.message fallback arms in maestro-inject.js — the branch-coverage gap that kept the package out of the coverage-gated matrix. Test-and-CI only; no production code changes.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials introduced; diff is specs + two matrix entries.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	N/A	No user input handling introduced.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Both specs verified to reach their intended err.code || err.message arms (maestro-inject.js:157 and :273).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The specs assert on the warn/debug payload rather than only that it was called.
High	Correctness	No race conditions or concurrency issues	N/A	Synchronous spec additions.
Medium	Testing	New code has corresponding tests	Pass	The change is test coverage; 81/81 specs pass locally.
Medium	Testing	Error paths and edge cases tested	Pass	Precisely the intent — the codeless-error arms were previously unreachable.
Medium	Testing	Existing tests still pass (no regressions)	Pass	All 49 checks green on PR CI, including Test @percy/cli-app on Linux and Windows.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the sibling EACCES/EROFS/EEXIST specs and the file's ctxFor + jasmine.createSpy idiom.
Medium	Quality	Changes are focused (single concern)	Pass	One concern: close the gap, then enable the gate.
Low	Quality	Meaningful names, no dead code	Pass	Spec names state the condition under test.
Low	Quality	Comments explain why, not what	Pass	Both specs explain why the arm was unreachable, which is the useful half.
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

• File: packages/cli-app/src/maestro-inject.js:87 (also :126, :298)

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: The injection helpers gate on path.basename(args[0]) !== 'maestro'. On Windows a real Maestro invocation may resolve to maestro.exe, maestro.cmd or maestro.bat, whose basename is not the literal maestro, so all three helpers would silently no-op. Pre-existing production code, untouched by this diff — but this PR is what starts exercising the package on the Windows matrix, so it becomes newly relevant.

• Suggestion: Strip a known executable extension before comparing (e.g. compare path.basename(args[0], path.extname(args[0]))), or match case-insensitively against maestro(\.(exe|cmd|bat))?$. Worth a follow-up ticket rather than expanding this PR.

• File: packages/cli-app/test/exec.test.js:35

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: The pre-existing spec degrades unrecognized exec options into the command (loose parsing trade-off) was observed timing out (10s Jasmine limit) on one local run under coverage instrumentation, with a toBeRejectedWithError assertion firing after the spec had already been marked failed. Untouched by this diff, and the workflows' spec-level retry (PER-9011) is designed to absorb exactly this — but the package is now gated in CI, so latent timing flakiness has somewhere to bite.

• Suggestion: No action required for this PR. If it recurs on CI, raise the spec's timeout or make the assertion await the rejection deterministically.

Dismissed after verification

• packages/cli-app/src/maestro-inject.js:151-154 High — untested nested catch will fail the 100% lines/statements gate this PR enables Dismissed — not a coverage gap. Refuted on two independent grounds:

  1. Mechanism: that catch (_) body contains only comments — no statements, no functions — so it contributes nothing to statements/lines, and Istanbul does not instrument try/catch as a branch. The surrounding lines (const fallback = …, fs.mkdirSync(fallback, …), resolved = fallback) sit in the outer catch, which the existing EACCES/EROFS/EEXIST specs already exercise.
  2. Evidence: Test @percy/cli-app passes on this PR's own CI, on both the Linux and Windows matrices — the very job this PR adds and the finding predicted would "fail outright". All 49 checks are green.

  Recorded here rather than dropped, since it was the reviewer's gating finding. The reviewer flagged that its local nyc run collected no coverage data (All files 0 0 0 0), so the claim rested on static analysis; I reproduced that same empty-data condition locally, which is why CI is the authority here.

---

Verdict: PASS — test-and-CI-only change, correctly targeted and green on CI; the two Low items are pre-existing and out of scope for this diff.