Repository navigation
T33 — Add the property-test check to the workspace - #1678
Conversation
The tiered spec's T1 lane adds a property-test dependency the workspace does not have (proxy parity, NET-071, is its main future consumer; the BEP doc names the same cost). Wire `proptest` into the workspace dependencies and `minimald`'s dev-dependencies, and prove the check runs with a first property over the B5 proxy's `parse_request`: for an arbitrary host, port, method, path, header casing, and whitespace, the parser reads back exactly the authority the head was built with — CONNECT from its request line, any other method from its `Host:` header — and classifies the kind to match. Adding the dependency also re-resolves shared transitive deps the existing ranges admit: prost-build's lock entry now carries its real heck ^0.5 requirement (ouroboros_macro keeps heck 0.4.1), and the new tempfile/rusty-fork windows edges unify windows-sys/windows-core to the versions the published manifests require.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Approved: 1 finding, none above this repo's threshold at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none: |
| // Property test over the request-head parser. The first consumer of the | ||
| // workspace's `proptest` dependency (the tiered spec's T1 lane): whatever |
There was a problem hiding this comment.
NIT · style · confidence 0.60 · in property_check_runs
The new comment block above the property test narrates the change process rather than explaining the code: "The first consumer of the workspace's proptest dependency (the tiered spec's T1 lane)" describes the PR's role in a spec, not what the test does. The same pattern recurs in the two added Cargo.toml comments ("NET-071 proxy parity is the first consumer" in the root, "the T1 lane adds this dev-dependency" in minimald). The useful half of the block — what the parser must read back, for CONNECT vs Host: forms — is worth keeping; drop the "first consumer / T1 lane" framing.
What the reviewer checked
proxy.rs:770-771 // Property test over the request-head parser. The first consumer of the
// workspace'sproptestdependency (the tiered spec's T1 lane): whatever
Hand this to an agent
Act on a code-review finding.
File: crates/minimald/src/net/proxy.rs
Lines: 770-771
Category: style
Severity: nit (reviewer confidence 0.60)
Finding:
The new comment block above the property test narrates the change process rather than explaining the code: "The first consumer of the workspace's `proptest` dependency (the tiered spec's T1 lane)" describes the PR's role in a spec, not what the test does. The same pattern recurs in the two added Cargo.toml comments ("NET-071 proxy parity is the first consumer" in the root, "the T1 lane adds this dev-dependency" in minimald). The useful half of the block — what the parser must read back, for CONNECT vs Host: forms — is worth keeping; drop the "first consumer / T1 lane" framing.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
Reviewed against the T33 plan block on #1437.
Plan compliance. Files touched are exactly the four layers the block names: Cargo.toml, Cargo.lock, crates/minimald/Cargo.toml, crates/minimald/src/net/proxy.rs. proptest is a dev-dependency only (crates/minimald/Cargo.toml:107). The test line cargo nextest run -p minimald property_check_runs is present (proxy.rs:779-800) and the task status records it and agent-verify at exit 0.
Findings, none blocking.
Cargo.lockre-points edges beyond the eight proptest crates (heck 0.4→0.5 for prost-build, getrandom 0.3→0.4 and windows-sys 0.52→0.61 for tempfile and friends). Main's lock was already valid under--locked, so the PR body's "corrected to its real heck ^0.5 requirement" is not accurate; the churn is harmless (no new duplicate versions,--lockedgreen).- The property is a round-trip through the same convention the parser uses, so it proves the check runs rather than specifying the parser. That is what the plan asks of an enablement task.
- Follow-up, not for this PR: no
.gitignoreentry forproptest-regressions/.
Open thread (proxy.rs:771, minimal-review, sub-threshold): valid style nit about process narration in three comments. Below the repo threshold, verdict APPROVE, not blocking.
Approving; merging with the bot review concluded and no revise round outstanding.
T33 — Add the property-test check to the workspace
slice: S8
kind: enablement
covers: none
size: S
est: 20 code, 60 tests
executor: agent
difficulty: routine
model: light
diagnostics: none: a test dependency adds no runtime behaviour a bundle could carry
observability: none: a test dependency emits nothing at runtime
layers:
- workspace:
- Cargo.toml: adds the property-test library to the workspace dependencies
- crates/minimald/Cargo.toml: takes it as a development dependency
- Cargo.lock: carries it
- crates/minimald/src/net/proxy.rs: a first property test over the request-head parser proving the check runs
test: cargo nextest run -p minimald property_check_runs
produces: none
consumes: none
proof: The workspace carries a property-test dependency, and a first property test over the proxy's request parser runs under the ordinary test command.
Proof
The workspace now carries
proptestas a workspace dependency taken byminimaldas a dev-dependency (Cargo.lock resolves it), and a first property test over the B5 proxy's request-head parser — arbitrary host, port, method, path, header casing, and whitespace; the parser must read back exactly the authority the head was built with for both the CONNECT andHost:-header forms — runs green under the ordinarycargo nextest run -p minimaldtest command.Notes from the agent: Library choice follows the specs: docs/specs/24-spec-box-egress-proxy names T1 as adding
proptestto the workspace as a dev-dependency, soproptest = "1"went into [workspace.dependencies] (resolves to 1.11.0, MSRV 1.85 <= the workspace floor 1.95; MIT/Apache-2.0, inside deny.toml's allowlist). The lockfile gained proptest plus seven helper crates and also re-resolved deps the existing ranges already admit: prost-build's lock entry was corrected to its real heck ^0.5 requirement (ouroboros_macro still holds heck 0.4.1, so the deny.toml skip stays used), and the new tempfile/rusty-fork windows edges unified windows-sys/windows-core to versions the published manifests require. Verifiedcargo metadata --lockedpasses andcargo clippy -p minimald --all-targetsis clean; cargo-deny itself is not installed on this box so the deny lane was checked by license fields, not by a run. No files outside the four layers were touched.Tests
cargo nextest run -p minimald property_check_runsVerify
min task run agent-verifypassed ([tasks.verify]).Closes #1623