Skip to content

fix(flow): treat an empty tenant as unattributed, not as a tenant - #469

Merged
lopadova merged 1 commit into
mainfrom
fix/flow-tenant-backfill-empty-string
Sep 1, 2026
Merged

fix(flow): treat an empty tenant as unattributed, not as a tenant#469
lopadova merged 1 commit into
mainfrom
fix/flow-tenant-backfill-empty-string

Conversation

@lopadova

@lopadova lopadova commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Found by cloud review on #468. The finding is about code already on main, so it gets its own PR rather than being smuggled into a sync commit — patching it inside #468 would make develop diverge from main on the very files that just conflicted.

The defect

backfillFromRuns() bucketed on (string) $run->tenant_id with no validation, and tightenToHostShape() behind it checked only whereNull. So a run whose tenant_id is '' stamped every one of its nodes '', and the migration reported success.

That is strictly worse than the outcome this migration exists to prevent. Its whole argument for NULL over default is that NULL is "distinguishable, invisible to where tenant_id = ?, and repairable". An empty tenant is invisible to the query and invisible to the guard — undetectable rather than merely wrong, and the one value that passes every gate while leaving rows no tenant can read.

Not hypothetical: TenantScopedDashboardReads::apply() guards $tenantId === '' explicitly with whereRaw('1 = 0'). The codebase already concedes the state occurs.

The fix, in two halves

Either half alone leaves the hole open:

  • backfill trims and skips an empty tenant, so those nodes stay NULL rather than being stamped
  • guard counts empty alongside NULL, so anything reaching it either way is refused by name

Whitespace is trimmed for the same reason the empty case exists — " " is non-empty to a = '' comparison and still matches no real tenant.

The runbook had the same blind spot

This is the part I would not have caught. docs/runbooks/flow-v2-migration.md pre-flight gate 1b read:

count(*) FILTER (WHERE tenant_id IS NULL) AS untenanted_runs

An operator would have cleared that check and hit the silent mis-stamp regardless. Now WHERE tenant_id IS NULL OR btrim(tenant_id) = '', with the reasoning stated so the empty arm does not look like defensive padding to whoever trims it later.

Verification

Mutation-checked in both directions, because a guard test that cannot fail is the failure mode this whole area keeps producing:

state result
guard reverted only whitespace case fails
guard and backfill reverted both new cases fail
fix applied 12/12 green

The existing orphan-node test moves to the new message text (have no usable tenant after the backfill) — worth noting it was asserting on the old wording, so leaving the message unchanged would have been the easy way to keep it passing while saying something less accurate.

R40 local critic: 0 must-fix, 0 nit.

🤖 Generated with Claude Code

https://claude.ai/code/session_019eS9Cj8rvPK85N517Emn9h

Cloud review on PR #468 found the flow_run_nodes backfill bucketing on
`(string) $run->tenant_id` with no validation, while the guard behind it
checked only whereNull. A run whose tenant_id is '' therefore stamped every
one of its nodes '' and the migration reported success.

That is strictly worse than the outcome this migration was written to prevent.
Its whole argument for NULL over 'default' is that NULL is "distinguishable,
invisible to `where tenant_id = ?`, and repairable". An empty tenant is
invisible to the query AND invisible to the guard — undetectable rather than
merely wrong, and the one value that passes every gate while leaving rows no
tenant can read.

It is not hypothetical either: TenantScopedDashboardReads::apply() guards
`$tenantId === ''` explicitly with `whereRaw('1 = 0')`, which is the codebase
already conceding the state occurs.

Two changes, because one alone leaves half the hole open. The backfill now
trims and skips an empty tenant, so those nodes stay NULL instead of being
stamped; the guard now counts empty alongside NULL, so anything that reaches
it either way is refused by name. Whitespace is trimmed for the same reason
the empty case exists — "   " is non-empty to a `= ''` comparison and still
matches no real tenant.

The runbook carried the identical blind spot in its pre-flight gate
(`count(*) FILTER (WHERE tenant_id IS NULL)`), so an operator would have
cleared that check and hit the silent mis-stamp anyway. Fixed there too.

Mutation-checked in both directions: reverting the guard alone leaves the
whitespace case failing, reverting both leaves both new cases failing, and
the fix turns both green. The existing orphan-node test moves to the new
message text.
@lopadova lopadova added the security Security hardening and review label Sep 1, 2026
@lopadova
lopadova requested a lite review from Copilot September 1, 2026 06:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The migration now fails closed on empty/blank tenants, and the behavior is covered by targeted feature tests plus aligned operational documentation.

Pull request overview

This PR fixes a data-integrity hole in the flow_run_nodes tenant backfill migration by treating empty/blank tenant_id values as “unattributed” (i.e., invalid) instead of letting them silently propagate into persisted nodes, and updates the operational runbook and tests to match.

Changes:

  • Migration backfill now trims flow_runs.tenant_id and skips empty results, leaving affected nodes NULL so the migration fails closed.
  • Tightening guard now refuses both NULL and empty-string tenant_id values and updates the exception message to reflect both root causes.
  • Runbook preflight SQL and migration feature tests are updated to catch empty/whitespace tenant defects and assert on the new error wording.
File summaries
File Description
database/migrations/2026_10_02_000008_add_tenant_id_to_flow_run_nodes.php Trim-and-skip empty run tenants during backfill; guard counts NULL and ''; improved failure message.
tests/Feature/Migrations/AddTenantIdToFlowRunNodesTest.php Adds explicit coverage for '' and whitespace-only run tenants and updates expected error message text.
docs/runbooks/flow-v2-migration.md Updates preflight query to include empty/blank tenants and documents why this arm is required.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@lopadova lopadova added the run-e2e Fire the gated Playwright E2E job (R46 final gate) label Sep 1, 2026
@lopadova
lopadova merged commit a543d3f into main Sep 1, 2026
12 of 16 checks passed
@lopadova
lopadova deleted the fix/flow-tenant-backfill-empty-string branch September 1, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-e2e Fire the gated Playwright E2E job (R46 final gate) security Security hardening and review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants