Fix PR review: 24 issues across 4 severity levels#269
Merged
Connorbelez merged 8 commits intomainfrom Mar 24, 2026
Merged
Conversation
CRITICAL fixes: - C1: templates.ts publish RBAC — was docGenMutation (document:generate permission), changed to adminMutation. Template publishing is a design operation, not document generation. - C2: reconciliationAction.ts dailyReconciliation — added try-catch around each layer (StatusCheck, BalanceCheck) so failures don't silently kill the cron. Each layer now logs [FATAL] on error and continues. - C3: lookupStatus — was handling only 4/14 entity types, silently skipping the rest. Added all remaining types (deal, broker, borrower, lender, etc.) and a console.error for any future unhandled types. - C4: postCashReceiptForObligation missing receivable — was console.warn + null return. Upgraded to console.error + auditLog entry. IMPORTANT fixes: - I1: Phase 1 comment on unbounded .collect() in replayJournalIntegrity - I2: Clarified incremental mode (accumulates only, no verification) - I3: getPostingGroupSummary null-check for missing accounts — was silently treating missing accounts as non-CONTROL via optional chaining - I4: Same fix in getControlBalancesByPostingGroup - I5: advanceReplayCursor regression guard (reject if sequence <= existing cursor) - I8: Renamed Layer 1/2 to StatusCheck/BalanceCheck to avoid CLAUDE.md terminology conflict SUGGESTIONS fixed: - S1: SerializedReplayMismatch exported from replayIntegrity.ts - S2: Runtime guard in filterByScope throws when both accountId and mortgageId are set - S3: ServicingFeeMetadata — removed [key:string]:unknown, replaced with extends Record for assignability to Record<string,unknown> - S4: isReplayPassing pure predicate exported - S6: JSDoc on accumulateEntries and compareAgainstStored - S7: Removed stale ENG-156 TODO (SUSPENSE routing already implemented) - S8: Clarified replayJournalIntegrity "read-only" claim Also: - cashReceipt.test.ts — registered auditLog component in the "returns null" test that now calls auditLog.log - Removed unused docGenMutation and authedMutation imports from templates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Docstring: filterByScope now correctly says it throws when both
accountId and mortgageId are set (not "accountId takes precedence")
- detectMissingSequences: initialize expectedNext from 0n instead of
first entry's sequence to catch gaps before the first entry
- replayJournalIntegrity query: added .order("asc") to enforce ascending
sequence order
- advanceReplayCursor: changed <= to < for regression guard (equality
case is allowed for idempotency)
- Incremental mode: set passed=false since no verification is performed
- Scoped full mode: skip compareAgainstStored to avoid false drift from
partial history vs lifetime totals
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d replay passed flag
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Connorbelez has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Reviewer's GuideImplements and documents critical fixes for replay integrity, daily reconciliation, and security linting, including stricter cursor regression guards, more precise replay semantics and logging, audit logging for missing receivables, and small test and tooling adjustments. Sequence diagram for dailyReconciliation with StatusCheck and BalanceCheck layerssequenceDiagram
participant Cron
participant dailyReconciliation
participant StatusCheck
participant BalanceCheck
participant Logger
Cron->>dailyReconciliation: trigger()
dailyReconciliation->>StatusCheck: runStatusCheck()
alt StatusCheck succeeds
StatusCheck-->>dailyReconciliation: statusResult
else StatusCheck throws error
dailyReconciliation->>Logger: logFatal(StatusCheck error)
end
dailyReconciliation->>BalanceCheck: runBalanceCheck()
alt BalanceCheck succeeds
BalanceCheck-->>dailyReconciliation: balanceResult
else BalanceCheck throws error
dailyReconciliation->>Logger: logFatal(BalanceCheck error)
end
dailyReconciliation-->>Cron: complete without stopping cron
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
advanceReplayCursor, silently logging and returning when a regression is detected gives callers no signal that their update was ignored; consider returning a status/boolean or throwing a typed error so upstream code can react explicitly. - The change in
scripts/lint-security.tsto computeROOTusing(import.meta as unknown as { dir: string }).dirassumes a nonstandardimport.meta.dirshape; it would be safer either to stick withnew URL(import.meta.url)or centralize this pattern so the runtime assumptions are clear and consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `advanceReplayCursor`, silently logging and returning when a regression is detected gives callers no signal that their update was ignored; consider returning a status/boolean or throwing a typed error so upstream code can react explicitly.
- The change in `scripts/lint-security.ts` to compute `ROOT` using `(import.meta as unknown as { dir: string }).dir` assumes a nonstandard `import.meta.dir` shape; it would be safer either to stick with `new URL(import.meta.url)` or centralize this pattern so the runtime assumptions are clear and consistent.
## Individual Comments
### Comment 1
<location path="scripts/lint-security.ts" line_range="31" />
<code_context>
// ── Configuration ───────────────────────────────────────────────────────
-const ROOT = resolve(new URL("..", import.meta.url).pathname);
+const ROOT = resolve((import.meta as unknown as { dir: string }).dir, "..");
const CONVEX_DIR = join(ROOT, "convex");
const ROUTES_DIR = join(ROOT, "src", "routes");
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting `import.meta` to a `{ dir: string }` shape is brittle and may break in environments without `import.meta.dir`.
The previous code used `new URL("..", import.meta.url).pathname`, which is standard in Node ESM. The new version depends on a non-standard `import.meta.dir` plus a type cast, so it will break in environments that only provide `import.meta.url`. Instead, consider deriving the directory from `import.meta.url` (e.g., `dirname(fileURLToPath(import.meta.url))`) to avoid the unsafe cast and the non-standard property.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // ── Configuration ─────────────────────────────────────────────────────── | ||
|
|
||
| const ROOT = resolve(new URL("..", import.meta.url).pathname); | ||
| const ROOT = resolve((import.meta as unknown as { dir: string }).dir, ".."); |
There was a problem hiding this comment.
issue (bug_risk): Casting import.meta to a { dir: string } shape is brittle and may break in environments without import.meta.dir.
The previous code used new URL("..", import.meta.url).pathname, which is standard in Node ESM. The new version depends on a non-standard import.meta.dir plus a type cast, so it will break in environments that only provide import.meta.url. Instead, consider deriving the directory from import.meta.url (e.g., dirname(fileURLToPath(import.meta.url))) to avoid the unsafe cast and the non-standard property.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
convex/payments/cashLedger/__tests__/postingGroups.test.ts (1)
211-244: Add the missing corruption-path assertion forisPostingGroupComplete.You now set
hasCorruptEntries: falsein all cases, but thetruebranch is still untested. Add one case asserting it returnsfalseeven when balance is zero and entries > 0.✅ Suggested test addition
describe("isPostingGroupComplete", () => { + it("false when hasCorruptEntries is true, even if other conditions pass", () => { + const result = isPostingGroupComplete({ + postingGroupId: "test:123", + controlAllocationBalance: 0n, + totalJournalEntryCount: 3, + hasCorruptEntries: true, + entries: [], + }); + expect(result).toBe(false); + }); + it("true when net-zero and entries > 0", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/__tests__/postingGroups.test.ts` around lines 211 - 244, Add a test for the corruption path of isPostingGroupComplete: create a case where postingGroupId is any string, controlAllocationBalance is 0n, totalJournalEntryCount > 0 (e.g., 3), and hasCorruptEntries is true, and assert the function returns false; this complements the existing "true when net-zero and entries > 0" test and uses the same input shape (entries: []) to verify hasCorruptEntries overrides the net-zero/entries condition.convex/payments/cashLedger/__tests__/cashReceipt.test.ts (1)
247-267: Reasonable workaround for missing convex-test types.The type assertion approach is pragmatic for handling the runtime/type mismatch. A few optional improvements:
- Consider adding a TODO with a tracking reference so this workaround can be removed when
convex-testships proper types.- If other tests need
runInComponent, extract this interface totestUtils.tsto avoid duplication.- The assertions on
entries[0].action,entries[0].severity, etc. (lines 275-282) access properties onRecord<string, unknown>without type narrowing—this works at runtime but would fail silently if the audit log schema changes. Consider adding a lightweight type guard or inline cast for better test resilience.💡 Optional: Add TODO and minor type safety
// convex-test@0.0.41 has runInComponent at runtime but its types omit it + // TODO(ENG-XXX): Remove this workaround when convex-test exports runInComponent types interface ComponentCtx {For the audit log assertions, you could add a simple type guard:
interface AuditLogEntry { action: string; resourceType: string; resourceId: string; actorId: string; severity: string; metadata: unknown; } function isAuditLogEntry(e: Record<string, unknown>): e is AuditLogEntry { return typeof e.action === "string" && typeof e.severity === "string"; } // Then in test: expect(entries).toHaveLength(1); expect(isAuditLogEntry(entries[0])).toBe(true); if (isAuditLogEntry(entries[0])) { expect(entries[0].action).toBe("cashLedger.receivable_not_found"); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/__tests__/cashReceipt.test.ts` around lines 247 - 267, Add a short TODO comment with a tracking reference above the ComponentCtx type assertion so it's clear this is a temporary workaround until convex-test exports proper types; if other tests use runInComponent extract the ComponentCtx interface and the augmented TestHarness type into a shared testUtils.ts to avoid duplication; for the assertions that access entries[0].action / severity etc. add a lightweight type guard or an inline cast (referencing the AuditLogEntry guard name or a cast to AuditLogEntry) before asserting to narrow Record<string, unknown> to a known shape and make the test safer while keeping the existing runtime behavior in runInComponent and TestHarness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/engine/reconciliationAction.ts`:
- Around line 117-119: Replace the incorrect Biome directive comment above the
data-access function in reconciliationAction.ts (the comment that explains the
function handles 14 distinct entity table types and multiple db.get calls) with
a properly formatted Biome ignore for the complexity rule: use the lint-style
directive for the complexity rule
(lint/complexity/noExcessiveCognitiveComplexity) followed by a short reason
stating that each entity type requires a separate db.get call and the high
cognitive complexity is unavoidable.
---
Nitpick comments:
In `@convex/payments/cashLedger/__tests__/cashReceipt.test.ts`:
- Around line 247-267: Add a short TODO comment with a tracking reference above
the ComponentCtx type assertion so it's clear this is a temporary workaround
until convex-test exports proper types; if other tests use runInComponent
extract the ComponentCtx interface and the augmented TestHarness type into a
shared testUtils.ts to avoid duplication; for the assertions that access
entries[0].action / severity etc. add a lightweight type guard or an inline cast
(referencing the AuditLogEntry guard name or a cast to AuditLogEntry) before
asserting to narrow Record<string, unknown> to a known shape and make the test
safer while keeping the existing runtime behavior in runInComponent and
TestHarness.
In `@convex/payments/cashLedger/__tests__/postingGroups.test.ts`:
- Around line 211-244: Add a test for the corruption path of
isPostingGroupComplete: create a case where postingGroupId is any string,
controlAllocationBalance is 0n, totalJournalEntryCount > 0 (e.g., 3), and
hasCorruptEntries is true, and assert the function returns false; this
complements the existing "true when net-zero and entries > 0" test and uses the
same input shape (entries: []) to verify hasCorruptEntries overrides the
net-zero/entries condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2b332e4-b137-4e7c-878b-3c9107d5672b
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (7)
convex/engine/effects/__tests__/obligationAccrual.integration.test.tsconvex/engine/reconciliationAction.tsconvex/payments/cashLedger/__tests__/cashReceipt.test.tsconvex/payments/cashLedger/__tests__/postingGroups.test.tsconvex/payments/cashLedger/replayIntegrity.tsscripts/lint-security.tsspecs/ENG-171/chunks/chunk-02-integration/context.md
Comment on lines
+117
to
+119
| // biome-ignore: cognitive complexity is 28 because each entity type requires | ||
| // a separate db.get call — this is unavoidable for a data-access function | ||
| // that must handle 14 distinct entity table types. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check biome-ignore usage patterns in the codebase to verify correct format
# Find other biome-ignore directives to confirm the pattern
rg -n "biome-ignore" --type=ts -C1 | head -30
# Locate Biome configuration to verify linter setup
fd "biome.json" -e json -e jsoncRepository: Connorbelez/tanstackTemplate
Length of output: 2158
🏁 Script executed:
rg -n "cognitive" --type=ts convex/engine/reconciliationAction.ts -A2 -B2Repository: Connorbelez/tanstackTemplate
Length of output: 391
🌐 Web query:
Biome cognitive complexity rule name noExcessiveCognitiveComplexity lint/complexity
💡 Result:
The Biome cognitive complexity rule is named "noExcessiveCognitiveComplexity". It is categorized under lint/complexity and disallows functions exceeding a configurable cognitive complexity score (default: 15), based on the SonarQube Cognitive Complexity algorithm. This is confirmed in official Biome documentation and source code.
Citations:
- 1: https://biomejs.dev/linter/rules/no-excessive-cognitive-complexity/
- 2: https://github.com/biomejs/biome/blob/main/crates/biome_js_analyze/src/lint/complexity/no_excessive_cognitive_complexity.rs
- 3: https://biomejs.dev/linter/javascript/rules/
Fix the biome-ignore directive format to suppress the cognitive complexity warning.
The comment uses an incorrect Biome directive syntax. Change:
// biome-ignore: cognitive complexity is 28 because each entity type requires
// a separate db.get call — this is unavoidable for a data-access function
// that must handle 14 distinct entity table types.
To:
// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: each entity type requires a separate db.get call — this is unavoidable for a data-access function that must handle 14 distinct entity table types.
The current format does not match Biome's expected syntax (// biome-ignore lint/<category>/<rule>: reason) and the directive will not suppress the warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/engine/reconciliationAction.ts` around lines 117 - 119, Replace the
incorrect Biome directive comment above the data-access function in
reconciliationAction.ts (the comment that explains the function handles 14
distinct entity table types and multiple db.get calls) with a properly formatted
Biome ignore for the complexity rule: use the lint-style directive for the
complexity rule (lint/complexity/noExcessiveCognitiveComplexity) followed by a
short reason stating that each entity type requires a separate db.get call and
the high cognitive complexity is unavoidable.
There was a problem hiding this comment.
Pull request overview
This PR completes/follows up ENG-170/171 reconciliation + cash-ledger replay integrity work, tightening correctness and operational behavior for daily reconciliation (cron) and replay integrity checks.
Changes:
- Updates cash-ledger replay integrity behavior (query bounds,
passedsemantics) and publishesreplayIntegrityin generated Convex API types. - Hardens
dailyReconciliationwith layered try/catch handling and expands entity-type coverage in status lookup. - Fixes/adjusts supporting tooling and tests (security lint root resolution, harness module glob reuse, auditLog component test access, posting group summary shape).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-171/chunks/chunk-02-integration/context.md | Updates integration spec with cursor regression guard + Layer 2 reconciliation details |
| scripts/lint-security.ts | Changes project-root resolution logic for the security lint script |
| convex/payments/cashLedger/replayIntegrity.ts | Adjusts replay query bounds and passed computation; includes cursor regression guard |
| convex/payments/cashLedger/tests/postingGroups.test.ts | Updates test inputs to include newly-required summary fields |
| convex/payments/cashLedger/tests/cashReceipt.test.ts | Adds typed workaround for runInComponent type gap to validate auditLog writes |
| convex/engine/reconciliationAction.ts | Adds Biome suppression comment + layered reconciliation logic (StatusCheck/BalanceCheck) |
| convex/engine/effects/tests/obligationAccrual.integration.test.ts | Reuses import.meta.glob modules value per test file |
| convex/_generated/api.d.ts | Regenerates API typings to include payments/cashLedger/replayIntegrity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+117
to
+119
| // biome-ignore: cognitive complexity is 28 because each entity type requires | ||
| // a separate db.get call — this is unavoidable for a data-access function | ||
| // that must handle 14 distinct entity table types. |
There was a problem hiding this comment.
The // biome-ignore suppression here doesn’t follow the repo’s usual Biome convention (targeting a specific rule like // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: ...). As written it suppresses all lint diagnostics for the next line, which is broader than needed and can mask unrelated issues. Please scope this ignore to lint/complexity/noExcessiveCognitiveComplexity with a short reason.
Suggested change
| // biome-ignore: cognitive complexity is 28 because each entity type requires | |
| // a separate db.get call — this is unavoidable for a data-access function | |
| // that must handle 14 distinct entity table types. | |
| // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: | |
| // cognitive complexity is 28 because each entity type requires a separate | |
| // db.get call — this is unavoidable for a data-access function that must | |
| // handle 14 distinct entity table types. |
Comment on lines
+290
to
+295
| // 1. Determine starting sequence and query lower bound. | ||
| // Full replay: inclusive (gte 0n) so sequence 0 is never dropped. | ||
| // Incremental replay: exclusive (gt cursor) to avoid re-processing verified entries. | ||
| const fromSequence = | ||
| scope.mode === "incremental" ? ((await getReplayCursor(ctx)) ?? 0n) : 0n; | ||
| const queryGt = scope.mode === "incremental"; |
There was a problem hiding this comment.
The full-replay path is now documented/implemented as if sequence numbers start at 0n (e.g. the comment about not dropping “sequence 0”). However, getNextCashSequenceNumber() returns currentValue + 1n with currentValue initialized to 0n, so production sequences start at 1n. With the current missing-sequence logic (starting from 0n), a non-empty journal will always report "0" as missing and make passed permanently false in full mode. Please align the replay lower bound and gap detection with the actual sequence numbering (e.g. treat 1n as the first expected sequence, and/or only consider 0 missing if the system truly emits a sequence 0 entry).
Merged
Owner
Author
Merge activity
|
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
# THIS PR CONTAINS MISSING COMMITS FROM eng-170
Fix PR review: 24 issues across 4 severity levels
CRITICAL fixes:
- C1: templates.ts publish RBAC — was docGenMutation (document:generate
permission), changed to adminMutation. Template publishing is a design
operation, not document generation.
- C2: reconciliationAction.ts dailyReconciliation — added try-catch
around each layer (StatusCheck, BalanceCheck) so failures don't silently
kill the cron. Each layer now logs [FATAL] on error and continues.
- C3: lookupStatus — was handling only 4/14 entity types, silently
skipping the rest. Added all remaining types (deal, broker, borrower,
lender, etc.) and a console.error for any future unhandled types.
- C4: postCashReceiptForObligation missing receivable — was console.warn
+ null return. Upgraded to console.error + auditLog entry.
IMPORTANT fixes:
- I1: Phase 1 comment on unbounded .collect() in replayJournalIntegrity
- I2: Clarified incremental mode (accumulates only, no verification)
- I3: getPostingGroupSummary null-check for missing accounts — was
silently treating missing accounts as non-CONTROL via optional chaining
- I4: Same fix in getControlBalancesByPostingGroup
- I5: advanceReplayCursor regression guard (reject if sequence <=
existing cursor)
- I8: Renamed Layer 1/2 to StatusCheck/BalanceCheck to avoid
CLAUDE.md terminology conflict
SUGGESTIONS fixed:
- S1: SerializedReplayMismatch exported from replayIntegrity.ts
- S2: Runtime guard in filterByScope throws when both accountId
and mortgageId are set
- S3: ServicingFeeMetadata — removed [key:string]:unknown, replaced
with extends Record for assignability to Record<string,unknown>
- S4: isReplayPassing pure predicate exported
- S6: JSDoc on accumulateEntries and compareAgainstStored
- S7: Removed stale ENG-156 TODO (SUSPENSE routing already implemented)
- S8: Clarified replayJournalIntegrity "read-only" claim
Also:
- cashReceipt.test.ts — registered auditLog component in the "returns
null" test that now calls auditLog.log
- Removed unused docGenMutation and authedMutation imports from templates
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix PR review: address 7 review comments on replayIntegrity
- Docstring: filterByScope now correctly says it throws when both
accountId and mortgageId are set (not "accountId takes precedence")
- detectMissingSequences: initialize expectedNext from 0n instead of
first entry's sequence to catch gaps before the first entry
- replayJournalIntegrity query: added .order("asc") to enforce ascending
sequence order
- advanceReplayCursor: changed <= to < for regression guard (equality
case is allowed for idempotency)
- Incremental mode: set passed=false since no verification is performed
- Scoped full mode: skip compareAgainstStored to avoid false drift from
partial history vs lifetime totals
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix: clarify misleading bigint conversion comment in spec
fix: add cursor regression guard to advanceReplayCursor in spec
fix: address review comments — ordering, sequence 0 bounds, and scoped replay passed flag
fix: add critical implementation details to dailyReconciliation spec
fixing merge conflicts
## Summary by Sourcery
Address review feedback and robustness gaps in cash ledger replay integrity, daily reconciliation, and related tests and tooling.
Bug Fixes:
- Prevent replay cursor regression by rejecting attempts to move the cursor backwards and logging the event.
- Ensure full replay integrity checks include sequence 0 while incremental mode skips already verified entries, and only report passed when verification actually runs and finds no issues.
- Fix daily reconciliation spec to describe guarded cursor advancement, per-layer error handling, and structured discrepancy logging for journal replay failures.
- Clarify and correct bigint serialization behavior in replay integrity query results to avoid misleading comments and conversions.
- Update posting group completeness tests to reflect the hasCorruptEntries flag so corrupt-entry scenarios are correctly handled.
- Adjust lookupStatus to document the unavoidable cognitive complexity of handling all supported entity types without suppressing behavior.
Enhancements:
- Document and codify detailed BalanceCheck implementation and error-handling semantics in the reconciliation spec, including audit logging and P0 diagnostics.
- Refine replay integrity behavior to distinguish verification from accumulation-only modes and to avoid false drift when running in scoped modes.
- Stabilize test harness module discovery by sharing a single import.meta.glob result across obligation accrual integration tests.
- Improve type-safety in cash receipt tests by casting the test harness to expose runInComponent with a minimal context interface.
- Modernize the security lint script’s project root resolution to use import.meta.dir for more reliable path handling across environments.
Build:
- Update the lint-security script’s root resolution logic to be compatible with ESM import.meta usage.
Tests:
- Update cash receipt audit-log test to use a typed runInComponent helper compatible with current convex-test behavior.
- Refactor obligation accrual integration tests to reuse a shared modules glob when creating harnesses, reducing duplication and improving reliability.
- Extend posting group tests to cover the new hasCorruptEntries field in completion checks.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Added cursor regression detection to prevent invalid replay cursor advancement.
* Enhanced replay integrity verification with improved boundary handling across replay modes.
* Improved error detection and reporting for reconciliation discrepancies.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
THIS PR CONTAINS MISSING COMMITS FROM eng-170
Fix PR review: 24 issues across 4 severity levels
CRITICAL fixes:
permission), changed to adminMutation. Template publishing is a design
operation, not document generation.
around each layer (StatusCheck, BalanceCheck) so failures don't silently
kill the cron. Each layer now logs [FATAL] on error and continues.
skipping the rest. Added all remaining types (deal, broker, borrower,
lender, etc.) and a console.error for any future unhandled types.
IMPORTANT fixes:
silently treating missing accounts as non-CONTROL via optional chaining
existing cursor)
CLAUDE.md terminology conflict
SUGGESTIONS fixed:
and mortgageId are set
with extends Record for assignability to Record<string,unknown>
Also:
null" test that now calls auditLog.log
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
fix PR review: address 7 review comments on replayIntegrity
accountId and mortgageId are set (not "accountId takes precedence")
first entry's sequence to catch gaps before the first entry
sequence order
case is allowed for idempotency)
partial history vs lifetime totals
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
fix: clarify misleading bigint conversion comment in spec
fix: add cursor regression guard to advanceReplayCursor in spec
fix: address review comments — ordering, sequence 0 bounds, and scoped replay passed flag
fix: add critical implementation details to dailyReconciliation spec
fixing merge conflicts
Summary by Sourcery
Address review feedback and robustness gaps in cash ledger replay integrity, daily reconciliation, and related tests and tooling.
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit