Repository navigation
Conversation
The four reach-matrix jobs installed requirements.lock.txt without --no-deps, so pip refused the filelock override (SECURITY.md) and every nightly run failed at its first step. A test now finds every install of the main lock in the workflows, Dockerfile, Makefiles, mkdocs.yml and docs, and fails without --no-deps. mkdocs.yml's preview note, which showed the install without it, now points to CONTRIBUTING.md. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
OpenCodeReview: Commit a2d83ae was not reviewed. This PR has reached its review limit (6 reviews). CI at `a2d83ae`: 9 still running, 2 passing, 1 skipped
To review this commit: re-request review from exai-bot in the Reviewers list. <details><summary>Last review (037c9ff): no findings</summary> Open threads from the bot: 1 </details> |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #680 +/- ##
==========================================
- Coverage 95.73% 95.72% -0.01%
==========================================
Files 403 415 +12
Lines 54061 55363 +1302
Branches 9308 9516 +208
==========================================
+ Hits 51755 52996 +1241
- Misses 1421 1464 +43
- Partials 885 903 +18 🚀 New features to boost your workflow:
|
vchamarthi
left a comment
vchamarthi
left a comment
There was a problem hiding this comment.
Review at a88e409. 2 new findings: 2 must-fix. In the line comments. Adds --no-deps to the four Install dokimi steps of .github/workflows/reach-matrix.yml and a test that every install of requirements.lock.txt passes it. The workflow change is correct; the test misses an install written with --requirement, and accepts --no-deps when it belongs to another command on the same line.
Verified
- Reach-matrix installs: On
a88e409the fourInstall dokimisteps in.github/workflows/reach-matrix.yml(lines 40, 93, 138, 185) installrequirements.lock.txtwith--no-deps; on main they do not. - Removed --no-deps caught: Removing
--no-depsfrom the install indokimi/Makefile(line 68) makestest_every_install_of_the_main_lock_passes_no_depsfail, and the failure namesdokimi/Makefile. - Crlf line continuation: A
run:line that ends in a backslash continuation is joined to its-r requirements.lock.txtline when the file has CRLF line endings, so_install_linesreturns the joined install line. - New test on the pr:
tests/test_dependency_locks.pypasses ona88e409: 40 passed. - New test on main: On main the new test fails, naming the install lines in
.github/workflows/reach-matrix.ymlthat lack--no-deps. - Merge: The PR merges onto current main with no conflict.
Not verified
- The four
reach-matrixjobs have not run on a runner with this change, so the--no-depsinstall and the job steps after it are unconfirmed on a runner: Dispatch thereach-matrixworkflow on this branch (workflow_dispatch) and check that all fourInstall dokimisteps pass before the next schedule.
| return [line for line in text.splitlines() | ||
| if re.search(r"\binstall\b", line) and re.search(r"-r\s+\S*\brequirements\.lock\.txt\b", line)] |
There was a problem hiding this comment.
must-fix: _install_lines matches only -r <lock>, so an install written with --requirement <lock> passes the test without --no-deps.
On a88e409, _install_lines keeps a line only when it matches -r\s+\S*\brequirements\.lock\.txt\b. For pip install --require-hashes --requirement requirements.lock.txt it returns [], so that install is never checked. Changing dokimi/Makefile line 68 from uv pip install --python $(VPY) --no-deps -r requirements.lock.txt to uv pip install --python $(VPY) --requirement requirements.lock.txt leaves test_every_install_of_the_main_lock_passes_no_deps passing. The repository's own reader of install steps, tool_specs() in dokimi/scripts/ci_local.py (lines 161-164), accepts --requirement, --requirement=<file> and -r<file>, so the two readers disagree. No test covers the long or joined form. The test does not exist on main, so there is no base result for this proof.
| return [line for line in text.splitlines() | |
| if re.search(r"\binstall\b", line) and re.search(r"-r\s+\S*\brequirements\.lock\.txt\b", line)] | |
| return [line for line in text.splitlines() | |
| if re.search(r"\binstall\b", line) | |
| and re.search(r"(?:^|\s)(?:-r\s*|--requirement[=\s]\s*)\S*\brequirements\.lock\.txt\b", line)] |
There was a problem hiding this comment.
Fixed in 8641053: the guard reads -r f, -rf, --requirement f and --requirement=f, the forms tool_specs() reads. One change from the suggestion: \S*\brequirements misses -rrequirements.lock.txt, since -r and requirements meet with no word boundary, so the path prefix is (?:\S*/)?. test_an_install_of_the_main_lock_without_no_deps_is_found pins each form; reverting to -r\s+ fails four cases, and restoring \S*\b fails the joined one.
| missing = [(name, line.strip()) for name, lines in found.items() for line in lines if "--no-deps" not in line] | ||
| assert missing == [] |
There was a problem hiding this comment.
must-fix: --no-deps is matched anywhere on the line, so another command's flag passes an install of the main lock without it.
On a88e409, _install_lines (lines 263-267) keeps a whole run: line that names the lock, and the check at line 275 accepts that line when --no-deps appears anywhere on it. Input: a .github/workflows/*.yml file with ten installs that pass --no-deps, plus the line run: pip install --no-deps wheel && pip install --require-hashes -r requirements.lock.txt. On a88e409 the test returns without an assertion error. The second command installs the main lock without --no-deps, the install the PR description says cannot resolve. On main the test does not exist, so there is no base result. No test in the PR covers a &&, ; or || chain. The suggestion checks each command segment that names the lock for --no-deps.
| missing = [(name, line.strip()) for name, lines in found.items() for line in lines if "--no-deps" not in line] | |
| assert missing == [] | |
| missing = [(name, line.strip()) for name, lines in found.items() for line in lines | |
| for part in re.split(r"&&|\|\||;", line) if "requirements.lock.txt" in part and "--no-deps" not in part] | |
| assert missing == [] |
There was a problem hiding this comment.
Fixed in 8641053 with your approach: &&, ||, ; and | split a line into commands, and each command that installs the main lock must carry --no-deps itself. The chained case (pip install --no-deps wheel && pip install --require-hashes -r requirements.lock.txt) fails when the split is removed, and the continued cases fail when a backslash continuation is no longer joined.
…h command The guard matched only `-r <lock>` and looked for --no-deps anywhere on the line, so `--requirement <lock>`, `-r<lock>` and `--no-deps x && pip install -r <lock>` passed. It now reads the forms scripts/ci_local.py reads and checks --no-deps per command; a parametrized test pins each form and the chain. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
|
|
||
|
|
||
| # pip's requirement-file forms, as scripts/ci_local.py reads them: -r f, -rf, --requirement f, --requirement=f | ||
| _MAIN_LOCK = re.compile(r"(?:^|\s)(?:-r\s*|--requirement[=\s]\s*)(?:\S*/)?requirements\.lock\.txt\b") |
There was a problem hiding this comment.
Must-fix — Impact: a valid quoted requirement path bypasses the new regression guard. Trigger: change a reach-matrix install to pip install --require-hashes -r "requirements.lock.txt" (without --no-deps). Evidence: this regex cannot consume the opening quote, so _installs_of_the_main_lock returns no installs for that command; the repository-wide test still passes with the other guarded installs, although pip resolves the same conflicting lock. --requirement="requirements.lock.txt" and single quotes have the same problem; ci_local.py already handles these with shlex.split. Workaround: leave paths unquoted. Tokenize command arguments before identifying requirement paths and add quoted-path cases. Severity is medium because this silently misses the bounded CI-install regression the test is intended to prevent.
There was a problem hiding this comment.
Fixed in 9d8311f. The guard no longer matches the line with a regex: from each pip install / uv pip install / python -m pip install it reads the command with shlex (POSIX: quotes removed, # comments dropped, &&/||/;/| separating commands; inside a Markdown code span only up to its backtick), then reads the requirement options as tool_specs() does. -r "requirements.lock.txt" and --requirement='requirements.lock.txt' are now quoted and single-quoted cases; lexing without POSIX quote removal fails three cases.
| found = {path.relative_to(ROOT.parent).as_posix(): _installs_of_the_main_lock(path.read_text(encoding="utf-8")) | ||
| for pattern in INSTALL_SOURCES for path in sorted(ROOT.parent.glob(pattern)) if path.is_file()} | ||
| assert sum(len(installs) for installs in found.values()) >= 10, found | ||
| missing = [(name, install) for name, installs in found.items() for install in installs if "--no-deps" not in install] |
There was a problem hiding this comment.
Must-fix — Impact: the new guard accepts an install that still invokes pip's resolver and fails on the documented filelock conflict. Trigger: replace a reach-matrix install with run: pip install --require-hashes -r requirements.lock.txt # --no-deps. Evidence: _installs_of_the_main_lock retains the trailing comment, so this substring check excludes the command from missing, although the comment supplies no pip option. The parametrized check at line 290 has the same weakness and no commented-flag case. Workaround: exclude comments and check actual argument tokens for an exact --no-deps option; add this input as an unguarded regression case. Severity: medium because this defeats the regression guard for a concretely failing install, with bounded CI impact.
There was a problem hiding this comment.
Fixed in 9d8311f: # comments are dropped by the lexer, and --no-deps has to be a token of the command that installs the lock. pip install --require-hashes -r requirements.lock.txt # --no-deps is the commented-flag case; keeping comments as tokens, or checking the line text instead of the command's tokens, fails it.
The guard's regex missed a quoted lock path (-r "requirements.lock.txt") and took a commented "# --no-deps" for the option. It now reads each pip invocation with shlex (quotes removed, comments dropped, &&/||/;/| separating commands; a Markdown code span ends at its backtick), reads the requirement options as scripts/ci_local.py does, and needs --no-deps as a token of the command that installs the lock. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
| _MAIN_LOCK = "requirements.lock.txt" | ||
| _SEPARATORS = {"&&", "||", ";", "|", "&"} | ||
| # A pip invocation, as scripts/ci_local.py's PIP_INSTALL, with uv's and python -m's forms. | ||
| _PIP_INSTALL = re.compile(r"\b(?:python3? -m |uv )?pip3? install\b") |
There was a problem hiding this comment.
Must-fix — Impact: a valid shell install without --no-deps can silently evade the guard and fail on the documented lock conflict. Trigger: change a reach-matrix install to pip install --require-hashes -r requirements.lock.txt (two spaces), or use a tab between pip and install. Evidence: this literal-space regex fails to match, so the helper skips the line before shlex can normalize whitespace; the other installs still satisfy the aggregate count assertion. Recognize shell whitespace rather than a single literal space and add whitespace-variant regression cases. Workaround: retain the exact single-space spelling. Severity justification: medium, a bounded correctness failure of the regression guard.
There was a problem hiding this comment.
Fixed in 3962c3d: a pip command is found from its tokens (pip or pip3, then install), after shlex splits on any whitespace, so two spaces or a tab no longer hide it; the spacing case is pip install<TAB>--require-hashes -r requirements.lock.txt.
| start = _PIP_INSTALL.search(line) | ||
| if _MAIN_LOCK not in line or start is None: | ||
| continue | ||
| span = line[start.start():] |
There was a problem hiding this comment.
Must-fix — Impact: the repository-wide guard rejects a valid, correctly guarded workflow command. Trigger: run: "pip install --no-deps -r requirements.lock.txt". Evidence: the caller supplies raw YAML text, and this slice removes the opening YAML quote but retains the closing quote; shlex therefore raises ValueError, converted below to AssertionError. Decode YAML run values before shell parsing (or otherwise distinguish enclosing source-format quotes), and add a quoted-run regression case. Workaround: use an unquoted run value or block scalar. Severity justification: medium, a bounded CI correctness failure on valid workflow syntax.
There was a problem hiding this comment.
Fixed in 3962c3d: workflow and action files are no longer read as text. Each run: step is taken from yaml.safe_load, so a quoted run: value reaches the lexer as the command YAML decodes it to; test_a_quoted_run_value_is_read_as_the_command_it_decodes_to pins it.
| if line[:start.start()].count("`") % 2: # inside a Markdown code span: up to its closing backtick | ||
| span = span.split("`", 1)[0] |
There was a problem hiding this comment.
Must-fix — Impact: an unguarded documentation install can pass the repository-wide guard and direct users into the documented resolver conflict. Trigger: a line containing Run pip install --no-deps -r requirements.lock.txtorpip install -r requirements.lock.txt. Evidence: _PIP_INSTALL.search finds only the first invocation, and this truncation discards the second code span; the returned installs contain only the guarded command. Resume scanning after each Markdown span and add a two-span regression case. Workaround: put each example on a separate line. Severity justification: medium, a concrete but bounded loss of the documentation-install guard.
There was a problem hiding this comment.
Markdown is no longer scanned (3962c3d). The guard now covers what runs: workflow and action run: steps, the eval image's Dockerfile and the Makefiles. A Markdown example is prose a reader copies, not a command CI or make executes, and parsing shell out of prose kept adding cases without guarding anything that runs; the test's docstring states this limit. Reading what runs also brought actions/setup/action.yml in, an install no earlier version of the guard saw.
| if token not in _SEPARATORS: | ||
| command.append(token) | ||
| continue |
There was a problem hiding this comment.
Must-fix — Impact: another command's --no-deps can still satisfy the guard for an unguarded main-lock install. Trigger: (pip install -r requirements.lock.txt); pip install --no-deps wheel. Evidence: shlex with punctuation_chars=True groups consecutive punctuation into the token );. That token is not in _SEPARATORS, so both installs remain in one command; the final check sees the second install's --no-deps and passes, although the first invokes the conflicting resolver. Workaround: keep these installs on separate lines. Recognize shell separators even when adjacent to parentheses (or restrict the lexer's punctuation set appropriately), and add this input as an unguarded regression case. Severity justification: medium, because this is a bounded but material false negative in the repository's installation regression guard.
There was a problem hiding this comment.
Fixed in 3962c3d: the lexer groups only ;, & and | as punctuation, so ) stays with its word and ; separates the commands; (pip is still read as pip. Your input is the subshell case, and it fails when the punctuation set or the bracket handling is reverted.
The guard now reads what runs: each workflow and action run: step decoded by YAML (a quoted run: value no longer trips it), the eval Dockerfile and the Makefiles, and pins that it sees each kind, the setup action included, which no version read before. Commands are lexed with shlex on ;&| only, so spacing, tabs and a subshell's ")" no longer hide a command. Markdown examples and commands built at run time are out of scope, as the docstring says. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
| for line in re.sub(r"\\\r?\n", " ", script).splitlines(): | ||
| if _MAIN_LOCK not in line: | ||
| continue | ||
| lexer = shlex.shlex(line, posix=True, punctuation_chars=";&|") |
There was a problem hiding this comment.
Must-fix — Impact: the repository-wide guard rejects a valid install that already disables dependency resolution. Trigger: a workflow run: value of (pip install -r requirements.lock.txt --no-deps). Evidence: with this punctuation set, shlex produces (pip and --no-deps); pip-name normalization recognizes the install, but the final exact-token check does not recognize --no-deps). The existing subshell case tests only an unguarded install with the requirement path last, so it misses this false positive. Handle shell parentheses consistently while preserving quoted argument contents, and add a guarded subshell regression case with the flag last. Workaround: place --no-deps before -r, or insert whitespace before ). Severity justification: medium, a bounded CI failure for valid syntax that the parser explicitly supports.
There was a problem hiding this comment.
Fixed in fd15a49: (pip and --no-deps) lose their parentheses, so (pip install -r requirements.lock.txt --no-deps) is read as guarded. test_an_install_of_the_main_lock_without_no_deps_is_found[guarded-subshell] pins it and fails without the change.
| if token.strip(";&|"): | ||
| command.append(token) | ||
| continue | ||
| pip = [i for i, word in enumerate(command[:-1]) if word.strip("()").rsplit("/", 1)[-1] in ("pip", "pip3")] |
There was a problem hiding this comment.
Must-fix — Impact: an executable Makefile install can silently evade the main-lock guard. Trigger: add a recipe \t@pip install -r requirements.lock.txt to either scanned Makefile while retaining the existing guarded installs. Evidence: _install_scripts() supplies raw Makefile text, so shlex leaves the executable token as @pip; this comparison does not recognize it, and the helper returns no installs. The repository-wide source assertion still passes, although make strips @ before executing the unguarded pip command and hits the documented resolver conflict. The same applies to Make's - and + recipe prefixes and prefixed pip3. Normalize recipe prefixes when extracting Makefile commands and add regression coverage. Workaround: use python -m pip or uv pip in prefixed recipes. Severity justification: medium, a bounded but material false negative in the installation regression guard.
There was a problem hiding this comment.
Fixed in fd15a49: A Makefile @, - or + recipe prefix and pip3.11 are read as pip. test_an_install_of_the_main_lock_without_no_deps_is_found[recipe-prefix] pins it and fails without the change.
The main-lock guard lexed line by line and matched only a bare `pip install`. - `(pip install -r requirements.lock.txt --no-deps)` is read as guarded - a global option, `python -m pip -q`, `pip3.11` and a Makefile `@` prefix are read - a quoted word spanning lines, or holding `\"`, no longer raises - the word after `>` or `<` is not a pip flag; a backslash path is read - `.yaml` workflows and `action.yaml` are scanned Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
|
Pushed 1 commit to
Threads: 2 fixed, 0 answered as not a defect. Questions for @xaleryb1. Should the PR description drop
|
| if not token.strip(";&|\n"): | ||
| commands.append(command) | ||
| command = [] |
There was a problem hiding this comment.
Must-fix — Impact: a valid unguarded pip invocation can disappear from the install guard. Trigger: pip install --log ";" -r requirements.lock.txt. Evidence: POSIX shlex returns the quoted filename as ;, and this branch treats it as a command separator. The resulting commands are pip install --log and -r requirements.lock.txt, so neither matches an installation; adding this run step alongside existing guarded installs leaves the aggregate check passing. Distinguish quoted argument content from shell operators (also for &, |, and quoted redirection characters), and add regression coverage. Workaround: avoid punctuation-only log filenames and pass the real --no-deps flag. Severity justification: medium, a concrete, bounded loss of the regression guard's supported quoting behavior.
There was a problem hiding this comment.
Fixed in 81e34fc: an operator inside quotes stays part of the word, so --log ";" no longer hides the install; the quoted-separator case fails without it.
| elif redirected: | ||
| redirected = False | ||
| elif token.strip("()"): | ||
| command.append(token.strip("()").replace("\0", '"')) |
There was a problem hiding this comment.
Must-fix — Impact: an install without dependency resolution disabled can pass the guard. Trigger: pip install -r requirements.lock.txt --log "(--no-deps)". Evidence: shlex removes the quotes, then strip("()") changes the log filename into --no-deps; the aggregate test's exact-token membership check therefore accepts it, although pip receives only a log filename, not the flag. Preserve parentheses in quoted arguments while normalizing actual shell grouping, and add this as an unguarded regression case. Workaround: pass a genuine --no-deps option. Severity justification: medium, a bounded but material false negative that permits the documented resolver conflict to recur.
There was a problem hiding this comment.
Fixed in 81e34fc: quoted parentheses are kept, so the log file is not read as --no-deps; the quoted-parentheses case fails without it.
| if any(re.fullmatch(r"pip[\d.]*", word.lstrip("@+-").rsplit("/", 1)[-1]) and "install" in command[i + 1:] | ||
| for i, word in enumerate(command)) |
There was a problem hiding this comment.
Must-fix — Impact: the repository-wide guard can fail on a command that does not install anything. Trigger: a scanned run step containing pip --log install freeze -r requirements.lock.txt. Evidence: install is the global --log option's filename, while the subcommand is freeze; this new suffix-membership check nevertheless returns the command as an installation, and the aggregate test rejects it for lacking --no-deps (which freeze does not support). Identify the pip subcommand after consuming global options instead of accepting any later install argument, and add a non-install regression case. Workaround: choose a log filename other than install. Severity justification: medium, a bounded but material false CI failure for a valid pip invocation.
There was a problem hiding this comment.
Not changed: a log file named install makes the guard fail loudly, not pass silently, and no scanned file has that shape; reading the real subcommand would need pip's list of value-taking global options.
fd15a49 to
1c21c64
Compare
napetrov
left a comment
napetrov
left a comment
There was a problem hiding this comment.
Change class: not computed (no receipt profile for this repo, or receipt.py failed)
Comment, no blocking. CI: 0 failing jobs on head. The four workflow steps are fixed, and most of the earlier guard bypasses are fixed on head. Still open and confirmed: --log ";" hides the install (T11) and --log --no-deps counts as guarded (T12, the same class as T4). I added one nit: a backslash at the end of a comment line hides the install after it. The --requirem abbreviation is the same class as T1 and I did not repeat it; it is still not read as the lock.
The body says the guard scans mkdocs.yml and the Markdown docs; the code does not (the test docstring says so). Please fix either the body or the scope. Docs installs (T7) are still unguarded.
Coverage: whole-PR and edge pass; the install finder on head with the bypass inputs; the four workflow steps; the open threads. Not run: the reach-matrix jobs, the full test suite.
| dropped from the words, and the word after a redirection (``>``, ``<``) left out.""" | ||
| commands: list[list[str]] = [] | ||
| command: list[str] = [] | ||
| script = re.sub(r"\\\r?\n", " ", script).replace('\\"', "\0") # an escaped quote is a letter, not a quote |
There was a problem hiding this comment.
nit: a backslash at the end of a comment line joins the next line into the comment, so a real install after it is not checked
Different from T9 (#680 (comment), same line, fixed on head): that one is a valid install rejected; this one is an install hidden.
_commands joins backslash-newlines before it strips comments. For # note \ followed by pip install --require-hashes -r requirements.lock.txt, the install becomes part of the comment and _installs_of_the_main_lock returns []. The shell does not do this: the comment ends at the newline. No scanned file has this shape today.
How to check: # note \ + newline + pip install --require-hashes -r requirements.lock.txt → [] (head).
Expected: strip comments before joining continuations, so the next line is read as a command.
There was a problem hiding this comment.
Fixed in 037c9ff: comments are dropped before backslash continuations are joined, as bash and dash do, so the install on the next line is checked; the comment-backslash case fails without it.
The lexer took a quoted ";" for a separator and stripped the parentheses of a quoted "(--no-deps)", so `--log ";"` hid an install and `--log "(--no-deps)"` passed one as guarded. Operators inside quotes are now letters until the word is built; the three cases fail without it. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Continuations were joined before comments were dropped, so a comment ending in a backslash swallowed the install on the next line, which the shell runs. Comments are now dropped first; the comment-backslash case fails without it. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Pushed 2 commits to
|
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The workflow fix is right: the four Install dokimi steps in reach-matrix.yml now pass --no-deps, as every other install of the main lock does. The guard test reads the repo's real installs correctly, but its final verdict line is not pinned by any test. One risk and one nit inline.
What I ran, on a Linux review host at 85bff5e:
make ci-local: 31 passed, 1 failed, 14 skipped. The one failure isRun tests: 19,787 tests passed, and 2 errors are 60 s timeouts intest_audit_test_quality.py. That file alone passes (172 passed in 20 s), so the timeouts are timing, not a failure the PR causes. The PR changes only tests, so there are no changed lines to cover.tests/test_dependency_locks.py: 76 passed.- Mutation check on the guard: replacing
if "--no-deps" not in installintest_every_install_of_the_main_lock_passes_no_depswithif Falsestill passes all 76 tests (see inline). - Probes of
_installs_of_the_main_lockwith string inputs, see the nit inline.
Checked, no issues
- Installs: every install of
requirements.lock.txtin the tree (workflows,actions/setup/action.yml, the eval Dockerfile,dokimi/Makefile,ci/hooks.py) passes--no-deps;reach-matrix.ymlhas no other install. - Docs header:
dokimi/mkdocs.ymlnow points to the "Docs build" entry inCONTRIBUTING.md, which exists, anddocs.ymldoes runmkdocs build --strict. - Repo rules: no changelog fragment is needed (no package change), the test file is within the size limits, all 7 non-merge commits carry
Signed-off-by. - Scope:
ci/hooks.py:364installs the lock from Python as an argv list with--no-deps; the guard does not read Python code, and the docstring does not list that among the cases it does not cover.
Bot points I checked that no longer apply at this head (11)
- exai-bot's point on a quoted requirement path does not apply: the guard reads commands with
shlex, and thequotedandsingle-quotedcases pass at this head. - exai-bot's point on a commented
--no-depsdoes not apply: comments are dropped before the flag is checked, and thecommented-flagcase pins it. - exai-bot's point on a double space or tab before
installdoes not apply: the install is found from tokens, and thespacingcase pins it. - exai-bot's point on a quoted YAML
run:value does not apply:run:values are taken fromyaml.safe_load, and a test pins it. - exai-bot's point on two Markdown installs on one line does not apply: Markdown is no longer scanned, and the test docstring says so.
- exai-bot's point on
(pip install ...); pip install --no-deps wheeldoes not apply: thesubshellcase passes at this head. - exai-bot's point on
(pip install -r requirements.lock.txt --no-deps)does not apply: theguarded-subshellcase passes at this head. - exai-bot's point on Makefile
@,-and+recipe prefixes does not apply: therecipe-prefixcase passes at this head. - exai-bot's point on
--log ";"does not apply: thequoted-separatorcase passes at this head. - exai-bot's point on
--log "(--no-deps)"does not apply: thequoted-parenthesescase passes at this head. - exai-bot's point on
pip --log install freezedoes not apply to any file the guard scans: the author declined it, and the result is a loud failure, not a missed install.
| missing = [(name, shlex.join(install)) for name, installs in found.items() for install in installs | ||
| if "--no-deps" not in install] |
There was a problem hiding this comment.
No test pins the --no-deps check of the repository-wide guard. The parametrized cases test _installs_of_the_main_lock, and the real files are all guarded, so missing is always [].
Mutation check: replacing if "--no-deps" not in install] with if False] passes the full file (76 passed). Removing the check turns the guard into a no-op, and nothing fails.
Suggestion: move the filter into a helper, for example _unguarded(found: dict[str, list[list[str]]]) -> list[tuple[str, str]], call it here, and add a test that feeds it {"x": [["pip", "install", "-r", "requirements.lock.txt"]]} and expects one entry, and the same install with --no-deps and expects none.
There was a problem hiding this comment.
Fixed in a2d83ae: The --no-deps filter is _unguarded, and a test feeds it an install with and without the flag. Replacing its condition with False now fails. tests/test_dependency_locks.py::test_an_install_without_no_deps_is_reported_and_one_with_it_is_not pins it and fails without the change.
| files.append(next(args, "")) | ||
| elif token.startswith("--requirement="): | ||
| files.append(token.partition("=")[2]) | ||
| elif token.startswith("-r") and not token.startswith("--"): |
There was a problem hiding this comment.
Nit: Some real spellings of an install of the main lock are not seen. _installs_of_the_main_lock('pip install -qr requirements.lock.txt') returns [], and so does '$(PIP) install -r requirements.lock.txt'. Both are valid, and an unguarded install written that way passes the guard. No scanned file has either shape today.
Suggestion: read a clustered short flag in _requirement_files by changing the first branch to if token == "--requirement" or re.fullmatch(r"-[A-Za-z]*r", token):. For a command held in a variable, add it to the docstring's "Not covered" list next to "a variable holding the file name".
There was a problem hiding this comment.
Fixed in a2d83ae: -qr requirements.lock.txt is read as the lock, and --requirem too. A pip command held in a variable is added to the docstring's list of what is not covered. tests/test_dependency_locks.py::test_an_install_of_the_main_lock_without_no_deps_is_found pins it and fails without the change.
The guard missed -qr, --requirem, pip.exe, a JSON-form Dockerfile RUN and a PowerShell backtick continuation, and raised ValueError on an escaped apostrophe or on '...\' . A comment ending in a backtick hid the next line's install. The --no-deps verdict is now a helper with its own test. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
|
Pushed 1 commit to
Threads: 2 fixed, 0 answered as not a defect. From the comments:
|
Summary
Every nightly
reach-matrixrun has failed at its first step since at least 2026-11-07. Its four jobs installrequirements.lock.txtwithout--no-deps, and that lock is not self-consistent by design: it overrides litellm'sfilelockcap (SECURITY.md, #383), so pip's resolver refuses it:SECURITY.mdsays every install that includes the main lock passes--no-deps, and the other workflows, the Dockerfile and the Makefile do. Nothing checked it, so a job copied without the flag went unnoticed.Changes
.github/workflows/reach-matrix.yml: the fourInstall dokimisteps pass--no-deps.dokimi/tests/test_dependency_locks.py:test_every_install_of_the_main_lock_passes_no_depsreads eachrun:step of.github/**/*.yml|yamlandactions/**/action.yml|yaml(YAML-decoded),.agent-eval/Dockerfileand both Makefiles, and fails on an install ofrequirements.lock.txtwithout--no-deps. Markdown examples and commands built at run time are not read.dokimi/mkdocs.yml: its header showed the docs install without--no-deps; it is now 2 lines and points to the "Docs build" entry inCONTRIBUTING.md, which has the right command.Evidence
On Linux, Python 3.13:
pip install --require-hashes -r requirements.lock.txtfails with the error above; with--no-depsit installs, and the job's next command,PYTHONPATH=. python scripts/reach_conformance.py --help, runs.main, naming the fourreach-matrix.ymllines, and passes here.tests/test_dependency_locks.py,tests/test_audit_test_quality.pyandtests/test_docs_links.py: 223 passed. Full suite: 18960 passed.check_comment_blocks,check_growthandcheck_changelog_fragmentpass; no fragment is needed, since the package does not change.workflow_dispatch.Risk
Low. Only the four install steps change, to the flags every other install of the lock already uses.