[cristim]

Summary

Part of #668 (follow-ups from PR #665): AmendLadderTranche returned the plain Validate() error, so non-handler callers could not tell an invalid amendment from a server fault. It now returns an error that matches ErrLadderAmendInvalid via errors.Is and keeps the specific validation message. A new unexported ladderAmendValidationError carries both, because the sentinel's own text is about the run total and per-run cap, which would mislead for "amount must be a positive decimal".

The HTTP handler already validates before calling the store, so API behavior is unchanged.

Not in this PR, with reasons (also on #668)

• FOR UPDATE OF c to FOR NO KEY UPDATE: not done. The run INSERT takes FOR KEY SHARE on the config row, which FOR NO KEY UPDATE does not block, so amend and a run save would stop serializing on the config. The review of PR 670 relied on that serialization. Needs a reviewer-proved ordering analysis first.
• Explicit ON DELETE RESTRICT on ladder_tranche_amendments.tranche_id: it is already the default, and editing the merged migration 000107 is not an option; a no-op follow-up migration is not worth it on its own.
• Timeline read scaling and tolerant handling of mid-load changes in completeTimeline: separate, larger designs (single pass or snapshot read), left for their own PR.

What is verified

Run locally (macOS): go vet ./internal/config, gofmt clean, go test ./internal/config -run 'AmendLadderTranche_Invalid|LadderAmendmentValidation' pass. The new test fails when the wrapping is removed (mutant run). Unit level only: no database, no handler run.

What is not proven

No integration or API test run locally for this change (only the store's pre-database validation path is touched). No deployed check. CI does the full run. Independent review at the head SHA is pending.

Refs #668

Summary by CodeRabbit

• Bug Fixes
  • Invalid ladder amendments now report the specific validation reason while also being identified as invalid amendments. For example, a zero hourly amount reports that the amount must be positive, without adding an unrelated per-run cap message.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 2d494ea6-e327-4cc9-9aee-6ed40dc59ac1

---

📥 Commits

Reviewing files that changed from the base of the PR and between 30d6c84 and 7cd3b42.

---

📒 Files selected for processing (3)

• internal/config/ladder_amend.go
• internal/config/store_postgres_ladder_amend.go
• internal/config/store_postgres_ladder_amend_test.go

---

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

---

📝 Walkthrough

Walkthrough

The change wraps ladder amendment validation failures with ErrLadderAmendInvalid while preserving the original validation message. A test verifies matching, the positive-decimal message, and the absence of unrelated per-run cap text.

Changes

Ladder amendment validation

Layer / File(s)	Summary
Validation error wrapping and coverage internal/config/ladder_amend.go, internal/config/store_postgres_ladder_amend.go, internal/config/store_postgres_ladder_amend_test.go	AmendLadderTranche now wraps validation failures so they match ErrLadderAmendInvalid and retain the underlying validation message. The test covers a zero hourly amount and checks the expected error text.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

---

Merge Risk: ⚪ Minimal · up to 7cd3b

Invalid ladder amendments remain distinguishable with errors.Is while retaining their specific validation messages. No material merge risk is evident.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: mapping invalid ladder amendments to ErrLadderAmendInvalid.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

---

---

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

---

🧪 Generate unit tests (beta)

• Commit to this branch
• 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 (ladder-graph-kimi, Claude Sonnet 5.5; full-diff pass by me, not a model-pinned Opus run) at head 710f0fb.

Verdict: no actionable findings. Approve at this SHA, conditional on CI green.

• Unwrap() []error needs Go 1.20; go.mod is 1.26.6. errors.Is walks both branches (sentinel and the Validate error), and Error() keeps the specific message.
• Handler: handler_ladder_amend.go already calls Validate (line 90 area) before the store, so API behavior is unchanged; the errors.Is(err, ErrLadderAmendInvalid) case would now also return 400 with the specific message for a store-level validation error, as intended. The only store caller is that handler.
• The PostgresStore{} zero value cannot reach the DB: Validate is the first statement of AmendLadderTranche, and the test input (amount "0") fails it.
• Evidence (macOS, main plus the PR's three files via go test -overlay, test-suite lock, no -race): -run 'AmendLadderTranche_Invalid|LadderAmendmentValidation' passes. Mutant return nil, err in place of the wrap fails with "Target error should be in err chain".
• Nit, not blocking: only the amount branch of Validate is exercised; the expected_revision/scheduled_date branch goes through the same wrapper.

Writer's declined items on #668: agree.

• FOR NO KEY UPDATE: correct. In store_postgres_ladder.go, SaveLadderRunWithTranches takes no explicit lock on ladder_configs; the only config-level serialization with an amend is the FK FOR KEY SHARE taken by the run INSERT, which conflicts with FOR UPDATE but not with FOR NO KEY UPDATE. Changing the mode would let a scheduler run save proceed concurrently with an amend's cap check, which the PR 670 review relied on. It would only be safe if the run save also took an explicit config lock first; that is a separate change with its own ordering analysis.
• Explicit ON DELETE RESTRICT: agree not to edit merged migration 000107. (The default is NO ACTION, which differs from RESTRICT only in deferrability; no behavior change for a non-deferrable FK.)

Limits: unit level only, no DB or API run; I did not run CI-dependent checks (CI was still pending at my check). Any new commit restarts the gate.