Repository navigation
Close the adversarial gaps with no PR yet - #41
solvyxtech wants to merge 6 commits into
Conversation
…says receiptEvidence cut the receipt at the first '## Output' anywhere, and the worker's claim is quoted above the check table, so a claim holding that heading blanked the commands, diffs and check results the reviewer weighs. Cut only at the receipt's own whole-line heading after the check table, and let the cap trim the worker-influenced text above the table, never the table.
The driver imported the deliverable in-process and read exit 0 as every input matched, so a deliverable that called os._exit(0) or sys.exit(0) on import passed without a value being compared. The deliverable now runs in a child that reports each result on nonce-tagged lines; the driver compares them to the expected values it kept and exits 0 only when every agreed input has a matching result. An early exit, by any code, fails the work rather than passing it or retiring the check.
test_count.py at a project root was not a test path, self.assertEqual(...) was not an assertion, and a skip decorator left every assertion's text in place, so rewriting an assertion into a tautology or skipping the test passed spec-intact. Test paths now follow each runner's naming, assertion methods count, and a skip, xfail or .only added to a test file is reported as a weakening through both the tool ledger and the disk snapshot.
A worker can read the project's example files and special-case them; a check that feeds the work nothing else is met by doing so. The drafter is now asked for at least one check on an input it writes into a temp folder.
Three ways a sound-looking check passes over wrong work, each readable off the tree at the claim (src/control.ts): - the runner was shadowed: a conftest.py, pytest config, a module named after the one run with -m, or a node_modules/.bin entry planted this turn; - the expected value was the worker's: a diff against an expected file, or a test snapshot, the worker created or changed this turn; - a drafted check from another model whose only input is a project file the worker could read and tailor its answer to. Such a check still passes and shows on the receipt, but tierOf does not count it toward verified and names the file. A subverted check in the person's bar voids the person basis for the rest of that bar. Drops the todo markers on the shadow-runner, worker-written expected.txt and visible-input-only fixtures, and the six 'new (no PR yet)' gap entries.
solvyxtech
left a comment
There was a problem hiding this comment.
Verdict: changes requested (3 blocking, 6 non-blocking). Review only, nothing merged.
The direction is right, and most of it holds up: the child-process driver, the nonce-tagged results, the cut that waits for the check table, and discounting checks instead of refusing them. Below are the holes I could reproduce against 898ae6f.
Blocking
1. spec-intact now refuses legitimate new tests that carry a conditional skip. Confidence: high (reproduced).
- Where:
src/engine.ts:3323(specWeakened(priorText, content), wherepriorTextis""for a new file) andsrc/files.ts:458-485(SKIP_MARKER, includingskipifand[{,]\s*(?:skip|todo)\s*:). - Scenario: the worker adds a new test file, or a new test in an existing file, with
@pytest.mark.skipif(sys.platform == 'win32', ...)orit('posix', { skip: process.platform === 'win32' }, ...).specWeakened("", <that file>)returns the skip line as "(turns a test off)". The ledger entry getsspecRemoved, andspec-intactfails with "This turn deleted or turned off 1 assertion(s)". - Why it matters:
spec-intactis in the barmaat initproposes (src/detect.ts:250), and the check's own comment says "Adding tests is free." This repo's tests use{ skip: !hasPython && "needs python3" }, so a Maat turn that adds a fixture to molt itself would be refused. - Fix: count a skip only when it lands on a test that already existed before the turn, or is unconditional on an existing test. Treat skips inside a brand-new file (
existed === false) or attached to a newly added test as free.
2. Fixture 13 (shadowed runner) can still be beaten through node_modules. Confidence: high (reproduced).
- Where:
src/control.ts:163-166. A.binsymlink whose target is insidenode_modulesis treated as a package's own link, whoever made the target. - Scenario:
mkdir node_modules/x && printf '#!/usr/bin/env node\nprocess.exit(0)\n' > node_modules/x/cli.js && ln -s ../x/cli.js node_modules/.bin/jest, with the bar'snpm testrunningjest.plantedBinsreturns[],discountedChecksreturns nothing, and the person's check still earns "verified (your checks)" over wrong work. Editingnode_modules/jest/bin/jest.jsin place is the same attack with no.binchange at all. - Why it matters: the README now marks 13 as held, but only the plain-file
.bin/nodeshim is covered. - Fix:
realpatheach.binentry, and flag it when the resolved target (or its package directory) was created or changed aftertakenAt. Better still, flag anynode_modulespath changed this turn that a script's bin resolves into.
3. A deliverable can still retire the reference check by raising a BaseException subclass. Confidence: medium-high. I reproduced the exit 3; I didn't trace the claim-time retire end to end.
- Where:
src/reference.ts:214(the child reportsraised) and:260(the driver turns that intoown(...), which is exit 3). - Scenario:
solution.py=class Out(BaseException): pass/raise Out(). The driver exits 3, and per the header comment, a 3 at a claim retires the check. Arun_deliverablewritten with the usualexcept Exceptiondoesn't catch it. - Why it matters: the PR body says an early exit "no longer passes the work or retires the check"; this one-line variant of the same fixture-12 family still escapes the comparison.
- Fix: in the child, report a non-
ExceptionBaseExceptionfromrun_deliverableas the work failing on that input, the same wayexitedis handled, rather than as the reference's own error.
Non-blocking
a. control.ts fails open on big or unsnapshotted trees. Confidence: medium-high.
- Where:
src/control.ts:52and:249. - When the snapshot is truncated (more than 20k entries) or missing,
touchedThisTurnfalls back to the tool ledger alone, so aconftest.pyorexpected.txtplanted through bash is missed.plantedBins(…, +Infinity)also flags nothing. - Fix: stat the specific hook and expected-file names directly against the turn start time.
b. suppliesOwnInput treats any echo or random as the check making its own input. Confidence: high (verified).
- Where:
src/control.ts:204-205. test "$(python3 count.py data.txt)" = "3" || { echo wrong; exit 1; }returnstrue, so rule 3 never fires on the very common|| echopattern.- Fix: require the generated text to actually feed the work, through a pipe, a here-doc, or a redirect into a file the command then passes.
c. The driver comment says the expected values are kept from the child, but they aren't fully. Confidence: high (verified). This predates the PR.
- With
same(), the expected values are pickled into the child. - Separately, the deliverable can find
check.pythroughsys.argv[0]and callreference()itself. A wrong solution that does this passes with exit 0. - This was already possible in-process, so it isn't a regression. Worth a todo fixture and a softer comment. Running the child from a copy that holds only
run_deliverable, or making the reference directory unreadable under #29's user split, would close it.
d. The idx fallback re-imports check.py in the child. Confidence: low. Nondeterministic INPUTS would then disagree with the parent's expected values and cause false failures. This path only runs for inputs that can't be pickled.
e. receiptEvidence still parses markdown. Confidence: low. Paths in "What the model changed" go into table cells raw, so a filename with embedded newlines could plant a whole-line heading. Escaping newlines in receipt paths would close it.
f. The wider isTestPath also changes src/engine.ts:4651 and :4673. Confidence: medium. After a check is revealed, writing a root test_*.py, a conftest.py or a spec/ file is now a hard refusal. That's probably intended, but it belongs in the PR description.
Verification and pairing rules
- I ran
npm run checkon this branch, on Node 20: 2112 tests, 2099 pass, 0 fail, 13 todo. Every remaining todo points at #23, #27, #29, #32, #33 or #34. npm teststill exited non-zero because Node 20's coverage reporter crashed ("Could not report code coverage"), which looks environmental since CI uses Node 22. So e2e and self-check didn't run here.- No checks ran on this PR, because its base is a
grok/branch. - Pairing rules: clean. The diff has no competitor names, no subscription backends and no fine-tune data changes, and the claude/ to grok/ PR flow is correct.
solvyxtech
left a comment
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Prior items: 0 of 3 blockers fixed. These are still open:
spec-intactrefuses legitimate conditional skips (pytestskipif,{ skip: !hasPython }).- The shadowed-runner fix can be beaten with a
node_modules/.bin/jestsymlink to a worker script, or by editing jest in place. - A
BaseExceptionsubclass raised on import makes the reference driver exit 3 and retire the check.
Non-blocking items a–f are still open as well. The head is still 898ae6f, the commit I reviewed in 5450255800, and the base grok/adversarial-fixtures hasn't moved.
Fresh pass from Grok, comment only. I re-ran it at 898ae6f: npm run typecheck is clean, and the suite gave 2112 tests, 2099 pass, 0 fail, 13 todo. Same as before, every todo points at an open PR. No checks run on this PR because its base is a grok/ branch, and the account-wide billing block would stop them anyway. I found nothing new in the code.
One stack note, non-blocking and high confidence: this branch sits on the adversarial-fixtures line (#39 into #32's branch), and #42–#46 have since stacked on claude/discriminating-verified. A trial merge of this PR into claude/value-classifier conflicts in src/engine.ts, src/reference.ts, src/tiers.ts, test/harness.test.ts and test/reference.test.ts. It's worth deciding the landing order before more work piles on either line.
|
Round-3 review, item by item. Fixed in claude/integration-all@3d1ca16 (#49); this branch is superseded by integration.
|
Closes the nine adversarial fixtures in #39 marked
new (no PR yet). Theirtodomarkers are removed, along with the sixGAPentries they used. The todos that point to #23, #27, #29, #32, #33 and #34 are untouched.What changed
Reviewer evidence (11):
receiptEvidencenow cuts only at the receipt's own whole-line## Outputheading, after the check table. A claim containing that heading is quoted with>and can no longer end the evidence early. When the receipt is too long, the cap trims the claim and the text above the table, never the table itself.Reference driver (12): the deliverable now runs in a child process. The child reports each result on a line tagged with a per-run nonce. The driver compares those results to the expected values it keeps to itself, and exits 0 only when every agreed input has a matching result. An early exit fails the work, whatever its exit code; that includes
os._exit(0),sys.exit(0)andos._exit(3). It no longer passes the work or retires the check.spec-intact (01): test files are now recognised by each runner's naming convention (
test_*.py,*_test.go,conftest.py,__tests__/, and so on). xUnit assertion methods (self.assertEqual(,assertEquals(,assert_eq!() now count as assertions. Adding a skip, xfail or.onlycounts as weakening the spec. Both the ledger route and the disk snapshot route catch it.Discounting worker-controlled passes (02, 04, 13): new
src/control.ts. It reads the tree at claim time and marks a passing check as worker-controlled when:conftest.py, pytest config, a module that shadows the one run with-m, or anode_modules/.binentry that is not a package link;tierOfdoes not count these checks toward "verified", and the reason names the file. If one of the person's own checks is subverted, the rest of that bar no longer stands in for the person. Nothing is refused: work that legitimately adds a conftest fixture keeps its claim and loses only the label.The drafter prompt now asks for at least one check whose input the check writes itself.
Tests
test/control.test.ts.review.test.ts,reference.test.ts(runs the real driver against early-exit deliverables) andcommandments.test.ts.npm run checkis green with~/.local/binoff PATH: 2110 tests, 2099 pass, 0 fail, 11 todo, then e2e and self-check.Side effect: the
#34todo "a judge's print(x == y) check" now also passes, because its only input is the visibledata.txt. Its marker is left for #34.🤖 Generated with Claude Code
https://claude.ai/code/session_01D63hP8FwZTxcamaLRq4jRt