review: render the quoted skill rule into the comment the author reads#248
Open
jwbron wants to merge 6 commits into
Open
review: render the quoted skill rule into the comment the author reads#248jwbron wants to merge 6 commits into
jwbron wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 514d9ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jwbron
force-pushed
the
jwbron/review-dispatch-tax
branch
from
8e015ff to
813767c
Compare
… into the comment the author reads
jwbron
force-pushed
the
jwbron/review-skill-rule-quote
branch
from
c415f4b to
7c07a42
Compare
Contributor
Author
|
Branch rebuilt on the refreshed chain (cherry-pick 7c07a42 onto the rebuilt jwbron/review-dispatch-tax; content unchanged apart from prettier formatting). This branch remains the TOP of the batch-two stack. Reset local state to origin before continuing. |
2 tasks
khan-actions-bot
requested review from
a team,
jaredly and
jeresig
and removed request for
a team
Contributor
Review live A/BBaseline:
Improvements (candidate caught, baseline missed)
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
… tmp/skill-rule-quote
…-tax' into jwbron/review-skill-rule-quote
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it. ## Format A case may now carry an opt-in `live` block (`corpus/live.ts`, re-exported through the loader): - `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff. - An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is one case; its tree is never parsed as corpus JSON. - `mustCatchSpecs` / `mustNotFlagSpecs`: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these. Enforced invariants: the `live` tag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear in `changedFiles` and the diff; every non-removed changed file must exist in the tree. `loadLiveCorpus()` returns the subset. ## Cases Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them. The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed `incident-sql-missing-index` in all four arms because live cases carried no `routerConfig` lens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit). ## Test plan: - `pnpm run test --run`: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion. - The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them. - `pnpm run typecheck` and eslint clean on the three touched TS files. ## Next steps (human) 1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines). 2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it. 3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #233
# Conflicts: # workflows/review/eval/corpus/live.ts
Contributor
Review Guidancegithub-actions (3 files)
Excluded from review (2 files)Not individually reviewed — generated, formatting-only, or
|
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.
Summary
Fifth and last PR of the fold-in batch two stack (stacked on #247; the stack is based on #238,
jwbron/review-trial-skill). This branch,jwbron/review-skill-rule-quote, is the TOP of the batch-two stack; downstream work (the re-review mode dial, the webapp preview runs) should base on it.Quote-the-rule requires a lens skill finding to carry the exact rule text, but it lands in
evidence_trace, which never reaches the PR; the author reads onlymodel_authored_prose, a paraphrase of a rule they cannot check.Failure scenario fixed: an author gets "suggestion (non-blocking): Datastore reads here should be strongly consistent per the datastore skill", has no idea what the skill actually says, either takes the bot's word for it or spends time hunting down the skill file, and cannot tell a faithful application from an overreach.
The change:
Findinggains an optionalrule_quote(the exact rule text, verbatim from the skill file). Optional, validated non-empty when present;FINDING_SCHEMA_VERSIONstays 2 (no serialized finding is invalidated).evidence_tracequotes, and the lens output JSON examples name the field.renderCommentand the orchestrator's normalization step surface it into the posted comment as a> **Rule:** <quote>blockquote between the prose and any suggestion block. Only the wrapping is code-owned; the quote is skill-file text copied verbatim, per the determinism boundary.The skill-auditor path is untouched: its prompt already requires both quotes inside
discussion, which posts as-is.cc @jeresig
Testing
npx vitest run workflows/review: 513 tests green (5 new: blockquote rendering and ordering, absent-field behavior, schema acceptance/rejection).npx tsc --noEmitclean.