Narrow broad rescue clauses

[kitcommerce]

Client Impact

• Improves error handling by avoiding rescuing system-level exceptions (Interrupt, SignalException).

Fixes #1033

[kitcommerce]

Wave 1 reviewers were dispatched last tick but no verdicts were posted. Re-dispatch queued for next tick.

[kitcommerce]

Architecture Review\n\nVerdict: PASS_WITH_NOTES\n\nNarrowing rescues to domain-appropriate exception classes is architecturally sound and improves error propagation (Interrupt/SignalException/etc. will now bubble as expected).\n\nNotes:\n- in monitoring/worker contexts is reasonable as a last-resort to keep checks/jobs from crashing, but consider tightening to known transient dependencies where feasible (e.g., Mongo/Sidekiq connectivity errors) to avoid hiding programmer bugs.\n- around is the right specificity (that’s what raises when the class isn’t defined).\n- for / fallbacks preserves intent while avoiding masking unrelated failures.

[kitcommerce]

Architecture Review

Verdict: PASS_WITH_NOTES

Narrowing rescues to domain-appropriate exception classes is architecturally sound and improves error propagation (Interrupt/SignalException/etc. will now bubble as expected).

Notes:

• Using rescue StandardError in monitoring/worker contexts is a reasonable last-resort to keep checks/jobs from crashing, but it can hide programmer bugs. Where feasible, consider tightening to known transient dependency errors (Mongo/Sidekiq/network).
• Using rescue NameError around constantize is the right specificity (that's what constantize raises when the class isn't defined).
• Using rescue Mongoid::Errors::DocumentNotFound for find_by/find fallbacks preserves intent while avoiding masking unrelated failures.

[kitcommerce]

Simplicity Review

Verdict: PASS

This is a straightforward, mechanical improvement with minimal churn.

Highlights:

• Replacing rescue Exception with rescue StandardError keeps the intent (capture application/runtime errors) while avoiding surprising behavior.
• The ProductRule#value change removes inline rescue in favor of an explicit begin/rescue block. That's clearer, easier to debug, and avoids the common "inline rescue hides too much" footgun.
• The narrowed exception lists (ArgumentError, TypeError, Mongoid::Errors::DocumentNotFound, NameError) make each rescue's purpose self-documenting.

[kitcommerce]

Security Review

Verdict: PASS

This directly addresses WA-SEC-022 / Brakeman's RescueException warnings by ensuring system-level exceptions (Interrupt, SignalException, NoMemoryError, etc.) are not swallowed.

Notes / edge cases:

• rescue StandardError still catches a broad set of failures. In places like monitoring checks and background jobs, that may be acceptable, but it can also conceal unexpected bugs. That said, this PR is net-positive and aligns with secure defaults.
• No new error suppression is introduced beyond what already existed; most rescues are simply narrowed to the appropriate class(es).

[kitcommerce]

Rails Conventions Review

Verdict: PASS_WITH_NOTES

Conventions are followed and improved overall.

What looks good:

• In Ruby, a bare rescue already defaults to StandardError, but being explicit where Brakeman flags patterns is fine and improves readability.
• Replacing inline expr rescue fallback with an explicit begin/rescue block in ProductRule#value is more idiomatic in Rails codebases (clearer control flow, easier to instrument/log).
• Narrowing to framework/domain-specific errors (Mongoid::Errors::DocumentNotFound, NameError) matches common Rails/Mongoid patterns.

Note:

• In a few spots (e.g., monitoring checks), consider rescuing specific dependency exceptions rather than all StandardError if you want to avoid masking programmer errors, but this is acceptable as-is.

[kitcommerce]

Wave 2 Review Summary

PR #1040 — Narrow broad rescue clauses (WA-SEC-022)
Wave 2 reviewers: rails-security · database · test-quality

---

🔐 Rails-Security Review — PASS_WITH_NOTES

Verdict: PASS_WITH_NOTES | Severity: LOW

Summary: All rescue narrowings are correct and appropriate. The Exception → StandardError changes in payment/operation.rb, data_file/import.rb, and latest_version.rb correctly exclude system-level signals from being rescued. NameError rescues on constantize calls in direct_upload.rb and product_rules_preview_view_model.rb are precisely targeted. ArgumentError, TypeError for Time.zone.parse in product_rule.rb is correct.

Findings:

• ⚠️ LOW — payment/operation.rb behavioral note: Narrowing rescue Exception to rescue StandardError means that if a SignalException or Interrupt is raised mid-transaction in a background worker, the rollback! call will no longer execute before the signal propagates. Previously, the rollback would run before re-raising. This is intentional per the PR's stated goal, and the risk is low (signals are rare in payment contexts; the platform should have external reconciliation), but it's a documented behavioral change worth noting in release notes.
• ✅ rescue Mongoid::Errors::DocumentNotFound in payment/profile.rb is correct — find_by raises this exception precisely, and database connectivity errors should propagate, not be silently swallowed.
• ✅ All rescue NameError replacements for constantize are precise and correct.
• ✅ rescue ArgumentError, TypeError for Time.zone.parse covers all documented failure modes.

---

🗄️ Database Review — PASS

Verdict: PASS | Severity: null

Summary: Mongoid exception type changes are correct and safe.

Findings:

• ✅ payment/profile.rb default_credit_card: rescue Mongoid::Errors::DocumentNotFound is the exact exception raised by Mongoid's find_by when no matching document exists. The bare rescue previously swallowed all StandardErrors (including DB connectivity failures), which was overly broad. The narrowed rescue is strictly better — connection errors will now surface correctly rather than silently returning a fallback.
• ✅ payment/tender/credit_card.rb saved_card: profile.credit_cards.find(saved_card_id) raises Mongoid::Errors::DocumentNotFound when the saved card has been deleted. This is the canonical Mongoid lookup-by-id exception. Narrowing is correct.
• ✅ Data integrity: The fallback behavior in both cases is read-only (returning a different/nil credit card, not mutating state), so there is no data integrity risk from the fallback path executing on the wrong exception class.
• ✅ Exception hierarchy: Mongoid::Errors::DocumentNotFound < Mongoid::Errors::MongoidError < StandardError — it was already covered by the bare rescue / rescue StandardError. The narrowing is more precise, not a regression.
• ✅ mongoid_check.rb: rescue StandardError for the database health check is appropriate — catches any connection or operation error and returns false.

---

🧪 Test-Quality Review — PASS_WITH_NOTES

Verdict: PASS_WITH_NOTES | Severity: LOW

Summary: Rescue paths are well-covered for payment operations, data file imports, and credit card tender. One gap found: the refactored ProductRule#value date-parsing fallback lacks a unit test.

Findings:

• ✅ payment/operation.rb rollback path: operation_test.rb explicitly tests the exception→rollback scenario (Test::StoreCredit.error = true raises RuntimeError, verifies rollback! ran via cancellation.present?). RuntimeError < StandardError, so the test is valid under the new rescue clause.
• ✅ data_file/import.rb error rescue: import_test.rb covers both UnknownFormatError (unknown file type) and JSON::ParserError (invalid format), asserting error_type is set and the record is saved. Both are StandardError subclasses — rescue path tested correctly.
• ✅ payment/profile.rb default_credit_card rescue: profile_test.rb tests the fallback: clears cards, creates two without default: true, asserts find_by(default: true) rescue triggers and returns the most recent card.
• ✅ payment/tender/credit_card.rb saved_card rescue: credit_card_test.rb explicitly does saved_card.delete; assert_nothing_raised { tender.valid? } — directly exercises the DocumentNotFound rescue path.
• ⚠️ LOW — product_rule.rb value method: No unit test covers the created_at? + invalid date string path (e.g., value_field = "not-a-date" → ArgumentError → returns value_field). The refactoring is logically correct, but the fallback behavior is untested. Suggest adding a test case to product_rule_test.rb.
• ✅ product_rule.rb value_is_date_if_field_is_date validation: The validator calls Time.zone.parse and adds an error on ArgumentError, TypeError. Covered indirectly by integration tests in admin/test/integration/workarea/admin/product_rules_integration_test.rb.

---

🏁 Wave 2 Gate Result: ✅ PASSED

Reviewer	Verdict	Severity
rails-security	PASS_WITH_NOTES	LOW
database	PASS	—
test-quality	PASS_WITH_NOTES	LOW

No CHANGES_REQUIRED findings. No HIGH or CRITICAL severity issues.

Recommended follow-up (non-blocking):

1. Add a ProductRule#value test for invalid date string with created_at? = true
2. Document the payment/operation.rb signal-handling behavioral change in release notes

Wave 2 complete. Ready for merge pipeline.

[kitcommerce]

Wave 3 Gate Result ✅ PASS

Auto-generated by Kit (OpenClaw review pipeline) · Wave 3 of 4 · 2026-03-17T05:55:31Z

Reviewer	Verdict	Notes
performance	PASS	Exception-type narrowing only — no algorithmic or I/O changes
accessibility	PASS	Backend Ruby only — no user-facing UI surface
frontend	PASS	Backend Ruby only — no JS/HTML/Stimulus/Turbo changes

All three Wave 3 reviewers passed. Proceeding to Wave 4 (documentation).

[kitcommerce]

Wave 4 Documentation Review

{
  "reviewer": "documentation",
  "verdict": "PASS_WITH_NOTES",
  "severity": "LOW",
  "summary": "Docs are generally fine for this internal error-handling tweak; PR description could include quick verification steps.",
  "findings": [
    {
      "severity": "LOW",
      "file": "PR_DESCRIPTION",
      "line": 1,
      "issue": "PR description explains the motivation but does not include any \"how to verify\" guidance.",
      "suggestion": "Add 1–3 bullets with a quick manual or automated verification path (e.g., what scenario triggers the rescued exception paths, or which specs/areas to focus on)."
    }
  ]
}

The diff only narrows broad rescue clauses (e.g., rescue Exception → rescue StandardError, bare rescue → specific exceptions). No new public APIs appear to be introduced, and existing inline docs (like @return tags) remain accurate.

[kitcommerce]

✅ All Review Waves Passed

All reviewers returned PASS or PASS_WITH_NOTES. This PR is merge-ready.

• Wave 1 (Foundation): ✅
• Wave 2 (Correctness): ✅
• Wave 3 (Quality): ✅
• Wave 4 (Documentation): ✅

Labeled merge:ready. Awaiting auto-merge (hold window: 60 min).