fix: fail-closed LCA polling and stop leaking API errors

[SavioBS629]

No description provided.

[SavioBS629]

Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[SavioBS629]

Claude Code PR Review

PR: #343 • Head: 1baab11 • Reviewers: stack:code-review

Summary

Makes LCA polling fail-closed (stops after 3 consecutive poll errors, cleaning up timers and resolving null) and stops leaking raw API error details to the client from createLCASteps (returns a generic message; logs the detail server-side).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None
High	Security	Authentication/authorization checks present	N/A	Auth unchanged (getBrowserStackAuth)
High	Security	Input validation and sanitization	Pass	—
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	Counter resets on healthy poll, stops at 3 consecutive errors, clears both timers; resolve(null) handled by caller's if (lcaResult && …) else-branch
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Errors logged then handled; fail-closed path is explicit
High	Correctness	No race conditions or concurrency issues	Pass	Counters are function-scoped (per invocation) — no shared/global state; multi-tenant safe
Medium	Testing	New code has corresponding tests	Fail (non-blocking)	LCA tests mock createLCASteps/pollLCAStatus entirely; the new generic message and fail-closed stop are uncovered
Medium	Testing	Error paths and edge cases tested	Fail (non-blocking)	No test drives 3-consecutive-errors → stop, nor the util's new generic-message path
Medium	Testing	Existing tests still pass (no regressions)	Pass	Tests at testmanagement.test.ts:685/694 assert the wrapper message (unchanged) — still pass
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Fail-closed actually bounds previously-unbounded error retries
Medium	Performance	Long-running tasks use background jobs	N/A	Polling model unchanged
Medium	Quality	Follows existing codebase patterns	Partial	console.error used instead of the pino logger other testmanagement wrappers use (see finding)
Medium	Quality	Changes are focused (single concern)	Pass	Two related concerns: fail-closed + stop leaking; matches title
Low	Quality	Meaningful names, no dead code	Pass	detail, consecutiveErrors, maxConsecutiveErrors are clear
Low	Quality	Comments explain why, not what	Pass	Removed a now-inaccurate "continue polling" comment
Low	Quality	No unnecessary dependencies added	Pass	None

Findings

1. console.error instead of the project logger (Low, Quality)

• File: src/tools/testmanagement-utils/create-lca-steps.ts:201
• Severity: Low
• Reviewer: stack:code-review
• Issue: The new server-side log uses console.error, while sibling testmanagement wrappers use the pino logger (import logger from "../../logger.js"; e.g. create-testcase.ts). It is operationally safe here — console.error writes to stderr, so it does not corrupt the stdio JSON-RPC stream — but it is inconsistent with the codebase convention. (Two other console.error calls in these files are pre-existing.)
• Suggestion: import logger from "../../logger.js" and use logger.error("Error creating LCA steps: %s", detail).

2. Test coverage for the new behavior (Medium, Testing — non-blocking)

• File: tests/tools/testmanagement.test.ts
• Issue: The LCA suite mocks createLCASteps and pollLCAStatus, so neither the util's new generic error message nor the fail-closed "3 consecutive errors → stop, resolve null" path is exercised. For a security-relevant fix this leaves the changed behavior unverified.
• Suggestion: Add a unit test on pollLCAStatus that makes apiClient.get reject 3× and asserts polling stops (timers cleared, resolves null, "stopped after 3 consecutive errors" notification sent); and one asserting a healthy poll resets the counter. Optionally a createLCASteps test asserting the generic message on an internal API error.

Raised by other reviewers (not independently confirmed)

None.

Informational (out of PR scope)

• The unchanged wrapper createLCAStepsTool (testmanagement.ts:~505) still returns Failed to create LCA steps: ${err.message}… on thrown errors — a residual, lower-sensitivity leak. In practice createLCASteps catches internally and returns rather than throws, so the primary path is fixed; this catch fires only for unexpected throws.
• The pre-existing poll retry notification (errors 1–2, poll-lca-status.ts) still includes error.message in the client progress message. Pre-existing; adjacent to the PR's goal — worth a follow-up if full non-leakage is desired.

---

Verdict: PASS — correct fail-closed logic and a real reduction in error leakage; only Low/Medium non-blocking follow-ups (logger consistency, test coverage, residual wrapper leak).