Repository navigation
test(arch): match build symbols and reject inactive source - #1056
Conversation
|
Independent security + code-quality review (Claude Opus 5.5, high: security-review, thermo-nuclear, ponytail) at head PR #1056 security review (Opus): head
|
| Wrapper | Scanner at main 7014566c |
Scanner at PR head 11a178be |
|---|---|---|
#if NET10_0 |
green (the test: source guards parse without preprocessor symbols, so #if code escapes them (incl. tenant-bypass) #1052 hole) | RED, RealSourceTree_AllBypassesAreAllowListed |
#if CLUCKWORK_NOPE / #elif NET10_0_OR_GREATER |
(not run) | RED (#elif handled) |
#if !DEBUG |
RED | green: escapes |
#if DEBUG / #else |
RED | green: escapes |
#if RELEASE |
green | green: escapes (pre-existing) |
No other guard catches these cases. AdapterTierScanner's UndeclaredDefineConstants reads only MSBuild DefineConstants. RELEASE and DEBUG are SDK-implicit, so it never sees them.
Why this is not "retains current configuration": The PR body says the DEBUG selection is "unchanged". That is true of the list. It is not true of the five guards, which now lose visibility they had on main. The body does not say this.
Fix (boring, about 15 lines, closes the whole class): A single symbol set cannot be correct, because #if X … #else … always hides one arm. Fail closed instead. In the shared parse path, or as one real-tree test beside RealSourceTree_AllBypassesAreAllowListed, fail when any src tree parsed with ParseOptions contains SyntaxKind.DisabledTextTrivia. Today that is zero files, so the test costs nothing. It catches RELEASE, !DEBUG, #else, custom symbols and any future framework moniker, and the failure message can say "add the symbol or remove the conditional". Mutation-prove it with the #if !DEBUG and #if RELEASE cases above. A smaller alternative is to drop DEBUG and add RELEASE so the parse matches production. That fixes !DEBUG and #if RELEASE, but it still hides #if DEBUG arms and custom symbols. I do not recommend it alone.
Scope note: The same Debug-view blind spot already existed on main in ModuleLedgerScanner, AdapterReachScanner, AdapterTierScanner, CompatibilityExceptionScanner and FarmDirectoryCallerTests, which refactor(accounts): move the cross-farm account reads behind IFarmDirectory #1043 converted. Those cases are pre-existing and outside this slice. The disabled-trivia check fixes them too if it lives in shared code.
F2. P3. The fixtures pin only the NET10_0 slice. CONFIRMED
TenantBypassAllowListTests.cs:84-117, FilterFreeSetIdentityTests.cs:33-49 and the three other fixtures each prove that #if NET10_0 is seen. None proves that an inactive arm fails. This is why F1 passed review. Once F1's fail-closed check exists, one fixture with an inactive region covers every guard.
Checks that came back clean
- Parse-options mismatch between the scan and the registry hash: none. All three
GuardScannerwalks (:158,:381,:817) use the same options.QuerySignaturehashesNormalizeQueryText(scope)overDescendantTokens()of the tree that was just parsed, and disabled text is trivia, so it does not enter the hash. Nothing reparses for hashing. The registries are byte-identical and the real-tree classification tests are green. - All guards that parse real
src:grep -rn -A2 "ParseText\|ParseSyntaxTree\|ParseCompilationUnit\|SyntaxTree.Create" tests/ tools/lists 15 call sites. Every one passesModuleLedgerScanner.ParseOptions. No other Roslyn parse API is used. No.csfiles are tracked outsidesrc/andtests/. #elif: handled (table above).- In-file
#define X/#if X: handled natively by Roslyn. - Custom symbol from
.csprojDefineConstants: caught by the existingAdapterTierScannerundeclared-constant check ([A] #514 slice 2: adapter-tier rows for the MCP and OpenIddict surfaces #843). - Files the scanner never walks:
EnumerateSourceFilesskips onlybin,objandnode_modules. Source-generator output is in-memory or underobj, and is not walked. The generators in use,GeneratedRegexandLoggerMessage-style, emit no EF queries, so I see no realistic path for generated tenant code. Accepted, not a finding. - The PR's mutations, reproduced by stripping the options from one
GuardScannercall site at a time and rebuilding:- M1
:158→UnlistedBypassInAnActiveFrameworkConditional_FailsRED. - M2
:381→RawSqlLockInAnActiveFrameworkConditional_RequiresAnAccountPredicateRED. - M3
:817→FilterFreeQueryInAnActiveFrameworkConditional_IsSeenRED. - Each mutation reddens exactly one test, so each call site has its own causal test.
- M1
- Baseline: 154/154 green across TenantBypass, ExportSnapshot, InsightsReadOnly, EggLotLockSql, TransactionDelegateShape, ModuleLedger and AdapterTier. Application suite only,
DOTNET_USE_POLLING_FILE_WATCHER=1.
security-review skill output
The skill's own git context resolved to the primary worktree at 05d7e7a7 and showed an empty diff. The brief forbids subagents, so I applied the skill's method by hand to the real diff (1056-11a178be.diff).
- Result: no HIGH or MEDIUM vulnerabilities. All seven files are under
tests/, so the skill's hard exclusion 11 applies: files used only for tests. The diff adds no production code path, input handling, auth logic, SQL or secrets. - The only security-relevant effect is indirect: the soundness of the tenant-isolation guard. That is F1, a weakened control rather than an exploitable defect today, because
src/has no conditional regions.
pstack:thermo-nuclear-code-quality-review (P3, slice only)
- Q1. P3. Code-judo: one parse helper instead of 15 copies of the same decision. Every guard now repeats
CSharpSyntaxTree.ParseText(text, ModuleLedgerScanner.ParseOptions, path). The PR fixes the bug by editing each copy, and each copy needs its own fixture to prove it, which is why there are five near-identical…InAnActiveFrameworkConditional…tests. Every guard already callsGuardScanner.EnumerateSourceFiles. Add a sibling, for exampleGuardScanner.ParseSource(text, path), that applies the options and the F1 disabled-trivia check. The parser policy then lives in one place, and one fixture proves it. That removes most of the per-guard fixtures and makes F1's fix land everywhere at once. - Q2. P3. Wrong home for the parse policy.
ParseOptionsbelongs toModuleLedgerScanner, soTenantBypass/GuardScanner.cs,Eggs/EggLotLockSqlTests.csandSales/TransactionDelegateShapeTests.csnow importCluckwork.Application.Tests.Architecturefor a parser constant. Move it next toEnumerateSourceFiles, or into a small sharedSourceParsetype. - Q3. P3. Mixed helper.
TransactionDelegateShapeTests.FindViolations(:130-219) asserts parse errors and site-shape failures internally, returns violations, and passesinScopeback through anoutparameter. Return a record(Violations, InScope, ParseErrors, SiteShapeFailures)and assert in the callers. - File sizes:
GuardScanner.csgoes from 1424 to 1425 lines. It was already over 1k, and this PR is not the cause. Every other file stays well under 1k. - No structural regression otherwise. Extracting the scan bodies into
FindLockStatementsandFindViolationsis reasonable. Removing the unusedskippedlist and theRelativePathalias is a net win.
ponytail:ponytail-review (P3, slice only)
GuardScanner.cs:158,381,817+ 5 test files: reuse: 10 hand-repeatedParseText(…, ModuleLedgerScanner.ParseOptions, …)calls in this diff. Replace with oneGuardScanner.ParseSourcebesideEnumerateSourceFiles.TenantBypassAllowListTests.cs:83-117,FilterFreeSetIdentityTests.cs:33-49,EggLotLockSqlTests.cs:34-47,TransactionDelegateShapeTests.cs:108-129,ExportSnapshotSourceTests.cs:39-60,InsightsReadOnlyTests.cs:66-78: delete: once parsing goes through one helper, the six per-guard conditional fixtures collapse to one helper fixture plus one inactive-region fixture.TransactionDelegateShapeTests.cs:130-131: shrink:out List<InScopeDelegate>. Return a tuple instead.- net: about -70 lines possible.
Verdict
Fix first: add a fail-closed check for disabled trivia in src, so that #if !DEBUG and #else, which only Release builds compile, cannot escape the tenant guard.
|
Review of record (Codex Astra, xhigh) at head PR #1056 review of recordReviewed No current product behavior defect or reachable cross-farm disclosure was found. There is one confirmed regression in a security guard: Release-only queries that the base detects become invisible after this change. FindingF1. P2, CONFIRMED: preserve Release branch coverage when adopting the shared optionsLocation: The newly supplied options unconditionally define Scenario: a handler adds Evidence: I inserted that method into The fixed DEBUG selection predates this PR and is documented as an architecture-walk limitation in #846. The PR also acknowledges it. However, adopting it in the tenant scanner newly removes coverage that this security guard already had. This is a changed-path regression, not merely a request to broaden an unchanged architecture scanner. Keep the shared options, but make their DEBUG selection match the build configuration and add Release regression coverage. Do not introduce a second symbol list. Evidence: probe source, Release binary-presence test, head passes, removing DEBUG exposes the query, exact base rejects it, Release reproduction script, base-control script. Parser coverageI searched the whole checkout for
No default-options fixture parser was left behind. CompatibilityExceptionScanner's generated-usings call is the only wholly generated-string call. Insights' parser also reads real BusinessRecordOrdering.cs, so correctly participates in this fix. Inventory. Independent mutationsI reproduced three of the PR's eight real-source mutations. The control preserves the new tests and extractions but removes exactly the eight added ParseOptions arguments. Each mutated production project compiled successfully.
Three additional branch probes used the same filter-free query. Reproduction script, results and assertion text, SQL mutation, transaction mutation. I inspected the author's remaining mutation evidence without claiming to have independently repeated those five cases. Registry stability and fixture qualityBoth tenant-bypass-allowlist.json and filter-free-set-sites.tsv are byte-identical between base and head. All 38 classified query identities remain valid, including their token hashes. The four TenantBypassRealTreeTests pass with both the old parser behavior and the head behavior on the unmodified tree. The 518 tracked C# files under src contain no conditional directives, so enabling these symbols exposes no previously invisible real code in the current tree. Registry hashes and counts, clean-tree inventory, old registry run, head registry run. All eight new fixture cases fail after removing the options and pass with them. The selected run contains 28 cases: the eight new cases and 20 existing Insights write cases. The latter remain green in the control. Failures occur at the intended assertions, not compilation errors. In particular, the SQL fixture requires the M4 failure, so a stale allowlist row alone cannot satisfy it. Export and transaction fixtures retain safe alternate branches; filter-free and lock fixtures require actual occurrences. None passes vacuously. Fixtures without options, fixtures at head. Code qualityThe strict maintainability pass found no structural regression or file crossing 1,000 lines. Scanner extractions exercise the same implementation as the real-tree tests and retain their existing floors and shape assertions. GuardScanner was already 1,424 lines and grows by one. Reusing the shared configuration is the right structure; F1 concerns its value. ponytail:ponytail-review: Lean already. Ship. This is the complexity-only result; F1 blocks the overall verdict. Net: -0 lines possible. Quality summary, details. NitsNone. Validation and cleanupThe clean head passed all 712 Application tests in Debug before mutation and all 712 in Release after restoration. Builds completed without warnings or errors. Every test command set DOTNET_USE_POLLING_FILE_WATCHER=1. No integration, Domain, or AppHost tests were run. Debug baseline, final Release run. All review edits were restored; git status was clean and git diff --check passed. No commits, pushes, GitHub comments, or edits to another checkout were made. The assigned detached checkout was removed with the requested git worktree remove command; the report and evidence remain in the reviews directory. Verdict: REQUEST CHANGES for F1, a confirmed P2 Release-only tenant-guard regression. |
|
Reply to Astra F1. The fix is prepared in I reproduced your Release DLL presence proof with the old options: the unclassified method existed while the registry assertion passed. With the fix, the same The full Application suite passes 720/720 in both Debug and Release. The original eight mutations still fail their own assertions in both builds. A new inactive-region tripwire rejects hidden branches and custom symbols. No registry row changed. The PR body has the updated mutation tables; reproduction scripts and TRX evidence are in |
|
Reply to Opus F1 and F2. The fix is prepared in Real-source probes for The optional parser-helper migration and policy relocation are deferred and listed in the PR body. They would broaden the parser and fixture changes; this fix keeps one symbol list and adds the shared real-source tripwire directly. The optional transaction result reshaping is also deferred. Evidence is in |
PR #1056 round 2 review of recordReviewed head The original F1 is fixed. No PRODUCT defect or currently reachable cross-farm disclosure remains identified. One nonblocking P3 inventory limitation remains below; it does not reopen the original Release regression. Reproduced mutationsEach source probe compiled in Release. I ran “Registry” means the named filter-free classification assertion rejected the unclassified query. “Tripwire” means
These were nine real-source probes and 36 separately checked class runs. An active bad query does not need the tripwire to fail: the original tenant assertion still rejects it. No unexpected failures occurred. Configuration and coverage
All 16 ParseText calls use the shared options, and there is still only one WithPreprocessorSymbols declaration. No ParseSyntaxTree call was found. Every current real-source parser input is under The nested and elif probes confirm that DescendantTrivia finds their DisabledTextTrivia. Both registry files remain byte-identical to Nonblocking findingN1. P3, CONFIRMED. The tripwire inherits a narrower directory inventory than the transaction guard. Location: Scenario and evidence: I placed a scratch transaction handler under This is an inherited enumeration limit, not a newly lost protection. The directory is gitignored and contains no tracked product source, so it is nonblocking for this slice. Align the inventories if compiled C# is allowed there. The tripwire should be described as covering its enumerated source set, not every possible compiled file. No current product defect follows from this finding. Code quality, P3 and slice-scopedThe thermo-nuclear review found no structural regression. The fix adds a 73-line test class, uses Roslyn directly, and retains one symbol policy. No file crosses 1,000 lines. A broad parser-wrapper migration, policy relocation, or transaction-result refactor is unnecessary for this fix; the existing caller-specific regressions still prove their individual parse sites. N1 is the only scope note. Ponytail-review: Lean already. Ship. Net: -0 lines possible. No additional nits. Validation and evidenceAfter restoring every mutation, 11 selected Application classes passed 146/146 in Release and 146/146 in Debug. The separate Review-configuration check passed 8/8, for 300 clean selected cases. These cover the new tripwire, module ledger, tenant registry and fixtures, export snapshot, Insights, lock SQL, transaction shape, adapter tiers, and farm-directory caller guard. Every invocation selected one class and set Evidence remains at All review edits were restored, git status is clean, and git diff --check passed. No product change, commit, or push was made. The assigned checkout will be removed after posting this comment. Verdict: APPROVE with nonblocking P3 note N1; the original Release-only guard regression is fixed, and no PRODUCT defect is identified. |
Code inside active framework conditionals escaped several source guards. The guards now share
ModuleLedgerScanner.ParseOptions, with DEBUG selected only in Debug and RELEASE selected only when the compiler defines it. A new real-source tripwire rejects every region the shared parse leaves inactive, so choosing one configuration cannot hide an alternate branch.Closes #1052
Change map
Architecture/ModuleLedgerScanner.cs: retain one shared symbol list and match its configuration symbol to the test build. MSBuild reportsTRACE;DEBUGfor Debug andTRACE;RELEASEfor Release.Architecture/SourcePreprocessorTests.cs:RealSourceTree_HasNoInactiveCodewalks every source file using the existing exclusions and 400-file floor, then rejects RoslynDisabledTextTriviawith file and line. Fixtures cover custom symbols, negation, else, both elif positions, an active framework region, and compiler-selected configuration.Architecture/ModuleLedgerTests.cs: the existing DEBUG-only edge fixture now expects an edge only in a Debug build.TenantBypass/GuardScanner.cshas three passes;Architecture/InsightsReadOnlyTests.cshas two;Architecture/ExportSnapshotSourceTests.cs,Eggs/EggLotLockSqlTests.cs, andSales/TransactionDelegateShapeTests.cshave one each. Their scanner-specific conditional regressions remain.docs/decisions/843-adapter-tiers.mdand846-adapter-reach-ratchet.md: replace the waived conditional-compilation limitation with the new configuration and inactive-region checks.Parser inventory
The inventory came from
git grep -n 'ParseText(' -- tests. All 16 calls use the shared options, including the two new preprocessor-test calls. There is oneWithPreprocessorSymbolsdeclaration and no copied list. The existing architecture scanners andFarmDirectoryCallerTestsalready used shared options.ExportSnapshotSourceTests.FindViolationsreads realExportQueries.cs. Insights' semantic parser reads real Insights files andBusinessRecordOrdering.cs, alongside generated usings and fixture strings. Neither is fixture-only. The generated-string-only parser left alone isCompatibilityExceptionScanner's appendedimplicitUsingscall, which already used shared options. The new configuration-branch fixture also uses them; no default-options fixture parser was found.Mutations
All twelve scratch source mutations compiled in Release with zero warnings and errors, then were restored. The eight original mutants passed the exact base
7014566ctests built in Release and still failed their own assertions with both revised Debug and Release test assemblies. The tripwire was not substituted for those assertions.EggLots.IgnoreQueryFilters()TenantBypassRealTreeTests.RealSourceTree_AllBypassesAreAllowListedunexcused bypass [IgnoreQueryFilters]raw-SQL row lock missing an AccountId predicate (M4)db.Users.ToList()without tenant comparisonTenantBypassRealTreeTests.RealSourceTree_FilterFreeSetSitesAreStableAndClassifiedunclassified filter-free-set candidateswithGuardProbeHandler.Handle(),db.Users,9f5b2b0adb.SaveChangesAsync(ct)InsightsReadOnlyTests.ReadSession_UsesOnlyApprovedPersistenceOperationsMicrosoft.EntityFrameworkCore.DbContext.SaveChangesAsync(System.Threading.CancellationToken)IReportQueriesInsightsReadOnlyTests.Endpoints_DoNotUseTheReadSessionPortsDirectlyAssert.False(NamesReadSessionPort(...), file)fails withsrc/Cluckwork.Api/Endpoints/Accounts/AccountEndpoints.csEggLotLockSqlTests.EveryEggLotsForUpdate_OrdersByProductionDateThenIdFOR UPDATE over EggLots must end ORDER BY "ProductionDate", "Id"TransactionDelegateShapeTests.NoExitBelowTheInnerSaveInAnyExecuteInTransactionDelegateexits the `ExecuteInTransactionAsync` delegate **after** its inner `SaveChangesAsync`, reported atAddOrderItemHandler.cs:231requestDb.CustomersExportSnapshotSourceTests.EveryDatasetArm_NamesTheSnapshotFieldDirectly"customers" reads through [requestDb], not only activeDbThe eight original conditional fixtures failed first with old parser options. The new configuration fixture also failed first in Release with
Expected: ["ReleaseOnly"], Actual: ["DebugOnly"].Review regression probes
Before the fix, a scratch reflection test confirmed that the Release Infrastructure DLL contained the
#if !DEBUGquery whileRealSourceTree_FilterFreeSetSitesAreStableAndClassifiedpassed. All four probes below also passed that registry assertion at reviewed head11a178bein Release.db.Users.ToList()#if !DEBUG#elseof#if DEBUG, with a safe nonempty Debug arm#if FOO, product compiled with-p:DefineConstants=FOO#if RELEASERegistry failures quote
unclassified filter-free-set candidates. Tripwire failures are onSourcePreprocessorTests.RealSourceTree_HasNoInactiveCodeand quotesource guards cannot inspect inactive C# regions, namingsrc/Cluckwork.Infrastructure/Repositories/GuardProbeHandler.csand the inactive line. A configuration with no hidden region keeps the tripwire green while the visible bad query fails the registry guard. Fixtures also prove disabled text in both elif positions.Registry and validation
No registry row or query-token hash changed. Both tenant registry files remain byte-identical to
7014566c. No real-source violation surfaced; the unmutated source tree has zero inactive regions in both configurations. Every scratch source edit and binary-presence fixture was removed.Measured reviewed-head baseline: Application 712/712 in Debug and 712/712 in Release. Final full Application suites: 720/720 in Debug and 720/720 in Release. After restoring all mutations, the seven original real-source assertions plus eight preprocessor cases passed again, 15/15 per configuration.
dotnet build Cluckwork.sln -c Releasepassed with zero warnings and errors. All test commands setDOTNET_USE_POLLING_FILE_WATCHER=1and ran sequentially. One custom-symbol control invocation had an internal CLR runner error; its repeated control and probe completed cleanly.git diff --checkpassed.The initial revision also passed Domain 495/495, AppHost 10/10, and nine targeted Postgres integration cases through
sg docker -c. Those unchanged suites were not repeated for this test-only follow-up; the full integration suite was not run.Deslop and deferred cleanup
The complete diff was reviewed with deslop, thermo-nuclear review, and ponytail-review. The follow-up adds no symbol copy, parser wrapper, registry exception, dependency, or production change. No file crosses 1,000 lines. The original extraction removed the unused skipped-delegate list and redundant file alias.
Opus' optional shared parser helper and relocation of the policy from
ModuleLedgerScannerare deferred. Migrating every parser and reshaping fixture setup would broaden this fix; the single options value and real-source tripwire provide the required behavior directly. The optional transaction-scan result reshaping is also deferred. Existing scanner-specific regressions remain because they prove all eight original call sites.Updated reproduction scripts, full assertion text, diffs, TRX results, parser inventory, and local review evidence remain at
/home/mforce/.cluckwork-slices/reviews/1052/revision2/; the original evidence is in its parent directory.