Skip to content

docs(release): reconcile v1.4.0 vs Unreleased moveWithinScope() ordering contract - #61

Merged
megyptm merged 3 commits into
mainfrom
claude/59-ordering-contract-reconcile-f43965
Oct 21, 2026
Merged

megyptm merged 3 commits into
mainfrom
claude/59-ordering-contract-reconcile-f43965

Conversation

@megyptm

@megyptm megyptm commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Documentation only. No change to src/, CI, runtime or public API; no version chosen or stamped; Unreleased not globally replaced.

What changed

  • F37-01 README Availability is split into API availability and behavior changes of an API that already exists in v1.4.0; the fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 caller-owned change (own shifts undone on failure/false vs. left for commit() in v1.4.0) is listed there. The Package Reference header now separates Unreleased API from Unreleased behavior changes of moveWithinScope(), with the 1:1 2:3 3:4 4:5 5:5 example. The How-to guide Availability box points to fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55.
  • F37-02 CHANGELOG no longer says "exception types unchanged". moveWithinScope() in a caller-owned transaction can now throw TransactionExecutionException (class exists in v1.4.0; the v1.4.0 method never threw it, its runner has no such throw). Sources: SAVEPOINT returns false, the dead-transaction guard, ROLLBACK TO returns false behind false, RELEASE returns false after a good write. Guard text now says it does not restore a server-ended transaction. README, Reference, ADR 0005 (decision 1) and the guide are aligned. The method docblock already says @throws Throwable and describes the guard, so it is untouched (src stays out of scope).
  • F37-03 "exactly two statements" is qualified as the success path in README, CHANGELOG, Reference, ADR 0005 and the guide. Failed paths also run ROLLBACK TO SAVEPOINT and attempt RELEASE.
  • F37-04 [Unreleased] sections reordered to Added, Changed, Deprecated, Fixed (Keep a Changelog), still on top, no version.
  • Narrow regression guard in MoveWithinScopeSavepointDocumentationTest (4 tests: no "exception types" claim, success-path qualification, README/Reference list fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 + TransactionExecutionException, section order).

Evidence: statement counts, caller-owned transaction, MySQL 8.4.10 (log_bin_trust_function_creators=1)

Script: FaultInjectingPdo (client-side attempted control statements) plus server Com_* session counters. "false" injections make the client exec() return false without sending it.

Case Result Attempted Server executed
success, mutating true SAVEPOINT, RELEASE 1 / 0 / 1
no-op; missing row true; false none none
1062 on write, cleanup ok PDOException 1062 SAVEPOINT, ROLLBACK TO, RELEASE 1 / 1 / 1
target write changes no row (trigger), undo ok false SAVEPOINT, ROLLBACK TO, RELEASE 1 / 1 / 1
1062 + ROLLBACK TO fails PDOException 1062 (original wins) SAVEPOINT, ROLLBACK TO 1 / 0 / 0
1062 + RELEASE fails PDOException 1062 SAVEPOINT, ROLLBACK TO, RELEASE 1 / 1 / 0
false + ROLLBACK TO fails TransactionExecutionException SAVEPOINT, ROLLBACK TO 1 / 0 / 0
false + RELEASE fails false SAVEPOINT, ROLLBACK TO, RELEASE 1 / 1 / 0
success + RELEASE fails TransactionExecutionException (move undone) SAVEPOINT, RELEASE, ROLLBACK TO, RELEASE 1 / 1 / 0
SAVEPOINT returns false TransactionExecutionException SAVEPOINT none

(Server columns are SAVEPOINT / ROLLBACK TO / RELEASE.) Raw output and script are in the shared notes folder (issue-59-statement-counts.txt, issue-59-measure.php). v1.4.0 check: git show v1.4.0:src/Pdo/Transaction/PdoTransactionRunner.php has no TransactionExecutionException; its run() just calls the callback when a transaction is active.

Verification (CONTRIBUTING.md sequence, real MySQL 8.4.10, all exit 0)

composer validate --strict, dump-autoload --strict-psr, check-platform-reqs, audit, analyse (phpstan), test:unit (379 tests), test:regression (90), test:integration (200), test:consumer, php-cs-fixer dry-run (0 of 100), git diff --check.

BC impact: none (documentation and a docs test). Base: origin/main e4aba20 (unchanged at start). Not touched: packaging, .gitattributes, CI.

Review round 1 (F61-01..F61-04) corrections, second commit

  • F61-01: README comparison now separates the throwing path (undo best effort, original exception rethrown) from the false path (undo must succeed, then false, no exception; undo failure is thrown). The v1.4.0 cell is qualified: partial shifts persist only if shifts had been executed and the transaction survives. Checked against ScopedOrderingManager.php 317-346 and OperationSavepoint::undoOrFail().
  • F61-02: cleanup counts are limited to a failure after the savepoint was established (ROLLBACK TO, then RELEASE attempted; skipped when the driver reports no active transaction; no universal count). Invalid argument, missing row, no-op and failing SAVEPOINT add no ROLLBACK TO/RELEASE. Applied in README (table and prose), CHANGELOG, Reference, guide, ADR 0005. New measurement: invalid argument inside a caller-owned transaction throws InvalidOrderingOperationException with 0 savepoint statements (earlier table already has no-op, missing row and SAVEPOINT failure).
  • F61-03: tests slice changelog sections by the next ## [ heading (not hard-coded [1.4.0]); the fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 wording guards use a stated transition rule (entry in exactly one section; TBD by the Owner required while under [Unreleased], a real ## [x.y.z] - date heading otherwise); statement-count guard is claim-local (each count sentence needs success-path and post-savepoint cleanup wording within its window); order test rejects unknown headings, skips explicitly when [Unreleased] is empty; unused local removed; synthetic-heading unit test added.
  • F61-04: Reference header separates Unreleased API from the Unreleased deprecation of released classes; "Cannot throw" replaced by the narrower fact that the v1.4.0 method issued no savepoint control statement.

RED/GREEN: new tests vs the previous docs: 4 failures (exception wording, count locality, throw/false split, API vs deprecation); GREEN on the corrected docs (45 docs tests).
Release-transition scratch checks (not committed): inserting ## [9.9.9] - 2099-01-01 above [1.4.0] with its own Fixed/Added headings: all pass, Unreleased unaffected. Moving everything under a new version heading: pass, order test skipped explicitly. Entry duplicated in two sections: fails. Misspelled heading in Unreleased: fails.
Not changed: DeadlockRetryDocumentationTest still slices at ## [1.4.0] for its own (deprecation) assertion; it stays correct after a release section is added, so it is left alone.

Gates re-run on MySQL 8.4.10: validate, dump-autoload, platform, audit, phpstan, unit 379, regression 93, integration 200, consumer, php-cs-fixer, diff --check all exit 0; container removed.

Review round 2 (F61-R2-01..03), third commit

  • R2-01: README rollback-ownership bullet rewritten: throws (best-effort rollback to savepoint, original exception rethrown, cleanup failure leaves shifts, original wins) vs changes no row (no exception exists; rollback must succeed, then false; undo failure thrown instead of false). Server-aborted and scope-lock qualifications kept; broken line wrap normalized. Same split applied to the README Reference-style summary, the Reference ownership table and the How-to guide.
  • R2-02: CHANGELOG fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 opening sentence no longer gives the false path an original exception. The docs: correct ordering UNIQUE, transaction rollback, 1213 and unreleased API guidance #46 entry now says the "no savepoint guarantee" text was the interim documentation state superseded by fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 on this release. The deprecation entry's "keeps its behavior" is scoped to the runner-generation change.
  • R2-03: the changelog fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 guards now use the entry bullet alone, not the whole section. New targeted guards: README bullet (both outcomes present, ordered, no "original exception" after the false outcome, old "(or false after the shifts)" forbidden), changelog entry opening, superseded/unchanged claims, Reference/README summaries.
  • Consistency check: README, CHANGELOG, Reference, ADR 0005 and the guide were grepped for every corrected claim. The moveWithinScope() docblock (not edited, src/ untouched) still groups "a Throwable ... or false" in one sentence and says it rethrows the original Throwable; it is comparison only and could get a docblock-only follow-up.

RED (new guards vs previous docs): 4 failures of 49. GREEN: OK (49 tests, 458 assertions).
Probes: restoring the old changelog opening while keeping the later qualifying parentheses fails 1 test; regressing the README bullet to "(or false after the shifts)" fails 1 test.
Gates on MySQL 8.4.10 (container removed): validate, dump-autoload, platform, audit, phpstan, unit 379, regression 97, integration 200, consumer, php-cs-fixer, diff --check all exit 0.

Ref #59, Ref #37

🤖 Generated with Claude Code

megyptm commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Technical Lead — Independent Cumulative Review #1 — CHANGES REQUIRED (2026-10-21)

Pinned review identity: base=e4aba2043fe0956d7097621d22dfb23e4efbbcfa, HEAD=005db329ac13e57bc11a07ba2fd3d9ee35aeff02, Tree=2d6253238c18ca1bfc2742b28f2289232c8bcf19; 1 commit, 6 changed files (+137/-31). Reviewed all six file patches and their surrounding current text against actual src/Pdo/Ordering/ScopedOrderingManager.php, src/Pdo/Ordering/Internal/OperationSavepoint.php, released v1.4.0 contract, AGENTS.md and applicable currently pinned standards. The reviewed changes are only documentation and one documentation-regression test; no src/, CI, Composer, version or standards-pin change. Exact-head GitHub Actions run 39642828630: 10/10 completed SUCCESS (quality, integration, lowest dependencies, unit/regression 8.2–8.5, workflow lint and CI gate). This is reported CI evidence, not an independent re-execution of the executor's local MySQL script.

Verdict: CHANGES REQUIRED / NO MERGE GO. The main F37-01/02/03 documentation corrections are directionally right, but all findings below need one consolidated correction pass, not serial blocker-by-blocker review.

F61-01 — inaccurate combined throws / false behavior in the newly added README comparison (required)

README.md:85 combines a range shift/target write that throws, and a target write returning false, then says for the new behavior “the original exception is still the one thrown.” There is no original exception in the false path: ScopedOrderingManager.php:338–352 calls undoOrFail() and returns false via its private sentinel if undo succeeds; it only throws if undo fails. The same row also implies v1.4.0 always leaves partial shifts for commit() on any exception, which is only true when shifts actually happened and the outer transaction survives (not, e.g., an error before any shifted rows or a server-aborted transaction). Fix: distinguish the throwing path from the false path, and qualify the old outcome by partial-work and surviving-transaction preconditions. Preserve the key #55 upgrade warning.

F61-02 — failure-path SQL statement counts overgeneralized (required)

Current new text says “**a failed path also runs ROLLBACK TO SAVEPOINT and attempts RELEASE” in README.md:87,286–290, CHANGELOG.md:27, PERSISTENCE_PACKAGE_REFERENCE.md:145+, docs/guides/MOVE_WITHIN_SCOPE_HOW_TO_USE.md:153–155, docs/adr/0005...md:40. Not every failed call enters a saved mutation window. An invalid precondition, missing target, or a SAVEPOINT that fails to start do not issue a ROLLBACK TO; OperationSavepoint::cleanupAfterFailure():59–60 skips cleanup when the driver's transaction has ended. The PR itself reports a “SAVEPOINT returns false” case with no ROLLBACK TO. Fix: explicitly limit rollback/release counts to a failure after a savepoint was established and cleanup could run in an active transaction. Document the distinctions for false, original thrown error, initial SAVEPOINT failure, and aborted/failed cleanup without promising universal statement counts. The existing two-statement successful mutating path claim is appropriate.

F61-03 — newly extended changelog regression guards are not release-safe (required)

In tests/Regression/Docs/MoveWithinScopeSavepointDocumentationTest.php:179–185, unreleasedChangelog() slices until hard-coded ## [1.4.0], not the next release heading. When the next version section is inserted between [Unreleased] and [1.4.0] during the imminent release work, the helper includes that released version inside what the tests call “Unreleased.” This can incorrectly pass testChangelogDoesNotClaim... / testChangelogRecordsTheBehaviorChangeUnderUnreleased even when the wording exists only in the released section, and may falsely fail the new section-order test because headings from multiple versions are mixed. The older test near line 104 independently uses the same brittle boundary.

Further, testStatementCountIsQualifiedAsTheSuccessPathEverywhere() searches success path and ROLLBACK TO SAVEPOINT anywhere in each file; these occurrences need not qualify the specific statement-count claim that the test title promises to guard. testUnreleasedChangelogSectionsFollowKeepAChangelogOrder() casts an unknown section's array_search(false) to index 0, potentially accepting an unrecognized heading; its $changelog local is unused.

Fix: make the Unreleased-section extraction structurally bounded by the next version header; give pre-release #55 wording guards an intentional version-transition rule rather than silently accepting a release section as Unreleased; strengthen the claim-local statement-count checks and validate known changelog headings. Demonstrate a small deterministic regression case with a synthetic next-version heading (no new version stamped in the actual repository). Do not weaken existing guards merely to make the next release green.

F61-04 — API/exception classification requires precise scope (release-facing accuracy)

PERSISTENCE_PACKAGE_REFERENCE.md:18–25 places deprecation of already published classes under “Unreleased API (not in v1.4.0)”; the two classes are in v1.4.0, while their deprecation annotation/status is unreleased. Separate API availability from deprecation state, as the new README table generally does. In README.md:86, “v1.4.0 Cannot throw TransactionExecutionException” is categorical for an API that takes an externally supplied PDO; instead state the narrower verified fact: the v1.4.0 implementation did not itself introduce a savepoint-control-origin TransactionExecutionException through this method, while the new path can do so. Avoid implying an impossible external Throwable from injected PDO behavior.

Requirements for the single remediation iteration

  • Fix all four groups together within docs(release): reconcile v1.4.0 vs Unreleased ordering failure contract before next stable release #59/PR docs(release): reconcile v1.4.0 vs Unreleased moveWithinScope() ordering contract #61, preserving narrow docs + regression-test scope; no Runtime, API, CI, adoption, tag or release modification.
  • Validate final wording against both released v1.4.0 and current code, with an explicit distinction between normal return-false, caught Throwable, SAVEPOINT initialization, rollback failure and server-aborted transaction.
  • Provide targeted RED/GREEN for the updated documentation guards where practical, and full applicable CONTRIBUTING.md checks / exact-head GitHub CI evidence. Ensure a future release-section transition cannot make the tests lie.
  • Push a new commit (no amend/force-push), update PR description if claims/evidence change, and report exact new HEAD/Tree and all findings resolved together. Do not merge.
  • I will then perform a Fresh Full Re-Review of the entire accumulated final PR diff (not only these fixes) before any MERGE GO proposal to Owner. Docs-only PR: no runtime flowchart is needed just for decoration.

#60 remains a separate parallel workstream.

…tract wording and changelog guards (Ref #59, #37)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

megyptm commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Technical Lead — Fresh Full Re-Review #2, full cumulative PR #61 — CHANGES REQUIRED (2026-10-21)

Immutable review refs: exact Base = e4aba2043fe0956d7097621d22dfb23e4efbbcfa (also current origin/main, so no unrelated-base drift); HEAD = 507dec09b0630121b5c4672dc4da33d1d93d4518; Tree = 3f710c1553120f37ac9b928e86224813f7077e0e. Two ordinary commits, all 6 cumulative changed files inspected, +241/-35. No src/, Composer metadata, CI, Runtime, public API or pinned standards changes. Reviewed against AGENTS.md, the locally pinned standards (Adoption Manifest VALID, pin 44c8827095ab4007c355aa21c56b853f3b49d795), ScopedOrderingManager::moveWithinScope() actual code, OperationSavepoint, ADR 0005 and the published v1.4.0 contract.

Exact HEAD verification: GitHub Actions run 39647210305, 10/10 completed SUCCESS, including Unit/Regression PHP 8.2/8.3/8.4/8.5, MySQL 8.4.10 Integration PHP 8.2/8.5, Quality, Lowest dependencies, Workflow lint, CI Gate. The executor's local MySQL RED/GREEN and synthetic release-heading checks are cited execution evidence; I independently inspected the code/diff/CI, not a rerun of those local tests. Previous F61-01, F61-02, F61-03, F61-04 are addressed in the specific originally reviewed locations. The newly added Changelog next-header parser and synthetic next-version unit case repair the originally reported version-boundary weakness.

Verdict: CHANGES REQUIRED — NOT MERGE GO. The fresh whole-document review found residual contract contradictions that must be reconciled together in the existing PR, not split into more serial blockers:

F61-R2-01 (required) — README still conflates a throwing failure and a false return outside its corrected comparison table

  • README.md:277–285 describes a range-shift / target-write failure “(or false after the shifts),” and then universally says “the original exception is rethrown unchanged” and “If that cleanup itself fails, the original exception still wins.”
  • The false path has no original exception: ScopedOrderingManager.php:338–352 calls OperationSavepoint::undoOrFail(), then returns false on successful undo; if undo fails, that failure propagates. Exception path (:332–336) preserves original Throwable after best-effort cleanup. The newly fixed comparison row in README.md:85 is accurate, but the neighboring consumer rollback guidance is inconsistent.
  • Required: rewrite that one pre-existing paragraph narrowly to clearly separate Throwable and returned-false semantics, retaining the server-aborted transaction and scope-lock qualifications. Also normalize the visibly broken line wrap README.md:297–298 while touching it.

F61-R2-02 (required) — two unreconciled, misleading items in the CURRENT [Unreleased] CHANGELOG

F61-R2-03 (verification guard, same scoped fix)

  • The new testReadmeComparisonSeparatesTheThrowingPathFromTheFalsePath() only asserts that Throwing path: and ``false path: occur somewhere in the README; it passed while the actual README rollback-ownership paragraph contradicts them. The test's scope/name promises a comparison, so keep that comparison guard and add a narrowly targeted guard preventing the specific original-exception/false-path conflation in the consumer rollback-ownership section. The fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 changelog test similarly checks a whole version section; verify a malformed main fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 entry cannot pass solely because a corrective sentence appears elsewhere in the same section.
  • Show targeted failing test/probe for a regression that reproduces the wrong wording, then GREEN, without merely checking a vague keyword over a huge text span; do not overfit historical prose or require already-approved release version changes. Preserve existing section-boundary regression (future version test) and no new runtime behavior.

Acceptance / ownership

Resolve all three groups in one consolidated change on Draft PR #61, including affected docs and narrowly relevant docs regression tests only. No Runtime, public API, CI, Composer, standards pin, release version/tag/release change. Re-run applicable CONTRIBUTING.md gates; provide new ordinary commit (no amend or force push), exact HEAD/Tree, and linked checks. Do not merge. After correction I must perform another independent Fresh Full Re-Review of the entire final cumulative diff before MERGE GO is even considered by the Owner.

Note: this is not a demand to refactor #55 Runtime or reopen SemVer prematurely; this is a release-facing accuracy check in #59's agreed scope. #62 can continue independently. No merge authorized.

…gelog claims (F61-R2-01..03; Ref #59, #37)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

megyptm commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Technical Lead — Fresh Full Re-Review #3: MERGE GO (Owner approval required)

Reviewed exact final cumulative state (not fix-only): Base e4aba2043fe0956d7097621d22dfb23e4efbbcfa = current origin/main, HEAD b08fef9be08b42fe393ee4cc328ba9d94cce2bbe, Tree 859adaeeb5aebcb3ce379c08289b1d48d375efe8; 3 linear commits, 6 changed files, +317/−52 lines. All six file diffs and surrounding document contexts independently reviewed against AGENTS.md, the existing VALID pinned Engineering Standards adoption 44c8827095ab4007c355aa21c56b853f3b49d795, actual ScopedOrderingManager::moveWithinScope() and OperationSavepoint, ADR 0005 and the stable v1.4.0 behavior. No modifications to src/, composer.json, CI, Runtime, public signatures, adoption manifest or release version.

All findings reconciled together:

  • Original F61-01..F61-04: closed by second commit, with explicit throwing vs nonthrowing false paths, changed exception reachability, successful vs failed savepoint statement counts, proper Unreleased API vs deprecation classification, Keep a Changelog ordering and version-transition-aware tests.
  • F61-R2-01: README rollback ownership paragraph now separates thrown Throwable (original wins; best-effort undo) from no-row false (undo must succeed; otherwise failure thrown) and preserves server-aborted/scope-lock caveats.
  • F61-R2-02: CHANGELOG.md fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 introduction likewise distinguishes outcomes; previous Issue docs: correct ordering UNIQUE, transaction rollback, 1213 and unreleased API guidance #46 "no savepoint guarantee" explicitly labeled superseded for moveWithinScope(), while deprecation's unchanged-behavior claim is scoped to runner generation and package-owned path.
  • F61-R2-03: added targeted regression checks for README rollback-ownership bullet, the specific fix(ordering): atomic partial-shift rollback in caller-owned moveWithinScope #55 entry in CHANGELOG, superseded versus current claims, README/Package Reference summaries; the earlier future-release changelog-header guard is preserved. Reviewed actual test names, behavior and assertions; no demonstrated misrepresentation or executable guard defect that blocks this release-prep documentation change.

Exact-head verification now confirmed: Actions run 39650260962 is completed SUCCESS, 10/10 jobs: Workflow lint, Quality, Lowest Dependencies with MySQL 8.4.10, Unit/Regression on PHP 8.2/8.3/8.4/8.5, Integration PHP 8.2/8.5 with MySQL 8.4.10, CI Gate. The executor's local real-MySQL CONTRIBUTING gates, RED (4 guards failed against previous docs), GREEN (49 docs tests/458 assertions) and consumer proof are separately reported in this PR; they were not independently re-executed by the Lead. The GitHub CI and exact HEAD/Tree were independently verified.

Non-blocking observation for the later whole-repository standards audit (Issue #37 Step 4): the untouched source docblock in src/Pdo/Ordering/ScopedOrderingManager.php:236–242 still compresses the Throwable and false paths into one paragraph, though it explicitly states that undo failure behind false is thrown instead of returning false. Its context does not establish a new Runtime/API defect, and touching src/ would exceed this PR's documentation-only scope. Reinspect docblock precision during the mandated whole-repository final audit; do not silently change Runtime here.

Decision: MERGE GO for this exact Tree only, pending explicit Owner authorization. No version, Tag or Release GO. No runtime follow chart required for a documentation-and-documentation-regression-only PR. If HEAD/Tree or relevant base changes, reassess and renew Fresh Full Re-Review before merge. PR #62 remains independent and does not block merging this reviewed #61.

After Owner-authorized merge: reconcile the actual merge SHA, resultant tree, updated main, Issue #59 status, Issue #37 release tracker and any branch cleanup; keep the final release gate separately closed to automatic publication.

@megyptm
megyptm marked this pull request as ready for review October 21, 2026 18:16
@megyptm
megyptm merged commit b3ec918 into main Oct 21, 2026
10 checks passed
@megyptm
megyptm deleted the claude/59-ordering-contract-reconcile-f43965 branch October 21, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant