Repository navigation
test(arch): adapter tiers, dated privilege exemptions declared before their surfaces exist (#843) - #880
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review round 1 (Codex gpt-5.6-sol, reviewer of record), head d61836a: changes requested, 3 findings, all accepted.
Fix commits and the mutation evidence follow on this PR; round 2 goes back to the same reviewer. |
Declares the MCP tier (Cluckwork.Api.Mcp -> DirectRepository, reviewBy #806) dormant, before #806's /mcp surface exists. AdapterTierScanner walks tool-attributed types and MapMcp call sites, reporting a tool type outside every declared tier namespace, a mapped surface with no tier row, and a dormant row (green, informational) when neither exists yet.
A tier is an adapter root with a privilege: adapterTiers[].namespace now feeds AdapterReachScanner's namespace roots (so a tool type's repository parameters are walked for module reach) and its persistence-forbidden set (DirectRepository is not direct DbContext).
…tence-forbidden roots (#843)
…osed set (#843) codex-sol review of #880, finding 1. AdapterTierScanner.SurfaceCalls was a private set the ledger never checked a row's `surface` against, so an unknown surface (e.g. a typo'd MapMcpTypo) passed registry validation while granting the DirectRepository exemption over a namespace the scanner never walks for that surface. AdapterTier.KnownSurfaces is now the one shared map from surface to the privilege it grants; both the scanner's invocation walk and ModuleLedger's row validation read it, so a surface outside the set is a registry error naming the surface and the closed set.
…843) codex-sol review of #880, finding 2. AdapterTierScanner.HasToolAttribute matched only an attribute's bare last identifier, so `using ToolMarker = ModelContextProtocol.Server.McpServerToolTypeAttribute;` plus `[ToolMarker]` went undetected outside Cluckwork.Api.Mcp — confirmed against the real tree with a locally-declared attribute and alias, all three AdapterTierRealTreeTests stayed green. The scanner now resolves file-local and namespace-block using aliases in the declaring file, plus `global using` aliases declared anywhere in the same project (grouped by ModuleLedgerScanner.ProjectRootNamespace), before comparing the attribute's identifier against the known tool-attribute names. Resolution is syntactic, matching the existing bare/qualified/Attribute-suffix handling. Verified with a real-tree mutation: a class outside every tier namespace carrying a locally-declared McpServerToolTypeAttribute, referenced only through a local alias, went RED (ToolTypeOutsideTier); reverted with `rm` and confirmed `git status` clean. Evidence in /tmp/514/mutations-843/6-alias-outside-tier.txt.
…essor symbol (#843) codex-sol review of #880, finding 3. AdapterTierScanner and ModuleLedgerScanner both parse every file with one fixed preprocessor symbol list (ModuleLedgerScanner.ParseOptions), so a project-defined constant guarding real code in an inactive #if branch (e.g. `#if MCP` around a future MCP surface) is invisible to the walk while the guard stays green — contrary to the decision record's claim that it walks every invocation and attributed type. ModuleLedgerScanner.UndeclaredDefineConstants enumerates every *.csproj under src/ plus the repo-root Directory.Build.props, collects each <DefineConstants> value (skipping the $(DefineConstants) inheritance placeholder), and reports any symbol not in ParseOptions.PreprocessorSymbolNames. It does not add a symbol to the parse or read an inactive branch; it only fails the walk-trust check the moment a project defines one the fixed list does not carry, which is the signal to add it to ParseOptions before the guarded code can hide behind it. Wired into AdapterTierScanner.Evaluate only in this PR, as a sibling scanners can adopt later. Verified with a real-tree mutation: added <DefineConstants>MCP</DefineConstants> to src/Cluckwork.Api/Cluckwork.Api.csproj, confirmed RED with the exact "add it to ModuleLedgerScanner.ParseOptions" message, reverted with `git checkout --` and confirmed `git status` clean. Evidence in /tmp/514/mutations-843/7-csproj-defines-mcp.txt.
d61836a to
7cae308
Compare
|
Round 1 fixes pushed, and the branch is replayed onto
Also 7cae308 drops narration comments. Architecture and Documentation guards 252/252 (was 246). Round 2 goes to the same reviewer on this head. |
|
Review round 2 (Codex gpt-5.6-sol), head 7cae308: changes requested, 3 findings, all accepted. Round-1 fixes 1 and 2 verified complete by the reviewer's own re-run of its mutations; fix 3 verified but incomplete.
Product-defect tally for the stop rule: round 1 found two real guard holes, round 2 found two more (props import, alias chain). The loop continues to round 3. |
…ving imports (#843) codex-sol round 2 finding 1: UndeclaredDefineConstants only read src/**/*.csproj plus the repo-root Directory.Build.props, so a nested Directory.Build.props or an imported .props file could define an undeclared symbol invisibly. Walk every *.csproj/*.props/*.targets under the repo root instead (skipping bin/, obj/, node_modules/, .git/, web/), matching wherever the element sits, Condition or not.
…ol attribute (#843) codex-sol round 2 finding 2: an alias whose target was itself an alias (e.g. `using ToolMarker = ActualMarker;`) matched against ToolAttributes only one hop deep and never matched, so a tool type reached through such a chain was invisible to the outside-tier check. Resolve local aliases then global aliases at each hop to a fixed point, bounded by the alias count so a cycle terminates.
…843) codex-sol round 2 finding 3: KnownSurfaces mapped a surface to the privilege it grants, but nothing read the value — the loader checked privilege against the separate DirectRepositoryPrivilege constant, and the closed set it validated against was hardcoded rather than derived from the map. Changing MapMcp's mapped privilege would have left every check and test green. Derive the allowed-privilege set from KnownSurfaces.Values, and add a registry error when a row's privilege does not equal KnownSurfaces[row.Surface], naming both. RenderRow's suggested-row template now reads the same map instead of the constant, so the two cannot drift from each other either.
|
Round 2 fixes pushed (7cae308 → e255139).
e255139 drops narration comments. Guards 259/259 (was 252). Round 3 goes to the same reviewer on this head. |
|
Review round 3 (Codex gpt-5.6-sol), head e255139: approve, 0 findings. The reviewer re-ran its own round-2 mutations (a Review yield over three rounds: 9 findings, 9 fixed with tests and real-tree mutations, 0 dismissed. Rounds 1 and 2 each found real holes in the guard (alias forms, unvalidated surface, MSBuild constants); round 3 found none. Loop stopped at the approval per the review rules. Ready to merge once CI finishes. |
Closes #843
Track A slice 2 of #514, stacked on #879 (retarget to
mainafter it merges). Enforcement only, nosrc/change.What it adds
A tier is a dated exemption: a namespace, a privilege a module may not otherwise have, the surface that maps it, a reason and a
reviewByissue. The ledger gains oneadapterTiersrow today,Cluckwork.Api.Mcp→DirectRepository, surfaced byMapMcp, reviewed by #806. It is committed dormant: green now, with no/mcpand no tool class in the tree, so the ledger is not wrong the day #806 lands.AdapterTierScanner(syntax-only, same walk as the sibling guards) fails on:[McpServerToolType]type outside every tier namespace,MapMcpcall with no tier row (prints the JSON row to add),reviewBythat is not#NNN(an exemption with no end date).A tier also unions into the reach ratchet's adapter roots and its persistence-forbidden set, so a tool class's repository parameters still need an
adaptersrow and anAppDbContextparameter still fails.Mutations run against the real tree (each red before it was claimed, then reverted)
[McpServerToolType] class ProbeunderCluckwork.Api.Endpointsapp.MapMcp("/mcp")inProgram.cswith the tier row removedreviewByremoved from the rowWaterTools(IWaterUsageRepository)underCluckwork.Api.McpWaterTools.ctor → GeneralInventoryAppDbContextWhy syntax, not
EndpointDataSourceThe issue suggested keying the tier on
EndpointDataSource. The ledger guards live in the Application test project with no host, and the reach ratchet already finds top-level routes by scanningMap*calls inProgram.cs; aMapMcpscan is the same mechanism. The decision record covers this and the other rejected shapes.Scope against the issue
The issue's second bullet, owner rows for the four OpenIddict tables, is delivered by #877, not here: the table-owner guard fails #795's PR the moment it maps a table with no owner row, so the rows are written there by that author. The "add a mapped table with no owner row → red" criterion is therefore met on
mainalready. The remaining criteria are the four mutations above.For #806's author
Put tool classes under
Cluckwork.Api.Mcp, add theiradaptersrows when the ratchet prints them, and deleteAdapterTierRealTreeTests.McpTierRow_IsDormantToday, which goes stale on purpose.Tests: Architecture and Documentation filter 246/246 (was 244). Full solution builds warnings-as-errors clean.