Skip to content

fix(purchases): don't leak raw persistence error into audit-gap status (refs #623) - #628

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/audit-gap-error-leak
May 21, 2026
Merged

fix(purchases): don't leak raw persistence error into audit-gap status (refs #623)#628
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/audit-gap-error-leak

Conversation

@cristim

@cristim cristim commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to a CodeRabbit review comment on #623 that landed right at merge time, so it couldn't be folded into that PR.

recordHistoryAuditGap (added in #623) embedded the raw SavePurchaseHistory error (histErr) verbatim into exec.Error. That field is surfaced to clients via the history status_description, so an internal storage error (e.g. SQL constraint text like monthly_cost violates not-null constraint) leaked to the UI.

Fix

  • Log histErr server-side via logging.Errorf for diagnosis.
  • Store only a generic, user-safe note in exec.Error ("commitment X purchased but its history record failed to save") with no raw error interpolation.
  • Extend the existing audit-gap regression test (TestManager_ExecuteAndFinalize_HistorySaveFailure_StaysVisible) with NotContains assertions on the raw error text, so it fails-on-regression if the leak is reintroduced.

The #621 invariants are preserved: a history-write failure still keeps the completed purchase visible (the note is still recorded), the execution still stays completed (not failed), and dollar totals are unaffected.

Test plan

  • go test ./internal/purchase/ -run TestManager_ExecuteAndFinalize_HistorySaveFailure_StaysVisible passes; the new NotContains assertions fail against the pre-fix code
  • gofmt / go vet clean

Refs #623.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Refined error messaging for purchase history failures to exclude internal technical details from user-facing notifications while preserving essential context information.

Review Change Stack

recordHistoryAuditGap embedded the raw SavePurchaseHistory error (histErr)
verbatim into exec.Error, which is surfaced to clients via the history
status_description. That leaks internal storage details (e.g. SQL constraint
text) to the UI.

Log histErr server-side via logging.Errorf for diagnosis, and store only a
generic user-safe note ("commitment X purchased but its history record failed
to save") in exec.Error. Extends the existing audit-gap regression test with
NotContains assertions so the raw error text can never reach the user-facing
field again.

Follow-up to the CodeRabbit review on #623 (landed at merge time).
@cristim cristim added bug Something isn't working triaged Item has been triaged priority/p2 Backlog-worthy severity/low Minor harm urgency/this-sprint Within the current sprint impact/all-users Affects every user effort/xs Trivial / one-liner type/security Security finding labels May 20, 2026
@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6c6e9689-c5ea-4700-b714-a46adc3ff9ea

📥 Commits

Reviewing files that changed from the base of the PR and between 7cbcb0c and 06579b7.

📒 Files selected for processing (2)
  • internal/purchase/execution.go
  • internal/purchase/execution_test.go

📝 Walkthrough

Walkthrough

Error sanitization in purchase history failure path: when SavePurchaseHistory fails, recordHistoryAuditGap now logs the raw database error server-side while updating the user-facing error message to be generic and safe, omitting persistence details.

Changes

Purchase History Audit Gap Error Sanitization

Layer / File(s) Summary
Error message sanitization in history save failure
internal/purchase/execution.go, internal/purchase/execution_test.go
recordHistoryAuditGap logs the raw persistence error via logging.Errorf and updates exec.Error with a generic "commitment purchased but its history record failed to save" message without including error details. Tests add negative assertions to verify raw database constraint text does not appear in exec.Error.

Estimated Code Review Effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly Related PRs

  • LeanerCloud/CUDly#623: Earlier PR that introduced the audit-gap recordHistoryAuditGap behavior to keep completed purchases visible; this PR enhances error message handling in that same path.
  • LeanerCloud/CUDly#233: Related test assertions that ensure exec.Error is sanitized when purchase execution/history persistence fails.

Suggested Labels

severity/medium, effort/s, type/bug

Poem

🐰 Errors stay safe, hidden from sight,
Raw logs on server, users sleep tight,
History stumbled? A generic note,
No constraint tales in what we quote!
Secrets are logged, but truth stays mute.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: preventing raw persistence errors from leaking into user-facing audit-gap status messages.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 fix/audit-gap-error-leak

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

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ 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.

@cristim
cristim merged commit 743a5ae into feat/multicloud-web-frontend May 21, 2026
4 checks passed
@cristim
cristim deleted the fix/audit-gap-error-leak branch May 21, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort/xs Trivial / one-liner impact/all-users Affects every user priority/p2 Backlog-worthy severity/low Minor harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant