light weight scheduler and outbox sweeper - #21
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
Note
|
| Layer / File(s) | Summary |
|---|---|
Data contracts, routing, and outbox separation libs/database/..., libs/domain/..., libs/pipeline/..., services/api/... |
Database, domain, repository, routing, transaction, and outbox contracts now use trading_partner_id and distinct control-plane/data-plane outbox repositories. |
Scheduler domain and worker service libs/scheduler/..., libs/database/..., services/workers/orchestrator/... |
Scheduled jobs gain persisted lifecycle, locking, retry, interval, and cron behavior with an asynchronous worker service. |
Worker orchestration and queue processing services/workers/orchestrator/..., docker/localstack/init-aws.sh, Makefile |
SQS polling, publishing, tenant resolution, pipeline and delivery handlers, scheduled-job dispatch, outbox sweeping, retention cleanup, and queue DLQs are wired together. |
AS2 certificate lifecycle and delivery services/api/src/api/routers/trading_partners/platform/as2_partners.py, frontend/web/src/features/partners/..., libs/pipeline/src/pipeline/... |
Certificate generation/deletion, Vault references, certificate display, SSRF-safe delivery validation, and idempotency-key propagation are added. |
Platform scheduler API and frontend services/api/src/api/routers/platform/..., frontend/web/src/features/platform/..., frontend/web/src/routes/platform/... |
Platform-admin scheduler and configuration endpoints, authenticated API hooks, job dashboard, cron editor, and scheduler navigation are added. |
Validation and supporting updates services/api/tests/..., services/workers/orchestrator/tests/..., services/as2_server/scripts/seed.py, TECHNICAL_DEBT.md |
Tests, system-job seeding, worker shutdown handling, platform settings naming, AWS settings, and technical-debt documentation are updated. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Possibly related PRs
- pramodnarayana/soopaedi#2: Overlaps on LocalStack SQS setup and CDC relay queue routing.
- pramodnarayana/soopaedi#4: Relates to control-plane outbox model and repository wiring.
- pramodnarayana/soopaedi#9: Relates to AS2 partner and certificate management flows.
Poem
A rabbit hops through queues of light,
With schedulers ticking through the night.
Certificates bloom, routes softly align,
Outboxes sort each event in line.
“Hop hop!” says Bunny, “the workers are bright!”
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 19.91% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title matches the main change set: adding a lightweight scheduler and an outbox sweeper. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
outbox-sweeper
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 @coderabbitai help to get the list of available commands.
pramodnarayana
force-pushed
the
outbox-sweeper
branch
from
July 16, 2026 17:23
72721e1 to
f37d29d
Compare
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/database/src/database/migrations/tenant/versions/f966e8446341_tenant_initial_schema.py (1)
386-410: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid modifying existing migration files.
Modifying an existing initial migration file (
f966e8446341_tenant_initial_schema.py) rather than creating a new migration script will cause schema drift for any existing databases that have already applied this migration.Alembic will not re-run this file, leaving existing databases without the
trading_partner_idcolumn (while keeping the oldoutbound_route_idcolumn) and causing runtime errors. Please revert the changes in this file and create a new Alembic revision to apply these schema updates.🤖 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 `@libs/database/src/database/migrations/tenant/versions/f966e8446341_tenant_initial_schema.py` around lines 386 - 410, Revert the schema changes in the existing revision f966e8446341_tenant_initial_schema.py, including the trading_partner_id/outbound_route_id adjustment. Create a new Alembic revision that migrates existing tenant databases by renaming or replacing the column as required, while updating the downgrade path to restore the prior schema.
🤖 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 `@frontend/web/src/features/partners/components/CreatePartnerModal.tsx`:
- Around line 187-190: Update the certificate-generation state in
CreatePartnerModal so it records the AS2 ID used by generateCert.mutate and
invalidates or regenerates setCertPem and setPrivateKeyVaultRef when the current
AS2 ID differs from that recorded value, before submission. Preserve the
existing credentials when the AS2 ID is unchanged.
- Around line 27-30: Update the CreatePartnerModal reset, mode-change, and
key-generation flows to track the currently generated private-key Vault
reference and delete that secret before replacing it or abandoning it. Add the
necessary draft cleanup lifecycle around setPrivateKeyVaultRef and ensure
cleanup is awaited or safely handled before state is cleared, preventing
orphaned credentials.
- Around line 103-107: Update the Remote branch in CreatePartnerModal so
switching away from local delivery clears the locally generated certificate and
private-key reference along with the URL state. Ensure the reset occurs before
submission so remote partners do not retain platform-controlled local
credentials, while preserving the existing available_as2_receive_urls check.
In `@frontend/web/src/features/partners/types.ts`:
- Line 71: The partner creation payload must use the backend field
private_key_vault_ref consistently. In
frontend/web/src/features/partners/types.ts:71-71, rename vault_key_ref to
private_key_vault_ref; in
frontend/web/src/features/partners/components/CreatePartnerModal.tsx:63-64, send
private_key_vault_ref: privateKeyVaultRef || undefined.
In
`@libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py`:
- Around line 342-370: Update the migration’s downgrade() function to remove the
schema created by upgrade(): drop scheduled_jobs indexes in reverse creation
order, then drop scheduled_jobs and platform_settings. Use the existing index
symbols ix_scheduled_jobs_status, ix_scheduled_jobs_next_run_at, and
ix_scheduled_jobs_name, with each index associated with scheduled_jobs.
In `@libs/pipeline/src/pipeline/core/transformation/outbound.py`:
- Around line 35-50: Initialize route_config and outbound_route to None before
the trading_partner_id conditional in the outbound transformation flow. Preserve
the existing repository lookups when trading_partner_id is present, ensuring
both variables remain safely defined when the identifier is missing.
In `@services/api/src/api/adapters/outbox_repository.py`:
- Around line 55-73: Extract the duplicated publish_outbox_event implementation
into a shared SqlAlchemyOutboxRepositoryMixin and have both outbox repository
classes reuse it. Preserve the existing event creation, idempotency-key
fallback, pending status, flush, and returned event_id behavior while removing
the duplicate method definitions.
In `@services/api/src/api/adapters/transaction_repository.py`:
- Line 279: Update _apply_dynamic_filters so every trading_partner_id operator
branch (eq, neq, contains, and in) includes model.trading_partner_id alongside
the existing sender and receiver conditions, guarded by attribute availability
where appropriate, ensuring explorer filters query the actual column.
In `@services/api/src/api/main.py`:
- Around line 32-34: Update the platform router import in the main API module to
use the existing api.routers.trading_partners.platform module path instead of
api.routers.platform, preserving the platform_admin alias.
In `@services/api/src/api/routers/trading_partners/platform/as2_partners.py`:
- Around line 75-76: In the partner creation flow around the self-signed
certificate condition, introduce an auto_generated flag initialized as false and
set it only when this request creates a new private-key vault secret. Update the
IntegrityError rollback cleanup to delete the secret only when auto_generated is
true, preserving externally provided private_key_vault_ref values.
In `@services/api/src/api/services/as2_receiver_service.py`:
- Around line 389-394: Update the data-plane event publishing flow in the
surrounding receiver-service method to instantiate and use
SqlAlchemyDataPlaneOutboxRepository, importing it from
api.adapters.outbox_repository, instead of calling publish_outbox_event on
SqlAlchemyTransactionRepository. Preserve the existing tenant_id,
TRANSFORM_EVENT, payload, and idempotency_key values.
---
Outside diff comments:
In
`@libs/database/src/database/migrations/tenant/versions/f966e8446341_tenant_initial_schema.py`:
- Around line 386-410: Revert the schema changes in the existing revision
f966e8446341_tenant_initial_schema.py, including the
trading_partner_id/outbound_route_id adjustment. Create a new Alembic revision
that migrates existing tenant databases by renaming or replacing the column as
required, while updating the downgrade path to restore the prior schema.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40795ff9-c8e9-451e-8546-1c3352f34e13
⛔ Files ignored due to path filters (1)
frontend/web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (48)
MakefileTECHNICAL_DEBT.mddocker/localstack/init-aws.shfrontend/web/package.jsonfrontend/web/src/features/partners/api/IPartnersRepository.tsfrontend/web/src/features/partners/api/partnerHooks.tsfrontend/web/src/features/partners/api/partnersApi.tsfrontend/web/src/features/partners/components/As2PartnerDetails.tsxfrontend/web/src/features/partners/components/CreatePartnerModal.tsxfrontend/web/src/features/partners/components/CreatePartnershipModal.tsxfrontend/web/src/features/partners/components/PartnershipDetails.tsxfrontend/web/src/features/partners/types.tsfrontend/web/src/features/platform/api/configHooks.tsfrontend/web/src/routeTree.gen.tsfrontend/web/src/routes/platform.tsxlibs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.pylibs/database/src/database/migrations/tenant/versions/f966e8446341_tenant_initial_schema.pylibs/database/src/database/models/__init__.pylibs/database/src/database/models/control_plane.pylibs/database/src/database/models/data_plane.pylibs/domain/src/domain/models.pylibs/pipeline/src/pipeline/adapters/repository.pylibs/pipeline/src/pipeline/core/delivery/router.pylibs/pipeline/src/pipeline/core/saga.pylibs/pipeline/src/pipeline/core/transformation/inbound.pylibs/pipeline/src/pipeline/core/transformation/outbound.pylibs/pipeline/src/pipeline/ports/repository.pypyproject.tomlservices/api/src/api/adapters/api_token_repository.pyservices/api/src/api/adapters/http/dtos.pyservices/api/src/api/adapters/outbox_repository.pyservices/api/src/api/adapters/transaction_repository.pyservices/api/src/api/cdc_relay.pyservices/api/src/api/core/services/as2_partner_service.pyservices/api/src/api/core/services/as2_partnership_service.pyservices/api/src/api/core/services/inbound_route_service.pyservices/api/src/api/core/services/outbound_route_service.pyservices/api/src/api/core/services/routing_resolver.pyservices/api/src/api/core/services/sftp_partner_service.pyservices/api/src/api/core/services/webhook_service.pyservices/api/src/api/core/uow.pyservices/api/src/api/domain/models.pyservices/api/src/api/main.pyservices/api/src/api/routers/trading_partners/platform/__init__.pyservices/api/src/api/routers/trading_partners/platform/as2_partners.pyservices/api/src/api/routers/trading_partners/platform/config.pyservices/api/src/api/services/api_receiver_service.pyservices/api/src/api/services/as2_receiver_service.py
💤 Files with no reviewable changes (2)
- frontend/web/src/features/platform/api/configHooks.ts
- services/api/src/api/routers/trading_partners/platform/config.py
Comment on lines
+32
to
+34
| from api.routers import ( | ||
| platform as platform_admin, | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if api/routers/platform/__init__.py exists and exports a router
fd __init__.py services/api/src/api/routers -x cat {} | grep -H "router = APIRouter("
ls -la services/api/src/api/routers/platform/__init__.py || echo "api.routers.platform does not exist"Repository: pramodnarayana/soopaedi
Length of output: 383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## api/routers package files"
fd -a '^__init__\.py$' services/api/src/api/routers
echo
echo "## api/routers/__init__.py"
cat -n services/api/src/api/routers/__init__.py
echo
echo "## platform-related router files"
fd -a 'platform' services/api/src/api/routers
echo
echo "## search for re-exports/imports of platform"
rg -n "from .* import platform|import platform|platform_admin|routers\.platform|routers\.trading_partners\.platform" services/api/src/api/routers services/api/src/api/main.pyRepository: pramodnarayana/soopaedi
Length of output: 1412
Import the router from the actual module path. api.routers.platform doesn’t exist, so this import will fail at startup; point it at api.routers.trading_partners.platform instead.
🧰 Tools
🪛 GitHub Actions: CI / 0_Build, Lint, and Test.txt
[error] 32-32: mypy: Module "api.routers" has no attribute "platform" [attr-defined]
🪛 GitHub Actions: CI / Build, Lint, and Test
[error] 32-32: mypy attr-defined: Module "api.routers" has no attribute "platform".
🤖 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 `@services/api/src/api/main.py` around lines 32 - 34, Update the platform
router import in the main API module to use the existing
api.routers.trading_partners.platform module path instead of
api.routers.platform, preserving the platform_admin alias.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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 (1)
libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py (1)
366-370: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce the scheduled-job singleton invariant in storage and writes. The API assumes exactly one row for each job name, but neither the schema nor the write flow guarantees it.
libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py#L366-L370: make thescheduled_jobs.nameindex unique.services/api/src/api/routers/platform/scheduler.py#L85-L112: replace the check-then-insert with an atomic upsert or handle uniqueness conflicts.services/api/src/api/routers/platform/scheduler.py#L118-L124: retain singleton-safe lookup after the database invariant is established.🤖 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 `@libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py` around lines 366 - 370, Enforce one scheduled job per name across storage and writes: in libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py:366-370, make the scheduled_jobs.name index unique; in services/api/src/api/routers/platform/scheduler.py:85-112, replace the check-then-insert flow with an atomic upsert or uniqueness-conflict handling; in services/api/src/api/routers/platform/scheduler.py:118-124, retain the singleton-safe lookup after applying the database invariant.
🤖 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 `@frontend/web/src/features/partners/api/partnerHooks.ts`:
- Around line 139-145: Update useDeleteCertificateSecretMutation to use the
standard useMutation hook instead of useToastMutation, preserving the existing
repository deletion callback while removing automatic success-toast behavior for
background cleanup.
In `@frontend/web/src/features/platform/components/SchedulerDashboard.tsx`:
- Around line 111-115: Update the Refresh button’s onClick handler in
SchedulerDashboard to trigger refetching for both queries used by the component,
using their returned refetch functions or invalidating both query keys. Preserve
the existing button styling and label.
- Around line 14-32: Update the SchedulerDashboard state-sync useEffect to
derive sweeper values inline or through memoized values, and make its dependency
list depend only on configs. Prevent getSweeperEnabled and getSweeperInterval
from triggering synchronization on every render so localEnabled and
localInterval remain editable after user changes.
In `@libs/database/src/database/models/scheduled_job.py`:
- Around line 29-32: Update the scheduled job timestamp defaults in the
created_at and updated_at mapped columns to use a timezone-aware UTC datetime
callable instead of datetime.utcnow, and add the required timezone import while
preserving the existing DateTime(timezone=True) configuration and updated_at
onupdate behavior.
In `@libs/scheduler/src/scheduler/adapters/repository.py`:
- Line 48: Update the query ordering around ScheduledJob.created_at so
ScheduledJob.next_run_at is the primary sort key, placing NULL next_run_at
values first for immediate execution, and retain created_at ascending as the
tie-breaker.
In `@libs/scheduler/src/scheduler/core/service.py`:
- Around line 58-60: Update the exception path in the scheduler service around
repository.mark_failed to evaluate ScheduledJob.retry_count against max_retries
before immediately marking the job failed. When retries remain, increment the
retry count and reschedule the job using the existing scheduling flow; otherwise
preserve the terminal failure behavior. If retry handling cannot be implemented
in this change, add a clear TODO at this location documenting the missing retry
and rescheduling logic.
In `@services/api/src/api/routers/platform/__init__.py`:
- Around line 5-6: Update the platform router setup around scheduler_router to
enforce platform-admin authorization by adding the existing
require_platform_admin dependency to the router or an equivalent parent-level
guard. Ensure all scheduler job and platform-configuration mutation endpoints
inherit this dependency when included.
In `@services/api/src/api/routers/platform/scheduler.py`:
- Around line 90-104: Enforce a single positive-integer interval contract across
all affected sites: in services/api/src/api/routers/platform/scheduler.py lines
90-104, require request.value to be a real boolean and validate the new
ScheduledJob interval as a positive integer; in lines 117-124, reject invalid
interval updates with HTTP 422. In
frontend/web/src/features/platform/components/SchedulerDashboard.tsx lines
34-47, block saving unless the parsed interval is a positive integer, and in
lines 98-106 add matching positive-integer input constraints. In
services/workers/orchestrator/src/worker/jobs/outbox_sweeper.py lines 74-75,
defensively validate persisted payload intervals before rescheduling.
In `@services/api/src/api/routers/trading_partners/platform/as2_partners.py`:
- Around line 56-65: Update delete_certificate_secret to authorize deletion of
vault_ref before calling vault.delete_secret: query the database for any
existing trading partner or active certificate referencing the secret, reject
the request when it is owned or in use, and only delete when the reference is
confirmed orphaned. Reuse the endpoint’s existing dependency and error-handling
conventions for database access and authorization.
- Around line 130-133: Update the exception handling around the AS2 partner
commit flow to catch Exception, ensuring an auto-generated secret identified by
private_key_vault_ref is deleted on every commit failure when auto_generated is
true. Preserve the existing IntegrityError response for duplicate AS2 IDs while
re-raising other exceptions after cleanup.
In `@services/workers/orchestrator/src/worker/jobs/outbox_sweeper.py`:
- Around line 61-68: Update _bounded_sweep to catch and log exceptions from
_sweep_shard, returning zero for the failed shard so asyncio.gather continues
processing other shards. Keep semaphore handling intact and ensure exceptions
are handled before they can propagate and disrupt the shared SQS client or
scheduled execution.
- Around line 34-39: Update DataPlaneOutboxSweeperJobHandler.__init__ and the
corresponding SQS adapter configuration so AWS_ENDPOINT_URL remains unset when
absent instead of defaulting to LocalStack, and derive the region from
AWS_REGION or AWS_DEFAULT_REGION rather than hardcoding us-east-1.
---
Outside diff comments:
In
`@libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py`:
- Around line 366-370: Enforce one scheduled job per name across storage and
writes: in
libs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.py:366-370,
make the scheduled_jobs.name index unique; in
services/api/src/api/routers/platform/scheduler.py:85-112, replace the
check-then-insert flow with an atomic upsert or uniqueness-conflict handling; in
services/api/src/api/routers/platform/scheduler.py:118-124, retain the
singleton-safe lookup after applying the database invariant.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c92f817a-88fd-4e7c-b8e8-ecc339c5297c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
frontend/web/src/features/partners/api/IPartnersRepository.tsfrontend/web/src/features/partners/api/partnerHooks.tsfrontend/web/src/features/partners/api/partnersApi.tsfrontend/web/src/features/partners/components/As2PartnerDetails.tsxfrontend/web/src/features/partners/components/As2PartnersTable.tsxfrontend/web/src/features/partners/components/CreatePartnerModal.tsxfrontend/web/src/features/partners/types.tsfrontend/web/src/features/platform/api/schedulerApi.tsfrontend/web/src/features/platform/api/schedulerHooks.tsfrontend/web/src/features/platform/api/settingsHooks.tsfrontend/web/src/features/platform/components/SchedulerDashboard.tsxfrontend/web/src/routes/platform/scheduler.tsxlibs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.pylibs/database/src/database/models/platform_settings.pylibs/database/src/database/models/scheduled_job.pylibs/pipeline/src/pipeline/core/transformation/outbound.pylibs/scheduler/README.mdlibs/scheduler/pyproject.tomllibs/scheduler/src/scheduler/__init__.pylibs/scheduler/src/scheduler/adapters/repository.pylibs/scheduler/src/scheduler/core/service.pylibs/scheduler/src/scheduler/domain/models.pylibs/scheduler/src/scheduler/ports/handler.pylibs/scheduler/src/scheduler/ports/repository.pyservices/api/src/api/adapters/outbox_repository.pyservices/api/src/api/adapters/platform_settings_repository.pyservices/api/src/api/adapters/transaction_repository.pyservices/api/src/api/ports/platform_settings_repository.pyservices/api/src/api/routers/platform/__init__.pyservices/api/src/api/routers/platform/scheduler.pyservices/api/src/api/routers/trading_partners/platform/as2_partners.pyservices/api/src/api/routers/trading_partners/platform/settings.pyservices/api/src/api/services/as2_receiver_service.pyservices/api/tests/api_fakes.pyservices/api/tests/test_api_receiver_service.pyservices/api/tests/test_as2_partner_service.pyservices/api/tests/test_as2_receiver_service.pyservices/api/tests/test_inbound_flow_e2e.pyservices/api/tests/test_provisioning_core.pyservices/api/tests/test_routers_partners.pyservices/api/tests/test_routers_transactions.pyservices/workers/compute/src/compute_worker/main.pyservices/workers/compute/src/compute_worker/worker.pyservices/workers/orchestrator/pyproject.tomlservices/workers/orchestrator/src/worker/adapters/db_outbox.pyservices/workers/orchestrator/src/worker/data/main.pyservices/workers/orchestrator/src/worker/jobs/outbox_sweeper.py
Comment on lines
+29
to
+32
| created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=datetime.utcnow) | ||
| updated_at: Mapped[datetime] = mapped_column( | ||
| DateTime(timezone=True), default=datetime.utcnow, onupdate=datetime.utcnow | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use timezone-aware datetimes for DateTime(timezone=True) columns.
datetime.utcnow() returns a naive datetime which may be incorrectly interpreted as local time when inserted into a timezone-aware column, leading to offset bugs. Additionally, datetime.utcnow() is deprecated in Python 3.12. Use a timezone-aware callable instead.
🛡️ Proposed fix
Update the imports at the top of the file to include timezone:
-from datetime import datetime
+from datetime import datetime, timezoneApply this change to the columns:
- created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=datetime.utcnow)
- updated_at: Mapped[datetime] = mapped_column(
- DateTime(timezone=True), default=datetime.utcnow, onupdate=datetime.utcnow
- )
+ created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc))
+ updated_at: Mapped[datetime] = mapped_column(
+ DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), onupdate=lambda: datetime.now(timezone.utc)
+ )📝 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
| created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=datetime.utcnow) | |
| updated_at: Mapped[datetime] = mapped_column( | |
| DateTime(timezone=True), default=datetime.utcnow, onupdate=datetime.utcnow | |
| ) | |
| created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc)) | |
| updated_at: Mapped[datetime] = mapped_column( | |
| DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), onupdate=lambda: datetime.now(timezone.utc) | |
| ) |
🤖 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 `@libs/database/src/database/models/scheduled_job.py` around lines 29 - 32,
Update the scheduled job timestamp defaults in the created_at and updated_at
mapped columns to use a timezone-aware UTC datetime callable instead of
datetime.utcnow, and add the required timezone import while preserving the
existing DateTime(timezone=True) configuration and updated_at onupdate behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/web/src/features/partners/components/CreatePartnerModal.tsx (1)
31-50: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake certificate-secret cleanup atomic with state transitions.
handleCleanupsuppresses deletion failures, so reset, toggling, and regeneration still discard or replace the Vault reference, orphaning the private key. Conversely, successful deletion followed by generation failure leaves the deleted reference in state. Propagate cleanup failure, abort the transition, and clear old credential state immediately after successful deletion.Also applies to: 88-98, 148-167, 247-260
🤖 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 `@frontend/web/src/features/partners/components/CreatePartnerModal.tsx` around lines 31 - 50, Update handleCleanup and every caller involved in reset, toggling, and certificate regeneration so deletion errors propagate instead of being swallowed, and abort the associated state transition when cleanup fails. After successful deletion, immediately clear the old Vault reference and certificate-related state before proceeding; ensure generation failures do not leave a deleted reference in state, using the affected handlers around reset, lines 88-98, 148-167, and 247-260.services/api/src/api/routers/trading_partners/platform/as2_partners.py (1)
99-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInitialize cleanup state before entering the
tryblock.If
uow.__aenter__()fails, the exception handler reads uninitialized locals and replaces the original error withUnboundLocalError.Proposed fix
+ auto_generated = False + private_key_vault_ref = request.private_key_vault_ref try: async with uow: public_cert_pem = request.public_cert_pem - private_key_vault_ref = request.private_key_vault_ref - - auto_generated = FalseAlso applies to: 156-158
🤖 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 `@services/api/src/api/routers/trading_partners/platform/as2_partners.py` around lines 99 - 104, Initialize the cleanup-state locals, including auto_generated and any related certificate/key variables used by the exception handler, before the try block surrounding the async with uow in the affected AS2 partner flows. Ensure failures from uow.__aenter__() can reach the handler without triggering UnboundLocalError, while preserving the existing cleanup behavior after successful entry.libs/scheduler/src/scheduler/adapters/repository.py (1)
39-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReclaim jobs abandoned by crashed workers.
A worker crash after this transaction commits leaves the job permanently
RUNNING; future claims only selectPENDINGrows. Add a configurable lock lease and reclaim expiredRUNNINGjobs, or run an equivalent recovery step on startup.🤖 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 `@libs/scheduler/src/scheduler/adapters/repository.py` around lines 39 - 74, The claim_next_jobs method only selects PENDING jobs, leaving RUNNING jobs abandoned after worker crashes. Add a configurable lock-lease duration and include RUNNING records whose locked_at timestamp has expired in the claim query, while preserving the existing eligibility, ordering, locking, and reassignment updates; ensure the lease configuration is available through the repository’s existing configuration path.
🤖 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 `@frontend/web/src/features/platform/components/SchedulerDashboard.tsx`:
- Around line 72-76: Update interval validation in the SchedulerDashboard
handler around intervalValue and val so fractional inputs such as “1.5” are
rejected rather than truncated by parseInt. Validate the complete input as a
positive integer before saving or scheduling it, while preserving the existing
error message and return behavior for invalid values.
In `@libs/scheduler/src/scheduler/core/service.py`:
- Around line 48-90: The scheduler lacks recovery for jobs left RUNNING after a
worker failure. Add a periodic stuck-job sweeper that identifies RUNNING jobs
whose locked_at exceeds a configurable timeout, resets them to PENDING, and
clears their lock metadata so claim_next_jobs can process them again; integrate
it with the scheduler lifecycle without changing normal _execute_job handling.
In `@services/api/src/api/routers/platform/scheduler.py`:
- Around line 38-41: Update the job-creation flow using JobCreateRequest so it
resolves the requested job name through SYSTEM_JOB_REGISTRY and populates the
created job’s target queue and dispatch configuration before scheduling.
Validate that the name exists and has usable registry configuration, rejecting
invalid requests rather than creating jobs with target_queue=None; preserve the
existing request fields and scheduling behavior for valid entries.
In `@services/as2_server/scripts/seed.py`:
- Around line 133-142: The seeding update in the existing-job branch must
preserve user-managed schedule fields. In the `else` block, stop assigning
`cron_expression` and `timezone` from `job_def`; synchronize only immutable
routing metadata and validation bounds, and ensure schedule-mode conversion does
not leave both cron and `interval_seconds` configured.
In `@services/workers/orchestrator/src/worker/adapters/sqs_poller.py`:
- Around line 73-80: Update the exception handling around the SQS
message-processing flow to stop treating broad KeyError and NotImplementedError
exceptions as permanent validation failures; remove the permanent delete path
using sqs.delete_message and allow these errors to propagate so SQS can apply
its normal retry/DLQ behavior, or replace the broad catches with a narrowly
scoped custom permanent-validation exception.
In `@services/workers/orchestrator/src/worker/adapters/sqs_publisher.py`:
- Around line 89-110: Update SQS exception handling in publish to re-raise
failures after logging: both the queue URL lookup handler and the send_message
handler must propagate the caught exception instead of returning normally,
allowing scheduler retries to retain the job.
In `@services/workers/orchestrator/src/worker/core/security.py`:
- Around line 26-49: Update the SSRF validation flow around the hostname
resolution and subsequent HTTP request so the validated IP addresses are pinned
to the actual connection, or route the request through an SSRF-aware egress
proxy. Do not rely on separate DNS validation followed by hostname-based
connection; ensure every redirect destination is resolved and validated again
when redirects are enabled.
In `@services/workers/orchestrator/src/worker/core/tenant_resolver.py`:
- Around line 11-14: The tenant cache in __init__ and its lookup/update flow
must enforce both TTL expiry and a maximum entry count. Add sweeping to remove
expired entries, and evict additional entries when the cache exceeds the
configured bound, while preserving valid cached tenant resolution behavior.
In `@services/workers/orchestrator/src/worker/data/handlers.py`:
- Around line 150-174: The idempotency handling in the delivery flow must not
create `ProcessedEvent` and mark `DataPlaneOutbox` as processed only in the
transaction after delivery. Update the surrounding handler and
`service.deliver()` integration to persist a recoverable delivery-attempt state
before sending, pass the stable `key_uuid` or equivalent identifier to
downstream transports, and reconcile any uncertain attempt before allowing an
SQS retry to deliver again.
In `@services/workers/orchestrator/src/worker/data/main.py`:
- Around line 124-127: Wrap the worker’s asyncio.gather(transform_task,
deliver_task, scheduled_jobs_task) flow in a try...finally block, and call await
scheduler_service.stop() in the finally clause. Keep scheduler_service.start()
before the gather and ensure stop executes whenever the gather is cancelled or
exits, allowing active jobs to finish gracefully.
In `@services/workers/orchestrator/src/worker/data/scheduled_jobs_handler.py`:
- Around line 30-34: Update the unknown-job branch in the scheduled job handler
lookup to raise an exception instead of logging and returning. Preserve the
existing error context with the unknown job name, allowing queue processing to
retain the message for retry or dead-letter handling.
In `@services/workers/orchestrator/src/worker/jobs/data_retention.py`:
- Around line 36-46: Update the _bounded_cleanup helper and surrounding
asyncio.gather flow so _cleanup_shard exceptions are not converted into
successful (0, 0) results. Preserve best-effort execution across all shards,
then propagate the failure—either by collecting failed shard names and raising
after gather completes or by allowing gather to propagate exceptions.
In `@services/workers/orchestrator/tests/test_data_main.py`:
- Around line 20-29: Update test_validate_target_url to mock socket.getaddrinfo
with a public address for the successful example.com assertion, eliminating live
DNS dependency. Change the failure mock to raise socket.gaierror instead of a
generic Exception, while preserving the existing expected validation results.
- Around line 101-103: Strengthen the cache assertion in the test around
resolver.resolve by verifying the second call does not access the database. Spy
on get_global_session and assert it is not invoked during the second resolution,
or make the backing row unavailable before that call while preserving the
existing value assertions.
---
Outside diff comments:
In `@frontend/web/src/features/partners/components/CreatePartnerModal.tsx`:
- Around line 31-50: Update handleCleanup and every caller involved in reset,
toggling, and certificate regeneration so deletion errors propagate instead of
being swallowed, and abort the associated state transition when cleanup fails.
After successful deletion, immediately clear the old Vault reference and
certificate-related state before proceeding; ensure generation failures do not
leave a deleted reference in state, using the affected handlers around reset,
lines 88-98, 148-167, and 247-260.
In `@libs/scheduler/src/scheduler/adapters/repository.py`:
- Around line 39-74: The claim_next_jobs method only selects PENDING jobs,
leaving RUNNING jobs abandoned after worker crashes. Add a configurable
lock-lease duration and include RUNNING records whose locked_at timestamp has
expired in the claim query, while preserving the existing eligibility, ordering,
locking, and reassignment updates; ensure the lease configuration is available
through the repository’s existing configuration path.
In `@services/api/src/api/routers/trading_partners/platform/as2_partners.py`:
- Around line 99-104: Initialize the cleanup-state locals, including
auto_generated and any related certificate/key variables used by the exception
handler, before the try block surrounding the async with uow in the affected AS2
partner flows. Ensure failures from uow.__aenter__() can reach the handler
without triggering UnboundLocalError, while preserving the existing cleanup
behavior after successful entry.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e9898f1-7f77-4826-8ab7-f7ade1ca6346
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
docker/localstack/init-aws.shfrontend/web/src/features/partners/api/partnerHooks.tsfrontend/web/src/features/partners/components/As2PartnerDetails.tsxfrontend/web/src/features/partners/components/CreatePartnerModal.tsxfrontend/web/src/features/partners/types.tsfrontend/web/src/features/partners/utils/certificate.tsfrontend/web/src/features/platform/api/schedulerApi.tsfrontend/web/src/features/platform/api/schedulerHooks.tsfrontend/web/src/features/platform/components/CronBuilder.tsxfrontend/web/src/features/platform/components/SchedulerDashboard.tsxlibs/config/src/config/settings.pylibs/database/src/database/migrations/global/versions/42c7e50a7b1c_global_initial_schema.pylibs/database/src/database/models/scheduled_job.pylibs/scheduler/pyproject.tomllibs/scheduler/src/scheduler/adapters/repository.pylibs/scheduler/src/scheduler/core/service.pylibs/scheduler/src/scheduler/domain/models.pylibs/scheduler/src/scheduler/ports/handler.pylibs/scheduler/src/scheduler/ports/publisher.pylibs/scheduler/src/scheduler/ports/repository.pylibs/scheduler/src/scheduler/registry.pyservices/api/src/api/adapters/http/dtos.pyservices/api/src/api/adapters/transaction_repository.pyservices/api/src/api/routers/platform/__init__.pyservices/api/src/api/routers/platform/scheduler.pyservices/api/src/api/routers/trading_partners/platform/as2_partners.pyservices/api/src/api/services/api_receiver_service.pyservices/api/tests/test_scheduler.pyservices/as2_server/scripts/seed.pyservices/workers/orchestrator/src/worker/adapters/sqs_poller.pyservices/workers/orchestrator/src/worker/adapters/sqs_publisher.pyservices/workers/orchestrator/src/worker/core/job_registry.pyservices/workers/orchestrator/src/worker/core/security.pyservices/workers/orchestrator/src/worker/core/tenant_resolver.pyservices/workers/orchestrator/src/worker/data/handlers.pyservices/workers/orchestrator/src/worker/data/main.pyservices/workers/orchestrator/src/worker/data/scheduled_jobs_handler.pyservices/workers/orchestrator/src/worker/jobs/data_retention.pyservices/workers/orchestrator/src/worker/jobs/outbox_sweeper.pyservices/workers/orchestrator/src/worker/ports/message_publisher.pyservices/workers/orchestrator/tests/test_data_main.pyservices/workers/orchestrator/tests/test_sqs_publisher.py
Comment on lines
20
to
+29
| def test_validate_target_url(): | ||
| assert validate_target_url("http://example.com") is True | ||
| assert validate_target_url("http://127.0.0.1") is False | ||
| assert validate_target_url("ftp://example.com") is False | ||
| assert validate_target_url("http://") is False | ||
| assert validate_target_url("http://192.168.1.1") is False | ||
| assert validate_target_url("http://10.0.0.1") is False | ||
|
|
||
| with patch("socket.getaddrinfo", side_effect=Exception("mock err")): | ||
| assert validate_target_url("http://example.com") is False |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove live DNS from this unit test.
The positive example.com assertion depends on CI network/DNS availability. Mock socket.getaddrinfo with a public address, and use socket.gaierror for the failure case so both paths are deterministic.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 20-20: Do not make http calls without encryption
Context: "http://example.com"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 23-23: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 24-24: Do not make http calls without encryption
Context: "http://192.168.1.1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 25-25: Do not make http calls without encryption
Context: "http://10.0.0.1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 28-28: Do not make http calls without encryption
Context: "http://example.com"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 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 `@services/workers/orchestrator/tests/test_data_main.py` around lines 20 - 29,
Update test_validate_target_url to mock socket.getaddrinfo with a public address
for the successful example.com assertion, eliminating live DNS dependency.
Change the failure mock to raise socket.gaierror instead of a generic Exception,
while preserving the existing expected validation results.
Comment on lines
+101
to
+103
| # Resolving again should hit cache | ||
| resolved_shard_name2, shard_dsn2 = await resolver.resolve(tenant_id) | ||
| assert resolved_shard_name2 == shard_name |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert that the second resolution actually uses the cache.
Returning the same values does not prove the database was skipped. Spy on get_global_session or make the backing row unavailable before the second call.
🤖 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 `@services/workers/orchestrator/tests/test_data_main.py` around lines 101 -
103, Strengthen the cache assertion in the test around resolver.resolve by
verifying the second call does not access the database. Spy on
get_global_session and assert it is not invoked during the second resolution, or
make the backing row unavailable before that call while preserving the existing
value assertions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
services/api/src/api/routers/platform/scheduler.py (1)
83-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
interval_secondsagainst job definition boundaries.The
POSTendpoint currently allows creating a job with aninterval_secondsvalue that violates the minimum or maximum constraints defined in theSYSTEM_JOB_REGISTRY. Validaterequest.interval_secondsbefore creating the job to ensure it complies withjob_def.min_interval_secondsandjob_def.max_interval_seconds, similar to how thePUTendpoint handles interval updates.🛡️ Proposed fix to add boundary validation
if not job_def.target_queue: raise HTTPException( status_code=422, detail=f"Job '{request.name}' has no configured target queue." ) + + if job_def.min_interval_seconds and request.interval_seconds < job_def.min_interval_seconds: + raise HTTPException( + status_code=422, detail=f"Interval must be at least {job_def.min_interval_seconds} seconds." + ) + if job_def.max_interval_seconds and request.interval_seconds > job_def.max_interval_seconds: + raise HTTPException( + status_code=422, detail=f"Interval must be at most {job_def.max_interval_seconds} seconds." + ) async with uow:🤖 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 `@services/api/src/api/routers/platform/scheduler.py` around lines 83 - 109, Validate request.interval_seconds after resolving job_def and before entering the transaction, enforcing job_def.min_interval_seconds and job_def.max_interval_seconds consistently with the PUT endpoint. Reject values outside either configured boundary with the endpoint’s existing validation error behavior, and only construct ScheduledJob after validation succeeds.libs/pipeline/src/pipeline/core/delivery/router.py (1)
35-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject outbound messages that lack
trading_partner_id.The
elsefallback sends malformed outbound messages through legacy sender/receiver routing, potentially selecting an unintended route. Handle outbound direction separately and raise when its trading-partner identifier is missing.Proposed fix
- if direction == "OUTBOUND" and edi_msg.trading_partner_id: + if direction == "OUTBOUND": + if not edi_msg.trading_partner_id: + raise ValueError( + f"Outbound EDI Message {trace_id} is missing trading_partner_id." + ) route = await self.repository.get_outbound_route_by_trading_partner_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 `@libs/pipeline/src/pipeline/core/delivery/router.py` around lines 35 - 47, Update the routing branch around the outbound check so every OUTBOUND message requires a non-empty edi_msg.trading_partner_id; raise a ValueError when it is missing, and only use the existing trading-partner route lookup when the identifier is present. Keep the legacy sender/receiver fallback restricted to non-outbound messages.services/api/src/api/routers/trading_partners/platform/as2_partners.py (1)
155-157: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not delete the key after the partner transaction has committed.
If the read-back at Line 143 fails after the successful commit, this handler deletes the now-active key while leaving the partner persisted. Track commit completion and only clean up secrets created before an unsuccessful transaction.
Proposed fix
auto_generated = False + committed = False try: async with uow: ... await uow.commit() + committed = True p = await uow.as2_partners.get_as2_partner(...) ... except Exception as e: - if auto_generated and private_key_vault_ref: + if auto_generated and private_key_vault_ref and not committed: vault.delete_secret(private_key_vault_ref)🤖 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 `@services/api/src/api/routers/trading_partners/platform/as2_partners.py` around lines 155 - 157, Update the exception handling around the partner transaction to track whether the commit completed successfully, and only call vault.delete_secret for an auto-generated private key when the transaction failed before commit. Preserve the persisted partner and active key when a post-commit read-back or other subsequent operation raises, using the commit-status state in the handler containing the auto_generated and private_key_vault_ref cleanup.frontend/web/src/features/partners/components/CreatePartnerModal.tsx (1)
52-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCancel or invalidate certificate generation when abandoning the draft.
Closing the modal or changing mode while generation is pending performs cleanup before a Vault reference exists. The later
onSuccessthen stores the generated secret in abandoned/remote state, leaking an orphaned private key. Invalidate pending callbacks and delete any stale response’sprivate_key_vault_ref.Also applies to: 152-183, 257-283
🤖 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 `@frontend/web/src/features/partners/components/CreatePartnerModal.tsx` around lines 52 - 56, Update handleOpenChange and the mode-change cleanup paths to invalidate or cancel pending certificate generation before resetting the draft. Guard the generation onSuccess handler so callbacks from abandoned generations cannot update partner or remote state, and delete any stale response’s private_key_vault_ref when such a response arrives.
🤖 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 `@libs/pipeline/src/pipeline/core/delivery/as2.py`:
- Around line 25-31: Update the AS2 delivery flow in deliver to propagate
idempotency_key into AS2 message construction or transport, using it as the
stable downstream duplicate identifier. Ensure the value reaches the remote
submission path so retries after acceptance do not create duplicate
transactions.
In `@libs/pipeline/src/pipeline/core/delivery/router.py`:
- Line 68: Update the delivery flow around strategy.deliver so swallowed
strategy failures produce an explicit unsuccessful outcome or are re-raised as
retryable errors. Ensure ProcessedEvent creation and outbox PROCESSED
finalization occur only after confirmed successful delivery, while failed
payloads remain eligible for retry.
In `@services/api/tests/test_api_receiver_service.py`:
- Around line 40-41: Strengthen the assertions in the affected API receiver
tests by inspecting mock_uow.transactions.create_edi_json.await_args rather than
only verifying it was awaited. Assert the persisted transaction_type is 850 or
855 as appropriate, and in the list case assert business_metadata contains the
expected aggregated extraction values; retain the existing trace and call-count
checks.
In `@services/workers/orchestrator/src/worker/core/tenant_resolver.py`:
- Around line 28-32: Update the cache expiration timing in
TenantResolver.resolve to use time.monotonic() instead of time.time(), ensuring
the now value passed to _sweep and subsequent TTL calculations uses a
process-local monotonic clock.
In `@services/workers/orchestrator/tests/test_security.py`:
- Around line 46-53: Update test_ssrf_safe_context_valid to patch
worker.core.security._orig_getaddrinfo instead of socket.getaddrinfo, preserving
the resolver override installed by ssrf_safe_context. Configure the original
resolver mock to return the expected address and assert it is called with
"93.184.216.34".
---
Outside diff comments:
In `@frontend/web/src/features/partners/components/CreatePartnerModal.tsx`:
- Around line 52-56: Update handleOpenChange and the mode-change cleanup paths
to invalidate or cancel pending certificate generation before resetting the
draft. Guard the generation onSuccess handler so callbacks from abandoned
generations cannot update partner or remote state, and delete any stale
response’s private_key_vault_ref when such a response arrives.
In `@libs/pipeline/src/pipeline/core/delivery/router.py`:
- Around line 35-47: Update the routing branch around the outbound check so
every OUTBOUND message requires a non-empty edi_msg.trading_partner_id; raise a
ValueError when it is missing, and only use the existing trading-partner route
lookup when the identifier is present. Keep the legacy sender/receiver fallback
restricted to non-outbound messages.
In `@services/api/src/api/routers/platform/scheduler.py`:
- Around line 83-109: Validate request.interval_seconds after resolving job_def
and before entering the transaction, enforcing job_def.min_interval_seconds and
job_def.max_interval_seconds consistently with the PUT endpoint. Reject values
outside either configured boundary with the endpoint’s existing validation error
behavior, and only construct ScheduledJob after validation succeeds.
In `@services/api/src/api/routers/trading_partners/platform/as2_partners.py`:
- Around line 155-157: Update the exception handling around the partner
transaction to track whether the commit completed successfully, and only call
vault.delete_secret for an auto-generated private key when the transaction
failed before commit. Preserve the persisted partner and active key when a
post-commit read-back or other subsequent operation raises, using the
commit-status state in the handler containing the auto_generated and
private_key_vault_ref cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32db7d9a-4fa9-4425-b2ff-3d1f8e5b6266
📒 Files selected for processing (34)
frontend/web/src/features/partners/components/CreatePartnerModal.tsxfrontend/web/src/features/platform/components/SchedulerDashboard.tsxlibs/pipeline/src/pipeline/adapters/as2.pylibs/pipeline/src/pipeline/adapters/http.pylibs/pipeline/src/pipeline/core/delivery/as2.pylibs/pipeline/src/pipeline/core/delivery/base.pylibs/pipeline/src/pipeline/core/delivery/router.pylibs/pipeline/src/pipeline/core/delivery/sftp.pylibs/pipeline/src/pipeline/core/delivery/webhook.pylibs/pipeline/src/pipeline/ports/http.pylibs/pipeline/tests/fakes.pylibs/scheduler/src/scheduler/adapters/repository.pylibs/scheduler/src/scheduler/core/service.pylibs/scheduler/src/scheduler/ports/repository.pyservices/api/src/api/routers/platform/scheduler.pyservices/api/src/api/routers/trading_partners/platform/as2_partners.pyservices/api/tests/test_api_receiver_service.pyservices/api/tests/test_scheduler.pyservices/as2_server/scripts/seed.pyservices/workers/orchestrator/src/worker/adapters/sqs_poller.pyservices/workers/orchestrator/src/worker/adapters/sqs_publisher.pyservices/workers/orchestrator/src/worker/core/security.pyservices/workers/orchestrator/src/worker/core/tenant_resolver.pyservices/workers/orchestrator/src/worker/data/handlers.pyservices/workers/orchestrator/src/worker/data/main.pyservices/workers/orchestrator/src/worker/data/scheduled_jobs_handler.pyservices/workers/orchestrator/src/worker/jobs/data_retention.pyservices/workers/orchestrator/src/worker/main.pyservices/workers/orchestrator/tests/test_data_main.pyservices/workers/orchestrator/tests/test_data_retention.pyservices/workers/orchestrator/tests/test_handlers.pyservices/workers/orchestrator/tests/test_outbox_sweeper.pyservices/workers/orchestrator/tests/test_scheduled_jobs_handler.pyservices/workers/orchestrator/tests/test_security.py
💤 Files with no reviewable changes (1)
- services/as2_server/scripts/seed.py
| partner_id = route.get(route_key) | ||
| if partner_id: | ||
| await strategy.deliver(trace_id, partner_id, edi_msg) | ||
| await strategy.deliver(trace_id, partner_id, edi_msg, idempotency_key) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not treat a swallowed strategy failure as processed delivery.
All supplied strategies catch delivery failures and return normally. Consequently, the worker records ProcessedEvent and marks the outbox PROCESSED after this call even when the payload was marked FAILED, preventing retries. Return an explicit outcome or re-raise retryable failures and only finalize successful deliveries.
🤖 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 `@libs/pipeline/src/pipeline/core/delivery/router.py` at line 68, Update the
delivery flow around strategy.deliver so swallowed strategy failures produce an
explicit unsuccessful outcome or are re-raised as retryable errors. Ensure
ProcessedEvent creation and outbox PROCESSED finalization occur only after
confirmed successful delivery, while failed payloads remain eligible for retry.
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
1 participant
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 CodeRabbit