fix: filter_prefixes regex grouping + guard filt_iter's added __len__ - #105
Merged
Merged
Conversation
…en__ Two of the three "related, same area" bugs from #82 (the main prefix-relativization corruption bug is a larger design question, left open -- see issue comment): - dol.trans.filter_prefixes built an unanchored/ungrouped regex ("^" + "|".join(...)) which for multiple prefixes like ['logs/', 'tmp/'] compiled to `^logs/|tmp/`, parsed as `(^logs/)|(tmp/)` -- so a string merely *containing* a later prefix anywhere (e.g. 'zzz/tmp/c') incorrectly matched. Grouped as `^(?:...)` to fix, matching how filter_suffixes already groups its own alternation. - dol.trans._filt_iter unconditionally added a counting __len__ to any wrapped class, even one that deliberately omits __len__. Now guarded with hasattr. Verified (and confirmed by independent review) this guard is currently a no-op on every path reachable through the public filt_iter() API, since the default wrapper=Store already provides Collection's own __len__ before _filt_iter runs, and filt_iter doesn't expose a wrapper= parameter -- so it's inert today but correct/defensive for any future or private caller of _wrap_store/_filt_iter with a custom wrapper. Reviewed by an independent sub-agent before merge (per crowsnest policy): confirmed both fixes correct, no regressions, found no other instance of the ungrouped-alternation bug elsewhere in the codebase. Refs #82 (partial) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Fixes two of the three "related, same area" bugs reported in #82 (the main prefix-relativization corruption bug -- unguarded slicing on
mk_relative_path_store/KeyCodecs.prefixed/prefixless_view-- is a larger design question left open; see comment on #82).filter_prefixesregex grouping:"^" + "|".join(map(re.escape, prefixes))compiled to e.g.^logs/|tmp/for['logs/', 'tmp/'], which regex parses as(^logs/)|(tmp/)-- so'zzz/tmp/c'incorrectly matched (only the first prefix was actually anchored to string-start). Grouped as^(?:...), matching howfilter_suffixesright above it already groups its own alternation._filt_iter's unconditional__len__: guarded withhasattr(store_cls, "__len__")before adding a counting__len__, so a class that deliberately omits__len__stays that way when wrapped.Review
Independent sub-agent review (per crowsnest policy) confirmed both fixes correct with no regressions, and found no other instance of the ungrouped-alternation bug elsewhere in the codebase. It also found that fix #2 is currently a no-op on every path reachable through the public
filt_iter()API --Store(the hardcoded default wrapper) already providesCollection's own__len__before_filt_iterruns, andfilt_iterdoesn't expose awrapper=parameter to override that. So today it's inert but correct/defensive for any future or private caller of_wrap_store/_filt_iterwith a custom wrapper. Keeping it since it's harmless and matches the issue's explicit ask.Test plan
pytest --doctest-modules dol/trans.py-> 47 passedpytest(full suite) -> 592 passed, 3 skipped🤖 Generated with Claude Code
https://claude.ai/code/session_011HSBVhDjRU4apSLcRkavv9