chore: sync develop with main (#460, #461, #462, #463, #465, #466, #467) - #468
Merged
Conversation
) * test(mcp): derive the tool roster from disk instead of counting it A hard-coded total is the same number written in three places - the method name, a comment's arithmetic, and the assertion - and keeping them in step is a discipline that kept failing. 47 outlived a 48-tool roster and held main red for four days, develop for six, and blocked fifteen Dependabot pull requests. Even the fix drifted: main currently asserts 50 under a method named forty_nine. The comparison is now against the FILES in app/Mcp/Tools, so it cannot go stale. Add a tool without registering it and it fails; register a class whose file is gone and the existing source-file test fails. Neither asks anybody to remember a number. Verified by introducing an unregistered tool file: both assertions fail with messages naming the divergence, and pass again once removed. The three vendor-namespaced invitations tools stay listed explicitly, because no file in this repository corresponds to them - and naming them means a fourth has to be acknowledged rather than absorbed into a total. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * docs: stop repeating a roster count the test no longer asserts Review caught that I removed the hard-coded number from the TEST and left it in CLAUDE.md and the composer suggest string - the same drift, one level up, and the more misleading half, since documentation is what people read when they are not running the suite. Both now describe the roster as whatever lives under app/Mcp/Tools plus the vendor tools, with no total to maintain. The suggest string also advertised laravel/mcp ^0.7 while require-dev pins ^0.9, which would have told an operator the wrong supported range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ADR 0028 specified default OFF for this phase and the product owner has reversed it. The reversal is recorded in the ADR itself, not left as a discrepancy between a document and a config file - a default that contradicts its own ADR is the drift that file exists to prevent. The ADR's caution stands and upgraders should know it: a deployment already ingesting email will find that turns grounded in a message stop being able to act. What decided it the other way is that a security control shipping OFF protects nobody until somebody remembers to switch it on, and the chain it closes is not hypothetical - IMAP ingests what anyone can send, ingested content becomes grounding, and the same platform exposes tools to the model, with no boundary between them. The failure direction is the safe one: the turn declines to act rather than acting on a stranger's instructions, and the answer still comes back from the same context with the same citations. Setting KB_PROVENANCE_TOOL_FIREWALL=false restores the previous behaviour exactly. Six places move together - config default, the service's fallback, .env.example, the ADR, the doc-site page and the README - because a default documented in five places and changed in one is how this kind of thing goes wrong. The test that asserted the shipped default now asserts ON rather than being deleted: the default IS the decision, so it is worth a test of its own. Both states stay covered (R43). 1337 tests green across the KB, API and MCP suites. Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(ai): move the platform to laravel/ai 0.11 The pin guard did its job: it failed this bump and forced the provider compatibility pass it exists to force. That pass is why the pin moved rather than the regex. 0.11's breaking change lands entirely BELOW where this application works. It inverts the multi-step tool loop - TextGateway becomes StepTextGateway, a gateway performs one step and the SDK owns the loop - and the host touches no gateway class at all. Its whole SDK surface is AnonymousAgent, Embeddings, Messages\*, Responses\*, TextGenerationOptions, Enums\Lab and Contracts\HasProviderOptions, none of which changed. The inversion was padosoft/laravel-ai-regolo's problem, and it is migrated in v2.0.0 - the loop deleted rather than duplicated, since two implementations of when a conversation ends would disagree the first time the SDK changed its mind. laravel-ai-guardrails v1.6.0 came through unchanged: it only touches Contracts\Tool. Both releases exist because composer refused this resolution while they pinned ^0.8, which was the correct behaviour and the reason the bump was deferred rather than forced. The pin test now locks the 0.11 line on the same terms, with the compatibility pass recorded in its docblock so the next person to move it knows what the guard is asking for. 154 AI tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * test(ai): say 0.11 everywhere the pin guard talks about a version Fourth time today the same defect class, and this time in my own change: the assertions moved to 0.11 while three inline comments still described the 0.8 line, and the header still called itself the guard for the 0.8 migration. That is exactly what kept a stale 47 alive against a 48-tool roster - a number corrected in some of the places it lives and left as fiction in the others. A guard whose comments contradict its assertions is worse than one with no comments, because the comments are what a reader trusts when deciding whether the guard still means what it says. The v8.19 history stays: it is accurate and it explains why the guard exists. It is now labelled as history rather than reading as the current state. 147 AI tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
My own oversight: I released beta.4 yesterday and never updated the
host lock, so AskMyDocs stayed on beta.3 while the fixes sat unused.
The constraint ^1.0@beta already allowed it - nothing was blocking it
except the update never being run.
Five commits, and one of them matters beyond housekeeping: produceConfig()
and testConfig() gated on isJson alone, so a 401 returning
{"message":"Unauthorized"} decoded fine, went to the analyst, and came
back as a GENERATED endpoint configuration - a confident description of
a call that never worked. beta.3 has that behaviour; beta.4 requires a
successful response.
Also arriving: ApiRouteRelation and the fail-closed-on-redirects work,
which beta.3 never had (it was cut from the wrong branch), plus the
cursor parameter now being read from the route instead of hard-coded,
and the surviving peer's tool definition being refreshed when a related
route is deleted.
321 tests run locally; the single failure is the known Windows-only
dataset generator issue (mkdir path handling in
demo:generate-case-study-emails), unrelated to this bump and absent on
Linux CI.
Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
v2 replaces the per-step table with a run-node graph: flow_steps becomes flow_run_nodes, StepRunRepository becomes RunNodeRepository, and FlowStepRecord is gone. AskMyDocs implements the persistence contracts itself — to add the tenant boundary the package deliberately does not have — so it is in the minority of consumers the rename actually touches. The surface is smaller than it looks. FlowStepHandler, FlowCompensator, FlowContext and FlowStepResult are byte-identical between v1.1.1 and v2.5.0, and Flow::execute() still runs the linear executor, which still populates stepOutputs. So the thirty step handlers, four compensators, nine definitions and the unit tests over them are untouched. Four things would have been fatal rather than merely broken: - FlowStepRecord::creating() in the provider is a class-not-found at boot under v2. - AskMyDocsFlowAuthorizer implemented eight of the nine ActionAuthorizer methods; the missing canEditDefinition is a fatal at class load. It refuses the empty flow name deliberately — the advisor's all-flows scan reaches the package helper with no flowName and passes '', and reading that as an unscoped allow would leave the one action that writes drafts across every flow with nothing to authorize against. - flow_runs.subject is written unconditionally on every run insert with no column guard, so omitting its migration fails every execute(). - flow_run_nodes keeps unique(run_id, node_id) rather than the R31 tenant-prefixed shape, because createOrUpdate upserts with ON CONFLICT on exactly those columns and errors without a matching index. run_id is a UUID owned by one tenant, so the pair is already transitively tenant-disjoint. The tenant on converted history is the real risk, and it is not in the code. Neither the package blueprint nor the package's conversion migration provides tenant_id, and the conversion drops its own source table. The host migration therefore adds the column NULLABLE WITH NO DEFAULT, backfills it from each node's run, and only then tightens to the R31 shape. A default here would make every tenant's history readable by the default tenant and be undetectable afterwards, since a legitimately-default row and a mis-stamped one are identical. NULL is invisible to a scoped read, and repairable. Running that migration last also removes a driver hazard rather than managing it: the package conversion's insertOrIgnore cannot swallow a NOT NULL violation on a column that does not exist yet. Separately, this closes a pre-existing cross-tenant read on the cockpit. Its authorizer gates destructive actions per row, but the run LIST is a plain role check, and v2 documents the canView* hooks as reserved and not invoked. TenantScopedDashboardReads constrains every dashboard query through the read scope released in laravel-flow 2.5.0. It resolves the tenant inside apply() rather than capturing it, because the read model is a singleton and a captured tenant would outlive its request. New coverage where there was none: the persistence decorator had no direct tests at all, and the migration's backfill has five, including its refusal to guess a tenant for a node whose run is missing. Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ease (#467) * chore(deps): pin @types/node to the runtime floor, not the newest release Dependabot opened #464 to take @types/node from 24.x to 26.x. Copilot flagged it, and the repo confirms the concern rather than the PR diff: package.json engines.node = ">=24.15.0" tests.yml node-version: '24.15.0' (all three jobs) package.json @types/node = ^24.10.1 Type definitions two majors ahead of the runtime are worse than merely useless. A Node-26-only API typechecks cleanly in tooling code, ships, and throws under the Node 24 the app actually runs on — which is precisely the failure the type checker exists to prevent, and one no CI job would catch because CI runs 24 too. The types would be quietly describing a runtime nobody has. This is the same rule already applied on the PHP side, where config.platform.php is pinned to the CI floor so Composer resolves against the version CI executes. Types track the floor; they do not lead it. The ignore is scoped to semver-major only, so 24.x minor and patch updates keep flowing normally, and it carries the condition that lifts it: move engines.node, the three node-version pins and @types/node together, in one change. #464 is closed against this. * ci: re-run with the run-e2e label already applied Diagnostic, and it will be squashed away on merge. Adding the label fired a fresh workflow run twice (04:27:45 and 04:46:53) and neither created the Playwright job — not even as a skipped check, which the pre-label run did create. Same workflow path, same head SHA, same github.event_name; only the event TYPE differs (opened vs labeled). Pushing with the label already present produces a synchronize event, which separates the two candidates: if Playwright runs here, the label arm of the R46 gate is broken specifically for labeled events; if it does not, the if expression itself never matches and the final gate has never actually run from a label.
* feat(flow): close the R32 and R44 gaps on the flow surface R32 — role access to the cockpit was inferred, not observed. Three tests already covered most of the chain: FlowAdminMountingTest asserts every package route carries `can:viewFlowAdmin`, FlowAdminGatesTest asserts the gate admits the right roles, FlowAdminDisabledTest covers the off state. None of them drove a request per role and checked what came back, and middleware-attached plus gate-defined is a strong inference rather than an observation. The first version of that test was theatre. It flipped the feature flag in the test body, but routes are registered at boot from that config, so the cockpit never mounted and every request fell through to the SPA catch-all — which answered plausibly enough that the assertions passed while proving nothing. Removing `can:viewFlowAdmin` from the config left it green. The rewrite flips the flag in getEnvironmentSetUp, and the same mutation now fails exactly the two denied-role cases. R44 — flow had no MCP surface and no documented exception. FlowRunStatusTool reads through the SAME FlowDashboardReadModel the cockpit uses rather than querying the tables, so the host tenant scope wired onto that model applies here too instead of being re-derived — which is where it would eventually drift. Read-only by construction: the read model exposes no writes, so starting, cancelling, replaying and approving stay behind the per-row authorizer where a human is present. Its OFF path reports `persistence_enabled: false` rather than an empty list, because flow persistence is opt-in and default-off: a caller has to be able to tell "nothing ran" from "nothing is recorded". Both halves are checked — the flag can be on while the tables are absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * docs(flow): close the R45 gap and write the v2 migration runbook The flow surface had no doc-site page at all, and the v2 move edited README schema names without adding one — an R45 gap on both counts. This adds the deep page and the operator procedure the migration needs, and corrects what the README had drifted into claiming. docs-site/flow-orchestration.mdx argues the saga from the failure it exists to prevent: ingesting one document is six writes across a database, a disk and a queue, and a transaction covers exactly the third of that which was never the problem. From there — compensation semantics, the nine definitions with their real step counts, the tenant boundary the package deliberately does not have and the four host classes that add it, the role matrix, the three surfaces over one read model, and the v1 to v2 move including why the tenant column on converted history is added nullable with no default. Seven claims were wrong on the first pass, and they are worth naming because each was plausible enough to ship: - kb.ingest is 6 steps, not 5. The README said 5 too. - kb.prune-embedding-cache is 3 steps with its gate inside AssessEmbeddingEvictionRiskStep, not 2 steps plus an approvalGate(). The primitive pauses unconditionally, and a gate that stops every nightly sweep to ask about two hundred rows is one operators learn to approve without reading. - The ingest compensator deletes the document row, chunks cascading behind it, and deliberately KEEPS the file on disk via deleteDbOnly() — an earlier version wiped it, so a transient indexer-dispatch failure destroyed a source-of-truth file over a Git mirror. - A step's compensator fires for a LATER step's failure, never its own. The original text implied otherwise, which makes the promote gate unreadable. - The host DECORATES the package store with $this->app->extend() rather than replacing it, and extend-not-rebind is load-bearing: laravel-flow and laravel-flow-admin both bind FlowDashboardReadModel, so a third bind() would race their registration order. - The KPI strip does not count failed compensations. It counts compensated runs; a compensation that itself failed lands in flow_runs.compensation_status, which is the more useful fact — a dashboard watching only `failed` misses the one case where the world was left half-unwound. - The worked example passed source_path, which PromotionFlow does not declare, and called flow:forensics with --run= instead of the positional argument. Both would have failed for the first person who copied them. The step counts came from a grep that missed `->step(self::CONST, ...)`; the rest from reading the classes rather than trusting the shape they suggested. docs/runbooks/flow-v2-migration.md is the deploy procedure. The conversion is forward-only — down() is a no-op and flow_steps is dropped — so the pg_dump is the only rollback and the runbook is built around that. Pre-flight gates size the window and, in the multi-tenant case, record the per-tenant run counts the post-migration verification checks against. Two of those gates were softened after reading the schema rather than assuming. Orphan steps are prevented by a foreign key with ON DELETE CASCADE, so that query became a defensive confirmation with an explanation of when the constraint might not have held. The duplicate-step check was dropped outright, because flow_steps is unique on (run_id, step_name) and the conversion's insertOrIgnore therefore has nothing to silently drop — a runbook that asks operators to check impossible conditions teaches them to skim the list. What replaced it is the work actually in flight: pending approval tokens whose expiry falls inside the window are dead on the other side. The window keeps persistence ENABLED throughout. Turning it off does not make the conversion safer, it removes the post-deploy signal that runs are being recorded again, and a deploy that forgets to turn it back on stops recording silently. README carried four stale version claims (laravel-flow v1.0 against a v2.5 composer constraint, flow-admin v1.0.0 against v2.4) and a persistence box whose right wall had moved four columns out when flow_run_nodes replaced the shorter flow_steps; the box floor was one column off before that. CLAUDE.md gains FlowRunStatusTool in the MCP roster enumeration. * fix(mcp): stop advertising a flow name that does not exist Cloud review caught FlowRunStatusTool telling MCP consumers to filter on 'kb.ingest-document'. No definition has ever had that name — the real one is `kb.ingest`, and the nine names are `kb.*` constants on the definition classes. Nothing failed, and that is the point. A schema description is not decoration: it is the surface an agent reads to decide what to send. A wrong value there produces an empty result the caller cannot distinguish from "no runs matched", so the failure is silent on both ends. The examples are now the definition constants themselves rather than string literals, so a rename is a compile error instead of stale prose. That fixes half of it. A constant is only as good as the register() call that puts it in the engine, so RegisteredDefinitionNamesTest closes the other half in three directions: every declared definition is registered, the engine registers nothing the host does not declare (which is what would make the doc-site table stale), and every `kb.*` name the rendered tool advertises resolves to a registered flow. That last one reads the description off `toArray()` — the wire format an MCP client actually receives — rather than out of the schema builder, so it asserts against what the caller sees and not an intermediate the framework is free to change. It is mutation-checked: putting 'kb.ingest-document' back fails it with "The MCP tool tells callers to filter on [kb.ingest-document], which is not a registered flow." The first attempt at that mutation silently did not apply, and the green run that produced would have been exactly the kind of proof-of-nothing this test exists to prevent. * fix(mcp): stop the two response branches disagreeing about which keys exist Cloud review flagged the disabled branch as a different shape. It was worse than reported, in both directions: `matching_runs` was missing when persistence was off, and `note` was missing when it was on. A caller could not parse one shape — it had to learn which keys existed from the path the request happened to take, which is the R27 failure ("never strip keys based on path"). Both branches now emit the same key set. The values still differ, and one of them deliberately does not follow the review's suggestion: the counts stay NULL when nothing is recorded rather than becoming zero. Zero is a measurement. A dashboard renders "0 failed" as healthy, which is the exact opposite of what an unrecorded corpus means, and `persistence_enabled` only helps a consumer that thought to read it. Null cannot be mistaken for a number nobody measured. FlowRunStatusToolShapeTest holds both halves: the key sets are identical across branches, and the disabled branch reports unknown rather than zero — asserted explicitly so a future "tidy up the nulls" change has to argue with a test instead of quietly passing one. Mutation-checked by deleting the very key the review named; the diff names it back. The doc-site page gains the same contract, since it is what an integrator reads before writing the client. * fix(mcp): guard every table the recording branch reads, not just one Cloud review pointed out that persistenceIsRecording() checked only flow_runs while kpis() also counts pending approvals and the webhook outbox. Reproduced before fixing: narrow the guard, drop flow_approvals, and the tool takes the recording branch and throws at FlowDashboardReadModel:248 — the exact contract its own docblock says it will never break. The state is reachable rather than theoretical, which is what makes it worth fixing. flow_approvals and flow_webhook_outbox are created by a different migration (2026_05_09_145344) from flow_runs (…_145342), so "some of the flow tables exist" is an ordinary partial-migration outcome, not a corrupted database. The list is exactly what the branch reads and no more. flow_run_nodes, flow_definitions, flow_node_children and flow_node_cache are never touched here, and demanding them would refuse to report on deployments this tool can serve perfectly well — trading a crash for a different wrong answer. Covered by a data provider over the three required tables: drop each in turn, assert the tool reports not-recording instead of throwing. Mutation-checked by narrowing the constant back to flow_runs; two of the three cases error, and the stack lands on the approvals count. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
develop had drifted seven commits behind main and was carrying a superseded
fix for the same problem one of them solved, so the two files that conflicted
are exactly the two that were fixed twice.
Both resolutions take main, and not merely because it is newer.
composer.json — the only conflicting hunk is the laravel/mcp suggest text.
develop's version repeats the roster total ("a 50-tool roster: 47 under
app/Mcp/Tools/ plus 3 vendor"), which is the fourth place that number lived.
main's stopped quoting a count at all. Every dependency bump auto-merged
cleanly (laravel/ai ^0.11, guardrails ^1.6, regolo ^2.0, flow ^2.5,
flow-admin ^2.4); the package SETS were already identical on both sides, so
nothing of develop's is dropped by taking main's file.
tests/Unit/Mcp/KnowledgeBaseServerRegistrationTest.php — develop fixed the
stale-count problem by making the count consistent in the three places it
lived (a4f7d26). main fixed it by deleting the count: the test now compares
registered classes against the FILES in app/Mcp/Tools/. The second approach
subsumes the first, so keeping develop's would reintroduce exactly the
maintenance burden main removed.
The lock needed no resolution. It auto-merged to something byte-identical to
main's, content-hash included, and `composer validate` confirms it still
matches the resolved composer.json.
Verified beyond a clean merge, because a clean merge proves very little when
one side carries 109 commits the other has never seen: the disk-derived roster
test passing is direct evidence that develop's tool files and main's
registration agree, which is the single assertion this merge could most
plausibly have broken. 379 tests green across tests/Unit/Mcp,
tests/Architecture, tests/Feature/Flow, tests/Unit/Flow,
tests/Feature/Migrations, tests/Feature/Mcp and tests/Feature/Security —
the last including the R32 authorization matrix, which is what would catch a
route-gating regression from combining the two histories.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The flow_run_nodes tenant backfill migration and a new FlowRunStatusTool schema test both contain issues that can cause silent bad stamping and/or test failures under SQLite FK constraints.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Syncs develop with main by taking the main-side conflict resolutions (notably the MCP tool-roster test and its related documentation), and brings develop up to date with recent platform work (laravel-flow v2 persistence shape, new Flow MCP read surface, and provenance tool-firewall default flip).
Changes:
- MCP server tool roster validation is now disk-derived (no hard-coded total), and related docs/suggest strings are updated accordingly.
- laravel-flow v2 integration: persistence table shape moves from
flow_steps→flow_run_nodes, tenant stamping is enforced via a decorator, and flow-admin dashboard reads are tenant-scoped; adds an MCP read-onlyFlowRunStatusToolplus contract tests. - Provenance tool firewall default is flipped to ON across config, tests, ADR,
.env.example, and docs-site.
File summaries
| File | Description |
|---|---|
| tests/Unit/Mcp/KnowledgeBaseServerRegistrationTest.php | Derives host tool roster from app/Mcp/Tools/*.php and asserts vendor tools explicitly. |
| tests/Unit/Flow/Persistence/TenantAwareRunNodeRepositoryTest.php | Unit coverage for tenant stamping behavior of the run-node repository decorator. |
| tests/Unit/Ai/LaravelAiPinTest.php | Updates laravel/ai pin guard expectations to the 0.11 line. |
| tests/TestCase.php | Updates comments to reflect flow_run_nodes in flow persistence setup. |
| tests/Feature/Migrations/AddTenantIdToFlowRunNodesTest.php | Exercises the tenant backfill migration behavior against reconstructed pre-migration schema. |
| tests/Feature/Mcp/FlowRunStatusToolShapeTest.php | Locks FlowRunStatusTool response shape across enabled/disabled/missing-table branches. |
| tests/Feature/Kb/ProvenanceToolFirewallTest.php | Updates assertions to match the firewall default shipping ON. |
| tests/Feature/Flow/RegisteredDefinitionNamesTest.php | Ensures Flow definition constants are registered and MCP examples reference real names. |
| tests/Feature/Flow/IngestDocumentFlowTest.php | Updates assertions to read persisted node rows from flow_run_nodes (plus tenant/node_type checks). |
| tests/Feature/Admin/FlowAdminTenantScopingTest.php | Adds read-model scoping assertions (list/detail/kpis) for cross-tenant isolation in the cockpit. |
| tests/Feature/Admin/FlowAdminRoleAccessTest.php | Verifies cockpit web route access per role (allowed vs forbidden vs guest redirect). |
| tests/Feature/Admin/FlowAdminGatesTest.php | Expands authorizer coverage to include definition editing authorization. |
| tests/database/migrations/0001_01_01_000026_create_laravel_flow_tables.php | Updates SQLite mirror schema to v2 tables/columns and adds graph-only tables. |
| tests/Architecture/GraphExecutorNotAdoptedTest.php | Guards that the host does not invoke the v2 graph executor until tenancy decisions are made. |
| docs/v4-platform/FEATURE-CATALOG-laravel-flow.md | Updates docs to refer to flow_run_nodes rather than flow_steps. |
| docs/v4-platform/FEATURE-CATALOG-flow-admin.md | Updates flow-admin catalog references to v2 persistence table names. |
| docs/runbooks/flow-v2-migration.md | Adds operational runbook for v2 migration/conversion/backfill verification. |
| docs/adr/0028-source-acl-mirroring-and-ingest-provenance.md | Records the decision to ship the tool firewall default ON (superseding prior ADR default). |
| docs-site/tool-firewall.mdx | Updates docs-site config snippet and narrative to reflect default ON. |
| docs-site/docs.json | Adds flow-orchestration to the docs-site nav. |
| database/migrations/2026_10_02_000008_add_tenant_id_to_flow_run_nodes.php | Adds nullable tenant_id, backfills from runs, verifies, then tightens to host shape. |
| database/migrations/2026_08_23_000001_add_subject_to_flow_runs_table.php | Adds subject column to flow_runs for agent/delegated identity attribution. |
| database/migrations/2026_07_09_000012_create_flow_node_cache_table.php | Creates flow node cache table (graph executor infra). |
| database/migrations/2026_07_09_000011_add_graph_to_laravel_flow_runs.php | Adds graph column to flow_runs (graph executor infra). |
| database/migrations/2026_07_09_000010_create_flow_node_children_table.php | Creates flow node-children ledger table (graph executor infra). |
| database/migrations/2026_07_09_000009_migrate_flow_steps_to_run_nodes.php | Converts flow_steps rows into flow_run_nodes then drops flow_steps (forward-only). |
| database/migrations/2026_07_09_000008_add_graph_columns_to_laravel_flow_runs.php | Adds v2 graph KPI columns to flow_runs with per-column guards. |
| database/migrations/2026_07_09_000007_create_flow_run_nodes_table.php | Creates flow_run_nodes table (v2 persistence shape). |
| database/migrations/2026_07_08_000006_add_definition_version_to_laravel_flow_runs.php | Adds definition version/checksum lineage columns to flow_runs. |
| database/migrations/2026_07_08_000005_create_flow_definitions_table.php | Creates flow_definitions table (graph executor infra). |
| config/kb.php | Flips tool firewall default to ON and updates rationale text. |
| composer.json | Bumps core deps (laravel/ai, guardrails, regolo, flow, flow-admin) and updates MCP suggest text (no roster count). |
| CLAUDE.md | Updates MCP tool roster documentation to “derived from disk” and mentions FlowRunStatusTool. |
| app/Services/Kb/Provenance/ProvenanceToolFirewall.php | Uses default-ON semantics when config key is absent. |
| app/Services/Kb/Pii/ChunkRedactor.php | Updates comment references from flow_steps to flow_run_nodes. |
| app/Providers/FlowServiceProvider.php | Wires tenant-scoped dashboard reads via FlowDashboardReadModel scope extension; updates stamping hook to FlowRunNodeRecord. |
| app/Mcp/Tools/FlowRunStatusTool.php | Adds read-only MCP tool reporting flow run KPIs and recent runs, with consistent response shape across branches. |
| app/Mcp/Servers/KnowledgeBaseServer.php | Registers FlowRunStatusTool on the MCP server. |
| app/Flow/Persistence/TenantAwareStepRunRepository.php | Removes v1 step persistence decorator (replaced by v2 run-node decorator). |
| app/Flow/Persistence/TenantAwareRunNodeRepository.php | Adds v2 run-node persistence decorator that stamps tenant_id only on writes. |
| app/Flow/Persistence/TenantAwareFlowStore.php | Updates host FlowStore decorator to expose runNodes() instead of steps(). |
| app/Flow/Definitions/IngestDocumentFlow.php | Updates documentation comment to reference flow_run_nodes observability. |
| app/Flow/Definitions/CanonicalIndexFlow.php | Updates documentation comment to reference flow_run_nodes observability. |
| app/Flow/Admin/TenantScopedDashboardReads.php | Adds DashboardReadScope enforcing tenant_id filtering per-query without capturing tenant_id at boot. |
| app/Flow/Admin/AskMyDocsFlowAuthorizer.php | Adds canEditDefinition() gate (super-admin only; refuses empty flow name). |
| .github/dependabot.yml | Ignores semver-major @types/node bumps to match the pinned runtime floor. |
| .env.example | Flips KB_PROVENANCE_TOOL_FIREWALL default to true and updates rationale text. |
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+111
to
+133
| foreach ($runs as $run) { | ||
| $byTenant[(string) $run->tenant_id][] = $run->id; | ||
| } | ||
|
|
||
| foreach ($byTenant as $tenantId => $runIds) { | ||
| DB::table(self::TABLE) | ||
| ->whereIn('run_id', $runIds) | ||
| ->update(['tenant_id' => $tenantId]); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Reach the R31 shape, refusing to guess. | ||
| * | ||
| * A row still NULL here has no run to inherit from, which means the | ||
| * conversion produced something the FK should have prevented. Stamping it | ||
| * 'default' would hide a real inconsistency inside a tenant that can read | ||
| * it, so this throws instead and names the count. | ||
| */ | ||
| private function tightenToHostShape(): void | ||
| { | ||
| $unattributed = DB::table(self::TABLE)->whereNull('tenant_id')->count(); |
| { | ||
| config(['laravel-flow.persistence.enabled' => true]); | ||
|
|
||
| Schema::drop($missing); |
Owner
Author
|
Two findings, one confirmed and one falsified. Both are about code this PR carries over from main, not about the merge resolutions — so neither gets fixed here (see the last section). 1. Empty-string tenant in the backfill — confirmed, and worse than reportedYou are right, and the hole is bigger than the migration.
That is strictly worse than the failure the migration's own docblock is built to prevent. It argues NULL over And it is reachable by the host's own admission: You also found a hole in documentation I wrote, which I would not have caught: the migration runbook's pre-flight gate is 2.
|
Owner
Author
|
Correction to my earlier reply, so the record is accurate. I said finding 1 would reach develop "on the next sync". It will reach it in this one. The fix is now open as #469 against main, and the ordering is:
So develop receives the empty-tenant fix as part of this sync rather than trailing it by a cycle. Deferring a tenant-isolation fix to a second sync was the wrong call and I am not taking it — the whole point of the finding is that an empty tenant passes every gate while leaving rows nobody can read, and develop should not knowingly carry that for a cycle. The rest of my earlier reply stands: this PR remains a faithful sync with both conflict resolutions taking main, and finding 2 remains falsified — the |
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.
Owner
Author
|
Refreshed with main at So this sync now delivers the fix to develop rather than deferring it, as I said in the correction above. Verified on the branch rather than assumed:
The merge brought exactly the three files from #469 and conflicted with nothing, which is the expected result: the sync had taken main's side on both contested files already, so main moving forward on them is a fast-forward of the same lineage rather than a second divergence. Both original findings are now closed — finding 1 fixed and present here, finding 2 falsified (the Reminder for whoever merges: this needs a merge commit, not a squash. Squashing gives develop the content without main's ancestry, so |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new flow_run_nodes tenant backfill migration references RuntimeException without importing or fully-qualifying it, which will cause a fatal error during migration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+5
to
+8
| use Illuminate\Database\Migrations\Migration; | ||
| use Illuminate\Database\Schema\Blueprint; | ||
| use Illuminate\Support\Facades\DB; | ||
| use Illuminate\Support\Facades\Schema; |
Owner
Author
|
Falsified, and this one is provable at runtime rather than by argument. The file declares no namespace — <?php
declare(strict_types=1);
use Illuminate\Database\Migrations\Migration;Laravel migrations are anonymous classes in the global namespace, so The decisive evidence is that three tests execute that exact throw and pass: Each asserts Worth noting this is the third appearance of this finding: it was raised on #465 for the same file and verified the same way before that PR merged. The other two findings on this PR are genuinely closed — the empty-tenant one is fixed and present here via #469, and the No change to make. This PR remains a faithful sync. |
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
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.
develop had drifted seven commits behind main and was carrying a superseded fix for the same problem one of them solved — so the two files that conflicted are exactly the two that were fixed twice, independently, on both branches.
The two resolutions, and why each takes main
composer.json— the only conflicting hunk is thelaravel/mcpsuggest text. develop's version repeats the roster total ("a 50-tool roster: 47 under app/Mcp/Tools/ plus 3 vendor-namespaced"), which was the fourth place that number lived. main's stopped quoting a count at all.Every dependency bump auto-merged cleanly —
laravel/ai ^0.11,guardrails ^1.6,regolo ^2.0,flow ^2.5,flow-admin ^2.4. I checked the package sets on both sides before resolving: identical inrequireandrequire-dev, nothing only-in-develop. So taking main's file drops nothing of develop's.tests/Unit/Mcp/KnowledgeBaseServerRegistrationTest.php— this is the interesting one. Both branches fixed the same stale-count bug, differently:a4f7d26b)app/Mcp/Tools/main's subsumes develop's. Keeping develop's would reintroduce precisely the maintenance burden main removed — the one that let
47outlive a 48-tool roster, kept main red four days and develop red six, and blocked fifteen Dependabot PRs.composer.lockneeded no resolution. It auto-merged to something byte-identical to main's,content-hashincluded, andcomposer validateconfirms it still matches the resolvedcomposer.json.Verification
A clean merge proves very little when one side carries 109 commits the other has never seen, so I went past it.
The load-bearing signal is the disk-derived roster test passing: it compares registration against the tool files actually present, which is direct evidence that develop's tool files and main's registration agree. That is the single assertion this merge could most plausibly have broken, and it is now structurally incapable of going stale.
379 tests green on PHP 8.4 across
tests/Unit/Mcp,tests/Architecture,tests/Feature/Flow,tests/Unit/Flow,tests/Feature/Migrations,tests/Feature/Mcpandtests/Feature/Security— the last including the R32 authorization matrix, which is what would catch a route-gating regression from combining the two histories.Merge method
Please merge this with a merge commit, not a squash. Squashing would give develop the content without main's ancestry, so the very same files would conflict again on the next sync. A real merge commit is what makes these two branches actually converge.
🤖 Generated with Claude Code
https://claude.ai/code/session_019eS9Cj8rvPK85N517Emn9h