Fix the queue, proof and guard defects from the adversarial review - #4
Open
addyosmani wants to merge 6 commits into
Open
Fix the queue, proof and guard defects from the adversarial review#4addyosmani wants to merge 6 commits into
addyosmani wants to merge 6 commits into
Conversation
A required gate the DETECT block never reaches emits neither run nor skip, so the verdict stayed GREEN. gates.conf's own comments invite exactly that config: mutation is gated to deep, and the python arm has no build line. Sweep REQUIRED at the end and record any gate with no verdict as MISCONFIGURED.
Reverting the non-test hunks deletes new implementation files, so a test that only imports the new module fails to load whether or not it asserts anything, and that was reported as PROVEN. Classify the reverted run and report UNPROVEN (exit 3) unless the test actually ran and failed. Also fixes an empty test_paths array under set -u on bash 3.2, and adds pytest's co-located src/utils/test_foo.py layout to the default patterns.
git push origin +main and git push origin mybranch:main both passed the protected-branch regex, which only recognised a destination written as a bare name, refs/heads/, or HEAD:. Match the refspec destination, and treat any +refspec as a force push. Guard .factory/scripts/ as well: prove-test.sh was the one load-bearing script with no write protection. The path check now looks for a write verb or a redirect target rather than any command mentioning a protected path, so running a guarded script and capturing its output still works. doctor now checks that settings.json and block-merge.sh were installed and wired, which install.sh's skip-if-exists can silently leave undone.
The implement routine opened a draft PR unconditionally while verify triggered on pull_request.opened filtered to non-drafts, so stage 3 never ran and every check happened inside the implementer's own session. Drop the draft filter. The skill and ROUTINES also disagreed about what a non-draft factory PR meant. Every factory PR is a draft, matching README and ADVICE.md; the four conditions that used to gate draftness now set Human read required.
…tates Three ways the live queue jammed for good. Nothing cleared factory:awaiting-review and the PR body carried no Closes keyword, so merged work counted against back-pressure forever. The PR body now closes the issue, and back-pressure counts open issues, in-progress included. An abort after claiming released the label but not refs/heads/claude/fq-<n>, so every later run picked the same item and lost the push race to its own ghost. Delete the ref first; if the delete fails, park the item at in-progress rather than advertise something no run can take. Triage no longer re-triages in-progress or awaiting-review items, which could strip a claim mid-implementation. Handoff comments now require a collaborator author, and gate_level is a floor rather than a dial.
Both verification paths now read the PROOF line rather than the exit code. A new module cannot be proved this way at all: report could-not-determine and accept with reservations instead of rejecting or retrying. Carries the untrusted-input rule into the two skills that consume handoff fields, and dates the limit itself in LIMITS.md.
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.
Fixes the twelve findings in #3. All of them reproduced; the notes below are only for the places where the fix needed a decision, or where a boundary moved.
Closes #3
Changed safety boundaries
Negative-test proof got stricter, and slower to satisfy.
prove-test.shused to accept any non-zero exit from the reverted run. Reverting deletes new implementation files, so a test that only imports the new module fails to load whether or not it asserts anything — an assertion-free test was certified as proving the fix. It now classifies the failure and reportsPROVENonly when the test actually ran and failed.The cost is real and worth agreeing to before merging: for a new module, the information isn't there. An import error and an assertion failure are the same observation from outside. That shape now reports
UNPROVEN, the verifier recordscould-not-determine, and the PR lands ataccepted-with-reservationswith a human read. Bug fixes to existing code still get a genuine proof. A test command that fails silently is alsoUNPROVEN, which is a deliberate nudge toward real runners over shell predicates.LIMITS.mdsection 8 documents this.Every factory PR is now explicitly a draft.
ROUTINES.mdand the implement skill disagreed about what a non-draft factory PR meant. README, ADVICE.md, and the charter already say every run ends at a draft, so that's the reading I kept; the four conditions that used to gate draftness now setHuman read requiredinstead. If conditional drafts were the intent, this is the commit to reverse.gates.shsweepsREQUIREDat the end. A required gate the DETECT block never reached emitted neitherrunnorskipand left the verdict GREEN.REQUIRED_FULL="... mutation"andbuildon a Python repo both hit it, and both are configsgates.conf's own comments invite. Anyone who has already tunedgates.confmay find a gate they thought was green is actually MISCONFIGURED — which is the point.The push guard matches refspec destinations.
git push origin +mainandgit push origin mybranch:mainboth got through before..factory/scripts/is now protected too; the path check looks for a write verb or a redirect target rather than any command that mentions a protected path, soprove-test.sh ... > logstill runs.Not fully fixed
Back-pressure is read at run start and the counted label is applied at run end. The count now includes
in-progress, which closes the window two overlapping runs used to slip through, but it isn't compare-and-swap. Claims have one in the remote ref; back-pressure doesn't, and I didn't invent one.Verification
bash tests/run.shpasses. New coverage, each written against the failing behavior first:test-gates.sh— a required gate the stack never reaches must be MISCONFIGURED, and the stock config must still reach GREENtest-proof.sh— load failure and unclassified failure both reportUNPROVEN; the classifier is pinned to sample output from pytest, jest, vitest, go and cargo; co-locatedtest_*files stay out of the revert; the default path runs under bash 3.2test-hook.sh— every spelling of a push at a protected branch blocks, while claiming and releasing a claim branch still workstest-doctor.sh— a repo whereinstall.shskippedsettings.jsonmust not report healthy