Conversation
`generate_in_scope_files` adds files that Git tracks but `.gitignore` excludes, because ripgrep omits them. That `git ls-files` call sent its stderr to `DEVNULL` and treated both a spawn failure and any non-zero exit as "no such files", so a repository whose Git listing fails still produced an inventory and reported success with those files missing. Raise `InventoryError` with the captured Git message instead, matching the ripgrep call a few lines above, which already fails the inventory on a spawn error or an unexpected exit status.
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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
generate_in_scope_filesbuilds the shared scan inventory from ripgrep, then adds the files that Git tracks but.gitignoreexcludes — ripgrep honours ignore rules, so without that step every committed-but-ignored file (vendored dependencies, checked-in build output, generated clients) would be missing from the audit.plugins/codex-security/tests/test_generate_in_scope_files.py::test_inventory_keeps_ignored_tracked_files_without_ignored_untracked_filespins that guarantee.That
git ls-filescall sent its stderr toDEVNULLand treated both a spawn failure and any non-zero exit status as "this repository has no ignored-but-tracked files". So when the Git listing fails for a reason unrelated to the file set, the inventory is silently written short, the command exits0, and the scan proceeds over a smaller file list while reporting normal success. Nothing on stdout, stderr, or in the artifact records that anything was dropped.The branch is only entered when
repository/.gitexists, so the code has already concluded it is looking at a Git repository. Real triggers that leave the index intact whilegit ls-filesfails: a repository format extension the local Git does not support (fatal: unknown repository extension found: ...),detected dubious ownership in repository at ...on a checkout owned by another user, or a linked worktree whosegitdirtarget is gone.The ripgrep call ten lines above already handles the identical two failure modes correctly —
raise InventoryErroronOSError, andraise InventoryErrorwith the captured stderr on an unexpected exit status. This change makes the Git call match its sibling.Changes
plugins/codex-security/scripts/generate_in_scope_files.py: capturegit ls-filesstderr instead of discarding it; raiseInventoryErroron a spawn failure and on a non-zero exit status, including Git's own message in the error. The success path is unchanged apart from losing one level of indentation.plugins/codex-security/tests/test_generate_in_scope_files.py: regression test that a failing ignored-but-tracked listing exits2and writes no inventory.Testing
Reproduced through the plugin's own inventory command before and after the change, on Windows. Fixture: a repository with
vendor/in.gitignoreandvendor/lib.pycommitted withgit add --force;git ls-filesbroken with an unsupported repository extension.Before, on unmodified
main:./vendor/lib.pyis gone, the command succeeded, and nothing said so. The same fixture with a healthy.git/configrecords 3 files including./vendor/lib.py.New test against unmodified
main:After the change, same command and fixture:
Still accepted, confirmed after the change:
Recorded 3 in-scope files.including./vendor/lib.py.git:Recorded 1 in-scope files.Checks run:
python -m pytest plugins/codex-security/tests/test_generate_in_scope_files.py -q— 21 passed, 4 skipped. The 4 skips are host-capability gates on this machine (2 need symlink creation privilege, 2 need executable script shims that Windows cannot launch); they skip onmainas well.python -m ruff check --config plugins/codex-security/pyproject.toml plugins/codex-security— All checks passed!python -m ruff format --check --config plugins/codex-security/pyproject.toml plugins/codex-security— 137 files already formattedtsc -p sdk/typescript/tsconfig.ci.json(build:ci) — cleannode .github/scripts/check_plugin_source_compatibility.mjs— Plugin source compatibility checks passed.node --test .github/scripts/test_check_plugin_source_compatibility.mjs— 8 pass, 0 fail, 1 skippedRisk and rollout
No public CLI surface changes: no new command, flag, environment variable, or default. The only behaviour change is that an inventory which previously succeeded with a silently truncated file list now fails with exit status
2and Git's own diagnostic, which is the same contract the ripgrep step in this function already has.This turns one previously tolerated situation into a failure:
.gitexists but Git cannot list the index. That is deliberate — for a security scan, a partial inventory reported as a complete one is worse than a stop with an actionable message, and the message points straight at the fix (addsafe.directory, upgrade Git, repair the worktree). A repository with no.gitentry is untouched, and a healthy repository behaves exactly as before.Public disclosure review