Skip to content

Fix uv cooldown rule with brackets in TOML sections - #4011

Open
Eljees wants to merge 2 commits into
semgrep:developfrom
Eljees:agent/fix-uv-cooldown-section-boundary
Open

Fix uv cooldown rule with brackets in TOML sections#4011
Eljees wants to merge 2 commits into
semgrep:developfrom
Eljees:agent/fix-uv-cooldown-section-boundary

Conversation

@Eljees

@Eljees Eljees commented Jul 20, 2026

Copy link
Copy Markdown

Summary

  • stop treating every [ inside [tool.uv] as the beginning of a new TOML table
  • recognize only table headers that begin a line when delimiting the section
  • add regression coverage for inline arrays, comments containing brackets, and a missing cooldown after an array

Root cause

The section matcher used [^\[]*, so ordinary TOML values and comments containing square brackets truncated $TARGET. As a result, a valid exclude-newer later in the same section was invisible to the rule.

Validation

  • semgrep test package_managers/uv (1/1: All tests passed)
  • semgrep validate package_managers/uv/uv-missing-dependency-cooldown.yaml
  • strict metadata-cwe-prohibited-or-discouraged lint on the changed rule (0 findings)

Fixes #3993.

Signed-off-by: Eljees <yurytumanov.r@yandex.ru>
@CLAassistant

CLAassistant commented Jul 20, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@LewisArdern

Copy link
Copy Markdown
Collaborator

@Eljees please sign the CLA and then we can merge.

@Eljees
Eljees marked this pull request as ready for review July 26, 2026 20:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4e1efb61f

ℹ️ 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".

Comment thread package_managers/uv/uv-missing-dependency-cooldown.yaml Outdated
@mmassime

Copy link
Copy Markdown

Was looking at #3993 myself and ran into your PR. The line-anchored boundary is the right call. Two things I hit while testing that I think are still open here:

1. A commented-out/mentioned exclude-newer hides a real finding. The absence check is still ^(?![\s\S]*exclude-newer), which matches the bare word anywhere. Now that $TARGET spans the whole section, a comment defeats it:

[tool.uv]
override-dependencies = ["urllib3<2"]
# TODO: set exclude-newer once we finalize the cooldown policy

No cooldown is set here, but the rule stays quiet. Anchoring to an actual assignment fixes it, e.g. (?m)^(?![\s\S]*^[ \t]*exclude-newer[ \t]*=).

2. ^\s*\[ treats indented lines as table headers. \s matches newlines/indentation, so a nested array element gets read as the start of a new table and the real exclude-newer below it falls out of $TARGET:

[tool.uv]
constraint-dependencies = [
  ["nested"],
]
exclude-newer = "7 days"

This is a valid config but gets flagged. Using ^\[ (column 0) instead of ^\s*\[ avoids it. The tradeoff is indented table headers aren't matched, but those basically never show up in real pyproject.toml, and allowing them reintroduces this FP.

Might be worth a couple of test cases for both. Happy to send a patch if useful.

@Eljees

Eljees commented Jul 27, 2026

Copy link
Copy Markdown
Author

Thanks for catching both cases. I addressed them in 405ce5b:

  • the absence check now only recognizes an actual exclude-newer = ... assignment, so comments mentioning the name no longer suppress a finding;
  • table boundaries are restricted to column-0 headers, so nested array entries no longer truncate the [tool.uv] section.

I also added regression cases for both examples. The Semgrep scan and CLA checks are green.

@Eljees

Eljees commented Jul 28, 2026

Copy link
Copy Markdown
Author

@LewisArdern the CLA is signed now (the check flipped green a while ago), and mmassime's two edge cases are addressed in 405ce5b with regression tests for both. Should be good to merge whenever you get a chance.

@Eljees

Eljees commented Jul 31, 2026

Copy link
Copy Markdown
Author

@mmassime both of those were real, and both are fixed in 405ce5b.

  1. The absence check is anchored to an actual assignment now — (?m)^(?![\s\S]*^[ \t]*exclude-newer[ \t]*=) — so a commented-out or prose mention no longer suppresses the finding.
  2. The section boundary is (?=^\[|\z) at column 0 instead of ^\s*\[, so an indented array element inside constraint-dependencies no longer terminates $TARGET early.

Both of your snippets are in uv-missing-dependency-cooldown.test.toml as cases: the # TODO ... exclude-newer one as ruleid, the nested-array one as ok.

@LewisArdern the CLA is signed and both checks are green, so this is ready whenever you are.

@Eljees

Eljees commented Jul 31, 2026

Copy link
Copy Markdown
Author

@LewisArdern this has been approved and green since the CLA went through, and @mmassime's two edge cases are fixed in 405ce5b with regression tests. Anything else needed before merge?

@Eljees

Eljees commented Aug 8, 2026

Copy link
Copy Markdown
Author

@LewisArdern this one is still open. It has been approved since 20 July, the CLA check is green, and the Semgrep scan passes on 405ce5b. @mmassime's two edge cases are fixed in that same commit, with regression cases for both in uv-missing-dependency-cooldown.test.toml.

This is my third nudge, so it is the last one. If something is still blocking the merge, tell me what it is and I will fix it. If the rule is not wanted in this shape, closing it is a fine outcome - I would rather that than leave it sitting open.

@Eljees

Eljees commented Aug 14, 2026

Copy link
Copy Markdown
Author

Following up on my note from 8 August, since nobody named a blocker.

As far as I can tell there is nothing outstanding on my side: approved by @LewisArdern on 20 July, CLA signed, both checks passing on 405ce5b, branch mergeable. The change is narrow — the section matcher used [^\[]*, so any [ inside [tool.uv] (an inline array, or a comment containing a bracket) truncated $TARGET and hid a valid exclude-newer later in the same section.

If the rule is not wanted in this shape, I would rather close it than leave it sitting open — just say the word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

uv-missing-dependency-cooldown fails on config that should pass

4 participants