docs: finance/books/ledger boundary contracts + proposals#119
Conversation
|
@coderabbitai review Please evaluate:
|
|
Warning Review limit reached
More reviews will be available in 52 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds documentation: clarifies API routing (/api vs /api/v1), publishes ChittyBooks↔ChittyFinance ownership contract, defines Mercury multitenant invariants and remediation, revises the ChittyBooks connector to use real services, and proposes a ChittyLedger naming-collision resolution. ChangesSystem Architecture Contracts and Proposals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Docs-only PR adding boundary contracts and proposals from a goal audit of the finance/books/ledger surfaces. No runtime or schema changes. Clarifies the /api/v1 prefix convention in CLAUDE.md, retires the fake books.chitty.cc live-API assumption, flags a schema gap for legal_person_chittyid on accounts, and proposes a rename plan for the legacy CHITTYOS/chittyledger repo.
Changes:
- Adds four new docs under
docs/contracts/anddocs/proposals/describing the engine/UI boundary, Mercury multitenant model, API prefix resolution, connector revisions, and naming plan. - Updates one line in
CLAUDE.mdto clarify resource routes (/api/<resource>) vs meta routes (/api/v1/*).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CLAUDE.md | Clarifies /api/v1 is meta-only; resource routes are unprefixed. |
| docs/contracts/chittybooks-chittyfinance.md | Defines ChittyBooks↔ChittyFinance engine/UI boundary and source-of-truth table. |
| docs/contracts/mercury-multitenant.md | Documents tenant + Legal Person isolation; flags missing legal_person_chittyid column. |
| docs/proposals/api-v1-prefix-fix.md | Resolves /api/v1/* 404 report as non-issue; proposes one-line CLAUDE.md note. |
| docs/proposals/ch1tty-connector-revision.md | Proposes routing through real services and gating dead CHITTYBOOKS_URL. |
| docs/proposals/chittyledger-naming-plan.md | Plan to rename CHITTYOS/chittyledger → -evidence-legacy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review — PR #119: docs: finance/books/ledger boundary contracts + proposalsScope: Docs-only — 5 new markdown files + 1 CLAUDE.md line edit. No runtime risk. OverviewThis PR documents the architecture audit of the finance/books/ledger surfaces. The research is thorough, the evidence is cited with file paths and line numbers, and the schema gap discovery is a genuine catch. The deploy-gate checklists and "not decided here" discipline are well-executed. FindingsBug / Correctness
Naming / Style
Tracking / Process
Minor
Summary
Recommendation: Open a GitHub issue for the |
![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 `@docs/contracts/chittybooks-chittyfinance.md`:
- Line 12: The table row that lists "ChittyLedger (legacy fork)" currently
references the wrong repository identifier `CHITTYOS/chittybooks`; update that
repo string to `CHITTYOS/chittyledger` so the legacy ledger entry points to the
correct repository (look for the row containing "ChittyLedger (legacy fork)" and
replace the `CHITTYOS/chittybooks` token with `CHITTYOS/chittyledger`), and make
the same fix for the duplicate occurrence noted around the other mentioned line.
In `@docs/contracts/mercury-multitenant.md`:
- Around line 14-22: Add a language tag to the fenced diagram block that begins
with "Person (P-Legal, e.g. "IT CAN BE LLC")" so it stops triggering
markdownlint MD040; update the opening triple-backtick to include a language
identifier (e.g., ```text) for that diagram block.
In `@docs/proposals/chittyledger-naming-plan.md`:
- Around line 16-25: The fenced code block that begins with ``` has no language
identifier causing markdownlint MD040; update that opening fence to include a
language such as text (e.g., change ``` to ```text or ```plaintext) so the block
is explicitly labeled and the linter warning is resolved.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aebb8db2-8511-4478-aed5-12b5c9fc1c30
📒 Files selected for processing (6)
CLAUDE.mddocs/contracts/chittybooks-chittyfinance.mddocs/contracts/mercury-multitenant.mddocs/proposals/api-v1-prefix-fix.mddocs/proposals/ch1tty-connector-revision.mddocs/proposals/chittyledger-naming-plan.md
![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: b5a1ff248b
ℹ️ 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".
* refactor(server): split routes into books vs accounting concerns Introduce a structural books/accounting boundary with no behavior change. Books writes facts (ingest, categorize, journal); accounting derives meaning (chart of accounts, reporting, tax, allocations). - move transactions/import/webhooks -> server/books/ - move accounts/reports/tax/allocations -> server/accounting/ - add concern-boundary READMEs for each tree - classification.ts stays in routes/ (straddles both); note added - fix import paths in app.ts and __tests__ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(deps): resolve @chittyos/schema lockfile drift Revert PR #103's refactor that introduced an unresolvable file: dependency. Root cause: PR #103 swapped the self-contained local scope-projector for a thin adapter over `@chittyos/schema`, declared as `file:../../CHITTYFOUNDATION/chittyschema`. That sibling-repo path only exists on the dev VM. In CI it cannot resolve, and worse, pnpm 10 refuses to record file: directory links in the lockfile importer block at all — so `pnpm install --frozen-lockfile` always sees a specifier mismatch ("1 dependencies were added: @chittyos/schema@file:..."), failing the Dependency Audit (High+) gate on every PR (#119, #120). No clean dependency form resolves in CI: the package is unpublished (npm 404), its dist/ is gitignored in the sibling repo (so a github: dep ships no build), and it has no prepare script. Fix: restore the pre-#103 self-contained implementation of server/lib/central-workflows.ts (175 lines, uses @neondatabase/serverless which is already a dependency) and drop the @chittyos/schema dependency. Public API (scopeLog, SCOPE_TYPES, ScopeStatus, etc.) is unchanged, so callers in workflows.ts and reports.ts are unaffected. Regenerated lockfile. Verified in a clean checkout (sibling repo absent, pnpm 10): pnpm install --frozen-lockfile -> exit 0 pnpm audit --prod --audit-level high ... -> exit 0 npm run check (tsc) -> exit 0 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chitcommit
force-pushed
the
goal/finance-books-ledger-substrate
branch
from
b5a1ff2 to
e15f239
Compare
|
@coderabbitai review Please evaluate:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== CLAUDE.md relevant section ==="
grep -n "api\|v1\|route\|mount" CLAUDE.md | head -30
echo "=== docs/contracts/chittybooks-chittyfinance.md ==="
cat docs/contracts/chittybooks-chittyfinance.md
echo "=== docs/contracts/mercury-multitenant.md ==="
cat docs/contracts/mercury-multitenant.md
echo "=== docs/proposals/api-v1-prefix-fix.md ==="
cat docs/proposals/api-v1-prefix-fix.md
echo "=== docs/proposals/ch1tty-connector-revision.md ==="
cat docs/proposals/ch1tty-connector-revision.md
echo "=== docs/proposals/chittyledger-naming-plan.md ==="
cat docs/proposals/chittyledger-naming-plan.md
echo "=== Check for secrets/tokens/keys in docs ==="
rg -i "(api.?key|secret|password|token|bearer|credential|private.?key|access.?key|webhook.?secret)" docs/ -n
echo "=== Check for hardcoded URLs with auth ==="
rg "https?://[^@\s]*:[^@\s]*@" docs/ -n
echo "=== Check for any package changes ==="
git diff origin/main --name-only | head -30
echo "=== Check for dependency file changes ==="
git diff origin/main -- package.json package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null | head -50Length of output: 40420
Security Implications
Credential Exposure RiskNone found in the 5 new files.
Dependency Supply Chain ConcernsNone. No Breaking API ChangesNone. No source code was changed. The SummaryThis PR is safe to merge from a security standpoint with one actionable recommendation:
Additionally, the ✅ Actions performedReview triggered.
|
Code Review — PR #119: docs: finance/books/ledger boundary contracts + proposalsSummary: Docs-only PR (267 additions, 1 deletion) that introduces 5 new contract/proposal files and a 1-line OverviewThis PR does good forensic work — verifying deployed surfaces, naming collisions, and dead code paths against actual DNS/file evidence rather than assumptions. The verification methodology (citing exact file paths, line numbers, What's Good
IssuesCRITICAL (Blocker)
MEDIUMFilename typo: This breaks Cross-repo line number references will go stale.
Line numbers in other repos drift immediately on the next commit. Since these are proposals (not contracts), consider citing symbols/function names instead of line numbers where possible, or add a "Verified at" date so readers know to re-verify.
The LOW / Nits
SecurityThe Mercury multitenancy model is correctly reasoned:
The Summary
The underlying analysis is solid and this docs infrastructure is useful. The blocker is the missing issue tracker entry for the schema gap — docs-only blockers get lost. The filename typo should be fixed before merge. Everything else is addressable in follow-up. Reviewed by Claude Sonnet 4.6 via Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e15f239ed8
ℹ️ 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".
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/contracts/chittybooks-chittyfinance.md (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect legacy repo identifier for ChittyLedger.
The contract points to
CHITTYOS/chittybooks, but based on the chittyledger naming plan in this proposal set, the legacy ledger collision isCHITTYOS/chittyledger.Suggested fix
-| ChittyLedger (legacy fork) | `CHITTYOS/chittybooks` | Express + React, in-memory, not deployed | n/a — DUPLICATE, candidate for retirement or repurpose as ChittyLedger-Evidence seed | +| ChittyLedger (legacy fork) | `CHITTYOS/chittyledger` | Express + React, in-memory, not deployed | n/a — DUPLICATE, candidate for retirement or repurpose as ChittyLedger-Evidence seed |Also applies to: 84-84
🤖 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 `@docs/contracts/chittybooks-chittyfinance.md` at line 12, Update the legacy repo identifier in the ChittyLedger table entry: replace the incorrect string `CHITTYOS/chittybooks` with the correct identifier `CHITTYOS/chittyledger` in the table row that currently reads "ChittyLedger (legacy fork) | `CHITTYOS/chittybooks`"; ensure the same replacement is applied to the duplicate occurrence noted (also at the second location mentioned).docs/proposals/chittyledger-naming-plan.md (1)
16-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
The fenced code block is missing a language identifier, which triggers markdownlint MD040. Add
textorplaintextto the opening fence.🤖 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 `@docs/proposals/chittyledger-naming-plan.md` around lines 16 - 25, The fenced code block containing the ChittyLedger tree should include a language identifier to satisfy markdownlint MD040; update the opening fence (the line starting with ```) to include a language token such as "text" or "plaintext" so it reads ```text (or ```plaintext) above the ChittyLedger block to silence the linter.
🧹 Nitpick comments (1)
docs/proposals/api-v1-prefix-fix.md (1)
21-21: ⚡ Quick winCapitalize "GitHub" for correctness.
The proper capitalization is "GitHub" with a capital "H".
📝 Proposed fix
-- All 5 in-repo references to `/api/v1` (`.github/workflows/register.yml`, `deploy/registration/chittyfinance.registration.json`, `client/src/pages/Landing.tsx:342`, `.claude/commands/quick-deploy.md:28`, plans) point at real endpoints. **Nothing to fix.** +- All 5 in-repo references to `/api/v1` (`.GitHub/workflows/register.yml`, `deploy/registration/chittyfinance.registration.json`, `client/src/pages/Landing.tsx:342`, `.claude/commands/quick-deploy.md:28`, plans) point at real endpoints. **Nothing to fix.**🤖 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 `@docs/proposals/api-v1-prefix-fix.md` at line 21, The document text "GitHub" is not capitalized correctly; locate the sentence that reads "All 5 in-repo references to `/api/v1` (...) point at real endpoints. **Nothing to fix.**" and any nearby mention of "Github" or "GitHub", and update the word to the correct capitalization "GitHub" (ensure every occurrence in this proposal line and surrounding notes uses "GitHub").
🤖 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.
Duplicate comments:
In `@docs/contracts/chittybooks-chittyfinance.md`:
- Line 12: Update the legacy repo identifier in the ChittyLedger table entry:
replace the incorrect string `CHITTYOS/chittybooks` with the correct identifier
`CHITTYOS/chittyledger` in the table row that currently reads "ChittyLedger
(legacy fork) | `CHITTYOS/chittybooks`"; ensure the same replacement is applied
to the duplicate occurrence noted (also at the second location mentioned).
In `@docs/proposals/chittyledger-naming-plan.md`:
- Around line 16-25: The fenced code block containing the ChittyLedger tree
should include a language identifier to satisfy markdownlint MD040; update the
opening fence (the line starting with ```) to include a language token such as
"text" or "plaintext" so it reads ```text (or ```plaintext) above the
ChittyLedger block to silence the linter.
---
Nitpick comments:
In `@docs/proposals/api-v1-prefix-fix.md`:
- Line 21: The document text "GitHub" is not capitalized correctly; locate the
sentence that reads "All 5 in-repo references to `/api/v1` (...) point at real
endpoints. **Nothing to fix.**" and any nearby mention of "Github" or "GitHub",
and update the word to the correct capitalization "GitHub" (ensure every
occurrence in this proposal line and surrounding notes uses "GitHub").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d10a2f6-50cc-42a2-9cbb-a414a8acbd92
📒 Files selected for processing (6)
CLAUDE.mddocs/contracts/chittybooks-chittyfinance.mddocs/contracts/mercury-multitenant.mddocs/proposals/api-v1-prefix-fix.mddocs/proposals/ch1tty-connector-revision.mddocs/proposals/chittyledger-naming-plan.md
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
Implements step 5 of the chittyledger naming-collision plan (PR #119). Documents ChittyFinance as the writer for the ChittyLedger-Finance projection over the substrate at ledger.chitty.cc. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements step 3 of the chittyledger naming-collision plan (chittyapps/chittyfinance#119). The nested chittyledger/ directory was working-tree noise from a prior submodule experiment — not an actual submodule (no .gitmodules entry, no .git inside, tracked as plain files). Deletes 104 stray files. No code references this path. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements step 3 of the chittyledger naming-collision plan (chittyapps/chittyfinance#119). The nested chittyfinance/ directory was working-tree noise from a prior experiment — not a submodule (no .gitmodules), not a nested git repo, tracked as plain files. The canonical ChittyFinance repo lives at CHITTYAPPS/chittyfinance. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…1 prefix fix proposal Three artifacts for /goal "ChittyFinance + ChittyBooks as finance/bookkeeping surface over ChittyLedger". All evidence-grounded — no invented schemas, no deploys. - docs/contracts/chittybooks-chittyfinance.md: engine/UI boundary, source-of-truth table, deploy decision (recommend retire chittybooks.chitty.cc, fold UI into ChittyFinance) with reconciled Charter/repo mismatch flagged - docs/contracts/mercury-multitenant.md: tenant_id + legal_person_chittyid binding rules, Wave mapping requirement, ChittyBooks reconciliation surface, explicit out-of-scope for direct Mercury calls - docs/proposals/api-v1-prefix-fix.md: finance.chitty.cc /api/v1/* 404 is a docs↔code mismatch (only /api/v1/documentation exists; data routes are /api/*). Verified ChittyCommand consumer already uses /api/* — Option 1 (fix docs) has zero downstream breakage.
- ch1tty-connector-revision.md: names the dead CHITTYBOOKS_URL endpoint in chittycommand's mercury-chittybooks plan + booksClient at integrations.ts:618, routes all bookkeeping reads/writes through real finance.chitty.cc paths - chittyledger-naming-plan.md: substrate (FOUNDATION/chittyledger) + projections (Finance, Evidence). Plan to rename CHITTYOS/chittyledger → -evidence-legacy, clean nested noise, add disambiguation banners.
…d schema gap, resolve api-v1 as non-issue
- contracts/chittybooks-chittyfinance.md: DECISION (was Recommendation) with
6-row repo-file proof table — Python+cloudrun+no wrangler+no DNS+engine already
in chittyfinance+tenant boundary at finance schema
- contracts/mercury-multitenant.md: CRITICAL adversarial finding —
legal_person_chittyid column absent from accounts schema; remediation paths
A/B; tenant_id NOT NULL defense-in-depth verified OK
- proposals/api-v1-prefix-fix.md: resolved as non-issue; /api/v1/{status,
metrics,documentation} are real meta routes (server/routes/health.ts:21,38),
resource routes correctly at /api/<resource>. Single CLAUDE.md sentence added.
- CLAUDE.md: clarify route mount split (meta under /api/v1, resources under /api)
…deploy choice as operator gate
chitcommit
force-pushed
the
goal/finance-books-ledger-substrate
branch
from
e15f239 to
fea07c1
Compare
|
@coderabbitai review Please evaluate:
|
|
The review for PR Security Implications
Credential Exposure RiskNone found.
Dependency Supply Chain ConcernsNone. No Breaking API ChangesNone. No source code was changed. Merge RecommendationSafe to merge from a security standpoint. Two follow-up actions are recommended before reconciliation reports go live:
🧠 Learnings used✅ Action performedReview finished.
|
Code Review — PR #119: docs/finance/books/ledger boundary contracts + proposalsSummary: Docs-only branch (267 additions, 1 deletion across 6 files). Surfaces real architectural findings from a ✅ What's working well
🔴 Should fix before merge1. A document filed as a contract implying authoritative boundary that simultaneously disclaims its own canonicity is contradictory. Downstream readers (and tooling/agents) will treat
Leaving it in 🟡 Should address (or explicitly defer)2. The
But a docs-only PR means this finding lives exclusively in a markdown file. Schema changes in this repo require coordinated cutover ( 3. CLAUDE.md routes line is now overlong for its context. Before: After: CLAUDE.md is a terse at-a-glance reference. The three added sentences are correct but belong in the proposal doc (where they already live). The in-file comment table should stay one line. Suggestion: 4. If this is intentional branding, it's fine — but it's inconsistent with every other file in this PR and could break glob patterns or future searches. Worth an explicit note confirming it's intentional, or rename to 🟢 Minor / optional5. Cross-org deploy gates are mixed into this repo's gate list. Several deploy gates in 6. The paths are verified as mounted on ChittyFinance — not verified as working end-to-end from ChittyBooks (which isn't deployed). The header could be misread as "verified round-trip." A small clarification like "Path (verified mounted on ChittyFinance)" would prevent that ambiguity for future readers. Summary
Good audit work overall. The schema gap finding alone justifies the PR. 🤖 Generated with Claude Code |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fea07c19ea
ℹ️ 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".
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@coderabbitai review Please evaluate:
|
Address review findings from coderabbitai and chatgpt-codex-connector: - chittybooks contract: fix legacy fork repo ref (chittyledger, not chittybooks); document hybrid auth (Bearer service token OR JWT session cookie); narrow /api/transactions filter claim to what the handler actually accepts (?limit); enumerate the real allocation routes (/rules /preview /execute /run); mark /api/reports/reconciliation and aggregator endpoints as proposed. - mercury contract: integration_account_links table does not exist yet — document as contract-only with interim metadata path; point native Mercury webhook at /api/webhooks/mercury/:tenantId (not the legacy shim); note legal_person_chittyid is resolved downstream, not in the webhook handler; mark ?source= filter and last-sync timestamps as proposed. - naming plan: add language tag to fenced block (MD040); use POST /api/entries for ledger writer paths (matches server/lib/ledger-client.ts). - ch1tty-connector revision: ledger write path is /api/entries, not /entries.
chitcommit
force-pushed
the
goal/finance-books-ledger-substrate
branch
from
9117ce6 to
4524ab1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/contracts/mercury-multitenant.md (2)
31-34: 🏗️ Heavy liftConsider adding tenant_id assertion at the mercury-proxy layer for defense-in-depth.
The current design relies entirely on the caller (ChittyFinance) to enforce tenancy before reaching the mercury-proxy. While this may be sufficient today, adding a proxy-side
tenant_idassertion would provide defense-in-depth against caller bugs or future refactoring that inadvertently bypasses tenant checks.The proxy is a critical write boundary to Mercury's API. A lightweight tenant_id validation at this layer would catch cross-tenant write attempts before they reach the external service.
🤖 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 `@docs/contracts/mercury-multitenant.md` around lines 31 - 34, Add a lightweight tenant_id assertion inside the mercury-proxy (CHITTYOS/mercury-proxy) request handling path: extract tenant_id from the incoming request (e.g. body/params or resolved from X-Mercury-Token) and compare it to the tenant_id implied by the caller’s authentication/lookup logic (the same mapping used by ChittyFinance/HMAC webhook resolution). Implement this as a small middleware or check in the proxy request handler that rejects requests with mismatched or missing tenant_id with a 4xx error before proxying to Mercury; reference the proxy entrypoint/handler that currently processes incoming write requests and the code that reads X-Mercury-Token so the assertion is inserted early and is defense-in-depth only.
68-68: ⚡ Quick winClarify the deploy gate to reflect both Path A and Path B options.
Line 67 correctly states that "column or metadata interim" must exist before reconciliation reports go live. However, line 68 only checks for "column present", which excludes the Path B interim metadata approach documented in line 59.
Consider rewording line 68 to align with line 67:
legal_person_chittyidcolumn or interim metadata present onaccounts(verify indatabase/system.schema.tsorserver/lib/legal-person-binding.tsbefore reconciliation reports go live).This ensures operators know that Path B (interim metadata) satisfies the deploy gate, not only Path A (schema migration).
🤖 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 `@docs/contracts/mercury-multitenant.md` at line 68, Update the deploy-gate checklist item to allow either the schema column or the Path B interim metadata: change the line referencing `legal_person_chittyid` on `accounts` to read "`legal_person_chittyid` column or interim metadata present on `accounts`" and add the verification locations `database/system.schema.ts` or `server/lib/legal-person-binding.ts` so operators know to check either the schema or the interim metadata implementation before reconciliation reports go live.
🤖 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 `@docs/proposals/chittyledger-naming-plan.md`:
- Line 35: Step 5 in docs/proposals/chittyledger-naming-plan.md duplicates
content already present in CHITTYAPPS/chittyfinance/CHITTY.md (the existing
"ChittyLedger-Finance" section). Update Step 5 to either remove it entirely or
change the action verb from "Add" to "Verify" and add a brief note referencing
the existing "ChittyLedger-Finance" section in CHITTY.md (so the checklist
clearly instructs verification rather than addition).
---
Nitpick comments:
In `@docs/contracts/mercury-multitenant.md`:
- Around line 31-34: Add a lightweight tenant_id assertion inside the
mercury-proxy (CHITTYOS/mercury-proxy) request handling path: extract tenant_id
from the incoming request (e.g. body/params or resolved from X-Mercury-Token)
and compare it to the tenant_id implied by the caller’s authentication/lookup
logic (the same mapping used by ChittyFinance/HMAC webhook resolution).
Implement this as a small middleware or check in the proxy request handler that
rejects requests with mismatched or missing tenant_id with a 4xx error before
proxying to Mercury; reference the proxy entrypoint/handler that currently
processes incoming write requests and the code that reads X-Mercury-Token so the
assertion is inserted early and is defense-in-depth only.
- Line 68: Update the deploy-gate checklist item to allow either the schema
column or the Path B interim metadata: change the line referencing
`legal_person_chittyid` on `accounts` to read "`legal_person_chittyid` column or
interim metadata present on `accounts`" and add the verification locations
`database/system.schema.ts` or `server/lib/legal-person-binding.ts` so operators
know to check either the schema or the interim metadata implementation before
reconciliation reports go live.
🪄 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: 5282ab86-4dd6-4450-a53d-854e28ff3edf
📒 Files selected for processing (6)
CLAUDE.mddocs/contracts/chittybooks-chittyfinance.mddocs/contracts/mercury-multitenant.mddocs/proposals/api-v1-prefix-fix.mddocs/proposals/ch1tty-connector-revision.mddocs/proposals/chittyledger-naming-plan.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
|
@coderabbitai review Please evaluate:
|
|
Security Implications
Credential Exposure RiskNone found.
Dependency Supply Chain ConcernsNone. No
Breaking API ChangesNone. No source code was changed. Merge RecommendationSafe to merge from a security standpoint. Two follow-up actions are recommended before reconciliation reports go live:
🧠 Learnings used✅ Action performedReview finished.
|
|
Security Implications
Credential Exposure RiskNone found in the 5 new markdown files.
Dependency Supply Chain ConcernsNone from the documentation changes. The original PR contained no Breaking API ChangesNone. No source code was changed by the PR author. Merge RecommendationSafe to merge the documentation content. Two blockers to track before reconciliation reports go live:
🧠 Learnings used✅ Action performedReview finished.
|
Code Review — PR #119: docs: finance/books/ledger boundary contracts + proposalsOverall: This is a well-researched, docs-only PR that accurately maps the current state of the ChittyFinance ecosystem and surfaces real gaps with receipts. Approachable in principle, with a few items to address before merge. What the PR doesAdds 5 new documentation files and one
IssuesFilename typo ( The filename uses a CRITICAL schema gap has no tracking issue
This is marked CRITICAL with two remediation paths (schema migration vs. interim metadata). However, there is no linked GitHub issue to track it. A finding this significant that lives only in a docs file risks being forgotten, especially since
The updated line in The original file uses terse, right-aligned descriptions (e.g., Minor notes
Summary
Suggest: fix the filename, trim the 🤖 Generated with Claude Code |
- CLAUDE.md: OpenAPI spec is partial, not authoritative; app.ts mounts are source of truth until spec is completed. - chittybooks contract: clarify writer for projection tables is ChittyFinance via POST /api/entries (matches naming plan + CHITTY.md); ChittyTrace is an upstream feeder, not a second writer. - ch1tty-connector revision: Mercury reads route through ChittyConnect (the canonical bank-API path); direct api.mercury.com is fallback only. - naming plan: step 5 changed from "Add" to "Verify" since CHITTY.md:109-114 already has the ChittyLedger-Finance section.
|
@coderabbitai review Please evaluate:
|
|
Tip For best results, initiate chat on the files or code changes.
Security Implications
Credential Exposure RiskNone found in the 5 new markdown files.
Dependency Supply Chain ConcernsNone from the documentation changes. The original PR contained no Breaking API ChangesNone. No source code was changed by the PR author. Merge RecommendationSafe to merge the documentation content. Track these before reconciliation reports go live:
🧠 Learnings used |
Code Review — PR #119: docs: finance/books/ledger boundary contracts + proposalsSummary: Docs-only PR adding 5 contract/proposal docs and a minor What this PR does well
Issues🔴 Internal contradiction between
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e179c20693
ℹ️ 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".
|
|
||
| ## API Surface ChittyBooks consumes | ||
|
|
||
| All paths are under `https://finance.chitty.cc`. Auth (per `server/middleware/auth.ts` hybridAuth): either a service `Authorization: Bearer <CHITTY_AUTH_SERVICE_TOKEN>` header (with `X-Chitty-User-Id`) **or** a ChittyAuth-issued JWT delivered as the session cookie (`jwt:` prefix) — Bearer is service-to-service only; user/browser callers use the cookie path. Tenant scoping is resolved server-side from session/JWT claims (`c.var.tenantId`); path params are not trusted. |
There was a problem hiding this comment.
For protected resource routes, server/app.ts runs tenantMiddleware after hybridAuth, and tenantMiddleware returns 400 unless the request includes X-Tenant-ID or tenantId query; the JWT/session path only resolves userId, not c.var.tenantId. A ChittyBooks caller following this auth paragraph with only the service token + X-Chitty-User-Id or a session cookie will still fail every /api/accounts, /api/transactions, etc. request until it supplies the tenant selector the middleware actually requires.
Useful? React with 👍 / 👎.
| | `/api/properties` | Property list | | ||
| | `/api/accounts` | Account list + balances | | ||
| | `/api/transactions` | Transaction feed for caller's tenant. Today the mounted handler accepts only `?limit=`; `?accountId=` and `?since=` are accepted on `/api/transactions/export`. Any source/date filter parity is a follow-up, not a current guarantee. | | ||
| | `/api/allocations/rules`, `/api/allocations/preview`, `/api/allocations/execute`, `/api/allocations/run` | Allocation rule CRUD + dry-run + execution (per `server/accounting/allocations.ts`). There is no umbrella `/api/allocations` index route. | |
There was a problem hiding this comment.
Fresh evidence after the prior thread: server/accounting/allocations.ts and CHARTER.md both expose the run-list/update surface as /api/allocations/runs, but this contract still advertises singular /api/allocations/run. ChittyBooks clients using the verified-mounted table will call a 404 for allocation run history/status operations.
Useful? React with 👍 / 👎.
|
|
||
| | Source | Path | Tenant binding | | ||
| |---|---|---| | ||
| | Mercury read API | `https://api.mercury.com/api/v1` (direct, OAuth tokens scoped per tenant) | Token issued per `(tenant_id, legal_person_chittyid)` via ChittyConnect | |
There was a problem hiding this comment.
This multitenant contract still names direct api.mercury.com reads, while the repo-level agent boundary says every Mercury Bank API call routes through ChittyConnect and server/routes/mercury.ts implements account reads via ${CHITTYCONNECT_API_BASE}/mercury/accounts. If implementers use this table for the Mercury read path, they bypass the static-egress/credential boundary and reintroduce direct Mercury token handling despite the connector doc being corrected elsewhere.
Useful? React with 👍 / 👎.
| |---|---|---| | ||
| | Mercury read | Via ChittyConnect (canonical path) — ChittyFinance's own Mercury account reader (`server/routes/mercury.ts`) goes through ChittyConnect, and `chittyagent-connect` is defined as the Mercury Bank proxy for every bank API call. ChittyCommand reads should follow the same path; direct `api.mercury.com` calls are reserved for narrow service-internal uses behind the ChittyConnect credential boundary. | ChittyConnect → Mercury (preferred); fall back to `mercuryClient(token)` in `chittycommand/src/lib/integrations.ts:551` only when ChittyConnect is unavailable. | | ||
| | Mercury write | mercury-proxy on chittyserv-dev | `https://mercury-proxy.chitty.cc` (CF tunnel) — POST `/proxy` with `X-Mercury-Token` | | ||
| | Mercury webhooks | ChittyFinance | `POST https://finance.chitty.cc/api/webhooks/mercury` (PR #113, per-business HMAC secrets) | |
There was a problem hiding this comment.
Fresh evidence after the prior webhook-path thread: this follow-up plan still tells ChittyCommand to use the base /api/webhooks/mercury URL even though the native HMAC receiver is mounted at /api/webhooks/mercury/:tenantId and the base path is documented as the legacy ChittyConnect-normalized shim. Any implementation registering the base URL from this table will miss the per-tenant HMAC path and route native Mercury payloads to the wrong handler.
Useful? React with 👍 / 👎.
|
|
||
| Add a single sentence to `CLAUDE.md` under "Where Things Live" → routes section: | ||
|
|
||
| > Resource routes are mounted at `/api/<resource>` (no `/v1` prefix). Only operational/meta routes (`status`, `metrics`, `documentation`) live under `/api/v1/`. The OpenAPI spec at `/api/v1/documentation` is authoritative. |
There was a problem hiding this comment.
Fresh evidence beyond the earlier CLAUDE.md finding: this proposal still instructs maintainers to add a sentence saying /api/v1/documentation is authoritative, but server/routes/docs.ts only lists a small subset of the mounted routes and the updated CLAUDE.md now correctly says to treat server/app.ts as the source of truth. Anyone following this proposal later can reintroduce the same bad guidance and cause consumers to omit supported APIs such as reports, imports, allocations, and classification.
Useful? React with 👍 / 👎.
|
|
||
| | Repo | Description | Deploy | Role | | ||
| |---|---|---|---| | ||
| | `CHITTYFOUNDATION/chittyledger` | Worker at `ledger.chitty.cc`. Has CHARTER/CHITTY/CLAUDE pentad. Endpoints: `POST /entries`, `GET /custody/:entityId`, `GET /verify`. | ✅ `200 ok` at `ledger.chitty.cc/health` | **Canonical substrate.** | |
There was a problem hiding this comment.
Fresh evidence after the prior connector-path fix: this endpoint inventory still advertises substrate writes as POST /entries, while server/lib/ledger-client.ts posts to ${base}/api/entries and the same docs set now directs Finance/Command writes to /api/entries. If the naming-plan follow-up copies this inventory into the canonical ledger README or register metadata, new projection writers can target the path that Finance no longer exercises.
Useful? React with 👍 / 👎.
| | Mercury write | mercury-proxy on chittyserv-dev | `https://mercury-proxy.chitty.cc` (CF tunnel) — POST `/proxy` with `X-Mercury-Token` | | ||
| | Mercury webhooks | ChittyFinance | `POST https://finance.chitty.cc/api/webhooks/mercury` (PR #113, per-business HMAC secrets) | | ||
| | Bookkeeping reads | ChittyFinance | `GET https://finance.chitty.cc/api/transactions` and `/api/reports/*` | | ||
| | Bookkeeping writes | ChittyFinance | `POST https://finance.chitty.cc/api/transactions`, `/api/allocations`, `/api/classification` — **not** a separate `chittybooks` write API | |
There was a problem hiding this comment.
This connector revision tells ChittyCommand to write to base /api/allocations and /api/classification, but the mounted write handlers are only on concrete subpaths such as /api/allocations/rules, /api/allocations/execute, and /api/classification/classify; there is no base POST handler for either prefix. A follow-up implementation using these URLs for bookkeeping mutations will get 404s instead of creating allocations or classifications.
Useful? React with 👍 / 👎.
|
|
||
| - [ ] **BLOCKER**: `legal_person_chittyid` column or metadata interim must exist before reconciliation reports reference it. Until then, the column is contract-only. | ||
| - [ ] `legal_person_chittyid` column present on `accounts` (verify in `database/system.schema.ts` before reconciliation reports go live). | ||
| - [ ] Per-business Mercury webhook secret in place (already shipped — PR #113). |
There was a problem hiding this comment.
The native webhook handler skips HMAC verification entirely when webhook:mercury:secret:<tenantId> is missing, but this deploy gate marks the per-business secret as already shipped instead of making secret presence a blocker. In any tenant where the KV secret was not actually stored, operators following this checklist can expose a native Mercury endpoint that accepts unsigned payloads, so the gate should require verifying the secret exists before enabling the webhook.
Useful? React with 👍 / 👎.
Summary
Docs-only branch from /goal audit of finance/books/ledger surfaces. No code or runtime changes.
Five new docs:
docs/contracts/chittybooks-chittyfinance.md— engine/UI boundary, source-of-truth table, API surface, deploy decision (retires the fakebooks.chitty.cclive-API assumption only; actual deploy choice stays an explicit operator gate)docs/contracts/mercury-multitenant.md— tenant_id + legal_person_chittyid + Wave mapping rules. Flags critical schema gap:legal_person_chittyidis not onaccountsindatabase/system.schema.tsdocs/proposals/api-v1-prefix-fix.md— resolves the/api/v1/*404 as a non-issue (meta routes are intentionally under/api/v1, resource routes under/api/<resource>). One-sentenceCLAUDE.mdclarification only.docs/proposals/ch1tty-connector-revision.md— names the deadCHITTYBOOKS_URLendpoint inchittycommand/docs/plans/2026-02-23-mercury-chittybooks-plan.md:168andchittycommand/src/lib/integrations.ts:618; routes bookkeeping reads/writes through realfinance.chitty.ccpathsdocs/proposals/chittyledger-naming-plan.md— substrate (FOUNDATION/chittyledger) + Finance/Evidence projections; plan to renameCHITTYOS/chittyledger→-evidence-legacyValidation
financeClientverified to call/api/accountsnot/api/v1/accounts→ docs-only path-prefix clarification has zero consumer breakage./api/v1/documentationenumerated: 9 paths, only/healthand/api/v1/{status,metrics,documentation}under/api/v1; resource paths at/api/*.SystemStorageenforces tenancy viatenant_id NOT NULLschema column + 117tenantIdreferences.Followups (not in this PR)
CHITTYOS/chittycommand): lazy-disablebooksClient— PR-ready, awaiting owner approvalCHITTYOS/chittyledgerrename: org-owner action🤖 Generated with Claude Code
Summary by CodeRabbit