Skip to content

fix(jobs): restore builder identity on intro requests and route them through the one Bedrock bot - #258

Open
kwame-kka wants to merge 9 commits into
mainfrom
claude/bedrock-intro-flow-review-bu07xt
Open

fix(jobs): restore builder identity on intro requests and route them through the one Bedrock bot#258
kwame-kka wants to merge 9 commits into
mainfrom
claude/bedrock-intro-flow-review-bu07xt

Conversation

@kwame-kka

Copy link
Copy Markdown
Collaborator

Fixes the intro request cards Joanna flagged in Slack on 30 Jul — every builder request rendered from — (builder).

Visual before/after: https://claude.ai/code/artifact/a4951263-3547-498b-8ac9-a93bf53d5168

Root cause

public.users has row-level security enabled. Its only two policies are scoped to uft_readonly and ceo_dev; the app role bedrock_user holds SELECT but matches no policy, so RLS default-denies and it reads zero rows.

jobs_intro.py joined that table directly. Two things made it fail silently rather than loudly:

  • it was a LEFT join, so nothing errored;
  • trim(coalesce(first,'') || ' ' || coalesce(last,'')) yields an empty string, not NULL, which slipped past the ?? "—" fallback and printed the dash.

The data was never wrong. Queried as a role that bypasses RLS, every row resolves — the card in Joanna's screenshot is builder 428, Adedoyin Ahoton. Nothing needs backfilling.

The fix already existed in the repo: bedrock.builder_by_id() is SECURITY DEFINER, already granted to bedrock_user, and jobs.py:1395 already used it via LEFT JOIN LATERAL. This one call site had skipped the pattern — which is why there is no migration.

- LEFT JOIN public.users u ON u.user_id = ir.builder_id            -- 0 rows under RLS
+ LEFT JOIN LATERAL bedrock.builder_by_id(ir.builder_id) b ON true

What the thread asked for

Ask
Joanna — "flowing through without associated Builders" Builder resolves via the SECURITY DEFINER lookup, with a name → email → Builder #<id> fallback so a blank label is unreachable
Avni — "captured in Sputnik, just need to surface it" Builder name + cohort on the card and in the Slack DM
Avni & Jac — "we should merge the two" Sputnik asks now fan into the existing Bedrock notification path (bell + Slack DM to the connector staff)
Avni — "indicate more clearly builder vs jobs team" Dedicated tag + left border; tag text carries the signal without relying on colour

Staff asks also gained requested_by_name — they were showing raw emails because the key was absent from _staff_row.

Notification merge

New services/intro_notification_poller.py, modelled on sf_notification_poller.py, wired into main.py startup outside the Salesforce guard (it's Postgres-only and shouldn't skip when SF is down). Reuses the existing intro_request type with requester_kind: "builder", so no CHECK-constraint migration.

  • Backlog skipped structurally — the watermark seeds to now() on first run, so the 17 pending asks (oldest March) can't be retro-DMed even by accident. First deploy will log seeded watermark … skipping backlog and send nothing; that's correct.
  • Whole cycle holds FOR UPDATE on the watermark row. Several Cloud Run instances each run this loop; the existing SF poller reads its watermark unlocked and can double-notify. That race wasn't copied into new code.
  • A row whose staff_user_id is missing from staff_user_id_map is skipped but still advances the watermark, or it would pin the mark and be re-scanned forever.

Also fixed, both caught during verification rather than by tests

  • Timestamps. intro_requests.created_at is timestamp (naive); the watermark and bedrock.intro_request.created_at are timestamptz. In the poller this was fatal — the SQL comparison and created_at > newest both raise TypeError, and run_forever swallows exceptions, so it would have died every cycle silently and never delivered anything. Both sites now read as UTC.
  • The colour coding wasn't a colour. First pass used sky vs accent: oklch(0.55 0.13 245) against oklch(0.55 0.15 250) — same lightness, 5° apart in hue. Identical on screen. Builder is now amber; green/red are spoken for by Accept/Decline on the same card.

Vocabulary and status

ASK_LABELS covered 3 of the 8 values across both tables, so Slack DMs rendered the raw introductory_call. Now shared and complete. BUILDER_STATUS_MAP mapped both accepted and completedapproved, collapsing "I'll do it" into "I did it" though Sputnik's CHECK allows completed; builder asks can now be marked intro-made.

Verification

  • Both new queries run against production: intro_request_id 15 resolves to the right builder, and all 15 pending rows have a resolvable staff_email.
  • 15 new tests, including guards that this flow must use builder_by_id and must not join public.users.
  • Full suite 919 passed against a 904 baseline — same 25 pre-existing local env failures (503s, no SF/DB), plus exactly the 15 new tests.
  • tsc --noEmit clean. npm run lint isn't runnable — no eslint.config.js in the repo, fails on main too.

Preview tooling (droppable)

Three commits add a no-database local preview of the card (scripts/preview_intro_requests.py, plus a Node-only equivalent for machines without Python 3.10+). Dev tooling, not part of the fix — happy to strip them if you'd rather keep the PR to the change itself. Sample data is invented; the real rows carry a builder's email and their request text, which shouldn't be committed.

Known gaps

  • Staff still can't see what the builder built. Every request carries builder_preparation, a live demo_url and readiness_checks — none surfaced. Arguably the rest of what "without associated Builders" means; left out only because this round was scoped to the name, the bot and the colour. Specced in tasks/builder-intro-requests-plan.md.
  • Builders aren't notified back. Verified: four builders miss by both email and display name in the Bedrock bot's Slack workspace while staff resolve immediately, and slack_user_cache holds 0 builders. Sputnik keeps builder-facing comms.
  • Sputnik may still post to its own channel — if so, staff hear each ask twice. Separate codebase; needs a decision, not a code change here.
  • jobs.py:713/743 and sputnik.py:109 hit the same RLS trap but degrade to Builder #N, so cosmetic. The SF poller's unlocked watermark read has the same race, left as-is to keep this scoped.

Generated by Claude Code

claude added 9 commits July 31, 2026 20:32
Root-caused the "from — (builder)" cards Joanna flagged: public.users has
RLS enabled and bedrock_user has no matching policy, so the naive
LEFT JOIN in jobs_intro.py reads zero rows and fails silently. trim() over
coalesce'd NULLs yields an empty string rather than NULL, which the card
renders as an em dash.

The repo already has bedrock.builder_by_id() (SECURITY DEFINER, granted to
bedrock_user) and jobs.py:1395 already uses it via LEFT JOIN LATERAL —
jobs_intro.py is the one call site that skipped the pattern.

Plan also covers the rich Sputnik fields never selected (builder_preparation,
demo_url, readiness_checks), builder-vs-jobs-team colour coding, merging the
Sputnik notification path onto the Bedrock poller, and the ask-label/status
vocabulary drift between the two tables.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
- Clarify that a "Bedrock DM" is just a Slack DM from the Bedrock app; the
  bell row and the DM are the two surfaces enqueue_notification produces.
- Builders are NOT in the Bedrock bot's Slack workspace: four builders miss
  by both email and display name while avni@pursuit.org resolves, and
  slack_user_cache holds 0 builders. DMs to builders would silently mark
  skipped/no_slack_id. Notifying builders back is deferred, not required —
  the connector staff who must act is reachable today.
- Scope Sputnik correctly: separate app, not in this repo; we only read its
  tables. The duplicate-notification risk is a coordination note, not a
  blocker.
- Backlog decided: seed the watermark at deploy, no retroactive DMs.
- demo_feedback/other are specific_ask enum values, not columns.
  demo_feedback has never been used in 24 rows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
…through the Bedrock bot

Builder intro requests from Sputnik were reaching Bedrock with no builder
attached — every card read "from — (builder)".

Root cause: public.users has RLS enabled and the app role (bedrock_user) has
SELECT but no policy, so it reads zero rows. jobs_intro.py joined that table
directly, and because it was a LEFT JOIN it failed silently; trim() over
coalesce'd NULLs yields '' rather than NULL, which the card rendered as an em
dash. The data was never wrong — every row resolves under a role that bypasses
RLS. bedrock.builder_by_id (SECURITY DEFINER, already granted) is the
established pattern and jobs.py already used it; this call site had skipped it.

- Builder identity via LEFT JOIN LATERAL bedrock.builder_by_id, plus a
  name → email → "Builder #<id>" fallback so a blank label is unreachable.
- Staff asks gain requested_by_name too; they were showing raw emails because
  the key was absent from _staff_row.
- New intro_notification_poller fans Sputnik asks into the existing Bedrock
  notification path (bell + Slack DM) so one bot carries both kinds of ask.
  Watermark seeds to now on first run, so the 17-row backlog is not retro-DMed.
  The cycle holds FOR UPDATE on the watermark row — several Cloud Run instances
  each run this loop, and an unlocked read lets two of them notify twice.
  Started outside the Salesforce guard since it is Postgres-only.
- Requester is now shown by a dedicated Builder/Jobs team tag and a left
  border, instead of being smuggled into the ask-type tag's colour variant.
  The tag text carries the signal without relying on colour.
- Ask labels cover all eight values across both tables; the Slack DM was
  rendering the raw introductory_call. BUILDER_STATUS_MAP no longer collapses
  completed into approved, and builder asks can now be marked intro-made.

No migrations required. 13 new tests, including guards that this flow must not
join public.users directly. Full suite 917 passed against a 904-passed
baseline, same 25 pre-existing env failures; tsc clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
public.intro_requests.created_at is `timestamp` (naive) while
bedrock.notification_watermark.last_seen and bedrock.intro_request.created_at
are `timestamptz`. asyncpg therefore returns a naive datetime on one side and
an aware one on the other.

In the poller this was fatal: both `WHERE ir.created_at > $1` (aware param
against a naive column) and the `r["created_at"] > newest` comparison raise
TypeError, so every cycle would have died inside run_forever's guard and no
builder ask would ever have been delivered. The fakes could not catch it —
FakeConn does not enforce asyncpg's typing — so the guard is an assertion on
the SQL instead.

On the read path it was cosmetic but wrong: builder rows serialised with no
UTC offset while staff rows carried one, so the browser parsed builder dates
as local time and relative days could be off near boundaries.

The database runs in UTC and these values are UTC, so both sites now read them
with AT TIME ZONE 'UTC' and Python stays uniformly tz-aware. Verified the cast
against production. 15 tests; full suite 919 passed against a 904 baseline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
…hable

Caught rendering the card locally: --sky is oklch(0.55 0.13 245) and --accent
is oklch(0.55 0.15 250) — identical lightness, five degrees apart in hue. Side
by side the two source tags read as the same blue, which defeats the point of
colour-coding them. green and red are spoken for by the Accept/Decline actions
on the same card, so builder asks now use amber.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
Mounts the real routes/jobs_intro.py router behind a stub connection so the
Jobs > Intro requests card can be rendered locally without a database, and
MODE=before/after shows the RLS failure mode against the fix.

Rows are invented rather than copied from production — the real ones carry a
builder's email and the text of their intro request, which does not belong in
the repo.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
db.py annotates asyncpg.Pool | None at module scope, so anything importing it
dies with a TypeError from an unrelated line on 3.9 — macOS's default python3.
Check the version up front and print the fix instead. Production runs 3.11 per
the Dockerfile.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
The Python preview needs 3.10+ (db.py annotates asyncpg.Pool | None at module
scope) which stock macOS does not ship, so previewing a frontend change meant
installing a second toolchain. Node is already required for the frontend, so
this serves the same payloads with no dependencies.

Payloads are recorded verbatim from the real router rather than invented; only
the timestamps are recomputed so relative dates stay sensible. Verified it
renders identically to the Python version.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
Landing on http://localhost:4200 redirects to /dashboard, which threw
"(q.data ?? []).filter is not a function" and rendered a white page — the
stub returned a {success, data} envelope for every route, but the Salesforce
endpoints return bare arrays and the notification badge reads data.data.count.

Shape the catch-all per endpoint family so any entry point renders instead of
only /jobs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XthvkMYPKTHgm86akR5zBB
Comment on lines 178 to 201
f"""
SELECT ir.intro_request_id, ir.contact_id, ir.contact_name, ir.contact_company,
ir.contact_title, ir.specific_ask, ir.request_context, ir.status,
ir.staff_response_notes, ir.responded_at, ir.created_at,
trim(coalesce(u.first_name,'') || ' ' || coalesce(u.last_name,'')) AS builder_name,
u.email AS builder_email
-- These are `timestamp` (naive) here but `timestamptz` on
-- bedrock.intro_request, so without the cast the two sources
-- serialise differently and the browser reads builder dates
-- as local time. The DB runs in UTC.
ir.staff_response_notes,
ir.responded_at AT TIME ZONE 'UTC' AS responded_at,
ir.created_at AT TIME ZONE 'UTC' AS created_at,
ir.builder_id,
b.full_name AS builder_name, b.email AS builder_email,
b.cohort AS builder_cohort
FROM public.intro_requests ir
LEFT JOIN public.users u ON u.user_id = ir.builder_id
-- public.users is RLS-scoped away from the app role (bedrock_user
-- has SELECT but no policy), so joining it directly returned zero
-- rows *silently* — a LEFT JOIN, so no error, and trim() over
-- coalesce'd NULLs yields '' rather than NULL. Every builder ask
-- rendered as "from —". bedrock.builder_by_id is SECURITY DEFINER
-- and already granted to bedrock_user; jobs.py uses the same shape.
LEFT JOIN LATERAL bedrock.builder_by_id(ir.builder_id) b ON true
WHERE ir.staff_user_id = $1{open_builder}
ORDER BY ir.created_at DESC
""", sid)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Accepted builder intro asks disappear from the default inbox before the new 'Mark intro made' action can be used, because the builder-row filter (open_builder) only keeps status = 'pending', unlike the staff-row filter (open_staff) which also keeps 'accepted'.

Extended reasoning...

routes/jobs_intro.py builds two different "still open" filters for the two sources that feed the intro-requests inbox. For staff→staff asks (bedrock.intro_request), line ~176 defines:

open_staff = "" if include_closed else " AND ir.status IN ('pending','accepted')"

For Sputnik builder asks (public.intro_requests), a few lines later:

open_builder = "" if include_closed else " AND ir.status = 'pending'"

open_staff deliberately keeps 'accepted' rows visible in the default (non-closed) view, because an accepted-but-not-yet-completed ask is still an actionable, open item — the connector still has to make the intro. open_builder only kept 'pending' — an asymmetry that was harmless before this PR because a builder ask could never be marked complete anyway (see below).

This PR changes that. It fixes BUILDER_STATUS_MAP so completed maps to completed instead of collapsing into approved (previously both accepted and completed mapped to approved, losing the distinction), and it removes the r.source === "staff" gate on the "Mark intro made" button in JobsHome.tsx, using isAccepted = r.status === "accepted" || r.status === "approved" to decide when to show it. The stated intent (in the PR description) is explicit: "builder asks can now be marked intro-made."

But the moment a staff member accepts a builder ask, respond_intro_request maps acceptedBUILDER_STATUS_MAP["accepted"] = "approved" and writes status='approved' into public.intro_requests. On the very next refetch of the default inbox (useIntroRequests('all', showClosed=false)include_closed=false → the open_builder clause), that row no longer matches ir.status = 'pending', so it's excluded from the response entirely. The card disappears from the staff member's "For you" list — along with the newly-enabled "Mark intro made" button that would let them close the loop.

The only way to reach the button again is to toggle "show closed", but that flips include_closed=true and pulls in every declined/completed ask too, defeating the purpose of a scoped default view. So the feature this PR set out to ship (staff can accept a builder's ask and later mark the intro made) is unreachable along its intended path — a staff member who accepts an ask immediately loses the ability to act on it from the default screen, and has to know to dig through the closed list to find it again.

Step-by-step proof:

  1. A builder ask arrives with status = 'pending' in public.intro_requests. It's fetched by open_builder's 'pending' check and shows up under "For you".
  2. Staff clicks Accept → frontend calls PATCH /api/jobs/intro-requests/{id} with status: 'accepted', source: 'builder'.
  3. respond_intro_request looks up BUILDER_STATUS_MAP['accepted']'approved', and runs UPDATE public.intro_requests SET status='approved' ....
  4. React Query invalidates ["jobs", "intro-requests"] and refetches with the same box='all', include_closed=false params used by IntroRequestsZone.
  5. The backend's builder query runs with open_builder = " AND ir.status = 'pending'"; the row's status is now 'approved', so it fails this predicate and is dropped from data.
  6. The card vanishes from both "For you" and the count. The "Mark intro made" button — which the frontend would otherwise render because isAccepted includes 'approved' — never gets a chance to render, because the row isn't in the payload at all.

This is a real functional gap introduced by this PR's own feature (it didn't exist before because builder asks couldn't be completed at all, so there was nothing to lose visibility of). The fix is small and mirrors the existing staff-side pattern: include 'approved' alongside 'pending' in open_builder, e.g. " AND ir.status IN ('pending','approved')", so an accepted-but-not-completed builder ask stays visible in the default view exactly like an accepted staff ask does.

Comment on lines +313 to +320
# Builder asks originate in Sputnik and are surfaced by
# intro_notification_poller; staff→staff asks are created in Bedrock.
# Say which, so one bot can carry both without ambiguity.
if payload.get("requester_kind") == "builder":
headline = f":raising_hand: *{actor}* (builder) asked you for an intro"
if payload.get("builder_cohort"):
section_lines.append(f"*Cohort:* {payload['builder_cohort']}")
else:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The Slack DM and Jobs card now clearly mark builder-sourced intro requests (a (builder) tag + cohort line), but NotificationBell.tsx's intro_request branch was never updated to match — it still renders every intro-request bell item with the generic ${actor} asked you for an intro headline regardless of requester_kind, and never reads builder_cohort. Since this PR is what first makes Sputnik builder asks reach the bell at all (via the new poller), staff will now see builder-sourced bell items rendered indistinguishably from staff ones — a minor, easily-fixed gap (the builder's real name still shows via actor_display_name, and the primary surfaces — the Jobs card and Slack DM — both correctly carry the distinction).

Extended reasoning...

_format_slack_message() in services/notifications.py (lines 313-320) now branches on payload.get("requester_kind") == "builder" for TYPE_INTRO_REQUEST, prepending a :raising_hand: (builder) marker to the headline and adding a *Cohort:* line when builder_cohort is present. This is new, PR-scoped logic added specifically to satisfy Avni's ask to "indicate more clearly builder vs jobs team." The frontend counterpart of the same notification, NotificationBell.tsx's intro_request branch (around lines 271-276), was not touched by this PR and still unconditionally renders ${actor} asked you for an intro, never inspecting payload.requester_kind or payload.builder_cohort.

Before this PR, that mismatch was latent and harmless: services/intro_notification_poller.py did not exist, so no Sputnik builder ask ever produced a bedrock.notification row, and the bell's intro_request branch only ever rendered staff-sourced payloads (created by routes/jobs_intro.py's staff-to-staff flow) where requester_kind was always "staff". This PR changes that: the new poller (_poll_builder_intros) now calls enqueue_notification(... payload=_payload(r)) with requester_kind: "builder" for every new Sputnik ask, which lands a bell row alongside the Slack DM. So the PR is what actually exercises this code path with builder payloads for the first time, and the pre-existing bell code silently fails to distinguish them.

The concrete effect: a staff member opens the notification bell after being pinged about a builder intro request, and sees the exact same "Jordan Reyes asked you for an intro" style line they would see for a staff-originated ask — no (builder) marker, no cohort. Notably, NotificationBell.tsx line 234 carries a comment stating the bell is meant to "mirror the Slack message layout," so this is a case where the two surfaces were explicitly designed to stay in sync, and this PR updated one half without the other.

Impact is limited, though. payload.actor_display_name (set by the poller via _builder_display()) still flows through to the bell's actor display, so the staff member does see the builder's real name, not something generic — they just don't get the extra "this is a builder, here's their cohort" framing that the Slack DM and the Jobs card (JobsHome.tsx, dedicated Builder/Jobs team tag + left border + cohort) both now provide. Nothing crashes, no data is lost, and the two surfaces that were the explicit target of Avni's ask (card + Slack) are both correct.

Proof, step by step: (1) A Sputnik builder makes an intro ask; it lands in public.intro_requests. (2) intro_notification_poller._poll_builder_intros picks it up on its next cycle and calls enqueue_notification(conn, recipient_email=staff_email, type=TYPE_INTRO_REQUEST, actor_email=builder_email, payload=_payload(r)), where _payload() sets requester_kind: "builder", actor_display_name: <builder name>, builder_cohort: <cohort>. (3) enqueue_notification inserts a bedrock.notification row with that payload and fires _dispatch_slack, which calls _format_slack_message(TYPE_INTRO_REQUEST, payload, actor_email) — this correctly branches on requester_kind == "builder" and renders the (builder) marker plus cohort line in the Slack DM. (4) Separately, the staff member's Bedrock frontend polls GET /api/notifications, gets back the same row with requester_kind: "builder" in its payload, and renders it through NotificationBell.tsx's intro_request case, which builds the headline ${actor} asked you for an intro without ever reading payload.requester_kind or payload.builder_cohort — so the bell item looks identical to a staff-sourced one, even though the Slack DM the same person got moments earlier correctly said "(builder)" and listed the cohort.

Fix is small and localized to the frontend: add the same requester_kind === "builder" branch to NotificationBell.tsx's intro_request case that already exists in _format_slack_message(), rendering a (builder) suffix/tag and an optional cohort line when present — mirroring the Slack formatting exactly as the existing "mirror the Slack layout" comment intends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants