Skip to content

feat: EnforceAuditSnapshotOnRetryRule (ADR-0001 §Snapshot-on-Retry Safety)#2

Open
Goosterhof wants to merge 2 commits intomainfrom
feat/audit-snapshot-on-retry-rule
Open

feat: EnforceAuditSnapshotOnRetryRule (ADR-0001 §Snapshot-on-Retry Safety)#2
Goosterhof wants to merge 2 commits intomainfrom
feat/audit-snapshot-on-retry-rule

Conversation

@Goosterhof
Copy link
Copy Markdown
Contributor

Summary

Promotes the cross-territory audit-snapshot-on-retry-safety AST harness — currently shipped as a Pest arch test in emmie, entreezuil, ublgenie, and kendo — to a canonical PHPStan rule in this package. Phase 2 promotion per ADR-0021.

What lands

  • EnforceAuditSnapshotOnRetryRule (Rule<MethodCall>) — flags App\Actions\* classes whose constructor injects an entity audit logger and whose $connection->transaction(...) calls do not begin with an in-memory state reset
  • Three compliant first-statement shapes:
    • (a) $model->refresh() or $this->prop->refresh() — updates
    • (b) $model = $this->prop->newQuery()->find*/first* or *->fresh() — deletes / fresh reads
    • (c) $model = new SomeClass(...) or $this->prop->newInstance() — creates
  • if-block escape hatch (mirrors emmie's SoftDeleteDocumentAction shape)
  • Opt-out marker: // @audit-snapshot-retry-safety: <rationale> preceding the transaction call
  • Identifier: enforceAuditSnapshotOnRetry.firstStatementMustResetState
  • Doctrine: ADR-0001 §Snapshot-on-Retry Safety

Design choices vs the original Pest harness

  • Type-based receiver detection via ObjectType::isSuperTypeOf(ConnectionInterface) instead of property-name matching ($this->db vs $this->connection). Removes territory variance as a class.
  • No territory-specific exclusions hardcoded. Per package CLAUDE.md, false positives are suppressed via consumer phpstan.neon ignoreErrors (e.g., ublgenie's BranchCredentialAuditLogger fixed-action-writer exemption migrates from arch-test allowlist to consumer config).
  • Marker scan via raw source. PHPStan/PHPParser does not propagate comment attribution to MethodCall nodes (verified empirically). Falls back to Scope::getFile() upward-line scan, mirroring the kendo arch test verbatim.

Tests

26/26 PHPUnit tests pass. 14 fixtures cover: three compliant shapes, three failure shapes, marker exemption, if-block escape hatch, channel-logger exclusion, non-Action class, non-ConnectionInterface receiver gate.

Parity gate (cross-territory verification)

Ran the rule against three of four consuming territories at their origin/development tips:

Territory Source Violations Notes
kendo post-#1029 worktree 0 parity
entreezuil local HEAD = origin/development 0 parity
ublgenie local HEAD = origin/development 2 territory-specific exemption — see below
emmie not run composer install blocked on stale lockfile; deferred

ublgenie: app/Actions/SaveBranchCredentials.php:36 and :59 — both attributable to ublgenie's arch test hardcoding BranchCredentialAuditLogger as a fixed-action-writer exemption (see ublgenie tests/Arch/AuditTest.php:516-524). The post-cascade migration is mechanical: hardcoded entry → consumer phpstan.neon ignoreErrors. A separate Sapper recon mission has been filed to verify the exemption is doctrinally sound (the null-coalesce $branch->credentials ?? new BranchCredentials shape may indicate a real retry-corruption path, not a true fixed-action writer).

Versioning

Per ADR-0021 SemVer: this is documented expected behavior (consumer-side ignoreErrors for territory-specific false positives). Targeting minor bump (v0.2.0) at the release-cut PR, which moves [Unreleased] to a versioned heading. This PR does not tag.

PHP constraint

Bumped composer.json php: ^8.3^8.4. The package's Pint config (mb_str_functions: true) normalizes ltrim/trim to mb_ltrim/mb_trim (PHP 8.4+ functions). The new rule introduced the first such callsites. All consuming territories run 8.4 — no real-world impact.

Test plan

  • CI passes (PHPUnit + PHPStan self-analysis + Pint check)
  • Reviewer confirms ObjectType::isSuperTypeOf(ConnectionInterface) is the appropriate type-based receiver detection idiom
  • Reviewer confirms raw-source marker scan is acceptable (PHPStan API limitation documented in rule docblock)

References

  • Origin campaign: war-room campaigns/war-room/2026-05-01-audit-snapshot-on-retry-safety-cross-territory.md
  • Doctrine: ADR-0001 §Snapshot-on-Retry Safety
  • Package framework: ADR-0021
  • Cross-territory PRs: emmie #187, entreezuil #139, ublgenie #166, kendo #1029

🤖 Generated with Claude Code

Goosterhof and others added 2 commits May 1, 2026 14:51
…y Safety)

Promotes the cross-territory AST harness from a Pest arch test to a
canonical PHPStan rule. Type-based receiver detection (ConnectionInterface)
replaces territory-specific property-name matching. Discovery filter scopes
to App\Actions\* classes injecting entity audit loggers.

Three compliant first-statement shapes (refresh, fresh-fetch, new-or-newInstance)
plus if-block escape hatch and @audit-snapshot-retry-safety opt-out marker.

Origin: emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029.
Doctrine: ADR-0001 §Snapshot-on-Retry Safety.
Phase 2 of phpstan-warroom-rules per ADR-0021.
The package's Pint config (mb_str_functions: true) normalizes ltrim/trim
to mb_ltrim/mb_trim, which are PHP 8.4+ functions. The new rule introduced
the first mb_ltrim/mb_trim callsites; the composer.json constraint should
match what the formatter actually produces.

All consuming territories already run PHP 8.4 — no real-world impact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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