Repository navigation
A junk id could still provision a schema the router would never route to - #932
Conversation
The creation vector for #714 is dead — the storage routers reject malformed first segments and never lazily CREATE SCHEMA — but the provisioning boundary itself was never taught the rule. EnsurePartitionProvisioned passed the namespace verbatim into public.ensure_partition_schema, and OwnsPartitionProvisioningValidator provisioned context.Node.Id verbatim, so a deliberate top-level create with id `search?q=x` still materialised a real schema + tables. That schema is unreachable by construction: the router refuses to route the very segment that named it. Provisioning and routing disagreed about what a partition name is, and only routing knew. So make it ONE rule. PartitionDefinition.IsValidPartitionSegment is now the single predicate — start with a letter/digit, then only letters/digits/. - _, <= 63 chars — and both routers (PG, Snowflake) call it instead of carrying their own copy. The boundary enforces the same predicate in three places a caller can reach: - EnsurePartitionProvisioned (PG + Snowflake) rejects BEFORE the promise-cache, so the failure is a loud OnError naming the offending id rather than a cached silent no-op that later callers replay as success. - EnsureSchemaAsync validates the RESOLVED schema name — the last stop before public.ensure_partition_schema — covering the boot-time static-partition seed path (EnsureSchemaForPartitionAsync) too. - OwnsPartitionProvisioningValidator rejects the create with InvalidPath and a speaking message, instead of provisioning junk for it. V51 cleans up what already leaked into memex/memexcloud: it enumerates every non-system schema and drops exactly those FAILING the same shared predicate — pattern-based, never a name list, which is why V03's hardcoded bare words could never match `search?q=query%20syntax&hq=scope%3adescendants`. A schema that fails the rule cannot be routed to, so it holds nothing reachable; the drop is CASCADE + best-effort per schema (a lock conflict must not abort the run and block the deploy), preceded by a row-count audit and followed by a searchable_schemas de-registration so queries stop fanning out to it. What remains false: this hardens the boundary, it does not re-validate ids already persisted as partition roots — an existing node whose id fails the rule keeps its row (its schema is dropped by V51 if it had one). And V51 runs only on non-fresh databases, by the migration runner's fresh-DB fast-forward: a fresh DB has no junk to drop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Unifies and enforces a single partition-segment validity rule across routing and provisioning to prevent malformed top-level IDs (e.g., URL/querystring-shaped) from ever materializing unreachable backing-store schemas, and adds a cleanup migration to drop already-created invalid schemas.
Changes:
- Centralizes the partition-segment predicate in
PartitionDefinition.IsValidPartitionSegmentand removes duplicated implementations from the Postgres/Snowflake routers. - Adds boundary validation to Postgres/Snowflake provisioning paths and to
OwnsPartitionProvisioningValidatorso invalid IDs fail loudly instead of provisioning. - Adds a v51 migration to enumerate and drop invalid-named schemas, and extends/adds tests to cover the new shared rule and boundary behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.PostgreSql.Test/ProvisioningBoundaryValidationTest.cs | New tests asserting provisioning rejects malformed namespaces/schema names loudly before any DB round-trip. |
| test/MeshWeaver.Hosting.PostgreSql.Test/PartitionSegmentValidationTest.cs | Retargets validation tests to the shared predicate and expands invalid cases with #714 examples. |
| src/MeshWeaver.Mesh.Contract/PartitionDefinition.cs | Introduces shared IsValidPartitionSegment predicate and a shared, human-readable requirement string for error messages. |
| src/MeshWeaver.Hosting.Snowflake/SnowflakePathRoutingAdapter.cs | Removes duplicated segment-validity logic and uses the shared predicate for routing decisions. |
| src/MeshWeaver.Hosting.Snowflake/SnowflakePartitionStorageProvider.cs | Adds early namespace validation to block provisioning invalid partitions (fail before promise-cache). |
| src/MeshWeaver.Hosting.PostgreSql/PostgreSqlPathRoutingAdapter.cs | Removes duplicated segment-validity logic and uses the shared predicate for routing decisions. |
| src/MeshWeaver.Hosting.PostgreSql/PostgreSqlPartitionStorageProvider.cs | Adds defense-in-depth validation in both the provisioning entry point and the schema-provisioning funnel. |
| src/MeshWeaver.Graph/Security/OwnsPartitionProvisioningValidator.cs | Rejects creates with invalid partition IDs before provisioning so malformed roots can’t create schemas. |
| memex/aspire/Memex.Database.Migration/Migrations/V51_DropInvalidPartitionSchemas.cs | New migration that enumerates and best-effort drops invalid-named schemas and de-registers them from searchable_schemas. |
| memex/aspire/Memex.Database.Migration/MigrationRegistry.cs | Registers migration v51 in the migration registry. |
Suppressed comments (1)
src/MeshWeaver.Mesh.Contract/PartitionDefinition.cs:176
- The rule claims to enforce the Postgres identifier limit, but
segment.Length <= 63checks UTF-16 code units, not the 63-byte identifier limit in Postgres’ server encoding (typically UTF-8). A segment containing non-ASCII letters/digits can pass validation yet be truncated/collide at the database level, reintroducing “unroutable/unexpected schema” risk in a different form.
public static bool IsValidPartitionSegment(string? segment)
=> !string.IsNullOrEmpty(segment)
&& segment.Length <= 63
&& char.IsLetterOrDigit(segment[0])
&& segment.All(c => char.IsLetterOrDigit(c) || c is '.' or '-' or '_');
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…boundary-validation
Test Results (shard 5)1 702 tests +18 1 511 ✅ +18 5m 43s ⏱️ + 1m 3s Results for commit 767b9ae. ± Comparison against base commit aec5aeb. This pull request removes 1 and adds 19 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results 65 files ± 0 65 suites ±0 37m 28s ⏱️ +54s Results for commit 767b9ae. ± Comparison against base commit aec5aeb. This pull request removes 1 and adds 20 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…boundary-validation
… CI regression CI caught a real regression this PR introduced: whole test classes went red with "Cannot create 'Space' with id '_Access': a partition name must start with a letter or digit…" (DeleteNodeBehaviorTest 5/5, CommentDeletePermissionTest, EffectivePermissionPostgresTest) — every one of them from the shared SetupAccessRightsAsync that creates the ROOT `_Access` container. The segment rule deliberately refuses a leading `_` because a USER partition may not start with one. But the framework mints `_`-prefixed system containers itself (root `_Access` and kin) — they never come from a request path, so they are not the junk-id vector #714 is about, and validating them there made the root access container un-creatable. All three enforcement points now exempt a leading `_`: the provisioning validator, EnsureSchemaAsync, and EnsurePartitionProvisioned. URL/query-shaped ids are still rejected exactly as before — the added tests for `search?q=…` / `login?returnurl=…` are unaffected (neither starts with `_`). DeleteNodeBehaviorTest 10/10; Graph + Hosting.PostgreSql clean under Release -warnaserror. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er limit is real Second CI regression from this PR's length rule, and unlike the `_Access` one the rule is RIGHT here: a partition id becomes a Postgres schema name, and PG truncates identifiers at 63 bytes — so two long ids sharing a 63-char prefix would silently land in the SAME schema. That is a collision worth rejecting. The collateral was QueryAsyncIntegrationTests, whose [CallerMemberName] prefix produced a 66-char partition from one long method name (plus the "other" suffix some callers append). Truncating the prefix to 40 keeps every one of the 26 test methods distinct (verified by comparing all truncations) and leaves room for the suffixes. QueryAsyncIntegrationTests 26/26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…boundary-validation
…boundary-validation
…gment as a partition
My previous commit exempted `_`-prefixed names from the partition-id rule at all three
enforcement points, because CI went red with "Cannot create 'Space' with id '_Access'".
That was the wrong half to change. `_Access` is not a partition — it is a satellite
CONTAINER namespace whose schema comes from a REGISTERED PartitionDefinition
(`_Access` -> `system_access`). The router only ever resolves a `_`-prefixed first segment
through that registered lookup and never derives a schema from the name, so a schema
literally named `_access` is unroutable by construction: exactly the ghost this PR exists
to prevent. (V38 already had to drop one such orphan, top-level `_provider`.)
The offending caller is MeshExtensions.EnsurePartitionBootstrap. Its skip test is
`Path.Contains("/_Access/")`, whose LEADING SLASH matches `{P}/_Access/...` but NOT the
ROOT-SCOPE grant `_Access/{subject}_Access` (namespace `_Access`, scope "") that
`AssignmentNodeFactory.UserRole(user, role)` with no scope produces at ~200 harness call
sites. That fell through, `Segments[0]` read `_Access` as a PARTITION, and the bootstrap
created a `Space` root at `_Access`, provisioned an `_access` schema, and minted a
doubled-up `_Access/_Access/{creator}_Access` grant.
Fixed at the caller: the bootstrap now gates on the shared
`PartitionDefinition.IsValidPartitionSegment` before treating a first segment as a
partition. General, not an `_Access` special case — no `_`-prefixed satellite container
and no URL-shaped junk segment is bootstrappable. The exemption is removed from all three
enforcement points, so `_access` is junk that errors loud, as the test always asserted.
Review threads:
- The 63 cap is now counted in UTF-8 BYTES, not chars. `char.IsLetterOrDigit` is
Unicode-aware, so a 63-char multi-byte name exceeded Postgres' NAMEDATALEN byte limit
and was SILENTLY TRUNCATED — provisioning a schema the router (computing the
untruncated name) could never route back to. Message and docs reworded to match.
- V51's XML doc no longer claims an ASCII-only `[A-Za-z0-9._-]` charset; it states the
rule is Unicode-aware so ops readers do not expect `müller` to be dropped.
- ProvisioningBoundaryValidationTest no longer leaks an NpgsqlDataSource per case: a
ProviderScope handle disposes provider + data source together.
Regression test: PartitionRootBootstrapTest.RootScopeAccessGrant_DoesNotBootstrap_AccessAsAPartition.
Green, all Release: ProvisioningBoundary + PartitionSegmentValidation 36/36;
PartitionRootBootstrapTest 10/10; DeleteNodeBehaviorTest 10/10 and
CommentDeletePermissionTest 1/1 (the two suites the exemption was added for);
NodeOperations 85/85; AccessControl 10/10.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…boundary-validation
|
ROOT CAUSE for the The previous commit exempted Is a if (seg.StartsWith('_'))
return _provider.TryGetRegisteredPartition(seg, out var registered) ? registered : null;
The offending caller: if (string.IsNullOrEmpty(node.Namespace)
|| node.Path.Contains("/_Access/", StringComparison.Ordinal))The LEADING SLASH matches
The method's own doc claimed the grant create "never re-enters the bootstrap" — true for scoped grants, false for root-scope ones. Fix: the bootstrap now gates on the shared The decisive check: with the exemption removed and the caller fixed, the two suites that motivated it are green — |
|
CI status on Not this diff, on four independent checks:
Leaving it untouched deliberately — no retry, no widened bound, no skip. Flagging rather than papering over it. |
|
Correcting my previous comment — its evidence was weaker than I presented, and I have now settled the question by direct experiment rather than inference. Two things I got wrong above: "green locally" was true only for a whole-project run, and "green on main" was about main's CI — but main does not carry this gate, so main being green could never have ruled the gate out. Both are superseded. The hypothesis, tested directlyThe concern was that narrowing Zero rejections. The gate is never reached with a failing segment during this test. Consistent with the paths it actually uses — The failure is pre-existing on mainRunning the single test in isolation reproduces it — and it reproduces identically on a clean detached worktree at
Byte-identical assertion in both: It passes when its sibling in the class runs first (class run 2/2, full project 249 passed / 1 skipped / 0 failed), which is why it is normally invisible — it is an order/timing-dependent defect in the test, latent on main and not introduced here. Repro on main, no branch involved: git worktree add --detach /tmp/MW-main-probe origin/main
cd /tmp/MW-main-probe
dotnet build test/MeshWeaver.Content.Test/MeshWeaver.Content.Test.csproj -c Release
dotnet test test/MeshWeaver.Content.Test -c Release --no-build \
--filter "FullyQualifiedName~RevertingAProjectedChange"Left untouched here — no retry, no widened bound, no skip, and the gate is not loosened back. It wants its own fix: line 188 asserts on |
`TrackedChangeProjectionTest.RevertingAProjectedChange_PutsTheTextBack_AndIsItselfVersioned`
failed deterministically when run ALONE and passed only when its sibling ran first. Not a
flake, and not order-independence luck: the sibling merely warmed the mesh, so the timing
shifted enough to hide an incomplete arrangement in the test itself.
Instrumented, both orderings, identical version rows:
run alone rows=[2,1] edited.Version=1 hasRECKLESS=False -> FAIL (0 changes)
with sibling rows=[2,1] edited.Version=2 hasRECKLESS=True -> PASS
The history had both rows in BOTH runs. What differed was the node the assertion projected
from. `WaitForVersions` waits on the version STORE; the node is a different store and had
not caught up. `stream.Update` returns the locally computed node optimistically and the
storage layer stamps the reconciled version a moment later, so `GetMeshNodeStream(path)`
replays the PRE-edit snapshot for a beat. The helper read it with a bare
`.Where(n => n is not null).Take(1)`, taking whichever snapshot happened to be cached.
`ChangeProjection.FromHistory` then projects only versions STRICTLY older than the node it
is handed (`v.Version < currentNode.Version`). Handed the stale v1, `older` is empty and it
returns `[]` — every version row present, nothing derivable from them. That is the
"found 0".
Not a product defect. `FromHistory`'s contract is explicit ("Emits once, then completes —
the caller re-subscribes when the node's version moves"), and the real consumer,
`CollaborativeMarkdownView`, re-projects with `Switch` on every node emission, so a stale
emission yields an empty list for an instant and is immediately superseded. Only a one-shot
assertion is exposed — which is exactly what this test is.
Fix: read the node AT the newest version the history recorded, via a new `NodeAtVersion`
helper replacing `CurrentNode`. This waits on the actual precondition the assertion depends
on — the same discipline the test already applied further down for the revert ("Wait on the
CONDITION, not on the optimistic emission"). No sleep, no widened bound, no retry, no
reordering, no merging of the two tests. Also asserts the edit is present in the node being
projected, so a future regression fails on the real cause instead of an empty collection.
The sibling test carried the identical hazard at its own read (three edits gave it more
slack); fixed the same way.
Verified: the previously failing test PASSES RUN ALONE, the sibling passes run alone, and
the full project is 249 passed / 1 skipped / 0 failed. Release, `-warnaserror` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…boundary-validation
|
Fixed the Why the sibling "rescued" it: timing, not leaked stateInstrumented both orderings and printed the version rows alongside the node actually being projected: The version rows are identical in both runs. The sibling leaves behind nothing this test consumes — it just warms the mesh, so reconciliation lands before the read. The only difference is which node snapshot the assertion projected from. The defect: the test asserted against a stale node
var older = all.Where(v => v.Version < currentNode.Version)...
if (older.Count == 0) return Observable.Return<IReadOnlyList<TrackedChange>>([]);Handed the stale v1, Not a product defect
The fixRead the node at the newest version the history recorded, via a None of the forbidden shapes: no sleep, no widened bound, no retry, no reordering, no merging the two tests. The sibling carried the identical hazard at its own read (three edits gave it more slack) and is fixed the same way. Verified
Re-verified after merging current |
…-built output, tests after the bundle A caller whose gates only COMPOSE a module bundle (Plugins' compile-check and ACR gates take the AI + Markdown.Collaboration bundles from the floor module-pack call, #932) had to depend on that call's RESULT, which also carries the module's test suite — so one AI flake turned both required gates red (Plugins #937: PreInstalledPackageInstallTest on main run 33279529511, ThreadSubmissionIntegrationTest on run 33280461137). The pack job now drops a module-built-<module> marker right after the bundle artifact is uploaded, and the tests run AFTER that (still before the hand-over: a failing suite never publishes). 'All selected bundles built' collects the markers and writes bundles_built=true|false (every selected module has a marker, independent of receipts and of the pack job's result), exposed as the workflow_call output bundles-built. A caller's gate depends on that; the lane's stable context still asserts the receipts AND the pack result, so a red suite keeps the lane red where the lane itself is required. A pack job that produced no bundle drops no marker, so nothing satisfies the edge with an empty artifact. Self-test: bundles-built true with a failed pack job; false naming a module with no marker; a sibling call's marker set aside; an empty selection is true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #714.
The creation vector was already dead on main — the storage routers reject malformed first segments and never lazily
CREATE SCHEMA. This closes the two items the issue's status audit left open, and — found while hardening the boundary — one live caller that was still provisioning an unroutable schema on every run.Why a dead vector still left a hole
Provisioning and routing disagreed about what a partition name is, and only routing knew.
EnsurePartitionProvisionedpassed the namespace verbatim intopublic.ensure_partition_schema, andOwnsPartitionProvisioningValidatorprovisionedcontext.Node.Idverbatim — so a deliberate top-level create with idsearch?q=xstill materialised a real schema + tables. That schema is unreachable by construction: the router refuses to route the very segment that named it.1. One rule, enforced at the boundary
PartitionDefinition.IsValidPartitionSegmentis now the single predicate (start with a letter/digit, then only letters/digits/.-_, ≤63 UTF-8 bytes). Both routers call it instead of carrying their own copy — the duplicated charset rule inPostgreSqlPathRoutingAdapterandSnowflakePathRoutingAdapteris gone, not forked a third time.The same predicate now gates every way a caller can reach schema creation:
EnsurePartitionProvisioned(PG + Snowflake) rejects before the promise-cache, so the failure is a loudOnErrornaming the offending id — not a cached silent no-op that later subscribers replay as success.EnsureSchemaAsyncvalidates the resolved schema name — the last stop beforepublic.ensure_partition_schema— which also covers the boot-time static-partition seed path (EnsureSchemaForPartitionAsync).OwnsPartitionProvisioningValidatorrejects the create withInvalidPathand a speaking message instead of provisioning junk for it.The cap is counted in UTF-8 bytes, not chars:
char.IsLetterOrDigitis Unicode-aware, so a 63-char multi-byte name exceeded Postgres'NAMEDATALENbyte limit and was SILENTLY TRUNCATED — provisioning a schema the router (computing the untruncated name) could never route back to.2. The live caller: a satellite segment passed where a partition belongs
Enforcing the rule surfaced a real defect it had been masking.
MeshExtensions.EnsurePartitionBootstrapskips grant creates withnode.Path.Contains("/_Access/"). That leading slash matches{P}/_Access/…but not the ROOT-SCOPE grant_Access/{subject}_Access(namespace_Access, scope"") — the shapeAssignmentNodeFactory.UserRole(user, role)with no scope produces, used at ~200 harness call sites and byTestUsers.DevLoginAdminAccess().So it fell through,
Segments[0]read_Accessas a PARTITION, and the bootstrap created aSpaceroot at_Access, provisioned a schema literally named_access, and minted a doubled-up_Access/_Access/{creator}_Accessgrant.A
_-prefixed name is never a partition. The router resolves a_-prefixed first segment only through the registered-partition lookup —DefaultPartitionProviderregisters_Accesswith the EXPLICIT schemasystem_access— and never derives one from the name, so_accessis unroutable by construction. (V38_DropLegacyProviderSchemais prior art: it had to drop exactly such an orphan, top-level_provider.)Fixed at the caller: the bootstrap now gates on the shared predicate before treating a first segment as a partition. General, not an
_Accessspecial case — no_-prefixed satellite container (_Thread,_Activity, …) and no URL-shaped junk segment is bootstrappable.3. V51 cleanup migration
Enumerates every non-system schema and drops exactly those failing the same shared predicate — pattern-based, never a name list, which is precisely why V03's hardcoded bare words could never match
search?q=query%20syntax&hq=scope%3adescendants. A schema failing the rule cannot be routed to, so it holds nothing reachable. Per schema: a row-count audit first (so a surprise is visible in the log), thenDROP … CASCADEbest-effort (a lock conflict must not abort the run and block the deploy), then asearchable_schemasde-registration so queries stop fanning out to it. Registered inMigrationRegistry.All. It also clears any_accessschema left behind by the bootstrap bug above.Tests
PartitionSegmentValidationTestretargeted to the shared predicate, extended with the literal junk names from the issue (search?q=query%20syntax&hq=scope%3adescendants,login?returnurl=https%3a%2f%2f…) plus=,&,%, null, the Unicode-accepting cases (müller,Straße), and the byte-cap boundary from both sides.ProvisioningBoundaryValidationTest(new) drives the realPostgreSqlPartitionStorageProviderseams —EnsurePartitionProvisionedand theEnsureSchemaAsyncfunnel — and asserts the rejection is loud: anArgumentExceptionnaming the offending id and stating why. The guard fires before any DB round-trip, so no container is needed.PartitionRootBootstrapTest.RootScopeAccessGrant_DoesNotBootstrap_AccessAsAPartition(new) pins the caller fix: the root-scope grant still writes, and neither_Accessnor_Access/_Access/…is created.Local runs, all Release:
PartitionSegmentValidationTest+ProvisioningBoundaryValidationTest36/36;PartitionRootBootstrapTest10/10;DeleteNodeBehaviorTest10/10;CommentDeletePermissionTest1/1;NodeOperations85/85;AccessControl10/10;GhostSchemaInvariantTests2/2 andPartitionLifecycleTests6/6 against real Postgres (Testcontainers) to confirm legitimate provisioning still works. Four src + two test projects build clean with-c Release -warnaserror.Scope notes
🤖 Generated with Claude Code