fix(portal+data): Secure Hub DB accessor, durable chat leads, booking race (P0b/P1b)#210
Conversation
… race (P0b/P1b) Holistic review, batch 3. - Secure Hub portal (P0b): get_secure_hub_deal / download used get_database() .collection(...), but get_database() returns the THODatabase wrapper (no .collection) -> every call 500'd. The portal was non-functional (and so not actually leaking PII, despite looking vulnerable). Use get_database().db (raw Firestore client) so the phone-verified portal actually works. Hardened the P0 test fake to expose .db, mirroring the real accessor (its absence is why the 500 shipped). - Chat leads (P1b): crm_tools.save_lead (the AI agent's lead tool) only logged to stdout + an ephemeral local file, then returned success -> high-intent chat leads (name+phone) were silently lost on Cloud Run. Now persists durably to Firestore and reports failure truthfully so the agent routes the customer to a human instead of falsely confirming. - Booking race (P1b): the slot-count read in the create-appointment transaction ran outside the transaction (query.stream() with no transaction=), so two concurrent bookings could both pass MAX_PER_SLOT and oversell. Now stream(transaction=transaction). Tests: hardened test_pii_endpoint_hardening (real .db accessor) + new test_lead_capture_and_booking (durable persist, fail-loud, txn-bound read). Full suite 1102 passed, ruff clean. Co-Authored-By: Claude Opus 4.8 <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: cedf161a74
ℹ️ 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".
| source="chat", | ||
| triage_notes=interest_notes, | ||
| ) | ||
| _get_lead_manager().db.collection("leads").document(lead.lead_id).set(lead.to_dict()) |
There was a problem hiding this comment.
In the GitHub workflow's Run unit tests (no Firestore/GCS) job, the existing tests/test_crm.py success-path cases call save_lead without monkeypatching _get_lead_manager. This new unconditional live Firestore write catches the ADC/credentials failure, leaves persisted false, and returns success: False, so those existing contract tests fail even though the new regression test mocks this path. Please either update those tests to inject a fake manager or keep this unit path from requiring live Firestore credentials.
Useful? React with 👍 / 👎.
| """ | ||
| try: | ||
| db = get_database() | ||
| db = get_database().db # THODatabase wrapper -> raw Firestore client |
There was a problem hiding this comment.
On Cloud Run, where the storage credentials do not have a private key, fixing this accessor lets verified requests reach the direct blob.generate_signed_url call below; the repo already has _generate_document_signed_url plus tests/test_documents_share.py covering the required ADC/IAM fallback for that exact AttributeError. Because this endpoint still calls generate_signed_url directly, valid Secure Hub document downloads will continue to 500 in the deployed environment after the Firestore read succeeds. Reuse the existing helper here before calling the portal fixed.
Useful? React with 👍 / 👎.
After save_lead was fixed to persist durably + fail-loud (it returns success only when the Firestore write succeeds), the existing test_crm success tests failed in CI (no ADC) and, run locally with ADC, wrote 'John Doe'/'Jane Smith' test leads into the live project. Stub _get_lead_manager with an in-memory fake so the success path is exercised without any real Firestore client. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b2f0fe71c
ℹ️ 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".
| user_id="chat", | ||
| session_id="", |
There was a problem hiding this comment.
When save_lead runs from a /run chat that also has captured preferences or homes, this hard-coded user_id/empty session_id means main.py:1316 cannot find the just-saved contact lead by the actual session and will fall through to creating a second chat lead at main.py:1334 without the customer's phone/name. That leaves the durable contact lead detached from chat history and preference updates, so CRM triage sees split/incomplete records for the same customer.
Useful? React with 👍 / 👎.
1 participant
Secure Hub portal fix + durable chat leads + booking race (P0b/P1b)
Holistic review, batch 3 — makes a broken feature work and stops two silent data-loss paths.
Secure Hub portal (P0b) —
get_secure_hub_deal/ download calledget_database().collection(...), butget_database()returns theTHODatabasewrapper (no.collection) → every call 500'd. The portal was non-functional (and therefore not actually leaking PII despite looking vulnerable). Now usesget_database().db(raw Firestore client) so the phone-verified portal actually works. The P0 test fake now exposes.db, mirroring the real accessor (its absence is why the 500 shipped).Chat leads (P1b) —
crm_tools.save_lead(the AI agent's lead tool) only logged to stdout + an ephemeral local file, then returned success → high-intent chat leads (name+phone) were silently lost on Cloud Run. Now persists durably to Firestore and reports failure truthfully so the agent routes the customer to a human instead of falsely confirming.Booking race (P1b) — the slot-count read in the create-appointment transaction ran outside the transaction (
stream()with notransaction=), so two concurrent bookings could both passMAX_PER_SLOTand oversell. Nowstream(transaction=transaction).Verification: hardened
test_pii_endpoint_hardening(real.dbaccessor) + newtest_lead_capture_and_booking(durable persist, fail-loud, txn-bound read). Full suite 1102 passed, ruff clean.