fix: allow pathspec 1.x alongside 0.x - #1142
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
685ef46 to
534b8ad
Compare
|
Context from Canva: |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes ignore-matching behavior used for directory sync and container image uploads (a change that, if wrong, could cause files to be unexpectedly included or excluded), a human look at the version-branching logic would still be worthwhile.
What was reviewed:
- Verified locally that pathspec 1.1.1 supports the
backend="simple"kwarg used to pin the stdlib regex engine, and that the new tests (test_sync.py,test_container.py) pass under the installed pathspec 1.1.1 — 85/85, matching the PR's reported local run. - Confirmed the
DeprecationWarningfor thegitwildmatchalias does fire on pathspec 1.1.1 outside the suppression block, and is correctly swallowed inside_build_ignore_spec/get_regex_patterns, so the no-warnings-leaked tests are meaningful rather than vacuous. - Checked the version gate (
packaging.version.Version(pathspec.__version__).major >= 1) and the pyproject exclusion range against the stated crash-bug versions (1.0.0-1.0.3) and Python-version split reasoning — consistent with the PR description. - Ruled out the two candidate issues the bug hunting system raised about
warnings.catch_warnings()thread-safety and duplicated suppression boilerplate insync.py— not real bugs for this code path (single-threaded, sequential CLI usage; the duplication is small and intentional per-module).
Extended reasoning...
Overview
This PR widens the pathspec dependency constraint from <1 to <2 (excluding the buggy 1.0.0-1.0.3 releases) so fal can co-install with tools like black 26 that require pathspec 1.x. To keep behavior stable across both pathspec majors, it adds a version-gated _build_ignore_spec helper in fal/sync.py that pins the stdlib regex backend on 1.x (avoiding nondeterministic re2/hyperscan auto-selection) and hoists PathSpec construction out of the per-file loop into a per-walk build. fal/container.py gets a matching warnings.catch_warnings() wrapper around the same deprecated gitwildmatch code path used for dockerignore regex generation. Two new test files assert concrete match/no-match semantics and absence of leaked warnings.
Security risks
No new security-sensitive surface (no auth, crypto, or permissions code touched). The main risk class is correctness: if the pathspec-1.x compatibility shim behaves differently than intended, files could be unexpectedly included in or excluded from directory-sync uploads or container build context uploads. This is a data-integrity/correctness risk rather than a classic injection/auth vulnerability.
Level of scrutiny
This sits between a mechanical dependency bump and a logic change: the pyproject.toml edit itself is simple and well-justified, but the accompanying code in sync.py/container.py introduces version-branching behavior for a third-party library across two incompatible majors (different regex backends, a soon-deprecated pattern alias). That is exactly the kind of dual-behavior compatibility code where a subtle mismatch could silently ship wrong ignore results in only one of the two pathspec majors, and a human reviewer familiar with the sync/upload path is well positioned to sanity-check the version-gate logic and the upper bound's future-safety.
Other factors
The PR author's own testing is credible and I independently reproduced it: pathspec 1.1.1 is the version actually installed in this environment, the new tests pass 85/85, and the DeprecationWarning firing/suppression behavior is exactly as described. No bugs were surfaced by the automated bug-hunting pass, and the two ruled-out candidates were not compelling (thread-safety of catch_warnings doesn't matter for this CLI's sequential upload path; the boilerplate duplication across two files is a minor style nit, not a defect). Given the medium-risk label the PR's own summary already carries and the behavioral (not just declarative) nature of the change, I'm deferring for a human pass rather than approving outright.
534b8ad to
2f25c72
Compare
Widen the pathspec constraint to <2 so fal co-installs with black 26 (requires pathspec>=1.0.0). Excludes 1.0.0-1.0.3, which each ship a crash bug fixed by 1.0.4. On 1.x, pin the simple regex backend for sync ignore matching so results do not depend on re2/hyperscan being importable, build the spec once per directory walk, and suppress the deprecated-alias DeprecationWarnings at both call sites. Adds matching-behavior tests that exercise both pathspec majors in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2f25c72 to
b2f25bc
Compare
SERV-1654
Problem
falpinspathspec>=0.11.1,<1, and black 26 requirespathspec>=1.0.0, so users upgrading black cannot co-install fal without a resolver override.Fix
Widen the constraint to
<2, excluding 1.0.0-1.0.3 (each ships a crash bug, all fixed by 1.0.4; issue links in the inline comment). Environments where something else still pinspathspec<1keep resolving 0.12.x unchanged.Because 1.x auto-selects a regex backend (re2 or hyperscan when importable), sync ignore matching now pins the stdlib
rebackend so results never depend on what else is installed in the user's environment. The gitignore spec is also built once per directory walk instead of per file, and the deprecatedgitwildmatchalias stays (it is the only spelling with identical semantics on both majors) with its DeprecationWarnings suppressed, so nothing leaks into user warnings-as-errors setups. Migrating to the 1.x API is queued behind dropping pathspec 0.x from the range.Tests
New
test_sync.pyplus a container test assert actual match/no-match behavior (negation, dir patterns, anchoring,**, empty gitignore, no leaked warnings). Verified locally on both 0.12.1 and 1.1.1:The
docker_ignoreregex text differs across majors but its only consumer is client-sidere.searchtruthiness infile_sync.py, where the forms are equivalent.🤖 Generated with Claude Code
Note
Medium Risk
Changes which local files are excluded during
sync_dirzips and dependency resolution; backend pinning reduces but does not eliminate semantic risk across pathspec versions.Overview
Widens the
pathspecdependency sofalcan install alongside tools (e.g. black 26) that require 1.x, while still allowing 0.12.x when other pins keep<1. Broken releases1.0.0–1.0.3are excluded.Directory sync now builds a single gitignore
PathSpecper zip instead of recreating it for every file. On pathspec 1.x it forces the stdlibsimplebackend so ignore results do not vary with optionalre2/hyperscan. The deprecatedgitwildmatchspelling is kept for cross-major semantics; DeprecationWarnings from pathspec are suppressed insyncand dockerignore regex conversion so-W errorsetups stay clean.Tests cover gitignore match/negation/anchoring, dockerignore regex behavior, and warning-free construction on both pathspec majors.
Reviewed by Cursor Bugbot for commit b2f25bc. Bugbot is set up for automated code reviews on this repo. Configure here.