From 97dfac477dda9179445fb87e48350fb2ffff55de Mon Sep 17 00:00:00 2001 From: Julien CHABLE Date: Mon, 7 Sep 2026 16:04:02 +0200 Subject: [PATCH] fix: close four Copilot review findings left open by merged PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of six findings from the reviews on #78, #81 and the §5.1 doc PR were still live on dev. Two were not: - Utf8Offsets.ToUtf16 returning a wrong index for an offset inside a multi-byte sequence was already fixed in #81 (`==` for `>=`, plus an explicit throw); nothing to do. - The "two consecutive blocks" in DocCommentSource.cs is a misread: the two summaries document StartsTag and IsOpaqueTagOpener, one each. Confirmed by building the producer solution clean. Fixed here: - The §5.1 design doc abbreviated two test names with an ellipsis, in a doc whose whole purpose is pinning behaviour to tests -- neither was greppable. Both now carry the full `Class.Method` name. The reviewer flagged one; the second is the same defect four dozen lines down. - ConceptSearch.TopDiversified took a null list all the way to scored.Count and surfaced a NullReferenceException, while its sibling TopDiversifiedBy throws ArgumentNullException. Guarded, with a test that was confirmed red first. - BundleWriter's IsWithinPrefixRoot doc opened a second before closing the first and closed both at the end. Malformed, though harmless today: producers inherits TreatWarningsAsErrors but does not set GenerateDocumentationFile, so no analyser reads it. - CheckTests asserted Scanner.Cache absent twice, ten lines apart. OKF4net 1273 tests green, producer 675 green, both solutions format-clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-01-okf-per-entry-usage-window-design.md | 4 ++-- .../src/OkfProducer.Core/Generation/BundleWriter.cs | 4 ++-- .../tests/OkfProducer.Tests/Generation/CheckTests.cs | 1 - src/OKF4net/ConceptSearch.cs | 2 ++ tests/OKF4net.Tests/ConceptSearchTests.cs | 9 +++++++++ 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/docs/superpowers/specs/2026-09-01-okf-per-entry-usage-window-design.md b/docs/superpowers/specs/2026-09-01-okf-per-entry-usage-window-design.md index 3e3dedae..6a6d0f4f 100644 --- a/docs/superpowers/specs/2026-09-01-okf-per-entry-usage-window-design.md +++ b/docs/superpowers/specs/2026-09-01-okf-per-entry-usage-window-design.md @@ -77,7 +77,7 @@ library never sees. `{}` as "nothing to override with" would be the per-field merge this design rejects. Both halves are pinned: `FrontmatterTests.EffectiveUsageWindow_falls_back_to_shared_when_the_entrys_override_is_not_a_mapping` - and `…_present_and_empty_entry_window_is_not_absent_and_does_not_fall_back`. + and `FrontmatterTests.EffectiveUsageWindow_present_and_empty_entry_window_is_not_absent_and_does_not_fall_back`. ## 4. Model @@ -141,7 +141,7 @@ Checked one at a time on `d6b778d`, **before** writing any implementation. | **C2** | Every `Source` construction site takes six arguments, positional or named | ✅ seven sites (`git grep -n "new Source(" d6b778d -- src tests`): three in `src/` (`OkfDocument.cs:212`, `OkfDocumentBuilder.cs:94`, `Provenance.cs:32`) and four in `ProvenanceTests.cs` (`:81`, `:95`, `:106`, `:115`) — an optional seventh member keeps all seven compiling | | **C3** | `usage_window` is consumed nowhere outside `Frontmatter`/`Validate` | ✅ no resolver, agent, viewer or CLI path reads it | | **C4** | The known-key list governs top-level keys only | ✅ `Frontmatter.cs:27-35` is a top-level list; a nested key inside a `sources` entry is not matched against it | -| **C5** | `ToYaml`'s round-trip is already exercised | ✅ `ProvenanceTests.ToYaml_round_trips_through_ParseSources_in_order` and `…_uses_canonical_per_entry_key_order` | +| **C5** | `ToYaml`'s round-trip is already exercised | ✅ `ProvenanceTests.ToYaml_round_trips_through_ParseSources_in_order` and `ProvenanceTests.ToYaml_uses_canonical_per_entry_key_order` | | **C6** | *(added by verification)* the producer-side write path covers the new field | ❌ **false** — see below | | **C7** | *(added by verification)* `Source(…, UsageWindow? UsageWindow = null)` compiles despite the member sharing its type's name, and every existing call site still builds | ✅ probed by actually adding the member and building: `OKF4net.sln` at 0 errors, 0 warnings, then reverted | | **C8** | *(added by verification)* nothing deconstructs `Source` positionally, so the arity change breaks no in-repo caller | ✅ no `is Source(…)`, no `Deconstruct` call anywhere in `src/`, `tests/`, `producers/`, `samples/` | diff --git a/producers/src/OkfProducer.Core/Generation/BundleWriter.cs b/producers/src/OkfProducer.Core/Generation/BundleWriter.cs index 1ac182ff..97f15f04 100644 --- a/producers/src/OkfProducer.Core/Generation/BundleWriter.cs +++ b/producers/src/OkfProducer.Core/Generation/BundleWriter.cs @@ -903,13 +903,13 @@ private static void RemoveEmptyDirectories(string outPath, string ownedPrefix, I /// Directory.Delete(recursive: true) and the bundle root. It is the most destructive /// statement on this branch, and until /// PruningTests.The_directory_ladder_stops_at_the_owned_prefix_root_and_not_above_it it was - /// on no test's critical path -- every other fixture left a sibling that broke the loop one rung in. + /// on no test's critical path -- every other fixture left a sibling that broke the loop one rung in. /// /// This comment used to describe a different scenario: a sibling directory whose name merely /// starts with the prefix (code2/ beside code/) being walked into and deleted. That /// cannot happen and never could -- the walk only ever goes upward, so it never reaches a sibling /// at all. The component comparison is still the right implementation; it just was not defending - /// against the thing the comment named. + /// against the thing the comment named. /// private static bool IsWithinPrefixRoot(string directory, string prefixRoot) => string.Equals(directory, prefixRoot, PathComparison) diff --git a/producers/tests/OkfProducer.Tests/Generation/CheckTests.cs b/producers/tests/OkfProducer.Tests/Generation/CheckTests.cs index c8d12e5e..57a8c53f 100644 --- a/producers/tests/OkfProducer.Tests/Generation/CheckTests.cs +++ b/producers/tests/OkfProducer.Tests/Generation/CheckTests.cs @@ -655,7 +655,6 @@ public void The_golden_bundle_holds_one_occurrence_of_each_shape() // The local function inside `Render` is Private too, so it is not a concept either. Assert.DoesNotContain(concepts, id => id.Contains("compose", StringComparison.Ordinal)); - Assert.DoesNotContain("code/csharp/n/scanner/cache", concepts); var register = File.ReadAllText(Path.Combine(ProducerFixture.GoldenBundle, "code/csharp/n/registry/register.md")); var count = File.ReadAllText(Path.Combine(ProducerFixture.GoldenBundle, "code/csharp/n/registry/count.md")); diff --git a/src/OKF4net/ConceptSearch.cs b/src/OKF4net/ConceptSearch.cs index 1cc5cbe3..34397864 100644 --- a/src/OKF4net/ConceptSearch.cs +++ b/src/OKF4net/ConceptSearch.cs @@ -177,6 +177,8 @@ private static int ScoreConcept(Concept concept, IReadOnlyList terms) /// At most results, in the order they should be shown. public static IReadOnlyList TopDiversified(IReadOnlyList scored, int count) { + ArgumentNullException.ThrowIfNull(scored); + if (count <= 0 || scored.Count == 0) { return []; diff --git a/tests/OKF4net.Tests/ConceptSearchTests.cs b/tests/OKF4net.Tests/ConceptSearchTests.cs index 6f3d8470..dd37f800 100644 --- a/tests/OKF4net.Tests/ConceptSearchTests.cs +++ b/tests/OKF4net.Tests/ConceptSearchTests.cs @@ -366,6 +366,15 @@ public void TopDiversifiedBy_returns_empty_for_an_empty_input_or_a_non_positive_ Assert.Empty(ConceptSearch.TopDiversifiedBy(["code/a"], FamilyOf, 0)); } + [Fact] + public void TopDiversified_rejects_a_null_list() + { + // Same contract as TopDiversifiedBy below: a null list is a caller + // mistake worth naming. Without the guard it fell through to + // scored.Count and surfaced as a NullReferenceException instead. + Assert.Throws(() => ConceptSearch.TopDiversified(null!, 1)); + } + [Fact] public void TopDiversifiedBy_rejects_null_arguments() {