Conversation
Closed
9 tasks
📝 WalkthroughWalkthroughAdds a domain model for assets (value objects, Asset aggregate, status enum, repository interface), a GraphQL upload API contract, ADR documenting idempotent presigned-upload flow, tests for domain/value objects, and several CI/config updates and dependency additions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant GraphQL as "GraphQL API"
participant App as "Application Layer"
participant Domain as "Domain (Asset Aggregate)"
participant Repo as "Repository / DB"
participant Storage as "Storage Adapter"
Client->>GraphQL: startUpload(input with uploadId, checksum, metadata)
GraphQL->>App: startUpload(input)
App->>Domain: find by UploadId?
Domain->>Repo: findByUploadId(uploadId)
Repo-->>Domain: existing Asset|null
alt not found
Domain->>Domain: createPending(uploadId, accountId)
Domain->>Repo: save(new Asset)
Repo-->>Domain: saved (may raise unique-key error)
Note right of Repo: On UNIQUE(upload_id) conflict,\nRepo re-queries by uploadId and returns existing
end
App->>Storage: request presigned uploadTarget (for uploadId/asset)
Storage-->>App: uploadTarget (url, method, headers, completionProofDescriptor)
App-->>GraphQL: StartUploadPayload (success with asset, uploadTarget, uploadGrant)
GraphQL-->>Client: response
Client->>Storage: perform PUT/POST to uploadTarget
Client->>GraphQL: completeUpload(assetId, uploadGrant, completionProof)
GraphQL->>App: completeUpload(...)
App->>Repo: findById(assetId)
Repo-->>App: Asset|null
App->>Domain: validate completionProof & markUploaded()
Domain->>Repo: save(updated Asset status UPLOADED)
Repo-->>Domain: saved
App-->>GraphQL: CompleteUploadPayload (success.asset)
GraphQL-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![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 @.gitignore:
- Around line 59-60: Remove the duplicate PHPUnit ignore lines so only one
"PHPUnit" section remains (consolidate the repeated entries into the existing
PHPUnit block) and keep the .php-cs-fixer.cache entry as-is; also update the
PR/commit summary to remove the incorrect claim about replacing a broad
"*.cache" pattern and instead state that this change simply adds
.php-cs-fixer.cache following the existing tool-specific cache pattern.
In `@src/Application/Asset/CompleteUploadService.php`:
- Around line 16-24: The complete(string $assetId): Asset method in
CompleteUploadService currently marks assets uploaded with placeholder metadata
without verifying credentials; change complete to accept and validate the upload
grant/completion proof (e.g., add parameters like $uploadGrant or
$completionProof), fetch and verify those credentials before proceeding (use
repository->findById to load the Asset, then validate the provided proof against
whatever verification logic or service exists), and only call
Asset::markUploaded with the actual MIME type, filename, and size obtained from
the validated proof or upload metadata; if validation fails or metadata is
missing, throw an appropriate exception and do not call
repository->save($asset).
In `@src/Application/Asset/StartUploadService.php`:
- Around line 26-30: The method that returns the payload (the array containing
'asset', 'uploadTarget', 'uploadGrant' in StartUploadService) is returning
uploadTarget as null which violates the GraphQL type
StartUploadSuccess.uploadTarget: UploadTarget!; update the return value so
'uploadTarget' is a non-null UploadTarget instance/array matching the
UploadTarget shape expected by your schema (or, if no target is available,
throw/return an explicit error variant instead of null); locate the method in
StartUploadService that returns ['asset'=> $asset, 'uploadTarget'=> null,
'uploadGrant'=> ''] and replace the null with a properly constructed
UploadTarget object/associative array (or adjust control flow to return an
error) so the resolver always returns a non-null UploadTarget.
- Around line 21-25: The start method in StartUploadService is not idempotent:
it always calls Asset::createPending and saves, causing duplicates on retries;
change start(string $uploadId, string $accountId) to first lookup an existing
asset by uploadId via the repository (e.g., repository->findByUploadId or
similar), return the existing pending/started asset if found, and only call
Asset::createPending + repository->save when no asset exists; also make the save
conflict-safe by re-querying after a failed unique constraint or handling a
concurrent-insert exception to return the existing asset instead of creating a
duplicate.
In `@src/Domain/Asset/Asset.php`:
- Around line 62-72: The markUploaded method currently allows transitions from
any non-UPLOADED state and accepts negative sizes; update it to only allow the
upload transition when the current status is exactly AssetStatus::PENDING and
the provided $size is >= 0, otherwise throw an AssetDomainException (use a clear
message like "Invalid asset state or size for upload"); keep setting $filename,
$contentType, $size, $status = AssetStatus::UPLOADED and $updatedAt = new
DateTimeImmutable() only after those guards pass; reference the method name
markUploaded, the enum constants AssetStatus::PENDING and AssetStatus::UPLOADED,
and the exception class AssetDomainException so reviewers can locate the change.
In `@src/Domain/Asset/AssetRepositoryInterface.php`:
- Line 11: The repository interface method signature public function
findById(string $id): ?Asset; uses a vague parameter name; rename the parameter
to $assetId in AssetRepositoryInterface (method findById) and update all
implementing classes and call sites to accept and pass $assetId so the contract
and implementations stay consistent and intent is clearer.
In `@src/Domain/Asset/ValueObject/UploadId.php`:
- Around line 9-17: The constructor of UploadId currently uses a hand-rolled
UUID regex (UUID_PATTERN) and preg_match to validate $value; replace that with
Ramsey\Uuid\Uuid::isValid($value) in the UploadId::__construct, remove the
UUID_PATTERN constant and the preg_match check, and keep throwing
\InvalidArgumentException('Invalid UploadId format') when Uuid::isValid returns
false; ensure the Ramsey\Uuid\Uuid class is referenced (import or
fully-qualified) so validation uses the library API.
In `@src/GraphQL/Schema/schema.graphql`:
- Around line 84-94: Update the GraphQL field descriptions to document browser
CORS requirements: for the upload contract described by fields url, method
(UploadHttpMethod), signedHeaders ([UploadParameter!]) and completionProof
(UploadCompletionProofDescriptor!), state that if url is cross-origin the upload
origin must allow all header names listed in signedHeaders via
Access-Control-Allow-Headers during preflight and must expose the header name
returned for completionProof via Access-Control-Expose-Headers (and allow the
requesting origin in Access-Control-Allow-Origin / allow credentials if used) so
a browser client can send the signed headers and read the completionProof
response header to finish the flow.
- Around line 15-17: The schema's ByteCount scalar lacks a canonical wire
format, risking loss for values > 2^53-1; update the schema comment for scalar
ByteCount to state that fileSizeBytes MUST be sent as a JSON string (e.g.,
"12345678901234567890") and then make the scalar implementation (the parseValue,
parseLiteral, and serialize functions for the ByteCount scalar) accept and emit
strings only (or accept numeric input but convert to string for downstream),
ensuring consistency between schema docs and the ByteCount
resolvers/serializers; refer to the ByteCount scalar declaration and any field
named fileSizeBytes to locate where to update comments and the scalar's
parser/serializer code.
In `@tests/Unit/Domain/Asset/AssetTest.php`:
- Around line 37-40: The test must ensure the Asset id is not the same as the
UploadId: in AssetTest (the createPending() test) add an assertion comparing
$asset->getId() to $uploadId (e.g. assertNotSame or assertNotEquals) to
guarantee the Asset id produced by createPending() is distinct from the provided
$uploadId; reference the existing $asset->getId(), $uploadId and the
createPending() fixture when adding this assertion.
🪄 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: d7390a16-b77d-4676-b8de-d26d83126ada
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/tasks/epics/epic-01-bootstrap/EPIC-01.md.github/tasks/epics/epic-01-bootstrap/US-01-agree-api-schema.md.github/tasks/epics/epic-01-bootstrap/US-01-bootstrap-schema-domain-scaffold.md.gitignorecomposer.jsondocs/adr/03-domain-first-idempotent-upload.mddocs/api/01-agree-api-schema.mddocs/logs/01-agree-api-schema.mdsrc/Application/Asset/CompleteUploadService.phpsrc/Application/Asset/StartUploadService.phpsrc/Domain/Asset/Asset.phpsrc/Domain/Asset/AssetRepositoryInterface.phpsrc/Domain/Asset/AssetStatus.phpsrc/Domain/Asset/Exception/AssetDomainException.phpsrc/Domain/Asset/ValueObject/AccountId.phpsrc/Domain/Asset/ValueObject/UploadId.phpsrc/GraphQL/Schema/schema.graphqltests/Unit/Application/Asset/CompleteUploadServiceTest.phptests/Unit/Domain/Asset/AssetTest.phptests/Unit/Domain/Asset/ValueObject/AccountIdTest.phptests/Unit/Domain/Asset/ValueObject/UploadIdTest.php
💤 Files with no reviewable changes (1)
- .github/tasks/epics/epic-01-bootstrap/US-01-bootstrap-schema-domain-scaffold.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/security.yml:
- Around line 18-19: Change the GitHub Actions checkout depth from the
unnecessary full history setting to a shallow clone by replacing the checkout
step's fetch-depth value "fetch-depth: 0" with "fetch-depth: 1"; update the
workflow entry that currently contains the literal "fetch-depth: 0" so Semgrep
p/ci still runs correctly but the checkout is faster and uses fewer resources.
🪄 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: 7a0c9601-1275-4cb4-bba8-e076859b7e1d
📒 Files selected for processing (2)
.github/workflows/security.yml.gitleaks.toml
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.phpunit.cache/test-results:
- Line 1: The `.phpunit.cache/test-results` cache file is tracked in git despite
`.gitignore` listing `.phpunit.cache/`; remove it from version control by
untracking that specific file (use git rm --cached on
.phpunit.cache/test-results), commit the removal, and push the change so the
generated cache stays ignored going forward; verify the `.gitignore` contains
the `.phpunit.cache/` pattern and that no other cache files are still tracked.
In `@docs/adr/03-domain-first-idempotent-upload.md`:
- Line 29: The ADR says uploadId must be UUID v4 but the validation in
UploadId.php currently allows versions 1-8 via the regex with [1-8]; decide
which is correct and update accordingly: if policy is v4-only, change the regex
in UploadId.php (the upload id validation logic / UploadId class or function
that builds the UUID pattern) to only accept version 4 (replace the version char
class with 4), or if broader versions are acceptable, update
docs/adr/03-domain-first-idempotent-upload.md to state that UUID v1–v8 are
allowed; ensure the change targets the UploadId validation symbol and the ADR
text so both match.
In `@src/Domain/Asset/Asset.php`:
- Around line 96-104: The markFailed() method currently updates updatedAt even
when status is already AssetStatus::FAILED; change markFailed() to be idempotent
by returning early if $this->status === AssetStatus::FAILED (so it neither
throws nor updates updatedAt), keeping the existing guard that throws when
$this->status === AssetStatus::UPLOADED; update or add a unit test to assert
that calling Asset::markFailed() twice leaves status as AssetStatus::FAILED and
does not change updatedAt, referencing the markFailed() method,
AssetStatus::FAILED, AssetStatus::UPLOADED, updatedAt, and AssetDomainException.
- Around line 41-57: Replace the manual UUID generation in Asset::generateUuidV4
with ramsey/uuid: remove the random_bytes/bit-mangling and bin2hex logic in
generateUuidV4 and instead return \Ramsey\Uuid\Uuid::uuid4()->toString(), adding
the appropriate use/import for Ramsey\Uuid\Uuid at the top of the Asset class
file; this leverages the existing dependency and eliminates the custom
implementation.
In `@src/Domain/Asset/ValueObject/AssetId.php`:
- Around line 7-29: AssetId duplicates UUID validation logic found in UploadId
via the same UUID_PATTERN and manual preg_match; extract that shared logic by
creating a UuidValidation trait (with the UUID_PATTERN constant and a protected
static assertValidUuid(string $value, string $errorMessage): void) or use
Ramsey\Uuid\Uuid::isValid(), then update AssetId::__construct to call the shared
assertValidUuid (or Uuid::isValid) instead of repeating the regex; while doing
this, align the property declaration style with UploadId (use constructor
promotion or match its style) and ensure AssetId still exposes the $value and
__toString() unchanged.
In `@tests/Unit/Domain/Asset/ValueObject/UploadIdTest.php`:
- Around line 67-70: The private test helper createUploadId() is an unnecessary
indirection for instantiating UploadId; replace calls to createUploadId($value)
with direct constructions new UploadId($value) and remove the private function
createUploadId() from the test class to simplify the tests and reduce cognitive
overhead. Ensure all usages in the test file are updated to use new
UploadId(...) and run the unit tests to confirm no references remain to
createUploadId().
🪄 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: 86f8dd6d-638c-46a3-88c2-14e2a03125ba
📒 Files selected for processing (13)
.gitignore.gitleaks.toml.phpunit.cache/test-resultsdocs/adr/03-domain-first-idempotent-upload.mddocs/api/01-agree-api-schema.mdsrc/Domain/Asset/Asset.phpsrc/Domain/Asset/AssetRepositoryInterface.phpsrc/Domain/Asset/Exception/AssetDomainException.phpsrc/Domain/Asset/ValueObject/AssetId.phpsrc/Domain/Asset/ValueObject/UploadId.phpsrc/GraphQL/Schema/schema.graphqltests/Unit/Domain/Asset/AssetTest.phptests/Unit/Domain/Asset/ValueObject/UploadIdTest.php
💤 Files with no reviewable changes (1)
- .gitignore
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Domain/Asset/ValueObject/AssetId.php (1)
9-20: 🧹 Nitpick | 🔵 TrivialExtract UUID validation to shared code path.
This repeats UUID regex/guard logic that already exists in another value object, which increases maintenance risk if rules change. Centralize the assertion in a shared helper/trait and call it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domain/Asset/ValueObject/AssetId.php` around lines 9 - 20, The UUID validation in AssetId (private const UUID_PATTERN and the preg_match check in __construct) should be moved to a shared assertion (e.g., a UuidAssertion::assertValid(string $value) static method or a ValueObjectUuidTrait::assertUuid(string $value)) and invoked from AssetId::__construct; remove the duplicate UUID_PATTERN constant and preg_match from AssetId, import/use the shared helper, and have AssetId throw the same \InvalidArgumentException when the shared assertion fails so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Domain/Asset/ValueObject/AssetId.php`:
- Around line 9-20: The UUID validation in AssetId (private const UUID_PATTERN
and the preg_match check in __construct) should be moved to a shared assertion
(e.g., a UuidAssertion::assertValid(string $value) static method or a
ValueObjectUuidTrait::assertUuid(string $value)) and invoked from
AssetId::__construct; remove the duplicate UUID_PATTERN constant and preg_match
from AssetId, import/use the shared helper, and have AssetId throw the same
\InvalidArgumentException when the shared assertion fails so behavior remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 397bf1cc-18e6-45e4-a365-38eac8ced29a
📒 Files selected for processing (3)
.phpunit.cache/test-resultssrc/Domain/Asset/ValueObject/AssetId.phpsrc/Domain/Asset/ValueObject/UploadId.php
💤 Files with no reviewable changes (1)
- .phpunit.cache/test-results
This was referenced Apr 1, 2026
Merged
2 tasks
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
This PR defines and formalizes the consumer-facing GraphQL API contract for the DAM upload flow (US-01: Agree API schema), adds domain-first artifacts and tests to support that contract, and provides ADRs and docs that record the agreed surface and architectural guidance. The change moves the epic’s US-01 from a bootstrap/schema-and-scaffold implementation to an API-contract-first story.
Changes Overview
API Contract & Schema
src/GraphQL/Schema/schema.graphql) defining:Domain Layer
Documentation & Architecture
docs/adr/03-domain-first-idempotent-upload.md): Domain-first presigned-upload architecture, idempotency via uploadId + DB UNIQUE(upload_id), persistence conflict handling, and adapter responsibilities for presigned URL generation.docs/api/01-agree-api-schema.md): Authoritative consumer-facing contract and semantics (detailed explanations, types, and error handling).docs/logs/01-agree-api-schema.md): Notes that GraphQL SDL is authoritative and lists files in scope..github/tasks/epics/epic-01-bootstrap/EPIC-01.md): Replaced previous US-01 (bootstrap scaffold) with US-01: Agree API schema; the original bootstrap document was removed.Tests
Config, CI & Security
*.cacheentry (retains scoped cache ignores)aws-access-key,private-key) and broadened private-key regexfetch-depth: 0(full clone).phpunit.cache/test-resultsfile was deleted (cache removed)Architecture Alignment
Testing & Risk
Deliverables / Acceptance