Repository navigation
fix(ingestion): stop absolute app.* imports resolving to graph in prod - #3780
Conversation
In the consolidated server every service dir is on sys.path and the
top-level ``app`` package is services/graph/app. Three ingestion modules
still imported their siblings as ``app.X``:
- stripe_billing/rate_limiting: ``app.config`` loaded graph's Settings,
which has no redis_url, so the Stripe webhook rate limiter always fell
back to per-worker memory ("'Settings' object has no attribute
'redis_url'") even with Redis healthy.
- audit_export_log: ``app.chain_verification_job`` raised
ModuleNotFoundError inside the verification fallback.
- labels/gs1_128: ``app.gs1_identifiers`` made the module unimportable
(no runtime caller yet).
All three are now relative. A new AST guard test fails on any absolute
``app`` import or ``import_module("app....")`` in services/ingestion/app.
It allows the two that open work already removes (#3774 and
fix/csv-edi-ingest-monolith-imports); those entries should be deleted when
that work lands.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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: cb6bbc7a-5804-48ee-b2b0-6d68181a1334) |
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preventing absolute app.* imports from resolving to the graph package in production. Description check The description explains the root cause, affected modules, code changes, test coverage, and runtime verification. It omits the template headings for Type of Change, Checklist, and Related Issues, but … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the root cause, affected modules, code changes, test coverage, and runtime verification. It omits the template headings for Type of Change, Checklist, and Related Issues, but it provides the critical implementation and validation details.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
PR Blocker ChecklistHead SHA: Status: Ready
|
…lute-app-imports Signed-off-by: Christopher Sellers <1@christophersellers.com>
…lute-app-imports Signed-off-by: Christopher Sellers <1@christophersellers.com>
PetrefiedThunder
left a comment
PetrefiedThunder
left a comment
There was a problem hiding this comment.
REVIEW: PASS pi 75f874b
Checked: the three absolute app.* imports (audit_export_log -> .chain_verification_job, labels/gs1_128 -> ..gs1_identifiers, stripe_billing/rate_limiting -> ..config) resolve to the same ingestion modules relatively, so the consolidated server can no longer pick up graph's app.config. The new AST guard catches from app..., import app... and import_module("app...").
Local (box, py3.12, requirements.lock): 701 passed, 6 skipped (guard + audit-export, stripe, rate-limit, sandbox and settings suites). CI: everything relevant is green; only Engine Review Pass is red, which is expected before this review.
Non-blocking: PENDING_ELSEWHERE lists edi_ingestion/routes.py / app.edi_ingestion.rejection_log. #3782 also removes that import, so drop the entry once either PR lands.
Brings in #3780 (relative imports in three ingestion modules plus the test_no_absolute_app_imports guard), #3793, #3794, #3807 and #3809. Both conflicts (stripe_billing/rate_limiting.py, webhook_compat.py) were the same relative-import change made on both sides; main's wording kept. The new guard test, the WI-02 bare-import guard, the route snapshot test and the repository gates pass on the merged tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyhDYGPjbfDzVjRUqkvz4B
Brings in #3780 (relative imports in three ingestion modules plus the test_no_absolute_app_imports guard), #3793, #3794, #3807 and #3809. Both conflicts (stripe_billing/rate_limiting.py, webhook_compat.py) were the same relative-import change made on both sides; main's wording kept. The new guard test, the WI-02 bare-import guard, the route snapshot test and the repository gates pass on the merged tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyhDYGPjbfDzVjRUqkvz4B
Summary
This is the same class of bug as the CSV/EDI 500 fixed in #3774. In the consolidated server, the top-level
apppackage isservices/graph/app. So anyfrom app.X import ...inside ingestion either fails or quietly loads graph's module instead. Unit tests putservices/ingestionfirst on the path, so they never see it.I checked each one inside the production image after
import server.main:stripe_billing/rate_limiting.py:app.configservices/graph/app/config.pySettingshas noredis_url, so the Stripe webhook rate limiter always falls back to per-worker memory, even with Redis healthy. Log:'Settings' object has no attribute 'redis_url'audit_export_log.py:app.chain_verification_jobModuleNotFoundErrorlabels/gs1_128.py:app.gs1_identifiersModuleNotFoundErrorChange
All three imports are now relative.
New
test_no_absolute_app_imports.pyparsesservices/ingestion/appand fails on any absoluteappimport orimport_module("app...."). It allows the two that open work already removes:webhook_compat.py, removed by fix(ingestion): resolve tenant_validation import in prod image #3774.edi_ingestion/routes.pyrejection_log, removed by the unpushedfix/csv-edi-ingest-monolith-importsbranch.Delete those entries when that work lands.
Verification
Runtime. Production image built from this branch, with Postgres 17 and Redis 7.
main): a signed-looking request toPOST /api/v1/billing/webhookslogsRedis unavailable for stripe webhook rate limiting, using in-memory fallback ... 'Settings' object has no attribute 'redis_url'.stripe_webhook:rate:<ip>. Inside the server,labels.gs1_128andaudit_export_logimport, and the audit fallback runs.Tests
main's code.🤖 Generated with Claude Code
Note
Medium Risk
Touches Stripe webhook rate limiting and audit fallback paths in production; behavior change is intentional (correct module resolution) but affects billing and observability under the monolith.
Overview
Fixes production mis-resolution of
from app.*inside ingestion when the consolidated server loads multiple services that each expose a top-levelapppackage (graph'sappwins onsys.path).Three imports are switched to relative paths: audit verification fallback now pulls
chain_verification_jobfrom.chain_verification_jobinstead of failing withModuleNotFoundError; GS1-128 labels importgs1_identifiersvia..gs1_identifiers; Stripe webhook rate limiting loads ingestionconfigvia..configsoredis_urlis read correctly and Redis-backed limits work instead of always falling back to per-worker memory.Adds
test_no_absolute_app_imports.py, which AST-scansservices/ingestion/appand fails on new absoluteapp/import_module("app...")usage, with a small allowlist for fixes landing in other PRs.Reviewed by Cursor Bugbot for commit 75f874b. Bugbot is set up for automated code reviews on this repo. Configure here.