Add ERP-first project payment automation#392
Open
michaelmwu wants to merge 1 commit into
Open
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a45722bd-8fc6-452f-af8b-2e48a4d2b9e2) |
📝 WalkthroughWalkthroughAdds end-to-end ERPNext project-payment automation: signed webhook ingestion, typed rule evaluation, durable allocations and notification outboxes, Discord delivery, recovery processing, dashboard management, configuration, migrations, and extensive tests. ChangesProject payment automation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ERPNext
participant BackendAPI
participant Worker
participant Postgres
participant DiscordBot
participant Discord
ERPNext->>BackendAPI: Signed bank transaction webhook
BackendAPI->>Postgres: Enqueue ingest job
Worker->>ERPNext: Fetch canonical bank transaction
Worker->>Postgres: Evaluate rules and persist allocation/outbox
Worker->>DiscordBot: Post notification with notification and lease IDs
DiscordBot->>Postgres: Claim delivery lease and load context
DiscordBot->>Discord: Send idempotent payment receipt
DiscordBot->>Postgres: Persist message receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| version=version, | ||
| ) | ||
| except ValueError as exc: | ||
| return None, JSONResponse({"error": str(exc)}, status_code=400) |
| metadata={"error": "invalid_project_payment_rule_payload"}, | ||
| ) | ||
| return JSONResponse( | ||
| {"error": "invalid_project_payment_rule_payload", "detail": str(exc)}, |
| metadata={"error": "invalid_project_payment_rule_payload"}, | ||
| ) | ||
| return JSONResponse( | ||
| {"error": "invalid_project_payment_rule_payload", "detail": str(exc)}, |
| ) | ||
| except (ValidationError, TypeError, ValueError) as exc: | ||
| return JSONResponse( | ||
| {"error": "invalid_project_payment_rule_payload", "detail": str(exc)}, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
tests/unit/test_worker_erpnext_bank_transaction_sync.py (1)
130-144: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that webhook revision metadata cannot replace the canonical ERP revision.
The test passes
"webhook-revision"but never verifies that the event and idempotency key use the document’smodifiedvalue. A regression trusting webhook metadata would still pass.Proposed assertions
event = captured["event"] assert getattr(event, "subject_id") == "local-transaction" + assert getattr(event, "subject_revision") == "2026-07-16T12:34:56+00:00" assert getattr(event, "facts")["transaction"]["amount"] == "1250.00" + assert captured["event_key"] == ( + "erpnext-bank-transaction:v1:ACC-BTN-0001:" + "2026-07-16T12:34:56+00:00" + )🤖 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 `@tests/unit/test_worker_erpnext_bank_transaction_sync.py` around lines 130 - 144, Update the test for processor.ingest_bank_transaction to assert that the captured event and idempotency key use the canonical ERP document modified revision, not the supplied "webhook-revision" metadata. Add assertions against the relevant captured event/idempotency fields while preserving the existing ingestion assertions.
🤖 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 `@apps/admin_dashboard/src/views/payment-automation-view.tsx`:
- Around line 168-184: Update the conditions built by the payment automation
rule creation flow in apps/admin_dashboard/src/views/payment-automation-view.tsx
(lines 168-184) to use the equals operator for identifier matching when mode ===
"automatic", while preserving contains for other modes. Update the
automatic-rule expectation in
apps/admin_dashboard/src/payment-automation-view.test.tsx (lines 88-105)
accordingly, then rebuild the dashboard bundle at
apps/api/src/five08/backend/static/dashboard/assets/index-BT3M2qFq.js (line 9).
In `@apps/api/src/five08/backend/api.py`:
- Around line 969-978: Update _project_payment_recovery_scheduler to remain
lightweight and re-read the live payment automation/notification flags and
recovery interval on every loop iteration instead of snapshotting
interval_seconds once. Only enqueue recovery when the current configuration
enables either applicable recovery mode, while preserving error logging and
enforcing the minimum interval. Update the startup logic around the scheduler
creation at the referenced initialization block so the scheduler is always
started, allowing runtime enablement, disablement, interval changes, and
notifications-only recovery to take effect without an API restart.
- Around line 9151-9169: Update the dead-job handling around revive_dead_job so
enqueueing occurs after every revival attempt, regardless of whether revived is
true or false. Preserve the existing revival log only for successful revival,
but move the request.app.state.queue.enqueue call outside the revived
conditional to ensure concurrent recovery attempts always redeliver the job.
- Around line 7923-7930: Extend validation of amount and parsed_amount to
enforce the allocations column’s NUMERIC(18, 6) constraints before saving the
rule. Reject finite positive values exceeding 12 integer digits or requiring
more than 6 fractional digits, including values that would round to zero, using
Decimal-based validation without silently rounding. Preserve the existing
invalid_project_payment_amount error for all rejected values.
In `@apps/discord_bot/src/five08/discord_bot/cogs/projects.py`:
- Around line 694-706: Update the project command handlers covering the
referenced validation, wrong-guild, lookup, and persistence branches to call
_audit_command_safe before every terminal return. Record invalid context,
wrong-guild, persistence failures, and project-lookup exceptions as distinct
denied/error outcomes, correcting the lookup exception currently labeled
project_not_open. Keep audit delivery best effort through the worker audit
ingest path so audit failures never alter the command response or control flow.
- Around line 572-638: The payment delivery flow must revalidate channel privacy
immediately after the final lease renewal and before channel.send. In the method
containing the shown context refresh and lease renewal, call
_private_channel_error at that point; if it reports a failure, release or fail
the delivery through the existing lease-cleanup path and return the appropriate
privacy error without sending the payment message.
In `@apps/worker/src/five08/worker/erpnext_bank_transaction_sync.py`:
- Around line 79-85: Update the transaction None-handling path around
erpnext_bank_transaction_to_input so canceled and zero-value documents first
persist the canonical terminal revision and supersede any revision-bound
confirmed allocations before returning the ignored response. Preserve the
existing ignored status, reason, and transaction_name fields, and continue
treating draft documents according to the established behavior.
In `@apps/worker/src/five08/worker/jobs.py`:
- Around line 216-219: The recover_project_payment_automation_job flow should
independently gate action and learning recovery on
project_payment_automation_enabled while still running notification outbox
recovery when its notification feature flag is enabled. Update the recovery
scheduler startup condition to activate when either recovery path is enabled. In
tests/unit/test_worker_project_payment_automation.py lines 762-776, replace the
full no-op assertion with coverage confirming disabled actions and enabled
notification recovery.
In `@packages/shared/src/five08/automation.py`:
- Around line 113-128: The _validate_value_for_operator method must validate
EXISTS conditions explicitly: when the operator is EXISTS, accept only bool or
None and raise ValueError for all other value types, including strings such as
"false". Preserve the existing IN and numeric-operator validation behavior.
In `@packages/shared/src/five08/project_payments.py`:
- Around line 996-1016: The outbox creation and delivery-context query must
enforce that the allocation and Discord channel belong to the same project.
Update the INSERT around the notification creation flow and the corresponding
delivery-context query near the referenced secondary location to validate
matching project IDs, preventing mismatched allocation/channel pairs from being
accepted or delivered.
In `@tests/unit/test_automation.py`:
- Around line 219-224: Update test_resolve_fact_path_only_traverses_mappings so
the list-based path assertion compares the result of
resolve_fact_path({"transaction": ["10"]}, "transaction.0") with the known
missing-path sentinel, rather than only checking that it is not None. Keep the
mapping traversal assertion unchanged.
In `@tests/unit/test_project_discord_channels.py`:
- Around line 112-127: Update
test_register_project_channel_is_idempotent_for_same_project and its _CursorStub
setup so the UPDATE operation returns refreshed channel metadata reflecting the
renamed channel, rather than the stale mapping. Assert the returned mapping
contains the new channel_name and verify cursor.executed[2] includes the
expected bound update values.
In `@tests/unit/test_project_payments.py`:
- Around line 102-138: The PII-leakage assertion in
test_approved_receipt_teaches_only_a_deterministic_review_suggestion must be
case-insensitive. Update the learning_key check to compare a normalized
lowercase form against the lowercase payer name, while preserving the existing
deterministic rule assertions.
In `@tests/unit/test_projects.py`:
- Around line 151-190: Strengthen
test_stale_erpnext_project_payload_cannot_overwrite_newer_cache_or_roster so
stale payloads are verified to perform no roster mutation at all. Inspect the
executed queries and assert that neither roster inserts nor DELETE/other roster
mutation statements are present, while preserving the existing stale revision
and external-ID assertions.
---
Nitpick comments:
In `@tests/unit/test_worker_erpnext_bank_transaction_sync.py`:
- Around line 130-144: Update the test for processor.ingest_bank_transaction to
assert that the captured event and idempotency key use the canonical ERP
document modified revision, not the supplied "webhook-revision" metadata. Add
assertions against the relevant captured event/idempotency fields while
preserving the existing ingestion assertions.
🪄 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: 6c64567f-da34-4230-8a4f-42e936f49e87
📒 Files selected for processing (52)
.env.exampleARCHITECTURE.mdapps/admin_dashboard/src/main.tsxapps/admin_dashboard/src/payment-automation-view.test.tsxapps/admin_dashboard/src/views/payment-automation-view.tsxapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/dashboard.pyapps/api/src/five08/backend/routes.pyapps/api/src/five08/backend/schemas.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-2UypYUrJ.cssapps/api/src/five08/backend/static/dashboard/assets/index-BT3M2qFq.jsapps/api/src/five08/backend/static/dashboard/assets/index-C3gljehK.cssapps/api/src/five08/backend/static/dashboard/assets/index-CjyViga_.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/src/five08/discord_bot/cogs/projects.pyapps/discord_bot/src/five08/discord_bot/utils/internal_api.pyapps/worker/src/five08/worker/erpnext_bank_transaction_sync.pyapps/worker/src/five08/worker/jobs.pyapps/worker/src/five08/worker/migrations/versions/20260716_0100_create_payment_automation_tables.pyapps/worker/src/five08/worker/migrations/versions/20260716_0101_add_payment_rule_learning_provenance.pyapps/worker/src/five08/worker/models.pyapps/worker/src/five08/worker/project_payment_automation.pyapps/worker/src/five08/worker/project_payment_learning.pycompose.yamldocs/configuration.mdpackages/shared/src/five08/automation.pypackages/shared/src/five08/automation_store.pypackages/shared/src/five08/clients/discord_bot.pypackages/shared/src/five08/clients/erpnext.pypackages/shared/src/five08/project_discord_channels.pypackages/shared/src/five08/project_payments.pypackages/shared/src/five08/projects.pypackages/shared/src/five08/queue.pypackages/shared/src/five08/runtime_config.pypackages/shared/src/five08/settings.pytests/unit/test_automation.pytests/unit/test_automation_store.pytests/unit/test_backend_api.pytests/unit/test_dashboard_html.pytests/unit/test_discord_bot_client.pytests/unit/test_erpnext_client.pytests/unit/test_internal_api.pytests/unit/test_project_discord_channels.pytests/unit/test_project_payments.pytests/unit/test_projects.pytests/unit/test_projects_cog.pytests/unit/test_runtime_config.pytests/unit/test_shared_queue.pytests/unit/test_worker_erpnext_bank_transaction_sync.pytests/unit/test_worker_project_payment_automation.pytests/unit/test_worker_project_payment_learning.py
💤 Files with no reviewable changes (1)
- apps/api/src/five08/backend/static/dashboard/assets/index-2UypYUrJ.css
Comment on lines
+168
to
+184
| const conditions: PaymentAutomationCondition[] = [ | ||
| { fact: "transaction.direction", operator: "equals", value: "inbound" }, | ||
| { fact: identifierFact, operator: "contains", value: identifierValue.trim() }, | ||
| { fact: "transaction.currency", operator: "equals", value: normalizedCurrency }, | ||
| ] | ||
| const payload: Record<string, unknown> = { project_id: selectedProjectId } | ||
| if (normalizedAmount) { | ||
| conditions.push({ fact: "transaction.amount", operator: "equals", value: normalizedAmount }) | ||
| payload.amount = normalizedAmount | ||
| } | ||
| const created = await props.onCreateRule({ | ||
| project_id: selectedProjectId, | ||
| priority: parsedPriority, | ||
| mode, | ||
| enabled: true, | ||
| conditions, | ||
| actions: [{ action_type: "project_payment.route", payload }], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require exact identity matching before automatic payment routing.
Automatic rules currently use substring matching, allowing a partial payer value to route an unrelated transaction.
apps/admin_dashboard/src/views/payment-automation-view.tsx#L168-L184: useequalswhenmode === "automatic".apps/admin_dashboard/src/payment-automation-view.test.tsx#L88-L105: update the automatic-rule expectation toequals.apps/api/src/five08/backend/static/dashboard/assets/index-BT3M2qFq.js#L9-L9: rebuild the dashboard bundle after fixing the source.
📍 Affects 3 files
apps/admin_dashboard/src/views/payment-automation-view.tsx#L168-L184(this comment)apps/admin_dashboard/src/payment-automation-view.test.tsx#L88-L105apps/api/src/five08/backend/static/dashboard/assets/index-BT3M2qFq.js#L9-L9
🤖 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 `@apps/admin_dashboard/src/views/payment-automation-view.tsx` around lines 168
- 184, Update the conditions built by the payment automation rule creation flow
in apps/admin_dashboard/src/views/payment-automation-view.tsx (lines 168-184) to
use the equals operator for identifier matching when mode === "automatic", while
preserving contains for other modes. Update the automatic-rule expectation in
apps/admin_dashboard/src/payment-automation-view.test.tsx (lines 88-105)
accordingly, then rebuild the dashboard bundle at
apps/api/src/five08/backend/static/dashboard/assets/index-BT3M2qFq.js (line 9).
Comment on lines
+969
to
+978
| async def _project_payment_recovery_scheduler(app: FastAPI) -> None: | ||
| """Periodically recover payment actions and notification outbox rows.""" | ||
| queue = app.state.queue | ||
| interval_seconds = max(60, settings.project_payment_recovery_interval_seconds) | ||
| while True: | ||
| try: | ||
| await _enqueue_project_payment_recovery_job(queue, reason="scheduler") | ||
| except Exception: | ||
| logger.exception("Failed scheduling project payment recovery job") | ||
| await asyncio.sleep(interval_seconds) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor live payment configuration without restarting the API.
Line 972 snapshots the interval, while Line 10655 only creates the scheduler when automation is enabled at startup. Dashboard enablement, disablement, interval updates, and notifications-only recovery therefore do not take effect dynamically.
Keep a lightweight scheduler running and re-read both flags and the interval each iteration.
Proposed fix
async def _project_payment_recovery_scheduler(app: FastAPI) -> None:
"""Periodically recover payment actions and notification outbox rows."""
queue = app.state.queue
- interval_seconds = max(60, settings.project_payment_recovery_interval_seconds)
while True:
try:
- await _enqueue_project_payment_recovery_job(queue, reason="scheduler")
+ if (
+ settings.project_payment_automation_enabled
+ or settings.project_payment_notifications_enabled
+ ):
+ await _enqueue_project_payment_recovery_job(
+ queue, reason="scheduler"
+ )
except Exception:
logger.exception("Failed scheduling project payment recovery job")
- await asyncio.sleep(interval_seconds)
+ await asyncio.sleep(
+ max(60, settings.project_payment_recovery_interval_seconds)
+ )- if settings.project_payment_automation_enabled:
- app.state.project_payment_recovery_task = asyncio.create_task(
- _project_payment_recovery_scheduler(app)
- )
- else:
- logger.info("Project payment automation recovery scheduler disabled by config")
+ app.state.project_payment_recovery_task = asyncio.create_task(
+ _project_payment_recovery_scheduler(app)
+ )Also applies to: 10655-10660
🤖 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 `@apps/api/src/five08/backend/api.py` around lines 969 - 978, Update
_project_payment_recovery_scheduler to remain lightweight and re-read the live
payment automation/notification flags and recovery interval on every loop
iteration instead of snapshotting interval_seconds once. Only enqueue recovery
when the current configuration enables either applicable recovery mode, while
preserving error logging and enforcing the minimum interval. Update the startup
logic around the scheduler creation at the referenced initialization block so
the scheduler is always started, allowing runtime enablement, disablement,
interval changes, and notifications-only recovery to take effect without an API
restart.
Comment on lines
+7923
to
+7930
| amount = rule.actions[0].payload.get("amount") | ||
| if amount is not None: | ||
| try: | ||
| parsed_amount = Decimal(str(amount).strip()) | ||
| except (InvalidOperation, ValueError, AttributeError) as exc: | ||
| raise ValueError("invalid_project_payment_amount") from exc | ||
| if not parsed_amount.is_finite() or parsed_amount <= 0: | ||
| raise ValueError("invalid_project_payment_amount") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate amounts against the database precision before saving the rule.
The API accepts values such as 1E100 or 0.0000001, but allocations use NUMERIC(18, 6). Execution will later overflow or round to zero and repeatedly fail instead of rejecting the rule upfront.
Proposed fix
if amount is not None:
try:
parsed_amount = Decimal(str(amount).strip())
+ persisted_amount = parsed_amount.quantize(Decimal("0.000001"))
except (InvalidOperation, ValueError, AttributeError) as exc:
raise ValueError("invalid_project_payment_amount") from exc
- if not parsed_amount.is_finite() or parsed_amount <= 0:
+ if (
+ not parsed_amount.is_finite()
+ or parsed_amount <= 0
+ or persisted_amount != parsed_amount
+ or parsed_amount > Decimal("999999999999.999999")
+ ):
raise ValueError("invalid_project_payment_amount")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| amount = rule.actions[0].payload.get("amount") | |
| if amount is not None: | |
| try: | |
| parsed_amount = Decimal(str(amount).strip()) | |
| except (InvalidOperation, ValueError, AttributeError) as exc: | |
| raise ValueError("invalid_project_payment_amount") from exc | |
| if not parsed_amount.is_finite() or parsed_amount <= 0: | |
| raise ValueError("invalid_project_payment_amount") | |
| amount = rule.actions[0].payload.get("amount") | |
| if amount is not None: | |
| try: | |
| parsed_amount = Decimal(str(amount).strip()) | |
| persisted_amount = parsed_amount.quantize(Decimal("0.000001")) | |
| except (InvalidOperation, ValueError, AttributeError) as exc: | |
| raise ValueError("invalid_project_payment_amount") from exc | |
| if ( | |
| not parsed_amount.is_finite() | |
| or parsed_amount <= 0 | |
| or persisted_amount != parsed_amount | |
| or parsed_amount > Decimal("999999999999.999999") | |
| ): | |
| raise ValueError("invalid_project_payment_amount") |
🤖 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 `@apps/api/src/five08/backend/api.py` around lines 7923 - 7930, Extend
validation of amount and parsed_amount to enforce the allocations column’s
NUMERIC(18, 6) constraints before saving the rule. Reject finite positive values
exceeding 12 integer digits or requiring more than 6 fractional digits,
including values that would round to zero, using Decimal-based validation
without silently rounding. Preserve the existing invalid_project_payment_amount
error for all rejected values.
Comment on lines
+9151
to
+9169
| if not job.created: | ||
| existing_job = await asyncio.to_thread(get_job, settings, job.id) | ||
| if existing_job is not None and existing_job.status == JobStatus.DEAD: | ||
| revived = await asyncio.to_thread( | ||
| revive_dead_job, | ||
| settings, | ||
| job.id, | ||
| expected_job_type=JOB_FUNCTIONS[ | ||
| "ingest_erpnext_bank_transaction_job" | ||
| ].__name__, | ||
| ) | ||
| if revived: | ||
| logger.info( | ||
| "Revived dead ERPNext Bank Transaction ingest job_id=%s", | ||
| job.id, | ||
| ) | ||
| await asyncio.to_thread(request.app.state.queue.enqueue, job.id) | ||
| else: | ||
| await asyncio.to_thread(request.app.state.queue.enqueue, job.id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Always redeliver after attempting dead-job revival.
Two requests can both observe dead; one revives it, then crashes before Redis delivery, while the other gets revived=False and skips enqueueing. This recreates the exact stranded queued-job window this recovery path intends to close.
Proposed fix
if not job.created:
existing_job = await asyncio.to_thread(get_job, settings, job.id)
if existing_job is not None and existing_job.status == JobStatus.DEAD:
revived = await asyncio.to_thread(
revive_dead_job,
settings,
job.id,
expected_job_type=JOB_FUNCTIONS[
"ingest_erpnext_bank_transaction_job"
].__name__,
)
if revived:
logger.info(
"Revived dead ERPNext Bank Transaction ingest job_id=%s",
job.id,
)
- await asyncio.to_thread(request.app.state.queue.enqueue, job.id)
- else:
- await asyncio.to_thread(request.app.state.queue.enqueue, job.id)
+ await asyncio.to_thread(request.app.state.queue.enqueue, job.id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not job.created: | |
| existing_job = await asyncio.to_thread(get_job, settings, job.id) | |
| if existing_job is not None and existing_job.status == JobStatus.DEAD: | |
| revived = await asyncio.to_thread( | |
| revive_dead_job, | |
| settings, | |
| job.id, | |
| expected_job_type=JOB_FUNCTIONS[ | |
| "ingest_erpnext_bank_transaction_job" | |
| ].__name__, | |
| ) | |
| if revived: | |
| logger.info( | |
| "Revived dead ERPNext Bank Transaction ingest job_id=%s", | |
| job.id, | |
| ) | |
| await asyncio.to_thread(request.app.state.queue.enqueue, job.id) | |
| else: | |
| await asyncio.to_thread(request.app.state.queue.enqueue, job.id) | |
| if not job.created: | |
| existing_job = await asyncio.to_thread(get_job, settings, job.id) | |
| if existing_job is not None and existing_job.status == JobStatus.DEAD: | |
| revived = await asyncio.to_thread( | |
| revive_dead_job, | |
| settings, | |
| job.id, | |
| expected_job_type=JOB_FUNCTIONS[ | |
| "ingest_erpnext_bank_transaction_job" | |
| ].__name__, | |
| ) | |
| if revived: | |
| logger.info( | |
| "Revived dead ERPNext Bank Transaction ingest job_id=%s", | |
| job.id, | |
| ) | |
| await asyncio.to_thread(request.app.state.queue.enqueue, job.id) |
🤖 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 `@apps/api/src/five08/backend/api.py` around lines 9151 - 9169, Update the
dead-job handling around revive_dead_job so enqueueing occurs after every
revival attempt, regardless of whether revived is true or false. Preserve the
existing revival log only for successful revival, but move the
request.app.state.queue.enqueue call outside the revived conditional to ensure
concurrent recovery attempts always redeliver the job.
Comment on lines
+572
to
+638
| # Re-check eligibility and renew immediately before the non-idempotent | ||
| # Discord write. This narrows the unavoidable DB-to-Discord race and | ||
| # never uses values supplied by the worker's HTTP request. | ||
| try: | ||
| context = await asyncio.to_thread( | ||
| get_project_payment_notification_delivery_context, | ||
| settings, | ||
| notification_id=notification_id, | ||
| worker_lease_token=worker_lease_token, | ||
| project_discord_channel_id=claimed_mapping_id, | ||
| ) | ||
| except Exception: | ||
| await self._fail_project_payment_delivery( | ||
| notification_id=notification_id, | ||
| project_discord_channel_id=claimed_mapping_id, | ||
| lease_token=lease_token, | ||
| error="notification_context_refresh_failed", | ||
| ) | ||
| return {"error": "payment_notification_context_unavailable"}, 503 | ||
| if context is None: | ||
| await self._fail_project_payment_delivery( | ||
| notification_id=notification_id, | ||
| project_discord_channel_id=claimed_mapping_id, | ||
| lease_token=lease_token, | ||
| error="notification_no_longer_eligible", | ||
| ) | ||
| return {"error": "payment_notification_not_eligible"}, 409 | ||
| try: | ||
| lease_renewed = await asyncio.to_thread( | ||
| renew_project_payment_discord_delivery_lease, | ||
| settings, | ||
| notification_id=notification_id, | ||
| worker_lease_token=worker_lease_token, | ||
| project_discord_channel_id=claimed_mapping_id, | ||
| lease_token=lease_token, | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| "Failed renewing project payment delivery lease before send id=%s", | ||
| notification_id, | ||
| exc_info=True, | ||
| ) | ||
| return {"error": "payment_notification_delivery_lease_unavailable"}, 503 | ||
| if not lease_renewed: | ||
| return {"error": "payment_notification_delivery_lease_lost"}, 503 | ||
|
|
||
| try: | ||
| # Discord limits a nonce to 25 characters. Keep it deterministic | ||
| # for diagnostics without treating it as our correctness boundary; | ||
| # the durable receipt and hidden marker remain the recovery source. | ||
| discord_nonce = ( | ||
| "pp-" + hashlib.sha256(notification_id.encode("utf-8")).hexdigest()[:22] | ||
| ) | ||
| message = await channel.send( | ||
| self._payment_message_content( | ||
| notification_id=notification_id, | ||
| amount=str(context.amount), | ||
| currency=context.currency, | ||
| posted_at=( | ||
| context.posted_at.isoformat() | ||
| if context.posted_at is not None | ||
| else None | ||
| ), | ||
| ), | ||
| allowed_mentions=discord.AllowedMentions.none(), | ||
| nonce=discord_nonce, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Recheck channel privacy immediately before sending.
The only privacy check occurs at Line 431. If permissions change while claiming the lease or scanning history, Line 625 can publish payment information to a newly public channel. Re-run _private_channel_error after the final lease renewal and release the lease if it fails.
Proposed fix
if not lease_renewed:
return {"error": "payment_notification_delivery_lease_lost"}, 503
+ channel_error = self._private_channel_error(guild, channel)
+ if channel_error is not None:
+ await self._record_project_channel_verification(
+ mapping_id=claimed_mapping_id,
+ error=channel_error,
+ )
+ await self._fail_project_payment_delivery(
+ notification_id=notification_id,
+ project_discord_channel_id=claimed_mapping_id,
+ lease_token=lease_token,
+ error=channel_error,
+ )
+ status = 503 if channel_error == "bot_member_unresolved" else 403
+ return {"error": channel_error}, status
+
try:
discord_nonce = (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Re-check eligibility and renew immediately before the non-idempotent | |
| # Discord write. This narrows the unavoidable DB-to-Discord race and | |
| # never uses values supplied by the worker's HTTP request. | |
| try: | |
| context = await asyncio.to_thread( | |
| get_project_payment_notification_delivery_context, | |
| settings, | |
| notification_id=notification_id, | |
| worker_lease_token=worker_lease_token, | |
| project_discord_channel_id=claimed_mapping_id, | |
| ) | |
| except Exception: | |
| await self._fail_project_payment_delivery( | |
| notification_id=notification_id, | |
| project_discord_channel_id=claimed_mapping_id, | |
| lease_token=lease_token, | |
| error="notification_context_refresh_failed", | |
| ) | |
| return {"error": "payment_notification_context_unavailable"}, 503 | |
| if context is None: | |
| await self._fail_project_payment_delivery( | |
| notification_id=notification_id, | |
| project_discord_channel_id=claimed_mapping_id, | |
| lease_token=lease_token, | |
| error="notification_no_longer_eligible", | |
| ) | |
| return {"error": "payment_notification_not_eligible"}, 409 | |
| try: | |
| lease_renewed = await asyncio.to_thread( | |
| renew_project_payment_discord_delivery_lease, | |
| settings, | |
| notification_id=notification_id, | |
| worker_lease_token=worker_lease_token, | |
| project_discord_channel_id=claimed_mapping_id, | |
| lease_token=lease_token, | |
| ) | |
| except Exception: | |
| logger.warning( | |
| "Failed renewing project payment delivery lease before send id=%s", | |
| notification_id, | |
| exc_info=True, | |
| ) | |
| return {"error": "payment_notification_delivery_lease_unavailable"}, 503 | |
| if not lease_renewed: | |
| return {"error": "payment_notification_delivery_lease_lost"}, 503 | |
| try: | |
| # Discord limits a nonce to 25 characters. Keep it deterministic | |
| # for diagnostics without treating it as our correctness boundary; | |
| # the durable receipt and hidden marker remain the recovery source. | |
| discord_nonce = ( | |
| "pp-" + hashlib.sha256(notification_id.encode("utf-8")).hexdigest()[:22] | |
| ) | |
| message = await channel.send( | |
| self._payment_message_content( | |
| notification_id=notification_id, | |
| amount=str(context.amount), | |
| currency=context.currency, | |
| posted_at=( | |
| context.posted_at.isoformat() | |
| if context.posted_at is not None | |
| else None | |
| ), | |
| ), | |
| allowed_mentions=discord.AllowedMentions.none(), | |
| nonce=discord_nonce, | |
| ) | |
| if not lease_renewed: | |
| return {"error": "payment_notification_delivery_lease_lost"}, 503 | |
| channel_error = self._private_channel_error(guild, channel) | |
| if channel_error is not None: | |
| await self._record_project_channel_verification( | |
| mapping_id=claimed_mapping_id, | |
| error=channel_error, | |
| ) | |
| await self._fail_project_payment_delivery( | |
| notification_id=notification_id, | |
| project_discord_channel_id=claimed_mapping_id, | |
| lease_token=lease_token, | |
| error=channel_error, | |
| ) | |
| status = 503 if channel_error == "bot_member_unresolved" else 403 | |
| return {"error": channel_error}, status | |
| try: | |
| # Discord limits a nonce to 25 characters. Keep it deterministic | |
| # for diagnostics without treating it as our correctness boundary; | |
| # the durable receipt and hidden marker remain the recovery source. | |
| discord_nonce = ( | |
| "pp-" + hashlib.sha256(notification_id.encode("utf-8")).hexdigest()[:22] | |
| ) | |
| message = await channel.send( | |
| self._payment_message_content( | |
| notification_id=notification_id, | |
| amount=str(context.amount), | |
| currency=context.currency, | |
| posted_at=( | |
| context.posted_at.isoformat() | |
| if context.posted_at is not None | |
| else None | |
| ), | |
| ), | |
| allowed_mentions=discord.AllowedMentions.none(), | |
| nonce=discord_nonce, | |
| ) |
🤖 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 `@apps/discord_bot/src/five08/discord_bot/cogs/projects.py` around lines 572 -
638, The payment delivery flow must revalidate channel privacy immediately after
the final lease renewal and before channel.send. In the method containing the
shown context refresh and lease renewal, call _private_channel_error at that
point; if it reports a failure, release or fail the delivery through the
existing lease-cleanup path and return the appropriate privacy error without
sending the payment message.
Comment on lines
+996
to
+1016
| with get_postgres_connection(settings) as conn: | ||
| with conn.cursor(row_factory=dict_row) as cursor: | ||
| cursor.execute( | ||
| """ | ||
| INSERT INTO project_payment_notification_outbox ( | ||
| id, | ||
| allocation_id, | ||
| project_discord_channel_id, | ||
| idempotency_key, | ||
| payload | ||
| ) VALUES (%s, %s::uuid, %s::uuid, %s, %s) | ||
| ON CONFLICT DO NOTHING | ||
| RETURNING id::text | ||
| """, | ||
| ( | ||
| notification_id, | ||
| allocation_id, | ||
| project_discord_channel_id, | ||
| normalized_key, | ||
| Jsonb(message_payload), | ||
| ), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require the allocation and Discord channel to belong to the same project.
The insert accepts both IDs independently, while the delivery authorization joins them without checking allocation.project_id = channel.project_id. A mismatched call or row could disclose one project's payment amount in another project's active channel. Validate this relationship when creating the outbox row and again in the delivery-context query.
Also applies to: 1338-1352
🤖 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 `@packages/shared/src/five08/project_payments.py` around lines 996 - 1016, The
outbox creation and delivery-context query must enforce that the allocation and
Discord channel belong to the same project. Update the INSERT around the
notification creation flow and the corresponding delivery-context query near the
referenced secondary location to validate matching project IDs, preventing
mismatched allocation/channel pairs from being accepted or delivered.
Comment on lines
+219
to
+224
| def test_resolve_fact_path_only_traverses_mappings() -> None: | ||
| assert ( | ||
| resolve_fact_path({"transaction": {"amount": "10"}}, "transaction.amount") | ||
| == "10" | ||
| ) | ||
| assert resolve_fact_path({"transaction": ["10"]}, "transaction.0") is not None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the missing sentinel, not merely a non-None value.
The current assertion also passes if list traversal incorrectly returns "10". Compare against a known missing-path result instead.
Proposed fix
def test_resolve_fact_path_only_traverses_mappings() -> None:
+ missing = resolve_fact_path({"transaction": {}}, "transaction.amount")
assert (
resolve_fact_path({"transaction": {"amount": "10"}}, "transaction.amount")
== "10"
)
- assert resolve_fact_path({"transaction": ["10"]}, "transaction.0") is not None
+ assert resolve_fact_path({"transaction": ["10"]}, "transaction.0") is missing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_resolve_fact_path_only_traverses_mappings() -> None: | |
| assert ( | |
| resolve_fact_path({"transaction": {"amount": "10"}}, "transaction.amount") | |
| == "10" | |
| ) | |
| assert resolve_fact_path({"transaction": ["10"]}, "transaction.0") is not None | |
| def test_resolve_fact_path_only_traverses_mappings() -> None: | |
| missing = resolve_fact_path({"transaction": {}}, "transaction.amount") | |
| assert ( | |
| resolve_fact_path({"transaction": {"amount": "10"}}, "transaction.amount") | |
| == "10" | |
| ) | |
| assert resolve_fact_path({"transaction": ["10"]}, "transaction.0") is missing |
🤖 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 `@tests/unit/test_automation.py` around lines 219 - 224, Update
test_resolve_fact_path_only_traverses_mappings so the list-based path assertion
compares the result of resolve_fact_path({"transaction": ["10"]},
"transaction.0") with the known missing-path sentinel, rather than only checking
that it is not None. Keep the mapping traversal assertion unchanged.
Comment on lines
+112
to
+127
| def test_register_project_channel_is_idempotent_for_same_project(monkeypatch) -> None: | ||
| cursor = _CursorStub([None, _mapping(), _mapping()]) | ||
| _install_connection_stub(monkeypatch, cursor) | ||
|
|
||
| mapping, created = project_channels.register_project_discord_channel( | ||
| project_channels.SharedSettings(), | ||
| project_id="project-1", | ||
| guild_id="guild-1", | ||
| channel_id="channel-1", | ||
| channel_name="renamed-private-project", | ||
| registered_by_discord_user_id="user-2", | ||
| ) | ||
|
|
||
| assert created is False | ||
| assert mapping.channel_name == "private-project" | ||
| assert "UPDATE project_discord_channels" in cursor.executed[2][0] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expect the refreshed channel metadata.
The stub returns the old mapping after UPDATE, so this test would accept an ignored rename. Return updated metadata and verify the bound values.
Proposed fix
def test_register_project_channel_is_idempotent_for_same_project(monkeypatch) -> None:
- cursor = _CursorStub([None, _mapping(), _mapping()])
+ updated_mapping = _mapping()
+ updated_mapping["channel_name"] = "renamed-private-project"
+ updated_mapping["registered_by_discord_user_id"] = "user-2"
+ cursor = _CursorStub([None, _mapping(), updated_mapping])
...
- assert mapping.channel_name == "private-project"
+ assert mapping.channel_name == "renamed-private-project"
assert "UPDATE project_discord_channels" in cursor.executed[2][0]
+ assert cursor.executed[2][1][:2] == ("renamed-private-project", "user-2")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_register_project_channel_is_idempotent_for_same_project(monkeypatch) -> None: | |
| cursor = _CursorStub([None, _mapping(), _mapping()]) | |
| _install_connection_stub(monkeypatch, cursor) | |
| mapping, created = project_channels.register_project_discord_channel( | |
| project_channels.SharedSettings(), | |
| project_id="project-1", | |
| guild_id="guild-1", | |
| channel_id="channel-1", | |
| channel_name="renamed-private-project", | |
| registered_by_discord_user_id="user-2", | |
| ) | |
| assert created is False | |
| assert mapping.channel_name == "private-project" | |
| assert "UPDATE project_discord_channels" in cursor.executed[2][0] | |
| def test_register_project_channel_is_idempotent_for_same_project(monkeypatch) -> None: | |
| updated_mapping = _mapping() | |
| updated_mapping["channel_name"] = "renamed-private-project" | |
| updated_mapping["registered_by_discord_user_id"] = "user-2" | |
| cursor = _CursorStub([None, _mapping(), updated_mapping]) | |
| _install_connection_stub(monkeypatch, cursor) | |
| mapping, created = project_channels.register_project_discord_channel( | |
| project_channels.SharedSettings(), | |
| project_id="project-1", | |
| guild_id="guild-1", | |
| channel_id="channel-1", | |
| channel_name="renamed-private-project", | |
| registered_by_discord_user_id="user-2", | |
| ) | |
| assert created is False | |
| assert mapping.channel_name == "renamed-private-project" | |
| assert "UPDATE project_discord_channels" in cursor.executed[2][0] | |
| assert cursor.executed[2][1][:2] == ("renamed-private-project", "user-2") |
🤖 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 `@tests/unit/test_project_discord_channels.py` around lines 112 - 127, Update
test_register_project_channel_is_idempotent_for_same_project and its _CursorStub
setup so the UPDATE operation returns refreshed channel metadata reflecting the
renamed channel, rather than the stale mapping. Assert the returned mapping
contains the new channel_name and verify cursor.executed[2] includes the
expected bound update values.
Comment on lines
+102
to
+138
| def test_approved_receipt_teaches_only_a_deterministic_review_suggestion() -> None: | ||
| snapshot = { | ||
| "direction": "inbound", | ||
| "currency": "gbp", | ||
| "counterparty": " Acme Ltd ", | ||
| "reference_number": "WIRE-123", | ||
| "description": "Invoice payment", | ||
| } | ||
|
|
||
| rule = learned_project_payment_suggestion_rule( | ||
| project_id="00000000-0000-0000-0000-000000000001", | ||
| subject_snapshot=snapshot, | ||
| ) | ||
| duplicate = learned_project_payment_suggestion_rule( | ||
| project_id="00000000-0000-0000-0000-000000000001", | ||
| subject_snapshot=snapshot, | ||
| ) | ||
|
|
||
| assert rule is not None | ||
| assert duplicate == rule | ||
| assert rule.origin is AutomationRuleOrigin.LEARNED | ||
| assert rule.mode is AutomationRuleMode.SUGGEST | ||
| assert rule.priority == -100 | ||
| assert rule.learning_key is not None | ||
| assert "Acme" not in rule.learning_key | ||
| assert [condition.model_dump(mode="json") for condition in rule.conditions] == [ | ||
| {"fact": "transaction.direction", "operator": "equals", "value": "inbound"}, | ||
| { | ||
| "fact": "transaction.counterparty", | ||
| "operator": "contains", | ||
| "value": "Acme Ltd", | ||
| }, | ||
| {"fact": "transaction.currency", "operator": "equals", "value": "GBP"}, | ||
| ] | ||
| assert rule.actions[0].payload == { | ||
| "project_id": "00000000-0000-0000-0000-000000000001" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Make the PII-leakage assertion case-insensitive.
"Acme" not in learning_key still passes if normalized payer text such as "acme ltd" leaks into the identifier.
Proposed fix
assert rule.priority == -100
assert rule.learning_key is not None
- assert "Acme" not in rule.learning_key
+ normalized_learning_key = rule.learning_key.casefold()
+ assert "acme" not in normalized_learning_key
+ assert "ltd" not in normalized_learning_key📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_approved_receipt_teaches_only_a_deterministic_review_suggestion() -> None: | |
| snapshot = { | |
| "direction": "inbound", | |
| "currency": "gbp", | |
| "counterparty": " Acme Ltd ", | |
| "reference_number": "WIRE-123", | |
| "description": "Invoice payment", | |
| } | |
| rule = learned_project_payment_suggestion_rule( | |
| project_id="00000000-0000-0000-0000-000000000001", | |
| subject_snapshot=snapshot, | |
| ) | |
| duplicate = learned_project_payment_suggestion_rule( | |
| project_id="00000000-0000-0000-0000-000000000001", | |
| subject_snapshot=snapshot, | |
| ) | |
| assert rule is not None | |
| assert duplicate == rule | |
| assert rule.origin is AutomationRuleOrigin.LEARNED | |
| assert rule.mode is AutomationRuleMode.SUGGEST | |
| assert rule.priority == -100 | |
| assert rule.learning_key is not None | |
| assert "Acme" not in rule.learning_key | |
| assert [condition.model_dump(mode="json") for condition in rule.conditions] == [ | |
| {"fact": "transaction.direction", "operator": "equals", "value": "inbound"}, | |
| { | |
| "fact": "transaction.counterparty", | |
| "operator": "contains", | |
| "value": "Acme Ltd", | |
| }, | |
| {"fact": "transaction.currency", "operator": "equals", "value": "GBP"}, | |
| ] | |
| assert rule.actions[0].payload == { | |
| "project_id": "00000000-0000-0000-0000-000000000001" | |
| } | |
| def test_approved_receipt_teaches_only_a_deterministic_review_suggestion() -> None: | |
| snapshot = { | |
| "direction": "inbound", | |
| "currency": "gbp", | |
| "counterparty": " Acme Ltd ", | |
| "reference_number": "WIRE-123", | |
| "description": "Invoice payment", | |
| } | |
| rule = learned_project_payment_suggestion_rule( | |
| project_id="00000000-0000-0000-0000-000000000001", | |
| subject_snapshot=snapshot, | |
| ) | |
| duplicate = learned_project_payment_suggestion_rule( | |
| project_id="00000000-0000-0000-0000-000000000001", | |
| subject_snapshot=snapshot, | |
| ) | |
| assert rule is not None | |
| assert duplicate == rule | |
| assert rule.origin is AutomationRuleOrigin.LEARNED | |
| assert rule.mode is AutomationRuleMode.SUGGEST | |
| assert rule.priority == -100 | |
| assert rule.learning_key is not None | |
| normalized_learning_key = rule.learning_key.casefold() | |
| assert "acme" not in normalized_learning_key | |
| assert "ltd" not in normalized_learning_key | |
| assert [condition.model_dump(mode="json") for condition in rule.conditions] == [ | |
| {"fact": "transaction.direction", "operator": "equals", "value": "inbound"}, | |
| { | |
| "fact": "transaction.counterparty", | |
| "operator": "contains", | |
| "value": "Acme Ltd", | |
| }, | |
| {"fact": "transaction.currency", "operator": "equals", "value": "GBP"}, | |
| ] | |
| assert rule.actions[0].payload == { | |
| "project_id": "00000000-0000-0000-0000-000000000001" | |
| } |
🤖 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 `@tests/unit/test_project_payments.py` around lines 102 - 138, The PII-leakage
assertion in
test_approved_receipt_teaches_only_a_deterministic_review_suggestion must be
case-insensitive. Update the learning_key check to compare a normalized
lowercase form against the lowercase payer name, while preserving the existing
deterministic rule assertions.
Comment on lines
+151
to
+190
| def test_stale_erpnext_project_payload_cannot_overwrite_newer_cache_or_roster() -> None: | ||
| """A delayed Open fetch must not reopen an action-time Closed refresh.""" | ||
| project_id = "00000000-0000-0000-0000-000000000001" | ||
| cursor = Mock() | ||
| cursor.fetchone.side_effect = [ | ||
| {"project_id": project_id}, | ||
| None, | ||
| ] | ||
| connection = Mock() | ||
| connection.__enter__ = Mock(return_value=connection) | ||
| connection.__exit__ = Mock(return_value=None) | ||
| cursor_context = Mock() | ||
| cursor_context.__enter__ = Mock(return_value=cursor) | ||
| cursor_context.__exit__ = Mock(return_value=None) | ||
| connection.cursor.return_value = cursor_context | ||
| stale_open = ProjectInput( | ||
| source="erpnext", | ||
| external_id="PROJ-001", | ||
| display_name="Project", | ||
| source_status="Open", | ||
| source_modified_at=datetime(2026, 7, 16, tzinfo=timezone.utc), | ||
| ) | ||
|
|
||
| with patch("five08.projects.get_postgres_connection", return_value=connection): | ||
| result = upsert_project(SharedSettings(), stale_open) | ||
|
|
||
| assert result == project_id | ||
| update_query, update_params = cursor.execute.call_args_list[2].args | ||
| assert "OR %s > projects.source_modified_at" in update_query | ||
| assert update_params[-2:] == ( | ||
| stale_open.source_modified_at, | ||
| stale_open.source_modified_at, | ||
| ) | ||
| executed_queries = [call.args[0] for call in cursor.execute.call_args_list] | ||
| assert not any( | ||
| "INSERT INTO project_roster_members" in query for query in executed_queries | ||
| ) | ||
| assert not any( | ||
| "INSERT INTO project_external_ids" in query for query in executed_queries | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Ensure stale payloads cannot execute any roster mutation.
The assertion only excludes roster inserts; an accidental roster deletion would still pass despite violating the stale-revision fence.
Proposed fix
executed_queries = [call.args[0] for call in cursor.execute.call_args_list]
assert not any(
- "INSERT INTO project_roster_members" in query for query in executed_queries
+ "project_roster_members" in query for query in executed_queries
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_stale_erpnext_project_payload_cannot_overwrite_newer_cache_or_roster() -> None: | |
| """A delayed Open fetch must not reopen an action-time Closed refresh.""" | |
| project_id = "00000000-0000-0000-0000-000000000001" | |
| cursor = Mock() | |
| cursor.fetchone.side_effect = [ | |
| {"project_id": project_id}, | |
| None, | |
| ] | |
| connection = Mock() | |
| connection.__enter__ = Mock(return_value=connection) | |
| connection.__exit__ = Mock(return_value=None) | |
| cursor_context = Mock() | |
| cursor_context.__enter__ = Mock(return_value=cursor) | |
| cursor_context.__exit__ = Mock(return_value=None) | |
| connection.cursor.return_value = cursor_context | |
| stale_open = ProjectInput( | |
| source="erpnext", | |
| external_id="PROJ-001", | |
| display_name="Project", | |
| source_status="Open", | |
| source_modified_at=datetime(2026, 7, 16, tzinfo=timezone.utc), | |
| ) | |
| with patch("five08.projects.get_postgres_connection", return_value=connection): | |
| result = upsert_project(SharedSettings(), stale_open) | |
| assert result == project_id | |
| update_query, update_params = cursor.execute.call_args_list[2].args | |
| assert "OR %s > projects.source_modified_at" in update_query | |
| assert update_params[-2:] == ( | |
| stale_open.source_modified_at, | |
| stale_open.source_modified_at, | |
| ) | |
| executed_queries = [call.args[0] for call in cursor.execute.call_args_list] | |
| assert not any( | |
| "INSERT INTO project_roster_members" in query for query in executed_queries | |
| ) | |
| assert not any( | |
| "INSERT INTO project_external_ids" in query for query in executed_queries | |
| ) | |
| def test_stale_erpnext_project_payload_cannot_overwrite_newer_cache_or_roster() -> None: | |
| """A delayed Open fetch must not reopen an action-time Closed refresh.""" | |
| project_id = "00000000-0000-0000-0000-000000000001" | |
| cursor = Mock() | |
| cursor.fetchone.side_effect = [ | |
| {"project_id": project_id}, | |
| None, | |
| ] | |
| connection = Mock() | |
| connection.__enter__ = Mock(return_value=connection) | |
| connection.__exit__ = Mock(return_value=None) | |
| cursor_context = Mock() | |
| cursor_context.__enter__ = Mock(return_value=cursor) | |
| cursor_context.__exit__ = Mock(return_value=None) | |
| connection.cursor.return_value = cursor_context | |
| stale_open = ProjectInput( | |
| source="erpnext", | |
| external_id="PROJ-001", | |
| display_name="Project", | |
| source_status="Open", | |
| source_modified_at=datetime(2026, 7, 16, tzinfo=timezone.utc), | |
| ) | |
| with patch("five08.projects.get_postgres_connection", return_value=connection): | |
| result = upsert_project(SharedSettings(), stale_open) | |
| assert result == project_id | |
| update_query, update_params = cursor.execute.call_args_list[2].args | |
| assert "OR %s > projects.source_modified_at" in update_query | |
| assert update_params[-2:] == ( | |
| stale_open.source_modified_at, | |
| stale_open.source_modified_at, | |
| ) | |
| executed_queries = [call.args[0] for call in cursor.execute.call_args_list] | |
| assert not any( | |
| "project_roster_members" in query for query in executed_queries | |
| ) | |
| assert not any( | |
| "INSERT INTO project_external_ids" in query for query in executed_queries | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 173-173: no timeout was given on call to external resource
Context: patch("five08.projects.get_postgres_connection", return_value=connection)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
🤖 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 `@tests/unit/test_projects.py` around lines 151 - 190, Strengthen
test_stale_erpnext_project_payload_cannot_overwrite_newer_cache_or_roster so
stale payloads are verified to perform no roster mutation at all. Inspect the
executed queries and assert that neither roster inserts nor DELETE/other roster
mutation statements are present, while preserving the existing stale revision
and external-ID assertions.
![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: a5414e46e9
ℹ️ 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".
| AND %s IS NOT NULL | ||
| AND ( | ||
| projects.source_modified_at IS NULL | ||
| OR %s > projects.source_modified_at |
There was a problem hiding this comment.
When the open-project sync temporarily omits a still-open ERPNext project, mark_missing_erpnext_open_projects_not_open() now marks it Not Open; the next successful sync often reads the same ERP modified timestamp because the project itself did not change. This strict > then rejects that otherwise-current payload and returns before updating status or roster, leaving the project hidden/closed locally until ERPNext changes it again. Same-revision payloads should still be able to repair local state created by the missing-project sweep.
Useful? React with 👍 / 👎.
| if not lease_renewed: | ||
| return {"error": "payment_notification_delivery_lease_lost"}, 503 | ||
| try: | ||
| async for message in channel.history(limit=100, oldest_first=False): |
There was a problem hiding this comment.
If the bot successfully sends a payment notification but crashes or loses the DB write before mark_project_payment_discord_delivery_sent, this history scan is the only recovery path before sending again. Limiting it to the latest 100 messages means a busy private project channel can push the marker out of the scanned window before the stale lease is retried, causing a duplicate payment receipt to be posted even though the original marker is still in the channel history.
Useful? React with 👍 / 👎.
| discord_nonce = ( | ||
| "pp-" + hashlib.sha256(notification_id.encode("utf-8")).hexdigest()[:22] | ||
| ) | ||
| message = await channel.send( |
There was a problem hiding this comment.
When a registered project channel is made public after the earlier _private_channel_error() check but before this write, the final pre-send checks only refresh the DB context and delivery lease, so the payment receipt can still be posted to a channel that is no longer private. The history scan and DB calls make that window real for the safety case this endpoint is trying to enforce, so rerun the live privacy check immediately before channel.send.
Useful? React with 👍 / 👎.
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
2 participants
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
Adds an ERPNext-first workflow that classifies canonical bank transactions against durable project-payment automation rules, associates private Discord channels with active ERP projects, and notifies the mapped channel when a payment is recognized.
It includes a reusable typed rule engine, durable evaluation/action/outbox state, signed ERPNext transaction ingest and canonical rechecks, Discord registration commands, configuration, migrations, and a Payments dashboard for rule and suggestion review.
Approved human suggestions can create deterministic learned suggestion rules. Learned rules remain suggestion-only and can never autonomously apply a payment.
Why
Keep ERPNext as the accounting and Plaid-sync source of truth while adding project-level workflow automation without a second Plaid credential or transaction-ingestion pipeline.
Validation
./scripts/test.sh— 2,063 passed, 21 skipped; dashboard 18 passed./scripts/lint.sh./scripts/pyrefly.sh— 0 errorsgit diff --check origin/main...Note
High Risk
Touches financial routing, signed webhook auth, automatic allocations, and multi-process Discord delivery with complex failure/recovery paths—errors could mis-route payments or duplicate notifications.
Overview
Introduces ERP-first project payment automation: a dedicated signed webhook at
/webhooks/erpnext/bank-transactionqueues canonical bank-transaction ingest (identity only; worker fetches ERP data), with idempotent enqueue, redelivery for duplicates, and revival of dead ingest jobs.The API gains dashboard endpoints for listing/creating/updating/disabling
project_paymentrouting rules and for approving or rejecting payment suggestions (with audit events, open-project checks, strict automatic-rule validation, and optional learned suggestion-only rules after approval). A periodic recovery scheduler runs whenPROJECT_PAYMENT_AUTOMATION_ENABLEDis on.The admin dashboard adds a Payments section (
/dashboard/payments) to manage rules, review suggestions with confirmation, and filter by project. Config documentsERPNEXT_BANK_TRANSACTION_WEBHOOK_SIGNING_SECRET(no fallback to generic webhook secrets) and notes runtime-managed payment flags; ARCHITECTURE.md describes the event/outbox model, Discord notifications, revision fencing, and supersession behavior.Reviewed by Cursor Bugbot for commit a5414e4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation