Repository navigation
feat: add quirk:full-code-review - #50
Merged
Merged
Conversation
Copies the sakura skill (minus provenance and Recharge repo guidance) and its 418 tests into quirk unchanged except for import paths, as the first step of the port described in docs/quirk/specs/2026-10-15-full-code-review/.
…lab calls into it
…MR refspec into forge.gitlab
…ls into forge.gitlab Every glab invocation now lives in scripts/forge/gitlab.py. Scripts keep their module-level names and pass late-bound runners, so all 418 ported tests pass without edits.
… prepare_diff through it Adds forge.detect_forge (--forge override, github.com/gitlab.com, self-hosted classified by gh/glab auth), drops the hard-coded default GitLab host, and switches prepare_diff to a Forge object. Tests retargeted to the neutral names.
… report, and triage Context uses change/number/forge and schema 1; reports carry schema_version, meta.target "change", meta.forge and meta.number; labels come from one forge.change_label (PR #N / MR !N). Adds a page/validator label round-trip test.
…lenses, generic risk rubric Moves references/dimensions to references/lenses with bare lens IDs and YAML front matter, splits the test checklist into test-adequacy, tests, test-coverage and a shared mutation-probe file, strips org framing and examples, replaces the risk tiers with blast radius, reversibility, rollout safety and repo-declared critical paths, and routes cross-repo search through sibling clones or a pack search hook.
…ger, and borrowed refs Posted-comment markers become quirk-review:v1 (the legacy one-line marker is no longer parsed), the ledger lives under $XDG_STATE_HOME/quirk/review-memory, localStorage keys, the triage handoff delimiter, borrowed refs and temp prefixes use quirk-review, test fixtures use bare lens IDs, and the bundled sample report is neutral.
discover_guidance reports only the reviewed repo's own convention files; add_guidance appends lessons to REVIEW.md, the project pack, or the user pack and prints a commit hint without committing.
…d guard tests SKILL.md drops to 379 lines and routes detail to references (posting, degradation, orchestration). The last org and origin-plugin names are gone from the skill, enforced by test_no_org_names.py; test_full_code_review_skill.py checks the line budget, bundled paths, lens front matter, and roster consistency.
resolve_policy.py merges the built-in lens roster, the user pack (or --pack), the project pack, and REVIEW.md front matter; reads reviewer-seat policy from the target ref; and with --apply drops linguist-generated and skip-glob files from the context. Adds an example org pack that replaces the risk rubric and adds a lens.
…and the posting floor render_report validates the schema_version 1 run keys, marks a report with failed lenses incomplete, refuses it a clean verdict, and shows the failed lenses, models, floor, and excluded files. Findings below run.severity_floor (default Should fix) render but never post from either seat.
… trust rule prepare_diff reads commit authors, stops with seat "ambiguous" when you and another author both committed unless --seat is given, rejects --seat otherwise, and records seat_source, is_fork and author_trusted. borrow_review_source refuses mutation consent on forks and untrusted authors; reviewer delivery allows a chosen reviewer seat on your own co-authored change.
…p into SKILL.md Adds references/policy.md (layers, pack layout, settings, generated files) and teaches SKILL.md the resolve_policy step, --pack and --seat, the mixed-seat question, the probe trust rule, incomplete runs, and the posting floor.
…ivery Adds forge/github.py (pull request reads with merge-base refs, files with missing-patch recovery, commit authors, fork and trust fields, review threads normalized to the shared discussion shape, author inline comments, and reviewer staging as one atomic pending review with GraphQL support replies). post_reviewer_review, review_memory and borrow_review_source now dispatch through the adapter named in the context.
…ment GitHub delivery post_author_comments.py posts the approved author batch through the adapter, skips lines outside the diff, and writes receipts as it goes. posting.md and SKILL.md describe the GitHub pending-review flow and the scripted author path.
… for collapsed diffs GitLab write calls send Content-Type: application/json. Files whose diff the forge collapsed or truncated are recorded as unanchorable_files, and both posting paths list their findings in the overview instead of creating inline notes that GitLab drops.
reconcile and check take --repo so memory identity comes from the reviewed repo, not the caller's cwd; SKILL.md exports REVIEW_OUT_DIR, applies run_when conditions (now tabulated in orchestration.md), re-dispatches lenses rejected by a concurrency limit, and quiets reconcile's stdout; pytest no longer collects the smoke repos.
zpyoung
changed the title
Fixes the review's will-fix findings that live inside one area: skill docs, the HTML report and renderer, the GitHub adapter and diff preparation, the GitLab adapter and posting, review memory and borrowed source, and policy and guidance discovery. Cross-cutting refactors (shared JSON writer, forge boundary, label tables, test fixtures) follow in a separate commit.
Fixes the confirmed findings from a --deep full-code-review of PR #50: sandbox hardening (ssh-agent socket, home read denial, bounded output, read-only release), deep merge gating and validation, review-memory fingerprints and C-quoted paths, forge/posting identity binding and stray-comment recovery, policy and guidance trust edges, render/triage anchor and fence safety, and diff/YAML/docs correctness. The size-gate finding is left for the separate size-gate change.
Fixes the confirmed findings from the second --deep review: markdown emphasis and fence parsing, BOM highlighting, sandbox timeout status and denied-stat handling, YAML flow-only quote openers, glob-safe guidance discovery with symlink-cycle protection, layered lens model resolution, GitHub pending-comment filtering and orphan cleanup, receipt-loss reporting, author-report binding without head_sha, deep-merge dedup and verification labelling, review-memory surrogate encoding, --local nested repos and subdirectory runs, area-map import edge cases, and a private copy per mutation probe, with the missing negative tests. The size-gate finding is left for the separate size-gate change.
Fixes the confirmed findings from the third --deep review: linear-time markdown emphasis, surrogate-safe rendering, budget-overrun banners, plausible caveats in the triage handoff, shallow-clone and TMPDIR-safe sandbox leases, checker output caps and script-path inputs, YAML escape and tab handling, CR-preserving git output, git path escape decoding, area-map caveats and test detection, a non-destructive mutation-probe revert, a file-based cross-family prompt, Python 3.9 glob compatibility, live-change rechecks before author posting, kind-aware GitHub cleanup, and the missing review-memory negative tests. The size-gate finding is left for the separate size-gate change.
Large diffs are batched by file and never truncated, so a size refusal only blocked reviews the pipeline can already handle. The report still states the post-exclusion size, and SKILL.md steps are renumbered.
Sample report conflicts took the fix branch's rewritten sample, which no longer carries a Large PR note. The deep logic and tech specs drop their size-gate scenario and non-goal, since the gate is gone.
…k coverage Areas are capped at 600 changed lines instead of 1,800 whole-file lines, and each area now lists the diff hunks its finder must cover, a hunt priority, and the files with no hunks. An optional capacity estimate says whether the budget covers every area at the given concurrency (8 minutes per area until recall runs calibrate it).
…ow delta reviews The deep merge marks an area incomplete when its finder left hunks unexamined or it was skipped for capacity, and reports hunk coverage. Going over the time budget is a note, not an incomplete review; recall scoring still fails it. The report validates and shows run.since for delta reviews, and the triage handoff asks for a failing-first test per fix, tests on every CI runtime, and a --since re-review of the fix commits.
prepare_diff.py --since REF narrows the reviewed diff to changes after REF while refs keep describing the full change, so policy reads and forge positions stay correct. Review memory and both posting scripts refuse a delta context or report.
… shell guidance deep.md plans capacity before hunting, records examined hunks per area, sends follow-ups for gaps and spends leftover time on a second wave. Area finders report examined hunks and take an id prefix and angle. SKILL.md adds --since for report-only delta reviews. Agent prompts warn that the shell may be zsh. A root REVIEW.md keeps the answer-key fixtures out of reviews.
An empty --since value now fails instead of silently running a full review, and every --since error suggests running without it. The capacity estimate no longer crashes when the budget-to-finder quotient overflows.
…posed The first deep run missed three issues that were already in the code, and the second run raised a false cross-repo claim. The fixes: - test-adequacy now asks for a test that goes through the caller whenever the diff passes a value from one function to another. - The mutation probe and area finders add a call-site mutant: replace the argument with its default or None at the caller. - Every lens works through its whole checklist and reports every qualifying finding, not only the strongest. - The cross-repo lens always keeps the caller checkout's parent directory as a sibling root. Extra roots can be added but never replace it, and checkers get the same roots. New tests pin each instruction. The tech spec's sibling-scan line now matches the current grep form.
Headless runs on small diffs reviewed the change inline, skipped the lenses, and wrote no report.json, and some replaced a caller-set REVIEW_OUT_DIR with a fresh mktemp. Step 4 now says diff size never shortens the run, dispatches one Agent per lens, and allows a skip only on run_when, policy, or a failed dispatch; a small diff, a headless run, no forge, or cost never counts. Setup says to run the block verbatim and keep the :- default. Two tests pin the wording. BUGS.md logs BUG-12 and BUG-14 and resolves BUG-13.
zpyoung
marked this pull request as ready for review
October 19, 2026 13:26
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
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.
Adds
quirk:full-code-review, an org-neutral port of sakura's review-mr-combined: up to 14 review lenses run in parallel and merge into one severity-ranked report with an HTML triage page, and only findings a human approved get posted. It works on GitHub PRs, GitLab MRs, and local changes. Specs are indocs/quirk/specs/2026-10-15-full-code-review/and the executed plan isdocs/quirk/plans/2026-10-15-full-code-review.md.Deep mode
--deep [minutes]now adds bounded area finders, a cross-file contract pass, and fresh candidate verification. The default budget is 60 minutes. Trusted, consented repros use independent throwaway source copies; standard lenses remain read-only in deep mode. Plausible findings stay visible but cannot be posted.Areas are capped at 600 changed lines, and each lists the diff hunks its finder has to account for. Before hunting, a capacity estimate checks the budget. If the budget is short, an interactive run asks once whether to raise it, hunt only the top-priority areas, or continue; a headless run hunts the top-priority areas and lists the rest as not hunted. Finders report which hunks they examined, gaps get a follow-up finder, and leftover time goes to a second wave. An area with unexamined hunks makes the review incomplete. Going over the budget alone is a note, though recall scoring still fails it.
Deep-mode recall acceptance has not passed. The two required runs at
3605fd8777b60646bcad11aba08f83c2c858e6b9remain blocked by Anthropic Sonnet OAuth refresh (invalid_grant), tracked inTEST_BACKLOG.mdas TEST-3. The corrected answer key has 48 defects. Earlier seeded/partial attempts were invalidated and do not count as acceptance runs.Deep-mode implementation checks
The synthetic CLI and macOS browser smoke exercised sandbox execution, checked/dropped claims, area mapping, triage export/validation, and a plausible finding marked Will fix remaining non-postable. Scoring rejects missing timing, target, or spot-check evidence. A bounded branch review and closure pass also ran. These checks establish implementation behavior, not measured defect recall.
Standard review and verification
What's in it:
scripts/forge/(GitHub and GitLab adapters); the forge is detected from the git remote, with--forgeto overrideresolve_policy.py), read at the merge base in the reviewer seat; an example org pack is included--statsshows per-lens keep/dismiss rates; a five-case smoke corpus withsmoke_check.py--since <ref>reviews only the changes after a ref, for checking fix commits. It is report only: review memory and both posting scripts refuse a delta run. The triage handoff asks for a test that fails without each fix, the tests on every CI runtime, and a--sincere-review of the fix commitsREVIEW.mdkeeps the answer-key fixtures out of reviews, and the deep agent prompts warn that the shell may be zshdocs/is left out because the porting notes name the source on purpose)skills/filing-requests/scripts/_yaml_mini.py, which must stay byte-identical to the skill'spolicy_yaml.py, so its YAML fixes land in filing-requests too (block chomping, sequences at the key's indent, apostrophes inside plain scalars)The skill has reviewed this PR twice in standard mode. Three earlier fix commits applied the first run's 63 findings and the second run's 71 (19 Should fix, 52 Nits); the second run found no Must fix. The changes in behaviour:
--policy-refand exit 2 without it; diff-only runs use the new--diff-onlyflag. Policy is read at the merge base, so policy added to the target branch after that point doesn't applybodies.jsonbuilt from a different report, and the--overviewreceipt goes to<out>.overview.jsonfallback_reasonand a newmixed_authorship_notechanged_pathsalso runs the reverse compare when the branch is behind or diverged; GitLab comments on unchanged lines send both line numbers\nonly, and rename paths are quoted and decoded the way git does--exclude-changedfails closed without a changed-files list, and generated-file detection handles non-ASCII paths and long path listsrun_whenis judged once per change, and lenses never run tests or the mutation probe unless the dispatch enables itforge.parse_originkeeps the port for http(s) remotes, so two instances on one host no longer look like the same repo--outskips comments that already have a receiptThree more
--deeprounds then ran against this PR, with a fix commit after each (401104c,db8a73f,b7264ab). Round 1 found 1 Must fix and 45 Should fix; rounds 2 and 3 found no Must fix and 37 and 35 Should fix. Each commit message lists what it fixed. The main areas were sandbox hardening (ssh-agent socket, home reads, output caps, shallow clones, TMPDIR), deep-merge gating and dedup, review-memory encoding, forge identity binding and orphan-comment cleanup, Markdown and YAML parsing edge cases, and render and triage safety. Round 3's fixes have not been reviewed by a fourth round; the test suite covers them. These rounds ran standard lenses one agent per batch and some agents saw the answer-key fixture, so they don't count toward the TEST-3 acceptance runs.How it was verified:
python3 -m pytest -q: 2924 passed, 1 skipped;/usr/local/bin/python3.9 -m pytest tests/full_code_review -q: 1593 passed, after the coverage-planning and--sincechanges. Three Codex lenses (correctness, spec, security) reviewed those changes; the four accepted findings were fixed in4a20c93, and a closure pass on that fix led to880e346. CI installs Pygments so the highlighting paths run there, and now runs on Python 3.9 and 3.12 with bubblewrap installed. The job fails if the sandbox can't isolate, and both versions pass there.gh/glabbinaries confirmed no test reaches a real CLI (run before the review fixes)Not covered yet: no live review in the reviewer seat on either forge, and none on a real GitLab MR. Posting is covered only by fakes, and GitLab's two-number position for unchanged lines is checked against the docs only.
Changelog
Add
quirk:full-code-review, a multi-lens pre-merge review for GitHub, GitLab, and local changes with author and reviewer seats, policy packs, human-gated posting, an opt-in--deep [minutes]defect hunt with per-hunk coverage planning, fresh verification and evidence-aware scoring, and report-only--since <ref>delta reviews.