Skip to content

fix(quickfiler): rename the EFC banner-rejection prefix and correct the stale SelectedFolder comment (#662) - #721

Merged
drmoisan merged 25 commits into
mainfrom
bug/efcselectionguard-banner-prefix-arity-and-stale-comment-662
Sep 1, 2026
Merged

fix(quickfiler): rename the EFC banner-rejection prefix and correct the stale SelectedFolder comment (#662)#721
drmoisan merged 25 commits into
mainfrom
bug/efcselectionguard-banner-prefix-arity-and-stale-comment-662

Conversation

@drmoisan

@drmoisan drmoisan commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Summary

Resolves the banner-prefix arity divergence and the stale SelectedFolder comment described in issue #662.

Three BannerPrefix constants existed under one name carrying two different values: EfcSelectionGuard declared a three-character "===", while both row producers (BreadcrumbRowBuilder and FolderSuggestionTree) declared a four-character "====". After this change there are two declarations rather than three: one producer constant shared by both producers, and one deliberately broader classifier constant that is renamed, documented, and test-pinned as broader.

This is a consistency and comment-accuracy fix. It is explicitly not a behavioural repair, and no predicate's return value changes for any input.

The direction this fix deliberately does not take

The issue reads as though the two arities should be unified upward to four characters. That reading is wrong and would be a behavioural regression, so it is worth stating plainly for reviewers:

  • EfcFormController.IsBannerRow classifies by the producers' four-character constant, so IsBannerRow("===") is false.
  • IsValidCreationSelection tests value.Length >= MinimumCreationLength, and MinimumCreationLength is 3, so for the input "===" that comparison is 3 >= 3 and the length rule rejects nothing.
  • The guard's three-character prefix is therefore the only mechanism rejecting a three-equals row at either EFC classification site.

Widening the guard to "====" would make IsValidFilingSelection("===") and IsSelectableFolder("===") both return true. The guard's value is unchanged; only its name and its documentation change.

One detail is worth flagging because it is the kind of thing a future reviewer would get wrong. In the pre-existing merged test IsSelectableFolder_AndIsBannerRow_ClassifyThreeAndFourEqualsRowsIdentically, the assertion that reads like the consistency guard — creationPath.Should().Be(filingPath) — still passes under the prohibited widening edit, because both sides become true together. Only the sibling creationPath.Should().BeFalse(...) line catches the relaxation. This was verified directly against the dispatch path rather than inferred.

Reachability

The underlying defect is latent, not live. No producer emits a three-character row today, so no user-visible misbehaviour was reachable before this change, and none is fixed by it. The cost was entirely to the next maintainer: the inaccurate comment invited a contributor to "correct" the guard in the dangerous direction. This PR is therefore a maintainability change, and it should be weighted as one.

What changed

Four files, 57 insertions and 9 deletions:

File Change
QuickFiler/Controllers/EfcSelectionGuard.cs Constant renamed to BannerRejectionPrefix, value unchanged; both StartsWith call sites updated; XML doc rewritten to state that it is deliberately a proper prefix of the producers' constant and must not be widened
QuickFiler/Controllers/EfcFormController.cs SelectedFolder comment replaced so it describes the composition the code implements. Comment-only; no executable line changed
UtilitiesCS/OutlookObjects/Folder/FolderSuggestionTree.cs Duplicate constant deleted; its single reader now references BreadcrumbRowBuilder.BannerPrefix directly
QuickFiler.Test/Controllers/EfcSelectionGuardTests.cs One [TestMethod] added, pinning that both arities are rejected on both predicates, with a because message naming the prohibited direction

The constant is deleted rather than re-aliased because an aliasing declaration would still match the declaration inventory and defeat the criterion it is meant to satisfy.

Verification

All ten acceptance criteria pass and are checked off in issue.md. Feature review reports zero blocking findings across the policy audit, code review, and feature audit.

  • Toolchain clean in one pass in order, using the exact CLAUDE.md commands with /t:Rebuild and without /p:Nullable=enable.
  • Tests: 6070 passed, 0 failed. QuickFiler.Test moved 1286 to 1287 passed, the single added test; UtilitiesCS.Test unchanged at 4783.
  • Coverage: 85.3741% line, 79.3761% branch, both above the repository floors. Changed-code coverage is 3/3 statements.
  • Counters above were read from the TRX <Counters/> elements in the committed evidence rather than from console summaries.

Defects found and handled during delivery

  1. Stale diff anchor in three plan gates. The plan pinned a base SHA that is an ancestor of both this branch and origin/main, so the two-dot git diff form those gates use reported everything origin/main had accumulated since that commit alongside this branch's own work. Measured before any edit existed, the scope gate returned 22 paths against an asserted union of 4. The anchor was replaced with git merge-base origin/main HEAD, resolved at run time; the same gate then returns empty pre-edit.

  2. Committed TRX evidence was not well-formed XML. The artifact-hygiene sweep substitutes angle-bracket placeholders, and vstest writes those redacted values into XML attribute values, where a raw angle bracket is illegal. All six committed TRX files failed to parse. They were repaired by escaping the placeholders, which leaves the redaction identical and restores parseability. The hygiene gate did not catch this because it asserts only that host identifiers were removed, never that the rewritten file still parses.

Note on the generated PR-context bundle

Two fields in artifacts/pr_context.summary.txt are wrong and were not relied on:

  • It reports "GitHub CLI unavailable: GitHub CLI (gh) is not installed." The CLI is installed and authenticated; this was verified directly.
  • Its author-asserted auto-close list contains #465, #498, #662 and #ISO-8601. #ISO-8601 is not an issue number at all but a token scraped from evidence-convention prose, and #465 and #498 are unrelated issues cited as precedent in the requirements, both already closed. Each was checked individually with gh. Only #662 is in scope for this PR.

Base, head, and merge-base in the bundle were correct.

Follow-ups recorded, not filed

This item carries footprint acceptance criteria restricting the branch diff, so no follow-up issue was opened from this branch. These are recorded in the audit artifacts for consolidated filing after merge:

  1. The acceptance-criterion text for AC5b and AC7 still quotes the stale anchor SHA. Both commands return empty today, but only because origin/main has not touched either protected file since that commit.
  2. The artifact-hygiene rule should escape its placeholders for XML targets, and should assert that rewritten XML still parses.
  3. The hygiene sweep excludes the plan file from its own residual scan.
  4. FolderSuggestionTree.IsBanner's doc comment still hard-codes the four-character literal rather than naming the constant.
  5. EfcFormController.cs is 1189 lines and at 25.5% line coverage. Pre-existing, unchanged by this PR, and inside the ratified Outlook Interop exemption class; recorded as a FAIL row in the audit rather than silently passed.
  6. The committed Cobertura and TRX evidence adds substantial volume to history. Unlike a prior item, these files are committed and retained, so there is no unreachable-blob concern and squash-merge is not a remedy. This one is merge-method-dependent and is stated as such.

GitHub Auto-close

Emitted from the author-asserted autoclose list after verifying the number directly with gh. The other three tokens on that list were checked and rejected as described above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL

drmoisan and others added 25 commits August 31, 2026 20:20
Create the active feature folder for issue 662 and author issue.md with
the content the bug issue template does not emit: an explicit Acceptance
Criteria section, the directional constraint on the remedy, and the
derived occurrence baseline.

The issue's Expected Behavior asks for one banner-prefix arity across
producers and classifiers. Unifying upward is a behavioural relaxation:
EfcSelectionGuard's three-character prefix is the only mechanism that
rejects a three-equals row at either EFC classification site, because
IsBannerRow classifies by the producers' four-character constant. The
merged test IsSelectableFolder_AndIsBannerRow_ClassifyThreeAndFourEquals
RowsIdentically asserts both rows are rejected on both paths. The remedy
therefore preserves the guard's breadth, renames its constant, corrects
the stale comment, and dedupes the producers' literal.

Occurrence counts in the acceptance criteria are scoped to a '*.cs'
pathspec. An unscoped search for the anchored four-character declaration
also matches two historical audit records under a closed feature folder,
so the unscoped count cannot reach the asserted value.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Record the research findings for issue 662. All eight orchestrator
hypotheses were independently confirmed, including the directional
constraint: widening EfcSelectionGuard's prefix to four characters makes
IsValidCreationSelection and IsValidFilingSelection accept a three-equals
row, and EfcFormControllerTests.cs:463 then fails.

Research surfaced two defects in the acceptance criteria as first
written, both corrected here.

AC2 and AC5 were mutually unsatisfiable. AC5 as written allowed
FolderSuggestionTree to keep an aliasing constant, but such a line still
matches AC2's declaration regex, so AC2's asserted count of one would
have been two. AC5 now requires deletion of the constant with a direct
qualified reference from its single reader.

AC2's second verification counted every occurrence of the new name in
the guard file, which AC3's doc-comment wording could perturb. It now
counts the two StartsWith call sites only.

Also: the unscoped occurrence figure is scope-dependent and grows as this
feature's own documents are authored, so no absolute unscoped count is
asserted. Diff anchors moved from origin/main to the pinned base commit
so the assertions cannot drift when main advances. AC5b added to record
that BreadcrumbRowBuilder.cs is read-only to this work, because feature
498's criteria assert that file is unmodified.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Three phases, 52 tasks. Replaces the generic seven-phase scaffold, which
used bold phase text rather than the contract's heading form and cited
Python test paths.

Records the directional constraint as decision D1: FolderSuggestionTree's
constant is deleted rather than re-aliased, because an aliasing
declaration still matches AC2's declaration regex and would make that
count two instead of one.

Not yet validated or preflighted.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Nine blocking and six non-blocking defects, enumerated in one review pass
with a complete per-task delta. Signal was REVISIONS REQUIRED with
CONVERGENCE: NO FURTHER ROUNDS EXPECTED.

The three most consequential: the msbuild and vstest variables are
resolved in a prose block but consumed in per-task command spans, and
shell state does not persist between tasks; the coverage-enabled test
runs name the scripts/vscode CLI runsettings, which carries MSTest
parallelization only and no coverage collector, so they would instrument
the Deedle and FSharp modules the repository maintains two exclusion
lists to keep out; and P1-T1 mandated a delegation the executor has no
tool to perform.

Recorded before the plan revision so the defects found before execution
are distinguishable from any found during it.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Applies all 20 delta items plus the two tasks B1 named but the per-task
list omitted (P0-T8, P0-T9 also consume the msbuild variable).

The planner rejected five delta items as written and corrected them:

- P2-T10 gave pre-change line numbers labelled as post-format. All three
  shift: the new multi-line XML doc sits above the guard call sites, the
  deleted declaration sits above the FolderSuggestionTree reader, and
  CSharpier wraps the 132-character rewritten reader. Replaced with an
  enclosing-member identification rule that records the resolved numbers.
- P2-T23's '*.xml' pathspec matches the plan's own committed Cobertura
  evidence, so the union could never equal the four in-scope files. The
  feature folder is now excluded from both pathspecs.
- Three 'replace the first sentence' items subsumed the sentence after
  them, which would have left duplicated or contradictory text.
- Bringing the test steps inside the Phase 2 restart loop left the loop
  untriggerable, because the test tasks' acceptance always holds. Each
  now restarts only when its failed count exceeds the Phase 0 baseline
  for the same assembly.

Still 3 phases and 52 tasks; no task added, removed, or renumbered.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
…sue 662

Includes a size-hook-required compaction of the shared MEMORY.md index.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
All 15 round-1 defects confirmed closed. Four new blocking and six
non-blocking defects found in one pass covering all 52 tasks.

D1 is the consequential one: the six vstest runs each write a TRX, and
.gitignore covers *.coverage and *.coveragexml but not *.trx. A TRX
carries the account and machine identifiers in runUser, computerName and
runDeploymentRoot, and the worktree root in the storage attribute of
every unit-test element. The two Cobertura copies carry an absolute
filename per class node whenever the document is the raw pre-processed
one, which the plan treats as an ordinary state. The git add spans would
have staged all of it.

D2 through D4 concern evidence that cannot fail: a loop restart can leave
the previous pass's TRX in place for the reading tasks to report as the
final pass's; the class-node comparison is undefined when a filename
carries more than one node, leaving the executor to choose the evidence
it is judged against; and the changed-code line rule resolves one of the
three statements to the minimum-length comparison rather than to the
renamed call site.

Convergence is FURTHER ROUNDS LIKELY, so a confirming round follows.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
All ten items D1 through D10 applied. Every underlying delta fact was
re-verified against the tree and none was wrong; .gitignore does carry
*.coverage and *.coveragexml and does not carry *.trx, which is D1's
premise.

Two supplied items were defective as written and were applied with a
reported correction rather than silently rewritten:

- D1's verification search covered the whole feature folder, but the plan
  file itself legitimately carries the worktree root in its Working
  Directory section, so the gate could never pass. The plan file is now
  excluded from the search and the rewrite scope is filtered by
  git check-ignore so the sweep cannot corrupt the binary .coverage file
  that /EnableCodeCoverage writes into the results directories.
- D10 recorded the agent-memory status into P2-T23's own artifact after
  the main commit, but that artifact sits inside the pathspec of the same
  task's 'returns no output' status gate, so the acceptance could never
  hold. An ordering clause now stages it correctly.

The planner also harmonized two sentences in P2-T10 that sat outside the
replaced span and still used the single-line-number convention D4 exists
to remove.

Still 3 phases and 52 tasks; no task added, removed, or renumbered.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
The round-2 delta added an artifact-hygiene rule forbidding an absolute
host path, an account name, or a machine name in any file this plan
commits. The plan's own Working Directory section carried the absolute
worktree root, including the account name, so the plan contradicted the
rule it now enforces and the rule's verification search could not pass
over the plan file.

The path is replaced with a reference to the worktree root the
delegating orchestrator supplies. The executor takes its working
directory from the delegation prompt, not from this file, so nothing
load-bearing is lost.

Twenty-six other committed plans in this repository carry the same
absolute path, so this is a pre-existing repository-wide condition rather
than something introduced here. Only this plan is changed; the others are
out of scope for issue 662.

The account name now appears nowhere in the feature folder. Structure is
unchanged at 3 phases and 52 tasks, line endings remain LF, and the plan
validator passes.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
PREFLIGHT: ALL CLEAR with CONVERGENCE: NO FURTHER ROUNDS EXPECTED, over
all 52 tasks and every prose region, with every citation and count
re-derived against the tree.

All ten round-2 defects closed and all four planner deviations accepted.
The reviewer confirmed the two gates that matter most are falsifiable
rather than pre-satisfied: the AC5b diff currently prints nothing at exit
0 and the P2-T23 scope diff is currently empty, so both can still fail.

Seven non-blocking observations recorded rather than remediated. The
notable one is that the plan-file carve-out D1 introduced is now dead
text, because the absolute host path it was written to accommodate was
removed from the plan afterwards. Excluding a file that cannot match
changes no outcome, so it was left rather than spending a fourth round.

Three rounds against a two-round target. The overrun cause on record is
that the planner substituted its own wording for supplied delta items,
leaving that text unreviewed until the next round; supplying verbatim
replacement text and requiring an explicit per-item disposition is what
closed the loop.

Refs #662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Two reusable findings. First, .gitignore covers *.coverage and
*.coveragexml but not *.trx, so a committed TRX leaks the account name,
machine name and worktree root, the last once per test in the storage
attribute. Second, preflight round overrun on this item came from the
planner paraphrasing supplied delta items rather than from the review;
supplying verbatim replacement text and demanding a per-item disposition
is what converged it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
…-banner-prefix-arity-and-stale-comment-662

# Conflicts:
#	.claude/agent-memory/atomic-planner/MEMORY.md
#	.claude/agent-memory/task-researcher/MEMORY.md
The plan pinned 2b85134 as the diff anchor for P2-T16, P2-T18 and
P2-T23. That commit is an ancestor of both this branch and origin/main,
so the two-dot diff form those gates use reports origin/main's own
accumulated changes alongside this branch's work. Measured before any
plan task ran and before any edit, P2-T23's listing returned 22 paths
against an asserted union of 4.

Replace the anchor for those three tasks with the merge base of
origin/main and HEAD, resolved at run time. The same listing returns
empty pre-edit under the corrected anchor.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Captures the Phase 0 baseline for issue 662: policy reads, base-commit
resolution with the execution amendment, SDK/tool/package bootstrap,
read-only CSharpier baselines, the two msbuild gate baselines, both
full-assembly test baselines, the numeric coverage baseline, and the
pre-change occurrence counts.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
…stale comment

Phase 1 of issue 662. The guard's constant keeps the value "===" exactly; only
its name and its XML documentation change.

- EfcSelectionGuard: rename BannerPrefix to BannerRejectionPrefix, update both
  StartsWith call sites, and rewrite the XML doc to record that the value is
  deliberately a proper prefix of BreadcrumbRowBuilder.BannerPrefix, that it
  therefore rejects a strict superset of the producers' banner rows, and that it
  must not be widened.
- EfcFormController: replace the SelectedFolder comment, which claimed
  IsValidSelection keeps a four-character rejection, with the composition the
  code implements. Comment only.
- FolderSuggestionTree: delete the duplicated four-character declaration and
  read BreadcrumbRowBuilder.BannerPrefix directly, so the two producers share
  one declaration. Deletion rather than aliasing, per Decisions Record D1.
- EfcSelectionGuardTests: add one regression test pinning that both arities are
  rejected on both predicates, with a because message naming the prohibited
  direction.

Behaviour-preserving at every call site; the fail-before exception dossier
records why no failing run exists.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Records the final clean Phase 2 loop pass: CSharpier format (no rewrite) and
check, both msbuild gates, the two scoped AC6/AC7 runs, and both full-assembly
runs. Includes the CSharpier wrap of the FolderSuggestionTree IsBanner reader,
which P1-T6 predicted and which triggered the single loop restart.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
… evidence

Completes Phase 2 for issue 662. All ten acceptance criteria in issue.md are
verified and checked off, each with its own verification artifact.

- Per-criterion verification artifacts for AC1 through AC9.
- Post-change coverage capture and the baseline-vs-post-change delta: changed-code
  coverage 3/3, and neither touched file's class line-rate falls.
- Issue acceptance-criteria mirror and the AC status summary.
- Scope confirmation: the union of the anchored diff and the porcelain status is
  exactly the four in-scope files.

AC5b and AC7 were verified against the merge base of origin/main and HEAD, per
the plan's Execution Amendment, because the plan's pinned anchor is an ancestor
of both refs and its two-dot diff would have reported unrelated origin/main work.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds two atomic-executor memories from the issue 662 run:
- a full-assembly test run can hang although the byte-identical Phase 0 baseline
  command passed; sample testhost CPU to distinguish hang from slow, and diff the
  failing set against the baseline before calling it a regression.
- never write an observed value into an evidence artifact before observing it.

Compacts the memory index from 161 to 102 lines by pairing related entries onto
single lines, as a repository hook requires it below its read limit. Every
pointer is preserved.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
The artifact-hygiene sweep substitutes angle-bracket placeholder tokens,
and vstest writes the redacted values into XML attribute values in the
TestRun header. A raw angle bracket is not permitted there, so all six
committed TRX files were left not well-formed and failed to parse at
line 2.

Escape the placeholder tokens inside the TRX files only. The parsed
attribute value is still exactly the placeholder, so the redaction is
unchanged and the sweep's zero-residual result still holds, while the
documents parse again.

The hygiene gate did not catch this because it asserts only that the
identifiers were removed, never that the rewritten file is still
parseable.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Policy audit, code review and feature audit for the branch. Zero
blocking findings; all ten acceptance criteria re-derived and passing.

One coverage row is recorded FAIL rather than PASS: EfcFormController.cs
sits at 25.5 percent line coverage against the uniform per-file floor.
It is dispositioned non-blocking because the file's diff contains no
executable line, its coverage counters are byte-identical to baseline,
and it falls in the ratified Outlook Interop exemption class. Recording
it as passing would have been inaccurate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds the sibling-assertion blind spot found on this branch: the
assertion that reads like the consistency guard still passes under the
prohibited widening edit, and only the BeFalse line catches it.

Extends the shared host-path note with the XML-escaping caveat, since a
zero-residual sweep result is not sufficient evidence that the rewritten
artifact is still parseable.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
@drmoisan
drmoisan merged commit 9ca9e99 into main Sep 1, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: efcselectionguard-banner-prefix-arity-and-stale-comment

1 participant