fix(producer): empty the findings register, then fix what an adversarial review found in that work - #81
Merged
Merged
Conversation
…ly half closes Working the register's parked Importants after sifting the 24 unverified ones against the code (13 were already closed by the remediation waves; 11 were not). **E2-2 — the CHANGELOG had no entry at all for the code-graph stage.** Grepping `[Unreleased]` for `okfgen|code-graph|tree-sitter` found nothing; CLAUDE.md, ROADMAP.md and the producer README had all been updated in detail and the changelog had not. Four entries now: the stage itself, the completeness line, `--check`, and the recursive `.sln` detection. **B1-2 — one unreadable `.csproj` aborted the whole run.** `FileEligibility.ReferencesTestSdk` caught `XmlException` and `IOException` from `XDocument.Load` and nothing else, and it is called from `CodeGraphBuilder.Build`'s per-file loop, which deliberately does not wrap its body -- so an `UnauthorizedAccessException` did not degrade one file, it ended the repository's run. Now filtered on the five ways that call can fail on a path that existed a moment ago. The comment says in its own words that this is NOT covered by a test and why: the two provokable exceptions were already caught, and the three added need either an elevation the suite does not have or a loader seam this type does not offer. **D3-1 — the escape guard could be deleted with the suite green.** `SourceOwnershipMapTests` queried the ABSOLUTE spelling twice, but removing `SourceOwnershipMap.Relativize`'s escape guard stores the entry under its `../`-prefixed relative form, which neither lookup asked about. The test now also asserts on that spelling. Verified by mutation: with the guard replaced by `return relative;` the test fails, and it is the only one that does. **B2-3 — improved, NOT closed, and the commit says so rather than the register.** `The_size_check_never_loads_an_oversized_file_into_memory` asserted only `FileStatus.SkippedTooLarge` over content that decodes cleanly, so the ordering it is named for was invisible to it. The fixture is now undecodable bytes, which separates a decode hoisted above the size check (`SkippedEncoding`) from the correct order (`SkippedTooLarge`). MEASURED: this still does not catch the regression the finding actually names -- moving `File.ReadAllBytes` above the length check keeps the test green, because the decode that would betray it runs later either way. Catching a read requires observing the read, which needs a seam. Left open deliberately, not silently. 611 tests green, format clean, no fixture touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…ember read seam B2-3 and B1-2 were both blocked on the same thing: the claims are about a read, and a `FileStatus` cannot describe one. Arbitrated with the user, who chose the seam over documenting the gap. `IFileSystemReader` has exactly two members -- `TryGetLength` and `OpenRead` -- and `SystemFileReader` is the default everywhere, so every seam parameter carries a default and no production call site changed. It is not a filesystem abstraction: writes, enumeration and metadata stay on `File`/`Directory` where their callers already are. **B2-3 closed.** `The_size_check_never_loads_an_oversized_file_into_memory` named an ordering it could not see: rejecting on the declared length and reading first then rejecting both end at `SkippedTooLarge`. Measured on the previous commit -- hoisting `File.ReadAllBytes` above the length check left it green. A recording reader now asserts the file was never opened, and the same mutation against the new code fails it. **B1-2 closed.** `ReferencesTestSdk` is reached from `CodeGraphBuilder.Build`'s per-file loop, whose body is deliberately unwrapped, so an escaping exception ends the repository's run rather than skipping one file. The catch was widened in the previous commit with a comment admitting it was untested; a reader that grants metadata and refuses to open now provokes exactly that, and removing `UnauthorizedAccessException` from the filter makes the test throw. Two deliberate non-changes. Reparse-point detection stays on the real `FileInfo` and is NOT routed through the seam -- it is a containment decision, and a seam able to answer it is a seam able to waive it. And an unreadable `.csproj` keeps its existing direction, "not demonstrably a test project", so the file stays in scope: treating it as a test project would silently drop real source on a permissions accident. 612 tests green, format clean, no fixture touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…critical path
**D3-3.** `IsWithinPrefixRoot` is the only thing between `RemoveEmptyDirectories`'
walk and the bundle root, and no test put it on the critical path: every fixture
in `PruningTests` leaves a sibling directory that breaks the loop after one rung,
so the walk never climbed far enough for the guard to be what stopped it.
The new test owns `code/csharp` rather than `code`, and that is the whole
fixture. With `code`, the rung above the prefix root is the bundle root, which
`overview.md` already protects through the files check -- so deleting the guard
changes nothing observable and a test written that way would prove nothing. With
`code/csharp`, the rung above is `code`, empty by then, with no file and no
sibling to protect it. Verified by mutation: replacing the guard with `true` fails
this test and only this test.
**E2-1.** Nothing in the solution called `Run("validate", ...)`, so everything the
composition root wires for that verb was held by no test -- while `CliTests`' own
doc claimed it exercises "the shipped composition, in process". Four tests now:
the conformant path, the non-conformant path, the `BundleLoadException` branch,
and the option name.
Two mutations, both measured: inverting `IsConformant ? 0 : 1` fails the first two;
renaming `--okf` to `--bundle` fails the fourth.
The fourth test needed correcting before it did. Its first version asserted over a
bundle that was never generated, so `--bundle` on a non-existent path exited
non-zero whether the option was recognised or not -- it held under the rename it
was named for, and the mutation is what showed it. It now generates the bundle
first, and the comment says why that is the fixture rather than setup.
617 tests green, format clean, no fixture touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
… it pins, surface the dirty-tree caveat **C1-2 — `RoslynResolver.IsAvailable` and `IsComplete` removed.** Remediation wave 2b had already found that nothing in `producers/src` reads either one (the run's report comes from `GenerateRun.ReportProjects` iterating `Projects`) and had corrected the docs and §7.2 accordingly, choosing to KEEP the properties on the grounds that a host embedding the resolver would ask that question. Reversed here on the user's arbitration: no host does, `producers/` is not a published library, and a property whose only readers are the tests asserting on it is the exact shape this branch has paid for thirteen times over. The two predicates now live in `RoslynResolverTests` as `AnyCompiled` and `AllCompiled`, derived from `Projects` in one line -- which is what any caller wanting the summary would write. The `Count > 0` clause is documented there rather than lost: `All` over an empty list is vacuously true, and an empty project list is precisely the state in which every call fell back to name matching. Fourteen assertions rewritten, including the scratch fixture's own oracle. §7.2 records the reversal beside the decision it overturns rather than replacing it. **F-3 — the test is named for what it pins.** `The_registry_spans_the_code_family_as_well_as_packages_and_docs` built an EMPTY graph, so no `code/` id was produced and reverting the code family to a `Generate`-local `usedIds` left both assertions green. Adding code concepts would not have fixed that, and the comment now says why rather than leaving it to be rediscovered: the four families use disjoint prefixes by construction, so a cross-family collision cannot be built and a code-local registry would disambiguate code ids among themselves exactly as the shared one does. §3.4's "one registry" is structural -- it is why a class of collision is impossible -- not behavioural, and no test can separate it from its absence without changing the id scheme. What the test does pin, and what a regression could break, is that a doc titled "overview" coexists with the bare `overview` id. **D4b-1 — the dirty-tree caveat reaches a user.** `GitRevision` stated correctly, in a code comment and nowhere else, that `revision` names the committed HEAD and so can name a commit the bundle was not generated from. It now also appears in `--rev`'s help, in `BundleDrift.CheckDescription` (where it matters most: both sides carry the same revision, so `--check` can report no drift for a bundle neither side was generated from), and in a README section of its own. The fourth surface the finding names -- the `overview` concept itself -- is deliberately NOT in this commit. Writing it changes emitted bytes and therefore the golden, which is regenerated once at the end of this remediation rather than twice. 617 tests green, format clean, no fixture touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…ld cannot tell apart Concept ids change for the shapes below, which is what the user arbitrated. D3-2's new test is what covers the run that follows such a change. **D1b-I1 — generic arity, on TYPES only.** `Foo`, `Foo<T>` and `Foo<T, U>` in one namespace all report `name` as `Foo`, so they collapsed into one symbol group and rendered as overloads of a single member. §3.2's merge rule was written for method overloads, where merging is the point; extending it to unrelated types was never the spec's intent. The suffix is `_N`, not `-N`: the registry already appends `-2` to disambiguate two concepts wanting one id, and a reader must be able to tell "arity 1" from "second thing called Foo". So `foo`, `foo_1`, `foo_2` -- and `foo_1-2` if two `Foo<T>` somehow met. Members are deliberately left alone, generic ones included, and a test says so rather than leaving the asymmetry to look like an oversight. A call site captures its callee as the bare identifier -- `Bar<int>()` yields `Bar` -- so qualifying a generic METHOD would stop every call to it from matching by name. That trades a merged concept for a lost edge, which is the worse of the two. **D1b-I2 — explicit interface implementations.** `public void Bar()` and `void IFoo.Bar()` on one type also both report `Bar`, and collapsed into one concept carrying ONE description -- the first declaration's -- and both signatures, for two deliberately different implementations. The qualified form takes the interface as a dotted prefix (`ifoo.bar`), which is how C# writes it. Name matching no longer reaches the explicit member, and that is the correct outcome rather than a cost: it is not callable as `Bar()` on the type, so a call that used to bind to it was binding to the wrong member. Both were measured against the vendored grammar before being written, not inferred: `type_parameter_list` with N `type_parameter` children, and `explicit_interface_specifier` as a child of `method_declaration`. The whole suite was green before these changes, which is the finding's point -- no test covered either shape. **D3-2 — a rename is covered.** The one destructive interaction no test ran: old id pruned, new id written, in one run. §6.3's "not visited AND not on disk" inference was accepted partly on the claim that it was covered; it was not. Both directions are asserted, and so is the manifest tracking the rename rather than accumulating the old id, which would make the next run treat it as a candidate again. 621 tests green, format clean, no fixture touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…a same-named type D1b-I3, the third residual collision §3.3 did not list. A class `Bar` in namespace `Foo` and a class `Baz` in namespace `Foo.Bar` reduce to the same raw path `[code, csharp, Foo, Bar]` for their parent, so the container was never synthesized -- a group already claimed the path -- and `Baz` registered under the TYPE's id. Measured on exactly the fixture the new test uses: `code/csharp/foo/bar/baz`, a top-level type rendered as a nested one, with the namespace having no concept at all and nothing dangling for `okf validate` to see. **Why the arbitrated fix had to change shape.** The chosen option was "disambiguate the container, like D1a-3". Measuring it first showed that closes only half: the children compute their parent key from their own container string, which still resolves to the type, so a discriminated container would exist and be empty -- a misleading concept added without removing the confusion. Put back to the user, who chose the real fix. **What separates them.** `SymbolFact.Container` is a flat dotted string, so a type nested in `Bar` and a top-level type in `Foo.Bar` both report `"Foo.Bar"`. The extractor knows the difference -- it walks ancestors and sees a namespace node in one case and a type node in the other -- and the flattening threw it away. `ContainerNamespace` now carries the namespace part, and the rule is a depth comparison: a group's parent is a namespace exactly when the parent's depth equals the namespace's. Two of the four rows in the new test have IDENTICAL raw segments and opposite parents, which is what no rule over the string can express -- the second test is that control, and it is the one a container-string fix would break. An `init` property defaulting to null, the same shape as `HeaderEndLine`: every fixture in this solution keeps compiling, `RoslynResolver` is untouched (it builds no `SymbolFact`), and a group with no recorded namespace is left exactly as it was, so the pass is inert rather than guessing. The namespace gets a marked raw path and its own id (`code/csharp/foo/bar-2`); the TYPE keeps the path it had, so no existing type's id moves. The marker is a keying device only: `SegmentName` now strips it everywhere a raw segment becomes something a reader sees. That was already true of a group's segments by accident -- ids and titles come from the `SymbolFact` -- but NOT of a synthesized container, whose id, title and description-chain fact are all derived from the segments, so a marker would have reached output. Verified by mutation: neutralising the new pass fails the collision test and leaves the nesting control green. 623 tests green, format clean, no fixture touched. §3.3 records the collision, the depth table, and what the pass deliberately does not do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
INT-I5. Spec 6.2's determinism-versioning mechanism was entirely absent: no
`packages.lock.json` anywhere, both engines pinned by a floor `Version=` only, and
`generated.by` carrying `okfgen/{version}` alone. Grepping the ledger, the plan and
the README for `lock|packages.lock|RestorePackages` found nothing -- an omission
rather than a deferral, and it undid 6.2's "a version bump is a deliberate
migration, never a drift you suffer".
No format decision was needed: 6.2 prescribes the spelling
(`okfgen/0.1.0 tree-sitter/x.y.z roslyn/a.b.c`), the surface (`overview`), and the
scope of the lock (the `CodeGraph.*` projects). Measured on the fixture repository
after the change:
by: okfgen/0.1.0 tree-sitter/1.3.0 roslyn/5.3.0-2.26078.5
**Locking and recording answer different halves.** A lock file makes a bump
deliberate; it cannot make a diff interpretable. Without the versions in the
artefact, a golden that moved says only that something changed.
**Each engine names itself.** `OkfProducer.Core` references neither, and should
not, so `TreeSitterExtractor.EngineVersion` and `RoslynResolver.EngineVersion` read
their own loaded assembly and the CLI -- the one project referencing everything --
collects them. `EngineVersions.Token` lives in Core so the two cannot come to spell
a version differently: informational version preferred (that is the package version
a reader can look up), anything after `+` dropped (SourceLink puts the commit sha
there, which would churn the field on rebuilds that changed no dependency), and
`unknown` rather than omission when an assembly declares neither -- a run whose
engine version could not be read is a run whose determinism claim is weaker.
**Two deliberate scopes.** On `overview` alone, like `at` and for 6.1's reason: the
engines are a fact about the run, and repeating them on hundreds of `code/`
concepts would rewrite every file on a bump that changed nothing in them. And none
at all under `--no-code`, where no engine touched the repository -- the same reason
that path passes a null manifest rather than an empty one. Both are pinned by their
own test.
Verified by mutation: dropping the engine tokens from the emission fails the first
test and only it.
626 tests green, format clean, no fixture touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…two defects it found F-2, F-4, F-1 and D4b-1's last surface. The golden was captured by a reduced pipeline over a fixture holding three classes and their methods: no interface, struct, record or enum, no constructor, event, field or local function, no nested or generic type, no explicit interface implementation, no block-scoped namespace, no non-public visibility, and no type whose header spans more than one line. Deleting the whole escaping layer would not have moved one golden byte. **F-2, the pipeline half.** `ProducerFixture` never set `SourceOwnership`, so the package -> namespace link was structurally absent and the whole `code/` family was an orphan in the captured bundle -- 0 of 12 concepts reachable from `overview`, measured. The map is now supplied directly rather than through MSBuild, and the reason is recorded where the code is: the fixture repository is never restored (committing an `obj/` would be machine-specific), so `dotnet msbuild` fails on it with NETSDK1004 and Roslyn compiles nothing. Composing the real query would add a network- and SDK-dependent restore to the one test that is fast and offline, and still capture zero `Exact` edges unless it succeeded. Arbitrated with the user. `Exact` edges therefore stay out of the golden, deliberately and in writing; `RoslynResolverTests` covers that engine against repositories it restores itself. **F-4.** No fixture type had a multi-line header, so R48's header cap -- the churn bound the whole id scheme rests on -- would have produced a byte-identical golden if it returned `StartLine + 1`. `Boxed`'s header spans three lines now, and that substitution fails the golden. **F-1.** `fixtures/README.md` said the regeneration command "asserts against what it just wrote". It does the opposite: update mode refuses to assert, because the expected side is produced by the same harness as the actual side, and a tautology reported green is how a stale variable disarms a golden. The README now says the command exits RED by design and that the procedure is two runs. **D4b-1's fourth surface.** The dirty-tree caveat now reaches the `overview` concept, where a reader of the bundle is. Pinned in `DeterminismTests` rather than the golden, and that is not a workaround: the golden is captured outside git, so it has no `revision` and correctly carries no caveat. Both directions are asserted. ## The two defects the enriched fixture found immediately Both are mine, from the batch that separated generic arity and explicit interface implementations, and neither had a test. **Members of a generic type all landed under the first arity.** `Holder`, `Holder<T>` and `Holder<T, U>` became three type concepts while `Count`, `Value` and `Find` all registered under `holder`, because a member's container path read the ancestor's bare `name` field. Ancestors now go through the same qualification the declaration itself gets. **That fix then desynchronised the two engines**, which is worse than the bug it fixed: `RoslynResolver.ContainerPathOf` exists precisely to spell a container the way the extractor does (C1-1), and it still used the bare identifier token. A spelling only one side knows makes `CodeGraphBuilder` overwrite the baseline with a non-joining `Exact` and then degrade it -- the graph left strictly worse than not running the resolver at all. `DeclaredName` is now the single place that rule lives on the Roslyn side, used for both a leaf name and every ancestor segment. The existing explicit-implementation test caught the drift and is what pins the agreement. ## One register claim measured false D1b-I2 said a public `Bar()` and an explicit `IFoo.Bar()` "collapse into one concept, one description, both signatures". Not reachable: an explicit interface implementation carries no access modifier, so it is Private and out of scope under every flag, filtered before `ConceptGenerator` groups anything. The golden confirms it -- the fixture's `IEquatable<Boxed>.Equals` produces no concept -- and the inventory test now asserts that absence. The separation is still right one layer down, where two different members shared one name. 626 tests green, format clean. The golden grew from 15 concepts to 37, absorbing the id changes and the `generated.by` engine tokens in a single reviewed regeneration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
… themselves First tranche of the Minor register, taken in the order the user arbitrated: the one that protects the other tests, then the ones about test strength. **D3-9 -- nothing asserted that `--check` leaves its input bundle alone.** Six tests in `CheckTests` run `BundleDrift.Check` against the COMMITTED golden. A regression that regenerated in place rather than into the copy would rewrite the golden, and every one of those six would go on passing, comparing the bundle against itself. The golden was just enlarged from 15 concepts to 37, so it has more to lose than it did. Now hashed byte for byte, before and against after. Not a file count and not a timestamp: regenerating in place leaves the same names, and `generated.at` would not move either, since the golden is captured outside git where that field is excluded from the COMPARISON but not from a write. Verified by mutation: passing `bundlePath` where `Check` passes `copyPath` fails it. **D3-5 -- every "ordinal" assertion pinned sortedness, not Ordinal.** `Claimants_come_back_in_ordinal_order_whatever_order_they_arrived_in` compared `A` against `Z`, a pair that sorts the same way under every culture on earth. Swapping every `StringComparer.Ordinal` in `SourceOwnershipMap` for `CurrentCulture` left it green, while 6.2 pins Ordinal. The fixture is now `Z` against `a`: Ordinal compares code points, where `Z` (0x5A) precedes `a` (0x61), and a linguistic comparison puts `a` first. The two orders are opposite, so only one passes -- and the culture swap now fails it. **E2-4 -- `--force` was exercised by no test at all.** Documented as an alias for `--reset`; dropping the `|| forceOption` clause broke it silently, leaving an operator with a run that refuses a non-empty `--out` instead of recreating it. The fixture is what discriminates: a bundle holding a file the regeneration does not produce is the only shape where reset and no-reset differ observably. Verified by mutation. **D3-10 -- a comment defending against something that cannot happen.** `IsWithinPrefixRoot`'s doc said the component comparison stops a sibling whose name merely starts with the prefix (`code2/` beside `code/`) from being walked into and deleted. The walk only ever goes upward, so it never reaches a sibling. What the guard really does is stop the walk climbing ABOVE the owned prefix -- the sole condition between a `Directory.Delete(recursive: true)` and the bundle root -- and the comment now says that, and names the test that finally put it on a critical path. 628 tests green, format clean, no fixture touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
**D4b-2.** `GitRevision.CurrentBranch` asked `git symbolic-ref` and stopped there. Measured: on a zero-commit repository that call SUCCEEDS and returns the unborn branch, while `rev-parse HEAD` fails. So `--repo-url` without `--rev` emitted a `resource` permalink on every code concept, each pointing into a branch that contains none of the files -- and the CLI's "no branch name could be read" note never fired to say otherwise. A wrong link reads exactly as confidently as a right one, which §2.3 calls the worse outcome, and this is not an exotic state: it is every repository on its first run of this producer. `symbolic-ref` is now followed by `rev-parse --verify HEAD`, and only where the first succeeded -- a caller that passed `--rev` never reaches this method. That second invocation is a real cost on a path whose whole documented virtue is being honest about process spawning, so `producers/README.md`'s count goes from "two to four" to "two to five" and says why. The test asserts both directions on the SAME repository: no branch before the commit, a branch after it. Without the second half, a machine with no working `git` would satisfy the first for the wrong reason. Verified by mutation: dropping the verify fails it. **B1-9 -- `RunStatus.Complete` has no production constructor, and now says so.** Grepped, not assumed: every production `RunStatus` comes out of `CodeGraphBuilder.Build` with the statuses it observed, and the only callers of this factory are test fixtures. Deliberately NOT removed, unlike `RoslynResolver`'s two summary properties earlier in this remediation, and the doc records the distinction: those made a claim §7.2 repeated and the code did not honour, while this is a factory for the type's own least surprising value and removing it would churn fourteen call sites for no behavioural gain. If you would rather it move to the test project for consistency, say so -- it is a one-pass change, and I did not want to read one arbitration as covering a different case. 629 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…or a site nobody emitted Both are about the identity of a call site, and both had the same failure mode: a plausible WRONG answer rather than a missing one, which §2.1 calls the worse of the two because a wrong `## Calls` link reads exactly as confidently as a right one. **B1-7 -- off-boundary offsets were silently wrong, and inconsistently.** `Utf8Offsets` is the join key between two engines, and its class summary said both "only ever produce offsets that land on a codepoint boundary". That was an assumption, stated and unchecked, and each direction broke differently when it did not hold. `ToUtf8` ended its span on a lone high surrogate, which `GetByteCount` runs through the replacement fallback and scores as 3 bytes -- neither the 0 of the codepoint's start nor the 4 of its end. `ToUtf16` tested `utf8Count >= utf8Offset` and so ROUNDED UP: on `"A\U0001D11E"`, an offset one byte into the four-byte codepoint came back as the index after it, a real UTF-16 index for a position the caller never asked about. Both now refuse. The `>=` becoming `==` is the whole of the second fix, and the post-loop branch tells the two caller mistakes apart -- overshooting means inside a sequence, falling short means past the end -- so the message names the real one. The suite stayed green, which is the useful part of the result: no production path was relying on either rounding. **B1-4 -- the verdict dictionary took whatever a resolver handed back.** `verdicts[key] = edge` with no membership test, so an edge naming a site the extractor never emitted became a `## Calls` link attributed to a call that does not exist. `ISymbolResolver.Resolve`'s contract says a resolver answers the sites it was given; the dictionary is seeded from those sites, so membership IS that contract, and it is now enforced. Seeding switched from the indexer to `Add` for the same reason one property over: two sites sharing (path, offset) would be two calls at one byte, which the grammar cannot produce -- and if a future profile ever emits two, this throws rather than keeping whichever came last. The phantom test needed correcting before it discriminated, and the correction is recorded in it. Its first fixture gave the phantom an invented CALLER, so it was dropped by the pre-existing invariant that no edge may name a caller absent from `Symbols` -- machinery with nothing to do with this contract -- and the test passed with the guard removed. Measured. The phantom now differs from the real site only by offset, with a caller that really is declared, so only the new guard can stop it. 632 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…jects are still queried Closes the behaviour lot of the Minor register. Neither of these changes behaviour, and both say so: one had a right outcome reached by two mechanisms that did not mention each other, and the other was read as an inconsistency when the code has a reason. **B2-6 -- the ripple of an accepted gap, now pinned.** Indexers, operator overloads and conversion operators are not extracted, deliberately: none has a `name` field in this grammar. What was never stated is what happens to a CALL inside one -- it finds no ancestor in `CallerMemberAncestorNodeTypes`, so the site comes out with an empty caller, and an edge naming a caller that has no concept is what §2.1 calls worse than no edge. `CodeGraphBuilder`'s invariant removes it. Right outcome, reached by two halves neither of which referenced the other, so a change to either would have let the orphan through unnoticed. The test pins both halves AND the gap itself, so it fails loudly rather than vacuously if the profile ever starts extracting those members -- at which point the ripple stops being the right behaviour. **INT-M7 -- test projects are queried with `--include-tests` off, and should be.** The register filed this as an inconsistency. It is not: the flag decides which FILES are extracted, while this list decides which projects are asked for their `Compile` item sets, and §5.1's ownership map is built from those. A test project can legitimately claim a PRODUCTION file -- a linked `<Compile Include="..\..\src\Shared.cs"/>` is ordinary -- and dropping it would silently remove that file's `## Also compiled by` entry, which is a fact about the production file, not about the test project. The cost is stated rather than hidden: one `dotnet msbuild` per test project on every run, for item sets whose own files the run will not extract. Measured on this repository, no test project claims a production file, so skipping them would change nothing in the emitted bundle here -- the price buys correctness in the general case, not in this one. If you would rather trade that for the subprocess time, the filter is one `Where` and I have deliberately not added it. 633 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…tened into a description D1b-M3. `IDescriptionSource.Describe` documents its `Text` as "a complete sentence, not a fragment", and a `<code>` block's content flowed straight through it: a multi-line example arrived at `CollapseWhitespaceRuns` and came out as `var a = 1; var b = 2;` reading like the tail of a sentence. Neither the code nor a description, and it ships in the bundle. `<code>` is now an opaque tag whose CONTENT is dropped -- the same argument the viewer's sanitizer makes for `<script>` and `<style>`: what is inside them is source, not prose. `<c>` is deliberately not in that set, and a test says so: inline code is part of the sentence, and dropping its content would turn "returns `null` when the file is missing" into "returns when the file is missing". **A third test asserted truncation on an unclosed opener, and was wrong about the code.** Measured: an opener nothing ever closes is not marked as a tag at all, so the opaque rule never sees it and it is emitted verbatim -- the same branch that keeps `List<T> of results` intact instead of eating `T`. That is the right answer here for the same reason, since this producer reads arbitrary repositories where a lone `<code>` is likelier to be prose than a block whose content should vanish. The test now pins the real behaviour, and `SkipToCloser`'s doc no longer claims an unclosed-opener policy it never gets to apply -- its end-of-comment return is a backstop, not a rule. That correction is the third time this session a fixture claimed something the code does not do. Recording it in the test rather than quietly rewriting the assertion. 636 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…ng hid C2-2, and the last finding of the arbitrated Minor scope. The register listed three uncovered branches; measuring reduced it to two. Malformed JSON is already covered and covered well -- `ReadInputs` is `internal` and visible to the tests, with a case per guard and a message fragment for each, so a refusal firing for the wrong reason fails rather than passing the type assertion. The two that were left could only be reached by uninstalling the SDK or waiting two minutes, because the executable name and the deadline were hard-coded. The register names this absence as the reason C2-1 went unnoticed -- an unwrapped exception on the success path taking generation down for the whole repository -- so this is the gap that cost a Critical, not a cosmetic one. `Query` gains an `internal` overload taking the executable and the timeout, arbitrated with the user, in the same shape as the read seam earlier in this remediation. The public overload is the only production caller and passes `"dotnet"` and the two-minute deadline: these are not knobs an operator gets, they are what a test needs to make a real failure happen instead of describing one. Both tests assert the MESSAGE and not only the type, and the reason is specific: `Run` guards a missing working directory separately and reports that instead, and both refusals are `MsBuildQueryException` -- so asserting the type alone let either answer for the other, and the wrong one sends an operator hunting for an SDK that is installed. The timeout fixture spawns a real process that does not exit on its own, so the deadline is what ends it rather than the process finishing early and the test passing for the wrong reason. Verified by mutation, one per branch: filtering the `Win32Exception` catch out fails the missing-CLI test, and filtering the `OperationCanceledException` catch out fails the timeout test. 638 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…left unclassified **The four unclassified, measured.** C1-3 is CLOSED: `TargetKind.Unbound` exists and separates "Roslyn had no answer" from "genuinely external", which the register said were folded together. D2-M1 is NOT a miscount, as it was filed: on `--update --no-code` the manifest is null so every previously-claimed concept is a candidate, and the count is exactly right. C1-6 and D4b-3 were open and are handled below. **C1-6 -- `EdgeConfidence.Exact` promised more than the edge can carry.** A `ResolvedEdge` names its target by `(TargetContainer, TargetName)`, and §3.2 merges a method's overloads into ONE concept, so an exact binding to `Register(Scanner)` and one to `Register(Scanner, string)` arrive at the same key. Correct given the merge, and now stated: `Exact` says the resolver knew which declaration it meant, not that the bundle can express which one. Explicit interface members are no longer part of this -- the extractor names them apart -- so overload sets are the residual case and the only one. **D4b-3 -- two rows the permalink table omitted.** `javascript:` is the scheme a permalink must never carry into a bundle a viewer renders. Scheme-relative `//host/path` is rejected because .NET parses it as `Scheme="file"`, so it fails the http/https check as a parser quirk rather than by an explicit rule -- exactly the kind of correctness a future parser change flips silently, and now a row rather than a footnote. **D2-M1 -- the refusal note names the prefix.** The count was right and the sentence was not useful: a bare "678 concept(s) this run did not generate" reads as a loss report on a run that wrote its other families perfectly well. It now says which family. Nothing asserted any part of that sentence, so its wording was free to drift; `PruningTests` pins it now. **B1-5 -- an assertion that could not fail.** `A_file_matching_no_profile_is_skipped_without_affecting_completeness` asserted `IsComplete` over a stub that reports `Extracted` for anything it is asked about, so the assertion held for the property AND its negation. The same builder over a file a profile does claim, reporting a skip, now moves it -- so the `true` is a fact about the .txt file being unreached rather than about the stub. **D3-6 -- two `--check` tests asserted drift without the exit code**, the half a CI gate actually reads. `IsClean` and `ExitCode` are separate fields on `DriftReport`. **B1-8 -- two `[Theory]` declarations with one case each**, facts in theory costume. Each has a second value now. 642 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
… temp repo per test
**B1-10 -- `ExtractionLimits` accepted values that cannot mean what a limit means.**
A non-positive `Timeout` cancels the linked source immediately, so
`CodeGraphBuilder.Build` RAISED out of the walk instead of returning an incomplete
`RunStatus` -- the honest reporting path this type exists to feed. An operator who
typed a bad number got a stack trace where the design promises a run that says what
it could not do. A non-positive `MaxFileBytes` or `MaxDepth` is refused for the same
reason: it does not bound a run, it empties one.
**The first version of this validation did not work, and the test is what said so.**
Property initialisers validate the primary constructor only: a record's `with`
expression copies backing fields through the compiler-generated copy constructor and
does NOT re-run them -- so `ExtractionLimits.Default with { Timeout = ... }`, which
is how every caller in this solution builds one, sailed straight past it. Written as
explicit `init` accessors over backing fields now, and the doc records the
difference so the cheaper form is not reintroduced as a tidy-up.
**F-M1 -- the last dangling pointer.** The Roslyn project's comment cited
`producers/spikes/RoslynCompilationSpike` as live evidence for its central design
claim. That directory was deleted after the spike answered, which is what a spike is
for; the comment now says so and carries the recovery commit, so someone checking
the claim finds it instead of finding nothing.
**B1-6 -- nine leaked temp repositories per run.** `CodeGraphBuilderTests.SnapshotWith`
made one per call and deleted none. The class now tracks and disposes them, the same
shape `TreeSitterExtractorTests` and `HostileInputTests` already use.
647 tests green, format clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…p a second temp-dir leak **D3-4 -- a comment claiming a leak this row would catch.** `A_commit_that_does_not_touch_code_changes_only_overview` ended "with a wall-clock stamp instead of the HEAD one, this same assertion would come back with every concept in the bundle". False: only `overview` carries `at` and `revision` at all -- `DeterminismTests.Only_overview_carries_at_and_revision` pins exactly that -- so a wall-clock stamp still returns `["overview"]` here and this row stays green. The leak is caught by the `AddPrivateMember` row. The comment now says which. **B2-4 -- `ScopeTests.SnapshotWithProject` leaked a temp repository per case.** The class already tracked and deleted the ones `CreateRepository` made; this helper was static and made its own. Both are instance methods now. **D1b-M4 -- `_` is not a word boundary, and that is a decision.** Filed as an inconsistency with the PascalCase rule; recorded as intentional instead. `MAX_VALUE` slugifies to `max_value` because `_` is a valid concept-id character (`ConceptId.ValidateSegment` admits it) and the underscore the author wrote is information about the name rather than punctuation this producer invented. Splitting on it would also make `_field` and `Field` collide, which the three boundary rules exist to avoid rather than create. No id changes. **D4b-4 deliberately NOT done, and the reason is the interesting part.** Four structurally identical `TempDir` classes live across these test files, and consolidating them onto `ProducerFixture.TempDir` is the same two-copies cleanup the branch did for the permalink rule. I started it and stopped: the shared type lacks `Write`, and the `Write` I drafted for it created parent directories where the local copies do not -- a behaviour difference that would quietly change what a test using a missing directory is asserting. A four-file refactor whose first draft already had a semantic difference in it is not a tail-of-session change. Left open, with the trap named. 647 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
There was a problem hiding this comment.
🟡 Changes recommended
There are malformed XML documentation comments in changed code (DocCommentSource.cs and BundleWriter.cs) that can break builds when XML doc warnings are enforced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR works through the “parked findings register” for the OKF producer (producers/OkfProducer), primarily by fixing several real producer defects and (critically) upgrading tests/fixtures so previously non-failing assertions can now fail under mutation—without materially changing emitted bundle identities beyond a small, intended set.
Changes:
- Strengthen producer test suite and golden fixture to cover many more declaration shapes and prevent self-disarming golden checks (e.g.,
--checkmust not rewrite the bundle). - Fix multiple code-graph correctness issues (UTF-8/UTF-16 offset conversion boundaries, generic type arity disambiguation, explicit interface member naming, namespace/type path collisions).
- Improve generation metadata and operational clarity (engine versions recorded in
overview.generated.by,revisioncaveat, safer pruning notes / directory ladder guard coverage).
File summaries
| File | Description |
|---|---|
| producers/tests/OkfProducer.Tests/Generation/SourceOwnershipMapTests.cs | Adds stronger assertions for escape-guard behavior and ordinal ordering. |
| producers/tests/OkfProducer.Tests/Generation/PruningTests.cs | Adds coverage for pruning notes, prefix-root ladder stopping, and rename prune/write ordering. |
| producers/tests/OkfProducer.Tests/Generation/ProducerFixture.cs | Supplies ownership map + engine versions to make golden representative and interpretable. |
| producers/tests/OkfProducer.Tests/Generation/DeterminismTests.cs | Pins revision caveat behavior and absence outside git. |
| producers/tests/OkfProducer.Tests/Generation/DescriptionTests.cs | Adds doc-comment description rules for <code> blocks vs inline <c>. |
| producers/tests/OkfProducer.Tests/Generation/CodeConceptGeneratorTests.cs | Renames misleading test; adds engine-version assertions and namespace/type collision fixtures. |
| producers/tests/OkfProducer.Tests/Generation/CheckTests.cs | Expands golden coverage (15→37 concepts), validates bundle invariants, asserts --check non-mutating. |
| producers/tests/OkfProducer.Tests/Generation/BlastRadiusTests.cs | Corrects misleading comment about stamp blast radius. |
| producers/tests/OkfProducer.Tests/fixtures/README.md | Documents intentional “update golden” workflow failing by design to avoid tautology. |
| producers/tests/OkfProducer.Tests/fixtures/golden/packages/fixture.md | Golden update: adds package containment link to namespace. |
| producers/tests/OkfProducer.Tests/fixtures/golden/overview.md | Golden update: generated.by now includes engine token(s). |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/size.md | New golden concept for record type shape. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/point/y.md | New golden concept for multi-declarator field member shape. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/point/x.md | New golden concept for multi-declarator field member shape. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/point/index.md | New golden index for member subdirectory. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/point.md | New golden type concept + contains links. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/index.md | New golden container index for shapes family. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/i-shape/render.md | New golden member concept for interface member. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/i-shape/index.md | New golden index for interface member subdir. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/i-shape.md | New golden interface type concept + contains link. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder/index.md | New golden index for holder members. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder/count.md | New golden member concept for method under non-generic type. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder.md | New golden non-generic type concept + contains link. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder_2/index.md | New golden index for arity-2 holder members. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder_2/find.md | New golden member concept for generic method shape under arity-2 type. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder_2.md | New golden arity-2 type concept + contains link. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder_1/value.md | New golden member concept for property under arity-1 type. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder_1/index.md | New golden index for arity-1 holder members. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/holder_1.md | New golden arity-1 type concept + contains link. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/hidden/never.md | New golden member concept for public member under internal type container. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/hidden/index.md | New golden index for hidden container members. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/hidden.md | New golden container concept for internal type (out-of-scope type, in-scope member). |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/corner.md | New golden enum type concept (members not emitted). |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/render.md | New golden member concept exercising escaping + unresolved call list. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/index.md | New golden index for boxed members/types. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/changed.md | New golden event-field member concept. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/builder/index.md | New golden index for nested type members. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/builder/build.md | New golden member concept under nested type. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/builder.md | New golden nested type concept + contains link. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed/boxed.md | New golden constructor member concept. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes/boxed.md | New golden multi-line header type concept. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/shapes.md | New golden container concept for block-scoped namespace N.Shapes. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n/index.md | Golden update: adds shapes container/type listings and subdirectory counts. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/n.md | Golden update: adds contains link for shapes. |
| producers/tests/OkfProducer.Tests/fixtures/golden/code/csharp/index.md | Golden update: shapes subdirectory included in counts. |
| producers/tests/OkfProducer.Tests/fixtures/golden/.okfgen-manifest.json | Golden manifest update to include Shapes.cs and new concept ids. |
| producers/tests/OkfProducer.Tests/fixtures/fixture-repo/src/Shapes.cs | Adds fixture source file with many declaration shapes for golden coverage. |
| producers/tests/OkfProducer.Tests/CodeGraph/Utf8OffsetsTests.cs | Adds boundary/invalid-offset coverage (surrogates, inside-sequence rejection). |
| producers/tests/OkfProducer.Tests/CodeGraph/TreeSitterExtractorTests.cs | Adds tests for arity disambiguation, explicit impl naming, and callerless call-sites behavior. |
| producers/tests/OkfProducer.Tests/CodeGraph/ScopeTests.cs | Adds reader seam coverage for unreadable csproj handling (don’t abort run). |
| producers/tests/OkfProducer.Tests/CodeGraph/RoslynResolverTests.cs | Updates tests after removing IsAvailable/IsComplete; adds MSBuild failure-path tests. |
| producers/tests/OkfProducer.Tests/CodeGraph/HostileInputTests.cs | Verifies “size check before read” via reader seam; validates limits invariants. |
| producers/tests/OkfProducer.Tests/CodeGraph/CodeGraphBuilderTests.cs | Ensures temp dirs cleaned; adds guard tests for skip completeness and phantom resolver edges. |
| producers/tests/OkfProducer.Tests/CliTests.cs | Adds CLI coverage for validate, unborn-branch behavior, and permalink base validation rows. |
| producers/src/OkfProducer.Core/Generation/GitRevision.cs | Fixes unborn-branch reporting to avoid invalid permalinks by verifying HEAD. |
| producers/src/OkfProducer.Core/Generation/GenerateOptions.cs | Adds EngineVersions option for overview.generated.by recording. |
| producers/src/OkfProducer.Core/Generation/DocCommentSource.cs | Drops <code> block content from descriptions and adds closer-skipping logic. |
| producers/src/OkfProducer.Core/Generation/ConceptGenerator.cs | Records engine versions in overview, adds revision caveat, disambiguates namespace/type collisions, strips discriminator in titles. |
| producers/src/OkfProducer.Core/Generation/CodeConceptIds.cs | Documents underscore non-boundary rule for slug/id stability. |
| producers/src/OkfProducer.Core/Generation/BundleWriter.cs | Improves prune refusal note clarity; documents/guards directory-ladder safety. |
| producers/src/OkfProducer.Core/Generation/BundleDrift.cs | Clarifies --check blind spot re: dirty working tree vs committed revision. |
| producers/src/OkfProducer.Core/CodeGraph/Utf8Offsets.cs | Fixes/refuses non-boundary offsets and distinguishes “inside sequence” vs “past end”. |
| producers/src/OkfProducer.Core/CodeGraph/SymbolFact.cs | Adds ContainerNamespace to disambiguate namespace/type containment. |
| producers/src/OkfProducer.Core/CodeGraph/RunStatus.cs | Documents Complete helper semantics and clarifies it’s not constructed in production. |
| producers/src/OkfProducer.Core/CodeGraph/ResolvedEdge.cs | Clarifies what Exact means given merge semantics. |
| producers/src/OkfProducer.Core/CodeGraph/IFileSystemReader.cs | Adds filesystem read seam + default SystemFileReader. |
| producers/src/OkfProducer.Core/CodeGraph/FileEligibility.cs | Adds reader seam + broader exception handling for unreadable .csproj. |
| producers/src/OkfProducer.Core/CodeGraph/ExtractionLimits.cs | Enforces positive bounds via init accessors (record with correctness). |
| producers/src/OkfProducer.Core/CodeGraph/EngineVersions.cs | Centralizes name/version token creation from assembly metadata. |
| producers/src/OkfProducer.Core/CodeGraph/CodeGraphBuilder.cs | Adds reader seam; makes site identity strict; drops resolver-returned phantom sites. |
| producers/src/OkfProducer.CodeGraph.TreeSitter/TreeSitterExtractor.cs | Adds reader seam; extracts engine token; fixes name qualification and container consistency. |
| producers/src/OkfProducer.CodeGraph.TreeSitter/Profiles/CSharpProfile.cs | Documents ripple of indexer/operator extraction gap into call edges. |
| producers/src/OkfProducer.CodeGraph.TreeSitter/packages.lock.json | Adds lockfile for deterministic engine versioning. |
| producers/src/OkfProducer.CodeGraph.TreeSitter/OkfProducer.CodeGraph.TreeSitter.csproj | Enables lockfile restore for determinism. |
| producers/src/OkfProducer.CodeGraph.Roslyn/RoslynResolver.cs | Removes unused summary properties; adds engine token; unifies declared naming with tree-sitter. |
| producers/src/OkfProducer.CodeGraph.Roslyn/packages.lock.json | Adds lockfile for deterministic Roslyn package versioning. |
| producers/src/OkfProducer.CodeGraph.Roslyn/OkfProducer.CodeGraph.Roslyn.csproj | Enables lockfile restore; corrects stale comment about removed spike location. |
| producers/src/OkfProducer.CodeGraph.Roslyn/MsBuildProjectQuery.cs | Adds injectable executable/timeout for testable failure paths; improves timeout reporting. |
| producers/src/OkfProducer.Cli/OkfgenCli.cs | Updates --rev description with committed-HEAD caveat. |
| producers/src/OkfProducer.Cli/GenerateRun.cs | Populates EngineVersions only when code stage runs; documents project-query scope. |
| producers/README.md | Documents git invocation count + revision semantics and --check blind spot. |
| docs/superpowers/specs/2026-08-31-okf-producer-code-graph-design.md | Updates design/spec notes with corrected findings and arbitration outcomes. |
| CHANGELOG.md | Adds changelog entries describing the producer code-graph stage and related features. |
Review details
- Files reviewed: 83/83 changed files
- Comments generated: 3
- 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
+881
to
+893
| /// <para><b>What it actually stops: the walk climbing ABOVE the owned prefix.</b> | ||
| /// <see cref="RemoveEmptyDirectories"/> starts at a pruned concept's directory and moves upward one | ||
| /// parent at a time, so this is the sole condition standing between a | ||
| /// <c>Directory.Delete(recursive: true)</c> and the bundle root. It is the most destructive | ||
| /// statement on this branch, and until | ||
| /// <c>PruningTests.The_directory_ladder_stops_at_the_owned_prefix_root_and_not_above_it</c> it was | ||
| /// on no test's critical path -- every other fixture left a sibling that broke the loop one rung in. | ||
| /// | ||
| /// <para>This comment used to describe a different scenario: a sibling directory whose name merely | ||
| /// starts with the prefix (<c>code2/</c> beside <c>code/</c>) 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.</para></para> |
Comment on lines
719
to
723
| @@ -706,6 +722,58 @@ private static bool StartsTag(string comment, int index) | |||
| /// here at all -- it is simply left in the stream, which is what makes an unrecognised paired tag | |||
| /// degrade to its inner text for free. | |||
Comment on lines
+655
to
+657
| // 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); |
B1-3, and it arrived as a direct contradiction between two findings. `FileEligibility.IsAncestorOrSame` compared path segments with a flat `Ordinal`, argued for on the grounds that a case-sensitive filesystem can hold `src/Foo` and `src/foo` as genuinely distinct directories. True, and pinned by `Project_ownership_matching_is_case_sensitive`, written for finding M-1. What that argument missed is where the two sides come from. A file's path is walked off disk; the project's comes from `PackageManifest.RelativePath`, which `RepositoryScanner` may have read out of a `.sln`'s TEXT and never normalised against disk. On Windows a differently-cased entry there still passes `File.Exists`, so the project was found and its `.csproj` read -- and then this comparison rejected it, silently INCLUDING a test project's files with `--include-tests` off. That is the one direction §5.4 cannot afford to get wrong. `BundlePaths.PathComparison` -- the codebase's single answer to "are these the same path" -- satisfies both findings rather than choosing between them, because it encodes exactly the fact they disagree about: where `src/Foo` and `src/foo` can both exist, they are compared as different; where they cannot, as the same. **The existing M-1 test asserted `true` unconditionally**, which is a platform-independent claim about a platform-dependent fact. On Windows those two paths name ONE directory, so the file really is owned by that project and excluding it is correct; the test passed there only because the production comparison was blind in the same way. Put to the user rather than rewritten quietly, since it was written deliberately for a prior finding. It now asserts the rule instead of one operating system's answer to it, and so does the new test for B1-3. Both are pinned by mutation: reverting to `StringComparison.Ordinal` fails the new test. 648 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
INT-M5. Two things this producer does appear in no section of its own design doc, which makes the doc the artefact a reader would act on and be wrong about. **The header cap on a TYPE's `resource` (R48).** A member keeps its full `StartLine..EndLine` span; a type is cut at the end of its header. The reason is §8.3's blast-radius promise: a type declaration's span runs to its closing brace, so any edit inside the body would move `EndLine` and rewrite the type's concept -- churn caused by where an edit landed rather than by what the type declares, which falsifies "adding a private member changes no concept". The finding also said the cap "CONTRADICTS §4.1's own full-span example". Checked: it does not. That example is a METHOD, and members are not capped. Recorded, because a correction that repeats a wrong reason is half a correction. **The neutralisation of lifted text.** A `description` derived from a doc comment is someone else's prose landing in a markdown document, and nothing here said what is done to it: markdown links neutralised, a leading block marker escaped, an UNCLOSED fence defused (a balanced one left alone), a `<code>` block's content dropped -- it is source, not prose, the same argument the viewer's sanitizer makes for `<script>` and `<style>` -- while inline `<c>` is kept because it is part of the sentence, and an unclosed run that merely looks like a tag is left verbatim by the rule that keeps `List<T> of results` intact. 648 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…ate was kept, name the resolved target **D2-M5 -- the one truncate-then-write in a stage-and-move design.** `File.WriteAllBytes` truncates first and fills after, so a process dying between the two left a manifest that was present, short, and not JSON. Everything else this producer writes is staged and moved; the manifest was not. It is now, through a GUID-named staging file rather than a fixed `.tmp` suffix -- `.okfgen-manifest.json` is a predictable name at a known root, which is exactly why the containment gate two lines up exists, and a name chosen at write time cannot be waited for. The damage was bounded rather than absent, which is why this is a Minor: `TryRead` answers null for anything it cannot parse, and null downstream means "own nothing, delete nothing". A crash cost a silent pruning-free run, not a wrong deletion. **D2-M3 -- a candidate with no recorded sources was kept silently.** The branch immediately above it notes its carry; this one did not, so two identical outcomes were distinguishable only by which the operator heard about. Not a hand-edited-only shape either: `ReadStrings` answers `[]` for a truncated sources list rather than rejecting the manifest, so a run interrupted while writing one leaves every candidate looking source-less on the next run -- a bundle that quietly stops pruning, with nothing said. **D1a-10 -- the defensive branch rendered the less informative of two names.** When the id lookup misses, it printed the call site's bare `CalledName` (`Get`) though the edge also carried the resolver's answer, which knows WHICH `Get` it meant. Throwing the better name away in the branch that only runs when something has already gone wrong is exactly where a reader needs it. **D2-M4 verified CLOSED rather than fixed.** Filed as "mid-commit failure leaves mixed state the operator is never told about"; `IBundleWriter`'s doc has a paragraph headed "what the guarantee does not cover is the commit itself", which states the window and how it differs under `Reset`. 649 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…lden's real job **D2-M2 -- `ReportUnownedFiles` was handed the pre-merge manifest.** The manifest this run writes is `merged`: `manifest` minus every id that failed to write, because a recorded id is a standing licence to delete whatever later appears at that path. The report still received `manifest`, so each failed id counted as owned -- and a file sitting at a failed id's path is precisely where a hand-written concept is most likely to be, since the failure is often the filesystem refusing to overwrite it. The one file §6.3 rule 2 exists to report was the one this report stayed silent about. Both now use the same set, for the same reason the manifest does. **F-M4 -- the golden is this solution's only cross-process determinism oracle, and nothing said so.** `DeterminismTests` runs twice in ONE process, so the two runs share the per-process string hash seed: an ordering that leaked from a dictionary compares equal to itself and the test stays green. The bytes in `golden/` were written by another process on another day and do not. Recorded at the top of `fixtures/README.md`, where someone about to delete the golden as redundant would read it. **C1-7 verified CLOSED rather than fixed.** Filed as "duplicate `projectPaths` cost a duplicate ~1 s subprocess"; `QueryProjectClosure` seeds its walk with `new HashSet<string>(pending, PathComparer)`, so a repeated path is queried once. 649 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
… one per call site C1-9. `BuildIndex` called `Utf8Offsets.ToUtf8` once per callee, and that method counts bytes from the start of the text every time -- so indexing a file cost O(file size x call sites), and a large file with many calls paid for its own length once for each of them. It accumulates now: the callees are collected, sorted by position, and the walk moves forward once. O(file size). **The trade is worth naming, because this is the most dangerous value in the codebase to get subtly wrong.** The offset is the join key between two engines, so a drift of one byte does not lose a call, it credits it to whatever sits nearby -- silently. Two things guard it. The boundary check `Utf8Offsets.ToUtf8` performs is kept rather than lost to the optimisation: an offset splitting a surrogate pair would make the segment start on a lone low surrogate and score 3 bytes for it through the replacement fallback, which is exactly the plausible-wrong-number failure this discipline exists to prevent. It is O(1) here, as it is there. And the new test is a real equivalence rather than a tautology, because the two sides are computed differently: the extractor converts each site with the per-site method, the resolver now walks the file once, and they must agree. The fixture is the NON-ASCII file on purpose -- on ASCII the two encodings coincide and any drift would be invisible. Verified by mutation: replacing the byte count with a code-unit count fails it. 650 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
Four Minor findings, each the same shape: a name or a comment claiming a property the body could not see. Every one measured, not reasoned about. **D3-5 -- the manifest's ordering test measured SORTEDNESS and called it Ordinal.** It handed in `a`/`b` and `src/A.cs`/`src/Z.cs`, pairs that sort identically under every comparer there is, so swapping every `StringComparer.Ordinal` in `GenerationManifest.Normalized` for `CurrentCulture` left it green -- though §6.2 pins Ordinal, and pins it because `--check` compares this file byte for byte: a linguistic sort makes the bytes depend on the machine's locale, so a bundle generated under one culture reports drift under another with nothing changed in the source. The pairs are now `m0`/`m_1` (punctuation vs digit) and `src/Z.cs`/`src/a.cs` (case), both measured to invert between Ordinal and `InvariantCulture` as well as `CurrentCulture`, so the test discriminates wherever it runs rather than only under the author's locale. Each is handed in the linguistic order, so the mutation cannot pass by leaving the input untouched. Verified: mutating the id sort fails the id assertion, the path sort the path one, independently. **F-M6 -- the unresolved-call fixture modelled a callee the extractor cannot produce.** It used `string.Substring`. An unresolved entry renders `edge.Site.CalledName`, the identifier as written at the call, and `CSharpProfile.CallQuery` captures the name node alone -- the golden proves it, where `int.Parse(raw)` comes out as `` `Parse` ``. So both assertions guarded the rendering of a string that could never arrive. The fixture is a bare identifier now. The same error had reached `fixtures/README.md`, whose table said the unresolved row was `int.Parse`; corrected against the golden. **F-M5 -- "copied through" was asserted only as two absences.** A producer that DELETED the author's backslash -- shipping `Keep [a]` -- satisfied both: `[a]` is not a link, so the scanner still finds none, and with the backslash gone there is no `\\]` either. Now asserted as exact bytes. MEASURED both ways: with the guard rewritten to swallow the backslash the new assertion goes red, and with it removed the old two pass green over the same mutated output. **F-M2, F-M3 -- two names promising verdicts their bodies never obtain.** A signature-line test named for forward slashes, over a fixture path already spelled with them; and a resource test named for earning no path warning, which runs no validator. Both renamed, and both say where the property really lives rather than dropping the fact: separator normalization happens once, in `CodeGraphBuilder.EnumerateFiles`, and deleting that `Replace` turns six tests red across `HostileInputTests` and `CliTests` (measured); the validator verdict is `CheckTests.The_golden_bundle_validates_with_no_error_and_only_the_warnings_we_know_about`. **F cross-slice, last instance.** `ProducerFixture`'s doc comment still said the CLI does not compose the code-graph stage and that Task 13 owned its flags. It does compose it. The divergence is real but is now a deliberate one rather than a gap waiting to close: this fixture stops at tree-sitter and `NameMatchResolver` because composing Roslyn would put a `dotnet msbuild` evaluation inside every golden comparison. Verified CLOSED rather than fixed, by reading the enclosing code rather than grepping for the symptom: F-M1 (the spike pointer carries its `git checkout` recovery hint), D3-6 (both exit codes asserted), D3-9 (`Check_leaves_the_bundle_it_was_given_byte_for_byte_unchanged`), D3-10, D3-8, B2-3 (the recording reader turned the claim into an assertion), B2-4 (`ScopeTests` is `IDisposable` and cleans its temp dirs), B2-6. 650 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
… the grammar contract **INT-M3 -- §2.3 asks for a single snapshot read; this pipeline has two.** The extractor reads a file in its own pass and records offsets against the bytes it saw then; `CompilationFactory.TryParse` reads the same file again, with the whole tree-sitter pass in between. Everything else on that method's doc argues that the two engines' offsets are comparable -- identical decoder, identical BOM handling, the encoding-fallback case reasoned through -- so the one condition identical decoding does NOT buy belonged there too. A file edited inside that window decodes identically in both engines and still yields offsets that do not line up, and this is the join key: the result is not a lost call, it is a call credited to whatever now sits at that offset. Recorded as a known, accepted window with its accounting, not as a fix. Nothing detects it -- §2.3 also asks for a content hash in the manifest, and `GenerationManifest` already records that no hash is computed anywhere in this pipeline -- and closing it is not local: the extractor decodes the text and drops it, so a real snapshot means carrying the decoded text through to the compilation, which changes the contract between the two engines rather than this method. The exposure is bounded by what runs the producer: a one-shot CLI over a checkout, where the window is one repository scan long. **B2-5 -- measured, and the concern no longer holds.** `HeaderEndNode` matches three grammar field names (`body`, `accessors`, `value`); the finding was that a grammar bump renaming one would silently fall back to the declaration's own last line -- the pre-R48 churn defect -- with nothing to notice. Renamed one at a time against the whole suite: `body` turns 12 tests red (the golden included, through `CheckTests`, plus two `BlastRadiusTests` rows), `accessors` turns 8 red, `value` turns 1. The contract is guarded by execution, and the golden's enrichment to one occurrence of each declaration shape is what carries most of it. Written down at the lookup so the next reader does not have to re-measure it. 650 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…nd language INT-M4. The layout table said `OkfProducer.Core` holds "the language-agnostic code-graph contracts", and that word was doing work the code does not do. Four places couple Core to C#, and each is one a second language has to touch: - `LanguageProfile.SplitContainer` picks its separator from the language name. Deliberately not a profile field: `ConceptGenerator.ProfileFor` synthesizes a throwaway profile from a bare language name for a symbol whose language matches none the caller supplied, and that fallback yields the right ids only while this method is a pure function of `Language` (ruling R29 pinned that at both ends). - `LanguageProfile.VisibilityOf` applies C#'s access rules, per declaration kind. - `FileEligibility` reads `.csproj` XML for ownership and test-project detection. - And the one that is correctness rather than organization, found while checking the other three: `CallSite` carries NO language, so both of `ConceptGenerator`'s joins are language-blind. With two profiles the same `(container, name)` in each attributes one call to both concepts. That one does not wait to be discovered -- `ConceptGenerator` throws on a multi-language graph, and the throw names the fix. Written as a section rather than fixed. A second language is not on the roadmap, and generalizing for one hypothetical consumer is the worse trade; the cost of leaving it silent is someone discovering it halfway through, having budgeted for "add a profile". Each claim was read against the code before writing it, including the fourth, which this file did not know about until then. 650 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
E2-3. Both `--help` tests in `CliTests` asserted the text and nothing else, in a file where every other test says what its run exited with. `--help` is a successful invocation, not a refused one, so a regression that printed the whole help and then exited non-zero -- breaking a shell script's `okfgen --help || exit 1` for nobody's benefit -- had nothing to fail. Closes the last Minor finding in the register that needed a code change. The rest of the batch was sifted and verified CLOSED by reading the enclosing code rather than grepping for the symptom, which is what the earlier rounds kept getting wrong: - B1-3 (`FileEligibility` compares with `BundlePaths.PathComparison` now, argued at the site), B1-4 (the verdict dictionary seeds with `Add`, so a duplicate offset throws instead of dropping a call), B1-5, B1-6, B1-8, B1-9 (`RunStatus.Complete` documents that production never constructs one, and why it is kept anyway). - C1-5 (the tautological `Assert.Empty(Resolve([], …))` was removed, and the removal is recorded where it stood), C1-6 (`EdgeConfidence.Exact`'s doc now states its own limit: it says the resolver knew which declaration it meant, not that the bundle can express which one), C2-2 (malformed-JSON is a theory, `Win32Exception` has a test), E2-4 (`--force` is exercised). - D2-M5 (a failed move produces a failure or a note, never silence), D2-M6 (the leftover staging directory is a documented, harmless outcome of the `finally`), D1a-7 (`ProducerActor`'s summary says which families carry `generated`). - INT-M1 (the spec was the stale artefact and was corrected on 2026-09-03), INT-M2 (the `--max-file-size` help carries the Roslyn qualification and a test pins it), INT-M6 (querying test projects with `--include-tests` off is argued and its cost measured). 650 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…at had none
C1-8, ruled opt-in. The extraction stage has always honoured
`ExtractionLimits.Timeout`; the Roslyn stage never had a bound of its own. Each
`dotnet msbuild` query is capped at two minutes by `MsBuildProjectQuery`, but nothing
caps their sum and nothing caps the compilations after them, so a large enough
repository runs for as long as it runs. Measured here: 9 s for 8 projects.
**Absent by default, and absent means unbounded.** A default budget would fix the
hazard by making the emitted bundle a function of how fast the machine is, and §6.2
pins determinism at a fixed extractor version, not at a fixed CPU. So an unbudgeted
run goes through `Create`, the overload that cannot return null, running exactly the
code it always ran and never consulting a clock.
**When it trips, the stage is abandoned WHOLE.** Publishing the projects that
happened to finish first would emit a bundle whose exact and name-matched `## Calls`
links are divided by machine speed, with nothing recording where the line fell --
§2.1 rates that below having no exact resolver at all, because its reader cannot tell
which half they are holding. Abandoning whole lands the run in exactly the
`--no-msbuild` state, and the note says so in the same two clauses, in the same order,
rather than inventing a third vocabulary for a state the operator already has a name
for. The deadline is checked at the top of each per-project iteration -- the finest
granularity that means anything, since one query is already capped -- and never inside
`Compile`'s recursion, where abandoning halfway down a chain would leave a project
compiled against a reference that was itself abandoned.
**A defect found while wiring it, and fixed here because this change would otherwise
ship it one case wider.** `generated.by` is a determinism claim, and Roslyn was named
on every run that was not `--no-code` -- including three where it demonstrably never
ran: `--no-msbuild`, a repository with no project file, and now an exhausted budget.
`ProducerFixture` had the rule right for its own capture ("tree-sitter alone, because
tree-sitter alone ran") while the shipped CLI did not, which is exactly why the golden
could not catch it. Named per engine now, keyed on the same resolver-is-non-null
condition as everything else on that path.
Seven tests, both new guarantees verified by mutation: naming Roslyn unconditionally
again fails the `generated.by` test, and returning what finished instead of null fails
the whole-abandonment test. The budget test is deterministic despite naming a
duration -- the first `dotnet msbuild` query is a subprocess and cannot return in
under a millisecond, so the compile loop always finds the budget spent.
657 tests green, format clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
D1a-9, ruled extract. `ConceptGenerator.cs` was 2492 lines, and roughly 560 of them were a different program: pure string-to-string transforms applied to text this producer did not author -- neutralizing link syntax, escaping a leading block marker, defusing an unclosed fence, building a code span or a link label the text inside cannot break out of. None of it knows about a `CodeGraph`, a `Bundle` or a `GenerateOptions`, and none of it needs to. **The seam is drawn where the correctness argument changes.** Everything moved is decided against ONE external contract -- `LinkScanner`'s `CodeFreeLines`, `BlankInlineCode` and `ParseInlineLink`, in `OKF4net`, which is what will read the markdown this producer emits. That is why the escaping here deliberately mirrors the consumer's rules rather than CommonMark's wherever the two differ. And it is why the split is worth making: the defects recorded in this neighbourhood are, repeatedly, one shape -- this code and `Links.cs` disagree about one character class (a backslash before a backtick, a bracket inside a code span, whitespace before a fence). Among two thousand lines of concept assembly the two rule sets could not be read side by side; in a file of their own, against a named contract, they can. Imported with `using static`, so every call site in `ConceptGenerator` reads exactly as it did: the seam is for whoever has to compare those rules, not a new spelling to learn at each call. Six entry points are `internal`, the ten helpers stay `private` -- the layer has a surface, not just a location. The class is `internal` too: it is `ConceptGenerator`'s text layer, not an API this producer offers anyone. **No behaviour changed, and the golden is the evidence rather than the claim.** `fixtures/golden/` is byte-identical across the move -- `CheckTests` regenerates it and compares raw bytes -- and 657 tests pass. 2492 lines become 1937 + 594. Also here, D4b-4 ruled the other way and written down where it will be re-raised. Four test files carry their own `TempDir` beside `ProducerFixture`'s. Each has a `Write` the shared one lacks, and the versions are not interchangeable: a `Write` that creates missing parent directories and one that does not make a test over an absent directory assert two different things, silently. Unifying means settling that semantics for four suites at once, on the helper whose job is to set up the conditions those suites measure. The duplication is visible and inert; the merge is the part with a way to be wrong. Recorded on the shared `TempDir`, where someone about to unify them would look. 657 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…able D1b-M1, the last Minor genuinely open. `ConceptIdRegistry` documents that the §3.3 tie-break is "stable across a file move or a line shift, not dependent on scan order", and that sentence reads wider than it is. The suffix is POSITIONAL: among colliding candidates the Ordinal-first keeps the bare slug and the rest take `-2`, `-3`. So a candidate arriving AHEAD of the others takes the bare slug and pushes each of them one place down, though their own declarations did not change -- which is the id churn §3.1 treats as unrecoverable, one level up from the overload merge that exists to prevent exactly this. A file move does not do it; a new sibling does. Asserted rather than described, because a limit nobody can execute is a limit nobody believes: registering `PARSE` ahead of an existing `Parse`/`parse` pair moves `Parse` to `-2` and `parse` to `-3`. Measured, not reasoned -- `P` ties, `A` (0x41) precedes `a` (0x61), and `parse`'s leading `p` (0x70) sorts after both, so that is the sequence a later run really would produce. **Left as it stands, with the accounting written down.** Closing it means suffixing by something intrinsic to the symbol instead of by position, which moves every already-suffixed id once -- churn now, to prevent churn later. Measured at ZERO occurrences on this repository and on the fixture: C#'s residual collisions are case-only pairs and a nested type sharing a member's name, and neither occurs here. §3.3 says the rule exists for Go and JS, where it is common, and no profile for either exists yet -- nor can one land without the wider work `producers/README.md` prices under "Adding a second language". That is when to pay for this, against a real corpus. Also closing the sift the previous commits did not reach: D4b-2, D4b-3, D1b-M2, M3 and M4 were all verified CLOSED by reading the code rather than the register -- the unborn branch is caught by a second `rev-parse --verify` and has its own test, the `TryPermalinkBase` table carries `javascript:` and scheme-relative rows, CDATA has its own token shape, a `<code>` block no longer flows into `CollapseWhitespaceRuns`, and the `MAX_VALUE` word-boundary rule is documented at the site. That empties the register: every Minor is now fixed, verified closed, or recorded as accepted with its reason. 658 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
jchable
changed the title
…ead as exhaustive Propagation, checked against the code rather than against my memory of what was added. **Two shipped changes were announced nowhere.** `--roslyn-timeout` is a new user-facing option and `generated.by` no longer names Roslyn on runs where Roslyn never ran -- a change to bytes already in people's bundles. Neither was in the CHANGELOG. Added under `Added` and `Fixed` respectively, each with the reason rather than the summary: the budget is opt-in because it makes the bundle a function of machine speed, and the `generated.by` fix matters because that field is a determinism claim, false in the direction that promises reproducibility against a tool that was not there. **The design's CLI table was missing TWO flags, not one.** §9 presents itself as the CLI surface of this batch, and `--no-msbuild` had never joined it -- it is described at length in §7.2 as a lever added mid-batch, and that description was where it stopped. An incomplete summary table is worse than an absent one, because it reads as exhaustive; someone auditing the flag surface against it would have concluded twice over that a flag did not exist. Both rows added, and the omission named in the table itself so the next person knows the list was once wrong. Verified by enumerating the real options out of `OkfgenCli.Run` and diffing against the table, and by grepping for every place the flag surface is listed: three (`producers/README.md`, this design, the CHANGELOG), all three now carry it. The web site does not enumerate `okfgen` flags, so nothing there to correct. 658 producer tests green, 1272 library tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…h's own work A 92-agent, 10-lens review of the branch, each finding put to three refuters. These four are the ones that reach the emitted bundle or crash the CLI. Two were introduced by this branch; two are conformance failures it shipped. **BLOCKER -- `generated.by` was not an actor.** §5.2 makes it one and §7 defines an actor as exactly `<producer>/<version>`, `human:<id>` or `process:<id>`. This wrote `okfgen/0.1.0 tree-sitter/1.3.0 roslyn/5.3.0`, which is none of them, and the test asserting it called that shape "the spec's own" -- the reverse of the truth. The failure was silent in the worst way: `OKF4net.Actor.Parse` splits on the FIRST slash, so it reported IsWellFormed:true with Version = "0.1.0 tree-sitter/1.3.0 roslyn/5.3.0", and `BundleValidator` warns only on a malformed actor -- `okf validate` called the bundle clean while every consumer reading the version off this producer's own artefact got a string naming no release of anything. The golden shipped it. `by` is the producer alone now; the engines move to a sibling `generated.engines`, which keeps §6.2's provenance because OKF preserves producer keys it does not know across a round-trip. Asserted twice over: once on the fields, and once by parsing the emitted value through `Actor.Parse` itself, since a string comparison cannot see what a consumer's reader does. **BLOCKER -- the namespace disambiguation marker leaked into emitted ids.** `RegisterContainerId`'s fallback passed RAW segments to `CodeConceptIds`, so a namespace marked by `DisambiguateNamespacesAgainstTypes` was slugified WITH its marker. Reproduced end to end on two files -- a global-namespace `class Foo` beside a `namespace Foo` -- and the bundle carried `code/csharp/foo-ns.md` under `title: Foo`, `"code/csharp/foo-ns"` in the manifest, and the marker on every id in that subtree, permanently (§3.1: unrecoverable). Not a corner case: for a TOP-LEVEL namespace that fallback is always the path taken, because the parent key `[code, <language>]` is never registered. The suite's only namespace-vs-type fixture was the nested one, where the clean branch runs -- which is exactly why 658 tests were green over it. `SegmentName`'s own summary already claimed every path back to something a reader sees went through it. It does now. **MAJOR -- the arity discriminator was not injective.** `_N` put the separator in the C# identifier alphabet, so a type genuinely named `Holder_1` and one named `Holder<T>` produced the same `SymbolFact.Name`, grouped as one concept, and were emitted with both signatures under one description -- the exact merge the arity rule was added to remove, reachable from legal C#. The marker is a backtick now: it cannot occur in a C# identifier under any spelling, it is the CLR's own arity convention, and it lives in `SymbolFact` in Core because it is a contract BETWEEN the two engines, the same reason `Utf8Offsets` does. Ids move from `holder_1` to `holder-1`, ruled and accepted. **MAJOR -- `--roslyn-timeout` was culture-sensitive and crashed on two value bands.** Without a custom parser System.CommandLine converts a double with the CURRENT culture and `NumberStyles.AllowThousands`: under de-DE `1.5` parsed as 15 and `0.001` as 1 -- exit 0, no diagnostic, a budget 10^n too large from the exact form the README documents -- while fr-FR refused the same `1.5` outright. A multiplied budget changes which projects finish, so it changes the emitted call links, the containment links and the engine list: the machine's locale decided bundle content, which is what §6.2 pins determinism against. Parsed invariantly now, `NumberStyles.Float`, no thousands. And my range guard tested the double, not the `TimeSpan` that reaches the API -- two different ranges. Above ~9.22e11 seconds `TimeSpan.FromSeconds` threw OverflowException and under one tick the value truncated to `TimeSpan.Zero`, which `TryCreateWithin` refuses: both escaped as unhandled exceptions with a stack trace, two lines below a guard that looked like argument validation. Both bands now return the error, verified on the real binary. Golden regenerated for both blocker fixes and the diff read: `generated` gains `engines`, `holder_1`/`holder_2` become `holder-1`/`holder-2`, and the arity concepts now sort ahead of `holder/count` because Ordinal puts `-` (0x2D) before `/` (0x2F). 666 tests green, format clean. Every fix verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
… edits displaced **MAJOR -- the timeout test could not time out anywhere but Windows, and there by a race.** It substituted `sleep` for `dotnet` off Windows, but `Run` always appends MSBuild's own argument list, so what launched was `sleep msbuild <proj> -nodeReuse:false ...`: GNU sleep rejects "msbuild" as a duration and exits 1 in milliseconds, so control took the EXIT-CODE branch and the assertion was red on Linux and macOS -- two thirds of the only guarantee `producers/` has, being outside CI. Substituting the executable was the wrong idea rather than the wrong stand-in: none can both ignore MSBuild's arguments and refuse to exit. It runs the real `dotnet msbuild` under a budget no real evaluation can meet, which is deterministic everywhere and exercises the production path. **MAJOR -- `CSharpProfile` said a test "pins both halves"; the test made one.** Everything it asserted came from the extractor alone, so deleting `CodeGraphBuilder`'s invariant -- no edge may name a caller absent from Symbols -- left the orphan edge in the graph with the test still green, under a name that promised "yields no edge at all". It builds the graph now. Verified by mutation: removing the invariant turns it red. **MAJOR -- `generated.by` still named Roslyn when Roslyn compiled nothing.** The per-engine fix keyed on `roslyn is not null`, but `Create` returns a resolver whatever happened -- every project can be MsBuildQueryFailed and it still constructs one. That is the COMMON degradation (an unrestored checkout, no `dotnet` on PATH), so the most likely case still wrote `roslyn/5.3.0` into a bundle Roslyn contributed no byte to. This producer's own fixture repository is in exactly that state, which is how the golden and the shipped CLI came to disagree about one repository. The condition is now "at least one project compiled". **Six doc comments documented the wrong member.** My `sed` insertions repeatedly landed between an existing `</summary>` and the member it described, so the summary moved onto the member I inserted and its own member lost its docs. No compiler warning fires: no producer project sets `GenerateDocumentationFile`. Five were moved back to their members (`QueryProjectClosure`, `SimpleNameOf`, `ComputeContainerPath`, `Substitution`, `DisambiguateSharedRawPaths` -- which is why that one read as undocumented earlier) and one was a duplicate summary on a single member, merged. A grep for the pattern now returns nothing. **`ReadFully` doubled the peak memory `--max-file-size` exists to bound**, under a comment claiming it "allocates once": a `MemoryStream` plus `ToArray` always allocates twice, so §2.3's guard on what one hostile file can make this process allocate was bounding half of it. Worse on the growth path, where a stream copies the whole file before it can fail and `File.ReadAllBytes` refused up front on the declared length. One pre-sized array, filled with `ReadExactly` so a short read is an error rather than a silent truncation -- truncation yields spans pointing at the wrong code. **`--roslyn-timeout 0` was accepted and meant unbounded.** The guard rejected only `< 0`, with a message saying the value must be positive, then mapped 0 to "no budget": an operator, or a wrapper whose remaining budget had counted down to zero, asked for the smallest bound and got the largest. The option is `double?` now, so absent and zero are distinguishable, and zero is refused like any other non-positive budget. **Documentation corrected against the code rather than against memory.** `GenerateRun` still said "two to four" git invocations and that the README "states the same breakdown" -- the unborn-branch fix made it five and corrected only the README, so both halves were false. `fixtures/README.md` still said the golden held fifteen concepts and its shape table had no row for anything `Shapes.cs` added, in the document a reviewer is sent to before accepting a golden diff. And `Shapes.cs` implied it had closed the explicit-interface-implementation and local-function gaps: both are Private, so scope filters them and neither reaches a concept -- they exercise the extractor only. Golden regenerated for the two-line fixture edit; the diff is pure line shift (+2 on every span in `Shapes.cs`), read before accepting. 667 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…d left half-done **The budget was never checked after the last unit of work.** Both loops consult the clock at the top of an iteration and the final decision read the LATCH, so an overrun during the last unit -- or the only one, on a single-project repository -- was never noticed: `TryCreateWithin` returned a resolver for a stage that took longer than its budget, contradicting its own doc and the help text's "abandoned WHOLE". It asks the clock once more after all the work, which is the only point where the total is known. **A test asserted "no budget is the default" entirely out of help text.** Reading a sentence that says "absent means unbounded" cannot show that absent means unbounded. Renamed to what it does check, and the behavioural half added: a default run must never carry the abandonment note, asserted against the note's own text rather than a timing measurement -- which would make the test a function of the machine exactly as the option makes the bundle one. **A comment cited a measurement that demonstrates the opposite of its point.** `Utf8Offsets` justified `==` over `>=` with `"A\U0001D11E"` and an offset of 2 "coming back as the index after it". Rounding up needs a rune AFTER the multi-byte sequence to round up to; on that two-rune string the old code threw instead. The example carries a trailing `B` now, and both readings were measured before rewriting the comment. **`EngineVersions` promised a version a reader can look up.** Measured on the pinned `Microsoft.CodeAnalysis.CSharp`: the package is `5.3.0` in the `.csproj` and the lock file, while the informational version is `5.3.0-2.26078.5+16f9bd2`, so the token is `roslyn/5.3.0-2.26078.5` -- a Roslyn build id matching no package. That is still the right value to record, because §6.2 needs the BUILD; what was wrong was the promise. For tree-sitter the two coincide, which is how the claim went unchallenged. **The lock files are rewritten by any RID-targeted restore.** Measured: `dotnet restore producers/src/OkfProducer.Cli -r win-x64` adds a `net10.0/win-x64` section to both new lock files, leaving a dirty tree. Committing all three RIDs is not available -- successive RID restores REPLACE each other's section rather than accumulating, also measured -- so the plain restore stays committed and the trap is written down where someone would hit it. **A rename I made two commits ago was left half-propagated.** Moving the engines out of `generated.by` into `generated.engines` left six places still naming the old field -- two engine `EngineVersion` properties, `EngineVersions`' own summary, `GenerateOptions`, a `.csproj` comment and `fixtures/README.md`. Found by grepping the bare term rather than by recalling where I had touched it, which is the only way this kind of thing gets found. 668 tests green, format clean, lock files unchanged by a plain restore. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…ublic member stays out The last of the review's findings, ruled to fix rather than document. C# caps a member at its container: `public void Never()` inside `internal class Hidden` is not reachable outside the assembly, whatever the keyword says. `FileEligibility` filtered on the DECLARED modifier alone, so that member landed in a default-scope bundle -- tagged `public`, a visibility the language does not give it -- and an operator who left `--include-internal` off precisely to keep internal API out of a shipped knowledge bundle got it published anyway, mislabelled. This producer's own fixture carries the shape, so the golden blessed it. The cap needs the whole declared set: a `SymbolFact` names its container as a dotted path and nothing more, so the container's visibility is not on it. The new overload takes the UNFILTERED symbols and walks the container chain upward taking the least visible tier -- filtering first would remove the very containers that do the capping, and a member whose container had been dropped would then look top-level and slip through. It fails OPEN, deliberately: a container absent from the declared set caps nothing. That container is a namespace, or a type in a file this run could not read, and treating an unknown container as private would delete concepts over a file that merely failed to open -- which §2.3 rates well below keeping them. That path is reached on every ordinary member, whose chain ends at a namespace, so it is the common case rather than a corner. `Least` is an explicit ladder rather than a comparison on the enum's numeric order: the members happen to be declared most-visible-first today, so `left > right` would work and would silently invert if anyone reordered them. Golden regenerated and the diff read: `code/csharp/n/shapes/hidden`, `hidden/index` and `hidden/never` are gone -- the member, and the container its presence had forced into existence. 35 concepts, and one fewer `resource` warning, since that container was one of the four that cannot carry a permalink. Five tests: the four visibility combinations (capped, both public, cap only ever reduces, private wins over the flag) and the fail-open case. 673 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
Three of the fixes change bytes in a regenerated bundle, so they belong where a user looks before regenerating rather than only in the commits that made them: `generated.by` becoming an actor again with the engines beside it, scope filtering on effective visibility (which REMOVES concepts), and the generic-arity ids moving from `holder_1` to `holder-1`. The `--roslyn-timeout` entry now names all three defects it had -- culture-sensitive parsing, two crashing value bands, and zero meaning unbounded -- rather than only the one the option was introduced with. Not announcing an output change is the same defect class this whole round was fixing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
…ontainer The effective-visibility fix removes concepts an earlier version of this producer emitted, and regenerating over such a bundle PRUNES them. Measured on a two-level fixture: five concepts deleted, nothing printed. **Nothing in the pipeline could have said so, and that is the point.** `BundleWriter` already refuses to prune when the previous run covered a wider scope -- and it works; dropping `--include-internal` between runs keeps the concepts and explains why. But it decides that by comparing the scope FLAGS recorded in the manifest, and here the flags are identical on both sides. It is the RULE that narrowed, not the run, and the manifest records nothing about the rule. Verified by simulating exactly that: a manifest written at default scope, regenerated at default scope by the new binary, silently lost five concepts. So the count comes from where the filtering happens. `CodeGraph.CappedByContainer` is the difference between the two filters -- a declaration whose own modifier is in scope and whose enclosing type is not -- and `GenerateRun` turns it into a note naming the number, why they are gone, that the deletion is this change rather than anything removed from the repository, and which flag brings them back. A note rather than a refusal to prune: the concepts really are out of scope now, and keeping them would republish exactly the internal API the flag was left off to exclude. What the operator needs is to know it happened. `CappedByContainer` is an `init` property on `CodeGraph` rather than a field on `RunStatus`: `RunStatus` is about FILES -- was each visited, did it parse -- while this is about declarations inside files that were read perfectly well. Defaulting to zero keeps every positional fixture in this solution compiling and meaning what it meant. Two tests, both verified by mutation: the note fires with the right count (only what the CAP removed -- an internal type is out by its own modifier and must not inflate it), and a run that capped nothing stays silent, so this cannot become a line every run prints and nobody reads. The general fix is recorded in ROADMAP: give the manifest a scope-RULE identifier beside `scope`, so the existing guard fires on a rule change as it does on a flag change. Not done here -- it changes the manifest format, which is a compatibility decision of its own, and not one to slip into a fix round. 675 tests green, format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166CcRSdoZrL8g1Suruy6Uz
jchable
changed the title
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.
Two rounds. First, the parked findings register from the code-graph review worked to
empty. Then an adversarial multi-agent review of that work, whose findings are
also fixed here.
All of it is
producers/, which is outside CI by decision — so the guarantee is onelocal command, and it was run at every step:
Merged with
dev(including theOKF4net.Rendersplit) and re-run against it, sinceOkfProducer.Corereferencessrc/OKF4netand nothing automatic would say otherwise.Read this first if you regenerate a bundle
Three changes alter the bytes of a regenerated bundle. All three are in the CHANGELOG.
generated.byis an actor again. §5.2 makes it one and §7 admits exactly<producer>/<version>,human:<id>,process:<id>. It was written asokfgen/0.1.0 tree-sitter/1.3.0 roslyn/5.3.0— none of them. The failure was silent:Actor.Parsesplits on the first/, so it reported the value well-formed with aversion of
0.1.0 tree-sitter/1.3.0 roslyn/5.3.0, andokf validatecalled such abundle clean while every consumer read a version naming no release. The engines moved
to a sibling
generated.engines, which OKF preserves across a round-trip.at its container: a
publicmember of aninternaltype is not reachable outside theassembly. It used to be emitted with
--include-internaloff and taggedpublic, soa bundle generated to exclude internal API published it anyway. Regenerating deletes
those concepts — and the run now says so by name and count, because
BundleWriter'sscope-narrowing guard compares the recorded scope flags, which are identical on both
sides here. It is the rule that narrowed, not the run.
holder_1toholder-1. The arity discriminator_Nwas drawn from the C# identifier alphabet, so a type genuinely named
Holder_1andone named
Holder<T>produced the same symbol name and collapsed into one conceptlisting both signatures — the exact merge the arity rule exists to remove, reachable
from legal C#. A backtick cannot occur in a C# identifier, and is the CLR's own
convention.
The review round
A 10-lens adversarial review over the branch diff, each finding put to three refuters
with distinct angles (read the code / build the repro / check the suite), majority to
kill. 28 raw findings, 27 unique, 24 confirmed and 3 refuted, plus 2 from a
completeness critic asked only what the lenses could not see.
The critic found the most serious one. None of the ten lenses compared the emitted
bundle against the vendored spec or against OKF4net's own reader — that boundary was the
gap, and the §7 violation above lived in it.
Four defects were in code this branch had already shipped green:
--roslyn-timeoutparsed with the machine's culture (1.5meant 15 on acomma-decimal locale, silently) and crashed with an unhandled exception on two value
bands its range guard did not cover — the guard tested the
double, not theTimeSpan.substituting
sleepfordotnetleft MSBuild's argument list appended, so theexit-code branch ran instead of the deadline branch.
CSharpProfileclaimed a test "pins both halves"; the test never built a graph.sedinsertions that landed between anexisting
</summary>and the member it described. No compiler warning fires, since noproducer project sets
GenerateDocumentationFile.Method, since it is what the diff is mostly made of
Every fix is verified by mutation: break the production code, watch the test go red.
That caught several of my own fixtures passing for a reason other than the one they
named, which is why some commits here remove or rewrite an assertion rather than add
one.
The same skepticism applied to the register itself. Roughly a third of its findings were
already closed, and four of its stated claims measured false — the register's
reasoning is as unverified as any other prose. Several entries closed not by changing
code but by correcting a name or comment that promised a verdict its body never obtained.
Accepted, with the reason written where it will be re-raised
UTF-8 offset that is the join key between the two engines. A file edited between them
yields a call credited to whatever now sits at that offset. Closing it means carrying
the decoded text between engines: a contract change, not a local fix.
correctness (
CallSitecarries no language, soConceptGeneratorthrows on amulti-language graph).
declarations. Now asserted executably. Zero occurrences measured here.
with the measurement; the general fix changes the manifest format, which is a
compatibility decision of its own.
window opens and closes inside one call, so no in-process test can stand in it; the
test's name and comment now say exactly that instead of implying coverage.
TempDirclasses, deliberately not unified: theirWritehelpersdiffer on whether parent directories are created, and merging would silently change
what four suites assert.