Unify repository adapters behind a backend-agnostic core#38
Conversation
PostgresRepository and LocalRepository re-implemented the same ~26
methods with identical domain orchestration; only the storage calls and
the transaction/RLS/idempotency wrapper differed. Lift all orchestration
into one RepositoryCore that implements the new Repository interface, and
express each backend as two ports: a scope-bound Entities accessor and a
UnitOfWork runner (read/command/peekReplay). Postgres binds these to
RLS-scoped Drizzle queries plus runCommand; local binds them to Maps plus
a key-to-result idempotency cache. The two repositories become thin
subclasses, so a behavior change is now written once.
Three deliberate reconciliations are the only behavior changes: one
recordUploadedFile shape (Postgres superset; local ignores objectKey and
sizeBytes), one core cursor type { createdAt: Date; id } with the wire
format unchanged, and peekIdempotentReplay on the interface. Everything
else is behavior-preserving. The local unit of work keys idempotency by
the run scope's workspace to stay provably aligned with the Postgres key
derivation. packages/commands and the Postgres executor are untouched.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
apps/api and apps/upload each declared their own structural database type (ApiDatabase, UploadDatabase) that was never the real exported shape, forcing Required<Pick<...>> casts, db as ... intersections, defensive re-parsing through unknown, and per-method database_unavailable guards. Import the single Repository interface from @agent-paste/db instead and drop all of that; the one if (!db) return 503 binding check stays. apps/upload now calls db.peekIdempotentReplay(...) and the direct @agent-paste/commands peekIdempotentReplay plus createHyperdriveExecutor fallback are removed from its replay path. apps/api gains no peek (it never had one). apps/api also aligns to the @agent-paste/db actor types: authenticateApiKey returns ApiKeyActor, authenticateAdmin returns AdminActor, and the now-redundant adminActor helper and dead getAdminWhoami branch are removed. Test DB fakes gain peekIdempotentReplay. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add ADR 0069 documenting the ports-and-adapters repository unification and the three reconciliations, index it in docs/adr/README.md, and add an ADR Coverage row plus a Recently Completed entry to project-status.md. Add docs/ops/repository-todo.md tracking the deferred follow-ups: faithful in-flight idempotency serialization in the local unit of work, the matching peekReplay in-flight distinction, and a possible RepositoryCore split if it keeps growing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR consolidates domain orchestration into a new backend-agnostic RepositoryCore and exposes a single Repository interface. It adds port/adaptor types (UnitOfWork, Entities, RunScope), implements RepositoryCore with idempotent command/read flows, and provides Postgres and local adapters (PostgresUnitOfWork/postgresEntities, LocalUnitOfWork/localEntities). Apps (api/upload) are refactored to use Repository and split actor types (ApiKeyActor vs AdminActor). Tests were updated to stub peekIdempotentReplay, and documentation (ADR 0070, project-status, repository-todo) records the architecture and follow-ups. Sequence Diagram(s)sequenceDiagram
participant Client
participant API as apps/api | apps/upload
participant RepositoryCore
participant UnitOfWork
participant Entities
Client->>API: HTTP request (web callback / upload / admin action)
API->>RepositoryCore: invoke Repository method (e.g. resolveWebMember, createUploadSession, deleteArtifact)
RepositoryCore->>UnitOfWork: command/read(spec, handler) (includes idempotency key)
UnitOfWork->>Entities: persistence calls (select/insert/update/delete)
Entities-->>UnitOfWork: persistence result
UnitOfWork-->>RepositoryCore: cached or fresh result (peekReplay used for idempotency)
RepositoryCore-->>API: domain response (publish result, web payload, cleanup result)
API-->>Client: JSON response
Possibly related PRs
Poem
🚥 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 There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 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 `@apps/api/src/index.ts`:
- Around line 526-535: The idempotency drift comes from calling
purgeArtifactBytes(env, artifactId) outside the runIdempotent block so retries
can change deleted_r2_objects; move the purge computation into the runIdempotent
closure (or have db.deleteArtifact return/persist deleted_r2_objects) so the
value is computed once and stored with the idempotent result. Concretely, update
the code around runIdempotent and db.deleteArtifact: either call
purgeArtifactBytes(env, artifactId) inside the async passed to runIdempotent and
include the purged count in the object returned by db.deleteArtifact (or merge
into its returned result), or extend db.deleteArtifact to persist
deleted_r2_objects and then return that persisted value so replayed responses
are identical; keep denyArtifact where it belongs but ensure deleted_r2_objects
is produced/stored within the idempotent operation (runIdempotent /
db.deleteArtifact).
In `@packages/db/src/repository/interface.ts`:
- Line 107: The interface methods getWhoami and peekIdempotentReplay currently
accept ApiActor but their implementations only support api_key actors and throw
for member actors; update their signatures to accept ApiKeyActor instead
(replace ApiActor with ApiKeyActor for getWhoami and peekIdempotentReplay) and
update any corresponding overloads/tests/usages to pass ApiKeyActor so callers
and implementations match, ensuring compile-time safety and avoiding runtime
throws in the functions named getWhoami and peekIdempotentReplay.
In `@packages/db/src/repository/postgres-entities.ts`:
- Around line 56-65: The rows returned by the Postgres queries (e.g., in
listExpiring) expose snake_case DB columns like workspace_id; map each row to
the camelCase shape the Entities port expects before returning (e.g., transform
{ id, workspace_id } => { id, workspaceId: workspace_id }) and update the
function return typing accordingly; apply the same mapping in the other similar
method around lines 85-94 so the core/local adapter receives { id, workspaceId }
instead of raw DB column names.
🪄 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 Plus
Run ID: 17bb285a-2ecb-491f-9157-f6b3a9c97c46
📒 Files selected for processing (23)
apps/api/src/index.tsapps/api/src/local-mvp.test.tsapps/upload/src/error-envelope.test.tsapps/upload/src/index.test.tsapps/upload/src/index.tsdocs/adr/0069-repository-core-ports-and-adapters.mddocs/adr/README.mddocs/ops/project-status.mddocs/ops/repository-todo.mdpackages/db/src/index.test.tspackages/db/src/index.tspackages/db/src/local-repository.tspackages/db/src/postgres/repository.tspackages/db/src/repository/core.tspackages/db/src/repository/interface.tspackages/db/src/repository/local-entities.tspackages/db/src/repository/local-state.tspackages/db/src/repository/local-unit-of-work.tspackages/db/src/repository/ports.tspackages/db/src/repository/postgres-entities.tspackages/db/src/repository/postgres-unit-of-work.tspackages/db/src/repository/shared.tspackages/db/src/repository/web-transforms.ts
| return runIdempotent(context, async () => { | ||
| const result = await dbWithDeleteArtifact.deleteArtifact({ | ||
| actor: adminActor(actor), | ||
| const result = await db.deleteArtifact({ | ||
| actor: actor, | ||
| idempotencyKey, | ||
| artifactId, | ||
| }); | ||
| await denyArtifact(env, artifactId); | ||
| const purged = await purgeArtifactBytes(env, artifactId); | ||
| if (result && typeof result === "object") { | ||
| return { ...(result as Record<string, unknown>), deleted_r2_objects: purged }; | ||
| } | ||
| return result; | ||
| return { ...result, deleted_r2_objects: purged }; | ||
| }); |
There was a problem hiding this comment.
Idempotent delete responses can drift across retries.
Line 527 runs an idempotent DB command, but Line 534 always overwrites deleted_r2_objects with the current purge count. A retry with the same idempotency key can therefore return a different payload than the first successful call.
Please move deleted_r2_objects computation into the idempotent command result (or persist/reuse the first computed purge count) so replayed responses stay stable.
🤖 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 `@apps/api/src/index.ts` around lines 526 - 535, The idempotency drift comes
from calling purgeArtifactBytes(env, artifactId) outside the runIdempotent block
so retries can change deleted_r2_objects; move the purge computation into the
runIdempotent closure (or have db.deleteArtifact return/persist
deleted_r2_objects) so the value is computed once and stored with the idempotent
result. Concretely, update the code around runIdempotent and db.deleteArtifact:
either call purgeArtifactBytes(env, artifactId) inside the async passed to
runIdempotent and include the purged count in the object returned by
db.deleteArtifact (or merge into its returned result), or extend
db.deleteArtifact to persist deleted_r2_objects and then return that persisted
value so replayed responses are identical; keep denyArtifact where it belongs
but ensure deleted_r2_objects is produced/stored within the idempotent operation
(runIdempotent / db.deleteArtifact).
There was a problem hiding this comment.
Confirmed, but deferring out of this PR. The drift predates the unification: the purge-then-merge shape came from #24 (commit 22c4b36); this refactor only swapped dbWithDeleteArtifact for db. R2 purge accounting deliberately lives in the Worker (cleanup likewise reports deleted_r2_objects: 0 and defers object deletion to the jobs path), so making the count replay-stable is a behavior change to delete semantics, out of scope for a behavior-preserving refactor. Tracked in docs/ops/repository-todo.md with two resolution options. Leaving this thread open for visibility.
There was a problem hiding this comment.
@isuttell, understood — the context makes sense. The drift predates this refactor, R2 purge accounting is intentionally a Worker-layer concern, and stabilizing the replay would constitute a behavior change rather than a structural cleanup. Happy to leave the thread open for visibility.
Would you like me to open a follow-up GitHub issue to track the two resolution options recorded in docs/ops/repository-todo.md? That way it stays visible outside of the PR thread.
Avoids a number collision with the Live Updates ADR 0069 added on main via #37. The repository-core decision keeps the next free number. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…4f64ee8da40 # Conflicts: # docs/ops/project-status.md
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-38.isaac-a46.workers.dev |
- Narrow getWhoami/peekIdempotentReplay to ApiKeyActor; both are api-key-only paths (the impl throws for member actors) and both callers authenticate via api key, so the wider ApiActor signature only invited compile-valid runtime failures. - Drop the unused workspace_id field from Entities.listExpiring on the port and both adapters. runCleanup consumes only row.id, so the field was dead weight that leaked snake_case SQL naming through the backend-agnostic seam. - Record the pre-existing deleted_r2_objects idempotency gap in repository-todo rather than fold a delete-semantics change into the behavior-preserving PR. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-38.isaac-a46.workers.dev |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
agent-paste PR preview resources were cleaned up. The pr-preview-${context.issue.number} environment is left in place; remove it from the GitHub UI if desired. |
1 participant
Summary
PostgresRepository(~948 lines) andLocalRepository(~839 lines) into one backend-agnosticRepositoryCorethatimplements Repository. Each backend now supplies only two ports: a scope-boundEntitiesaccessor and aUnitOfWorkrunner (read/command/peekReplay). Postgres binds them to RLS-scoped Drizzle queries plusrunCommand; local binds them toMaps plus a key-to-result idempotency cache. The two repositories become thin subclasses, so a behavior change is written once.ApiDatabase/UploadDatabasestructural types with the singleRepositoryinterface exported from@agent-paste/db.apps/apiandapps/uploadimport it and drop theRequired<Pick<...>>casts,db as ... & ...intersections, defensiveunknownre-parsing, and per-methoddatabase_unavailable503 guards (the singleif (!db) return 503binding check stays).apps/apialso aligns to the db actor types and drops the now-deadadminActorhelper andgetAdminWhoamibranch.recordUploadedFileshape (Postgres superset; local ignoresobjectKey/sizeBytes), one core cursor type{ createdAt: Date; id }with the wire format unchanged, andpeekIdempotentReplayon the interface (apps/uploadswitches off the direct@agent-paste/commandspeek +createHyperdriveExecutorfallback;apps/apigains no peek). Everything else is behavior-preserving.packages/commandsandpackages/db/src/postgres/executor.tsare untouched. Decision recorded in ADR 0069; deferred follow-ups (faithful local in-flight idempotency, a possiblecore.tssplit) tracked indocs/ops/repository-todo.md.Test plan
pnpm verifygreen (62/62 Turbo tasks: lint, typecheck, test, openapi:check, db:check, web build).pnpm --filter @agent-paste/db test(24 passed) after the local unit-of-work scope-key hardening.Notes
coderabbit review --plain) ran on the first pass and surfaced three findings: the twominoritems onlocal-unit-of-work.tsare the documented deferred in-flight idempotency follow-up (no change needed) and a scope-key alignment suggestion that I applied so the local idempotency key derivation provably matches Postgres; the third was atrivialADR-prose nitpick. A re-run hit the org credit limit ("Your organization has run out of usage credits"), so the remaining diff after the hardening edit was self-reviewed (em-dash scan clean, core/ports/adapters re-read).Summary by CodeRabbit
New Features
Improvements
Documentation
Tests