[pipeline-dcdad92d][slice-12/12] P2 items (R13, R14, R15, R16, R17) - #207
Merged
Conversation
🦋 Changeset detectedLatest commit: 71e3354 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
egg/pipeline-dcdad92d/slice-11
branch
from
July 3, 2026 21:37
ee3bc58 to
d7427e8
Compare
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-12
branch
from
July 3, 2026 21:37
2c4b5bc to
bbb4d55
Compare
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-11
branch
from
July 3, 2026 21:42
d7427e8 to
41c87b4
Compare
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-12
branch
from
July 3, 2026 21:42
bbb4d55 to
e73288b
Compare
…ing (slice-1) R1 (task-1-2): standardize Step 6 review submission on one robust submit-pull-request-review call with a real, non-empty body; remove the empty-body branch and any fallback/retry path. E2 (task-1-3): stage /tmp/gh-aw/review/pr-context.json in Step 1 and wire all six sub-agent read blocks to consume it. Extends #194 diff staging; foundation for slice-7 lenses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
task-1-1: workflows/review/lib/finding-schema.ts - FINDING_SCHEMA_VERSION exported constant; each finding carries schema_version and the validator rejects an unrecognized version (breaking-shape guard). - Finding shape (snake_case wire keys, matching #194 per-run JSON artifacts): id, lens, anchor, severity, confidence, evidence_trace, optional suggested_patch, optional pre_merge_obligation, producing_hunt, model_authored_prose. - Anchor is a discriminated union with the required PR-level type (pr | file | line, line supports side + multi-line start_line..line range). - KNOWN_LENSES (11 specialist + always-on/triage) and SEVERITIES exported for reuse by the slice-3 router and slice-2 verdict; lens/severity validated against them. - validateFinding collects ALL violations (diagnosable per-lens drop rate), plus isValidFinding narrowing wrapper and throwing assertFinding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-11
branch
from
July 3, 2026 21:43
41c87b4 to
21b1988
Compare
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-12
branch
from
July 3, 2026 21:43
e73288b to
3ce90f4
Compare
jwbron
marked this pull request as ready for review
July 3, 2026 21:44
khan-actions-bot
requested review from
a team and
kevinb-khan
and removed request for
a team
July 3, 2026 21:44
…nts) - Restore the empty-review-body convention: with inline comments present the verdict is submitted with an empty body; a non-empty body exists only for comment-less reviews and skipped-dimension notes. Keeps the single-call submission rule. - Deliver workflows/review/lib/ to consumer repos: gh-aw's source: import only copies the .md, so the job checks out Khan/actions at the pinned review-v* release (single version surface for prompt + code) and installs its runtime deps. - Drop planning identifiers (E2, R8, slice numbers, operator-directive note) from shipped file comments.
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-11
branch
from
July 6, 2026 18:19
21b1988 to
0a45c34
Compare
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-12
branch
from
July 6, 2026 18:19
3ce90f4 to
595641f
Compare
jeresig
requested changes
Jul 7, 2026
jeresig
left a comment
jeresig
left a comment
Member
There was a problem hiding this comment.
Pushing back on the version stamp stuff as I'm not sure all that logic is needed as we're already versioning things.
| the `actions/` packages use), so the nested `workflows/review/review.md@<ref>` path | ||
| resolves for `gh aw add`. | ||
|
|
||
| ### Config drift guard (the version stamp) |
Member
There was a problem hiding this comment.
I don't think we need this - if the behavior changes then we should do a major release. If we're still on the same major version then we should assume that the fundamental behavior hasn't changed.
| @@ -0,0 +1,361 @@ | |||
| /** | |||
… rendering (slice-2) task-2-1/2-2 verdict.ts: pure computeVerdict(labels + dimension availability + policy conflicts) -> APPROVE | REQUEST_CHANGES | HOLD_FOR_HUMAN. Consumes #194's mechanical label rule (isBlockingLabel); does not re-implement it. R2 gate: a missing core dimension (correctness/skill-severity) or a policy-named conflict holds for a human; a lost pattern-triage is note-and-continue. Documented default blocking threshold = 1 (tunable, not a HITL gate). task-2-3 render-comment.ts: code-owned Conventional-Comment label taxonomy + deterministic label-wrapping/templating from the finding schema; models own all prose. Review-body rendering mirrors review.md Step 6 + skipped-dimension notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pshot tests (task-2-4) Truth-table tests for computeVerdict (verdict.test.ts): APPROVE / REQUEST_CHANGES / HOLD_FOR_HUMAN precedence, the R2 core-dimension gate (correctness + skill-severity absence holds; hold dominates a blocking label with all reasons retained), pattern-triage note-and-continue, policy-named conflicts (verbatim detail passthrough), blocking-threshold clamping (>=1) + never-RC-on-zero, and purity (deterministic, non-mutating). Rendering snapshot tests (render-comment.test.ts): isBlockingLabel over the full label taxonomy + unknown-label safe default; labelForFinding severity x lens matrix (conventions => best-practice, specialists => plain); renderComment inline snapshots (label wrap + verbatim prose + optional suggestion block) with verbatim prose/patch passthrough asserted; renderReviewBody inline snapshots for every verdict x inline branch + skipped-dimension notes + non-empty safe-output guard. Fixtures validated through assertFinding so tests can't pass on schema-invalid findings. 50 tests green; full pnpm test suite 253/253; tsc --noEmit clean (node24). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…den aggregate join Addresses reviewer_code_holistic v1 NACK (dangling cross-slice build dependency): judge.ts imported `DownvoteReason` from ../lib/thumbs-sweep, but slice-8 (the thumbs sweep) is not in slice-11's ancestry, so that module does not exist on this branch. The judge consumes thumbs labels as data and never needs the sweep module at build time, so the import was gratuitous coupling. Declared a local `DownvoteReason` union (structurally identical to slice-8's) so judge.ts is self-contained and typechecks against the branch as it stands. Also cleared the two non-blocking advisories while re-proposing: - aggregate() now rejects duplicate finding ids across requests (the join is by bare finding id, only case-unique in the schema — a collision would mis-join), and throws on a score whose finding id matches no request. The docstring now matches the code (previously claimed an unknown-id throw that did not exist). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sk-11-6) Adds workflows/review/eval/suite.test.ts, the task-11-6 deliverable: - Suite self-tests over the coder's slice-11 modules (loader four-dataset load, five metrics, adversarial/overfitting gates, LLM-judge with a STUB model, and the version-stamp drift surface), exercised against the real corpus plus focused synthetic fixtures built through the public parseCase/runCase path. - Whole-suite green-on-baseline check: every corpus case meets its expected block; must-catch recall = 100%, clean false-block = 0, adversarial hard gate passes (automatic mode allowed). - CI-wiring guard: the smoke subset is a strict subset (the fast per-PR pnpm-test gate), and the ONLY model seam is judgeCorpus (injected model, async, off the deterministic path) -- so the live-judge full suite is scheduled/opt-in, never per-PR. Metrics+gates run over the full corpus with zero model calls. Deterministic: no model, no network, no clock, no randomness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1-6, resolves gap-1)
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-11
branch
from
July 7, 2026 20:35
f2efbd9 to
983461b
Compare
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-12
branch
from
July 7, 2026 20:36
64089d7 to
0932cd9
Compare
… is a committed script on a weekly workflow, version-stamp machinery dropped
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-11
branch
from
July 7, 2026 20:40
983461b to
af7ad3a
Compare
…n + R14 drift-guard doc R13 (task-12-1): add the three-way per-finding resolution rule to the thread-reconciler in review.md — on re-review every actionable prior finding resolves as fixed / deferred-to-filed-issue / disagreed-with-reason, else keep. R14 (task-12-2): document the slice-11 version stamp in review/README.md as the single consumer-readable config-drift surface (interface §4.5); reads the existing pr-reviewer:version marker, adds no second mechanism. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er_code NACK v1) Reviewer_code correctly flagged that the README overstated the version stamp as 'already rendered into the posted review comment'. Grep confirms renderVersionMarker/ computeVersionStamp are invoked only in eval/suite.test.ts; review.md Step 6/7 emit no pr-reviewer:version marker. Reworded the 'Config drift guard' section to state accurately: the stamp is produced by lib/version-stamp.ts and exercised by the eval suite today; the marker is the DESIGNATED single drift surface (Step 7 approval comment / committed skill snapshot) but is NOT yet emitted by the workflow — wiring that emit point is pending follow-through. Still adds no new mechanism; no second surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nditional-approval task-12-3 (R15): counters.ts — pure aggregation over #194 per-run artifacts + run summary: validator drop rate per source, comments/PR, verdict mix, thumbs agree rate, cost/run. No new logging mechanism; best-effort fs loader over the conventional artifact layout, defensive normalization from loosely-typed JSON. task-12-4 (R16): dismissal-learning.ts — mines resolved-without-fix threads, 👎-with-reply, and correct-pushback signals into candidate do-not-flag rules, emitted as a unified diff against a committed config for human approval. Never auto-applied; min-occurrences noise guard; deterministic ordering + diff. task-12-5 (R17): render-comment.ts — renderObligationsComment() builds the prominent pre-merge obligations comment from the schema's pre_merge_obligation field (verbatim), for the existing add-comment safe output; APPROVE body reflects the obligation count. Determinism boundary preserved (code owns structure only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing groupKey Addresses NACKs from reviewer_code_holistic, reviewer_contract, reviewer_security, and tester (all the same defect): the groupKey() field joiner and its adjacent comment held 3 raw U+0000 (NUL) bytes, which made git classify the file as binary and hid its diff from BRC review. Replaced the raw NUL bytes with the \0 escape sequence in the template literal and the comment. Runtime delimiter is byte- identical (NUL remains a collision-safe joiner — it cannot appear in lens/path/ label), so this is semantics-preserving; the file is now plain UTF-8 text and diffs normally. No logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…contract); defer dismissal-learning until the thumbs sweep has real signals
jwbron
force-pushed
the
egg/pipeline-dcdad92d/slice-12
branch
from
July 7, 2026 20:41
0932cd9 to
a8492aa
Compare
This was referenced Jul 7, 2026
jeresig
approved these changes
Jul 8, 2026
…/slice-12 # Conflicts: # workflows/review/eval/corpus/smoke/adversarial-injection-moderation.json # workflows/review/eval/judge.ts # workflows/review/eval/smoke.test.ts # workflows/review/eval/suite.test.ts # workflows/review/lib/lenses.test.ts # workflows/review/lib/render-comment.ts # workflows/review/review.md
jwbron
added a commit
that referenced
this pull request
Jul 8, 2026
… conflicts after #207 squash-merge)
jwbron
added a commit
that referenced
this pull request
Jul 8, 2026
#212) Stacked on #207. ## Problem webapp documents sub-tree-specific review expectations in `REVIEW.md` files (a root one plus one per documented top-level directory, next to each `AGENTS.md`), but nothing in the review workflow ever reads them. They are not pulled in by default: - gh-aw runs the Claude Code CLI headless (`claude --print`, no `--bare`), so the repo's `CLAUDE.md` (webapp symlinks it to `AGENTS.md`) IS loaded for the orchestrator and inherited by the inline sub-agents. - But `AGENTS.md` references the contracts as plain Markdown links, not `@`-imports, so they are never inlined; `REVIEW.md` is not a memory filename to Claude Code; and the sub-agent prompts point only at the diff, the `.github/aw/review/` runtime imports, and the skill files. So the review contract only reached the reviewer if a model happened to follow a link. ## Change - `correctness-reviewer`: when the checkout carries `REVIEW.md` files, read the root contract plus the nearest `REVIEW.md` above each reviewed file, and use them to sharpen `riskReason` wording and calibrate finding severity for that sub-tree. - `claim-validator`: use the same contracts to calibrate claim labels (a contract calling a category a nit supports correcting an overstated label). Contracts never decide `verification`; that stays code-evidence-only. - Both steps are conditional on the files existing, so frontend and other consumers without `REVIEW.md` files are unaffected. - README: document the convention and its trust boundary. ## Trust boundary Unlike `.github/` and the agent config folders (restored from the base branch by gh-aw before the agent runs), `REVIEW.md` files are read from the PR head. The prompts therefore treat contract text as guidance that can adjust emphasis but never override the workflow's rules, and an edit to a `REVIEW.md` in the diff is reviewed on its merits under the existing steering-text rule. Author: jwbron Reviewers: jeresig, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ✅ 3 checks were successful Pull Request URL: #212
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
2 participants
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.
Land the P2 follow-ups: per-finding resolution rule, the drift-guard documentation reusing the version stamp, live counters, dismissal-learning candidates, and the conditional-approval verdict.
Base PR: #196
What's in this PR
Commits (5):
This slice
P2 items (R13, R14, R15, R16, R17)
Files affected:
workflows/review/review.mdworkflows/review/README.mdworkflows/review/lib/counters.tsworkflows/review/lib/dismissal-learning.tsworkflows/review/lib/render-comment.tsTasks (5) + acceptance criteria
Stack
pipeline-dcdad92degg/pipeline-dcdad92d/slice-11