feat: add test cases to existing test run via updateTestRun

[SavioBS629]

No description provided.

[SavioBS629]

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

[SavioBS629]

[Medium] No .min(1) on test_case_ids — empty selection fires a no-op PATCH reported as success

hasTestCases only checks the outer add_test_cases array length, so add_test_cases: [{ test_case_ids: [] }] fires a real PATCH to /test-cases with an empty selection and returns "...(added 0); changes apply asynchronously." as if it did something — a real API side effect masquerading as a no-op.

Suggestion: Add .min(1) to test_case_ids (and consider it on add_test_cases), and/or filter empty selections before building the request body.

Suggested change

	configuration_ids: z
	test_case_ids: z
	.array(z.string())
	.min(1)
	.describe("Test case IDs, e.g. TC-123"),

Reviewer: stack:code-reviewer

[SavioBS629]

[Medium] Duplicate getTMBaseURL round-trips when both concerns are supplied

Both updateTestRunMetadata and this helper call getTMBaseURL(config) independently. The module-level cache in tm-base-url.ts is intentionally skipped in REMOTE_MCP mode, so a call supplying both name/run_state and add_test_cases runs the region-discovery probe twice — doubling latency/API load, and letting the two sub-requests theoretically land on different regions if one probe flakes.

Suggestion: Resolve tmBaseUrl once in the updateTestRun dispatcher and pass it into both helpers; optionally Promise.all the two independent PATCHes.

Reviewer: stack:code-reviewer

[SavioBS629]

Claude Code PR Review

PR: #333 • Head: e7cfff0 • Reviewers: stack:code-reviewer

Summary

Splits updateTestRun into a dispatcher plus two endpoint-specific helpers (updateTestRunMetadata → .../update, updateTestRunTestCases → .../test-cases) and adds an add_test_cases / preserve_existing_results capability, with a new 8-case vitest suite. Core dispatch, error propagation, and tests are correct; no security or data-corruption issues. A few efficiency/validation/coverage items are worth addressing before merge.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Auth from config; Basic header matches existing pattern
High	Security	Authentication/authorization checks present	Pass	Same getBrowserStackAuth flow as sibling tools
High	Security	Input validation and sanitization	Pass	Zod schema; IDs url-encoded — but see empty-array gap below
High	Security	No IDOR — resource ownership validated	N/A	Delegated to BrowserStack API
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	Metadata path not regressed (base only ever had name/run_state); dispatch sound
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	isError OR'd across both concerns; failures never masked
High	Correctness	No race conditions or concurrency issues	Pass	Sequential; see perf note on parallelizing
Medium	Testing	New code has corresponding tests	Pass	Dedicated 187-line suite added
Medium	Testing	Error paths and edge cases tested	Fail	catch/rejection paths, configuration_ids forwarding, both-fail case uncovered
Medium	Testing	Existing tests still pass (no regressions)	Pass	Metadata behavior preserved; assertions match impl
Medium	Performance	No N+1 queries or unbounded data fetching	Fail	getTMBaseURL resolved twice per dual-concern call (uncached in remote mode)
Medium	Performance	Long-running tasks use background jobs	N/A	Backend applies test-case add asynchronously
Medium	Quality	Follows existing codebase patterns	Pass	Matches sibling tools
Medium	Quality	Changes are focused (single concern)	Pass	Focused feature + tests
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Good dispatcher docstring
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: src/tools/testmanagement-utils/update-testrun.ts:180 (and the metadata helper's own getTMBaseURL call)

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: updateTestRunMetadata and updateTestRunTestCases each call getTMBaseURL(config) independently. Per src/lib/tm-base-url.ts, the module-level cache is intentionally skipped in REMOTE_MCP mode (the deployment this reviewing repo targets), so a call that supplies both name/run_state and add_test_cases performs the region-discovery probe twice, doubling latency/API load and theoretically allowing the two sub-requests to land on different regions if one probe flakes.

• Suggestion: Resolve tmBaseUrl once in the dispatcher and pass it into both helpers; optionally Promise.all the two independent PATCHes.

• File: src/tools/testmanagement-utils/update-testrun.ts:14

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: Neither TestCaseSelectionSchema.test_case_ids nor the outer add_test_cases array has .min(1). hasTestCases only checks the outer array length, so add_test_cases: [{ test_case_ids: [] }] fires a real PATCH to /test-cases with an empty selection and reports "...(added 0); changes apply asynchronously." as if it did something — a real API side effect masquerading as a no-op.

• Suggestion: Add .min(1) to test_case_ids (and consider it for add_test_cases), and/or filter empty selections before computing hasTestCases/building the body.

• File: src/tools/testmanagement-utils/update-testrun.ts:122

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The Authorization: "Basic " + Buffer.from(...) block is now duplicated verbatim in both helpers (was single before the split).

• Suggestion: Extract a buildAuthHeader(config) helper shared with the existing pattern.

• File: tests/tools/update-testrun.test.ts (whole file)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Coverage gaps: neither catch/formatAxiosError branch is exercised (no rejected apiClient.patch); configuration_ids forwarding is never asserted; only "one call fails" is tested (not "both fail" surviving the flatMap); no empty-test_case_ids test.

• Suggestion: Add rejection-path tests for both helpers, a configuration_ids forwarding assertion, a both-fail case, and (after the .min(1) fix) an empty-selection rejection test.

Non-blocking notes

• [Info] When both concerns run and one fails, the aggregated content blocks are concatenated unlabeled — a caller infers which failed from wording only. Consider prefixing blocks ([metadata] / [test-cases]) or a short summary line.
• [Info, pre-existing] README.md:337 still describes updateTestRun as covering "tags, notes" (never implemented) and omits the new add_test_cases capability — worth refreshing here since it's on-topic.

Raised by other reviewers (not independently confirmed)

None — no human reviewer comments on this PR.

---

Verdict: PASS — the refactor and new capability are correct with a solid test suite and no security/regression issues; the two Medium items (double base-URL resolution in remote mode, empty-selection validation gap) and the Low test/dedup items are recommended before merge but non-gating.