Corpus selection, the write server, and a probe that runs without an installer - #1
Merged
Conversation
The selection rule lives in bench/select_corpus.py rather than in prose, so the
corpus a reader gets from the same snapshot is the corpus that was measured. The
snapshot is pinned by sha256 in bench/PROVENANCE.json.
Three arms. A corpus made only of records that are already known to be wrong
cannot measure false-correction rate, because nothing in it should be left alone:
A 6 PYSEC records a disjoint affected range where GHSA records a single
interval. A correction is expected. ansible, scrapy, keras, airflow,
qutebrowser, vantage6 -- all six that exist.
B 8 Both databases agree exactly. The agent should change nothing.
C 11 No fix commit is published; the answer has to be reconstructed from
repository history.
Two rules were written wrong first and are covered by tests:
* Reference type is not a reliable signal. `type: FIX` appears zero times in
GHSA-reviewed records while 4,854 commit URLs sit under `type: WEB`, so the
check matches the URL and ignores the declared type.
* Ranges must be read per type. PYSEC carries a GIT range whose events are
commit SHAs alongside an ECOSYSTEM range whose events are versions; counting
across both makes an ordinary record look disjoint. A GIT fix event is a
lead, never a boundary.
PYSEC is a held-out second opinion, written to bench/reference/ and never read by
an agent-facing path. Agreement is only evidence if the two were arrived at
separately.
Held-out split is round-robin within each arm, so it cannot be steered toward
advisories the implementation happens to handle. At this size the held-out set is
small enough that one error moves the figure by ten points; report raw counts.
Every advisory considered and not taken is recorded in bench/EXCLUSIONS.json with
a reason from a closed set.
This server exposes the two tools that change something outside this project, and nothing else. It does not read git, compute ranges, or reason about evidence -- rangecore does that, in a sandbox, holding no credentials. What thinks has no key; what holds the key does not think. Stateless by necessity rather than preference. A stateful MCP server that restarts while TrueForge is mid-turn leaves the session unrecoverable: -32000 "Server not initialized", the granted approval consumed, the retry a 422. Observed on TrueForge e9bf976. Statelessness is expressed by omitting sessionIdGenerator, since the option is declared optional and the SDK disables session management when it is absent. Both write tools carry readOnlyHint: false and destructiveHint: true. TrueForge resolves its approval policy entirely from annotations -- @Write is readOnlyHint === false, @destructive is destructiveHint === true -- so a tool carrying neither executes with no approval at all. Both hints are set so the tool matches either selector however the policy is configured. A test asserts this, because losing an annotation does not fail loudly; it starts writing without asking. idempotentHint is deliberately absent. Tool execution is at-least-once across a crash inside the write window, and the write path does not yet dedupe. Claiming otherwise would be false. branchNameFor() is a pure function of the advisory id so that a second call can find the first call's work; openOrFindPullRequest is not written yet and is marked todo in the test suite rather than left unsaid. The evidence schema enforces two rules in code. A boundary needs a commit and the hunk it changed, checked at named refs. And text anyone can write may nominate a candidate but never justify a boundary -- an issue comment pointing at a genuine old commit passes every other check here, because this schema stops fabrication and not misdirection. Node is pinned to >=22.6: sources run under --experimental-strip-types with no build step. exactOptionalPropertyTypes is off because the SDK's own Transport types are not assignable under it.
The sandbox has Python and no way to install anything, so the behavioural evidence
tier either works by acquiring built wheels or it does not exist. Running this now
rather than on the day it was scheduled turns a late discovery into an early one.
It works. certifi 2024.6.2 -> 2024.7.4, probe written against the published fix,
executed at both releases from wheels pulled and digest-checked at run time:
control VULNERABLE @ 2024.6.2
boundary NOT_VULNERABLE @ 2024.7.4
calibrated true, differential true
Acquire from wheels, not from a git checkout. A git tree of a pure-Python project
often will not import -- src/ layouts, setuptools_scm generating _version.py at build
time, generated parser tables. All build-step failures, all absent from a wheel, which
is already built. Affected ranges are expressed over released versions anyway.
Two things the first runs got wrong, both now enforced:
* The probe ran without -S, so the host's own copy of a dependency was visible and
the probe measured the wrong tree. It read INCONCLUSIVE for the right reason by
accident.
* A probe written against urllib3 returned VULNERABLE at both releases, because it
tested the wrong thing. The negative control caught it: the probe did not fire at
the release still known to be vulnerable, so calibrated was false and the result
was discarded rather than reported. A probe that cannot distinguish the release
before the fix has nothing to say about the release after it.
Dependencies, not wheel availability, are the gate. 7.4% of candidate releases have
zero runtime dependencies and a portable wheel; the rest cannot be imported without
an installer this design does not have. apache-airflow has 62 dependencies,
open-webui 95.
So arm C now reserves four slots for probe-capable packages. Without the reservation
the corpus contained one by chance. The reservation biases arm C toward simpler
packages; that is a declared stratum, recorded in EXCLUSIONS.json as
PROBE_QUOTA_ONLY, and probe coverage is reported separately and never folded into the
headline. Five of twenty-five now qualify, against an exit test that needs two --
eligibility is checked at the fix version only, so the achieved number will be lower.
probe_eligibility.py zipped a dict against a differently-sorted parallel map, so every
verdict was attached to the wrong package. parso, a zero-dependency parser, came back
with 74 dependencies and calibre-web with none. Nothing failed; the corpus would just
have spent its probe slots on packages that cannot carry a probe. Eligibility is now
keyed by package@version and three tests cover it.
Owner
Author
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Four of the five stand. The first does not, and the reason is worth recording. ZIP SLIP -- not reproducible, guard added anyway. The report says extractall() on a downloaded wheel allows path traversal and lets an archive overwrite files on the host. I built an archive containing `../../escaped.txt`, an absolute path, and a symlink entry pointing at /etc/passwd, and extracted it: CPython strips `..` segments, strips leading separators, and writes symlink entries as ordinary files. Nothing escaped. So the vulnerability as described is not exploitable here. safe_extract() is added regardless, because the safety of that function was an undocumented property of the standard library rather than anything this code stated or checked -- and this is the one place in the project where an archive from an unreviewed third party is unpacked. A matching digest proves PyPI served that wheel. It proves nothing about what is inside it. METADATA FAILURE READ AS "NO DEPENDENCIES" -- correct, and the worst of the five. runtime_requirements() returns None on HTTP error or timeout, and differential() tested it for truthiness, so a transient network failure marked a release eligible. That inflates a reported coverage number on a flaky connection. Now checked explicitly. MARKER-GUARDED DEPENDENCIES MISCOUNTED -- correct, and it moved the number. Dependencies behind an environment marker were counted as if they always applied. Evaluating PEP 508 markers needs a parser this module deliberately does not have, so they are now excluded from the gate and a probe that turns out to need one fails as INCONCLUSIVE, which is refused rather than reported. Being wrong here costs a probe, never a wrong answer. Probe eligibility across the snapshot goes from 195 releases to 235 -- the estimate was biased downward, as reported. ERROR RESPONSE BYPASSED redact() -- correct. github.ts states that a token is never interpolated into a log, an error, or a tool result, and the HTTP error path returned err.message raw. An error raised deep in a request path is exactly where one would arrive. It goes through redact() now. GHSA IDS REJECTED ON CASE -- correct, with a second reason the report did not give. The pattern is now case-insensitive and shared between the branch helper and the tool schema. Normalising also means two spellings of one advisory cannot produce two branches, which matters because that name is what lets a repeated write find the first write's work. Covered by a test.
|
Code review by qodo was updated up to the latest commit adde340 |
Both findings from the second review are correct, and both are mine. The pattern was spelled with character classes -- [0-9a-zA-Z] in each group -- which left the `GHSA-` prefix literal. So `GHSA-x7jh-595q-wq82` was rejected while the comment directly above it promised case-insensitivity. A comment asserting behaviour the code does not have is worse than no comment: it stops the next reader checking. The test did not catch it because it only varied the suffix. It compared `GHSA-X7JH-...` against `GHSA-x7jh-...`, never touched the prefix, and passed. A test written from the same assumption as the code cannot find the assumption. The pattern now carries the `i` flag, which covers the whole string, and the test walks three spellings including a lower-case prefix and a mixed-case one.
|
Code review by qodo was updated up to the latest commit 951ef54 |
Every boundary this project files rests on one comparison, and a wrong answer here does not fail anywhere downstream. It produces a well-formed correction that is wrong, which is the single outcome the rest of the design exists to prevent. So this is hand-written, and then checked against the reference implementation. The trap it exists for: there are two comparisons in Python packaging and they disagree. PEP 440 ORDERING places a pre-release below its own release, so a range of "< 1.4.0" contains 1.4.0rc1. pip's SPECIFIER MATCHING excludes pre-releases unless asked for, so the same range would not match it. OSV ECOSYSTEM ranges are ordering -- the question is whether a published version was affected, not whether pip would install it. Built on the other one, this project would disagree with the published record on every advisory whose boundary is a pre-release, and the disagreement would look like a finding rather than a bug. Dependency-free because rangecore runs in a sandbox with no installer. That is a deployment constraint and not a licence to be approximately right, so tests/test_version_vs_reference.py imports `packaging` -- never a runtime dependency -- and compares the two over every version string OSV publishes for PyPI in the pinned snapshot: 43,265 strings, 41,214 parseable, zero disagreements about which strings are versions and zero disagreements across 200,000 sampled ordering comparisons. The committed test samples 20,000 with a fixed seed and skips cleanly when `packaging` or the snapshot is absent. Three cases a naive tuple comparison gets wrong, each covered: trailing zeros carry no meaning (1.0 == 1.0.0); a dev release precedes its own version's pre-releases (1.0.dev1 < 1.0a1) while an absent dev sorts above any dev number; an absent post sorts below any post number. An unparseable version raises rather than sorting. Ordering a version wrongly produces a boundary; refusing produces an abstention, and abstention is the design.
|
Code review by qodo was updated up to the latest commit 54b354c |
Both findings stand. The Version.local comment said "compared only for equality, never for ordering" while _key() includes local segments and the suite asserts 1.0 < 1.0+local. PEP 440 does order local versions above their plain form; the comment described an earlier intent and was never updated. That is the third comment in two days claiming behaviour the code does not have -- the same shape as the idempotency row in AI_USAGE.md and the case-insensitivity note on the GHSA pattern. Worth naming rather than quietly fixing: the comment gets written from the intent, the implementation moves, and nothing reconciles them. The reference test filtered on the local parser only, so an entry the reference parser rejected would reach `ra < rb` and raise TypeError rather than failing through the parity assertion that explains the divergence. Test execution order is not guaranteed, so it could not rely on the parity test running first. It now requires both.
|
Code review by qodo was updated up to the latest commit fe2e3fd |
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
1 participant
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.
Three pieces of the agent, and the measurements behind each.
bench/select_corpus.py— the corpus rule as code rather than prose, against an OSV snapshot pinned by sha256. Three arms: six advisories where PyPA and GitHub describe the same CVE differently, eight where they agree exactly, and eleven where no fix commit is published and the answer has to come from repository history. A corpus made only of records already known to be wrong cannot measure how often the agent wrongly corrects something, so the second arm is not optional.mcp-introduced/— the only process holding a token that can write. It exposes two tools and does no reasoning;rangecorereasons and holds no credentials. Both tools are annotated so the harness stops for a person before either fires, and a test asserts that, because losing an annotation does not fail loudly.rangecore/wheel_spike.py— the sandbox has no package installer, so a behavioural probe either works by acquiring built wheels or the evidence tier does not exist. It works: certifi 2024.6.2 → 2024.7.4, probe fired at the vulnerable release and not at the fixed one. 7.4% of candidate releases can carry a probe at all; dependencies, not wheels, are the constraint.Two mistakes worth reading the tests for: a probe that returned the same verdict at both releases was caught by its own negative control rather than reported, and an eligibility pass zipped a dict against a differently-sorted map, so every verdict was attached to the wrong package. Both now have regression tests.
One test is marked
todo: the write path is not yet idempotent, and tool execution is at-least-once across a crash inside the write window.