feat(procore): Track A P0 hardening — fail-closed, durable, async /v1 webhook (T1-T4,T10)#24
Merged
Merged
Conversation
The deprecated Stage 2 /procore webhook route is registered by default alongside the canonical /v1/procore/webhook, exposing a second (pre-auth- ordering) attack surface. Disable its registration unless PROCORE_LEGACY_ROUTE_ENABLED=true (rollback only); the lazy import means api/procore.py is not loaded by default. Module stays on disk for the direct-import tests in tests/test_procore_stage2.py. Adds a request-level test asserting POST /procore/webhook -> 404 by default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A submittal for a project without a rule pack returned early with status=no_rule_pack and received NO review at all, even though the engine supports pack-less structural review. Replace the early return with an empty fallback pack carrying project_id=event.project_id, so the baseline/ structural review always runs; the rule pack only ADDS project-specific criteria. project_review_status resolves to UNAVAILABLE and the artifact's project_id stays populated. Updates the request test: no rule pack -> review runs (200/reviewed), project_id preserved, project_review_status=UNAVAILABLE. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
/v1/procore/webhook previously verified the signature only when a secret was configured (fail-open) and hardcoded an unverified header. Replace it with a pluggable _verify_procore_request selected by env so Procore's confirmed scheme is a config swap, not a code change: - PROCORE_REQUIRE_AUTH (default true), PROCORE_AUTH_SCHEME (default unverified -> reject), schemes hmac_sha256 / authorization_bearer, and a configurable PROCORE_SIGNATURE_HEADER. - Fail closed: unknown/unset scheme rejects. Auth runs before any side effect (logging, idempotency, fetch, review, comment). - No Procore scheme invented; validate_signature is one selectable scheme. Tests: TestWebhookAuth covers unverified->401 with no side effects, no-secret->401, bad bearer->401, valid bearer/HMAC->200. Functional tests run with auth bypassed via an autouse fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In-memory idempotency was lost on restart and checked AFTER log_payload; deliveries without a delivery_id were never de-duplicated. Add a durable reservation: - delivery_key() prefers delivery_id, falls back to sha256(raw body). - reserve_delivery() reserves via a Supabase RPC when configured, else an in-memory fallback (logged as non-durable); on RPC error it falls back too. - The route reserves right after parse_event, BEFORE log_payload/fetch/ review/comment; duplicate -> already_processed. Migration 007 adds the deliveries table in a private (non-exposed) schema with RLS, and a SECURITY DEFINER reserve RPC in public with EXECUTE granted to service_role only. Not yet applied to the project; until then the route degrades to the in-memory fallback. Tests: body-hash fallback dedupes, reservation precedes log_payload, durable path used when configured, falls back to memory on error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
/v1/procore/webhook processed fetch/review/store/compare/comment synchronously and returned the review body, risking Procore's webhook timeout. Extract the pipeline into _process_procore_v1_webhook and run it via BackgroundTasks: after auth + durable reservation + log_payload, the endpoint returns 202 'accepted' immediately. - Failures in the background pipeline are recorded (record_state) and alerted via a new canonical core/procore/alerts.py, never raised to the caller (which already got 202). /v1 no longer depends on api/procore.py. - BackgroundTasks is interim async; certification-grade durability may need a worker/queue (noted in code). Tests: endpoint asserts 202/accepted; TestProcorePipeline covers reviewed, baseline-UNAVAILABLE, ignored, and failure-alerted-not-raised. record_state stubbed so background tasks never hit Supabase. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 4265868fda
ℹ️ 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".
Comment on lines
+1524
to
+1528
| if not reserve_delivery(delivery_key(event.delivery_id, body), correlation_id): | ||
| return JSONResponse(content={"status": "already_processed", "delivery_id": event.delivery_id}) | ||
|
|
||
| # Phase 1: only handle submittal events | ||
| if "submittal" not in event.event_type: | ||
| return JSONResponse(content={"status": "ignored", "reason": "not a submittal event"}) | ||
|
|
||
| # Extract PDF attachments | ||
| attachments = extract_submittal_attachments(event) | ||
| if not attachments: | ||
| return JSONResponse(content={ | ||
| "status": "no_pdf", | ||
| "reason": "No PDF attachments found in submittal", | ||
| }) | ||
|
|
||
| # Load project rule pack | ||
| _PROCORE_RULE_PACKS_DIR.mkdir(parents=True, exist_ok=True) | ||
| rule_pack_path = _PROCORE_RULE_PACKS_DIR / f"project_{event.project_id}.json" | ||
| if not rule_pack_path.exists(): | ||
| return JSONResponse(content={ | ||
| "status": "no_rule_pack", | ||
| "reason": f"No project rule pack found for project {event.project_id}", | ||
| "project_id": event.project_id, | ||
| }) | ||
|
|
||
| with open(rule_pack_path, encoding="utf-8") as f: | ||
| rule_pack = json.load(f) | ||
|
|
||
| att = attachments[0] | ||
| swms_text = "" | ||
| retrieval_mode = "none" | ||
|
|
||
| # Phase 1B: try live Procore API retrieval first | ||
| try: | ||
| from core.procore.api_client import fetch_attachment, is_live_configured | ||
| if is_live_configured() and att.url: | ||
| pdf_bytes = fetch_attachment(event.project_id, att.url) | ||
| from core.intake_extractor import extract_from_pdf | ||
| extraction = extract_from_pdf(pdf_bytes, source_label=att.filename) | ||
| if extraction.text_extraction_succeeded: | ||
| swms_text = extraction.raw_text | ||
| retrieval_mode = "live_api" | ||
| logger.info(f"Live retrieval: {len(swms_text)} chars from {att.filename}") | ||
| except Exception as live_err: | ||
| logger.warning(f"Live Procore retrieval failed: {live_err}") | ||
|
|
||
| # Phase 1A fallback: simulated/fixture text | ||
| if not swms_text: | ||
| swms_text = payload.get("_simulated_swms_text", "") | ||
| if swms_text: | ||
| retrieval_mode = "simulated" | ||
|
|
||
| if not swms_text: | ||
| fixture_path = payload.get("_fixture_pdf_text_path", "") | ||
| if fixture_path and _Path(fixture_path).exists(): | ||
| swms_text = _Path(fixture_path).read_text(encoding="utf-8") | ||
| retrieval_mode = "fixture" | ||
|
|
||
| if not swms_text: | ||
| return JSONResponse(content={ | ||
| "status": "no_text", | ||
| "reason": f"Could not extract text from {att.filename}. " | ||
| "Configure PROCORE_ACCESS_TOKEN for live retrieval, " | ||
| "or provide _simulated_swms_text for testing.", | ||
| "attachment": att.filename, | ||
| }) | ||
| # Log payload for replay/debugging (after reservation). | ||
| log_payload(event) |
There was a problem hiding this comment.
When log_payload raises, for example on a read-only container filesystem or a full disk, this path has already committed the delivery key via reserve_delivery but has not yet scheduled _process_procore_v1_webhook. The request then fails, and any Procore retry is answered already_processed, so the submittal is never reviewed or alerted; make the replay log best-effort or only finalize the reservation once the background work is safely queued.
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
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
Track A — Procore go-live P0 hardening (P0 blockers from the Stage 3 certification review). Bounds the integration to a single, fail-closed, durable, async
/v1/procore/webhook. Confirmation-independent work only; Procore-gated final wiring (real auth scheme, write-back resource) is deferred.Source of truth:
docs/procore/STAGE3_CERTIFICATION_PLAN_V2.md(tickets T1–T10).Tickets in this PR (one commit each)
feat(procore): disable legacy /procore route behind a flag— only/v1is registered by default;/procore→ 404 unlessPROCORE_LEGACY_ROUTE_ENABLED=true. Removes the pre-auth-ordering attack surface.feat(procore): run baseline review when no project rule pack— empty fallback pack so baseline/structural review always runs;project_review_status=UNAVAILABLE,project_idpreserved.feat(procore): scheme-agnostic fail-closed webhook auth— pluggableverify(headers, body)selected by env (PROCORE_REQUIRE_AUTH,PROCORE_AUTH_SCHEME); fail-closed by default; runs before any side effect. No Procore scheme invented (UNVERIFIED pending Procore).feat(procore): durable idempotency reserved before side effects— Supabase reserve RPC (migration 007, private schema + SECURITY DEFINER + service-role EXECUTE) with in-memory fallback; SHA-256(body) key when no delivery_id; reserved before logging/fetch/review/comment.feat(procore): async 202 + dead-letter failure surface— pipeline extracted to a background task; endpoint returns 202 fast; failures recorded + alerted via newcore/procore/alerts.py, never raised.Deployment prerequisites (not done in this PR)
007_procore_webhook_deliveries.sqlto the Supabase project; setSUPABASE_SERVICE_ROLE_KEYin the app env (else idempotency degrades to in-memory).PROCORE_AUTH_SCHEME(+ secret) once Procore confirms the real scheme — until then the endpoint is fail-closed (401).Out of scope (next batches)
Tests
pytest tests/test_procore_webhook.py→ 67 passed. Full suite green (pre-push gate passed). flake8 clean.🤖 Generated with Claude Code