Release v0.1.3 — Phase 03: Converter Domain Core#59
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…m-skeleton CONV-031: Create Format enum skeleton
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lization-rules CONV-032: Test format normalization rules
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…normalization CONV-033: Implement format normalization
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…contract CONV-034: Create Converter contract
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…target-dto CONV-035: Create ConverterTarget DTO
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rter-implementations CONV-036: Create fake converter implementations
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…registry-skeleton CONV-037: Create ConverterRegistry skeleton
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ts-converters-by-source CONV-038: Test registry lists converters by source
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…y-lookup-methods CONV-039: Implement registry lookup methods
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…hema-field-structure CONV-040: Create options schema field structure
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ma-validation CONV-041: Test options schema validation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-schema-validator CONV-042: Implement options schema validator
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lidator-skeleton CONV-043: Create OptionsValidator skeleton
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ults-and-invalid-values CONV-044: Test options defaults and invalid values
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-validation CONV-045: Implement options validation
📝 WalkthroughWalkthroughThis PR introduces a converter system foundation: a ChangesConverter System and Options Validation
Sequence Diagram(s)sequenceDiagram
participant User
participant Registry
participant OptionsValidator
participant Converter
User->>Registry: targetsFor(format)
activate Registry
Registry->>Registry: forSource(format)
Registry-->>User: ConverterTarget[]
deactivate Registry
User->>Registry: find(source, target)
activate Registry
Registry-->>User: Converter
deactivate Registry
User->>Converter: validateOptions(options)
activate Converter
Converter->>OptionsValidator: validate(schema, options)
OptionsValidator-->>Converter: normalized_options
Converter-->>User: options_result
deactivate Converter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Previously enabled=false with labels listed, which disabled all reviews including release/* → main PRs. Switched to enabled=true so the labels filter actually narrows auto-review to PRs carrying the "release" label. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously enabled=false with labels listed, which disabled all reviews including release/* → main PRs. Switched to enabled=true so the labels filter actually narrows auto-review to PRs carrying the "release" label. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/Unit/Converters/OptionsSchemaFieldTest.php (1)
26-44: ⚡ Quick winAdd a regression case for “no default provided”.
Please add a test that omits
defaultand assertstoArray()does not emit adefaultkey. That locks the intended semantics and prevents validator behavior regressions.🤖 Prompt for 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. In `@tests/Unit/Converters/OptionsSchemaFieldTest.php` around lines 26 - 44, Add a regression test that constructs an OptionsSchemaField without passing the default argument and asserts that toArray() does not include a 'default' key; specifically, in OptionsSchemaFieldTest create a new case similar to the existing "exposes toArray representation" but omit the default parameter when instantiating OptionsSchemaField and assert the returned array equals the expected array that lacks any 'default' entry (use OptionsSchemaField::toArray() as the inspected method and ensure 'options', 'required', and 'help' behave as before).tests/Unit/Converters/ConverterContractTest.php (1)
10-20: ⚡ Quick winStrengthen contract test with signature assertions.
This currently checks only method names. Add assertions for parameter counts/types and return types so contract drift is caught early.
Example addition
foreach ([ 'key', 'sourceFormat', 'targetFormat', 'label', 'description', 'optionsSchema', 'validateOptions', ] as $method) { expect($reflection->hasMethod($method))->toBeTrue(); } + + expect((string) $reflection->getMethod('key')->getReturnType())->toBe('string'); + expect((string) $reflection->getMethod('sourceFormat')->getReturnType())->toBe('string'); + expect((string) $reflection->getMethod('targetFormat')->getReturnType())->toBe('string'); + expect((string) $reflection->getMethod('label')->getReturnType())->toBe('string'); + expect((string) $reflection->getMethod('description')->getReturnType())->toBe('string'); + expect((string) $reflection->getMethod('optionsSchema')->getReturnType())->toBe('array'); + expect($reflection->getMethod('validateOptions')->getNumberOfParameters())->toBe(1); + expect((string) $reflection->getMethod('validateOptions')->getReturnType())->toBe('array');🤖 Prompt for 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. In `@tests/Unit/Converters/ConverterContractTest.php` around lines 10 - 20, The test currently only asserts the presence of methods; enhance it to assert each method's signature by creating ReflectionMethod instances for 'key', 'sourceFormat', 'targetFormat', 'label', 'description', 'optionsSchema', and 'validateOptions' and verifying expected parameter counts via getNumberOfParameters(), checking parameter type hints via getParameters()[index]->getType() (or isBuiltin()/getName()), and asserting return types with hasReturnType()/getReturnType()->getName(); for example ensure no-arg methods return string where applicable and validateOptions accepts the expected parameter type/count and return type—add these checks after the existing hasMethod loop using the reflection symbols above to catch contract drift.
🤖 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 `@app/Support/Converters/DTO/OptionsSchemaField.php`:
- Around line 11-27: The toArray() method on OptionsSchemaField is including a
'default' key even when $this->default is null which makes validators like
OptionsValidator treat an implicit null as an explicit default; change
OptionsSchemaField::toArray() to only include the 'default' key when
$this->default !== null (e.g. build the array and conditionally set 'default' or
use a conditional spread) so that fields without a default do not have the
'default' key and downstream checks (OptionsValidator using array_key_exists)
behave correctly.
In `@app/Support/Converters/OptionsValidator.php`:
- Around line 102-109: The ensureColor method currently accepts 5- and
7-character hex strings because the regex uses a broad range {3,8}; update the
validation in ensureColor to only allow hex lengths 3, 4, 6, or 8 by replacing
the current length quantifier with an alternation that matches exactly those
lengths (keep the optional leading "#" and the same hex character class), so the
preg_match only returns true for valid 3/4/6/8-length hex colors and still
throws InvalidConverterOptionsException::becauseValueIsNotAllowed($key) for all
others.
---
Nitpick comments:
In `@tests/Unit/Converters/ConverterContractTest.php`:
- Around line 10-20: The test currently only asserts the presence of methods;
enhance it to assert each method's signature by creating ReflectionMethod
instances for 'key', 'sourceFormat', 'targetFormat', 'label', 'description',
'optionsSchema', and 'validateOptions' and verifying expected parameter counts
via getNumberOfParameters(), checking parameter type hints via
getParameters()[index]->getType() (or isBuiltin()/getName()), and asserting
return types with hasReturnType()/getReturnType()->getName(); for example ensure
no-arg methods return string where applicable and validateOptions accepts the
expected parameter type/count and return type—add these checks after the
existing hasMethod loop using the reflection symbols above to catch contract
drift.
In `@tests/Unit/Converters/OptionsSchemaFieldTest.php`:
- Around line 26-44: Add a regression test that constructs an OptionsSchemaField
without passing the default argument and asserts that toArray() does not include
a 'default' key; specifically, in OptionsSchemaFieldTest create a new case
similar to the existing "exposes toArray representation" but omit the default
parameter when instantiating OptionsSchemaField and assert the returned array
equals the expected array that lacks any 'default' entry (use
OptionsSchemaField::toArray() as the inspected method and ensure 'options',
'required', and 'help' behave as before).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa5bd67e-8d47-4873-8370-0b4f2c902dbc
📒 Files selected for processing (22)
.coderabbit.yamlapp/Enums/FileFormat.phpapp/Support/Converters/Contracts/Converter.phpapp/Support/Converters/ConverterRegistry.phpapp/Support/Converters/DTO/ConverterTarget.phpapp/Support/Converters/DTO/OptionsSchemaField.phpapp/Support/Converters/Exceptions/InvalidConverterOptionsException.phpapp/Support/Converters/Exceptions/InvalidOptionsSchemaException.phpapp/Support/Converters/Exceptions/UnsupportedFormatException.phpapp/Support/Converters/OptionsSchemaValidator.phpapp/Support/Converters/OptionsValidator.phptests/Fakes/Converters/FakeJpgToWebpConverter.phptests/Fakes/Converters/FakePngToJpgConverter.phptests/Fakes/Converters/FakePngToPdfConverter.phptests/Unit/Converters/ConverterContractTest.phptests/Unit/Converters/ConverterRegistryTest.phptests/Unit/Converters/ConverterTargetTest.phptests/Unit/Converters/FakeConverterTest.phptests/Unit/Converters/FileFormatTest.phptests/Unit/Converters/OptionsSchemaFieldTest.phptests/Unit/Converters/OptionsSchemaValidatorTest.phptests/Unit/Converters/OptionsValidatorTest.php
Comment on lines
+11
to
+27
| public mixed $default = null, | ||
| public array $options = [], | ||
| public bool $required = false, | ||
| public ?string $help = null, | ||
| ) {} | ||
|
|
||
| public function toArray(): array | ||
| { | ||
| return [ | ||
| 'key' => $this->key, | ||
| 'type' => $this->type, | ||
| 'label' => $this->label, | ||
| 'default' => $this->default, | ||
| 'options' => $this->options, | ||
| 'required' => $this->required, | ||
| 'help' => $this->help, | ||
| ]; |
There was a problem hiding this comment.
Avoid serializing an implicit null default as an actual default value.
toArray() currently always includes default, so fields with no intended default are treated as having one. Because OptionsValidator checks array_key_exists('default', $field), this changes validation behavior and can bypass required checks or feed null into type normalizers.
Suggested fix
public function toArray(): array
{
- return [
+ $payload = [
'key' => $this->key,
'type' => $this->type,
'label' => $this->label,
- 'default' => $this->default,
'options' => $this->options,
'required' => $this->required,
'help' => $this->help,
- ];
+ ];
+
+ if ($this->default !== null) {
+ $payload['default'] = $this->default;
+ }
+
+ return $payload;
}🤖 Prompt for 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.
In `@app/Support/Converters/DTO/OptionsSchemaField.php` around lines 11 - 27, The
toArray() method on OptionsSchemaField is including a 'default' key even when
$this->default is null which makes validators like OptionsValidator treat an
implicit null as an explicit default; change OptionsSchemaField::toArray() to
only include the 'default' key when $this->default !== null (e.g. build the
array and conditionally set 'default' or use a conditional spread) so that
fields without a default do not have the 'default' key and downstream checks
(OptionsValidator using array_key_exists) behave correctly.
Comment on lines
+102
to
+109
| private function ensureColor(string $key, mixed $value): string | ||
| { | ||
| if (! is_string($value) || ! preg_match('/^#?[0-9a-fA-F]{3,8}$/', $value)) { | ||
| throw InvalidConverterOptionsException::becauseValueIsNotAllowed($key); | ||
| } | ||
|
|
||
| return $value; | ||
| } |
There was a problem hiding this comment.
Color regex accepts invalid hex lengths (5 and 7).
{3,8} matches lengths 3–8, but valid hex colors are only 3 (RGB), 4 (RGBA), 6 (RRGGBB), or 8 (RRGGBBAA). A 5- or 7-character string like #abcde passes validation and flows downstream as a valid color.
🛡️ Proposed fix to restrict to valid lengths
- if (! is_string($value) || ! preg_match('/^#?[0-9a-fA-F]{3,8}$/', $value)) {
+ if (! is_string($value) || ! preg_match('/^#?(?:[0-9a-fA-F]{3,4}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})$/', $value)) {
throw InvalidConverterOptionsException::becauseValueIsNotAllowed($key);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private function ensureColor(string $key, mixed $value): string | |
| { | |
| if (! is_string($value) || ! preg_match('/^#?[0-9a-fA-F]{3,8}$/', $value)) { | |
| throw InvalidConverterOptionsException::becauseValueIsNotAllowed($key); | |
| } | |
| return $value; | |
| } | |
| private function ensureColor(string $key, mixed $value): string | |
| { | |
| if (! is_string($value) || ! preg_match('/^#?(?:[0-9a-fA-F]{3,4}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})$/', $value)) { | |
| throw InvalidConverterOptionsException::becauseValueIsNotAllowed($key); | |
| } | |
| return $value; | |
| } |
🤖 Prompt for 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.
In `@app/Support/Converters/OptionsValidator.php` around lines 102 - 109, The
ensureColor method currently accepts 5- and 7-character hex strings because the
regex uses a broad range {3,8}; update the validation in ensureColor to only
allow hex lengths 3, 4, 6, or 8 by replacing the current length quantifier with
an alternation that matches exactly those lengths (keep the optional leading "#"
and the same hex character class), so the preg_match only returns true for valid
3/4/6/8-length hex colors and still throws
InvalidConverterOptionsException::becauseValueIsNotAllowed($key) for all others.
- OptionsSchemaField::toArray() now omits the "default" key when no default
was provided, so OptionsValidator's array_key_exists("default", …) check
no longer treats an implicit null as an explicit default.
- OptionsValidator::ensureColor() restricts hex lengths to 3, 4, 6, or 8;
previously {3,8} let 5- and 7-character strings pass.
- Add regression test asserting toArray() omits "default" when not set.
Addresses CodeRabbit review on PR #59.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- OptionsSchemaField::toArray() now omits the "default" key when no default
was provided, so OptionsValidator's array_key_exists("default", …) check
no longer treats an implicit null as an explicit default.
- OptionsValidator::ensureColor() restricts hex lengths to 3, 4, 6, or 8;
previously {3,8} let 5- and 7-character strings pass.
- Add regression test asserting toArray() omits "default" when not set.
Addresses CodeRabbit review on PR #59.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…view-config INFRA-002: Fix CodeRabbit auto_review config
…-color-fixes CONV-046: Fix OptionsSchemaField null default and tighten color regex
…se03-converter-domain-core
This was referenced May 31, 2026
Merged
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
1 participant
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.
Summary
Phase 03 — Converter Domain Core (CONV-031 → CONV-045).
Introduces:
App\Enums\FileFormatwith normalization (jpeg→jpg, uppercase, leading dot) andUnsupportedFormatExceptionConvertercontract +ConverterTargetDTOConverterRegistrywithall/forSource/find/targetsForOptionsSchemaFieldDTO,OptionsSchemaValidator,OptionsValidatortests/Fakes/ConvertersNo real conversion drivers, file upload, billing, or API in this phase.
Test plan
composer test— 113 passedcomposer lint— passednpm run build— passedphp artisan migrate:fresh— clean🤖 Generated with Claude Code
Summary by cubic
Builds the Converter domain core: format normalization, converter contract/registry, DTOs, and options/schema validation (CONV-031 → CONV-046). Also tightens color option validation and restricts
CodeRabbitauto-review to PRs labeled "release" (INFRA-002).New Features
App\Enums\FileFormatnormalizes inputs (jpeg→jpg, case, leading dot) and throwsUnsupportedFormatException.Converterinterface andConverterRegistry(all,forSource,find,targetsFor) withConverterTargetDTO.OptionsSchemaFieldDTO plusOptionsSchemaValidatorandOptionsValidatorfor supported types (select, segmented, toggle, color, number, range), defaults, required fields, allowed values, and boolean/number/color normalization with clear errors.Bug Fixes
OptionsSchemaField::toArray()omitsdefaultwhen not set to avoid treating null as a default.OptionsValidatorcolor check now only accepts 3/4/6/8-digit hex codes..coderabbit.yaml: enable auto-review with label filter so reviews run only on PRs labeled "release".Written for commit 7d10b59. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
New Features
Chores