Import path policy issues#237
Import path policy issues#237djm81 merged 3 commits intocursor/codebase-import-runtime-hardening-98dffrom
Conversation
…alization Replace recursive rglob counting for ignored directories with a single os.scandir of the immediate directory so pruning stays cheap. Strip only leading ./ and / path prefixes from specfactignore patterns instead of lstrip character classes, preserving dot-prefixed names. Co-authored-by: Dom <djm81@users.noreply.github.com>
_install_patch now accepts **kwargs and merges them via dataclasses.replace so positional+keyword calls (e.g. incremental_callback) work after patching. Explicit-root bypass in should_ignore_path only skips the entry root path, not descendants, so scoped discovery still prunes node_modules and defaults. Tests cover entry-point pruning and patch keyword forwarding. Co-authored-by: Dom <djm81@users.noreply.github.com>
- OpenSpec bundle-packaged-resources: heading blank lines for MD041/MD022 - protocol.yaml.j2: YAML-safe string emission via tojson - Remove eager apply_import_runtime_patches from package __init__; call from sync_repository and from_code with commands_module to avoid import cycles - template_paths: RuntimeError when no candidate matches required_glob - import_runtime_patches: RLock around Path.rglob swap; importlib for commands - import_path_policy: merge repo .specfactignore with caller patterns; capped ignored-dir counts; file entry_point handling; discover_code_files kwargs typing - Tests: entry-point under .venv, YAML assertions for protocol/workflow, patch fixture Co-authored-by: Dom <djm81@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
djm81
merged commit 001a0be
into
cursor/codebase-import-runtime-hardening-98df
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57cbbc5d1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if count >= _MAX_IGNORED_ENTRY_COUNT_SAMPLE: | ||
| break |
There was a problem hiding this comment.
_count_ignored_entries now stops counting at 100 top-level entries, but ImportDiscoveryOptions.heavy_ignored_entry_threshold still defaults to 500 in this module, so a single large ignored tree (for example one very large node_modules directory) can no longer trigger the heavy-artifact warning path. This silently removes the warning/ETA signal for the most common large-repo case and makes import progress messaging less accurate unless users manually override thresholds.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Lock doesn't protect
original_rglobcapture causing race- Capture of Path.rglob and definition of patched now happen inside the RLock so each context saves/restores the true unpatched descriptor under mutual exclusion.
- ✅ Fixed: Sampling cap makes heavy-artifact warning unreachable with defaults
- Default heavy_ignored_entry_threshold was lowered to 100 to match _MAX_IGNORED_ENTRY_COUNT_SAMPLE so one ignored tree can trigger the warning with defaults.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 57cbbc5. Configure here.
| try: | ||
| yield | ||
| finally: | ||
| Path.rglob = original_rglob # type: ignore[assignment] |
There was a problem hiding this comment.
Lock doesn't protect original_rglob capture causing race
Medium Severity
original_rglob = Path.rglob is captured at line 67, outside the _rglob_patch_lock acquired at line 89. If thread A holds the lock (and has patched Path.rglob), thread B captures the patched version as its original_rglob before blocking on the lock. When thread B later restores Path.rglob in its finally, it permanently installs thread A's stale patched closure instead of the real Path.rglob. This corrupts Path.rglob for the rest of the process lifetime, directly undermining the stated goal of the RLock.
Reviewed by Cursor Bugbot for commit 57cbbc5. Configure here.
| break | ||
| except OSError: | ||
| return 1 | ||
| return max(count, 1) |
There was a problem hiding this comment.
Sampling cap makes heavy-artifact warning unreachable with defaults
Medium Severity
_count_ignored_entries caps its return value at _MAX_IGNORED_ENTRY_COUNT_SAMPLE (100), but the default _DEFAULT_HEAVY_IGNORED_ENTRY_THRESHOLD is 500. A single ignored directory like node_modules can now only contribute a count of at most 100, which can never reach 500. The "heavy artifact tree" warning is effectively dead under default settings. The test only passes because it overrides the threshold to 2.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 57cbbc5. Configure here.
2 participants
Summary
This PR addresses multiple critical issues across
specfact-cli-modulesto improve module discovery performance,.specfactignorepattern handling, runtime patching robustness, and YAML template generation. Key changes include:**kwargsforwarding in runtime patches, serializedPath.rglobpatching with athreading.RLockto prevent race conditions, and moved patch application to command-specific entry points..specfactignorewith user-provided patterns, added comprehensive unit tests, and addressed various code quality, typing, and import cycle issues.Refs:
Scope
packages/registry/index.json,packages/*/module-package.yaml).github/workflows/*)docs/*,README.md,AGENTS.md)scripts/sign-modules.py,scripts/verify-modules-signature.py)Bundle Impact
List impacted bundles and version updates:
nold-ai/specfact-project:old -> newValidation Evidence
Paste command output snippets or link workflow runs.
Required local gates
hatch run formathatch run type-checkhatch run linthatch run yaml-linthatch run check-bundle-importshatch run contract-testhatch run smart-test(orhatch run test)Signature + version integrity (required)
hatch run verify-modules-signature --payload-from-filesystem --enforce-version-bumppasses (matches PRs targetingdev)main, also confirmed:hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump(and/or approval triggeredsign-modules-on-approvalfor same-repo PRs)dev/mainsame-repo PRs)CI and Branch Protection
verify-module-signaturessign-modules-on-approval(on approval, same-repo PRs todev/mainonly)quality (3.11)quality (3.12)quality (3.13)Docs / Pages
docs/)docs-pages.yml, if changed)specfact-clidocs updated (if applicable)Checklist