Skip to content

Resolve a schema's relative paths through Semantics.Paths [patch] - #208

Merged
matt-edmondson merged 2 commits into
mainfrom
claude/nifty-bohr-ajnazk/schema-200-paths
Sep 22, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
claude/nifty-bohr-ajnazk/schema-200-paths

Conversation

@matt-edmondson

@matt-edmondson matt-edmondson commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #200

What changed

Schema/Models/Schema.Paths.cs combined two already-typed Semantics.Paths values through a System.IO.Path string round-trip — re-parsing and re-validating both sides of a combine the library already owns.

before after
TryResolvePath(RelativeFilePath, …) Path.GetFullPath(Path.Combine(SourceDirectory, relativePath)).As<AbsoluteFilePath>() relativePath.AsAbsolute(SourceDirectory)
TryResolvePath(RelativeDirectoryPath, …) same shape relativePath.AsAbsolute(SourceDirectory)

SetSourceFile is deliberately left alone — see below. The net diff is two lines of implementation plus the reasoning that keeps them that way.

AsAbsolute, not the / operator

The issue sketched the / combine operator, and flagged as unverified "whether Path.GetFullPath normalization was doing anything the / operator's own construction doesn't already guarantee". It was. Measured against ktsu.Semantics.Paths 5.4.2:

base relative Path.GetFullPath(Path.Combine(…)) / operator AsAbsolute(base)
/src/schemas data.json /src/schemas/data.json same same
/src/schemas sub/data.json /src/schemas/sub/data.json same same
/src/schemas ../data.json /src/data.json ❌ /src/schemas/../data.json ✅ same
/src/schemas ./data.json /src/schemas/data.json ❌ /src/schemas/./data.json ✅ same
/src/schemas a/../b/data.json /src/schemas/b/data.json ❌ /src/schemas/a/../b/data.json ✅ same
/src/schemas a//data.json /src/schemas/a/data.json ❌ /src/schemas/a//data.json ✅ same

/ joins; it does not normalise. A schema whose data source sits in a sibling directory ("file": "../shared/items.json" — an ordinary thing to write) would have resolved to a path containing .. rather than the path it names. The resolved value is compared, displayed and used as a dictionary key, so that is a behaviour change, not a cosmetic one.

AsAbsolute(basePath) reproduces Path.GetFullPath(Path.Combine(…)) exactly on every case above, in both overloads. The edge cases the existing guards already reject match too: an empty anchor is refused by CanResolvePaths before either implementation is reached, and an empty relative path by the string.IsNullOrEmpty guard.

Why SetSourceFile keeps Path.GetDirectoryName

The issue also proposed SourceDirectory = schemaFilePath.AbsoluteDirectoryPath. That is not safe, and CI caught it — the first push failed FileBrowserTests.SavingRecordsTheChosenPathAsRecentlyUsed with an expected and actual that render character-for-character identical.

Reading AbsoluteFilePath.AbsoluteDirectoryPath mutates the instance it is read from. In ktsu.Semantics.Paths 5.4.2:

var a = "/tmp/x/y.schema.json".As<AbsoluteFilePath>();
var b = "/tmp/x/y.schema.json".As<AbsoluteFilePath>();
a == b;                  // True
_ = a.AbsoluteDirectoryPath;
a == b;                  // False   ← and GetHashCode() now differs too
(string)a == (string)b;  // still True — the text is unchanged

SchemaEditor.SaveToCurrentPath calls CurrentSchema.SetSourceFile(CurrentSchemaPath) and then Options.RecordRecentFile(CurrentSchemaPath) — the same instance. So anchoring the schema silently broke equality for a path this code does not own, and the recorded recent file stopped comparing equal to itself.

FileNameWithoutExtension has the same defect; FileName, FileExtension and FullFileExtension do not. AsAbsolute carries no such hazard — measured, it mutates neither its receiver nor its argument — which is why the resolution moved and the anchor did not. Reverting the anchor also removes the Ensure.NotNull guard, which CA1062 only asked for because the parameter was being dereferenced.

Root cause is upstream and now filed as ktsu-dev/Semantics#265: AbsoluteFilePath is a record class carrying lazily-populated _cachedDirectoryPath / _cachedFileNameWithoutExtension fields, and record-generated Equals/GetHashCode include them, so populating a cache on read changes both. It is a hazard well beyond this repo — an AbsoluteFilePath used as a dictionary key becomes unfindable once anything reads its directory.

Tests

Added to SchemaDataSourceTests:

  • TestFilePathsResolveThroughTraversalSegments — ../shared/./items.json
  • TestDirectoryPathsResolveThroughTraversalSegments — ../build/./generated
  • TestTheAnchorIsTheSchemaFilesDirectory — SetSourceFile splits anchor from filename
  • TestSettingTheSourceFileLeavesTheCallersPathEqualToItself — the regression above

The resolution change is behaviour-preserving, so no test can separate it from the old code — that is the point. What the two traversal tests pin is the invariant a naive swap would break, verified by temporarily applying the issue's literal / sketch (both fail: /tmp/…/../shared/./items.json vs the expected /tmp/shared/items.json). The mutation test was likewise verified to fail on the AbsoluteDirectoryPath version and pass on this one.

Not included

SchemaGenerator.cs:173-175, which the issue flags as "the weakest of the four" — adopting the operator there means first typing file.Key as a RelativeFilePath, which is a change to the generator's key handling rather than to path resolution. Left for its own change.

Verification

Locally, on the final head:

  • Schema.Test — 502/502
  • tests/Schema.Editor.UITests — 190/190 (the suite that failed; run on Linux, as CI does)
  • Schema.Cpp.Test — 104/104
  • Schema.Editor.Test — 17/17

CI is green on all three platforms, plus CodeQL, GHAS, and SonarCloud (100% coverage on new code).

🤖 Generated with Claude Code

https://claude.ai/code/session_01By7NnPN7STCZqeAftJ1BmH

Schema.Paths.cs combined two already-typed Semantics.Paths values through a
System.IO.Path string round-trip, re-parsing and re-validating both sides of a
combine the library already owns.

Resolution now goes through RelativeFilePath/RelativeDirectoryPath.AsAbsolute,
and the anchor through AbsoluteFilePath.AbsoluteDirectoryPath.

AsAbsolute rather than the `/` combine operator the issue sketched: `/` joins
without normalising, so a data source reaching a sibling directory through `..`
came back as a route to the file rather than the file. AsAbsolute reproduces
Path.GetFullPath(Path.Combine(...)) exactly, including `..`, `.`, repeated
separators and the empty-path cases the existing guards already reject.

SetSourceFile gains the null guard CA1062 requires once the parameter is
dereferenced rather than cast.

Fixes #200

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

CI caught a real regression: FileBrowserTests.SavingRecordsTheChosenPathAsRecentlyUsed
failed with an expected and actual that render character-for-character
identical.

Reading AbsoluteFilePath.AbsoluteDirectoryPath mutates the instance it is read
from. In ktsu.Semantics.Paths 5.4.2 an AbsoluteFilePath stops comparing equal
to an identical one, and its hash code changes, once that property has been
touched, while its text stays the same. SchemaEditor.SaveToCurrentPath hands
the same instance to SetSourceFile and then to RecordRecentFile, so anchoring
the schema silently broke equality for a path this code does not own.

So SetSourceFile goes back to Path.GetDirectoryName, and with it the null guard
CA1062 only wanted because the parameter was being dereferenced. The two
TryResolvePath overloads keep AsAbsolute: measured, it mutates neither its
receiver nor its argument, and it is where the issue's actual win was.

Adds a test that fails on the property version and passes on this one, and
records the hazard where the next reader would otherwise 'tidy' it back.

Verified: Schema.Test 502/502, Schema.Editor.UITests 190/190 (the suite that
failed), Schema.Cpp.Test 104/104, Schema.Editor.Test 17/17.

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

Copy link
Copy Markdown
Contributor Author

CI caught a real regression, and it was mine — fixed in e57fa12

The first push failed FileBrowserTests.SavingRecordsTheChosenPathAsRecentlyUsed (1 of 1921). I reproduced it locally, and confirmed it passes on main and failed on this branch, so it was this PR's, not a pre-existing or flaky failure.

Cause. The issue proposed SourceDirectory = schemaFilePath.AbsoluteDirectoryPath in SetSourceFile. Reading that property mutates the instance it is read from: in ktsu.Semantics.Paths 5.4.2 the value stops comparing equal to an identical one and its hash code changes, while its text stays the same. SchemaEditor.SaveToCurrentPath hands the same instance to SetSourceFile and then to RecordRecentFile, so anchoring the schema broke equality for a path this code does not own — hence a failure whose expected and actual print character-for-character identical.

Root cause is upstream: AbsoluteFilePath is a record class carrying lazily-populated _cachedDirectoryPath / _cachedFileNameWithoutExtension fields, and record-generated Equals/GetHashCode include them. Filed as ktsu-dev/Semantics#265 with the reduction and a suggested fix.

Fix here. SetSourceFile goes back to Path.GetDirectoryName (and loses the Ensure.NotNull that CA1062 only wanted because the parameter was being dereferenced). The two TryResolvePath overloads keep AsAbsolute, which I measured to mutate neither its receiver nor its argument — and which is where the issue's actual win was. Added TestSettingTheSourceFileLeavesTheCallersPathEqualToItself, verified to fail on the property version and pass on this one, plus a comment recording the hazard so the next reader does not tidy it back.

The PR description is updated to describe what actually landed.

Now green locally, including the suite that failed:

suite result
Schema.Test 502/502
tests/Schema.Editor.UITests 190/190
Schema.Cpp.Test 104/104
Schema.Editor.Test 17/17

Generated by Claude Code

@sonarqubecloud

Copy link
Copy Markdown

@matt-edmondson
matt-edmondson merged commit 35118fb into main Sep 22, 2026
12 checks passed
@matt-edmondson
matt-edmondson deleted the claude/nifty-bohr-ajnazk/schema-200-paths branch September 22, 2026 03:08
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.

Delegate path resolution to ktsu.Semantics.Paths' combine operators

2 participants