Harden review workflow and repository checks

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.

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")

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(

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<string, number>();
   private readonly publishedTodayByRule = new Map<string, { date: string; count: number }>();
 
-  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));
   }
 

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",