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]

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.

[SavioBS629]

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

[SavioBS629]

[Medium] Newest builds can be silently missed when a window exceeds the page cap

Pages are oldest-first and next_page walks forward toward newer builds; tail keeps only the last limit. If a window exceeds MAX_PAGES_PER_WINDOW (60) pages, the loop breaks mid-window and returns builds from the middle — not the newest at the window's end. The outer loop then sees collected.length >= limit and stops, silently returning stale builds. Only triggers if a build name runs >~600 times inside the narrowest 2-day window, so impact is low — but it's a silent-correctness gap.

Suggestion: If the cap is hit while has_next is still true, reach the newest page (descending sort or smaller window) or surface that results may be incomplete, rather than returning the middle of the window.

Reviewer: stack:code-review

[SavioBS629]

[Low] Required BuildSummary fields sourced from untyped JSON

build_number/status/started_at are typed required, but they come from response.json() (any) and only build.build_id is null-checked. A build missing those fields yields undefined in a non-optional field — no crash (output is JSON-stringified), but the type is dishonest.

Suggestion: Mark these fields optional, or default them when mapping.

Reviewer: stack:code-review

[SavioBS629]

Claude Code PR Review

PR: #323 • Head: cb612fc • Reviewers: stack:code-review

Summary

Adds a new listBuildIds RCA tool that lists up to 5 recent builds for a project + build name across all users (progressively-widening date-window walk over the project-builds endpoint), exports extractFailedTestIds and drops its run_count guard so zero-run failures are collected, reverts the earlier getBuildId change (kept user-scoped), and adds unit tests for extractFailedTestIds, listBuildIds, and listBuildIdsTool.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Auth via getBrowserStackAuth(config), Basic header
High	Security	Authentication/authorization checks present	Pass	Header-based auth; no creds in errors
High	Security	Input validation and sanitization	Pass	Inputs from typed Zod params
High	Security	No IDOR — resource ownership validated	Pass	"across all users" is intentional, documented in prompt
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	One low-probability silent gap (page cap) — see Findings #1
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Throws on non-ok; tool wraps to isError
High	Correctness	No race conditions or concurrency issues	Pass	Sequential fetches, function-scoped state
Medium	Testing	New code has corresponding tests	Pass	Algorithm + both error paths covered; prior gap addressed
Medium	Testing	Error paths and edge cases tested	Pass	run_count:0, missing details, 404, unresolved project
Medium	Testing	Existing tests still pass (no regressions)	Pass	tsc + suites pass (verified by reviewer)
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Bounded by MAX_PAGES_PER_WINDOW (window re-walk noted, acceptable)
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Matches rca-agent-utils fetch convention
Medium	Quality	Changes are focused (single concern)	Pass	RCA build-lookup improvements
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Good rationale comments on windowing
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: src/tools/rca-agent-utils/list-build-ids.ts:139 (loop at :107)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: Pages are oldest-first and next_page walks forward toward newer builds; tail keeps only the last limit. If a window exceeds MAX_PAGES_PER_WINDOW (60) pages, the loop breaks mid-window and returns builds from the middle — not the newest at the window's end. The outer loop then sees collected.length >= limit and stops, silently returning stale (non-newest) builds. Only triggers if a single build name runs >~600 times inside the narrowest 2-day window, so impact is low, but it's a silent-correctness gap.

• Suggestion: If the cap is hit while has_next is still true, skip to a behavior that reaches the newest page (request a descending sort, or surface that results may be incomplete) rather than returning the middle of the window.

• File: src/tools/rca-agent.ts:100 (listBuildIdsTool catch)

• Severity: Low

• Reviewer: stack:code-review

• Issue: listBuildIdsTool catches internally and returns { isError: true } instead of re-throwing, so the outer wrapper's handleMCPError (which carries the error-path trackMCP) never fires on a business-logic failure — only the success-path trackMCP runs. Pre-existing pattern shared by the other RCA tools, perpetuated here.

• Suggestion: Let the util throw and rely solely on the outer try/catch + handleMCPError for both instrumentation paths (follow-up; not blocking).

• File: src/tools/rca-agent-utils/list-build-ids.ts:126

• Severity: Low

• Reviewer: stack:code-review

• Issue: BuildSummary types build_number/status/started_at as required, but they're read from untyped response.json() and only build.build_id is null-checked. A build missing those fields yields undefined in a non-optional field. No crash (output is JSON-stringified), but the type is dishonest.

• Suggestion: Mark the fields optional or default them when mapping.

Notes (not blocking)

• Direct fetch use rather than src/lib/apiClient.ts follows the existing rca-agent-utils convention (get-build-id.ts, get-failed-test-id.ts) — pre-existing module tech debt, not a new deviation.
• Window re-walk on widening re-fetches narrower spans, but the common active-build case resolves in the first 2-day window (1 page); acceptable and documented.
• limit is intentionally not exposed as a tool param (prompt says "up to 5", util default is 5) — correct minimal-surface choice.
• Prior review's two items are addressed: extractFailedTestIds is now exported + covered by 6 tests; the getBuildId user_name change was reverted and documented as user-scoped.

---

Verdict: PASS — no Critical/High issues; build, type-check, and tests pass. The page-cap silent edge case (#1) is the only correctness item worth a follow-up guard.