test: fix harness failures and update manifest#393
Merged
Connorbelez merged 19 commits intomainfrom Apr 14, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 16 seconds. ⌛ 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 (19)
📝 WalkthroughWalkthroughThis PR introduces authorization resource checks for payment entities, implements a manual inbound collection flow for obligations via collection plans, refactors payment settlement pipelines to support transfer-based cash receipts, simplifies admin permissions to a wildcard "admin:access" model, and establishes e2e testing infrastructure for session validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Reviewer's GuideStabilizes Vitest and Playwright suites by updating test harnesses, fixtures, and expectations to match current auth, audit-log, document engine, and payments behavior, while introducing shared helpers and a test-failure manifest documenting prior instability. Sequence diagram for onboarding auth storage state creation and role review flowsequenceDiagram
actor Member
actor Admin
participant Playwright as PlaywrightTests
participant AuthStorage as AuthStorageHelper
participant Browser
participant AuthKit as AuthKitHostedLogin
participant App as AppBackend
Member->>Playwright: run onboarding.spec member tests
Playwright->>AuthStorage: createAuthStorageState(orgId = TEST_MEMBER_ORG_ID)
AuthStorage->>Browser: browser.newContext()
Browser-->>AuthStorage: Context
AuthStorage->>AuthKit: loginViaWorkOS(testAccountEmail)
AuthKit-->>AuthStorage: authenticated session
AuthStorage->>App: GET /e2e/switch-org?orgId=member
App-->>AuthStorage: redirect to /
AuthStorage->>App: GET /demo/workos
AuthStorage->>App: open Organizations tab
App-->>AuthStorage: session card with orgId and role member
AuthStorage->>Browser: context.storageState(path)
AuthStorage-->>Playwright: storageState path
Playwright->>Browser: browser.newContext({ storageState })
Browser-->>Playwright: MemberContext
Playwright->>MemberContext: open /demo/rbac-auth/onboarding
MemberContext-->>Playwright: onboarding UI loaded
Member->>MemberContext: submit role request
MemberContext->>App: POST /onboarding/request
App-->>MemberContext: request pending
Admin->>Playwright: run admin onboarding tests
Playwright->>AuthStorage: createAuthStorageState(orgId = TEST_ADMIN_ORG_ID)
AuthStorage->>AuthKit: loginViaWorkOS(testAccountEmail)
AuthStorage->>App: switch org to admin, verify orgId and role admin
AuthStorage-->>Playwright: admin storageState path
Playwright->>Browser: browser.newContext({ storageState })
Browser-->>Playwright: AdminContext
Playwright->>AdminContext: open /demo/rbac-auth/onboarding
AdminContext->>App: GET pending requests
App-->>AdminContext: pending request for testAccountEmail
Admin->>AdminContext: click Approve or Reject
AdminContext->>App: POST /onboarding/review
App-->>AdminContext: success or error
AdminContext-->>Playwright: UI update (card removed or error banner)
Playwright-->>Admin: expect() assertions pass
Sequence diagram for document engine admin test helper and storage state reusesequenceDiagram
actor QAEngineer
participant Playwright as DocumentEngineTests
participant DocHelper as DocumentEngineHelper
participant AuthStorage as AuthStorageHelper
participant Browser
participant App as DocumentEngineApp
QAEngineer->>Playwright: run document-engine templates.spec
Note over Playwright,Browser: beforeAll setup via openAdminPage
Playwright->>DocHelper: openAdminPage(browser)
DocHelper->>Browser: browser.newContext()
Browser-->>DocHelper: Context
DocHelper->>Browser: Context.newPage()
Browser-->>DocHelper: Page
DocHelper->>AuthStorage: createAuthStorageState(orgId = TEST_ADMIN_ORG_ID, page, path = ADMIN_STORAGE_STATE)
AuthStorage->>App: AuthKit login + switch org to admin
AuthStorage->>Browser: context.storageState(path)
DocHelper-->>Playwright: { context, page }
Playwright->>DocHelper: uploadTestPdf(page, pdfName)
DocHelper->>App: POST /demo/document-engine/library upload
App-->>DocHelper: PDF stored
DocHelper->>Browser: context.close()
Note over Playwright,Browser: per-test contexts reuse ADMIN_STORAGE_STATE
loop each test case
Playwright->>Browser: browser.newContext({ storageState: ADMIN_STORAGE_STATE })
Browser-->>Playwright: AuthenticatedContext
Playwright->>AuthenticatedContext: newPage()
AuthenticatedContext-->>Playwright: Page
Playwright->>Page: goto /demo/document-engine/... (templates, groups, variables, generate, library, navigation, designer)
Page->>App: authorized GET/POST requests
App-->>Page: protected admin UIs
Playwright-->>QAEngineer: expect() assertions pass
Playwright->>AuthenticatedContext: context.close()
end
Note over Playwright,Browser: afterAll cleanup via openAdminPage
Playwright->>DocHelper: openAdminPage(browser)
DocHelper->>Browser: browser.newContext()
Browser-->>DocHelper: Context
DocHelper->>Browser: Context.newPage()
Browser-->>DocHelper: Page
Playwright->>Page: goto /demo/document-engine/library
Playwright->>Page: delete test PDF if present
Page->>App: DELETE library asset
App-->>Page: asset deleted
Playwright->>Browser: context.close()
Sequence diagram for simulation settlement outcome handling and retry logicsequenceDiagram
actor Tester
participant Playwright as SimulationTest
participant Helper as SettlementHelper
participant Browser
participant App as SimulationApp
Tester->>Playwright: run Marketplace Simulation — Full Flow
Playwright->>Browser: open obligation row
Playwright->>Browser: fill Settled amount (¢) with remainingPaymentAmount
Playwright->>Browser: click Apply Payment
Playwright->>Helper: waitForSettlementOutcome(page)
alt success message first
Helper->>Browser: waitFor "Payment applied. Dispersal scheduled."
Browser-->>Helper: visible
Helper-->>Playwright: outcome = success
Playwright->>Browser: expect success message visible
else balance error first
Helper->>Browser: waitFor text /exceeds remaining balance/
Browser-->>Helper: visible
Helper-->>Playwright: outcome = error
Playwright->>Browser: expect error message visible
Playwright->>Browser: read obligationAmountCell innerText()
Browser-->>Playwright: formatted amount
Playwright->>Playwright: retryAmount = currencyTextToCents(amount)
Playwright->>Browser: click Pay on obligationRow
Playwright->>Browser: waitFor Settled amount (¢) input
Playwright->>Browser: fill retryAmount
Playwright->>Browser: click Apply Payment
Playwright->>Browser: wait for success message visible
end
Playwright->>Browser: expect obligationRow toHaveCount(0)
Browser-->>Playwright: assertion passes
Playwright-->>Tester: settlement flow verified without flakes
Class diagram for updated Playwright test helper modules and usagesclassDiagram
class AuthStorageHelper {
+string TEST_ADMIN_ORG_ID
+string TEST_MEMBER_ORG_ID
+createAuthStorageState(orgId string, page Page, path string) Promise<void>
+requireEnv(name string) string
}
class DocumentEngineHelper {
+string BASE_URL
+string ADMIN_STORAGE_STATE
+uniqueKey(prefix string) string
+openAdminPage(browser Browser) Promise<OpenAdminPageResult>
+uploadTestPdf(page Page, pdfName string) Promise<void>
+uniqueName(prefix string) string
}
class RBACHelpers {
+expectResolvedRole(page Page, role string) Promise<void>
}
class LoginSpec {
-string testEmail
+getProfileEmailCard(page Page) Locator
}
class OnboardingSpec {
-string onboardingAuthStateDir
-string testAccountEmail
+createFreshStorageState(browser Browser, orgId string) Promise<string>
+openOnboardingPage(browser Browser, orgId string) Promise<OnboardingSession>
+clearPendingRequests(browser Browser) Promise<void>
}
class SimulationHelper {
+currencyTextToCents(value string) number
+waitForSettlementOutcome(page Page) Promise~string~
}
class GovernedTransitionsHelpers {
+goToTab(page Page, path string, heading string) Promise<void>
+resetDemo(page Page) Promise<void>
+createApplication(page Page, opts CreateApplicationOpts) Promise<void>
+getEntityCard(page Page, label string) Locator
+getStatCard(page Page, label string) Locator
}
AuthStorageHelper <.. OnboardingSpec : uses
AuthStorageHelper <.. DocumentEngineHelper : uses
DocumentEngineHelper <.. DocumentEngineSpecs : applied_via_test.use
class DocumentEngineSpecs {
+beforeAll(browser Browser) void
+tests_use_storageState_ADMIN_STORAGE_STATE
}
RBACHelpers <.. RBACAdminSpec : expectResolvedRole
RBACHelpers <.. RBACMemberSpec : expectResolvedRole
class RBACAdminSpec {
+admin session shows admin role
}
class RBACMemberSpec {
+member session shows member role
}
LoginSpec <.. AuthTests : verifies profile email and sign out
OnboardingSpec <.. AuthTests : onboarding role request scenarios
SimulationHelper <.. SimulationSpec : settlement flow assertions
GovernedTransitionsHelpers <.. GovernedTransitionsSpec : layout and UC tests
class AuthTests {
+authenticated user sees profile on workos demo page
+sign out clears session
+protected routes redirect to AuthKit hosted page
}
class SimulationSpec {
+Marketplace Simulation — Full Flow
}
class GovernedTransitionsSpec {
+Layout and navigation tests
+Use cases UC_1_to_Reactive
}
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.
Pull request overview
This PR focuses on stabilizing failing Vitest + Playwright suites by updating fixtures/expectations to match current domain behavior, improving e2e selector robustness, and centralizing test harness/auth utilities (plus adding a failure triage manifest).
Changes:
- Refactors Convex test harness registration (audit-log) and updates several Vitest expectations/fixtures to match current schemas/behavior.
- Hardens multiple Playwright specs via helper utilities (RBAC role assertions, governed-transitions selectors, auth storage-state generation) and more resilient async flows.
- Adds a detailed
docs/test-failure-manifest.mdto document historical failures and post-fix status.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/convex/registerAuditLogComponent.ts | Re-export audit-log component registrar from canonical Convex test helper. |
| src/test/convex/payments/helpers.ts | Adds allowMixedMortgageObligations option to unblock mixed-obligation execution tests. |
| src/test/convex/engine/hash-chain-reconciliation.test.ts | Updates hash-chain tests to seed richer audit journal entries + new canonical envelope expectations. |
| e2e/simulation.spec.ts | Replaces brittle settlement branching with a deterministic outcome wait helper. |
| e2e/rbac/member.spec.ts | Uses shared helper for role badge assertion. |
| e2e/rbac/helpers.ts | Adds expectResolvedRole helper for role badge checks. |
| e2e/rbac/admin.spec.ts | Uses shared helper for role badge assertion. |
| e2e/helpers/document-engine.ts | Adds admin-auth bootstrapping helper + shared admin storage-state path. |
| e2e/helpers/auth-storage.ts | Strengthens auth storage-state creation by verifying org switch has settled before saving. |
| e2e/governed-transitions.spec.ts | Hardens navigation and entity selectors; reduces ambiguous locator usage. |
| e2e/document-engine/workflow.spec.ts | Switches Document Engine workflow spec to use admin storageState. |
| e2e/document-engine/variables.spec.ts | Uses admin storageState + bootstraps admin auth before running guarded-route tests. |
| e2e/document-engine/templates.spec.ts | Uses admin storageState + bootstraps admin auth for setup/teardown flows. |
| e2e/document-engine/navigation.spec.ts | Uses admin storageState + bootstraps admin auth before running guarded-route tests. |
| e2e/document-engine/library.spec.ts | Uses admin storageState + bootstraps admin auth before running guarded-route tests. |
| e2e/document-engine/groups.spec.ts | Uses admin storageState + bootstraps admin auth before running guarded-route tests. |
| e2e/document-engine/generate.spec.ts | Uses admin storageState + bootstraps admin auth before running guarded-route tests. |
| e2e/document-engine/designer.spec.ts | Uses admin storageState + bootstraps admin auth for setup/teardown flows. |
| e2e/demo-listings.spec.ts | Replaces hard-coded listing slug/title with fixture-driven featured listing selection. |
| e2e/auth/protected-routes.spec.ts | Updates /sign-in redirect assertion to AuthKit host pattern. |
| e2e/auth/onboarding.spec.ts | Reworks onboarding auth to generate fresh per-test storage states and removes hard-coded fixture email usage. |
| e2e/auth/login.spec.ts | Scopes profile email assertions to a specific “Email” card to avoid strict-mode collisions. |
| docs/test-failure-manifest.md | Adds comprehensive failure triage/manifest and post-fix verification notes. |
| convex/test/registerAuditLogComponent.ts | Switches to convex-audit-log/test’s official registrar. |
| convex/payments/webhooks/tests/vopayWebhook.test.ts | Seeds missing transfer context fields (mortgage/obligation) for webhook tests. |
| convex/payments/webhooks/tests/eftVopayWebhook.test.ts | Seeds required lender + accounts and fills lenderId for EFT VoPay tests. |
| convex/payments/transfers/tests/collectionAttemptReconciliation.integration.test.ts | Updates expectations for transfer-owned cash receipt posting. |
| convex/payments/collectionPlan/tests/ruleContract.test.ts | Updates rule-config tests to assert typed config behavior. |
| convex/payments/collectionPlan/tests/execution.test.ts | Enables mixed-mortgage obligation seeding for the intended rejection-path test. |
| convex/payments/cashLedger/tests/transferReconciliation.test.ts | Updates tests to assert escalation behavior (vs self-healing) and revises assertions. |
| convex/machines/tests/deal.integration.test.ts | Adds fake-timer setup/teardown to prevent timer leakage/cascading harness failures. |
| convex/engine/machines/tests/collectionAttempt.test.ts | Updates confirmed-state assertions to reflect recordSettlementObserved on FUNDS_SETTLED. |
| convex/crm/tests/viewEngine.test.ts | Updates computed-field fieldDefId expectation. |
| convex/crm/tests/userSavedViews.test.ts | Updates CRM auth expectations to match current admin-only contract. |
| .gitignore | Ignores .tmp/ directory used by e2e onboarding auth-state generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Connorbelez
force-pushed
the
04-11-provider_managed_payment_schedule
branch
from
102424e to
a99f4e2
Compare
Connorbelez
force-pushed
the
test-fixes
branch
3 times, most recently
from
ab2b98e to
3a5ca5e
Compare
Connorbelez
changed the base branch from
04-11-provider_managed_payment_schedule
to
graphite-base/393
This was referenced Apr 13, 2026
Merged
Merged
Owner
Author
Merge activity
|
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/payments/transfers/queries.ts (1)
301-307:⚠️ Potential issue | 🟠 MajorClose the
getTransfersByPipelineauth bypass.Line 306 adds a deal-level guard to
getPipelineStatus, but the siblinggetTransfersByPipelinequery below is still.public()and returns the same legs for anypipelineId. That leaves the new restriction bypassable and leaks cross-tenant transfer metadata. Please either makegetTransfersByPipelineinternal or resolve the owning deal/mortgage and run the same access assertion before returning legs.As per coding guidelines, Authorization (RBAC) must be wired into every query and mutation from day one via middleware chains.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/queries.ts` around lines 301 - 307, getTransfersByPipeline currently remains public and leaks transfer legs for any pipelineId while getPipelineStatus enforces deal-level access; fix by wiring the same authorization: either change getTransfersByPipeline from .public() to the internal paymentQuery chain used by getPipelineStatus, or resolve the owning deal/mortgage for the supplied pipelineId (lookup pipeline -> mortgage/deal) and call assertDealAccess(ctx, dealId) before returning legs. Reference getTransfersByPipeline, getPipelineStatus, and assertDealAccess when applying the change so the query enforces RBAC consistently.convex/payments/cashLedger/integrations.ts (1)
1387-1420:⚠️ Potential issue | 🟠 MajorDon't treat corrected receipts as ambiguous originals.
postCashCorrectionForEntry()preservesattemptId/transferRequestIdon replacement entries. If aCASH_RECEIVEDis corrected with anotherCASH_RECEIVED, both rows satisfy this predicate and this branch now throwsAMBIGUOUS_ORIGINAL_ENTRY, so a later payment/transfer reversal can no longer run against an otherwise valid history. Resolve the current live receipt from the correction/reversal ancestry before enforcing the single-match invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/integrations.ts` around lines 1387 - 1420, The current filter over allObligationEntries (used in cashReceivedMatches) can match both an original CASH_RECEIVED and its corrected replacement because postCashCorrectionForEntry preserves attemptId/transferRequestId; update the logic to first resolve each candidate to its current live entry via the correction/replacement ancestry (follow replacementId/previousEntryId chain or use the same helper used by postCashCorrectionForEntry to get the canonical entry) and then deduplicate by that canonical entry id before enforcing the single-match invariant; keep throwing ORIGINAL_ENTRY_NOT_FOUND when none, but only throw AMBIGUOUS_ORIGINAL_ENTRY when multiple distinct canonical CASH_RECEIVED entries remain (include matchingEntryIds from those canonical ids).
🧹 Nitpick comments (6)
convex/payments/transfers/providers/__tests__/registry.test.ts (1)
40-44: Add the missing failure-path test forpad_rotessaenv gating.You now cover the configured path; add one test for missing/empty
ROTESSA_API_KEYto lock down registry behavior and prevent silent regressions.Suggested test addition
it('resolves "pad_rotessa" to RotessaTransferProvider when configured', () => { vi.stubEnv("ROTESSA_API_KEY", "test_rotessa_api_key"); const provider = getTransferProvider("pad_rotessa"); expect(provider).toBeInstanceOf(RotessaTransferProvider); }); + +it('throws for "pad_rotessa" when ROTESSA_API_KEY is unset', () => { + vi.stubEnv("ROTESSA_API_KEY", ""); + expect(() => getTransferProvider("pad_rotessa")).toThrow(/rotessa|api key|configured/i); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/providers/__tests__/registry.test.ts` around lines 40 - 44, Add a test that verifies the registry rejects "pad_rotessa" when ROTESSA_API_KEY is not set: stub the environment so ROTESSA_API_KEY is undefined or empty (using vi.stubEnv), call getTransferProvider("pad_rotessa") and assert the call fails (e.g., throws an Error or returns a clearly-failing result per the registry's contract) instead of returning a RotessaTransferProvider; reference getTransferProvider and RotessaTransferProvider in the new test to ensure the env-gating behavior is locked down.e2e/rbac/helpers.ts (1)
4-7: Scope the "Role" locator to the Session Context card for maintainability.Currently,
page.getByText("Role", { exact: true })is page-global. While only one "Role" label exists in the demo/workos UI, scoping the selector to the explicit "Session Context" container makes the test more resilient and clearly documents the intended target.Suggested refactor
export async function expectResolvedRole(page: Page, role: string) { - const roleLabel = page.getByText("Role", { exact: true }); + const sessionCard = page.getByText("Session Context", { exact: true }).locator(".."); + const roleLabel = sessionCard.getByText("Role", { exact: true }); await expect(roleLabel).toBeVisible({ timeout: 10_000 }); await expect(roleLabel.locator("xpath=following-sibling::*[1]")).toHaveText( role, { timeout: 10_000 } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/rbac/helpers.ts` around lines 4 - 7, The Role label locator is currently page-global (page.getByText("Role", { exact: true })) and should be scoped to the "Session Context" card to make the test resilient; update the selector so you first locate the "Session Context" container (e.g., via getByText("Session Context") or a container locator used in this file) and then call getByText("Role", { exact: true }) on that container to produce roleLabel, then keep the existing visibility/assertion and the following-sibling locator usage (roleLabel.locator("xpath=following-sibling::*[1]")) unchanged so behavior is the same but scoped.e2e/governed-transitions.spec.ts (1)
74-76: ScopegetEntityCardto entity-card containers to reduce selector collisions.Current locator targets any matching button text, which can accidentally match nav/action buttons and cause flaky assertions as the page grows.
Refactor direction
function getEntityCard(page: import("@playwright/test").Page, label: string) { - return page.locator("button").filter({ hasText: label }).first(); + return page + .locator("[data-testid='entity-card']") + .filter({ has: page.getByText(label, { exact: true }) }) + .first(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/governed-transitions.spec.ts` around lines 74 - 76, The getEntityCard locator is too broad and matches any button with the given text; restrict it to buttons inside the entity-card component to avoid collisions with nav/action buttons. Update the getEntityCard function to first scope to the entity-card container (e.g., locate the "entity-card" host/selector) and then find the button with the given label within that container (still using .first() to keep existing behavior); reference getEntityCard and the entity-card container when making the change so the selector now targets only buttons inside entity-card elements.convex/engine/machines/__tests__/collectionAttempt.test.ts (1)
469-475: Clarify theFUNDS_SETTLEDcase name in this loop.This branch now expects
recordSettlementObserved, so the generated titleconfirmed ignores FUNDS_SETTLEDis misleading. Consider splitting/renaming that case to reflect “same-state with side effect.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/machines/__tests__/collectionAttempt.test.ts` around lines 469 - 475, The test branch checks event.type === "FUNDS_SETTLED" and expects actions to include "recordSettlementObserved", so rename or split the test case that currently reads something like "confirmed ignores FUNDS_SETTLED" into a clearer description (e.g., "confirmed same-state emits recordSettlementObserved for FUNDS_SETTLED") or split into two tests: one that asserts FUNDS_SETTLED yields recordSettlementObserved and another that asserts all other events produce no actions; update the test title(s) near the assertions referencing "FUNDS_SETTLED" and "recordSettlementObserved" in collectionAttempt.test.ts accordingly.src/test/auth/permissions/permission-metadata-sync.test.ts (1)
52-95: Factor repeated orphan rationale into a shared constant.The repeated
"Protected in practice by admin:access wildcard..."string appears many times; extracting it into a constant will reduce copy/paste churn and typo drift.♻️ Suggested refactor
+const ADMIN_WILDCARD_ORPHAN_REASON = + "Protected in practice by admin:access wildcard; no longer explicitly assigned in ROLE_PERMISSIONS"; + const KNOWN_ORPHANS: Record<string, string> = { "onboarding:manage": "Reserved for future onboarding admin workflow; metadata kept for UI completeness", - "onboarding:review": - "Protected in practice by admin:access wildcard; no longer explicitly assigned in ROLE_PERMISSIONS", - "role:assign": - "Protected in practice by admin:access wildcard; no longer explicitly assigned in ROLE_PERMISSIONS", + "onboarding:review": ADMIN_WILDCARD_ORPHAN_REASON, + "role:assign": ADMIN_WILDCARD_ORPHAN_REASON, // ... };Based on learnings: “DRY: Don't Repeat Yourself - abstract repeated code into reusable functions or modules”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/auth/permissions/permission-metadata-sync.test.ts` around lines 52 - 95, The large repeated rationale string ("Protected in practice by admin:access wildcard; ...") should be extracted into a single constant (e.g., PROTECTED_BY_ADMIN_RATIONALE) in the test module and reused for each permission entry instead of repeating the literal; update permission-metadata-sync.test.ts to declare the constant near the top and replace all occurrences in the permission map (references around the ROLE_PERMISSIONS/permission entries) with that constant to reduce duplication and prevent typos.convex/payments/cashLedger/__tests__/transferReconciliation.test.ts (1)
640-695: Align the "to SUSPENSE" test names with what the assertions now prove.Both cases still describe a SUSPENSE/LENDER_PAYABLE posting, but the updated checks now require
getSuspenseEscalatedEntryForTransfer(...)to returnnull. Rename the tests/comments or restore the ledger assertion if suspense posting is still part of the contract.Also applies to: 786-835
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/__tests__/transferReconciliation.test.ts` around lines 640 - 695, The test name and comments imply a SUSPENSE/LENDER_PAYABLE ledger posting but the assertions (using getSuspenseEscalatedEntryForTransfer and expecting null) prove no suspense entry is created; update the test and its duplicate (the other case around transferReconciliation.test lines ~786-835) to reflect the actual behavior by renaming the test descriptions/comments to state "escalates to SUSPENSE without creating a suspense journal entry" or "escalates without suspense posting", or if the contract should still create a suspense entry, restore the ledger assertion to expect a non-null suspense entry; locate the tests referencing transferReconciliationCron.retriggerTransferConfirmation and getSuspenseEscalatedEntryForTransfer to make the change.
🤖 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/auth/resourceChecks.ts`:
- Around line 344-369: canAccessBorrowerEntity currently grants access if the
viewer can access any mortgage for that borrower, which lets a viewer with
access to mortgage A wrongly pass checks for borrower-linked resources of
mortgage B; change canAccessBorrowerEntity to accept an optional resource
context id (e.g., mortgageId or obligation/transfer id) and only treat
third-party access as valid when a found mortgageBorrowers link matches that
context before calling canAccessMortgage; keep the existing short-circuits for
viewer.isFairLendAdmin and self-access via getBorrowerByAuthId, but when
iterating ctx.db.query("mortgageBorrowers").withIndex("by_borrower") only
consider links where link.mortgageId === providedContextId (and then call
canAccessMortgage for that matching mortgageId).
In `@convex/engine/commands.ts`:
- Around line 146-159: The code currently hardcodes requestedByActorId,
requestedByActorType, and triggerSource as admin values when calling
runManualInboundCollectionForObligation; instead derive these metadata fields
from ctx.viewer (use ctx.viewer.authId for requestedByActorId and a
viewer-sourced type/role for requestedByActorType) and set triggerSource based
on the actual caller context (e.g., "admin_manual" only if ctx.viewer indicates
an admin, otherwise "manual" or another appropriate value), or alternatively
tighten the endpoint to require an explicit admin-only guard; update the call
site that constructs the manualSettlement payload and the requestedBy* fields to
use ctx.viewer-derived values rather than the hardcoded "admin" literals.
In `@convex/payments/collectionPlan/__tests__/admin.test.ts`:
- Around line 184-209: The audit assertion is too weak because
Array.isArray(attemptAuditEvents) passes for an empty array; update the test
(around the attemptAuditEvents result from auditLog.queryByResource) to assert a
non-zero count or that a specific expected audit action exists (e.g.,
expect(attemptAuditEvents.length).toBeGreaterThan(0) or check for an event with
the expected action/type in attemptAuditEvents), ensuring the
auditLog.queryByResource call for resourceType "collectionAttempts" and
resourceId `${execution.collectionAttemptId}` actually produced entries.
In `@convex/payments/collectionPlan/manualCollection.ts`:
- Around line 83-119: The current non-atomic check in
findExistingManualPlanEntryId (which iterates MANUAL_COLLECTION_ENTRY_STATUSES
and calls internal.payments.collectionPlan.queries.getPlanEntriesByStatus)
allows race conditions where concurrent requests both miss and insert duplicate
collectionPlanEntries; replace this by moving the existence check and insert
into a single atomic indexed mutation keyed by executionIdempotencyKey (e.g.,
add a unique index on collectionPlanEntries.executionIdempotencyKey and
implement a mutation like createOrGetCollectionPlanEntry that upserts or returns
the existing document id), update callers (createEntry / executePlanEntry) to
call that mutation and return the canonical entry _id, and remove the
multi-bucket scan in findExistingManualPlanEntryId so idempotency is enforced by
the DB-backed mutation.
In `@convex/payments/collectionPlan/workout.ts`:
- Around line 71-97: The write handlers createWorkoutPlan, activateWorkoutPlan,
completeWorkoutPlan, and cancelWorkoutPlan currently call their mutation
implementations without resource checks; update each handler to call the
appropriate guard (assertMortgageAccess for handlers operating on a mortgage id
and assertWorkoutPlanAccess for handlers operating on a workoutPlanId) with the
same ctx and ctx.viewer before invoking the mutation logic (ensure you pass the
correct id param into the assert call), so authorization enforced by
canAccessWorkoutPlan/canAccessMortgage is applied to all mutations invoked by
paymentMutation.
In `@convex/payments/webhooks/__tests__/stripeWebhook.test.ts`:
- Around line 411-447: The test currently only exercises the deferred handler
processUnsupportedStripeWebhook (it pre-seeds a webhookEvents row and calls that
function directly) while claiming to test the "persistence bridge"; update the
test to either (A) rename the describe/it to clearly state it tests the deferred
handler only, or (B) add a companion end-to-end test that invokes the actual
entrypoint stripeWebhook (or the code path that calls persistStripeWebhook and
ctx.scheduler.runAfter) so the full persistence + scheduling + HTTP handling
flow is covered; reference processUnsupportedStripeWebhook,
persistStripeWebhook, ctx.scheduler.runAfter and stripeWebhook when adding or
renaming tests so the intended unit under test is unambiguous.
In `@convex/payments/webhooks/stripe.ts`:
- Around line 153-177: The catch block in
scheduleUnsupportedStripeWebhookProcessing currently calls
markTransferWebhookFailed and if that throws it will bubble up and mask the
original scheduler failure; wrap the markTransferWebhookFailed call in its own
try/catch so the status update is best-effort (log any error from
markTransferWebhookFailed but do not rethrow), and always return the original {
ok: false, error: message } from scheduleUnsupportedStripeWebhookProcessing
after attempting the status update; reference ctx.scheduler.runAfter,
internal.payments.webhooks.stripe.processUnsupportedStripeWebhook, and
markTransferWebhookFailed to locate and change the code.
- Line 4: The exported webhook action processUnsupportedStripeWebhook is created
with raw internalAction(...) which violates the project's fluent-convex pattern;
replace its export to use the convex.action() builder by wiring the existing
handler function into
convex.action().handler(processUnsupportedStripeWebhook).internal() (or inline
the handler into .handler(...)) so the action follows the fluent chain; apply
the same replacement to other webhook modules (rotessaPad.ts, verification.ts,
legacyReversal.ts) to keep exports consistent.
In `@e2e/auth/onboarding.spec.ts`:
- Around line 28-46: createFreshStorageState and openOnboardingPage can leak
browser contexts and temp storage files when intermediate steps throw; wrap
creation and usage of bootstrapContext, test context/page and the
storageStatePath in try/finally blocks so that bootstrapContext.close() and test
context.close() are always called and the temporary file in
onboardingAuthStateDir (storageStatePath) is unlinked in the finally.
Specifically, update createFreshStorageState to create bootstrapContext,
bootstrapPage, and storageStatePath then call createAuthStorageState inside a
try and ensure bootstrapContext.close() in finally; similarly update
openOnboardingPage to wrap creation of the test context/page, page.goto and
visibility checks in try/finally so testContext.close() always runs and remove
the temp storageStatePath after use.
In `@e2e/helpers/document-engine.ts`:
- Around line 16-17: ADMIN_STORAGE_STATE being a fixed filename causes race
conditions when tests run in parallel; replace the shared constant with a
per-test unique storage path (e.g., build the filename using the orgId and a
random UUID like `storage-${orgId}-${randomUUID()}.json`) or change the write
logic to perform atomic writes (write to a temp file then rename) around
functions that read/write the storage state (references: ADMIN_STORAGE_STATE,
any helper functions that call existsSync()/writeFile()/readFile()) so each
worker uses its own file or performs atomic replace to avoid concurrent
read/write races.
In `@src/lib/auth.ts`:
- Around line 25-28: The hasPermission function currently treats
ADMIN_ACCESS_PERMISSION as a global override which lets callers like
canAccessAdminPath and requireFairLendAdmin escape staff-boundary checks; change
hasPermission to accept an options flag (e.g., allowAdminOverride: boolean =
true) and only treat ADMIN_ACCESS_PERMISSION as an override when that flag is
true, defaulting to true to preserve existing behavior; then update
boundary-sensitive callers (canAccessAdminPath, requireFairLendAdmin, and any
underwriter checks) to call hasPermission(..., { allowAdminOverride: false }) so
admin:access does not implicitly grant staff-boundary permissions.
In `@src/routes/e2e/session.tsx`:
- Around line 32-54: The async IIFE can throw when calling refresh(),
getAccessToken(), or decodeAccessToken, leaving tokenSnapshot null and causing
/e2e/session to hang; wrap the token acquisition and decode steps (the calls to
refresh(), getAccessToken(), and decodeAccessToken(token)) in a try/catch, and
in the catch ensure you call setTokenSnapshot with the same "logged-out" default
object (organizationId: null, permissions: [], role: null) and return,
optionally logging the error, so the route can terminate even on token
acquisition/decoding failures; keep the existing cancelled checks around
setTokenSnapshot.
---
Outside diff comments:
In `@convex/payments/cashLedger/integrations.ts`:
- Around line 1387-1420: The current filter over allObligationEntries (used in
cashReceivedMatches) can match both an original CASH_RECEIVED and its corrected
replacement because postCashCorrectionForEntry preserves
attemptId/transferRequestId; update the logic to first resolve each candidate to
its current live entry via the correction/replacement ancestry (follow
replacementId/previousEntryId chain or use the same helper used by
postCashCorrectionForEntry to get the canonical entry) and then deduplicate by
that canonical entry id before enforcing the single-match invariant; keep
throwing ORIGINAL_ENTRY_NOT_FOUND when none, but only throw
AMBIGUOUS_ORIGINAL_ENTRY when multiple distinct canonical CASH_RECEIVED entries
remain (include matchingEntryIds from those canonical ids).
In `@convex/payments/transfers/queries.ts`:
- Around line 301-307: getTransfersByPipeline currently remains public and leaks
transfer legs for any pipelineId while getPipelineStatus enforces deal-level
access; fix by wiring the same authorization: either change
getTransfersByPipeline from .public() to the internal paymentQuery chain used by
getPipelineStatus, or resolve the owning deal/mortgage for the supplied
pipelineId (lookup pipeline -> mortgage/deal) and call assertDealAccess(ctx,
dealId) before returning legs. Reference getTransfersByPipeline,
getPipelineStatus, and assertDealAccess when applying the change so the query
enforces RBAC consistently.
---
Nitpick comments:
In `@convex/engine/machines/__tests__/collectionAttempt.test.ts`:
- Around line 469-475: The test branch checks event.type === "FUNDS_SETTLED" and
expects actions to include "recordSettlementObserved", so rename or split the
test case that currently reads something like "confirmed ignores FUNDS_SETTLED"
into a clearer description (e.g., "confirmed same-state emits
recordSettlementObserved for FUNDS_SETTLED") or split into two tests: one that
asserts FUNDS_SETTLED yields recordSettlementObserved and another that asserts
all other events produce no actions; update the test title(s) near the
assertions referencing "FUNDS_SETTLED" and "recordSettlementObserved" in
collectionAttempt.test.ts accordingly.
In `@convex/payments/cashLedger/__tests__/transferReconciliation.test.ts`:
- Around line 640-695: The test name and comments imply a
SUSPENSE/LENDER_PAYABLE ledger posting but the assertions (using
getSuspenseEscalatedEntryForTransfer and expecting null) prove no suspense entry
is created; update the test and its duplicate (the other case around
transferReconciliation.test lines ~786-835) to reflect the actual behavior by
renaming the test descriptions/comments to state "escalates to SUSPENSE without
creating a suspense journal entry" or "escalates without suspense posting", or
if the contract should still create a suspense entry, restore the ledger
assertion to expect a non-null suspense entry; locate the tests referencing
transferReconciliationCron.retriggerTransferConfirmation and
getSuspenseEscalatedEntryForTransfer to make the change.
In `@convex/payments/transfers/providers/__tests__/registry.test.ts`:
- Around line 40-44: Add a test that verifies the registry rejects "pad_rotessa"
when ROTESSA_API_KEY is not set: stub the environment so ROTESSA_API_KEY is
undefined or empty (using vi.stubEnv), call getTransferProvider("pad_rotessa")
and assert the call fails (e.g., throws an Error or returns a clearly-failing
result per the registry's contract) instead of returning a
RotessaTransferProvider; reference getTransferProvider and
RotessaTransferProvider in the new test to ensure the env-gating behavior is
locked down.
In `@e2e/governed-transitions.spec.ts`:
- Around line 74-76: The getEntityCard locator is too broad and matches any
button with the given text; restrict it to buttons inside the entity-card
component to avoid collisions with nav/action buttons. Update the getEntityCard
function to first scope to the entity-card container (e.g., locate the
"entity-card" host/selector) and then find the button with the given label
within that container (still using .first() to keep existing behavior);
reference getEntityCard and the entity-card container when making the change so
the selector now targets only buttons inside entity-card elements.
In `@e2e/rbac/helpers.ts`:
- Around line 4-7: The Role label locator is currently page-global
(page.getByText("Role", { exact: true })) and should be scoped to the "Session
Context" card to make the test resilient; update the selector so you first
locate the "Session Context" container (e.g., via getByText("Session Context")
or a container locator used in this file) and then call getByText("Role", {
exact: true }) on that container to produce roleLabel, then keep the existing
visibility/assertion and the following-sibling locator usage
(roleLabel.locator("xpath=following-sibling::*[1]")) unchanged so behavior is
the same but scoped.
In `@src/test/auth/permissions/permission-metadata-sync.test.ts`:
- Around line 52-95: The large repeated rationale string ("Protected in practice
by admin:access wildcard; ...") should be extracted into a single constant
(e.g., PROTECTED_BY_ADMIN_RATIONALE) in the test module and reused for each
permission entry instead of repeating the literal; update
permission-metadata-sync.test.ts to declare the constant near the top and
replace all occurrences in the permission map (references around the
ROLE_PERMISSIONS/permission entries) with that constant to reduce duplication
and prevent typos.
🪄 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: 4a859562-b568-4969-90ec-252b7c681b26
📒 Files selected for processing (74)
.gitignoreAGENTS.mdCLAUDE.mdconvex/auth/__tests__/resourceChecks.test.tsconvex/auth/resourceChecks.tsconvex/crm/__tests__/userSavedViews.test.tsconvex/crm/__tests__/viewEngine.test.tsconvex/demo/simulation.tsconvex/engine/commands.tsconvex/engine/effects/__tests__/transfer.test.tsconvex/engine/effects/collectionAttempt.tsconvex/engine/effects/obligationPayment.tsconvex/engine/machines/__tests__/collectionAttempt.test.tsconvex/fluent.tsconvex/machines/__tests__/deal.integration.test.tsconvex/payments/bankAccounts/queries.tsconvex/payments/cashLedger/__tests__/transferReconciliation.test.tsconvex/payments/cashLedger/integrations.tsconvex/payments/cashLedger/queries.tsconvex/payments/collectionPlan/__tests__/admin.test.tsconvex/payments/collectionPlan/__tests__/execution.test.tsconvex/payments/collectionPlan/__tests__/ruleContract.test.tsconvex/payments/collectionPlan/execution.tsconvex/payments/collectionPlan/executionContract.tsconvex/payments/collectionPlan/initialScheduling.tsconvex/payments/collectionPlan/manualCollection.tsconvex/payments/collectionPlan/mutations.tsconvex/payments/collectionPlan/workout.tsconvex/payments/transfers/__tests__/collectionAttemptReconciliation.integration.test.tsconvex/payments/transfers/collectionAttemptReconciliation.tsconvex/payments/transfers/providers/__tests__/registry.test.tsconvex/payments/transfers/queries.tsconvex/payments/webhooks/__tests__/eftVopayWebhook.test.tsconvex/payments/webhooks/__tests__/reversalIntegration.test.tsconvex/payments/webhooks/__tests__/stripeWebhook.test.tsconvex/payments/webhooks/__tests__/vopayWebhook.test.tsconvex/payments/webhooks/stripe.tsconvex/test/registerAuditLogComponent.tsdocs/test-failure-manifest.mde2e/auth/login.spec.tse2e/auth/onboarding.spec.tse2e/auth/protected-routes.spec.tse2e/demo-listings.spec.tse2e/document-engine/designer.spec.tse2e/document-engine/generate.spec.tse2e/document-engine/groups.spec.tse2e/document-engine/library.spec.tse2e/document-engine/navigation.spec.tse2e/document-engine/templates.spec.tse2e/document-engine/variables.spec.tse2e/document-engine/workflow.spec.tse2e/governed-transitions.spec.tse2e/helpers/auth-storage.tse2e/helpers/document-engine.tse2e/rbac/admin.spec.tse2e/rbac/helpers.tse2e/rbac/member.spec.tse2e/simulation.spec.tsplaywright.config.tssrc/lib/auth.tssrc/routeTree.gen.tssrc/routes/_authenticated/authenticated.tsxsrc/routes/demo/convex.tsxsrc/routes/demo/simulation.tsxsrc/routes/e2e/session.tsxsrc/test/auth/chains/role-chains.test.tssrc/test/auth/middleware/requirePermission.test.tssrc/test/auth/permissions.tssrc/test/auth/permissions/new-permissions.test.tssrc/test/auth/permissions/permission-metadata-sync.test.tssrc/test/auth/route-guards.test.tssrc/test/convex/engine/hash-chain-reconciliation.test.tssrc/test/convex/payments/helpers.tssrc/test/convex/registerAuditLogComponent.ts
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.
Summary by Sourcery
Update tests and harnesses across e2e and Convex suites to align with current auth, routing, audit log, and payments behavior, and add a manifest documenting historical test failures and coverage.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
admin:accesspermission serving as a system-wide overrideBug Fixes
Improvements