test(accounts): regression guards for discover-org admin scope + 5xx-on-cred-failure (post-#212 CR)

[cristim]

Follow-up to PR #212 — addresses the only un-applied CR finding from that PR's reviews.

Background

PR #212 wired the AWS Organizations discovery endpoint and went through three CR rounds (15:25Z, 19:32Z, 19:42Z on 2026-04-30). It was merged at 20:34Z with most CR findings addressed in-line, but one nitpick from the 19:32Z review didn't make it into the merge:

Add regression tests for the admin-only and 5xx behaviors. This block locks down the 400/404 paths, but it still doesn't assert the two review fixes that changed the handler contract: non-admin callers must be rejected, and credential-resolution failures must not surface as client errors.

Both behaviours are correctly implemented in the merged code; without tests they're a refactor away from quietly regressing.

What this PR adds

Two test cases in internal/api/handler_accounts_test.go:

TestDiscoverOrgAccounts_RejectsNonAdmin

Non-admin session must hit a 403 ClientError. Without this guard, a refactor that swaps requireAdmin back to requirePermission("create","accounts") silently re-opens org discovery to non-admin users — and discovered rows go straight into cloud_accounts, which is a privilege-escalation surface (not just a UX preference).

TestDiscoverOrgAccounts_CredResolutionFailureIs5xx

Credential-resolution failures must surface as 5xx (retryable), not 4xx. Wraps a CredentialStore stub (errCredStore) whose LoadRaw returns a transient-style error; asserts the handler returns a non-ClientError carrying the "resolve credentials" stage marker so operators can find it in logs. Without this, a future refactor that re-applies NewClientError(400, ...) on resolver errors would silently make transient store/network failures non-retryable.

The test's discoverOrgFn injection is a t.Fatal — proving the failure path exits before reaching discovery (and that the test is exercising the credential-resolution failure path specifically, not a different shortcut).

Notes on the other CR findings

PR #212 had 5 distinct CR concerns across 3 reviews. Status:

#	Concern	Status
1	Member ID/CreatedAt/UpdatedAt init missing	✅ fixed in #212
2	Duplicate-key race not handled in persist loop	✅ fixed in #212
3	AWSAuthMode = "" will fail DB CHECK constraint	⚠️ false alarm — see below
4	Nil guard on disco.Accounts	✅ fixed in #212
5	Stale aws_auth_mode=bastion comments	✅ fixed in #212
6	Tests for admin-only + cred-resolution → 5xx	❌ this PR

#3 false alarm

The store layer uses nullStringFromString(account.AWSAuthMode) for the INSERT (internal/config/store_postgres.go:CreateCloudAccount). That converts "" → sql.NullString{Valid: false} → SQL NULL. The cloud_accounts.aws_auth_mode column allows NULL (no NOT NULL constraint in migrations/000011_cloud_accounts.up.sql:18-19), and Postgres CHECK (col IN (...)) constraints satisfy NULL by default (the constraint is vacuously true on NULL). So persisting member.AWSAuthMode = "" is correct as-is. I'll reply on the CR thread with this rationale.

Verification

• go build ./... clean
• go test ./internal/api/... — full package green (incl. the two new cases + all PR feat(accounts): wire AWS Organizations discovery endpoint (closes #208) #212 cases)
• Pre-commit (gocyclo / gosec / trivy / migration-conflicts / go-test) — green

Triage

type/chore, severity/medium, urgency/this-sprint, impact/internal, effort/xs, priority/p2, triaged. Pure-test PR; no behaviour change.

Summary by CodeRabbit

• Tests
  • Added regression tests for organization account discovery to verify non-admin access is properly rejected with appropriate error codes and credential resolution failures are handled as expected.

[cristim]

@coderabbitai review

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eacf4944-222e-4f35-b6b2-1920c010c4e1

📥 Commits

Reviewing files that changed from the base of the PR and between c84fd02 and 7e8bab7.

📒 Files selected for processing (1)

• internal/api/handler_accounts_test.go

---

📝 Walkthrough

Walkthrough

Added regression tests for discoverOrgAccounts handler to validate non-admin session rejection with 403 errors and credential resolution failures as non-client (5xx retryable) errors. Included test stub errCredStore and fixed compilation issue in existing test.

Changes

Test Additions for discoverOrgAccounts Handler

Layer / File(s)	Summary
Test Support Stub internal/api/handler_accounts_test.go	Added errCredStore type implementing credential store interface with all methods stubbed to return injected failure from LoadRaw, simulating transient credential-resolution outages.
Regression Tests internal/api/handler_accounts_test.go	Added TestDiscoverOrgAccounts_RejectsNonAdmin validating non-admin sessions are rejected with ClientError code 403; added TestDiscoverOrgAccounts_CredResolutionFailureIs5xx validating credential resolution failures surface as non-ClientError (5xx retryable) and discoverOrgFn is not called.
Test Compilation Fix internal/api/handler_accounts_test.go	Removed stray numeric literals from TestDiscoverOrgAccounts_NotFound that prevented test file compilation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat(accounts): wire AWS Organizations discovery endpoint (closes #208) #212: Modifies internal/api/handler_accounts_test.go and refactors credential-resolution error handling in discoverOrgAccounts; this PR adds regression tests validating the behavior of that refactored flow.

Suggested labels

effort/s

Poem

🐰 A rabbit hops through test files bright,
Catching errors left and right,
Non-admins blocked, credentials fail—
Coverage grows, no stone untailed! 🧪✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: adding regression tests for admin scope rejection and 5xx error handling on credential resolution failures in the discover-org functionality.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/issue-208-discover-admin-and-5xx-regression-guards

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

_{Review rate limit: 4/5 reviews remaining, refill in 12 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.