Distinguish input vs runtime semantic validation failures - #75
Merged
Merged
Conversation
Semantic variable validation could fail at any point in a metamorphosis chain, but all failures raised the same SemanticVariableException with no indication of where they occurred. The first metamorphosis validates the data flowing in from the user-supplied input object (an input error), while any later metamorphosis validates already-validated state (a runtime error signalling an internal inconsistency). These are semantically different and callers should be able to tell them apart. Add two subtypes of SemanticVariableException - InputSemanticVariableException and RuntimeSemanticVariableException - so the distinction is carried by type. Becoming, which is the only component aware of a transformation's position in the chain, refines the base exception into the appropriate subtype on catch, preserving the original as the previous exception. Catching the base type still catches both, keeping existing behaviour intact.
Contributor
📝 WalkthroughWalkthroughAdds ChangesSemantic Validation Origin Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The exception-type contract was only pinned by direct-construction unit tests and by instanceof checks in the integration tests, leaving regression gaps a green suite would not catch: - The cause chain (getPrevious) through Becoming's rewrap was never asserted on the real metamorphosis path, so dropping the $previous argument would silently lose the original validation site. - The array/branching first step (performTypeMatching) had no test proving a validation failure there is classified as an input error. - The chain-close log now records the refined subtype FQCN; nothing pinned the "refine before closeChain" ordering. Add getPrevious/getErrors assertions to the two integration tests, a branching first-step input-classification test, and a chain-close logging test that inspects the emitted becoming_close payload via a real SemanticLogger.
When a constructor body threw the base SemanticVariableException, the inner being_error_close span logged that base class while the outer becoming_close span logged the refined subtype, so a single failure was labelled with two different class names across spans. Keep the chain-close log faithful to the actual error class (matching the inner span) and express the input/runtime distinction as a dedicated `origin` field on BecomingCloseContext instead of swapping the class name. Becoming now logs the original base exception plus origin while still throwing the refined subtype to callers, so the type-based contract is unchanged. - Add ORIGIN_INPUT/ORIGIN_RUNTIME and an optional origin field to BecomingCloseContext, emitted only on semantic-validation error exits. - Thread origin through LoggerInterface::closeChain / Logger::closeChain. - Document origin in the becoming-close schema (enum input|runtime, and forbidden on the success branch). - Update the chain-close logging tests to assert the base error class plus the input/runtime origin.
koriym
marked this pull request as ready for review
June 27, 2026 14:10
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SemanticLog/Context/BecomingCloseContext.php`:
- Around line 41-46: Validate the origin value in BecomingCloseContext before it
reaches jsonSerialize(), since the constructor currently accepts any string but
becoming-close.json only permits input or runtime. Add normalization/validation
in the BecomingCloseContext constructor or a dedicated helper so invalid origin
values are rejected or mapped to an allowed value, and ensure jsonSerialize()
only emits a permitted origin from the existing origin property.
In `@tests/BecomingTest.php`:
- Around line 762-768: The intentional unused constructor parameter in
BecomingTest::__construct is only silencing PHPCS, so PHPMD still flags
UnusedFormalParameter. Update the fixture annotation on the $value parameter to
suppress the PHPMD rule as well, keeping the existing PHPCS ignore in place, so
both analyzers are covered for this deliberate unused Input parameter.
- Around line 304-312: The log-shape validation in becomingCloseContext
currently uses assert(), which can be disabled and allow malformed
SemanticLogger output to slip through. Replace those checks with PHPUnit
assertions in becomingCloseContext so the structure checks on $logData['open'],
$logData['open'][0]['close'], and the type/context fields fail reliably and
clearly before returning the context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7f86b1c-28fe-46fc-9908-ab4108e8376f
📒 Files selected for processing (11)
docs/schemas/becoming-close.jsonsrc/Becoming.phpsrc/Exception/InputSemanticVariableException.phpsrc/Exception/RuntimeSemanticVariableException.phpsrc/Exception/SemanticVariableException.phpsrc/SemanticLog/Context/BecomingCloseContext.phpsrc/SemanticLog/Logger.phpsrc/SemanticLog/LoggerInterface.phptests/BecomingTest.phptests/Exception/InputSemanticVariableExceptionTest.phptests/Exception/RuntimeSemanticVariableExceptionTest.php
… param - becomingCloseContext(): promote the becoming_close type check from an assert() to a real PHPUnit assertion so it still holds when zend.assertions is disabled; keep the is_array() narrowing asserts that match the repo-wide logger-test convention. - BecomingTestRuntimeMiddle: use the #[Input] $value instead of a hardcoded string, removing the unused parameter (and its phpcs:ignore) at the root rather than papering over it with a suppression annotation. The seed value is itself an invalid email, so the second metamorphosis still fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Semantic-variable validation can fail at any point in a metamorphosis chain, but until now every failure raised the same
SemanticVariableExceptionwith no indication of where it occurred.Semantically there are two distinct kinds of failure:
An early idea was to distinguish them with HTTP status codes (400/500), but bringing HTTP semantics into the framework felt out of place. Instead, the distinction is carried by exception subtype.
Changes
SemanticVariableExceptionis kept as the common base type (made non-final, gains an optional?Throwable $previous).catch (SemanticVariableException)still catches both subtypes, preserving backward compatibility.InputSemanticVariableException— validation failed on the first metamorphosis (input error)RuntimeSemanticVariableException— validation failed on a later metamorphosis (runtime error)Becoming::__invoke()is the only component aware of a transformation's position in the chain, so it classifies the failure there: an$isFirstflag selects the subtype on catch, the original exception is preserved as$previous, and the refined subtype is thrown to callers.BecomingArguments/SemanticValidatorare unchanged.Semantic-log consistency (
originfield)The classification is also surfaced in the semantic log without introducing a span/chain class-name asymmetry:
BecomingCloseContextgainsORIGIN_INPUT/ORIGIN_RUNTIMEconstants and an optionaloriginfield (emitted only on semantic-validation error exits).LoggerInterface::closeChain/Logger::closeChainthread theoriginvalue through.being_error_closespan) and expresses input/runtime asorigin, rather than swapping the class name.becoming-close.jsondocumentsorigin(enuminput/runtime, forbidden on the success branch).Tests
BecomingTest: first metamorphosis failure →InputSemanticVariableException; later metamorphosis failure →RuntimeSemanticVariableException. Existing tests that expect the base type still pass.Becomingrewrap path: assertsgetPrevious()chaining andErrorspropagation (not only in direct-construction unit tests).performTypeMatchingis classified as an input error.SemanticLoggeris injected to assert the emittedbecoming_closepayload records the base error class plus the correctorigin.InputSemanticVariableExceptionTest/RuntimeSemanticVariableExceptionTest: covergetErrors(), message building, base-type relationship, and$previouschaining.Verified
composer test— 252 passingcomposer cs-fix— no violationscomposer sa— PHPStan and Psalm both cleancomposer phpmd— cleanBackward compatibility
SemanticVariableExceptionkeeps working, since both subtypes are instances of the base.getErrors()behavior are unchanged.LoggerInterface::closeChaingains a trailing optional parameter only.🤖 Generated with Claude Code
https://claude.ai/code/session_01RPywNci6PvvBNvYYqDA8v8