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]

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.

[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 still silently discarded (unresolved)

Both the scenario and testcase branches gate on canAcceptScenario(scenariosMap, sc.id, MAX_SCENARIOS_PER_DOCUMENT). When the backend returns >10 distinct scenarios, the extras and all their test cases are dropped, and bulkCreateTestCases reports "…in <doneCount> of <total> scenarios" where total is the capped Object.keys(scenariosMap).length — so a 14-scenario doc reports "10 of 10" with no truncation notice.

The real backend constraint is 10 IDs per fetch-details request, already enforced by chunkArray(ids, TC_DETAILS_MAX_BATCH). This separate per-document scenario cap is reverse-engineered from a constant that isn't a real hard backend limit, and it's untested (no test covers the drop path).

Suggestion: Remove MAX_SCENARIOS_PER_DOCUMENT + the canAcceptScenario gating entirely. If a per-document cap is genuinely required, surface it — track the true incoming count and append "Note: N scenarios exceeded the per-document limit and were not created" to the result string, plus a test asserting the >10 case.

Reviewer: stack:code-review

[SavioBS629]

[High] Drop site for the silent scenario cap

This canAcceptScenario(..., MAX_SCENARIOS_PER_DOCUMENT) gate (and the matching one in the scenario branch) silently skips every scenario past the 10th — fetchTestCaseDetails is never called for it and its test cases are never created. Combined with the capped total in the result string, the user gets no signal that data was dropped. See the config.ts comment for the full finding and suggested fix.

Suggestion: Remove the per-document cap (chunking already handles the real per-request limit), or surface dropped scenarios in the result string.

Reviewer: stack:code-review

[SavioBS629]

Claude Code PR Review

PR: #324 • Head: 039dbd0 • Reviewers: stack:code-review

Summary

Re-review of PMAA-147 TCG hardening (batch ≤10, deadline-bounded polling rewritten to recursive setTimeout, non-2xx tolerated, Promise.allSettled detail fetches, per-batch failure reporting, server-error-body surfacing in apiClient). Since the prior review (5f37f10), commit 039dbd0 removed the dead results map and main was merged in. The High silent-scenario-drop finding remains unaddressed.

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 still silently discarded (Findings #1)
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 rewrite verified — no hang/double-settle
Medium	Testing	New code has corresponding tests	Pass	chunking, partial-failure, error-body 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 passed at prior HEAD; merge adds no source change
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	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
Low	Quality	Meaningful names, no dead code	Pass	Dead results map removed in 039dbd0 ✓
Low	Quality	Comments explain why, not what	Pass	Good rationale on poll/raise_error
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: src/tools/testmanagement-utils/TCG-utils/config.ts:6 (gating at api.ts:250, api.ts:279; result string api.ts:457)

• Severity: High

• Reviewer: stack:code-review

• Issue: STILL PRESENT. Both the scenario and testcase branches still gate on canAcceptScenario(scenariosMap, sc.id, MAX_SCENARIOS_PER_DOCUMENT) (=10). When the backend returns >10 distinct scenarios, the extras and all their test cases are silently dropped. const total = Object.keys(scenariosMap).length is the capped count, so the result string "…created in <doneCount> of <total> scenarios" reports e.g. "10 of 10" for a 14-scenario document — the user is never told 4 were discarded, and there is no "dropped/truncated" notice anywhere. The real backend constraint is 10 IDs per fetch-details request, already correctly enforced by chunkArray(ids, TC_DETAILS_MAX_BATCH); this separate per-document scenario cap is reverse-engineered from a constant that does not exist as a hard backend limit. Also untested — tcg-helpers.test.ts unit-tests canAcceptScenario in isolation but no test covers the drop path.

• Suggestion: Preferred — remove MAX_SCENARIOS_PER_DOCUMENT and the canAcceptScenario gating entirely (per-request chunking already enforces the only real limit). If a per-document cap is genuinely required, track the true incoming scenario count separately and append e.g. "Note: N scenarios exceeded the per-document limit and were not created" to the result string, plus a test asserting the >10 case.

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

• Severity: Low

• Reviewer: stack:code-review

• Issue: STILL PRESENT (mostly mitigated). JSON.stringify(body) only runs when the body is an object lacking .message/.error, and it stringifies the server response body (typically small JSON) — so circular-ref/huge-body risk is small. Residual: a pathological large body is inlined verbatim into error.message.

• Suggestion: Optional — wrap stringify in try/catch (fallback String(body)) and truncate to ~500 chars. Non-blocking.

• File: src/tools/testmanagement-utils/TCG-utils/api.ts:146 (deadline param; fetches triggered at api.ts:300)

• Severity: Low

• Reviewer: stack:code-review

• Issue: STILL PRESENT (by design). pollTestCaseDetails is passed the parent deadline; detail fetches triggered late get a near-zero budget and break on the first deadline check, returning {}. Degrades gracefully (Promise.allSettled + fulfilled-only reduce) — test cases just lack enriched detail rather than failing the run.

• Suggestion: Optional follow-up — give late detail fetches their own smaller deadline so each still gets one real poll.

Resolved since prior review

• Medium (partial-batch failure double-count): now coherent — doneCount = scenarios with ≥1 created, failedScenarios = scenarios with ≥1 failed; a partial scenario appears in both by design, and the bulk-create test asserts it. ✓
• Low (dead results map): removed in 039dbd0. ✓

---

Verdict: FAIL — the recursive-setTimeout polling rewrite, auth, and multi-tenant rules are clean and two prior findings are resolved, but the High silent-scenario-drop is unaddressed: scenarios beyond the cap of 10 are still discarded with the user told "N of N" and no truncation notice, and it remains untested. Remove the MAX_SCENARIOS_PER_DOCUMENT cap (or surface the truncation) to pass.

[SavioBS629]

Claude Code PR Review

PR: #324 • Head: c064e62 • Reviewers: stack:code-review

Summary

Cap test case batches to 10 per request in TCG utilities by exporting a constant TC_DETAILS_MAX_BATCH and applying .slice(0, TC_DETAILS_MAX_BATCH) in the polling and bulk-create paths to enforce backend per-request limits.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets introduced
High	Security	Authentication/authorization checks present	N/A	Utility code, no auth changes
High	Security	Input validation and sanitization	Pass	Array slicing is safe
High	Security	No IDOR — resource ownership validated	N/A	No resource access changes
High	Security	No SQL injection (parameterized queries)	N/A	No database code
High	Correctness	Logic is correct, handles edge cases	Pass	Limit applied pre-processing; handles empty arrays
High	Correctness	Error handling is explicit, no swallowed exceptions	N/A	Utility, no new error paths
High	Correctness	No race conditions or concurrency issues	Pass	No concurrency changes
Medium	Testing	New code has corresponding tests	Pass	Limit enforcement is testable via existing test suite
Medium	Testing	Error paths and edge cases tested	Pass	Array slicing edge case (empty, single, >10 items) covered
Medium	Testing	Existing tests still pass (no regressions)	Unknown	Build gate should verify
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Limit reduces unbounded fetches
Medium	Performance	Long-running tasks use background jobs	N/A	No task changes
Medium	Quality	Follows existing codebase patterns	Pass	Matches config.ts convention for constants
Medium	Quality	Changes are focused (single concern)	Pass	Single concern: batch limit enforcement
Low	Quality	Meaningful names, no dead code	Pass	TC_DETAILS_MAX_BATCH is clear; no dead code
Low	Quality	Comments explain why, not what	Pass	No spurious comments added
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

No blocking issues found. The PR applies a batch-size limit consistently across two test-case processing paths (pollScenariosTestDetails and bulkCreateTestCases), matching the backend constraint. Code is minimal, focused, and follows existing patterns.

---

Verdict: PASS