Conversation
📝 WalkthroughWalkthroughRefactors the Asset aggregate to use value objects, adds upload-target and completion-proof domain contracts, extends the repository with uploadId lookup, introduces storage adapter/upload enums, and adds comprehensive unit tests and documentation for US-02. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API Server
participant Storage as StorageAdapter
participant Repo as AssetRepository
Client->>API: request upload creation (uploadId, file meta)
API->>Repo: createPending(asset with fileName,mimeType)
API->>Storage: createUploadTarget(asset)
Storage-->>API: UploadTarget(url, method, signedHeaders, completionProofSpec, expiresAt)
API-->>Client: return UploadTarget
Client->>Storage: perform upload to UploadTarget (PUT with signed headers)
Note right of Storage: Storage accepts upload and produces proof (e.g., response header)
Storage-->>Client: upload response including proof
Client->>API: notify upload completion with completionProofValue
API->>Repo: markUploaded(assetId, completionProofValue)
Repo-->>API: persist uploaded state (including completionProof)
API-->>Client: confirm upload recorded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
hsc00
changed the title
9 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Around line 82-89: The "Feedback Carry-Forward" section uses undefined terms
and an overly long sentence: add concise definitions for REQUEST CHANGES and
DECLINE at first mention, and either split the long sentence referencing "scoped
instruction", "skill", "workspace instructions", and "agent files" into two
sentences or move short definitions into a new "Customization Surfaces"
subsection (or inline footnote) so each surface is briefly explained and the
guidance reads clearly; update the paragraph in the "Feedback Carry-Forward"
block to reference those definitions and simplify the wording accordingly.
In `@docs/logs/02-domain-model-core-objects.md`:
- Line 49: The file ends without a trailing newline which triggers markdownlint
MD047; open the document (the last line containing "Later application and
infrastructure work should consume these contracts directly instead of
introducing parallel repository or storage shapes.") and add a single newline
character at end-of-file so the file terminates with a newline.
In `@src/Domain/Asset/ValueObject/UploadCompletionProof.php`:
- Around line 7-18: The constructor of UploadCompletionProof currently throws a
fully-qualified \InvalidArgumentException; import InvalidArgumentException at
the top and replace the FQCN usage in the __construct method with the imported
class to match project style—update the use statements and ensure the throw in
UploadCompletionProof::__construct(string $name, UploadCompletionProofSource
$source) uses the imported InvalidArgumentException.
In `@src/Domain/Asset/ValueObject/UploadTarget.php`:
- Around line 112-119: In isAllowedLocalDevelopmentUrl(string $scheme, string
$host) remove the redundant trim call and use the already-normalized $host
directly: keep the scheme check (if ($scheme !== 'http') return false;) and then
return in_array($host, ['localhost', '127.0.0.1', '::1'], true) so you rely on
the prior normalization done before this method (refer to
UploadTarget::isAllowedLocalDevelopmentUrl and the earlier host normalization
where $host is lowercased and bracket-stripped).
- Around line 1-10: The UploadTarget value object currently references
\InvalidArgumentException repeatedly; add a top-level import "use
InvalidArgumentException;" in the UploadTarget class namespace declarations and
then replace all occurrences of "\InvalidArgumentException" in the class (e.g.,
in the constructor and any validation methods) with the unqualified
"InvalidArgumentException" to match project conventions and remove repetition.
In `@tests/Unit/Domain/Asset/AssetTest.php`:
- Around line 101-129: Update the test
itReconstitutesUploadedAssetWithPersistedValues to also assert that the
completion proof was preserved: after reconstituting via
Asset::reconstituteUploaded, call $asset->getCompletionProof() and assert it
equals the proof created by $this->createCompletionProofValue() (and do the same
for the other affected test around lines 187-198); this ensures
Asset::reconstituteUploaded and Asset::markUploaded correctly retain the
completion-proof state.
- Line 32: Rename the test method
itCreatesPendingAssetWithGeneratedIdentifierAndRequiredMetadata to follow the
repository test-naming patterns—for example
itReturnsPendingAssetWhenCreatedWithGeneratedIdentifierAndRequiredMetadata—and
similarly rename the other flagged methods (lines referenced: 72, 102, 171, 188,
227) to one of the allowed shapes (itReturns...When..., itThrows...When...,
itEmits...When..., or itDoesNot...When...) while keeping their assertions and
visibility intact; update any usages (data providers, `@depends` annotations, or
test references) to the new method names so tests still run unchanged.
In `@tests/Unit/Domain/Asset/ValueObject/AssetIdTest.php`:
- Around line 82-84: Remove the extra blank line immediately before the final
closing brace of the AssetIdTest class; edit the class AssetIdTest so the
closing "}" directly follows the preceding line (no blank line between the last
statement/method and the class closing brace) to comply with styling/formatting.
In `@tests/Unit/Domain/Asset/ValueObject/UploadTargetTest.php`:
- Around line 108-117: Update the data provider used by the
itNormalizesSchemeAndHostToLowercase test (schemeAndHostNormalizationProvider)
to include cases that exercise userinfo and bracketed IPv6 hosts — e.g. add a
row for input like "HTTPS://User:Pass@[2001:DB8::1]:8443/Some/Path?Q=1#Frag"
with an expected URL that has the scheme and host lowercased, the IPv6 brackets
preserved, the port and path/query/fragment intact, and the userinfo preserved
exactly; also add the same row to the other provider referenced around the
itNormalizesSchemeAndHostToLowercase duplicate (lines ~198-205) so both tests
cover userinfo + bracketed IPv6 branches when creating the UploadTarget via
createUploadTarget.
- Line 21: Rename the test method itStoresUploadTargetWhenInputsAreValid to
follow the mandated pattern itReturnsUploadTargetWhenInputsAreValid, and
similarly rename any other non-throw tests (e.g., methods named itStores...,
itAllows..., itNormalizes...) to the itReturns{X}When{Condition}() form; update
the method names in the test class (and any references like annotations or data
providers) so the test runner and IDE links still work.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78061d0b-bca1-4bdc-bbbe-b9718d47f761
📒 Files selected for processing (24)
.github/copilot-instructions.md.gitignore.php-cs-fixer.cachecaptainhook.jsondocs/adr/04-asset-domain-contracts.mddocs/logs/02-domain-model-core-objects.mdmkdocs.ymlsrc/Domain/Asset/Asset.phpsrc/Domain/Asset/AssetRepositoryInterface.phpsrc/Domain/Asset/StorageAdapterInterface.phpsrc/Domain/Asset/UploadCompletionProofSource.phpsrc/Domain/Asset/UploadHttpMethod.phpsrc/Domain/Asset/ValueObject/AssetId.phpsrc/Domain/Asset/ValueObject/UploadCompletionProof.phpsrc/Domain/Asset/ValueObject/UploadCompletionProofValue.phpsrc/Domain/Asset/ValueObject/UploadParameter.phpsrc/Domain/Asset/ValueObject/UploadTarget.phptests/Unit/Domain/Asset/AssetTest.phptests/Unit/Domain/Asset/ValueObject/AssetIdTest.phptests/Unit/Domain/Asset/ValueObject/UploadCompletionProofTest.phptests/Unit/Domain/Asset/ValueObject/UploadCompletionProofValueTest.phptests/Unit/Domain/Asset/ValueObject/UploadIdTest.phptests/Unit/Domain/Asset/ValueObject/UploadParameterTest.phptests/Unit/Domain/Asset/ValueObject/UploadTargetTest.php
💤 Files with no reviewable changes (2)
- captainhook.json
- .php-cs-fixer.cache
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/logs/02-domain-model-core-objects.md (1)
49-49:⚠️ Potential issue | 🟡 MinorAdd a trailing newline at EOF to satisfy MD047.
The file still appears to end without a single trailing newline character.
Proposed fix
- Later application and infrastructure work should consume these contracts directly instead of introducing parallel repository or storage shapes. +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/logs/02-domain-model-core-objects.md` at line 49, Add a single trailing newline character at the end of the markdown file that contains the line "Later application and infrastructure work should consume these contracts directly instead of introducing parallel repository or storage shapes." to satisfy MD047; open the document (02-domain-model-core-objects.md), move the cursor to EOF after the last character and insert one newline, then save the file so the file ends with exactly one trailing newline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domain/Asset/ValueObject/UploadTarget.php`:
- Around line 57-111: The normalizeUrl() method is doing parsing,
validation/policy checking, and URL reconstruction in one place; split it into
focused private helpers to reduce complexity and make testing easier. Extract
responsibilities into methods like parseAndValidateUrl(string): array (wrap
parse_url + basic checks + empty check), validateSchemeAndHost(array
$parsedUrl): void (enforce HTTPS or call isAllowedLocalDevelopmentUrl and throw
on failure), and buildNormalizedUrl(array $parsedUrl, string $scheme, string
$host): string (handle auth, IPv6 bracket logic, port, path, query, fragment and
return the reconstructed normalized URL), then have normalizeUrl() call these
helpers in sequence (use existing isAllowedLocalDevelopmentUrl where needed).
In `@tests/Unit/Domain/Asset/ValueObject/AssetIdTest.php`:
- Around line 59-69: Add an explicit "Assert" section comment and blank line in
the test method itThrowsInvalidArgumentExceptionWhenValueIsNotUuidV4 to follow
the Arrange/Act/Assert convention: after the Act step (the new AssetId($value)
invocation) add a blank line and an inline comment like "// Assert" (keeping the
existing expectException and expectExceptionMessage calls grouped in the Assert
section) so the test is clearly separated into Arrange, Act, and Assert; locate
the method by name and update the comments accordingly.
In `@tests/Unit/Domain/Asset/ValueObject/UploadTargetTest.php`:
- Around line 73-81: The test
itThrowsInvalidArgumentExceptionWhenUrlIsNotAValidAbsoluteUrl currently uses an
inline hardcoded invalid URL; change it to use a static data provider by adding
a #[DataProvider('invalidAbsoluteUrlProvider')] attribute on the test, make the
test accept the URL as a parameter and call createUploadTarget($url, []), and
add a static method invalidAbsoluteUrlProvider(): array that returns an array of
invalid absolute URL cases (including 'https://example.test/contains space') so
the expectation (InvalidArgumentException and message) is exercised for each
input.
---
Duplicate comments:
In `@docs/logs/02-domain-model-core-objects.md`:
- Line 49: Add a single trailing newline character at the end of the markdown
file that contains the line "Later application and infrastructure work should
consume these contracts directly instead of introducing parallel repository or
storage shapes." to satisfy MD047; open the document
(02-domain-model-core-objects.md), move the cursor to EOF after the last
character and insert one newline, then save the file so the file ends with
exactly one trailing newline.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98c36f1a-81af-4f9f-91b4-eac74d9b1075
📒 Files selected for processing (6)
docs/logs/02-domain-model-core-objects.mdsrc/Domain/Asset/ValueObject/UploadCompletionProof.phpsrc/Domain/Asset/ValueObject/UploadTarget.phptests/Unit/Domain/Asset/AssetTest.phptests/Unit/Domain/Asset/ValueObject/AssetIdTest.phptests/Unit/Domain/Asset/ValueObject/UploadTargetTest.php
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.
Overview
This PR implements a domain-driven refactor of the Asset domain to enforce stronger invariants around uploads, introduces typed value objects and enums for upload targets and proofs, adds repository/storage contracts, extends test coverage for the new types and behaviors, and updates documentation and pre-commit/tooling metadata.
Key Changes
Domain Model
Value Objects & Enums
Repository & Storage Contracts
Tests
Documentation & Site
Tooling & Misc
Validation
Reviewer Guidance