Repository navigation
fix(seed): roll back transaction and exit non-zero when seedOnce entries fail - #2984
wheels-bot[bot] wants to merge 2 commits into
Conversation
…ies fail Seeder.runSeeds() previously committed the wrapping transaction and returned success=true even when individual seedOnce() calls recorded action="failed", so a half-applied seed file looked identical to a fully-applied one in CI and in `wheels seed` output. The framework now tracks totalFailed, rolls back the transaction on any failed entry, names the failing models in the message, and surfaces totalFailed in the response envelope so the CLI bridge can exit non-zero via the existing parseCliResponse path. seedOnce() stays idempotent. Fixes #2973 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…mits-when-individual PR #2987 (d654586, issue #2973) already shipped the equivalent seeder partial-failure fix on develop, so this resolution converges the branch onto develop's implementation: - CHANGELOG.md, vendor/wheels/Seeder.cfc, vendor/wheels/public/views/cli.cfm: conflicts resolved by taking develop's side (#2987 includes the changelog.d/seeder-partial-failure.fixed.md fragment; this branch's direct [Unreleased] edit predates the fragment convention). - vendor/wheels/tests/specs/seederSpec.cfc: restored to develop's version; the auto-merged duplicate spec asserted totalCreated=0 on failure, which contradicts #2987's shipped semantics (pre-rollback attempted count). - vendor/wheels/tests/_assets/seeder/withfailure/: removed; develop's partialfailure asset covers the same scenario. The resulting tree is identical to origin/develop. Signed-off-by: Peter Amiri <peter@alurium.com>
Conflict resolution + disposition summaryRecommendation: close this PR as superseded by #2987. Every functional change here already shipped to develop via #2987 ( Findings addressedNone — this PR had zero reviews, zero comments, and zero review threads (draft; the bot review loop never fired), so there was no reviewer feedback to implement. Conflicts resolved (merge commit
|
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR is superseded. The merge commit at HEAD (5ebb7727b) explicitly resolved conflicts by taking develop's side across all changed files, leaving the PR with zero additions and zero deletions against develop. Merging it would be a no-op. I'm commenting rather than approving or requesting changes because there is no live diff to act on — the fix already shipped via PR #2987. The original fix commit (8f7c3e72b) had two issues worth noting for the record.
Correctness
totalCreated hardcoded to 0 in the failure-return path — the original fix commit set totalCreated = 0 in the rollback branch of runSeeds():
// vendor/wheels/Seeder.cfc (commit 8f7c3e72b, ~line 104)
return {
success = false,
message = "Seeding failed: ...",
totalCreated = 0, // hardcoded
...
};This means callers cannot distinguish "nothing was attempted" from "entries were attempted but then rolled back." PR #2987 correctly returns totalCreated = this.totalCreated so the pre-rollback attempted count is preserved. The new spec in this PR reinforces the wrong contract by asserting expect(local.result.totalCreated).toBe(0) — a spec that would have failed against #2987's shipped implementation. The merge commit body acknowledges this directly: "the auto-merged duplicate spec asserted totalCreated=0 on failure, which contradicts #2987's shipped semantics (pre-rollback attempted count)."
Conventions
Direct CHANGELOG.md edit — the fix commit added a bullet directly under [Unreleased] -> Fixed in CHANGELOG.md. Per CLAUDE.md section "Changelog entries":
User-facing
fix/featPRs add a fragment file, never a directCHANGELOG.mdedit. Direct[Unreleased]edits recreate the same-anchor merge conflicts the fragment system removes.
The correct artifact is changelog.d/<slug>.fixed.md. PR #2987 used changelog.d/seeder-partial-failure.fixed.md correctly; the conflict-resolution merge commit took that side.
The PR checklist marks [x] CHANGELOG.md -- entry added under [Unreleased] as complete, which signals success for the wrong approach. Future bot runs should write a changelog.d/ fragment from the start.
Status
The resulting tree at HEAD is identical to origin/develop. This PR should be closed without merging — the fix for issue #2973 is already live via #2987.
Wheels Bot — Reviewer B (round 1)A's review is accurate and complete. The PR is confirmed as a no-op against SycophancyNone detected. A did not approve; the COMMENTED state and "close without merging" recommendation are appropriate given the empty diff. False positivesNone detected. I verified each claim:
Missed issuesNone detected. Given the diff is empty there is no live code to flag. A's two historical observations (hardcoded counter, wrong changelog artifact) are accurate and serve as useful record-keeping. Commit linting: the original fix commit header ( Verdict alignmentA's COMMENTED verdict is consistent with the finding: no live diff means neither APPROVE nor REQUEST_CHANGES is appropriate. The underlying issue is already resolved upstream. ConvergenceAligned. Both A and B agree: this PR is a no-op superseded by #2987 and should be closed without merging. No code changes are needed on this branch. A human should close the PR. |
Summary
Seeder.runSeeds()previously committed the wrapping transaction and returnedsuccess=trueeven when individualseedOnce()calls recordedaction="failed", so a half-applied seed file looked identical to a fully-applied one in CI and inwheels seedoutput. This change trackstotalFailed, rolls back the transaction when any entry fails, names the failing models inmessage, and bubblestotalFailedthrough the CLI bridge (vendor/wheels/public/views/cli.cfm::runDbSeed) sowheels seedexits non-zero via the existingparseCliResponse→Wheels.Cli.CommandFailedpath.seedOnce()stays idempotent, so the caller can fix the offending entry and re-run safely.The rollback-vs-commit-with-report decision called out in the issue is resolved as rollback, documented in the
runSeeds()docblock. Rationale: the wrapping transaction already signalled intent for atomicity; "half-applied" is the bug being fixed; andseedOnce()'s idempotency makes retry-after-fix safe.Related Issue
Fixes #2973
Type of Change
Feature Completeness Checklist
Signed-off-by:vendor/wheels/tests/specs/seederSpec.cfcplus awithfailure/seeds.cfmfixture; failing → passing locallybot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-up[Unreleased] → FixedTest Plan
vendor/wheels/tests/_assets/seeder/withfailure/seeds.cfmexercises one validseedOnce(author, ...)followed by an invalidseedOnce(user, ...)(user model requiresusername, password, firstname, lastname; supplying onlyusernameforces validation failure).returns success=false and rolls back when a seedOnce() entry fails validationassertsresult.success === false,result.totalFailed >= 1,result.totalCreated === 0, message includes"failed", and the would-have-been-created author cannot be found post-run (rollback verified at the data level, not just the return struct).initializes totalFailed alongside totalCreated and totalSkippedcovers the counter contract oninit().