chore: promote development history slice 3 - #33
Conversation
…nance feat(iae): complete evidence governance and revision-safe foundations
📝 WalkthroughWalkthroughThe PR strengthens domain validation, persists artifact scan states, adds evidence and retention retrieval APIs, enforces optimistic concurrency across repositories, improves spreadsheet audit detection, and updates application wiring and lint configuration. ChangesDomain validation and evidence contracts
Artifact persistence and retrieval
Cross-service revision safety
Spreadsheet audit analysis
Application wiring and lint configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ArtifactReadController
participant ArtifactService
participant ArtifactRepository
Client->>ArtifactReadController: Resolve versionId and evidenceId
ArtifactReadController->>ArtifactService: Resolve evidence with tenant context
ArtifactService->>ArtifactRepository: Load version, evidence, and placement
ArtifactRepository-->>ArtifactService: Return evidence and placement state
ArtifactService-->>ArtifactReadController: Return resolved or unavailable result
ArtifactReadController-->>Client: Return response or NOT_FOUND
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/api/src/features/iae/adapter/prisma-artifact-repository.adapter.ts (1)
261-278: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winForward
scanStatefromPrismaArtifactRepositoryAdapter.Line 261 adds
scanStateonly toPrismaArtifactTransactionAdapter.updateVersionStatus. The publicPrismaArtifactRepositoryAdapter.updateVersionStatusstill accepts and forwards only three arguments. A direct repository caller therefore loses the supplied scan state, and the persisted value remains unchanged. Add the optional parameter to the public adapter and forward it. Add a direct-adapter test for aPENDINGtoCLEANupdate.🤖 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 `@services/api/src/features/iae/adapter/prisma-artifact-repository.adapter.ts` around lines 261 - 278, Update PrismaArtifactRepositoryAdapter.updateVersionStatus to accept the optional scanState parameter and forward it to the underlying transaction/update implementation, preserving existing status behavior. Add a direct-adapter test covering a PENDING-to-CLEAN update that verifies the supplied scan state is persisted.services/api/src/features/dso/adapter/prisma-device-capability-repository.adapter.ts (1)
293-345: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a revision-increment check to
replaceCapabilityandreplaceGrant.Neither method verifies that
capability.revision/grant.revisionequalsexpectedRevision + 1before writing it. TheupdateManypredicate only matches onrevision: expectedRevision; it does not enforce that the write actually advances the revision.Two concurrent calls that each pass the same
expectedRevisionand a non-incremented (or otherwise incorrect) new revision value can both match the predicate and both succeed. Each write silently overwrites the other. This defeats the purpose of the revision-guardedupdateManyconversion introduced in this PR.Compare with
saveOperationinservices/api/src/features/dso/adapter/prisma-device-sync-repository.adapter.ts, which checksoperation.revision !== current.revision + 1before itsupdateManycall, and withPrismaIamTransactionAdapter.saveMembership, which performs the equivalent check formembership.revision.Add the same guard here:
🔒 Proposed fix
const current = await this.findCapability(context, capability.capabilityId); if (!current) throw new Error('DSO_CAPABILITY_NOT_FOUND'); if (current.revision !== expectedRevision) throw new Error('DSO_REVISION_CONFLICT'); + if (capability.revision !== expectedRevision + 1) throw new Error('DSO_REVISION_CONFLICT'); if (const current = await this.findGrant(context, grant.grantId); if (!current) throw new Error('DSO_GRANT_NOT_FOUND'); if (current.revision !== expectedRevision) throw new Error('DSO_REVISION_CONFLICT'); + if (grant.revision !== expectedRevision + 1) throw new Error('DSO_REVISION_CONFLICT'); if (Run the following script to check whether callers already enforce the correct revision increment before invoking these methods:
#!/bin/bash # Description: Inspect call sites of replaceCapability/replaceGrant for revision-increment handling. rg -n -B5 -A15 '\.replaceCapability\(|\.replaceGrant\(' --type=ts services/api/src🤖 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 `@services/api/src/features/dso/adapter/prisma-device-capability-repository.adapter.ts` around lines 293 - 345, Update replaceCapability and replaceGrant to validate that the incoming capability.revision or grant.revision equals the current persisted revision plus one before calling updateMany; throw the established revision-conflict error when the increment is invalid, while preserving the existing optimistic-lock and immutable-field checks.services/api/src/features/iae/application/artifact.service.ts (1)
91-98: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify that
QUARANTINEDartifact versions cannot resolve to an open handle.
resolveEvidencereturnsUNAVAILABLEonly whenversion.status === 'DELETED'orevidence.sourceState !== 'AVAILABLE'. It does not check forversion.status === 'QUARANTINED'. Other adapters in this codebase treatQUARANTINEDas a distinct, non-terminal status separate fromDELETED.If a
QUARANTINEDversion already has anAVAILABLEevidence reference and a populated placement, this method can still returnOPEN_CLOUDorOPEN_ON_SOURCE_DEVICEwith a liveplacementReference. Confirm whether quarantined content (pending or failed scan) can reach this state, and if so, addversion.status === 'QUARANTINED'to the unavailable condition.#!/bin/bash # Check whether QUARANTINED status is already excluded from evidence/placement resolution elsewhere, # and inspect how scanState/status interact before evidence/placements become populated. rg -n "QUARANTINED" --type=ts -C5 rg -n "scanState" --type=ts -C5 services/api/src/features/iae🤖 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 `@services/api/src/features/iae/application/artifact.service.ts` around lines 91 - 98, Update resolveEvidence to treat version.status === 'QUARANTINED' as unavailable alongside DELETED and non-AVAILABLE evidence. Preserve the existing UNAVAILABLE result shape and all other resolution behavior for non-quarantined versions.
🧹 Nitpick comments (5)
services/api/test/features/iam/prisma-identity-bootstrap-repository.test.ts (1)
119-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse a distinct transaction client in the test double.
$transactionpasses the baseclientdirectly towork. The test would still pass ifsaveused the base client instead of the transaction-scoped client. Pass a distinct transaction object, or record which client performs each write, and assert that the transaction client is used. This protects the contract implemented inservices/api/src/features/iam/adapter/prisma-identity-bootstrap-repository.adapter.tsLines 308-310.🤖 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 `@services/api/test/features/iam/prisma-identity-bootstrap-repository.test.ts` around lines 119 - 130, Update the `$transaction` test double to pass a distinct transaction-scoped client to `work` instead of the base `client`, and ensure the test asserts that writes performed by `save` use that transaction client. Preserve the existing transaction state and rollback behavior while validating the contract in the identity bootstrap repository adapter.services/api/test/features/foundation-module-composition.test.ts (1)
42-50: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify that both module options reach their feature modules.
The
satisfies ApiApplicationOptionscheck validates the type contract, but the runtime assertion only checks the top-levelAppModule. A regression that dropsauditRepositoryorentitlementRepositorybeforeAudModule.registerorBuaModule.registerwould still pass. Assert the repository identities in the child module metadata, or spy on both registration calls.🤖 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 `@services/api/test/features/foundation-module-composition.test.ts` around lines 42 - 50, Extend the test around AppModule.register to verify that both auditRepository and entitlementRepository are forwarded to their respective child modules, AudModule.register and BuaModule.register. Assert the original repository identities in the registered child-module metadata (or observe both registration calls), while retaining the existing top-level AppModule assertion.services/api/prisma/migrations/20260803000000_iae_lineage_uniqueness/migration.sql (1)
2-3: 🚀 Performance & Scalability | 🔵 TrivialPlan the index build for write availability.
Normal unique-index creation can block writes while the index builds. If
iae.artifact_lineagecan be large or this migration runs during traffic, use an approved online-index procedure, such asCREATE UNIQUE INDEX CONCURRENTLY, or schedule a write pause. Verify the migration runner and transaction settings before choosing the procedure.🤖 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 `@services/api/prisma/migrations/20260803000000_iae_lineage_uniqueness/migration.sql` around lines 2 - 3, Update the migration’s artifact_lineage unique-index creation to use the approved write-availability procedure after verifying the migration runner and transaction settings: use concurrent index creation when supported, or explicitly schedule a write pause when required. Preserve the unique constraint on derived_artifact_version_id and ensure the chosen procedure is compatible with the migration framework.Source: Linters/SAST tools
services/api/test/features/iae/artifact-retention.service.test.ts (1)
105-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit failure path when
authorized.acceptedisfalse.The assertions for
AUTHORIZEDstate and the newfindround-trip run only insideif (authorized.accepted). Ifauthorized.acceptedisfalse, the test completes without failing and without checking the intended behavior. Add anelsebranch that fails the test explicitly, for exampleassert.fail('expected authorization to succeed'), so a regression in theauthorizepath cannot pass silently.🤖 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 `@services/api/test/features/iae/artifact-retention.service.test.ts` around lines 105 - 109, The authorization test currently skips all assertions when authorized.accepted is false. Add an else branch to the authorized.accepted check that explicitly fails the test, while preserving the existing state assertion and find round-trip validation for successful authorization.services/api/src/features/iae/api/artifact-retention.controller.ts (1)
46-60: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUpdate the request contract for the now-ignored
requestedByfield.The controller discards
input.requestedByand always usescontext.actorIdfor attribution. This is a correct security fix and matches the controller test. However,CreateArtifactDeletionRequestDtoinservices/api/openapi/v1.jsonstill listsrequestedByas required. A client must supply a valid UUID for a value that the server never uses.Mark
requestedByas optional or deprecated in the DTO and OpenAPI schema, or remove it from the request contract. Document that attribution now always comes from the authenticated actor.🤖 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 `@services/api/src/features/iae/api/artifact-retention.controller.ts` around lines 46 - 60, Update CreateArtifactDeletionRequestDto and its OpenAPI v1 schema to make requestedBy optional or remove it from the request contract, since request always attributes via context.actorId in the request method. Document that attribution comes from the authenticated actor and preserve the existing request handling.
🤖 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 `@packages/domain/src/artifact/v1.ts`:
- Around line 344-385: Update isEvidenceGeometry to normalize each spreadsheet
sheet name before storing it in the names set, using the same normalization
applied to coordinate.sheet. Ensure duplicate detection and subsequent lookups
operate on normalized names, or reject names that are not already canonical.
In `@services/api/src/features/iae/adapter/prisma-artifact-repository.adapter.ts`:
- Around line 355-359: Update the Prisma adapter flow around the existing
placement lookup and contentPlacement.updateMany to validate visibility and
mutation permission using the persisted row’s scope, not caller-provided
placement. Reject mismatched or unauthorized persisted scopes before updating,
and apply the same persisted-scope authorization rule in the in-memory adapter.
In `@services/api/src/features/iam/adapter/in-memory-mfa-repository.adapter.ts`:
- Around line 26-40: Update saveState to match the Prisma MFA invariants: reject
any factor or recovery code present in the existing state but missing from the
incoming state, and require revision 1 for newly added records. Preserve the
existing user/digest checks and sequential revision validation for records
present in both states, covering both the factors and recoveryCodes loops.
In `@services/api/src/features/iam/adapter/prisma-mfa-repository.adapter.ts`:
- Around line 195-210: Update the persistence logic for both MFA factors and
recovery codes to use conditional updateMany operations matching each record’s
id and prior.revision, rather than ID-only updates. Check that each updateMany
result has count === 1 and throw IAM_MFA_REVISION_CONFLICT otherwise; add
race-condition tests covering both record types.
In `@services/api/test/features/iae/prisma-artifact-lineage-repository.test.ts`:
- Around line 44-51: Update the test double’s create behavior near the
findUnique mock to reject inserts when an existing row has the same
derivedArtifactVersionId, matching the migration’s uniqueness invariant.
Preserve successful insertion for distinct derived versions and ensure
repository tests cannot create duplicate lineage records.
In `@services/api/test/prisma-foundation.test.mjs`:
- Around line 497-504: Update the lineageUniquenessMigration assertion to match
the complete CREATE UNIQUE INDEX statement, including the iae.artifact_lineage
relation and derived_artifact_version_id column, rather than only the index
name.
In `@services/engine/src/databreeze_engine/processors/spreadsheet_auditor.py`:
- Around line 254-264: Update the formula-gap detection around
_normalized_formula and the formula_rows pairwise loop to group formula rows by
normalized formula family before evaluating gaps, so same-family rows are paired
even when intervening rows belong to another family. Preserve the existing gap
and validation behavior, and add a regression case covering matching-family
formulas above and below a different formula.
---
Outside diff comments:
In
`@services/api/src/features/dso/adapter/prisma-device-capability-repository.adapter.ts`:
- Around line 293-345: Update replaceCapability and replaceGrant to validate
that the incoming capability.revision or grant.revision equals the current
persisted revision plus one before calling updateMany; throw the established
revision-conflict error when the increment is invalid, while preserving the
existing optimistic-lock and immutable-field checks.
In `@services/api/src/features/iae/adapter/prisma-artifact-repository.adapter.ts`:
- Around line 261-278: Update
PrismaArtifactRepositoryAdapter.updateVersionStatus to accept the optional
scanState parameter and forward it to the underlying transaction/update
implementation, preserving existing status behavior. Add a direct-adapter test
covering a PENDING-to-CLEAN update that verifies the supplied scan state is
persisted.
In `@services/api/src/features/iae/application/artifact.service.ts`:
- Around line 91-98: Update resolveEvidence to treat version.status ===
'QUARANTINED' as unavailable alongside DELETED and non-AVAILABLE evidence.
Preserve the existing UNAVAILABLE result shape and all other resolution behavior
for non-quarantined versions.
---
Nitpick comments:
In
`@services/api/prisma/migrations/20260803000000_iae_lineage_uniqueness/migration.sql`:
- Around line 2-3: Update the migration’s artifact_lineage unique-index creation
to use the approved write-availability procedure after verifying the migration
runner and transaction settings: use concurrent index creation when supported,
or explicitly schedule a write pause when required. Preserve the unique
constraint on derived_artifact_version_id and ensure the chosen procedure is
compatible with the migration framework.
In `@services/api/src/features/iae/api/artifact-retention.controller.ts`:
- Around line 46-60: Update CreateArtifactDeletionRequestDto and its OpenAPI v1
schema to make requestedBy optional or remove it from the request contract,
since request always attributes via context.actorId in the request method.
Document that attribution comes from the authenticated actor and preserve the
existing request handling.
In `@services/api/test/features/foundation-module-composition.test.ts`:
- Around line 42-50: Extend the test around AppModule.register to verify that
both auditRepository and entitlementRepository are forwarded to their respective
child modules, AudModule.register and BuaModule.register. Assert the original
repository identities in the registered child-module metadata (or observe both
registration calls), while retaining the existing top-level AppModule assertion.
In `@services/api/test/features/iae/artifact-retention.service.test.ts`:
- Around line 105-109: The authorization test currently skips all assertions
when authorized.accepted is false. Add an else branch to the authorized.accepted
check that explicitly fails the test, while preserving the existing state
assertion and find round-trip validation for successful authorization.
In `@services/api/test/features/iam/prisma-identity-bootstrap-repository.test.ts`:
- Around line 119-130: Update the `$transaction` test double to pass a distinct
transaction-scoped client to `work` instead of the base `client`, and ensure the
test asserts that writes performed by `save` use that transaction client.
Preserve the existing transaction state and rollback behavior while validating
the contract in the identity bootstrap repository adapter.
🪄 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 Plus
Run ID: cc169056-2cb5-4437-b900-e3252a0a571f
📒 Files selected for processing (58)
eslint.config.mjspackages/domain/src/artifact-governance/v1.tspackages/domain/src/artifact-intake/v1.tspackages/domain/src/artifact/v1.tspackages/domain/src/dataset-quality/v1.tspackages/domain/src/spreadsheet-audit/v1.tspackages/domain/test/artifact-governance-v1.test.mjspackages/domain/test/artifact-v1.test.mjspackages/domain/test/dataset-quality-v1.test.mjspackages/domain/test/spreadsheet-audit-v1.test.mjsservices/api/openapi/v1.jsonservices/api/prisma/migrations/20260803000000_iae_lineage_uniqueness/migration.sqlservices/api/prisma/schema/iae.prismaservices/api/src/bootstrap.tsservices/api/src/features/bua/adapter/prisma-entitlement-repository.adapter.tsservices/api/src/features/dsm/adapter/prisma-governed-dataset-repository.adapter.tsservices/api/src/features/dsm/adapter/prisma-mapping-repository.adapter.tsservices/api/src/features/dso/adapter/prisma-device-authorization-repository.adapter.tsservices/api/src/features/dso/adapter/prisma-device-capability-repository.adapter.tsservices/api/src/features/dso/adapter/prisma-device-sync-repository.adapter.tsservices/api/src/features/iae/adapter/in-memory-artifact-repository.adapter.tsservices/api/src/features/iae/adapter/prisma-artifact-lineage-repository.adapter.tsservices/api/src/features/iae/adapter/prisma-artifact-repository.adapter.tsservices/api/src/features/iae/adapter/prisma-artifact-retention-repository.adapter.tsservices/api/src/features/iae/api/artifact-read.controller.tsservices/api/src/features/iae/api/artifact-retention.controller.tsservices/api/src/features/iae/application/artifact-admission.service.tsservices/api/src/features/iae/application/artifact-repository.port.tsservices/api/src/features/iae/application/artifact-retention.service.tsservices/api/src/features/iae/application/artifact.service.tsservices/api/src/features/iam/adapter/in-memory-mfa-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-device-identity-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-iam-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-identity-bootstrap-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-mfa-repository.adapter.tsservices/api/test/features/bua/prisma-entitlement-repository.test.tsservices/api/test/features/dsm/prisma-governed-dataset-repository.test.tsservices/api/test/features/dsm/prisma-mapping-repository.test.tsservices/api/test/features/dso/prisma-device-authorization-repository.test.tsservices/api/test/features/dso/prisma-device-capability-repository.test.tsservices/api/test/features/dso/prisma-device-sync-repository.test.tsservices/api/test/features/foundation-module-composition.test.tsservices/api/test/features/iae/artifact-admission.service.test.tsservices/api/test/features/iae/artifact-read.controller.test.tsservices/api/test/features/iae/artifact-retention.controller.test.tsservices/api/test/features/iae/artifact-retention.service.test.tsservices/api/test/features/iae/prisma-artifact-lineage-repository.test.tsservices/api/test/features/iae/prisma-artifact-repository.test.tsservices/api/test/features/iae/prisma-artifact-retention-repository.test.tsservices/api/test/features/iam/prisma-device-identity-repository.test.tsservices/api/test/features/iam/prisma-iam-repository.test.tsservices/api/test/features/iam/prisma-identity-bootstrap-repository.test.tsservices/api/test/features/iam/prisma-mfa-repository.test.tsservices/api/test/features/sa/spreadsheet-audit.controller.test.tsservices/api/test/openapi.test.tsservices/api/test/prisma-foundation.test.mjsservices/engine/src/databreeze_engine/processors/spreadsheet_auditor.pyservices/engine/tests/test_spreadsheet_auditor.py
|
Disposition recorded in docs/operations/coderabbit-pr-33-disposition.md and merged into dev via PR #34 (merge 89506bc). Seven actionable findings were fixed in focused commits; the Prisma MFA conditional-update suggestion was already satisfied on the reviewed head and the concurrent-index suggestion was rejected for the preproduction migration policy. Local and hosted checks are green, and CodeRabbit was invoked exactly once for this promotion. |
1 participant
Summary
Review policy
This main-target promotion PR receives exactly one automatic CodeRabbit full review. Do not request a rerun. Every finding will be reproduced against current dev, fixed there if valid, or documented if rejected.
Boundary
Summary by CodeRabbit