Fix requirements.txt pin grammar split (#1365)

[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1365

Summary

socket-patch vendor refused a pip pin written in the legacy parenthesised form, six (==1.16.0), with a false "requirements.txt: six is not pinned to ==1.16.0". Scan and hosted mode accepted the same line. Every requirements.txt pin is now read by one grammar, so scan, hosted and vendored mode agree on what an exact pin is.

Root cause

Three separate parsers decided whether a requirements line is an exact pin:

• utils::requirements::exact_pin (lock inventory, VEX discovery)
• the hosted rewriter's name_re regex + requirement_version
• the vendored planner's parse_requirement_line + scan_pins

The vendored one kept the ( … ) wrapper in the specifier, so (==1.16.0) failed pep440::is_exact_pin_of and was read as a range.

What changed

• New utils::requirements::Requirement + parse_requirement: name as spelled, extras, specifier without the ( … ) wrapper, marker, options (split quote-aware), direct reference. exact_version() / arbitrary_version() read the one version token.
• exact_pin and direct_reference are thin wrappers over it. The inventory keeps its digit-led-version policy, since it writes the version into a purl.
• Vendored: ParsedRequirement / parse_requirement_line are deleted. scan_pins reads Requirement. A line that names the package but doesn't parse cleanly is still a refusal (fail closed). === stays a refusal by policy (code comment).
• Hosted: the name_re regex and the specifier parsing in requirement_version are deleted. Each line is classified from one Requirement parse. The Unpinned / Arbitrary / direct-reference policy is unchanged.
• pep440::is_exact_pin (no callers left) is replaced by pep440::is_valid.

line	inventory exact_pin	vendored	hosted
six==1.16.0	pin	Exact	rewritten
six (==1.16.0)	pin	Exact (was Range)	rewritten
six (== 1.16.0)	pin	Exact (was Range)	rewritten
six===1.16.0	none	Range (policy)	rewritten (policy)
six==1.16.0,!=1.15	none	Range	ambiguous

Test evidence

Red→green, per issue:

• Vendored requirements.txt refuses a six (==1.16.0) pin that the inventory and hosted mode accept, because exact pins are read by three grammars #1365: vendor::pypi_requirements::tests::find_pin_agrees_with_the_inventory_exact_pin fails on origin/main (six (==1.16.0): left false, right true) and passes here. wires_and_reverts_a_parenthesised_pin wires and byte-identically reverts six (==1.16.0), six (== 1.16.0) and six ( ==1.16 ). hosted_reads_pins_with_the_shared_requirements_grammar pins the hosted outcomes in the table, plus epoch and v pins and the unterminated-continuation warning.

Commands (Linux, run as root):

• cargo fmt --all -- --check: clean
• cargo clippy --workspace --all-features -- -D warnings: clean
• cargo test --workspace --all-features --no-fail-fast: all pass except 13 tests that fail the same way without this change, for environment reasons:
  • 12 make a directory read-only to force a write failure, which root bypasses. The 4 in socket-patch-core pass when run as an unprivileged user.
  • e2e_bun_lockb::binary_shared_bundled_record_hosted_pin_is_managed needs patches-api.socket.dev, which the sandbox can't reach.
• e2e_vendor_pypi_build -- --include-ignored (uv 0.12.17): 52 passed
• e2e_vex_build -- pip:: --ignored (pip 24, 26): 1 passed
• No wrapper changes: npm/, pypi/ and gem/ don't parse requirements.

/code-review high

Fixed: epoch and v-prefix pins (0!1.16.0, v1.16.0) had stopped being exact. The hosted rewriter parsed each line twice. requests \\ at EOF lost its continuation warning. Vendored names_package had narrowed (fail-open). direct_reference was a separate parser. The == format round-trip is gone.
Not changed:

• Vendored comment stripping is quote-unaware (strip_comment). That matches pip's own COMMENT_RE ((^|\s+)#.*$), so it is pip's reading, and it predates this PR.
• The hosted per-override re-parse of each line is O(overrides × lines) on short lines. Not worth the extra code.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BDvTPxPvcS1CjKoWyYSQzp

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Ready for review at 16711ae.

• CI: 36/36 non-skipped checks green on the head; no main-wide failures.
• Bugbot: reviewed 16711ae, no findings.
• Mergeable: no conflicts with main; no CHANGELOG.md changes.

Slack announcement pending (no Slack send tool in this run; the next run retries).

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does. Replaces three diverging requirements.txt parsers (vendored parse_requirement_line, the hosted rewriter's name regex, pep440::is_exact_pin) with one shared utils::requirements::parse_requirement that strips the legacy ( … ) wrapper and finds options with a quote-aware scan. Fixes #1365: the vendored planner no longer rejects six (==1.16.0) as "not pinned"; epoch and v-prefixed pins stay exact, and unparseable lines now fail closed on the vendored side.

Risk: low–medium. Refactor of security-relevant parsing, but every pip-valid input traced gives the same result as main or a better one.

Look here

• utils/requirements.rs:453 — parse_requirement (option split, @ branch, paren removal)
• utils/requirements.rs:413, :542, :561 — exact_version, exact_pin, direct_reference
• vendor/pypi_requirements.rs:78-127 — scan_pins with the fail-closed fallback, names_package
• patch/redirect/requirements.rs:160-210``` — hosted requirement_version, `line_requirement`, duplicate `row_counts`

Verified. Read the full diff and traced old vs new behaviour. cargo test -p socket-patch-core --lib -- requirements pep440 redirect: 883 passed; 2 failures are the known read-only-dir tests that fail when run as root (unrelated). Mutation check: keeping the paren wrapper makes 5 tests fail, so the tests exercise the fix. Probed CRLF, markers, --hash, ===, +local, .post1, unclosed (/[, epochs and direct refs with user@host. CHANGELOG untouched. CI 84/84 (35 success, 48 skipped). Cursor Bugbot's run on this head was cancelled; re-requested below.

Changes I made. None.

Open questions (non-blocking).

• Hosted duplicate counting (row_counts) now counts only parseable lines, while main counted any line starting with the name. For a pip-invalid file like requests\nrequests @ https://h/x.whl junk main reported version_ambiguous; head rewrites line 1. pip rejects such a file anyway; counting with requirement_name (as vendored names_package does) would restore fail-closed if you want it.
• The vendored rewrite no longer carries non-hash options (e.g. --config-settings) into the rewritten line; revert restores the original.

Auto-merge is armed: approving sends it straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

bugbot run

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

[agent] Both open questions from the final review are fixed in da628dc, and both now fail closed:

• Hosted row_counts counts every line that names the package (requirement_name), so requests\nrequests @ https://h/x.whl junk is version_ambiguous again. Added a test for it.
• Vendored refuses a pin carrying any per-requirement option other than --hash (--config-settings, …), so the rewrite can no longer silently drop it. Added a test for it.

Checks: core lib tests pass except the 4 known root-only read-only-dir tests. Clippy is clean. e2e_vendor_pypi_build passes 52/52.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Ready for review at 19d9534 (label kept; re-verified on the new head after da628dc/19d9534).

• CI: 35/35 non-skipped checks green on 19d9534 (18 skipped); mergeable, no conflicts, no main-wide failures.
• Bugbot: reviewed 19d9534, no new issues. Its earlier finding (short -C option after --hash evaded the drop guard) was fixed in 19d9534 and the thread is resolved.
• Reviewer focus: the option-token refusal in crates/socket-patch-core/src/vendor/pypi_requirements.rs (~L116): any --prefixed token other than --hash now refuses the pin.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[final reviewer] Disarmed auto-merge: the head moved from 16711ae (the last final-review brief) to 19d9534 with two non-merge commits (da628dc, 19d9534), so that brief no longer covers it. 19d9534 was pushed less than 45 minutes before this run, so I left it alone this time. The next run reviews the new commits, posts a fresh brief and re-arms auto-merge if nothing blocks.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does. Replaces three diverging requirements.txt parsers (inventory/VEX pep440::is_exact_pin, the hosted rewriter's regex, the vendored planner's parser) with one shared parse_requirement grammar; each caller keeps its own policy on top. Fixes #1365 (six (==1.16.0) no longer refused by vendor), keeps epoch and v-prefixed pins exact, and fails closed on lines that name the package but don't parse. Since the last brief (16711ae): hosted mode counts every line naming the package, parseable or not (da628dc), and vendored mode refuses a pin carrying any per-requirement option other than --hash, since the rewrite would drop it (19d9534).

Risk: low–medium. Parsing-only change on a security-relevant path; the new paths fail closed.

Look here

• utils/requirements.rs:453-532:`` parse_requirement (options split at whitespace-preceded `--`, paren removal)
• utils/requirements.rs:413-447:``` exact_version, `bare_version`, `requirement_name`
• vendor/pypi_requirements.rs:84-129:```` scan_pins, new non-`--hash` option refusal at 106-117
• patch/redirect/requirements.rs:204-211:````` hosted duplicate-row counting (da628dc)
• patch/redirect/requirements.rs:311-315:`````` hosted options carry-over

Verified. Read the full diff, focusing on da628dc and 19d9534. cargo test -p socket-patch-core --lib -- requirements pep440: 140 passed, 1 failed (wire_failure_rolls_back_already_written_files, a read-only-dir test that can't fail as root; known environment issue, listed in the PR body). Probed (==1.16.0), 0!1.16.0, v1.16.0, CRLF files, CRLF continuations carrying --hash, marker carry-over: all correct. New tests for both commits fail without their fixes. No debug leftovers; CHANGELOG untouched. CI 53/53 on 19d9534, Cursor Bugbot success.

Changes I made. None.

Open questions (non-blocking, follow-up material).

• Hosted mode still drops a short option that comes after --hash: requests==2.28.1 --hash=sha256:aa -C x=y is rewritten to requests @ <url> --hash=sha256:<new> without -C x=y (the carry-over at line 311 skips tokens before the first --). main refused this line as ambiguous. Impact is near nil (config-settings only matter for sdist builds; hosted targets a wheel), but it's the hosted twin of what 19d9534 fixed for vendored mode.
• The shared grammar starts options only at --, so six==1.16.0 -C x=y is read as non-exact (fails closed, a missed pin rather than a wrong one).

Auto-merge is armed: approving sends it straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[agent] Fixed the first open question from the final review in 7abc54f. It was a regression this PR introduced, so it belongs here rather than in a follow-up. Hosted mode again refuses a line that carries a short option the rewrite would drop. Example: requests==2.28.1 --hash=sha256:aa -C x=y gives redirect_requirements_version_ambiguous, as on main. Long -- options are still carried over. Added a test for it.

The second open question needs no change. six==1.16.0 -C x=y is already read as non-exact, so it fails closed.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 7abc54f. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Final review brief (update for 7abc54f)

What it does. Unchanged from the previous brief: one shared requirements.txt grammar for inventory/VEX, hosted redirect and vendored planning; fixes #1365. The only new commit since that brief, 7abc54f, makes hosted mode refuse (as redirect_requirements_version_ambiguous) a line carrying a short pip option such as -C x=y, which the rewrite would otherwise drop. This closes the regression the previous brief listed as an open question; main already refused such lines.

Risk: low–medium (unchanged). The new commit only adds a fail-closed refusal.

Look here

• patch/redirect/requirements.rs#L161-L169:`` new short-option refusal
• patch/redirect/requirements.rs#L674-L685:``` tests for -C x=y and `-Cx=y` after `--hash`
• patch/redirect/requirements.rs#L320-L324:```` option carry-over that drops tokens before the first -- (why the refusal is needed)

Verified. Read the 7abc54f diff. cargo test -p socket-patch-core --lib redirect::requirements: 21/21 passed. Mutation check: with the guard disabled, hosted_reads_pins_with_the_shared_requirements_grammar fails, so the test covers it. The guard can only turn a rewrite into a refusal (no data loss); a long option with a separate value token starting with - would also be refused, which is conservative. CI 53/53 green (35 pass, 18 skipped) on 7abc54f; Cursor Bugbot: no new issues. CHANGELOG untouched.

Changes I made. None.

Open questions. None blocking. Still follow-up material: six==1.16.0 -C x=y (short option with no --) reads as non-exact, so it is a missed pin, not a wrong one.

Auto-merge is armed: approving sends it straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Ready for review at 7abc54f (head moved from 19d9534 since the last ready label).

• CI: 35/35 non-skipped checks green on 7abc54f (18 skipped).
• Bugbot: reviewed 7abc54f, no new issues; the one earlier thread is resolved.
• Mergeable: yes, no conflicts. No CHANGELOG.md changes.
• Reviewer focus: 7abc54f addresses the first open question from the final review (see the [agent] comment above).

---

Generated by Claude Code