feat: resolve <inheritdoc/> across assembly boundaries

[Malcolmnixon]

Summary

Adds native cross-assembly <inheritdoc/> resolution to ApiMark.DotNet, so <inheritdoc/> correctly resolves documentation from base types/interfaces defined in external assemblies (NuGet packages, the .NET BCL, etc.) — not just within the same assembly as before.

This is conceptually inspired by SauceControl/InheritDoc (same core idea: resolve reference assemblies via ReferencePath-style items, find sibling XML doc files with ref/<->lib/ fallback), but implemented natively with no new external dependency.

What changed

• ExternalXmlDocResolver (new) — locates and lazily parses external assemblies' sibling XML doc files, with ref/<->lib/ folder-swap fallback (some NuGet packages ship docs in only one of those folders) and per-member caching, including negative-cache (miss) results.
• XmlDocReader — gained an optional external-member-lookup fallback, used only when a member isn't found in the local assembly's own XML docs. Fully backward compatible.
• DotNetGenerator — seeds an assembly resolver from configured reference paths and wires the external resolver into XmlDocReader.
• New ReferencePaths option, surfaced end-to-end:
  • ApiMark.Tool: --reference-paths <path> CLI flag (repeatable)
  • ApiMark.MSBuild: ApiMarkReferencePaths task property, auto-harvested from @(ReferencePath) when not explicitly set (never clobbers a user-supplied value)

Verification

• Verified end-to-end against a real project referencing System.IDisposable from the .NET BCL reference-assembly XML docs shipped with the SDK, and against a real external NuGet-style package reference — confirmed <inheritdoc/> correctly pulls in the external documentation text.
• Full test coverage added: ExternalXmlDocResolverTests, extended XmlDocReaderTests/DotNetGeneratorTests, new external fixture project, new MSBuild/CLI option tests, and new ApiMark.MSBuild.PackageTests integration tests proving the .targets auto-harvest behavior and its suppression when set explicitly.
• All non-C++ suites pass clean: ApiMark.DotNet.Tests 312/312 (×3 TFMs), ApiMark.MSBuild.Tests 35/35, ApiMark.Tool.Tests 99/99, ApiMark.Core.Tests 118/118, ApiMark.MSBuild.PackageTests 7/7 (excluding the pre-existing, environment-only clang-toolchain C++ test).
• dotnet reviewmark --lint / --plan --enforce, dotnet reqstream --lint / --enforce --tests, and pwsh ./lint.ps1 (cspell, markdownlint, yamllint, sysml2tools, dotnet format --verify-no-changes) all clean.

Review process

• Ran the built-in code-review agent on the full diff — found 1 Medium issue (stale thread-safety claim in XmlDocReader's doc comment now that an external lookup delegate can be wired in) — fixed.
• Ran formal reviews (using gpt-5.4-mini) across all 21 .reviewmark.yaml review-sets touched by this change. Initial sweep: 12 passed clean, 9 failed with Medium/High findings (mostly system/subsystem-level docs not yet updated to mention the new unit/option, plus a couple of missing edge-case requirements/tests). All feature-caused findings were fixed and independently re-verified; a few unrelated pre-existing gaps were confirmed as such and left untouched per scope discipline.

Documentation

Design, reqstream (requirements), verification, SysML2 model, and user-guide docs updated for all affected units in ApiMark.DotNet, ApiMark.MSBuild, and ApiMark.Tool.

---

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in compatibility, reference resolution, resource cleanup, caching, and MSBuild override behavior.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds cross-assembly .NET <inheritdoc/> resolution through CLI and MSBuild reference paths.

Changes:

• Adds external XML documentation lookup with caching and ref/lib fallback.
• Wires repeatable reference-path options through CLI, MSBuild, and the generator.
• Adds fixtures, tests, requirements, design, and verification documentation.

File summaries

File	Reviewed changes	Review notes
test/ApiMark.Tool.Tests/ProgramTests.cs	CLI end-to-end coverage	—
test/ApiMark.Tool.Tests/Cli/ContextTests.cs	Reference-path parsing tests	—
test/ApiMark.MSBuild.Tests/ApiMarkTaskTests.cs	MSBuild argument tests	—
test/ApiMark.MSBuild.PackageTests/PackageIntegrationTests.cs	Package integration scenarios	Nit (3 votes, lines 259 and 296): assertions do not prove reference harvesting or external inherited documentation.
test/ApiMark.MSBuild.PackageTests/Fixtures/DotNet/SampleLibWithReference/SampleLib.csproj	Reference fixture project	—
test/ApiMark.MSBuild.PackageTests/Fixtures/DotNet/SampleLibWithReference/SampleClass.cs	Reference fixture source	—
test/ApiMark.DotNet.Tests/XmlDocReaderTests.cs	External inheritdoc tests	—
test/ApiMark.DotNet.Tests/FixturePaths.cs	External fixture paths	—
test/ApiMark.DotNet.Tests/ExternalXmlDocResolverTests.cs	Resolver unit tests	Nit (1 vote, line 337): the negative-cache test should rewrite the XML before the second lookup.
test/ApiMark.DotNet.Tests/DotNetGeneratorTests.cs	Generator integration tests	—
test/ApiMark.DotNet.Tests/ApiMark.DotNet.Tests.csproj	External fixture reference	—
test/ApiMark.DotNet.Fixtures/ExternalInheritDocClass.cs	Cross-assembly inheritdoc fixture	—
test/ApiMark.DotNet.Fixtures/ApiMark.DotNet.Fixtures.csproj	Fixture project reference	—
test/ApiMark.DotNet.Fixtures.External/IExternalBaseInterface.cs	External interface fixture	—
test/ApiMark.DotNet.Fixtures.External/ExternalBaseClass.cs	External base-class fixture	—
test/ApiMark.DotNet.Fixtures.External/ApiMark.DotNet.Fixtures.External.csproj	External fixture project	—
src/ApiMark.Tool/Program.cs	CLI option wiring	—
src/ApiMark.Tool/Cli/Context.cs	CLI option parsing	—
src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets	MSBuild harvesting and forwarding	Moderate (3 votes, line 70): explicit empty ApiMarkReferencePaths is repopulated; preserve an explicit-set marker or provide an opt-out.
src/ApiMark.MSBuild/ApiMarkTask.cs	MSBuild property and argument support	—
src/ApiMark.DotNet/XmlDocReader.cs	External lookup fallback	Critical (1 vote, line 63): preserve the original two-parameter constructor and add a separate three-parameter overload for binary compatibility.
src/ApiMark.DotNet/ExternalXmlDocResolver.cs	External XML documentation resolver	Moderate (1 vote, line 50): use a platform-aware path comparer to avoid conflating distinct Linux paths.
src/ApiMark.DotNet/DotNetGenerator.cs	Cecil resolver and generator wiring	Moderate (3 votes, line 110): normalize reference paths with Path.GetFullPath before extracting directories. Moderate (1 vote, line 118): protect assembly loading with resolver cleanup on failure. Moderate (1 vote, line 129): dispose the resolver on the successful path.
requirements.yaml	Requirements inclusion	—
docs/verification/api-mark-tool/program.md	CLI verification documentation	—
docs/verification/api-mark-tool/cli/context.md	Context verification documentation	—
docs/verification/api-mark-tool/cli.md	CLI verification updates	—
docs/verification/api-mark-msbuild/api-mark-task.md	MSBuild task verification	—
docs/verification/api-mark-msbuild.md	MSBuild verification updates	—
docs/verification/api-mark-dot-net/xml-doc-reader.md	Reader verification documentation	—
docs/verification/api-mark-dot-net/external-xml-doc-resolver.md	Resolver verification documentation	—
docs/verification/api-mark-dot-net/dot-net-generator.md	Generator verification documentation	—
docs/verification/api-mark-dot-net.md	.NET verification updates	—
docs/user_guide/dotnet.md	User-facing option documentation	—
docs/sysml2/model/api-mark-dot-net/external-xml-doc-resolver.sysml	Resolver model unit	—
docs/sysml2/model/api-mark-dot-net.sysml	Model hierarchy update	—
docs/reqstream/api-mark-tool/program.yaml	CLI requirements	—
docs/reqstream/api-mark-tool/cli/context.yaml	Context requirements	—
docs/reqstream/api-mark-tool/cli.yaml	CLI requirement linkage	—
docs/reqstream/api-mark-msbuild/api-mark-task.yaml	MSBuild task requirements	—
docs/reqstream/api-mark-msbuild.yaml	MSBuild requirement linkage	—
docs/reqstream/api-mark-dot-net/xml-doc-reader.yaml	Reader requirements	—
docs/reqstream/api-mark-dot-net/external-xml-doc-resolver.yaml	Resolver requirements	—
docs/reqstream/api-mark-dot-net/dot-net-generator.yaml	Generator requirements	—
docs/reqstream/api-mark-dot-net.yaml	.NET requirement linkage	—
docs/design/api-mark-tool/program.md	CLI design update	—
docs/design/api-mark-tool/cli/context.md	Context design update	—
docs/design/api-mark-tool/cli.md	CLI design update	—
docs/design/api-mark-tool.md	Tool architecture update	—
docs/design/api-mark-msbuild/api-mark-task.md	Task design update	—
docs/design/api-mark-msbuild.md	MSBuild architecture update	—
docs/design/api-mark-dot-net/xml-doc-reader.md	Reader design update	—
docs/design/api-mark-dot-net/external-xml-doc-resolver.md	Resolver design	Nit (1 vote, line 130): the design calls the public resolver internal; align the design contract or source visibility/tests.
docs/design/api-mark-dot-net/dot-net-generator.md	Generator design update	—
docs/design/api-mark-dot-net.md	.NET architecture update	—
ApiMark.slnx	Adds external fixture project	—
.reviewmark.yaml	Adds review coverage	—
.cspell.yaml	Adds technical terms	—

Review details

Suppressed comments (6)

docs/design/api-mark-dot-net/external-xml-doc-resolver.md:131

• This design section calls the resolver an internal class, but ExternalXmlDocResolver is declared public and exposes public construction and lookup (src/ApiMark.DotNet/ExternalXmlDocResolver.cs:40,66,84). Update the design contract to describe the public helper and its lack of external service interfaces, or change the source visibility and tests consistently.

N/A — this is an internal class with no external interfaces exposed beyond
its assembly.

src/ApiMark.DotNet/DotNetGenerator.cs:120

• assemblyResolver is allocated before AssemblyDefinition.ReadAssembly, but the cleanup try starts only after that call. If reading the primary assembly fails, the resolver is never disposed, so the failure-path cleanup documented above is skipped. Move the read into a protected block or dispose the resolver in a catch around this call.
  src/ApiMark.DotNet/DotNetGenerator.cs:133
• The successful-parse path never disposes assemblyResolver; DotNetEmitter.Emit only disposes Model.Assembly, while the Cecil resolver can cache every external AssemblyDefinition loaded during inheritance analysis. Reusing this library in one process can therefore retain dependency file handles and metadata until process exit, despite the implementation being safe only because the CLI is short-lived; transfer resolver ownership to the emitter/model and dispose it with the assembly.
  src/ApiMark.DotNet/ExternalXmlDocResolver.cs:50
• This cache uses a case-insensitive comparer for filesystem paths even on Linux, where /refs/Foo.dll and /refs/foo.dll can be distinct files. If both paths are configured, the second path reuses the first path's cached member index and is never loaded, so valid external documentation can be missed; use the platform-aware filesystem comparer already established by src/ApiMark.Cpp/CppEmitter.cs:95-107.
  test/ApiMark.DotNet.Tests/ExternalXmlDocResolverTests.cs:339
• Deleting the XML file after the first miss does not distinguish a negative cache from a fresh lookup: an implementation that re-reads disk would also return null when the file is gone. Rewrite the file to contain T:Foo.Missing before the second call and assert that the second result is still null, so the test fails when the miss is not cached.
  test/ApiMark.MSBuild.PackageTests/PackageIntegrationTests.cs:298
• This success/file-exists assertion cannot prove that an explicit empty property suppressed harvesting: the build succeeds either way, and no generated content depends on external documentation. Make the test observe the child arguments or an external-inheritdoc result that disappears when the empty override is honored; otherwise it will not catch the target's current overwrite behavior.

• Files reviewed: 58/58 changed files
• Comments generated: 4
• Review effort level: Lite

---

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

[Copilot]

🔵 Needs a closer look

Moderate findings remain in path handling, resolver lifecycle and caching, MSBuild opt-out semantics, and integration-test coverage.

Review details

Suppressed comments (8)

Previously missed (4) — in code that hasn't changed since the last review.

src/ApiMark.DotNet/DotNetGenerator.cs:113

• This comparer is case-insensitive on every platform. On Linux, two distinct reference directories that differ only by case can both be valid; deduplicating them here can prevent Cecil from searching the directory containing the needed assembly. Use the platform-aware path comparer used by CppEmitter instead.
  src/ApiMark.DotNet/ExternalXmlDocResolver.cs:50
• The per-path cache is also case-insensitive on Linux. If the first configured path (for example /tmp/Foo.dll) has no XML file and a second, case-distinct path (/tmp/foo.dll) does, the second lookup hits the first path's cached null and never loads its valid documentation. Match the repository's platform-aware file-system comparer here.
  src/ApiMark.DotNet/ExternalXmlDocResolver.cs:203
• The fallback swaps the first ref/lib segment it encounters, so a parent directory named lib or ref can be changed instead of the package layout segment. For example, /var/lib/.nuget/packages/Pkg/ref/net8.0/Pkg.dll is probed under /var/ref/... rather than the corresponding .../lib/net8.0 directory. Select the layout segment nearest the DLL (or otherwise identify the package ref/lib segment) instead of stopping at the first match.
  src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets:67
• An empty value declared in the project (for example, <ApiMarkReferencePaths></ApiMarkReferencePaths>) still satisfies this condition and is overwritten by the harvested list; the command-line test does not expose this because global properties are protected from reassignment. If project-level empty is part of the “explicitly set” contract, add a definedness/opt-out mechanism and an integration test that declares it in the fixture.

src/ApiMark.DotNet/DotNetGenerator.cs:112

• Path.GetDirectoryName("External.dll") returns an empty directory, so a valid relative --reference-paths External.dll is filtered out here. The XML resolver can still see the sibling file, but Mono.Cecil receives no search directory and bare cross-assembly <inheritdoc /> chain construction fails; normalize the path before extracting its directory.
  src/ApiMark.DotNet/DotNetGenerator.cs:113
• On the success path the new DefaultAssemblyResolver is never disposed; only the exception path disposes it. Mono.Cecil's resolver caches external AssemblyDefinition instances, so in-process library callers can retain dependency memory/file handles after Emit and repeated generations can accumulate them. Tie the resolver to the emitter/model lifetime and dispose it after emission rather than relying on the CLI process exiting.
  src/ApiMark.DotNet/DotNetGenerator.cs:113
• These path assumptions break valid configurations: a bare relative reference such as --reference-paths External.dll has no directory and is filtered out, so Cecil cannot resolve the external base even though the file is valid; additionally, OrdinalIgnoreCase collapses distinct reference directories on case-sensitive Linux. Normalize/fallback to the current directory and use a platform-aware filesystem comparer here.
  test/ApiMark.MSBuild.PackageTests/PackageIntegrationTests.cs:261
• This assertion does not prove that auto-harvesting occurred: the fixture contains no <inheritdoc /> that needs the referenced package, and api.md would still be generated if the new @(ReferencePath) ItemGroup were removed. Add an external-inheritdoc fixture/assertion or otherwise capture the task arguments so this integration test fails when harvesting is broken.

• Files reviewed: 66/67 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Moderate findings remain in resolver fallback, external inheritance chaining, MSBuild option handling, and test coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

src/ApiMark.DotNet/ExternalXmlDocResolver.cs:220

• The fallback swaps the first ref/lib segment, so a valid package path with an earlier segment of the same name (for example /home/lib/.nuget/.../ref/net8.0/Foo.dll) probes the wrong directory and misses the XML file. Search from the assembly path backwards so the package's ref/lib segment is selected.

docs/design/api-mark-dot-net/dot-net-generator.md:128

• This design note repeats that the assembly resolver is not disposed on the success path, but the changed emitter now owns and disposes it after emission. Keeping this statement makes the documented ownership/lifetime contradict the implementation and can mislead API callers about using the resolver after Emit; update the design contract to describe disposal with the returned emitter.

  explicitly added this way. The resolver is intentionally not disposed on the
  success path (see the class-level remarks / Known limitation below); it is
  disposed on the failure path alongside the parsed assembly.

docs/verification/api-mark-dot-net/external-xml-doc-resolver.md:99

• Deleting the XML file cannot prove the negative cache: a fresh lookup after deletion would also return null. The test actually rewrites the file with the previously missing member and asserts the second lookup remains null, which is the distinguishing evidence; update this verification text to match that behavior.
  src/ApiMark.DotNet/DotNetGenerator.cs:117
• This deduplicates reference directories case-insensitively on every platform. On Linux, valid distinct paths such as /tmp/Lib and /tmp/lib can contain different referenced assemblies, so dropping one can make Mono.Cecil fail to resolve an external base/interface. The established filesystem convention uses StringComparer.Ordinal on Linux (src/ApiMark.Cpp/CppEmitter.cs:95-107); use that platform-aware comparer here as well.
  src/ApiMark.DotNet/XmlDocReader.cs:432
• An externally resolved member is returned as a raw XElement, but if that member's own documentation contains a bare <inheritdoc />, ResolveInheritdocSource can only consult _inheritanceChain, which is built for the primary assembly and has no entry for the external member. A chain such as primary C.M → external I1.M → external I2.M therefore still loses its documentation; either provide inheritance metadata for referenced assemblies or explicitly constrain and test this behavior.
  test/ApiMark.DotNet.Tests/DotNetGeneratorTests.cs:2305
• This end-to-end test is described as proving external base-type and interface resolution, but it only asserts ExternalInterfaceMethod. A regression in the external ExternalBaseClass.DescribeBase inheritance path or its XML lookup would still pass; add an assertion for the DescribeBase page containing Describes the base implementation..

• Files reviewed: 68/69 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (7)

.reviewmark.yaml:316

• The new shared src/ApiMark.DotNet/FileSystemPathComparer.cs is not included in any ReviewMark path set, while the other changed .NET source files are explicitly assigned to the generator or resolver sets. As a result, the review plan does not cover this new path-comparison implementation; add it to the appropriate set so ReviewMark enforces review coverage for the changed source.

      - src/ApiMark.DotNet/ExternalXmlDocResolver.cs
      - test/ApiMark.DotNet.Tests/ExternalXmlDocResolverTests.cs

docs/reqstream/api-mark-msbuild/api-mark-task.yaml:54

• This requirement says the targets must not override any explicitly set ApiMarkReferencePaths value, but the implementation intentionally repopulates the property when the explicit value is empty because MSBuild cannot distinguish empty from unset. Narrow the requirement to non-empty explicit values or state the required ApiMarkDisableReferencePathsHarvest opt-out so the requirement matches the documented behavior and test.

              ApiMarkTask's .targets file shall automatically populate
              ApiMarkReferencePaths from the resolved @(ReferencePath) items when
              ApiMarkReferencePaths is not explicitly set, and shall not override
              an
              explicitly set value.

docs/verification/api-mark-msbuild/api-mark-task.md:174

• This verification scenario claims that an explicitly empty ApiMarkReferencePaths suppresses harvesting and names a test that does not exist. The implementation and the adjacent integration test document the opposite: an empty value cannot be distinguished from unset, and ApiMarkDisableReferencePathsHarvest=true is the opt-out. Please align this scenario with the actual behavior and test name.
  src/ApiMark.DotNet/ExternalXmlDocResolver.cs:71
• The constructor keeps the caller-owned IReadOnlyList by reference while this type caches misses for the lifetime of the instance. If a caller passes a mutable List<string> and adds a reference after a miss, the new path is never searched for that member because _memberCache still returns the old negative result. Snapshot the paths at construction so the cache and search set stay consistent.
  src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets:75
• The new .targets contract also promises that a user-supplied ApiMarkReferencePaths value is not overwritten, but the package integration tests only prove default harvesting and the separate disable switch. The direct task test bypasses this target logic, so add a real MSBuild integration case with a non-empty explicit path and assert that the effective property remains exactly that value.
  src/ApiMark.Tool/Cli/Context.cs:603
• pattern is misleading here: this value is a single assembly file path, not a wildcard pattern, and the surrounding repeatable path option uses path-oriented names. Rename it to path so the parser's local variable matches the ReferencePaths contract.
  test/ApiMark.MSBuild.PackageTests/PackageIntegrationTests.cs:349
• This checks whether all captured stdout is whitespace, but dotnet build -getProperty emits a property report (including an object even when the property's value is empty), and the helper also captures normal build output. Consequently the test will fail even when auto-harvest is correctly disabled. Assert the extracted ApiMarkReferencePaths value is empty instead of checking the entire stdout stream.

• Files reviewed: 69/70 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical lookup issues and moderate type-support, command-line-size, test-isolation, and documentation findings remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

docs/design/api-mark-dot-net/external-xml-doc-resolver.md:88

• The implementation scans backward and swaps the last ref/lib segment (lines 218-230), but this design description still says it swaps the first segment. Please update the description to say "last" so the documented fallback matches the behavior for paths containing an earlier segment with the same name.

**SwapRefLibSegment** (private static): Swaps the first path segment named
exactly `ref` or `lib` (case-insensitively) for the other, mimicking the
folder layout convention used by many NuGet packages where compile-time

src/ApiMark.DotNet/DotNetGenerator.cs:139

• The new external resolver does not make bare type-level <inheritdoc/> work: BuildInheritanceChain/BuildTypeInheritanceEntries only add method, property, and event IDs, so there is no T:... chain entry for a derived class or interface declaration. A type documented only with bare <inheritdoc/> therefore remains unresolved even with ReferencePaths, despite the new contract claiming base types/interfaces are supported. Add type inheritance candidates (and tests), or narrow the documented/required scope to member implementations.
  src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets:83
• This default serializes every resolved @(ReferencePath) into ApiMarkReferencePaths, and ApiMarkTask forwards each one through ProcessStartInfo.ArgumentList. Large projects with many or long reference paths can exceed the host OS command-line limit (especially Windows' ~32K limit), causing the post-build task to fail before ApiMark runs. Avoid requiring the full reference set on the command line (for example, use a response/config file or resolve the references inside the task) or explicitly define a bounded strategy.
  test/ApiMark.MSBuild.PackageTests/Fixtures/DotNet/SampleLibWithReference/SampleLib.csproj:35
• This new package reference makes the isolated integration tests depend on a network restore: RunInIsolation redirects NUGET_PACKAGES to a fresh temp directory, while its generated nuget.config only has the ApiMark package locally and still lists nuget.org (PackageIntegrationTests.cs:529-538). Consequently every run must download Newtonsoft.Json, so these tests fail in offline/restricted CI despite the package-test harness being designed to use local packages. Use a local fixture/package source (or an SDK/in-repository reference) instead of introducing an uncached nuget.org dependency.

• Files reviewed: 69/70 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/ApiMark.DotNet/ExternalXmlDocResolver.cs:224

• On case-sensitive systems this treats directory segments such as REF or Lib as the NuGet ref/lib layout even though they are different paths on Linux. If an unrelated lowercase counterpart exists, a reference under /pkg/REF/... can resolve documentation from it incorrectly. Use the shared platform-aware comparer for these segment checks (ordinal on Linux, ignore-case only on Windows/macOS).
  src/ApiMark.DotNet/ExternalXmlDocResolver.cs:77
• The per-file cache is keyed by the raw path string, so aliases such as ./ref/Foo.dll and /cwd/ref/Foo.dll (or equivalent separator forms) are treated as different entries and cause the same XML file to be parsed twice. That violates the documented "each reference assembly's XML documentation file is parsed at most once" guarantee and is especially costly when MSBuild supplies many references; normalize and deduplicate paths when taking the snapshot.
  src/ApiMark.Tool/Cli/Context.cs:603
• An empty value passed as --reference-paths is accepted and added to the configured list. That makes ReferencePaths.Count > 0 and later lets ExternalXmlDocResolver probe Path.ChangeExtension("", ".xml") (and the resolver search directory resolve from the current directory), so a working-directory .xml file could be treated as external documentation instead of the option being ignored or rejected. Reject whitespace-only paths before adding them.

• Files reviewed: 71/72 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Four unresolved review findings remain, including two moderate issues requiring code and test fixes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

docs/design/api-mark-dot-net/external-xml-doc-resolver.md:87

• This description says the ref/lib match is case-insensitive everywhere, but SwapRefLibSegment uses the platform-aware comparer, so ref and REF do not match on Linux. Please document the platform-specific behavior here so the design does not promise a fallback that the implementation intentionally does not provide.

exactly `ref` or `lib` (case-insensitively) for the other, mimicking the

src/ApiMark.DotNet/ExternalXmlDocResolver.cs:85

• Blank entries are filtered only in the CLI/MSBuild argument path; direct callers can set DotNetGeneratorOptions.ReferencePaths (or construct this public resolver) with "". Path.GetFullPath then treats that as a real path, so the resolver can probe an unrelated XML file relative to the current working directory, while DotNetGenerator.Parse also derives an incorrect search directory. Normalize/filter empty entries at the shared API boundary before both resolver and Cecil setup.

• Files reviewed: 71/72 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/ApiMark.MSBuild/ApiMarkTask.cs:667

• A failure to delete the temporary response file escapes the finally block and can turn an otherwise successful tool run (or the intended graceful false result) into an unhandled MSBuild task exception. This can occur when the file is briefly locked or the temp directory denies deletion. Treat cleanup failures separately—log them and preserve the child-process result, while still attempting deletion.

src/ApiMark.DotNet/DotNetAstModel.cs:100

• The new AssemblyResolver field changes the model's ownership and disposal contract, but the companion docs/design/api-mark-dot-net/dot-net-ast-model.md still documents only Assembly as the owned resource and omits this field from the data model. Please update that design artifact so the parse/emit lifetime and resolver dependency are traceable alongside the source change.
  src/ApiMark.DotNet/DotNetEmitter.cs:53
• This makes the emitter single-use: after the first Emit returns, both the parsed Cecil assembly and resolver are disposed, so a second Emit cannot produce the alternate output format. That regresses the IApiEmitter contract, which explicitly says the same parsed data can drive different formats without reparsing (src/ApiMark.Core/IApiEmitter.cs:7-12). Please move ownership to an explicit emitter lifetime (or otherwise keep the model alive across emits), or update the public contract and callers if single-use is intentional.
  src/ApiMark.DotNet/FileSystemPathComparer.cs:165
• On a case-sensitive file system, the case-insensitive fallback can canonicalize a path that does not exist to an arbitrary existing sibling. For example, if both /tmp/Foo.dll is valid and /tmp/foo.dll is absent, normalizing /tmp/FOO.dll selects Foo.dll; if both case variants exist, enumeration order is only avoided when the input exactly matches one. This can make ExternalXmlDocResolver read documentation for an assembly the caller did not actually reference. Only accept the case-insensitive match after confirming the original segment path exists (or otherwise preserve the supplied casing on case-sensitive misses).

• Files reviewed: 72/73 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/ApiMark.Tool/Cli/Context.cs:557

• Response-file lines bypass the @@ branch because they are appended after expansion. Therefore a documented response-file value such as @@mylib reaches parsing unchanged and becomes @@mylib, unlike the equivalent command-line token; the current escape test covers only direct arguments. Apply the same leading-@@ unescape to response-file lines without recursively expanding single-@ lines.

src/ApiMark.MSBuild/ApiMarkTask.cs:655

• The new response-file parser treats every argument beginning with @ as a response-file token, but ApiMarkTask forwards all MSBuild property values verbatim. Consequently an existing value such as ApiMarkLibraryName=@mylib (and likewise any path/description beginning with @) now makes the tool try to read mylib and fail; the @@ escape only helps callers that construct the CLI arguments themselves. Escape literal-leading-@ values in the MSBuild task while preserving the generated response-file token, or constrain expansion to the intended response-file usage.
  src/ApiMark.Tool/Cli/Context.cs:538
• Expanding every top-level token that starts with @ changes the meaning of existing literal values. ApiMarkTask forwards C++ and shared values such as --includes, --library-name, and --output without applying the @@ escape, so a valid MSBuild value like @include is treated as a response-file path and the build fails before parsing. Either make expansion context-aware or escape literal @ values in the MSBuild argument-forwarding path, and cover this regression.

• Files reviewed: 74/75 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The review includes one critical and two moderate unresolved findings, including unsafe response-file creation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/ApiMark.DotNet/FileSystemPathComparer.cs:89

• The drive/UNC root is preserved exactly as supplied, so on Windows two spellings such as c:\... and C:\... (or differently cased UNC roots) normalize to different Ordinal keys even though they name the same location. That breaks the documented deduplication/at-most-once parsing guarantee and can make the resolver parse the same XML more than once; canonicalize the root prefix separately before combining it with the normalized segments.
  src/ApiMark.MSBuild/ApiMarkTask.cs:667
• This handler covers both PrepareArgumentsForProcess and RunToolProcess, so an IOException or UnauthorizedAccessException raised while starting the child process or handling its redirected streams is reported as “unable to create the reference-paths response file.” That produces a misleading diagnostic (and also affects invocations with no response file); limit this catch to response-file creation or use a separate process-failure message/path.

• Files reviewed: 74/75 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/ApiMark.DotNet/FileSystemPathComparer.cs:218

• On a case-sensitive filesystem, this fallback treats a wrong-case path as if it named the existing file whenever there is exactly one case-insensitive match. For example, configuring /tmp/foo.dll when only /tmp/Foo.dll exists normalizes to Foo.dll and can load unrelated external XML documentation instead of leaving the configured reference unresolved. Gate the case-insensitive fallback on the candidate path itself existing with the filesystem's normal semantics (or use a filesystem-aware comparison policy) so this normalization does not conflate distinct Linux paths.
  src/ApiMark.MSBuild/ApiMarkTask.cs:731
• The response-file format treats each line as a complete argument, but this writes each reference path verbatim. On Unix, a newline is valid in a file name, so a valid ApiMarkReferencePaths entry containing \n is split into multiple arguments (and could even become an injected flag), causing the child invocation to resolve the wrong path or fail. Encode line breaks in the response-file protocol or reject them before writing the file.

• Files reviewed: 74/75 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Four unresolved moderate findings affect resolver behavior/performance and MSBuild argument handling; a documentation nit also remains.

Review details

Suppressed comments (4)

src/ApiMark.DotNet/ExternalXmlDocResolver.cs:132

• With the new MSBuild default, ReferencePaths can contain every resolved dependency. On any external member miss, this loop parses the XML index for every configured reference before caching null; a project with many references and distinct unresolved/unsupported <inheritdoc/> targets therefore incurs an O(reference-count) set of disk reads and XML parses. Carry the resolved declaring assembly/path with inheritance targets, or otherwise index candidates by assembly, so a miss does not probe the entire auto-harvested dependency set.
  src/ApiMark.DotNet/FileSystemPathComparer.cs:218
• On a case-sensitive file system, a non-existent spelling can still have one case-insensitive match (for example, configured foo.dll when only Foo.dll exists). This branch currently treats that other entry as the requested path, so ExternalXmlDocResolver can silently load documentation for an assembly the supplied path does not identify (and the same issue applies to a wrongly-cased intermediate directory). Only accept the case-insensitive fallback after confirming that the original candidate path itself exists; otherwise leave the segment unchanged.
  src/ApiMark.DotNet/XmlDocReader.cs:543
• This external fallback still cannot resolve a common transitive case: if the external member returned here has its own bare <inheritdoc/>, the recursive call reaches ResolveInheritdocSource with an ID absent from the primary assembly's _inheritanceChain and returns null. Consequently a local override silently loses documentation even though the external XML contains an inheritance path; build/cache inheritance metadata for resolved external assemblies (or otherwise recurse their bare targets) so cross-assembly resolution remains correct beyond one hop.
  src/ApiMark.MSBuild/ApiMarkTask.cs:732
• PrepareArgumentsForProcess searches the entire logical argument list for the literal --reference-paths, even when that token is actually an unrelated option value. For example, an MSBuild ApiMarkLibraryDescription of --reference-paths makes this code consume the following flag as a reference-path value, write a malformed response file, and cause the tool invocation to fail. Restrict the substitution to the generated .NET reference-path pairs (or parse option/value pairs so values are skipped) instead of scanning arbitrary tokens.

• Files reviewed: 74/75 changed files
• Comments generated: 0 new
• Review effort level: Lite