fix: surface server error body & cap scenarios

[SavioBS629]

No description provided.

[SavioBS629]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[SavioBS629]

[High] Scenarios beyond this cap are silently discarded

When the backend returns more than 10 distinct scenarios, canAcceptScenario rejects every new scenario id once the map holds 10 — those scenarios and all their test cases are dropped (scenario branch never creates the entry; testcase branch skips fetchTestCaseDetails). bulkCreateTestCases then reports "…in N of <total>" where <total> is the capped count, so a 14-scenario document reports "10 of 10" — the user never learns 4 were dropped.

The real backend constraint is 10 IDs per fetch-details request, already handled correctly by chunkArray(ids, TC_DETAILS_MAX_BATCH). This separate per-document scenario cap looks reverse-engineered from a constant that doesn't exist on the backend.

Suggestion: Drop MAX_SCENARIOS_PER_DOCUMENT capping entirely. If a safety valve is genuinely wanted, track dropped scenario ids and surface them in the result string, and justify the value against the backend.

Reviewer: stack:code-review

[SavioBS629]

[Low] Dead results map

results[id] = resp.data is written but results is never read or returned (bulkCreateTestCases returns resultString). So the "only last batch's resp.data kept" concern is not a regression — the value was already unused — but the write is now misleading dead code.

Suggestion: Remove the results variable entirely.

Reviewer: stack:code-review

[SavioBS629]

Claude Code PR Review

PR: #324 • Head: 5f37f10 • Reviewers: stack:code-review

Summary

PMAA-147 hardens the Test Case Generator (TCG) flow: batches test-case-detail fetches and bulk-create into chunks of ≤10, caps scenarios per document at 10, adds an 8-minute poll deadline, rewrites setInterval polling as recursive setTimeout with a stopped guard, tolerates non-2xx polls (raise_error:false) instead of throwing, uses Promise.allSettled for detail fetches, tracks per-batch failures in bulkCreateTestCases, and enriches thrown API errors with the server response body. Adds three test files (203 tests pass locally).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Auth via getBrowserStackAuth(config)
High	Security	Authentication/authorization checks present	Pass	Unchanged auth path
High	Security	Input validation and sanitization	Pass	chunkArray guards size<1
High	Security	No IDOR — resource ownership validated	N/A	Scoped to caller's config/project
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Fail	Scenarios beyond cap of 10 are silently discarded (see Findings)
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Graceful-stop paths documented; rejections logged
High	Correctness	No race conditions or concurrency issues	Pass	Recursive setTimeout fixes prior setInterval overlap; no hang/double-settle
Medium	Testing	New code has corresponding tests	Pass	chunking, partial-failure, error-body shapes covered
Medium	Testing	Error paths and edge cases tested	Fail	No test pins the >10-scenario silent-drop behavior
Medium	Testing	Existing tests still pass (no regressions)	Pass	203 tests pass
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Polling bounded by deadline + chunked fetches
Medium	Performance	Long-running tasks use background jobs	N/A	Polling bounded to 8 min
Medium	Quality	Follows existing codebase patterns	Pass	Consistent with TCG utils
Medium	Quality	Changes are focused (single concern)	Pass	All within PMAA-147 hardening
Low	Quality	Meaningful names, no dead code	Fail	results map written but never read
Low	Quality	Comments explain why, not what	Pass	Good rationale comments on poll/raise_error
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: src/tools/testmanagement-utils/TCG-utils/config.ts:6 (used at api.ts:250, api.ts:279, helpers.ts:32)

• Severity: High

• Reviewer: stack:code-review

• Issue: When the backend returns more than MAX_SCENARIOS_PER_DOCUMENT (10) distinct scenarios, canAcceptScenario returns false for any new scenario id once the map holds 10 — those scenarios and all their test cases are silently dropped (the "scenario" branch never creates the entry; the "testcase" branch skips the whole block, so fetchTestCaseDetails is never called). The final resultString reports "…in N of <total>" where <total> is the capped Object.keys(scenariosMap).length, so a document that produced 14 scenarios reports "10 of 10" and the user gets no signal that 4 were dropped. The real backend constraint is 10 IDs per fetch-details request (already correctly handled by chunkArray(ids, TC_DETAILS_MAX_BATCH)); the separate per-document scenario cap appears reverse-engineered from a constant that does not actually exist on the backend.

• Suggestion: Preferred — drop MAX_SCENARIOS_PER_DOCUMENT capping entirely, since the genuine limit (per-fetch ID count) is already solved by chunking. If a safety-valve cap is genuinely wanted, track dropped scenario ids and surface them in resultString, and justify the cap value against the backend rather than 10.

• File: src/tools/testmanagement-utils/TCG-utils/api.ts:426

• Severity: Low

• Reviewer: stack:code-review

• Issue: results[id] = resp.data is written but results is never read or returned (bulkCreateTestCases returns resultString). The "only last batch's resp.data kept" concern is therefore not a regression — the value was already unused — but the write is now misleading dead code.

• Suggestion: Remove the results variable entirely.

• File: src/tools/testmanagement-utils/TCG-utils/api.ts:442 (failure accounting)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: If a scenario has 2 batches and only the first succeeds, it is both pushed to failedScenarios AND counted in doneCount (because createdInScenario > 0). The message then counts the same scenario on both sides ("created in N of M … Failed to create for 1 scenario").

• Suggestion: Word it as "partially failed", or only increment doneCount when !scenarioFailed.

• File: src/lib/apiClient.ts:157

• Severity: Low

• Reviewer: stack:code-review

• Issue: JSON.stringify(body) (reached when body is a non-string object lacking .message/.error) would throw on a circular body and mask the original error; a very large body is inlined verbatim into error.message. Mutating error.message itself is safe — the error is local to this call, not shared. No credential-leak risk (body is the server's own error response).

• Suggestion: Wrap the stringify in try/catch (fallback String(body)) and consider truncating to ~500 chars.

• File: src/tools/testmanagement-utils/TCG-utils/api.ts:213 (shared deadline)

• Severity: Low

• Reviewer: stack:code-review

• Issue: deadline is computed once in pollScenariosTestDetails and passed to every pollTestCaseDetails. A detail fetch kicked off near the 8-minute mark inherits a near-expired deadline and may return an empty detailMap, degrading to "testcase without steps" rather than erroring. Acceptable, but easy to mistake for a bug.

• Suggestion: Add a one-line comment documenting the shared-deadline behavior.

---

Verdict: FAIL — the polling rewrite, instrumentation, auth, and multi-tenant rules are clean and the suite passes, but scenarios beyond the cap of 10 are silently discarded with the user told "N of N", a data-loss-without-surfacing path (High). Resolve that (most likely by removing the cap) before merge.