fix: getBuildId & failedTestIds mcp tool

[SavioBS629]

No description provided.

[SavioBS629]

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

[SavioBS629]

[Low] Behavioral change is untested

Dropping && node.details?.run_count is a plausible, intended fix (zero-run failed tests were being skipped), but extractFailedTestIds is non-exported and getTestIds is mocked in tests/tools/rcaAgent.test.ts, so neither the old nor new condition is ever exercised — no regression guard.

Suggestion: Export extractFailedTestIds (or drive via getTestIds with a mocked fetch + hierarchy fixture) and add cases: status match with run_count: 0, status match with missing details, and a non-matching status.

Reviewer: stack:code-review

[SavioBS629]

Claude Code PR Review

PR: #323 • Head: 35b9a51 • Reviewers: stack:code-review

Summary

Two RCA-agent utility bug fixes: get-build-id.ts drops the redundant user_name query param (auth is fully carried by the Basic header), and get-failed-test-id.ts removes the && node.details?.run_count guard so a node matching the requested status is collected even when run_count is 0/absent.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Auth via Basic header; no creds added
High	Security	Authentication/authorization checks present	Pass	Unchanged; header-based auth intact
High	Security	Input validation and sanitization	N/A	No new user input
High	Security	No IDOR — resource ownership validated	N/A	Read-only build/test lookup
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	run_count guard removal is the intended fix
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Unchanged; errors thrown/logged
High	Correctness	No race conditions or concurrency issues	Pass	Pure synchronous extraction
Medium	Testing	New code has corresponding tests	Fail	extractFailedTestIds has no direct coverage; mocked in tests
Medium	Testing	Error paths and edge cases tested	Fail	New run_count: 0 behavior unpinned
Medium	Testing	Existing tests still pass (no regressions)	Pass	No test changes; suite unaffected
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Pagination capped at 5 requests
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Consistent with surrounding utils
Medium	Quality	Changes are focused (single concern)	Pass	Two small targeted fixes
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	N/A	—
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: src/tools/rca-agent-utils/get-failed-test-id.ts:76

• Severity: Low

• Reviewer: stack:code-review

• Issue: Dropping the && node.details?.run_count guard is a plausible, intended fix (zero-run failed tests were being silently skipped), but extractFailedTestIds is non-exported and the getTestIds module is mocked in tests/tools/rcaAgent.test.ts, so neither the old nor new condition is ever exercised. No regression guard.

• Suggestion: Export extractFailedTestIds (or drive it via getTestIds with a mocked fetch returning a hierarchy fixture) and add cases: status match with run_count: 0, status match with missing details, and a non-matching status.

• File: src/tools/rca-agent-utils/get-build-id.ts:12

• Severity: Low

• Reviewer: stack:code-review

• Issue: Removing the user_name query param is safe for auth (Basic header carries username:accessKey). Residual risk only if /builds/latest used user_name as a filter/disambiguator rather than auth — removing it would then change which build is returned.

• Suggestion: Confirm with the API owner that user_name was vestigial (not a filter). No code-level blocker.

Pre-existing (not introduced here, not blocking)

• get-failed-test-id.ts:80 pushes idMatch[1] (a string) into FailedTestInfo.test_id typed number (types.ts). Follow-up.
• Both utils call Node fetch directly rather than routing through src/lib/apiClient.ts; pre-existing, no new fetch calls added.

---

Verdict: PASS — two reasonable bug fixes, no Critical/High issues; add test coverage for extractFailedTestIds as a follow-up.