[IAmJSD]

Summary

Test-only change. The 4-eyes self-approval check (checkDifferentApprover, strings.EqualFold) and the SQS approver match (matchActorAgainstApprovers, strings.ToLower on both sides) are both case-insensitive on main today, but no fixture tested that. Every email literal in those tests was lowercase and identical on both sides, so replacing EqualFold with ==, or dropping either ToLower, left the suite green.

Root cause: missing test coverage, not a production bug. With ASCII emails the new case-differing tests pass against unchanged production code, so this PR makes no production change.

Closes #307

Changes

All changes are in internal/purchase tests:

• approvals_test.go: TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied is now table-driven. It has three rows: byte-identical emails; creator stored as Creator@Example.COM with actor creator@example.com; creator creator@example.com with actor CREATOR@Example.com. Every row must return ErrFourEyesDenied and must not reach TransitionExecutionStatus.
• coverage_extra_test.go:
  • TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied gets a new row. The creator's user email and the account contact_email are stored as Owner@Example.COM, and the SQS actor_email is owner@example.com. This runs both case-insensitive comparisons end to end through ProcessMessage: the approver check has to accept the actor, and then 4-eyes has to deny them as the creator.
  • New TestProcessMessage_ApproverMatchIgnoresEmailCase has one row for each side of the approver comparison (mixed-case contact_email with a lowercase actor, and the reverse). Removing either ToLower fails one of the rows. The existing TestProcessMessage_ApproveRejectsNonMatchingActor still catches a change that accepts everyone.
• In the new rows, the mocks are bound with .Test(t). Without that, an unexpected TransitionExecutionStatus call panics and the remaining rows are skipped. With it, each regressed row fails on its own.

Verification

All of this is unit tests with mocks. No Postgres, no real cloud, no email. Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

There is no production fix, so "fail before" is shown with mutation probes: I edited the production code by hand, ran go test ./internal/purchase/ -count=1 -run 'FourEyes|ApproverMatch|ProcessMessage_Approve|ProcessMessage_Cancel', then restored it.

Mutation	Tests on main	Tests in this PR
M1: approvals.go strings.EqualFold(creatorEmail, actorEmail) changed to creatorEmail == actorEmail	ok (all pass)	FAIL: SQSActorEqualsCreator_Denied/creator_stored_mixed_case, /actor_mixed_case, ProcessMessage_ApproveFourEyesOn_SelfApproveDenied/stored_mixed_case,_actor_lowercase
M2: messages.go strings.ToLower removed from the actor side	ok	FAIL: ApproverMatchIgnoresEmailCase/contact_email_lowercase,_actor_mixed_case
M3: messages.go strings.ToLower removed from the contact_email side	ok	FAIL: ApproverMatchIgnoresEmailCase/contact_email_mixed_case,_actor_lowercase, SelfApproveDenied/stored_mixed_case,_actor_lowercase
none (unmodified code)	ok	ok

Other checks, all passing:

go build ./...
go vet ./internal/purchase/
go test ./internal/purchase/ -count=1                      # ok
go test -race ./internal/purchase/ -count=1 -run 'FourEyes|ApproverMatch|ProcessMessage_Approve'   # ok
go test ./internal/mocks/ -count=1                         # ok (repo-wide mock-assertion guards)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/purchase/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/purchase     # silent
bash scripts/gosec-hook.sh <changed files>                 # exit 0
git-secrets --register-aws, --scan on all tracked files    # clean (isolated temp repo/HOME)
bash scripts/test-git-secrets-allowlist.sh                 # 42 PASS, 0 FAIL

No DB or schema change, so there is no concurrency surface to race-test.

Out of scope (noted, not changed)

• The HTTP-side approver match, authorizeApprovalAction in internal/api/handler_purchases.go, uses the same ToLower-both-sides comparison. Its case handling is likewise untested, but the issue covers internal/purchase only.
• The two code paths use different case-insensitive comparisons: 4-eyes uses strings.EqualFold, approver matching uses strings.ToLower. They agree for ASCII but not for every non-ASCII character. For example, ToLower turns the Kelvin sign U+212A into an ASCII k, while EqualFold treats U+017F ſ as equal to s and ToLower does not. Whether this can be exploited depends on whether a non-ASCII address can become a session or actor_email. I didn't change it here. It may be worth a separate issue to normalize emails in one shared place.

Maintainers: please mirror the issue's triage labels onto this PR (I can't apply labels from the fork workflow): priority/p2, severity/medium, impact/internal, type/chore, urgency/this-quarter, effort/xs.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: fd7859a5-1d26-45f3-99c3-5ca31979b770

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and 91d736d.

📒 Files selected for processing: 2

• internal/purchase/approvals_test.go
• internal/purchase/coverage_extra_test.go

+----------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Fix the problem, not the blame. It doesn't really matter whether the bug is your fault or someone else's - it is still your problem, and it still needs to be fixed. |
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review (claude-cc-platform reviewer, cold brief): APPROVE 91d736d780772461b25fabd67f6136bc736b8303

No blocking findings. The change is test-only: approvals_test.go and coverage_extra_test.go (+139/-53).

Every email comparison on a four-eyes or approver path is already case-insensitive:

• approvals.go:315: strings.EqualFold on the trimmed creator and actor;
• messages.go:267-269: the approver allow-list trims and lowercases both sides;
• handler_purchases.go:4086-4088 (outside this PR): the HTTP approver match does the same.

The API four-eyes check (requireDifferentApprover, :1413) and approvals.go:288 compare user ids, not emails. A grep of non-test Go code under internal/ found no case-sensitive email comparison on these paths.

Mutation probes, all caught:

• EqualFold replaced by == (approvals.go:315): 2 tests fail;
• ToLower removed on the actor side (messages.go:267): 1 test fails;
• ToLower removed on the contact side (:269): 2 tests fail.

The new rows bind their mocks with .Test(t), so a regressed row fails on its own instead of panicking.

Base drift: clean merge onto current main (tree 5c90a7be). go test -race -count=1 ./internal/purchase/ passes at the head and on the merged tree.

Nits:

• No test pins the TrimSpace handling.
• EqualFold and ToLower disagree on a few non-ASCII characters (U+212A, U+017F). As the PR says, this is out of scope unless non-ASCII addresses can become actors.
• The PR does not yet carry test(purchase): 4-eyes and approver matching are never tested with a case-differing email #307's labels.

[cristim]

Independent review (claude-fleet): APPROVE 91d736d

Findings (none blocking):

• [non-blocking] coverage_extra_test.go, TestProcessMessage_ApproverMatchIgnoresEmailCase: the name and comment say ProcessMessage, but the test calls verifyAsyncApprovalActor directly. That is the shared gate both paths use, so the coverage is real; only the name is loose.

Attacked:

• Production sites: the tests run the real checkDifferentApprover (approvals.go:315) and matchActorAgainstApprovers (messages.go:267-269), not copies, and also reach both end to end through ProcessMessage.
• Other comparison sites, from a grep of internal/ for email ==/ToLower/EqualFold:
  • The tier-1 UUID compare is at approvals.go:288.
  • HTTP authorizeApprovalAction (handler_purchases.go:4041) has the same pattern and is not covered here, as the PR body says.
  • RI exchange under 4-eyes refuses direct execution (handler_ri_exchange.go:2567), so it has no email compare.
  • internal/server/app.go:961 is admin bootstrap, not approval.
• Trimming: both production sites trim whitespace. Plus-addressing and a trailing dot are treated as different addresses, which is the conservative choice.
• The 53 deleted lines: every test kept its assertions, with Error tightened to ErrorIs(ErrFourEyesDenied). The byte-identical case survives as row 1 of both tables, so nothing was dropped.
• Mutation probes on production code, each restored and confirmed identical with cmp:
  • M1, EqualFold changed to ==: 3 new rows FAIL.
  • M2, the self-approval check disabled: all 5 4-eyes rows FAIL.
  • M3, the actor-side ToLower dropped: the actor-mixed row FAILS.
  • M4, the contact-side ToLower dropped: the contact-mixed and self-approve-mixed rows FAIL.
  • M5, the creator compared against the wrong field: the SQSActorEqualsCreator rows FAIL.
  • M1 and M4 both pass against the base tests, which confirms the gap test(purchase): 4-eyes and approver matching are never tested with a case-differing email #307 describes.

Verified locally, on a git archive export of HEAD with no cloud credentials:

• go build ./... && go vet ./internal/purchase/ && go test -count=1 -race ./internal/purchase/: ok.
• gofmt -l on both files: clean.
• diff -rq between the base and HEAD exports: only the two test files differ.

Reviewed by an independent agent with no author context. Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification (macOS, exact head 91d736d780772461b25fabd67f6136bc736b8303, detached worktree): go build ./..., go test -race -short -count=1 ./... and make lint pass.

Gate status: two independent reviews approve this SHA (posted above), and local verification passes. Merge waits on @IAmJSD marking the PR ready (see #974).

[cristim]

Local verification at 91d736d780772461b25fabd67f6136bc736b8303 merged onto current main (daf87135; merge-tree 4c04c747; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 91d736d780772461b25fabd67f6136bc736b8303).