diff --git a/AGENTS.md b/AGENTS.md index 7c041ce..d263f6c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,106 +1,56 @@ -## Branching and pull requests +# AutoReview contributor guidance -Branch off `dev` and target `dev` with every pull request; `gh pr create` defaults to `main`, so -pass `--base dev` explicitly. `main` is the released branch, kept as a fast-forward of `dev` and -synced as-is — never open a backport pull request to `main`, because anything merged to `dev` -reaches it at the next sync. Pull requests opened against `main` are retargeted automatically. -`Fixes #N` does not close the issue on a `dev` merge — GitHub honors closing keywords only on the -default branch, so close linked issues by hand. Worktrees share one stash stack, so never use a bare -`git stash pop`. See the detailed policy in `CLAUDE.md` under "Branching and Pull Requests". +## Repository layout -Write the description for a reader who has not followed the branch: what breaks, what triggers it, -how it behaves after the change, then one or two views of the mechanism — a focused diff, a call -tree, a shallow file tree, or a Mermaid sequence. Keep only what the change carries, and describe -the code as it stands rather than narrating earlier commits or review rounds. Naming the merged -pull request that caused the bug is not the same thing; that is history the reader needs. The -formats and examples live in `.github/pull_request_template.md`. +AutoReview is a pnpm and Turborepo monorepo: -## Review and completion +- `apps/api`: NestJS API and workflow orchestration. +- `apps/worker`: authenticated Pub/Sub and Cloud Tasks handlers. +- `apps/web`: Next.js operations dashboard. +- `apps/mobile`: Expo companion application. +- `packages/contracts`: shared Zod schemas and TypeScript types. +- `packages/core`: provider-independent domain, policy, and integration logic. +- `packages/database`: Drizzle schema, migrations, and PostgreSQL helpers. +- `infra/terraform`: Google Cloud infrastructure. -Read the inline review threads themselves — a summary comment or notification list omits findings. -Audit each one against the current code, fix what is valid, and reject what is obsolete in a reply -that says why. After each round: focused tests, `npx tsc --noEmit` in every workspace you changed, -push, then request the next review naming the pull request's exact remote head — a clean review of -an earlier head says nothing about what you just pushed, and CI runs on its own clock. After two -actionable rounds, stop patching thread by thread and read the subsystem by invariant instead. -Which reviewer and what phrase triggers it will change; that the review must cover the exact pushed -head will not. +Keep domain behavior in `packages/core`, transport validation in `packages/contracts`, persistence +in `packages/database`, and application wiring in the relevant app. Apps may depend on packages; +packages must not depend on apps. -A clean review is one completion signal, not the definition of done. Ship the observable experience -— loading, empty, success, failure, cancellation, retry, restored session — with strings localized, -accessibility intact, defaults and stored data preserved, and no backend capability left without a -frontend entry point. Report the pushed head, what you ran locally, CI state, the review result at -that head, and any finding you rejected with the reasoning. See `CLAUDE.md` under "Review and -Completion". +## Branching -## Verification - -For startup, auth, config, file, or message-loading changes, avoid serial database -reads and reuse loaded request data. Run `npm run lighthouse` before completion: -the CI lane adds 250 ms per Mongo query and checks the visible conversation's LCP. -See [budgets, reproduction and failure diagnosis](e2e/lighthouse/README.md). - -A green build is not a typecheck: `packages/api`, `packages/client` and `packages/data-schemas` build -with `tsdown`, which emits without checking types. Run `npx tsc --noEmit` in the workspace you -changed. `packages/client` excludes `*.spec.ts(x)` and `*.test.ts(x)` from typechecking entirely. -`npm run sort-imports` with no arguments rewrites every source root — pass the paths you touched. See -`CLAUDE.md` under "Typechecking" and "Formatting". - -## Module boundaries and configuration +Develop on `dev`. The `main` branch is the released, synchronized branch. Keep commits focused and +do not commit generated output, credentials, `.env` files, Terraform state, or real customer data. -`/api` holds wiring, not behavior. When a change would add logic to a CJS file there — a branch, a -helper, a validation step, a service call — the logic belongs in `packages/api`, and the JS file -keeps requires, route registration and the call into the TS module (`MCPRequestContext.js` is the -shape, thirteen lines of re-export). "Minimum" means how much behavior `/api` gains, not how small -the diff is, and the rule applies to editing existing CJS, which is the common case. - -Database contracts belong to `packages/data-schemas`. Keep Mongoose types (`FilterQuery`, -`Types.ObjectId`, `Document`) out of exported signatures in `packages/api`, `packages/data-provider` -and `client`, because they make the storage engine part of that module's public API. Take and return -plain typed objects and express the query behind a data-schemas method. The boundary already leaks -across `packages/api`, so stop widening it rather than rewriting what exists; the client carries none -of it and must stay that way. - -New levers ship configurable: a limit, timeout, toggle or capability introduced in code earns a field -on `configSchema` (`packages/data-provider/src/config.ts`) so it can be set in `librechat.yaml`, with -a default that reproduces today's behavior. Hard-coded constants and env-only switches need a reason. -Modules take their dependencies rather than reaching for them: code in `packages/api` receives its -config, database methods and clients from the caller, the way `createModels(mongoose)` receives the -app's connection, instead of importing app singletons or reading global state. Integrations (provider -SDKs, storage backends, vector stores, OAuth servers) arrive through an interface the caller -supplies, so a second implementation is a new argument instead of a new branch. The static singletons -under `packages/api/src/mcp` are the shape to stop extending, not a pattern to copy. This is the -backend half of client state ownership: pass it in, do not reach for it. +## Verification -See `CLAUDE.md` under "Workspace Boundaries". +Use Node.js 24 and pnpm 11. Before pushing a change, run: -## Frontend theming and styling +```sh +pnpm lint +pnpm typecheck +pnpm test +pnpm build +pnpm audit --prod +``` -For frontend work, compose existing `@librechat/client` primitives and variants before adding -feature-local styles. Use semantic theme/Tailwind roles for color and shared appearance; do not -introduce raw palette utilities, hard-coded colors, or arbitrary theme CSS. If the system cannot -express a reusable design need, deepen the shared primitive or versioned theme-token registry -instead of copying classes into a feature. Keep genuine layout and behavior local, and document -why any new custom CSS cannot be expressed by the shared system. See the detailed policy in -`CLAUDE.md` under “Theming and styling.” +Add or update tests for behavior changes. Typecheck every affected workspace; a successful bundle +alone is not a substitute for TypeScript validation. -## Backend auth cache +## Security and workflow invariants -When adding or changing code that mutates user documents, invalidate the auth user document cache -for affected users, including bulk role and user mutations. See the detailed policy in `CLAUDE.md` -under “Auth cache invalidation”. +- Preserve tenant isolation and parameterize all database input. +- Never expose Google credentials, review content, or access tokens in logs or notifications. +- Keep external events and publication operations idempotent. +- Re-read the canonical Google review immediately before publication. +- Respect optimistic concurrency through `expectedVersion` on review mutations. +- Keep hard stops deterministic and independent from model output. +- Only approved knowledge may influence generated replies. +- Failed asynchronous operations must leave reviews in a recoverable state and produce an audit + event without storing sensitive payloads. -## Client state ownership +## Configuration -The client is migrating from Recoil to Jotai. New state is always Jotai, even in a file that already -imports Recoil; many files import both, so mixed imports say nothing about which to use. For existing -state the unit of conversion is one atom plus every file that reads or writes it, because an atom -cannot be half converted — convert the areas you touch, not the whole store. -Split by ownership: state a feature both writes and reads is feature-owned, so convert it to Jotai -and keep it inside the feature; app-global preferences and shell state a feature merely consumes -(`maximizeChatSpace`, `showScrollButton`, `enterToSend`, artifact visibility) must be passed in -through props or a small host-supplied context rather than reached for through `~/store`; when a -consumer sits outside the feature you are changing, leave that atom on Recoil and pass it in. Passing -them in is what lets a feature move to its own workspace later without a rewrite, and it keeps the -Jotai conversion scoped to the state a feature owns. See the detailed policy in `CLAUDE.md` under -“Client State Ownership”. +Document new environment variables in `.env.example`. Production secrets belong in Secret Manager, +and live integrations must fail closed when required configuration is missing. Keep local defaults +explicitly non-production and safe. diff --git a/apps/api/src/controllers.ts b/apps/api/src/controllers.ts index c0aaa28..718e178 100644 --- a/apps/api/src/controllers.ts +++ b/apps/api/src/controllers.ts @@ -54,7 +54,7 @@ export class ReviewsController { @Get() list(@Principal() principal: RequestPrincipal, @Query() query: unknown) { const parsed = reviewListQuerySchema.parse(query); - return { data: this.reviews.list(principal, parsed.status), meta: { limit: parsed.limit } }; + return { data: this.reviews.list(principal, parsed), meta: { limit: parsed.limit } }; } @Get(":id") diff --git a/apps/api/src/review.service.ts b/apps/api/src/review.service.ts index 0100f3f..74e4796 100644 --- a/apps/api/src/review.service.ts +++ b/apps/api/src/review.service.ts @@ -1,5 +1,10 @@ import { Inject, Injectable } from "@nestjs/common"; -import type { RequestPrincipal, ReviewCase, ReviewSnapshot } from "@reviewguard/contracts"; +import type { + RequestPrincipal, + ReviewCase, + ReviewListQuery, + ReviewSnapshot, +} from "@reviewguard/contracts"; import { assertExpectedVersion, decideAutomation, @@ -22,8 +27,8 @@ export class ReviewService { private readonly tasks: PublishTaskScheduler, ) {} - list(principal: RequestPrincipal, status?: ReviewCase["status"]): ReviewCase[] { - return this.store.listReviews(principal.tenantId, status); + list(principal: RequestPrincipal, filters: ReviewListQuery): ReviewCase[] { + return this.store.listReviews(principal.tenantId, filters); } get(principal: RequestPrincipal, id: string): ReviewCase { @@ -72,8 +77,25 @@ export class ReviewService { instruction, previousDraft: current.activeDraft?.text, }; - const generated = await this.ai.generateDraft(input); - const checked = await this.ai.validateDraft({ ...input, draft: generated.value }); + const { generated, checked } = await (async () => { + try { + const generatedDraft = await this.ai.generateDraft(input); + const checkedDraft = await this.ai.validateDraft({ + ...input, + draft: generatedDraft.value, + }); + return { generated: generatedDraft, checked: checkedDraft }; + } catch (error) { + const latest = this.store.getReview(principal.tenantId, id); + if (latest.status === "generating" && latest.version === generating.version) { + this.store.transition(principal.tenantId, id, "needs_attention", generating.version); + } + this.store.appendAudit(principal, "draft.generation_failed", "review", id, { + errorCode: error instanceof Error ? error.name : "unknown", + }); + throw error; + } + })(); const deterministicFlags = detectHardStops(current.snapshot); const validation = { ...checked.value, @@ -189,47 +211,66 @@ export class ReviewService { const publishing = this.store.transition(principal.tenantId, id, "publishing", expectedVersion); this.store.appendAudit(principal, "review.approved", "review", id); this.store.appendAudit(principal, "reply.publish_started", "review", id); - const accessToken = await this.currentAccessToken(principal.tenantId); - const canonical = await this.google.getReview(accessToken, review.snapshot.googleReviewName); - if (canonical.updateTime !== review.snapshot.updateTime || canonical.existingReply) { - const attention = this.store.transition( + try { + const accessToken = await this.currentAccessToken(principal.tenantId); + const canonical = await this.google.getReview(accessToken, review.snapshot.googleReviewName); + if (canonical.updateTime !== review.snapshot.updateTime || canonical.existingReply) { + return this.store.transition( + principal.tenantId, + id, + "needs_attention", + publishing.version, + { + snapshot: canonical, + scheduledAt: null, + matchedRuleId: null, + validation: review.validation + ? { + ...review.validation, + valid: false, + riskFlags: [ + ...review.validation.riskFlags, + canonical.existingReply ? "existing_reply" : "review_updated", + ], + } + : null, + }, + ); + } + const published = await this.google.updateReply( + accessToken, + review.snapshot.googleReviewName, + review.activeDraft.text, + ); + const result = this.store.transition( principal.tenantId, id, - "needs_attention", + "published", publishing.version, { - snapshot: canonical, + publishedAt: published.updateTime, + publishedReply: published.comment, scheduledAt: null, - matchedRuleId: null, - validation: review.validation - ? { - ...review.validation, - valid: false, - riskFlags: [ - ...review.validation.riskFlags, - canonical.existingReply ? "existing_reply" : "review_updated", - ], - } - : null, }, ); - return attention; + this.store.appendAudit(principal, "reply.published", "review", id, { + googleUpdateTime: published.updateTime, + }); + this.store.recordPublished(result, manual); + return result; + } catch (error) { + const latest = this.store.getReview(principal.tenantId, id); + if (latest.status === "publishing" && latest.version === publishing.version) { + this.store.transition(principal.tenantId, id, "needs_attention", publishing.version, { + scheduledAt: null, + matchedRuleId: null, + }); + } + this.store.appendAudit(principal, "reply.publish_failed", "review", id, { + errorCode: error instanceof Error ? error.name : "unknown", + }); + throw error; } - const published = await this.google.updateReply( - accessToken, - review.snapshot.googleReviewName, - review.activeDraft.text, - ); - const result = this.store.transition(principal.tenantId, id, "published", publishing.version, { - publishedAt: published.updateTime, - publishedReply: published.comment, - scheduledAt: null, - }); - this.store.appendAudit(principal, "reply.published", "review", id, { - googleUpdateTime: published.updateTime, - }); - this.store.recordPublished(result, manual); - return result; } reject( diff --git a/apps/api/src/store.ts b/apps/api/src/store.ts index 1ae5f4a..ea4df28 100644 --- a/apps/api/src/store.ts +++ b/apps/api/src/store.ts @@ -6,6 +6,7 @@ import type { KnowledgeSource, RequestPrincipal, ReviewCase, + ReviewListQuery, ReviewSnapshot, } from "@reviewguard/contracts"; import type { GoogleTokens } from "@reviewguard/core"; @@ -31,10 +32,16 @@ export class MemoryStore { private readonly manualApprovalsByLocation = new Map(); private readonly publishedTodayByRule = new Map(); - listReviews(tenantId: string, status?: ReviewCase["status"]): ReviewCase[] { + listReviews(tenantId: string, filters: ReviewListQuery = { limit: 50 }): ReviewCase[] { return [...this.reviews.values()] - .filter((review) => review.tenantId === tenantId && (!status || review.status === status)) + .filter( + (review) => + review.tenantId === tenantId && + (!filters.status || review.status === filters.status) && + (!filters.locationId || review.snapshot.locationId === filters.locationId), + ) .sort((left, right) => right.createdAt.localeCompare(left.createdAt)) + .slice(0, filters.limit) .map((review) => structuredClone(review)); } diff --git a/apps/api/test/api.test.ts b/apps/api/test/api.test.ts index d4e5dba..1e18f44 100644 --- a/apps/api/test/api.test.ts +++ b/apps/api/test/api.test.ts @@ -26,6 +26,19 @@ describe("ReviewGuard API", () => { expect(response.json().data).toHaveLength(3); }); + it("applies review list filters and limits", async () => { + const limited = await app.inject({ method: "GET", url: "/v1/reviews?limit=1" }); + const missingLocation = await app.inject({ + method: "GET", + url: "/v1/reviews?locationId=missing-location", + }); + + expect(limited.statusCode).toBe(200); + expect(limited.json().data).toHaveLength(1); + expect(missingLocation.statusCode).toBe(200); + expect(missingLocation.json().data).toHaveLength(0); + }); + it("rejects stale approvals", async () => { const response = await app.inject({ method: "POST", diff --git a/apps/api/test/review.service.test.ts b/apps/api/test/review.service.test.ts new file mode 100644 index 0000000..bbe8306 --- /dev/null +++ b/apps/api/test/review.service.test.ts @@ -0,0 +1,77 @@ +import type { RequestPrincipal, ReviewSnapshot } from "@reviewguard/contracts"; +import { + FakeGoogleBusinessClient, + MockReplyProvider, + type ReplyModelProvider, +} from "@reviewguard/core"; +import { describe, expect, it } from "vitest"; +import { DEMO_TENANT_ID, DEMO_USER_ID } from "../src/demo.js"; +import { ReviewNotificationService } from "../src/notifications.js"; +import { ReviewService } from "../src/review.service.js"; +import { MemoryStore } from "../src/store.js"; +import { PublishTaskScheduler } from "../src/tasks.js"; + +const principal: RequestPrincipal = { + tenantId: DEMO_TENANT_ID, + userId: DEMO_USER_ID, + role: "owner", + mfaVerified: true, +}; + +function createService( + store: MemoryStore, + ai: ReplyModelProvider, + google: FakeGoogleBusinessClient, +): ReviewService { + return new ReviewService( + store, + ai, + google, + new ReviewNotificationService(store), + new PublishTaskScheduler(), + ); +} + +describe("ReviewService failure recovery", () => { + it("moves failed draft generation to needs_attention", async () => { + const store = new MemoryStore(); + const failingAi: ReplyModelProvider = { + generateDraft: async () => { + throw new Error("provider unavailable"); + }, + validateDraft: async () => { + throw new Error("provider unavailable"); + }, + }; + const service = createService(store, failingAi, new FakeGoogleBusinessClient()); + + await expect( + service.generate(principal, "55555555-5555-4555-8555-555555555552", 1), + ).rejects.toThrow("provider unavailable"); + + expect(store.getReview(principal.tenantId, "55555555-5555-4555-8555-555555555552").status).toBe( + "needs_attention", + ); + expect(store.listAudit(principal.tenantId)[0]?.action).toBe("draft.generation_failed"); + }); + + it("moves failed publication to needs_attention", async () => { + class FailingGoogleClient extends FakeGoogleBusinessClient { + override async getReview(_accessToken: string, _reviewName: string): Promise { + throw new Error("google unavailable"); + } + } + + const store = new MemoryStore(); + const service = createService(store, new MockReplyProvider(), new FailingGoogleClient()); + + await expect( + service.approve(principal, "55555555-5555-4555-8555-555555555551", 3), + ).rejects.toThrow("google unavailable"); + + expect(store.getReview(principal.tenantId, "55555555-5555-4555-8555-555555555551").status).toBe( + "needs_attention", + ); + expect(store.listAudit(principal.tenantId)[0]?.action).toBe("reply.publish_failed"); + }); +}); diff --git a/package.json b/package.json index 99f7a7a..79305e8 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "version": "0.1.0", "private": true, "description": "Human-controlled AI review reply platform for Google Business Profile.", - "license": "UNLICENSED", + "license": "MIT", "packageManager": "pnpm@11.19.0", "engines": { "node": ">=24.0.0", diff --git a/packages/contracts/src/enums.ts b/packages/contracts/src/enums.ts index ed252f6..cdd3c68 100644 --- a/packages/contracts/src/enums.ts +++ b/packages/contracts/src/enums.ts @@ -54,6 +54,7 @@ export type RiskFlag = z.infer; export const auditActionSchema = z.enum([ "review.received", "draft.generated", + "draft.generation_failed", "draft.revised", "review.approved", "review.rejected", diff --git a/packages/contracts/src/reviews.ts b/packages/contracts/src/reviews.ts index 462c40f..923aa23 100644 --- a/packages/contracts/src/reviews.ts +++ b/packages/contracts/src/reviews.ts @@ -56,6 +56,7 @@ export const reviewListQuerySchema = z.object({ locationId: z.string().optional(), limit: z.coerce.number().int().min(1).max(100).default(50), }); +export type ReviewListQuery = z.infer; export const revisionRequestSchema = z.object({ instruction: z.string().min(2).max(2_000), diff --git a/packages/database/src/client.ts b/packages/database/src/client.ts index 905c717..9479210 100644 --- a/packages/database/src/client.ts +++ b/packages/database/src/client.ts @@ -1,3 +1,4 @@ +import { sql } from "drizzle-orm"; import { drizzle } from "drizzle-orm/node-postgres"; import { Pool, type PoolConfig } from "pg"; import * as schema from "./schema.js"; @@ -23,9 +24,7 @@ export async function withTenant( ) => Promise, ): Promise { return database.transaction(async (transaction) => { - await transaction.execute( - `select set_config('app.tenant_id', '${tenantId.replaceAll("'", "")}', true)`, - ); + await transaction.execute(sql`select set_config('app.tenant_id', ${tenantId}, true)`); return operation(transaction); }); }