Fix EFC store-root path leakage at the filing boundary - #639
Merged
drmoisan merged 39 commits intoAug 27, 2026
Merged
Conversation
Adds the promoted potential-bug record and the active full-bug feature folder for issue #614, covering the defect chain that lets a full Outlook hierarchy path (the mailbox store root) reach EmailFilerConfig.DestinationOlStem and surface as an ArgumentException from FolderConverter.ToFsFolderpath. issue.md carries the complete record, including the Suspected Cause and Proposed Fix sections that the promotion template does not map into the GitHub issue body. spec.md and plan.md are the seeded templates and are filled in by the subsequent feature-document and planning steps. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…otstrap Bootstrapping a clean worktree for issue #614 surfaced a defect that is not on the #614 path-representation chain: all 16 .csproj files pin <Analyzer Include> paths to Meziantou.Analyzer 3.0.156 and Roslynator.Analyzers 4.16.0 while every packages.config requests 3.0.174 and 4.16.1, so a clean-checkout analyzer build fails with 80 CS0006 errors. CI stays green only because the NuGet cache restore-keys prefix fallback serves analyzer directories left over from before the version bump, which also means CI has been running superseded analyzer versions since the Dependabot bump in f8e22af. Promoted to its own issue rather than absorbed into #614. The #614 branch works around it locally in the gitignored packages/ directory and changes no project file. Refs #615 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Confirms the full producer-to-consumer chain that carries a mailbox store root into EmailFilerConfig.DestinationOlStem, with file:line evidence at every hop, and records nine confirmed defects (D1-D9) plus three off-chain findings. Key corrections to the starting hypothesis set: leaf breadcrumb segments are not activatable, so the D2 exposure is ancestors and children only; the crash-class leak for presented rows flows through SelectRow's FilingTarget pass-through rather than ToHierarchyPath; and PR #611 for issue #609 modified only FolderPredictor.ProjectSuggestionPath, so it does not cover the store-root stem. The Copilot "delete the dot" hypothesis is re-confirmed as symptom-level: it would convert the crash into silent misfiling. Recommends a shared pure static ArchiveStemContract enforced at the router, both EmailFilerConfig.ResolvePaths overloads, and an extracted stem helper for EfcDataModel, chosen over a wrapper value type and over a router-only fix. Records how to avoid regressing issue #499. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
The issue #614 defect census confirmed three real defects that are not on the path-representation chain. Each is promoted to its own issue rather than left as prose in the feature folder, where it would disappear at merge. - #616 UtilitiesCS/EmailIntelligence/FolderConverter.cs is an uncompiled duplicate of the UtilitiesCS.FolderConverter type with two always-false guards and a latent CS0101 under an SDK-style project migration. - #617 FolderPredictor.CreateFolder uses a non-short-circuiting bitwise or, so an empty parentBranchPath throws IndexOutOfRangeException even when the left operand is already true. - #618 MatchBestSpecialFolder matches by substring rather than a separator-bounded path prefix, and its tests pin that relation as expected. None is absorbed into #614. The reasoning for each exclusion is recorded in the issue body and in the orchestration checkpoint. Refs #614, #616, #617, #618 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
enforce-powershell-batch-budget.ps1 writes .claude/state/powershell-batch-budget.<session_id>.json on every agent session, but no ignore rule covered it, so each run left the worktree dirty. The file is session-scoped and records absolute local paths including the account name, so committing it would also violate the host-identifier redaction constraint tracked in issue #602. The .gitignore comment block immediately above already states the intent to keep per-agent state out of version control (issue #149); this only adds the missing rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Replaces the seeded template's hypothesis list with the confirmed D1-D9 census, each carrying file:line evidence, and fills every previously blank section: proposed fix, ArchiveStemContract signatures, boundaries and invariants, test strategy, risks, and rollout. Records the three corrections the research made to the original hypotheses: leaf breadcrumb segments are not activatable; the crash-class row leak flows through SelectRow's FilingTarget pass-through rather than ToHierarchyPath; and D7's MatchBestSpecialFolder half has no production caller and is off-chain. Adds 26 acceptance criteria as the authoritative source for full-bug work mode: one per confirmed defect, the fail-before/pass-after regression pair, non-regression of the #609 and #439 scenarios, the explicit non-absorbing interaction with open #499, redaction, and the four-step toolchain. D6 and D7 are held in scope. Any later deferral must be a recorded scope reduction with a promoted follow-up issue, not a silently unchecked box. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Replaces the seeded generic bug template with an 80-task plan covering all nine confirmed defects D1-D9 and all 26 acceptance criteria. Ordering follows the repo bugfix workflow: Phase 1 adds the two regression tests tagged [expect-fail] with ExpectedExitCode: 1 evidence artifacts and proves them failing on the pre-fix tree, before Phase 2 introduces the shared ArchiveStemContract and Phases 3-7 apply the fixes behind it. Notable plan decisions: - Every new .cs file carries an explicit <Compile Include> item. These are non-SDK packages.config projects with no globbing, so an unregistered file compiles nowhere while the build still exits 0. Wiring is proven by test discovery, not by grep. - New companion test files avoid pushing three near-limit test files past the 500-line ceiling, and EfcSelectionGuard.cs keeps EfcFormController.cs from growing further past its pre-existing 1084 lines. - No suite-wide "Failed: 0" gate. The suite carries a pre-existing Console.Out race tracked by open issue #594, so the gate is no new failure against the 6481/6482 baseline plus zero failures in touched classes. - Raw TRX and coverage output embed account and host names, so they are written only to the gitignored coverage/ tree; evidence artifacts are redacted Markdown summaries (issue #602). - Two EVIDENCE_LOCATION_OVERRIDE_REJECTED corrections route the spec's non-canonical evidence/coverage/ and evidence/qa/ to the canonical folders. Validated with the MCP plan validator (ok, no warnings); the planner has no access to that tool and correctly declined to self-report a result. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
The plan passed the MCP structural validator but the atomic-executor
preflight found six blocking defects. All are the same class: acceptance
conditions that read precisely but cannot fail.
- P5-T2 asserted a zero-hit search for `fsPath.Substring(3)` claiming it was
present pre-change. CSharpier wrapped the chain across lines 157-159, so
the literal has zero hits and the gate passed before any work. Replaced
with `.Substring(3)` and the unanchored-replace literal, both verified
present exactly once.
- P5-T3 asserted "zero hits in exception-message context", which is not
expressible as a command, and conceded its gate was carried elsewhere.
Replaced with the message fragment `has a value of {fsPathExDividers}`.
- P5-T4 asserted `.Should().BeEmpty();`, which returns three hits pre-change
and would still return at least one after. Replaced with the full :329
assertion line, and a production-side gate was added for D5f, which
previously had none.
- P8-T2 and P9-T6 used `git diff --name-only $(git merge-base ...)..HEAD`,
which does not parse under pwsh: it emits git's usage block, produces no
path list, and still reports exit 0. It was also blind to the change
because no commit exists before P10-T28. Replaced with a two-statement
form comparing the working tree to the merge base.
- P10-T22 cited an inline check-off comment with nowhere to live, leaving
AC22 the only one of 26 without on-disk evidence. Now produces an artifact.
- P10-T25 instructed checking off an AC the plan itself calls unsatisfiable.
Now cites the orchestrator ratification of the net-non-growth reading.
Twelve non-blocking items also applied, including a nullable-context rule,
a TRX carve-out for the coverage runner, and the AC5 IsDeleteRelevant
separator-boundary cases.
Phase and task counts unchanged at 11 and 80. Re-validated with the MCP plan
validator (ok, no warnings).
Refs #614
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Preflight iteration 2 confirmed all six earlier gate repairs and found two further blocking defects. B7 is a regression the earlier B4 repair created, and surfacing it is the point. The old `..HEAD` form emitted no path list at all, so nothing could be compared against the in-scope allowlist. Now that the command works, it reports the five pre-plan commits this branch already carries: `.gitignore` and five promoted potential-bug records. Those are committed and cannot be reverted without destroying the promotion, research and spec work the plan depends on, so "zero out-of-scope paths" was unsatisfiable before execution began. They are now allowlisted explicitly as pre-existing branch state, and a fourth statement, `git diff --name-only HEAD`, proves this change does not modify them so the allowance cannot mask new drift. B8 scopes the AC21 redaction sweep correctly. FolderConverterTests.cs carries the fabricated literal `first.last@company.com` at :22-23 and this change edits that file at :329, so a file-level search could never record `SearchResult: none`. The `@`-string claim is now scoped to changed hunks with the pre-existing literal recorded as an evidenced exception; the account-name claim stays scoped to whole changed files. Six non-blocking items applied, including a correction to a Global rule that described a `\.claude\` assembly exclusion the coverage runner does not implement. Phase and task counts unchanged at 11 and 80. Re-validated with the MCP plan validator (ok, no warnings). Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Preflight iteration 3 returned ALL CLEAR. Records the CSharpier chain-wrap hazard that defeats single-line search gates, and the PowerShell double-quoted-string quoting trap that produced a false zero-hit result during literal verification. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…idence Completes plan Phase 0 and the primary half of Phase 1 ahead of any production change, per the repo bugfix workflow. Adds Issue614_ResolvePaths_WithStoreRootStem_RejectsNonRelativeStemWithoutLeakingIdentifiers to UtilitiesCS.Test/EmailIntelligence/EmailFilerConfig_Tests.cs, driving the pure configuration seam with OlAncestor \mailbox@example.com\Archive and DestinationOlStem \mailbox@example.com. The test asserts two things: that the thrown diagnostic names DestinationOlStem and the archive-relative rule, and that it does not embed the mailbox address. Both assertions fail on the unmodified pre-fix tree, exit code 1 against the declared ExpectedExitCode: 1. The recorded failure text shows why: the exception originates in FolderConverter rather than the EmailFilerConfig stem contract, names fsPath instead of DestinationOlStem, and leaks mailbox@example.com verbatim. That is the D4 plus D5 defect chain observed live rather than argued. Also lands the Phase 0 baseline evidence set: environment, csharpier check, analyzer build, nullable build, and the test-coverage baseline at 84.8099% filtered. Raw TRX stays in the gitignored coverage/ tree with an explicit LogFileName and ResultsDirectory so vstest cannot create an account-or-host-named file. Every identifier in the committed evidence is a fabricated placeholder (issue #602). Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…avior Execution surfaced a conflict three preflight rounds missed. The plan listed BreadcrumbBridgeRouterIssue439Tests in the must-stay-green set while D1 and D9 change behavior that one of its tests pins, which is unsatisfiable as written. Issue439ArchiveRootBoundarySelectionAndHostEventRemainDeterministic binds archiveRoot \Archive, activates a segment whose path is \External\Clients, outside that root, and asserts the router stores it verbatim. Its own comment describes selecting "a verified hierarchy path outside that root". That is the D1 verbatim pass-through the fix must reject, so the fix necessarily breaks it. New P3-T4 corrects the assertions as a deliberate, documented spec correction, following the treatment the plan already applies to FolderConverterTests.cs:329 for D5f. Issue #439 deliberately permitted selecting outside the archive root; issue #614 intentionally revokes that for the filing surface, which the spec already describes as a user-visible behavior change. Recorded in P8-T1 item (h). The former P3-T4 becomes P3-T5. The planner also found that the must-stay-green set does not appear in P5-T7 where the delta assumed, and amended its real locations instead; verified the adjacent slash-only and Issue609 tests stay green rather than assuming it; and pinned P3-T2 so the guard rejects only out-of-root targets, leaving the at-or-under-root rooted assertion at :165 passing verbatim. A third pre-existing over-limit file, BreadcrumbBridgeRouterIssue439Tests.cs at 694 lines, is folded into the AC25 net-non-growth handling under a new orchestrator adjudication. The planner recorded 695; the measured value is 694, and at 695 the gate would have permitted a line of growth, so all five references were corrected. Phase and task counts 11 and 81. Re-validated with the MCP plan validator. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…contract Adds ArchiveStemContract, the single authority for the archive-relative stem contract, and routes the BreadcrumbBridgeRouter producer surface through it (defects D1, D2, D3, D9 at the router). - ArchiveStemContract: IsFullOutlookPath, RequireArchiveRelativeStem, and a prefix-anchored, OrdinalIgnoreCase, separator-terminated TryMakeArchiveRelative that never passes its input through on failure. - SelectHierarchyPath: an activated path outside the bound archive root, and the archive root itself, are deterministic non-selections. The prior selection is left unchanged and is never set to null (#499 boundary). - SelectRow: an out-of-root FULL Outlook filing target is rejected; a rooted target at or under the root still passes verbatim (#439 contract), and an unbound root leaves the row unguarded (#349 no-root binding mode). - ToHierarchyPath: a relative presented target keeps its root prefix (#609); an out-of-root full path is no longer fabricated into a hierarchy identity. - Diagnostics name the rejection reason only; the path is withheld (#602, AC21). Test correction: Issue439ArchiveRootBoundarySelectionAndHostEventRemainDeterministic codified the D1 defect as expected behaviour by requiring selection of a path outside the bound root. Its three assertions are corrected as a deliberate, documented spec change, the D1/D9 analogue of the FolderConverterTests.cs:329 correction the spec already mandates. log4net is added to QuickFiler.Test so the spec-required "emits a diagnostic" assertion in AC2/AC3 is testable through the established MemoryAppender pattern. BreadcrumbBridgeRouter.cs finishes at 596 lines and BreadcrumbBridgeRouterIssue439Tests.cs at 694, both at their recorded pre-existing baselines (AC25 net non-growth). Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…d in FolderConverter Phase 4 (D4, D9) and Phase 5 (D5a-D5g, dead parameter) of the #614 fix. EmailFilerConfig (D4): - Both ResolvePaths overloads call RequireArchiveRelativeStem(DestinationOlStem, ...) BEFORE concatenating the stem onto the ancestor, so a non-relative stem can no longer reach folder resolution or the filesystem projection. - GetStem and IsDeleteRelevant use prefix-anchored, separator-terminated, OrdinalIgnoreCase comparisons instead of unanchored Replace/Contains. EfcSelectionGuard (D9): - New shared predicate rejecting null, empty, whitespace, banner sentinels, sub-three-character values, and any full (rooted) Outlook or filesystem path. - ActionOkAsync and IsValidSelection both delegate to it, so a value can no longer be accepted by one path and rejected by the other. FolderConverter: - D5a/D5b: per-segment Windows folder-name validation (invalid characters, trailing dot, trailing space, reserved device names) applied ONLY to segments the converter derives. The caller-supplied filesystem ancestor is never validated, so a root of the "OneDrive - Org" shape no longer fails. '.', '[' and ']' are no longer banned. - D5c: the Substring(3) drive-prefix assumption is gone; UNC and sub-three-character ancestors are neither mangled nor throw ArgumentOutOfRangeException. - D5d: the ancestor strip is prefix-anchored, separator-terminated and OrdinalIgnoreCase via TryMakeArchiveRelative. - D5e: the thrown message names the violated segment rule only and embeds no derived path value; paramName stays nameof(fsPath) for the pre-existing contract test. - D5f: "Remove illegal characters" removes only the illegal characters. The test assertion that codified the defect by requiring an empty result is corrected as a deliberate, documented spec change. - D5g: ResolveOlRoot selects by separator-terminated prefix, not Contains. - The never-read `bool ask = true` parameter is removed; no call site supplied it. ArchiveStemContract messages now state the "archive-relative" rule verbatim, which the spec-pinned primary regression test asserts. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…ppGlobals Phase 6 (D8) and Phase 7 (D6, D7) of the #614 fix. EfcDataModel (D8): - New pure internal helper ToArchiveRelativeStem replaces the unanchored Replace + single Substring(1) in MoveToFolderAsync(MAPIFolder, olAncestor, ...). It fails explicitly through the shared contract instead of returning a mangled stem for a folder outside the archive root. Both live callers are unchanged. AppOlObjects (D6): - ArchiveRootPath no longer returns an unverified default-store-scoped string combine. The composed path is cross-checked once, at resolution time, against the folder that actually resolves for it, and the validated result is cached, so no per-filed-item COM round-trip is added. The decision logic is the new pure ArchiveRootPathGuard helper, unit-testable without an Outlook COM object. AppFileSystemFolderPaths (D7): - LoadFolders reads the environment through an injectable Func<string, string> seam defaulting to Environment.GetEnvironmentVariable, so no test mutates process environment state. - The OneDrive root resolves from OneDriveCommercial, then OneDrive, then OneDrivePersonal, and now fails explicitly with a redacted diagnostic when none is set. The AppData and arbitrary-first-entry fallbacks are removed: both silently produced a filing root unrelated to OneDrive. - MatchBestSpecialFolder is not modified; no diff hunk intersects it. All diagnostics name the violated rule only and withhold the path, which can carry a mailbox address or a user-profile path (#602). Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…nostic assertions Two test-only corrections found by the Phase 9 QC loop. 1. ArchiveStemContract.TryMakeArchiveRelative's `root.Length == 0` guard after TrimEnd was an uncovered pure branch. It is reachable: a root consisting only of separators is not whitespace, so it passes the emptiness guard and then trims to length zero. Added TryMakeArchiveRelative_SeparatorOnlyRoot_ReturnsFalse; the method is now at 100% line coverage. 2. BreadcrumbBridgeRouterIssue614Tests asserted an EXACT count of rejection diagnostics captured from a log4net MemoryAppender. log4net binds one logger per type, so that appender is shared with router tests running concurrently in other classes, making the count order-dependent; SegmentActivate_CrossStoreAncestor_LeavesSelectionUnchangedAndDiagnoses failed intermittently with "2 such items were found". The three count assertions are replaced by an existence assertion plus an assertion that every matching message is value-free. Concurrency can only ADD events, never remove them, so the existence claim is deterministic; the unchanged-selection assertion in the same test is what proves the rejection came from this router. Also records the Phase 8 and Phase 9 evidence: change description, scope audit, redaction sweep, manual-validation record, the four QC gate artifacts, the coverage delta, and the file-size and clean-pass declarations. Final clean pass: csharpier check 0 (0 files rewritten of 4732 hashed), analyzer Rebuild 0, nullable Rebuild 0, full suite 6569/6569 passed with filtered first-party line coverage 84.8696% against an 84.7797% merge-base baseline. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…tion outcome Phase 10 of the #614 fix. - Checks off AC1 through AC26 in spec.md, one at a time against named evidence, changing only the checkbox and never the criterion text. - Adds the test-policy audit backing AC22: all six banned-API searches (Thread.Sleep, Task.Delay, DateTime.Now, Random.Shared, Path.GetTempPath, Path.GetTempFileName) return zero hits across the 11 test files this change created or modified, and a companion File.Create / Directory.Create search also returns zero. - Updates spec.md status metadata and appends the outcome summary to issue.md. - Adds the issue-update mirror. It carries a POSTING BLOCKED header: this session has no verified GitHub authentication and the plan authorizes no gh write, so the intended text is recorded locally instead of posted. Two items are flagged for maintainer review and are recorded in the evidence, not only in prose: the five live-Outlook manual validation steps are each recorded as NOT EXECUTED with reason, and one path outside the plan's enumerated in-scope list was modified (QuickFiler.Test/packages.config, one line pinning the log4net version the rest of the solution already pins, the mechanically necessary companion of the allowlisted QuickFiler.Test.csproj reference addition that made the AC2/AC3 "emits a diagnostic" assertion testable). Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…log4net appenders Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Execution surfaced two real defects outside the #614 path-representation chain. Both are promoted to their own issues rather than left as prose in the feature folder, where they would disappear at merge. - #627 UtilitiesCS.Test/OutlookExtensions/FolderConverter_Tests.cs exists on disk but has no <Compile Include> entry, so in these non-SDK projects it is never compiled and its assertions never run. Found because a TestCaseFilter naming the class matched zero tests. No behavior is presently unverified, since a compiled test duplicates the assertions, but the file is indistinguishable from working coverage and will absorb future edits that silently do nothing. - #628 TaskMaster/TaskMaster.csproj line 37 carries a <PublishUrl> containing a real user-profile path, account name, and organization name, committed and present in history. This extends the issue #602 host-identifier concern from runtime exception messages to committed build configuration, where it is more durable. This change does not touch that line; its only hunk in that file is at line 416. Also records the issue #614 outcome update, which the executor correctly reported as POSTING BLOCKED rather than claiming success, and which the orchestrator has now posted with verified authentication. Refs #614, #627, #628 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Records the policy audit, code review, and feature audit for the issue #614 branch at head 0209250 against merge-base c279d40. The review independently re-ran all four toolchain gates rather than reading evidence: csharpier check exit 0, analyzer rebuild exit 0, nullable rebuild exit 0 with zero CS86xx, and 6093 tests passing across the three changed test assemblies. Every executor figure it re-measured matched. Both PR context artifacts were absent and were regenerated from git plumbing so the changed-language set resolves correctly to CSharp. Coverage is recorded as an explicit FAIL against the 85% line floor in .claude/rules/general-unit-test.md: 53972/63594 = 84.8696%. The shortfall is pre-existing, the merge-base measured 84.7797%, and this change improves it by 0.0899 points with 100% coverage on new code. Branch coverage passes at 78.8331%. The remediation trigger is not met; the workflow's numeric threshold is 80%, cleared by 4.87 points. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Feature review returned GO with zero blocking findings, classifying CR-1 and CR-2 as Major but non-blocking. That disposition is overridden for delivery. Both are defects this change introduces on the very chain issue #614 exists to fix, and both were verified against merge-base c279d40 before this cycle opened. CR-1: the pre-change OK guard was only null or the ==== banner prefix. The Length < 3 rule lived in IsValidSelection, which gated folder creation, not filing. Consolidating both call sites onto IsValidFilingSelection silently applied a creation rule to the filing path, so filing to an archive folder named HR, IT, PR, QA or Q1 now fails with "Please select a valid folder". That worked before. AC16 asks for empty and full-path rejection; it does not ask for a minimum length. CR-2: SelectRow deliberately passes rooted at-or-under-root targets verbatim, as the untouched Issue439AlreadyRootedTargetRemainsUnchangedWithCaseInsensitive ArchiveMatch requires, but IsValidFilingSelection rejects every rooted value. That class is selectable in the breadcrumb surface and unfilable at OK. Making a correct destination unreachable is a stronger failure than reaching a wrong one, so shipping a store-root fix that breaks filing to a two-letter folder is not an end-to-end solve. Out of scope for this cycle and retaining the reviewer's disposition: CR-3, CR-4, all Minor findings, the pre-existing coverage shortfall, and the unexecutable live-Outlook AC26 steps. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Six phases, 34 tasks, MCP-validated. CR-1 is resolved by splitting the predicate rather than loosening it. The filing predicate drops the length rule; a new IsValidCreationSelection keeps today's full rule set and takes over IsValidSelection, whose two consumers are both on the folder-creation path. The rule stays where it always belonged. CR-2 is resolved by making the filing guard archive-root aware, mirroring the scope-pinning already applied to the router in P3-T2: a rooted value is rejected only when it genuinely fails TryMakeArchiveRelative, so a store-root or cross-store value still fails while a valid under-root rooted value passes. The archive-root design question was resolved deliberately. After the D6 fix ArchiveRootPath can throw InvalidOperationException, which is verified as both the documented and the actual throw type. A new ResolveArchiveRootOrEmpty helper catches only that type, logs a redaction-safe diagnostic, and returns an empty root, which makes the guard reject rooted selections while relative stems continue to file. No exception reaches the OK button. Three alternatives were considered and recorded, including an inline try/catch rejected because it needed 12 to 13 lines against 12 lines of headroom in a file already over the size ceiling. Phase 1 is a behavior-preserving seam phase because the two-argument regression test cannot compile against the current one-argument signature. Both expect-fail tests then compile and fail for the behavioral defect, each with its own ExpectedExitCode 1 artifact before its fix. Every fixed-string gate literal was verified against the tree before the plan was accepted, and the orchestrator re-verified the load-bearing ones independently. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…rd length rule and rooted-target scope pinning) CR-1: the filing path applied a folder-creation minimum-length rule, so filing to an archive folder named HR, IT, PR, QA or Q1 failed. The predicate is split: IsValidFilingSelection carries no length rule; a new IsValidCreationSelection keeps it and is what the IsValidSelection property now delegates to. CR-2: the guard rejected every rooted value while BreadcrumbBridgeRouter.SelectRow deliberately admits an at-or-under-root rooted target verbatim, making that class selectable but unfilable. The guard now rejects a rooted value only when it fails ArchiveStemContract.TryMakeArchiveRelative against the archive root, mirroring the router. D1/D4/D9 are preserved: store-root, cross-store, above-root, drive-rooted and separator-boundary near-miss values all still fail that test, each pinned by a named regression test. The archive root reaches the OK path through a new throw-tolerant helper, EfcSelectionGuard.ResolveArchiveRootOrEmpty, taking a Func<string> accessor and an Action<string> diagnostic sink. It degrades the one documented D6 failure to an empty root plus a fixed redaction-safe diagnostic instead of tearing down the form; every other exception propagates. Fail-before and pass-after evidence recorded for both findings. Full suite 6587/6587/0 against a 6569/6569/0 baseline; filtered line coverage 84.8790% against 84.8712% and branch 78.8523% against 78.8454%; EfcSelectionGuard.cs at 100% line and 100% branch. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…the plan checklist Adds the P5-T10 final-commit evidence artifact naming commit cbad2da and its committed path list, and marks P5-T10 complete. Written after cbad2da by necessity, so it lands as a follow-up commit; git status --porcelain is empty after this commit. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
The cycle 1 re-audit returned NO-GO with one blocking finding. CR-1 is confirmed resolved; CR-2 is not, and the cycle 1 remedy made one path worse. RC-1: the widened filing guard now accepts rooted under-root values but normalizes nothing, so the accepted value is carried verbatim to the D4 boundary this same feature added, where RequireArchiveRelativeStem throws on any rooted value. ButtonOK_Click is async void and rethrows, so the ArgumentException is unhandled, and it lands after the form has already been hidden. At the pre-remediation head that same value produced a benign "Please select a valid folder." dialog. Reachability is real: ProjectSuggestionPath strips the archive prefix only when the suggestion is strictly under it, so a suggestion whose folder is the archive root is returned rooted and verbatim. The orchestrator addendum records that the root cause is the cycle 1 requirement wording. Those inputs named a symptom, "the two guards must agree", which the planner correctly satisfied by widening the strict guard. The requirement should have named the invariant: the value reaching the filing boundary must already be an archive-relative stem. Cycle 1 preflight then traced the guard in isolation and never followed an accepted value forward, so every gate asked whether the guard rejects what it should and none asked whether what it accepts is filable. Cycle 2 enforces the invariant at the producer and makes the composition test mandatory, since that is the test whose absence permitted RC-1. Refs #614 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
…637 Maintainer decision, recorded verbatim in the cycle inputs. Cycle 2 is a partial revert of the cycle-1 CR-2 remedy rather than the producer-side normalization the orchestrator had drafted. The maintainer verified RC-1 independently and supplied a sharper diagnosis: the D8 normalizer EfcDataModel.ToArchiveRelativeStem is wired only to the MAPIFolder overload, so the string overload assigns DestinationOlStem verbatim and an accepted rooted value reaches the boundary throw. The orchestrator confirmed both. Full revert was rejected because it reintroduces CR-1, a real user-facing regression this feature created. Producer-side normalization was deferred because it is a full round spent converting a benign dialog into a marginally nicer benign outcome. Issue #637 covers CR-2 done properly: normalize at SelectRow so SelectedFolderPath is archive-relative before it reaches any guard, closing the half of D1 this feature left open. It also carries the async-void ArchiveRootPath exposure, which the orchestrator verified is reachable through a try/finally with no catch and an async void rethrow, and which was deliberately not folded into the revert because a benign degrade is a behavior decision needing its own tests. Refs #614, #637 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
… remedy Six phases, 33 tasks, MCP-validated. Restores IsValidFilingSelection to the single-argument form rejecting any full Outlook path with no minimum-length rule, keeps IsValidCreationSelection so the CR-1 fix survives, reverts the ActionOkAsync call site, and removes ResolveArchiveRootOrEmpty and RootUnavailableDiagnostic, which the planner confirmed have no production consumer once the call site goes. The composition test is the point of the cycle and its fail-before is real, not asserted. It is added in Phase 1 against the current two-argument guard, where three rooted candidates are accepted and ResolvePaths throws on each, so NotThrow fails with ExpectedExitCode 1. Phase 2 then reverts the signature, which mechanically drops the second argument at every call site while the candidate matrix and assertions stay byte-identical. Verified independently: ResolvePaths calls RequireArchiveRelativeStem before anything touches Globals, and TryResolveDestinationFolder swallows its own exceptions, so with Globals null the contract throw is the only escape path from the pure seam. The two cycle-1 tests asserting rooted acceptance are inverted rather than deleted, keeping the value class pinned while flipping the expectation. AC16 gets its text rewritten to describe the two scope-specific predicates that now exist, with the checkbox cleared at the edit and re-checked only after the toolchain passes. Editing AC text is lifted for AC16 alone. Producer-side normalization and the async-void ArchiveRootPath exposure are deferred to #637 and cited as such. Refs #614, #637 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8iDVuJ3fbrz3VonttZWay
Preflight round 1 on remediation-plan.2026-08-26T22-12.md returned REVISIONS REQUIRED with two blocking findings. Both are verified and fixed. B-1: the EfcSelectionGuardTests.cs baseline was recorded as 317; the measured value is 316. P0-T10 carries a hard STOP on any count disagreeing with the table, so the executor would have halted at task 10 of 33 over an off-by-one. Corrected in global rule 10 and the P0-T10 expected-count list; the other five counts are unchanged. B-2: the P5-T5(b) coverage no-regression gate compared raw filtered rates, which is arithmetically unsatisfiable for a change that deletes fully-covered lines. This cycle removes 14 covered production lines and one uncovered line, so the projection is 53986/63605 = 84.8770% against a baseline of 84.8790% -- a deterministic -0.0020pp miss caused by deletion arithmetic, not measurement noise. The gate would have failed correct work, which is the inverse of the defect class that produced RC-1. Replaced with a deletion-adjusted gate asserting that the denominator did not grow and that no retained line lost coverage; verified satisfiable at 53986 >= 53985. The replacement is strictly more discriminating, not weaker. Also applied four non-blocking preflight items, including a vacuity counter on the composition test so a future guard that rejected every candidate could not leave it green with zero assertions. Plan re-validated: MCP plan validator ok, 6 phases, 33 tasks, LF preserved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The coordinator asked whether the D6 InvalidOperationException from AppOlObjects.ArchiveRootPath can reach the async void boundary via EfcDataModel.cs:288. It can. Verdict: REACHABLE, with every link verified. AppOlObjects.cs:253-267 delegates to ArchiveRootPathGuard, which throws unconditionally at ArchiveRootPathGuard.cs:44 when the archive folder does not resolve and at :56 on a cross-store or renamed archive. The backing field is assigned only on a returning call, so there is no negative caching and the failure repeats on every attempt. EfcDataModel.cs:289 reads the property with no try/catch, and nothing between there and the UI catches: ExecuteMoves.cs:31-46 is try/finally with no catch, which resets state and lets the exception through. EfcFormController.cs:429-443 is async void and rethrows at :441, so it surfaces as an unhandled UI-thread exception. Cycle 1's ResolveArchiveRootOrEmpty at EfcFormController.cs:708 degrades to an empty root, but IsValidFilingSelection returns true for any non-rooted value regardless of the root, so the guard passes and the second unguarded read at EfcDataModel.cs:289 throws anyway. Cycle 1 made the OK path look protected without protecting it. Scope is unchanged. The crash path is pre-existing: present at 0209250, neither created nor removed by cycle 1, and unchanged by the cycle-2 partial revert. That independently corroborates the plan's net-effect claim that no path ends up worse than 0209250. Plan rule 11 deferred the :289 read to #637, which was mis-targeted: #637 covers producer-side rootedness normalization, whereas this is an archive-root resolution failure that fires even for a well-formed archive-relative stem. Opened as #638 instead. The lifecycle tool retained only the Summary section, so Environment, Steps to Reproduce and Expected Behavior were posted as a comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…deleted Preflight round 2 found that plan item D-E scheduled IsValidFilingSelection_SingleSeparatorLeadingSelection_IsRejected for deletion on the premise that it exists only to pin the two-argument semantics this cycle removes. That premise is false. git show cee7897 has the test at line 54 of the original #614 delivery, in exactly the single-argument form this cycle restores; cycle 1 only appended a null second argument and reworded the comment, which cycle 1's own plan records at line 133. Approved scope requires every D1/D4/D9 rejection test to survive, and this is a rooted-rejection test whose behavior survives the revert, so deleting it would have dropped regression coverage while looking like cleanup. The genuine cycle-1 addition is IsValidFilingSelection_RootedTargetWithUnavailableRoot_IsRejected, absent at cee7897. Both currently assert the same rooted value is rejected, one passing null and one passing string.Empty, so once the second argument drops they are byte-identical. Deleting the newer one and keeping the pre-existing one is the correct de-duplication. That change propagates through the suite arithmetic: the cycle now removes 5 tests rather than 6, EfcSelectionGuardTests lands at 25 rather than 24, and the net delta is minus 1 (6586) rather than minus 2 (6585). Rule 6, D-F, P2-T5 and P5-T4 all agree; a stale total here would have fired the post-change total gate against correct work. Also applied three non-blocking corrections: the deletion-adjusted coverage gate prose no longer claims more than the arithmetic delivers (the line gate carries one line of slack because one deleted line was uncovered; the branch gate is exact), the GetStem branch assertion no longer pins to a single line of a ternary spanning six, and the Issue609 test count is corrected from three to six. Finally aligned item D-D, which still said both referencing comments are removed while P2-T3(d) now rewords one of them. The token still reaches zero hits either way, so no gate changed, but the two statements disagreed about a real edit. Plan re-validated: MCP plan validator ok, 6 phases, 33 tasks, LF preserved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ine gate real
Preflight round 3 found two blocking defects.
F5-1: P4-T1's three full-assembly runs carried no /TestCaseFilter, while
Invoke-MSTestWithCoverage.ps1:76 appends TestCategory!=LiveOutlook to the P0-T9
baseline and the P5-T4 final run. TaskMaster.Test contains exactly one
[TestCategory("LiveOutlook")] test, LiveHookup_OnSta_CompletesAndDoesNot
BlockStaBeyondThreshold, which constructs a real Outlook.Application and polls a
live store with a 120000 ms ceiling. Outlook.Application is registered on this
machine, so the construction-scoped skip would not have fired. Beyond running an
external process in a test suite, which policy prohibits, P4-T1 asserts zero new
failures against a baseline that excluded the test, so any live failure would
have been attributed to the revert. All three commands now carry the filter, and
rule 5 records that any run compared against a baseline figure must exclude the
category.
F5-2: the retained-line check added in the previous commit could not fail. It
verified the claim against the section-(d) listing, but (d) is scoped to changed
lines in one file, so it contains no retained lines and returned clean whichever
retained line regressed. That replaced a gate that could not pass with one that
could not fail. Section (e) now derives D_valid and D_covered as net per-file
deltas for the two edited files, read from the Cobertura files rather than
counted by hand, and asserts that the global movement equals them: equality
proves the edited files account for all of it, leaving none for a retained line.
A shortfall of N means N retained lines regressed. An excess is a gain, not a
regression, and passes with its source recorded, because the RC-4 test newly
covers the GetStem out-of-ancestor arm in the retained EmailFilerConfig.cs.
Since an equal and opposite gain could mask a single-line loss, any retained
file with a negative per-file lines-covered delta fails regardless of whether
the identities balance.
The hand-count and exact-equality forms in the routed text would both have
failed on correct work; the planner caught that and is right. Also applied four
non-blocking corrections and marked an unreconciled illustrative figure in
section (b) as not gated, since section (e) now forbids hand-derived counts.
Plan re-validated: MCP plan validator ok, 6 phases, 33 tasks, LF preserved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e quantity Preflight round 4 found two blocking defects in P5-T5 section (e). Both are repaired here by the orchestrator rather than routed to the planner: the direction was supplied by round 4, and three of this gate's five prior versions were orchestrator-authored, so a fifth planner round would have continued the loop rather than ended it. F6-1: the per-file negative-delta clause could not pass. lines-covered is nondeterministic. Parsing the two same-tree measurements p9-t4 and p0-t9r directly shows an identical 550-file set and zero per-file lines-valid disagreements, yet three files disagree on lines-covered: EfcHomeController.cs +1, PropertyStore.cs +4, SubjectMapSco.Orchestration.cs -4. A gate failing on any negative per-file delta therefore fails on correct work and emits the same signal for ambient drift as for a real one-line regression. Section (e) is now four sub-gates. E1 hard-gates only lines-valid, which the same comparison proves deterministic. E2 demotes the aggregate shortfall and every per-file negative delta to a candidate. E3 requires a candidate to reproduce in the same direction across two independent post-change measurements before it can fail, with the per-filename comparison authoritative. E4 fails on a confirmed negative delta, exempting only the two files already carried by a hard 100% gate in section (c). No tolerance band was introduced; reproduction, not a threshold, separates drift from regression. The gate fails closed, so the reproduction assumption can produce a false failure but never a false pass. F6-2: section (e) did not state the per-file counting reduction. Cobertura repeats every line under both ./lines and ./methods/method/lines, so a naive count is 1.65x to 2.00x high (EfcSelectionGuard.cs 31/31 deduplicated vs 62/62 naive; EfcFormController.cs 717/81 vs 1185/151), which breaks the identities. The deduplication rule is now mandatory and stated explicitly, with a self-check that the deduplicated lines-valid sum must equal the root attribute before any identity is evaluated. That reduction reproduces the root attributes exactly on all four filtered files. This gate has now been wrong in five successive versions, twice in the cannot- pass direction and twice in the cannot-fail direction. The recorded root cause is that each version asserted a property of the measurement that had not been measured, so every premise behind this revision was verified by parsing the Cobertura files before the text was written. N6-1: section (d) corrected. The filtered Cobertura does carry per-line entries at 701-703, 705, 708-715, 718-720; the cycle-1 "only line 709" claim was a misreading and is recorded as not to be reproduced. N6-2: section (b) corrected from one line of slack to four (EfcFormController.cs measures 717/81 at the cycle-1 head against 713/81 before it). Plan validator returns ok:true. Structure unchanged at 6 phases, 33 tasks, 0 CR bytes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Preserve real-environment fail-fast behavior for existing production constructors - Adapt eight affected test files to use deterministic in-memory OneDrive roots - Record red-green evidence, full QA results, and the accepted AC24 wording risk Refs: #614
- Add exact-head policy, code, and feature review artifacts - Record passing QA and zero blocking findings across all three reviews - Preserve the accepted documentation risks and partial AC24 disclosure Refs: #614
drmoisan
deleted the
bug/efc-store-root-selection-leaks-full-outlook-path-into-filing-boundary-614
branch
August 28, 2026 11:56
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
1 participant
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.
Summary
ApplicationGlobalsenvironment-reader seam and deterministic updates to eight test files.PARTIAL - ACCEPTED RISKstatus.Why
A full Outlook hierarchy path could be stored as
EmailFilerConfig.DestinationOlStem, even though that value must be archive-relative. When the mailbox store root reachedFolderConverter.ToFsFolderpath, filing either failed with anArgumentExceptionor risked deriving an incorrect destination.Removing
.from the invalid-character set would address only the observed exception and could permit silent misfiling. This change instead enforces the path-representation boundary at the producer, filing consumer, and secondary producer while validating only filesystem segments derived by the converter.What Changed
Core behavior and architecture
ArchiveStemContractto identify full Outlook paths, derive archive-relative stems, and reject invalid filing-boundary values.EmailFilerConfig.ResolvePathsoverloads.EfcDataModelstem derivation through the shared contract.EfcSelectionGuard.FolderConverterwith anchored, case-insensitive ancestor handling and per-segment Windows filename validation.Remediation cycles
Func<string, string>environment-reader seam toApplicationGlobals. Existing runtime constructors retain the default environment reader, while twelve affected test construction paths across eight test files now inject deterministic in-memory OneDrive values.Tests
ApplicationGlobalsconstructor seam.Documentation and review evidence
Architecture / How It Fits Together
The corrected path flow is:
ArchiveStemContract.EmailFilerConfigindependently enforces the archive-relative invariant before constructing a destination path.FolderConverterstrips the Outlook ancestor using anchored, case-insensitive matching and validates only the derived filesystem segments.ArchiveRootPathGuardand OneDrive-root resolution fail explicitly with redacted diagnostics when required roots cannot be resolved.For initialization,
ApplicationGlobalsnow optionally passes an injected environment reader toAppFileSystemFolderPaths. Production constructors continue using the real process environment and retain the explicit OneDrive failure contract. Tests use fixed in-memory values so unrelated initialization tests remain deterministic on hosted runners.Verification
Completed
mainthrough branch heade33ec43c31c4f3f4f77cf361092f3eb5089c02d5.eaf29fb1b1341a0217e5feb4759cd22fd1deb8d6; the review artifacts were then committed at the canonical branch head.System.Reactivepackage warnings.53,986 / 63,603).12,751 / 16,168).Required before completion
Backward Compatibility / Migration Notes
OneDriveCommercial,OneDrive, andOneDrivePersonalresolution remains in use..,[, or]are now accepted when otherwise valid.askparameter was removed fromFolderConverter.ToFsFolderpath; in-repository callers did not pass it explicitly.Risks and Mitigations
UtilitiesCS,QuickFiler, andTaskMaster. Full-solution builds, the complete 6,587-test suite, interaction regressions, and 100% changed-method line coverage mitigate this scope.PARTIAL - ACCEPTED RISK. The authoritative formatter, analyzer, nullable, and canonical coverage runner passed, but AC24's literal directvstest.console.exe ... /EnableCodeCoveragecommand and evidence-path wording were not fully satisfied.FAIL; this PR does not relabel that row as passing. The authoritative 6,587-of-6,587 result is separate exact-head review evidence.#637where#638was intended.Review Guide
EmailFilerConfig.DestinationOlStemis archive-relative.ArchiveStemContract,EfcSelectionGuard, andArchiveRootPathGuardas the primary invariant boundaries.FolderConverterancestor stripping and per-segment validation, especially dotted/bracketed roots, UNC paths, repeated substrings, reserved names, and trailing spaces or dots.ApplicationGlobalsthree-argument constructor and confirm that existing one- and two-argument constructors retain real-environment behavior.Follow-ups
GitHub Auto-close
Closes #614
Related issues / PRs