fix(windows): support long paths in scan tooling - #435
Conversation
…-long-path-inventories
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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
codex-security/sdk/typescript/_bundled_plugin/scripts/workbench_db.py
Lines 3494 to 3495 in 054f72a
Compare portable artifact paths before rejecting them
On Windows, when an artifact path reaches the 248-character threshold, filesystem_path(candidate) adds the \\?\ prefix, while the persisted candidate passed by scan_result remains portable. This raw comparison therefore rejects the valid artifact (and candidate.is_file() may also fail on its unprefixed long spelling), causing completed scans to omit reports, findings, and SARIF from their returned artifact map. Normalize both paths with portable_path and inspect the resolved filesystem path instead.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L24
ℹ️ 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".
| "displayName": target.name, | ||
| "targetMetadata": git_target_metadata(target), | ||
| "targetPath": str(target), | ||
| "targetPath": str(portable_path(target)), |
There was a problem hiding this comment.
When a Windows target exceeds the legacy path limit, this portable spelling is subsequently stored in security_targets.current_path, but workbench_native_indexes.py:213 checks it with the raw Path(...).is_dir() rather than converting it back through filesystem_path. In environments that require the extended prefix, every successfully scanned long-path repository is consequently returned by list-repositories with checkoutAvailable: false; reapply the filesystem spelling at that consumer before probing the checkout.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| output = filesystem_path(Path(args.out).expanduser()) | ||
| write_jsonl(output, rows) |
There was a problem hiding this comment.
For a diff scan whose discovery directory exceeds the Windows legacy limit, this now successfully writes rank_input.jsonl using an extended path, but the required next copy-deep-review-input stage constructs both args.rank_input and args.out as raw Path objects at lines 1140-1146. That stage therefore cannot read the file just produced or write deep_review_input.jsonl, stopping the diff scan before review; apply filesystem_path to those consumer paths as well.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4d2f045f7
ℹ️ 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".
| portable_scan_dir = portable_path(scan_dir.absolute()) | ||
| scan_dir = filesystem_path(portable_scan_dir) |
There was a problem hiding this comment.
When a Windows scan directory is just below the 248-character threshold, filesystem_path leaves this base path unprefixed, but callers such as artifact_path subsequently append scan-manifest.json or nested export paths that exceed the legacy limit. Those artifact probes therefore still fail in environments requiring extended-length paths, preventing an otherwise valid near-limit scan from being completed or exported; apply the helper to each fully constructed artifact path, or return an extended base path that remains safe for descendants.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| timestamp: str, | ||
| canonical_directory: Callable[[Path], Path], | ||
| ) -> None: | ||
| portable_scan_dir = portable_path(scan_dir) |
There was a problem hiding this comment.
When upgrading a Windows database whose scan was created with an explicit \\?\ path, the existing scans.scan_dir row retains that extended spelling, but this lookup now checks only the stripped portable spelling. archive_scan consequently returns as though no previous scan exists; in an --archive-existing flow the files may already have moved, yet the completed scan and its artifact rows are not repointed and can become unavailable or alias the new scan directory. Migrate existing path columns or match both spellings before switching persistence formats.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
|
|
||
| def make_repo_scope_input(args: argparse.Namespace) -> None: | ||
| repo = Path(args.repo).expanduser().resolve() | ||
| repo = filesystem_path(Path(args.repo).expanduser()).resolve() |
There was a problem hiding this comment.
When make-repo-scope-input receives a non-Git repository whose root itself exceeds the legacy Windows limit, this conversion produces a \\?\ repository path, but the fallback at line 546 still launches ripgrep with cwd=repo. That working-directory form is not usable by CreateProcess in the affected Windows environments, so SDK scans with explicit target paths stop before producing scoped-source-input.jsonl; mirror generate_in_scope_files.py by passing the absolute scope to ripgrep and using no long working directory.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dc0fd8cca
ℹ️ 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".
| state_dir = os.environ.get("CODEX_SECURITY_STATE_DIR") | ||
| if state_dir: | ||
| return Path(state_dir).expanduser().resolve() | ||
| return filesystem_path(Path(state_dir).expanduser()).resolve() |
There was a problem hiding this comment.
When CODEX_SECURITY_STATE_DIR is an existing Windows directory just below the 248-character threshold, this returns an ordinary path, but database_path() later appends workbench.sqlite3 without reapplying filesystem_path. The resulting filename can exceed the legacy 260-character limit, so opening the SQLite database fails and all workbench commands remain unusable for this valid near-limit state directory; convert the complete database path after appending its filename.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| if value.startswith("\\\\?\\") or len(value) >= WINDOWS_DIRECTORY_PATH_LIMIT: | ||
| return extended_path(Path(value)) |
There was a problem hiding this comment.
When a Windows path contains supplementary Unicode characters such as emoji, len(value) counts each character once even though Win32 stores it as two UTF-16 code units. A path split across valid components can therefore remain below 248 Python characters while exceeding the legacy Win32 buffer, causing this helper to leave it unprefixed and the filesystem operations added in this change to fail; calculate the threshold using UTF-16 code units or use the extended spelling unconditionally.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| from ctypes import wintypes | ||
| from pathlib import Path, PurePosixPath | ||
|
|
||
| from windows_paths import filesystem_path |
There was a problem hiding this comment.
When the shipped windows_scan_local_files.py script is invoked directly by a plugin host or package probe using python -I, isolated mode omits the script directory from sys.path, so this new local import raises ModuleNotFoundError before the script's own main() can run. Add the same explicit script-directory insertion used by the other bundled Python entrypoints before importing windows_paths.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a7f97c7f1
ℹ️ 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".
| repository_path = str(portable_path(repository)) | ||
| target_id = ensure_security_target(connection, repository_path) |
There was a problem hiding this comment.
When upgrading a database containing a target stored with the legacy extended-length spelling, converting the same repository to its portable spelling here makes ensure_security_target hash a different string and create a second target ID. Consequently, registering a rerun with --parent-scan-id rejects the actual parent at line 1667 as belonging to another repository, and repository listings can contain duplicate identities. Although archive_scan now dual-matches legacy and portable scan_dir values, the current code still neither migrates nor dual-matches security_targets.current_path and historical target paths.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa404ff11
ℹ️ 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".
| *, cwd: Path, profile_layer_path: Path | None | ||
| ) -> tuple[list[Path], dict[str, Any]]: | ||
| resolved_cwd = cwd.expanduser().resolve() | ||
| resolved_cwd = filesystem_path(cwd.expanduser()).resolve() |
There was a problem hiding this comment.
When a trusted Windows project root is shorter than 248 UTF-16 units but appending /.codex/config.toml pushes the path past the legacy limit, converting only resolved_cwd here leaves project_config_paths() returning ordinary paths, which read_toml() opens directly. The project-local configuration is then ignored or preflight fails despite the directory itself being valid; apply filesystem_path to each fully constructed config filename.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def write_jsonl(output: Path, rows: list[JsonRow]) -> None: | ||
| output = filesystem_path(output) |
There was a problem hiding this comment.
When ranking artifacts are placed in a Windows scan directory that requires extended-length spelling, converting paths only inside write_jsonl() does not protect the required make-rank-shards stage: it still calls mkdir() and glob() on the raw args.out_dir at lines 765-767, and later pool and merge stages likewise enumerate a raw shard directory. The scan can produce rank_input.jsonl but then stop before dispatching ranking workers; convert each shard-directory argument before performing directory operations.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6391332ece
ℹ️ 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 kind == "directory" and not resolved.is_dir(): | ||
| raise SystemExit(f"{label} must be a directory.") | ||
| return str(resolved) | ||
| return str(portable_resolved) |
There was a problem hiding this comment.
When a Windows scan directory requires the \\?\ spelling and a Deep Scan exceeds --max-cost, canonical_discovery_artifacts() now returns portable artifact paths here, while complete_budget_exhausted_scan() passes an extended scan_dir to budget_exhausted_candidates(). Its inventory.relative_to(scan_dir) and ledger.relative_to(scan_dir) calls then compare different anchors and raise ValueError, so the budget-exhausted scan cannot preserve or complete its discovery results. Convert both operands to the same spelling before deriving the scan-local relative paths.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| for table in ("workspaces", "scans"): | ||
| connection.execute( | ||
| f"UPDATE {table} SET target_id = ?, target_path = ? WHERE target_id = ?", | ||
| (target_id, target_path, previous_id), |
There was a problem hiding this comment.
When an upgraded database already contains both portable and extended-path target records for the same checkout, this update changes every scan using the discarded ID to the selected ID. Completed scans have that original target ID sealed into scan-manifest.json and used to derive finding fingerprints, so afterward verify_manifest_binding() rejects the preserved manifest and historical findings are regrouped under an identity they were not generated with. Reconcile duplicate records without changing the target ID of completed scans, or migrate their sealed identity consistently.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| diff_target.get("contentDigest") if diff_target else None, | ||
| target_summary, | ||
| str(scan_dir), | ||
| str(portable_path(scan_dir)), |
There was a problem hiding this comment.
When a Windows scan directory requires extended-length spelling and a finding has files under its writeup.poc directory, persisting only this portable spelling reaches finding_artifact_paths() unchanged. That consumer constructs poc_root from the raw stored path and calls stat() and os.walk() without filesystem_path; in environments requiring the prefix, the initial probe raises OSError and the function silently returns only the report path, so PoC attachments disappear from finding results. Convert the fully constructed PoC root before probing and walking it while deriving returned paths from portable spellings.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e04622e0cc
ℹ️ 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".
| scan_root = str( | ||
| portable_path(filesystem_path(Path(args.scan_root).expanduser()).resolve()) | ||
| ) |
There was a problem hiding this comment.
When an upgraded Windows database contains scans whose scan_dir was stored with the legacy \\?\ spelling, this normalization always queries with the portable spelling, and the SQL below checks only that spelling and its prefix. Consequently, list-scans --scan-root omits those historical scans even when the caller supplies the extended path explicitly; match both portable and extended spellings as archive_scan does.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff8b06b612
ℹ️ 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".
| ? AS target_path | ||
| """, | ||
| (str(repository), str(repository)), | ||
| (repository_path, repository_path), |
There was a problem hiding this comment.
When an upgraded Windows database contains a non-Git directory target stored with the legacy \\?\ spelling, list-scans --repository now queries both security_targets.current_path and scans.target_path using only the portable spelling. Because _same_repository() has no normalized-path equality fallback and a non-Git target has neither a Git common directory nor an origin, the historical scans are omitted until another operation happens to migrate that target; match both portable and extended spellings as the scan-root filter does.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
| def scan_directory_path(scan: sqlite3.Row) -> Path: | ||
| return filesystem_path(Path(scan["scan_dir"])) |
There was a problem hiding this comment.
When a Windows scan directory is below the 248-unit threshold but appending artifacts/deep_discovery/... or artifacts/02_discovery/... exceeds the legacy limit, this helper returns an unprefixed base path. Direct consumers such as coordinator_lease_is_live() and recover_candidate_ledger_publication() then call read_text(), glob(), and exists() on the longer unprefixed descendants, potentially treating a live coordinator as stale or failing ledger recovery; apply filesystem_path after constructing each complete artifact path.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4641e9b8cd
ℹ️ 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".
| canonical_path = filesystem_path(Path(canonical_candidate_ledger_path)) | ||
| publication_copy = canonical_path.with_name( | ||
| f".{canonical_path.name}.{uuid.uuid4()}.publish" |
There was a problem hiding this comment.
When the canonical ledger path is below 248 UTF-16 units but the UUID-suffixed .publish filename pushes the complete path beyond the legacy Windows limit, filesystem_path runs before with_name, leaving publication_copy unprefixed. In Windows environments that require extended-length spelling, os.link then fails and aborts the required commit-deep-scan-dedup stage; apply filesystem_path after constructing the publication-copy filename.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
2 participants
Summary
Windows scans failed when repository paths, scan state, generated artifact names, or Deep Scan directories exceeded legacy Win32 path limits. Filesystem operations now use extended-length Windows paths while persisted state and public scan results retain ordinary portable paths.
Changes
Testing
pnpm run typespnpm run formatRisk and rollout
Extended-length spelling is limited to Windows filesystem access. Persisted paths, scan metadata, target identities, completed artifacts, containment checks, and non-Windows behavior remain compatible.
Public disclosure review