feat: COA classification API + auto-suggest on import#86
Conversation
- Add 10 classification endpoints: COA CRUD, suggest (L1), classify (L2), reconcile (L3), batch-suggest, stats, unclassified list, audit trail - Wire keyword-based auto-suggestion into TurboTenant import (L0 ingest) - Add SystemStorage methods for chart_of_accounts and trust-path operations - Reconciled transactions locked from modification (except L3/L4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review Please evaluate:
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
chittyfinance | 854bbf7 | Apr 10 2026, 08:03 PM |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds tenant-scoped Chart of Accounts (COA) APIs and a tiered transaction classification workflow (suggest/classify/reconcile) with audit logging, integrates COA suggestion into CSV imports, and implements storage methods and errors for COA, classification, stats, and audit retrieval. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as Classification API
participant Storage as SystemStorage
participant DB as Database
Client->>API: POST /api/classification/suggest\n(txId, description, category)
API->>Storage: findAccountCode(description, category)
Storage-->>API: suggestedCoaCode, confidence
API->>Storage: classifyTransaction(txId, suggestedCoaCode, {isSuggestion:true,...})
Storage->>DB: UPDATE transactions (suggestedCoaCode/confidence)
Storage->>DB: INSERT classification_audit (action: suggest)
API-->>Client: 200 Suggestion recorded
Client->>API: POST /api/classification/classify\n(txId, coaCode)
API->>Storage: getChartOfAccountByCode(coaCode, tenant)
alt COA exists
API->>Storage: classifyTransaction(txId, coaCode, {isSuggestion:false,...})
Storage->>DB: UPDATE transactions (coaCode, confidence, reconciled=false)
Storage->>DB: INSERT classification_audit (action: classify)
API-->>Client: 200 Classified
else COA missing
API-->>Client: 404 / 400
end
Client->>API: POST /api/classification/reconcile\n(txId)
API->>Storage: reconcileTransaction(txId, tenant, actorId)
Storage->>DB: VERIFY coaCode exists && not reconciled
Storage->>DB: UPDATE transactions (reconciled=true, reconciledBy)
Storage->>DB: INSERT classification_audit (action: reconcile)
API-->>Client: 200 Reconciled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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 |
Code Review — PR #86: COA Classification API + Auto-Suggest on ImportThe overall structure is solid and the trust-path concept is well-designed. The storage abstraction, audit trail, and import integration all follow existing patterns correctly. A few issues need attention before merge — one is a security gap, the rest are correctness/performance problems. Critical: Trust Level Is Declared but Never EnforcedThe PR description marks endpoints with L1/L2/L3/L4 labels, but // POST /api/classification/reconcile — L3: lock a classified transaction
classificationRoutes.post('/api/classification/reconcile', async (c) => {
// ❌ No check that the caller has L3 (auditor) roleMinimum fix for L2/L3 (using the existing role field on const caller = c.get('callerContext');
if (!['auditor', 'admin', 'owner'].includes(caller.role)) {
return c.json({ error: 'L2 classification requires auditor role or above' }, 403);
}L4 (COA governance) should similarly gate Critical: Cross-Tenant Audit Leak
// server/storage/system.ts
async getClassificationAudit(transactionId: string) {
return this.db
.select()
.from(schema.classificationAudit)
.where(eq(schema.classificationAudit.transactionId, transactionId));
// ❌ No tenantId filter
}If Tenant A knows a valid transaction UUID from Tenant B, they can read its full audit trail. Add // route
const audit = await storage.getClassificationAudit(transactionId, tenantId);
// storage
async getClassificationAudit(transactionId: string, tenantId: string) {
return this.db.select().from(schema.classificationAudit)
.where(and(
eq(schema.classificationAudit.transactionId, transactionId),
eq(schema.classificationAudit.tenantId, tenantId),
))
.orderBy(desc(schema.classificationAudit.createdAt));
}High:
|
| Issue | Severity | File |
|---|---|---|
| Trust level not enforced (any user can reconcile / govern COA) | Critical | routes/classification.ts |
| Cross-tenant audit trail leak (no tenantId filter) | Critical | storage/system.ts |
updateChartOfAccount lacks tenant scope |
High | storage/system.ts |
N+1 queries in batch-suggest + no ledger audit |
High | routes/classification.ts |
| Suggestion confidence not differentiated | Medium | routes/classification.ts, routes/import.ts |
| No COA code validation on L1 suggest | Medium | routes/classification.ts |
Raw SQL template instead of or() |
Medium | storage/system.ts |
| TOCTOU race in reconcile guard | Low | storage/system.ts |
Unbounded limit parameter |
Low | routes/classification.ts |
The two critical and one high security issues should be addressed before merge. Happy to look at a revised diff.
🤖 Generated with Claude Code
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1415924c90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/classification.ts`:
- Around line 109-130: The suggest route missing a catch for errors from
storage.classifyTransaction leads to inconsistent responses; wrap the call in a
try/catch in the POST /api/classification/suggest handler (the function handling
classificationRoutes.post('/api/classification/suggest')) and mirror the sibling
handlers (classify and reconcile): if storage.classifyTransaction throws the
"locked transaction" error (same sentinel/message/type they check for), return
c.json({ error: 'Transaction locked' }, 403); otherwise rethrow or pass the
error to the global handler so other failures remain unchanged.
- Around line 23-84: The POST /api/coa and PATCH /api/coa/:id handlers
(classificationRoutes.post and classificationRoutes.patch) are missing an L4
trust-level guard; add a check at the start of each handler to enforce the
advertised "L4 only" gate (e.g., read the request context like
c.get('trustLevel') or call the existing trust-check helper/middleware and
return a 403 if the caller is not L4), or wire an existing
ensureL4/requireTrustLevel middleware to these routes so only L4-trusted users
can run storage.createChartOfAccount and storage.updateChartOfAccount and
perform ledgerLog.
- Around line 101-106: The unvalidated `limit` query in the route handler for
classificationRoutes.get('/api/classification/unclassified') (and the similar
handler around lines 200-205) can be NaN, negative, or huge; use a Zod schema
from `@shared/schema` to parse and validate the query (e.g., an integer with min
and max bounds and a default) and replace the current parseInt usage with the
validated/clamped value before calling
storage.getUnclassifiedTransactions(tenantId, limit); ensure the handler uses
the schema.parse or safeParse result and falls back to the default or returns a
400 on invalid input so the limit passed into getUnclassifiedTransactions is
always a sane integer.
In `@server/storage/system.ts`:
- Around line 927-1011: Both classifyTransaction and reconcileTransaction
perform an update to schema.transactions and then insert into
schema.classificationAudit as separate statements; wrap each method's DB
operations in a single this.db.transaction(...) so the update and the insert run
inside the same transaction and will commit or roll back together. Specifically,
in classifyTransaction (handling both opts.isSuggestion and the authoritative
branch) wrap the update to schema.transactions and the subsequent
this.db.insert(schema.classificationAudit) call in one transaction callback, and
likewise in reconcileTransaction wrap the update to schema.transactions and the
subsequent audit insert in one transaction; keep all existing field values and
error handling but move the inserts/updates into the same transactional block to
ensure atomicity.
- Around line 1031-1036: The getClassificationAudit method currently only
filters by transactionId and must be tenant-scoped: change the signature of
getClassificationAudit(transactionId: string) to include tenantId (e.g.,
getClassificationAudit(transactionId: string, tenantId: string)), update the
query on schema.classificationAudit to add a where clause for
eq(schema.classificationAudit.tenantId, tenantId) alongside the existing
transactionId filter, and update any callers/routes that invoke
getClassificationAudit to pass the tenantId from the incoming request so the
audit lookup returns only tenant-scoped records.
- Around line 916-922: The updateChartOfAccount method currently updates rows by
id only; change its signature to accept tenantId (e.g., tenantId: string | null)
and add a tenant-scoping condition to the UPDATE WHERE clause on
schema.chartOfAccounts.tenantId so only the matching tenant (or a NULL tenantId
when appropriate) can be updated; ensure you handle the NULL case explicitly
(tenantId IS NULL) rather than comparing to the string "null", and keep
returning the updated row as before from updateChartOfAccount.
🪄 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
Run ID: 61e5e726-6211-416f-95ca-e3289bdc587b
📒 Files selected for processing (4)
server/app.tsserver/routes/classification.tsserver/routes/import.tsserver/storage/system.ts
There was a problem hiding this comment.
Pull request overview
Adds a Chart of Accounts (COA) + transaction classification workflow to the server, including tenant/global COA retrieval, classification/reconciliation operations, and automatic COA suggestions during TurboTenant imports.
Changes:
- Extends
SystemStoragewith COA CRUD + classification/audit/stats helpers. - Introduces new
/api/coaand/api/classification/*routes to drive the trust-path workflow. - Auto-suggests
suggested_coa_code(and confidence) during TurboTenant CSV import, and wires the new routes into the app + protected middleware prefixes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| server/storage/system.ts | Adds storage methods for COA queries/mutations and classification/reconciliation/audit/statistics operations. |
| server/routes/import.ts | Adds keyword-based COA auto-suggestion fields during TurboTenant transaction ingestion. |
| server/routes/classification.ts | New route group implementing COA endpoints and classification workflow endpoints. |
| server/app.ts | Registers the new route group and ensures /api/coa + /api/classification are protected by auth+tenant middleware. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses CodeRabbit/Copilot review comments: - POST/PATCH /api/coa now require L4 (owner/admin) role via getUserRoleForTenant — enforces the advertised trust gate - updateChartOfAccount now tenant-scoped (prevents cross-tenant writes and protects global templates from edits) - getClassificationAudit now tenant-scoped (prevents cross-tenant audit trail leakage) - classifyTransaction and reconcileTransaction wrap update + audit insert in db.transaction() for atomicity - POST /api/classification/suggest wrapped in try/catch to mirror classify/reconcile sibling handlers - All query params and bodies validated via Zod (limit bounded 1-500, transactionId required UUID, COA create/update schemas) - batch-suggest continues past locked rows instead of failing hard Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review Please evaluate:
|
Code Review: COA Classification API + Auto-Suggest on ImportGood foundational work on the trust-path classification system. The architecture aligns well with the L0–L4 levels documented in CLAUDE.md, and the atomic transaction pattern in 🔴 Security / Auth Issues1. L2 and L3 operations have no role enforcement
Suggested fix — add a const CLASSIFY_ROLES = new Set(['owner', 'admin', 'manager']);
const RECONCILE_ROLES = new Set(['owner', 'admin']);And call it at the top of each handler. 2.
const txIdParsed = z.string().uuid().safeParse(c.req.param('transactionId'));
if (!txIdParsed.success) return c.json({ error: 'invalid_transaction_id' }, 400);3. async function requireL4(c: any): Promise<Response | null>This drops type safety. Should be 4.
🟡 Bug: Partial
|
🧩 Analysis chain🏁 Script executed: Length of output: 106397 🏁 Script executed: Length of output: 6425
🔐 Security Implications🔴 Critical — Reconcile Lock Bypass via
|
| Change | Type | Risk |
|---|---|---|
7 new nullable columns on transactions table |
Additive schema | Low — nullable, no existing constraint changed |
GET /api/transactions now returns new fields (coaCode, suggestedCoaCode, reconciled, etc.) |
Additive response fields | Low for REST; could break strict typed deserialization clients |
Reconciled transactions now locked in classifyTransaction |
Behavioral | Medium — pre-existing client workflows calling classify on old reconciled rows will receive 400/403 (though currently bypassable via PATCH) |
POST /api/import/turbotenant now auto-populates suggestedCoaCode |
Behavioral | Low — additive fields on created records |
New protected prefixes /api/coa, /api/classification |
Net-new paths | None — no existing consumers |
The most impactful behavioral change is the reconcile lock — once the PATCH bypass is fixed, existing integrations that modify transactions post-reconciliation will start receiving errors and should be documented in the changelog.
Summary
| Severity | Issue |
|---|---|
| 🔴 Critical | Reconcile lock fully bypassable via PATCH /api/transactions |
| 🟠 High | No role gate on L2 classify / L3 reconcile / batch-suggest |
| 🟠 High | :id param in PATCH /api/coa/:id not UUID-validated |
| 🟡 Medium | batch-suggest DoS — 500 serial writes, no rate limit, no role gate |
| 🟡 Medium | Silent catch {} in batch-suggest — zero observability |
| 🟡 Medium | requireL4(c: any) — untyped context |
| 🟡 Medium | coaCode reflected in error with no max length |
| ✅ None | Credential exposure |
| ✅ None | Supply chain (zero new deps) |
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
server/routes/classification.ts (1)
198-235:⚠️ Potential issue | 🔴 CriticalEnforce real L2/L3 authorization before these writes.
Both handlers hardcode
trustLevel: 'L2'/trustLevel: 'L3'after only checking tenant membership, so any tenant user can authoritatively classify or reconcile a transaction. That bypasses the trust-path model entirely.Also applies to: 238-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/classification.ts` around lines 198 - 235, The handler sets trustLevel: 'L2' without verifying the caller is authorized to assert L2; before calling storage.classifyTransaction in classificationRoutes.post('/api/classification/classify') (and the similar reconcile handler around lines 238-263), verify the caller's authority to assign that trust level (e.g., call an existing auth utility or add authorizeTrustLevel(userId, tenantId, 'L2'|'L3') that returns true/throws) and return 403 if not authorized; only then pass the validated trustLevel into storage.classifyTransaction (and storage.reconcileTransaction), otherwise do not elevate trustLevel based solely on tenant membership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/classification.ts`:
- Around line 193-195: The three catch blocks in server/routes/classification.ts
(the catches at the shown ranges) currently return 403 with the raw exception
message for every error; change each to only map known domain errors to
appropriate HTTP statuses and user-safe messages (e.g., if (err instanceof
ValidationError) return c.json({ error: "Invalid input" }, 400); if (err
instanceof NotFoundError) return c.json({ error: "Not found" }, 404); if (err
instanceof AuthorizationError) return c.json({ error: "Forbidden" }, 403);), and
for all other/unexpected errors either rethrow/pass to the shared/global error
handler or return a 500 with a generic message (do not include err.message).
Update the catch blocks at the three locations accordingly and reference the
domain error classes/types used across the module to detect known errors.
- Around line 179-189: The /suggest handler is currently accepting any coaCode
from parsed.data and passing it into storage.classifyTransaction (transactionId,
tenantId, coaCode...), which can create unresolvable suggestions; add the same
validation used by the /classify path to verify the submitted coaCode exists in
the tenant/global COA list before calling storage.classifyTransaction. Locate
the handler that reads const { transactionId, coaCode, confidence, reason } =
parsed.data and, before invoking storage.classifyTransaction, call the existing
COA lookup/validation routine (the same helper or storage method / validation
logic used by /classify) and return a 4xx error if the code is unknown; only
call storage.classifyTransaction when validation succeeds.
In `@server/storage/system.ts`:
- Around line 965-998: The audit insert treats suggestion updates as
authoritative; update the logic around updateSet and the classification audit
insert so that when opts.isSuggestion is true you record an audit row that
distinguishes suggestions (e.g., action = 'suggest' or 'suggest_reclassify', or
add a boolean like isSuggestion and a targetField = 'suggestedCoaCode') instead
of 'classify'/'reclassify'; modify the values passed to
trx.insert(schema.classificationAudit).values(...) to include this flag/action
and ensure newCoaCode still reflects coaCode and action determination uses
opts.isSuggestion (and previousCoaCode as needed) so the audit trail accurately
reflects suggestion vs authoritative classification.
- Around line 953-985: The current check of tx.reconciled (from getTransaction)
happens before starting the DB transaction and can be raced; fix by enforcing
the reconciled lock in the atomic update: add a WHERE clause to the
trx.update(...) that requires schema.transactions.reconciled = false (or NOT
reconciled) so the UPDATE fails if the row was reconciled concurrently, and then
check the update result/row count from the trx.update call — if no rows were
updated, throw the same 'Transaction is reconciled — only L3/L4 can modify'
error. Reference getTransaction, tx.reconciled, db.transaction,
trx.update(...).set(updateSet), and schema.transactions in your change.
---
Duplicate comments:
In `@server/routes/classification.ts`:
- Around line 198-235: The handler sets trustLevel: 'L2' without verifying the
caller is authorized to assert L2; before calling storage.classifyTransaction in
classificationRoutes.post('/api/classification/classify') (and the similar
reconcile handler around lines 238-263), verify the caller's authority to assign
that trust level (e.g., call an existing auth utility or add
authorizeTrustLevel(userId, tenantId, 'L2'|'L3') that returns true/throws) and
return 403 if not authorized; only then pass the validated trustLevel into
storage.classifyTransaction (and storage.reconcileTransaction), otherwise do not
elevate trustLevel based solely on tenant membership.
🪄 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
Run ID: bd124be4-ab2f-4246-95b5-da8f985d6699
📒 Files selected for processing (2)
server/routes/classification.tsserver/storage/system.ts
Addresses the 4 post-fix CodeRabbit critical/major findings: - **Concurrency: reconciled lock bypass (critical)** classifyTransaction now enforces the reconciled lock inside the UPDATE WHERE clause itself (eq(reconciled, false)) for L0/L1/L2 writes. If the row was reconciled between the initial SELECT and the UPDATE, the transaction rolls back and a ClassificationError is thrown. L3/L4 continue to have unconditional write access. - **Audit naming: suggestions labeled 'classify' (major)** classification_audit.action now uses 'suggest'/'re-suggest' when isSuggestion is true, distinguishing L1 writes from authoritative L2+ writes. previousCoaCode also tracks the prior suggestion (not the authoritative code) for suggestion-path audits. - **Suggest missing COA validation (major)** /api/classification/suggest now validates coaCode against the tenant's COA (same check as /classify) to prevent dangling unresolvable suggestions. - **Error mapping: 403 catch-all (major)** Introduced ClassificationError sentinel with codes: reconciled_locked, not_classified, transaction_not_found. Routes use mapClassificationError() to return stable HTTP responses (403/400/404) and re-throw unknown errors to the shared handler so DB/runtime failures surface as 500 instead of being masked as 403 with raw exception text. - batch-suggest now only swallows reconciled_locked errors; other failures propagate instead of being silently ignored. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review Please evaluate:
|
|
Tip For best results, initiate chat on the files or code changes.
🔐 Security Implications🔴 Critical — Reconcile Lock Bypass via
|
| Change | Type | Risk |
|---|---|---|
7 new nullable columns on transactions table |
Additive schema | Low — nullable, no existing constraint changed |
GET /api/transactions now returns new fields (coaCode, suggestedCoaCode, reconciled, etc.) |
Additive response fields | Low for REST; could break strict typed deserialization clients |
Reconciled transactions now locked in classifyTransaction |
Behavioral | Medium — pre-existing client workflows calling classify on old reconciled rows will receive 400/403 (though currently bypassable via PATCH) |
POST /api/import/turbotenant now auto-populates suggestedCoaCode |
Behavioral | Low — additive fields on created records |
New protected prefixes /api/coa, /api/classification |
Net-new paths | None — no existing consumers |
The most impactful behavioral change is the reconcile lock — once the PATCH bypass is fixed, existing integrations that modify transactions post-reconciliation will start receiving errors and should be documented in the changelog.
Summary
| Severity | Issue |
|---|---|
| 🔴 Critical | Reconcile lock fully bypassable via PATCH /api/transactions |
| 🟠 High | No role gate on L2 classify / L3 reconcile / batch-suggest |
| 🟠 High | :id param in PATCH /api/coa/:id not UUID-validated |
| 🟡 Medium | batch-suggest DoS — 500 serial writes, no rate limit, no role gate |
| 🟡 Medium | Silent catch {} in batch-suggest — zero observability |
| 🟡 Medium | requireL4(c: any) — untyped context |
| 🟡 Medium | coaCode reflected in error with no max length |
| ✅ None | Credential exposure |
| ✅ None | Supply chain (zero new deps) |
Code Review — COA Classification API + Auto-Suggest on ImportOverall this is solid, well-structured work. The Critical — Trust-level role enforcement is missing for L1/L2/L3File:
Suggested fix: add role guards consistent with the trust model (e.g. Bug — Race condition in
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Return tenant-specific overrides + global defaults (tenant_id IS NULL) | ||
| return this.db | ||
| .select() | ||
| .from(schema.chartOfAccounts) | ||
| .where( | ||
| sql`${schema.chartOfAccounts.tenantId} = ${tenantId} OR ${schema.chartOfAccounts.tenantId} IS NULL`, | ||
| ) | ||
| .orderBy(asc(schema.chartOfAccounts.code)); |
There was a problem hiding this comment.
getChartOfAccounts() currently returns both tenant-specific rows and global (tenant_id IS NULL) rows, which means if a tenant overrides a global code you’ll return duplicate codes. If callers expect “overrides + defaults”, consider de-duping by code (prefer tenant row) via a UNION/CTE/window function or by querying tenant rows first and merging in code order.
| // Return tenant-specific overrides + global defaults (tenant_id IS NULL) | |
| return this.db | |
| .select() | |
| .from(schema.chartOfAccounts) | |
| .where( | |
| sql`${schema.chartOfAccounts.tenantId} = ${tenantId} OR ${schema.chartOfAccounts.tenantId} IS NULL`, | |
| ) | |
| .orderBy(asc(schema.chartOfAccounts.code)); | |
| // Return the effective chart of accounts for the tenant: | |
| // tenant-specific rows override global defaults with the same code. | |
| const tenantAccounts = await this.db | |
| .select() | |
| .from(schema.chartOfAccounts) | |
| .where(eq(schema.chartOfAccounts.tenantId, tenantId)) | |
| .orderBy(asc(schema.chartOfAccounts.code)); | |
| const globalAccounts = await this.db | |
| .select() | |
| .from(schema.chartOfAccounts) | |
| .where(isNull(schema.chartOfAccounts.tenantId)) | |
| .orderBy(asc(schema.chartOfAccounts.code)); | |
| const tenantCodes = new Set(tenantAccounts.map((account) => account.code)); | |
| return [...tenantAccounts, ...globalAccounts.filter((account) => !tenantCodes.has(account.code))].sort( | |
| (a, b) => a.code.localeCompare(b.code), | |
| ); |
| const tx = await this.getTransaction(txId, tenantId); | ||
| if (!tx) return undefined; | ||
|
|
||
| const previousCoaCode = tx.coaCode; | ||
| const previousSuggested = tx.suggestedCoaCode; | ||
| const now = new Date(); | ||
|
|
||
| // Decide which fields to update | ||
| const updateSet = opts.isSuggestion | ||
| ? { | ||
| // L1: write to suggested_coa_code only | ||
| suggestedCoaCode: coaCode, | ||
| classificationConfidence: opts.confidence ?? null, | ||
| updatedAt: now, | ||
| } | ||
| : { | ||
| // L2+: write to authoritative coa_code | ||
| coaCode, | ||
| classifiedBy: opts.actorId, | ||
| classifiedAt: now, | ||
| classificationConfidence: opts.confidence ?? null, | ||
| updatedAt: now, | ||
| }; | ||
|
|
||
| // Reconciled lock: L0/L1/L2 writes must reject rows that were reconciled | ||
| // between the initial SELECT and this UPDATE. We enforce this inside the | ||
| // WHERE clause of the UPDATE itself (conditional update) so concurrent | ||
| // reconciliations can't race us. | ||
| const canWriteReconciled = opts.trustLevel === 'L3' || opts.trustLevel === 'L4'; |
There was a problem hiding this comment.
classifyTransaction() reads the transaction (and captures previousCoaCode/previousSuggested) outside the DB transaction that performs the UPDATE + audit INSERT. If the transaction is modified concurrently (e.g., another classification), the audit trail can record an incorrect previousCoaCode/action for this write. To keep the audit accurate, fetch the current row inside the same transaction (optionally with row locking) and derive previous*/action from that snapshot.
| const tx = await this.getTransaction(txId, tenantId); | |
| if (!tx) return undefined; | |
| const previousCoaCode = tx.coaCode; | |
| const previousSuggested = tx.suggestedCoaCode; | |
| const now = new Date(); | |
| // Decide which fields to update | |
| const updateSet = opts.isSuggestion | |
| ? { | |
| // L1: write to suggested_coa_code only | |
| suggestedCoaCode: coaCode, | |
| classificationConfidence: opts.confidence ?? null, | |
| updatedAt: now, | |
| } | |
| : { | |
| // L2+: write to authoritative coa_code | |
| coaCode, | |
| classifiedBy: opts.actorId, | |
| classifiedAt: now, | |
| classificationConfidence: opts.confidence ?? null, | |
| updatedAt: now, | |
| }; | |
| // Reconciled lock: L0/L1/L2 writes must reject rows that were reconciled | |
| // between the initial SELECT and this UPDATE. We enforce this inside the | |
| // WHERE clause of the UPDATE itself (conditional update) so concurrent | |
| // reconciliations can't race us. | |
| const canWriteReconciled = opts.trustLevel === 'L3' || opts.trustLevel === 'L4'; | |
| const now = new Date(); | |
| return await this.db.transaction(async (trx) => { | |
| const [tx] = await trx | |
| .select({ | |
| coaCode: schema.transactions.coaCode, | |
| suggestedCoaCode: schema.transactions.suggestedCoaCode, | |
| }) | |
| .from(schema.transactions) | |
| .where(and( | |
| eq(schema.transactions.id, txId), | |
| eq(schema.transactions.tenantId, tenantId), | |
| )) | |
| .limit(1); | |
| if (!tx) return undefined; | |
| const previousCoaCode = tx.coaCode; | |
| const previousSuggested = tx.suggestedCoaCode; | |
| // Decide which fields to update | |
| const updateSet = opts.isSuggestion | |
| ? { | |
| // L1: write to suggested_coa_code only | |
| suggestedCoaCode: coaCode, | |
| classificationConfidence: opts.confidence ?? null, | |
| updatedAt: now, | |
| } | |
| : { | |
| // L2+: write to authoritative coa_code | |
| coaCode, | |
| classifiedBy: opts.actorId, | |
| classifiedAt: now, | |
| classificationConfidence: opts.confidence ?? null, | |
| updatedAt: now, | |
| }; | |
| // Reconciled lock: L0/L1/L2 writes must reject rows that were reconciled | |
| // after the in-transaction snapshot and before this UPDATE. We enforce | |
| // this inside the WHERE clause of the UPDATE itself (conditional update) | |
| // so concurrent reconciliations can't race us. | |
| const canWriteReconciled = opts.trustLevel === 'L3' || opts.trustLevel === 'L4'; |
| await trx | ||
| .update(schema.transactions) | ||
| .set({ reconciled: true, reconciledBy: actorId, reconciledAt: now, updatedAt: now }) | ||
| .where(and(eq(schema.transactions.id, txId), eq(schema.transactions.tenantId, tenantId))); | ||
|
|
There was a problem hiding this comment.
reconcileTransaction() unconditionally updates the row and always inserts a new audit record. Calling reconcile twice (or racing requests) will generate duplicate 'reconcile' audit entries and re-write reconciledBy/reconciledAt. Consider making this a conditional update (e.g., WHERE reconciled=false) and returning a stable error (or no-op) when the transaction is already reconciled.
| await trx | |
| .update(schema.transactions) | |
| .set({ reconciled: true, reconciledBy: actorId, reconciledAt: now, updatedAt: now }) | |
| .where(and(eq(schema.transactions.id, txId), eq(schema.transactions.tenantId, tenantId))); | |
| const updatedRows = await trx | |
| .update(schema.transactions) | |
| .set({ reconciled: true, reconciledBy: actorId, reconciledAt: now, updatedAt: now }) | |
| .where(and( | |
| eq(schema.transactions.id, txId), | |
| eq(schema.transactions.tenantId, tenantId), | |
| eq(schema.transactions.reconciled, false), | |
| )) | |
| .returning({ id: schema.transactions.id }); | |
| if (updatedRows.length === 0) { | |
| return; | |
| } |
| const account = await storage.updateChartOfAccount(id, tenantId, { | ||
| ...parsed.data, | ||
| modifiedBy: userId, | ||
| }); | ||
|
|
There was a problem hiding this comment.
PATCH /api/coa/:id doesn’t handle uniqueness conflicts when updating code (there is a unique index on (tenant_id, code)). If a client changes the code to one that already exists for the tenant, this will likely surface as a 500 from the DB. Consider pre-checking for an existing code (excluding this id) or catching the constraint error and returning a 409 with a stable error shape.
| const code = findAccountCode(tx.description, tx.category ?? undefined); | ||
| try { | ||
| await storage.classifyTransaction(tx.id, tenantId, code, { | ||
| actorId: 'auto:keyword-match', | ||
| actorType: 'system', |
There was a problem hiding this comment.
batch-suggest performs a per-transaction classifyTransaction() call in a sequential loop; with limit up to 500 this can translate into hundreds of DB transactions and round-trips, increasing latency and timeout risk. Consider batching (single transaction for the whole batch, or bulk UPDATE + bulk INSERT audit) and/or running updates with a small concurrency limit.
| // GET /api/coa — list Chart of Accounts for current tenant (includes global defaults) | ||
| classificationRoutes.get('/api/coa', async (c) => { | ||
| const storage = c.get('storage'); | ||
| const tenantId = c.get('tenantId'); | ||
| const accounts = await storage.getChartOfAccounts(tenantId); | ||
| return c.json(accounts); | ||
| }); |
There was a problem hiding this comment.
This PR introduces a new route group with multiple behaviors (COA CRUD, suggest/classify/reconcile, batch-suggest, stats/audit) but there are no new tests covering these endpoints. The repo already has route-level tests using mocked storage (e.g., server/tests/routes-tenants.test.ts, routes-consumer-contract.test.ts); adding similar tests here would help prevent regressions and validate error mapping/role enforcement.
| // ── CHART OF ACCOUNTS ── | ||
|
|
||
| async getChartOfAccounts(tenantId: string) { | ||
| // Return tenant-specific overrides + global defaults (tenant_id IS NULL) | ||
| return this.db |
There was a problem hiding this comment.
SystemStorage gained substantial new COA/classification logic (COA listing/CRUD, trust-path classify/reconcile, stats/audit queries) but existing SystemStorage tests only cover basic construction/getAccounts/getSummary. Adding unit tests for the new methods (especially reconciled locking behavior and audit writes) would improve confidence and guard against subtle regressions.
Frontend for the trust-path classification workflow added in #86/#87. New: client/src/pages/Classification.tsx - Stats cards (total, classified, reconciled, unclassified + %) - Unclassified queue sorted by date/amount/confidence (ascending or descending) - Per-row actions: accept suggested code (one-click), classify as any COA code (dropdown), reconcile (L3 lock) - Three bulk actions: - Keyword Suggest: runs /api/classification/batch-suggest (L1) - AI Suggest (GPT): runs /api/classification/ai-suggest (L1, 25 at a time) - Accept High-Confidence: writes authoritative coa_code (L2) for rows passing the bulkAcceptCandidates guardrails bulkAcceptCandidates rules (defense in depth): - Confidence >= 0.8 (AI "success" threshold) - suggestedCoaCode !== '9010' (suspense means "needs a human") - |amount| <= $500 (large-dollar → manual review regardless of confidence) New: client/src/hooks/use-classification.ts - TanStack Query hooks scoped by tenantId for automatic cache invalidation on tenant switch - Queries: coa, stats, unclassified, audit trail - Mutations: classify (L2), suggest (L1 manual), reconcile (L3), batch-suggest (L1 keyword), ai-suggest (L1 GPT) Wiring: - /classification route added to App.tsx - Sidebar link added (cfo, accountant, bookkeeper roles) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat: AI-assisted COA classification with GPT-4o-mini
Adds a new POST /api/classification/ai-suggest endpoint that batches
up to 25 unclassified transactions through GPT-4o-mini with structured
JSON output, falling back to keyword matching on any failure.
Library (server/lib/classification-ai.ts):
- classifyBatchWithAI() — per-request OpenAI client (Workers-safe)
- Validates AI-returned codes against the tenant's COA (rejects hallucinations)
- Clamps confidence to [0, 1]
- Always returns one suggestion per input (gap-fills with keyword fallback)
- Respects CF AI Gateway via AI_GATEWAY_ENDPOINT env
Route:
- Only processes transactions without an existing suggested_coa_code
(respects prior human/auto work)
- Writes via L1 suggest (never authoritative coa_code)
- Returns {processed, suggested, aiCount, keywordCount, aiAvailable}
Tests (9 new):
- Empty input, missing key, API error, malformed JSON, missing choices
- Confidence clamping, hallucinated-code rejection, gap-filling
- Batch size guard (MAX_BATCH=25)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: PR #87 review feedback — docstring, silent catch, processed field
- Docstring now correctly describes gpt-4o-mini default (was: GPT-4o)
- Remove unused `date` field from ClassifiableTransaction interface
- ai-suggest only swallows ClassificationError.reconciled_locked;
DB/runtime failures now propagate to the shared error handler
instead of being silently skipped
- `processed` field is now consistent: all branches return txns.length
(count of transactions fetched and considered), matching the semantics
of the early-return paths
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat: classification review page with bulk-accept guardrails
Frontend for the trust-path classification workflow added in #86/#87.
New: client/src/pages/Classification.tsx
- Stats cards (total, classified, reconciled, unclassified + %)
- Unclassified queue sorted by date/amount/confidence (ascending or
descending)
- Per-row actions: accept suggested code (one-click), classify as any
COA code (dropdown), reconcile (L3 lock)
- Three bulk actions:
- Keyword Suggest: runs /api/classification/batch-suggest (L1)
- AI Suggest (GPT): runs /api/classification/ai-suggest (L1, 25 at a time)
- Accept High-Confidence: writes authoritative coa_code (L2) for rows
passing the bulkAcceptCandidates guardrails
bulkAcceptCandidates rules (defense in depth):
- Confidence >= 0.8 (AI "success" threshold)
- suggestedCoaCode !== '9010' (suspense means "needs a human")
- |amount| <= $500 (large-dollar → manual review regardless of confidence)
New: client/src/hooks/use-classification.ts
- TanStack Query hooks scoped by tenantId for automatic cache invalidation
on tenant switch
- Queries: coa, stats, unclassified, audit trail
- Mutations: classify (L2), suggest (L1 manual), reconcile (L3),
batch-suggest (L1 keyword), ai-suggest (L1 GPT)
Wiring:
- /classification route added to App.tsx
- Sidebar link added (cfo, accountant, bookkeeper roles)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
suggested_coa_code)chart_of_accounts(80 global accounts),classification_audit, and 7 new columns ontransactions— all live on NeonEndpoints
GET /api/coaGET /api/coa/globalPOST /api/coaPATCH /api/coa/:idGET /api/classification/statsGET /api/classification/unclassifiedPOST /api/classification/suggestsuggested_coa_code)POST /api/classification/classifycoa_codePOST /api/classification/reconcilePOST /api/classification/batch-suggestGET /api/classification/audit/:txIdTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit