[pipeline-dcdad92d][slice-2/12] Determinism boundary: computed... by jwbron · Pull Request #198 · Khan/actions

jwbron · 2026-07-03T08:23:03Z

Compute the verdict in code from findings + posted-comment labels (with a hold-for-human path), render Conventional Comments from templates, and hold for a human when a core review dimension produced no output.

Base PR: #196

What's in this PR

Commits (5):

Persist BRC history for slice-2 (#2548)
implement(coder): prettier 2.6.2 lint fixes for slice-2 + finding-schema (tester NACK v1)
implement(tester): prettier-format task-2-4 test files (lint gate)
implement(tester): R8(b)/R2 verdict truth-table + R8(c) rendering snapshot tests (task-2-4)
implement(coder): R8(b) computed verdict + R8(c) Conventional-Comment rendering (slice-2)

.egg-state/brc-history/pipeline-dcdad92d-implement-slice-2.json | 2782 +++++++++++++++++++++++++++
 .egg-state/brc-history/pipeline-dcdad92d-implement-slice-2.md   | 2634 +++++++++++++++++++++++++
 workflows/review/lib/finding-schema.ts                          |   18 +-
 workflows/review/lib/render-comment.test.ts                     |  218 +++
 workflows/review/lib/render-comment.ts                          |  192 ++
 workflows/review/lib/verdict.test.ts                            |  291 +++
 workflows/review/lib/verdict.ts                                 |  195 ++
 7 files changed, 6322 insertions(+), 8 deletions(-)

This slice

Determinism boundary: computed verdict, rendering, missing-dimension gate

Files affected:

workflows/review/lib/verdict.ts
workflows/review/lib/render-comment.ts
workflows/review/lib/verdict.test.ts
workflows/review/lib/render-comment.test.ts

Tasks (4) + acceptance criteria

task-2-1: R8(b): computed verdict in code from findings + posted-comment labels, including a hold-for-human outcome for policy-named conflicts. Consume review: skip redundant approvals, drop skipped-dimensions note, persist sub-agent outputs #194's mechanical label model (do not re-implement it). Pick and document a sensible blocking-threshold default (operator: implementer judgment, tunable later, not a HITL gate).
- Acceptance criteria: Verdict is a pure function of schema + labels; hold-for-human path covered by a test; chosen default documented.
task-2-2: R2: never auto-approve when the correctness pass OR the skill/severity pass produced no output -- hold for a human. A lost pattern-triage may still note-and-continue. This is the gate atop review: skip redundant approvals, drop skipped-dimensions note, persist sub-agent outputs #194's visibility-only note.
- Acceptance criteria: Missing core dimension -> hold-for-human (not approve-with-note); missing pattern-triage -> note-and-continue.
task-2-3: R8(c): templated rendering of Conventional Comments from the schema. Code owns label-wrapping + template; models own every human-read sentence.
- Acceptance criteria: Rendering is deterministic given a finding; no prose synthesis in code.
task-2-4: Tests: verdict truth table (incl. hold-for-human + missing-dimension) and rendering snapshots.
- Acceptance criteria: Truth-table and snapshot tests green.

Stack

Position: slice 2 of 12 in pipeline pipeline-dcdad92d
Base PR: review: roster restructure, determinism boundary, eval suite #196
Stacked on top of egg/pipeline-dcdad92d/slice-1

changeset-bot · 2026-07-03T08:23:07Z

🦋 Changeset detected

Latest commit: 0950bf2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
review	Minor

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

…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>

jeresig

In general seems good but I think we need to have a better flow for helping folks get unstuck when we get into the "holding for human review" state.

jeresig · 2026-07-06T14:09:19Z

+                : "Approved — no blocking issues found.";
+            break;
+        case "REQUEST_CHANGES":
+            head = "Changes requested — see inline comments.";


This can be empty, as well.

jeresig · 2026-07-06T14:10:00Z

+            break;
+        case "HOLD_FOR_HUMAN":
+            head = input.hasInlineComments
+                ? "Holding for human review — see inline comments."


I think we're going to need to provide more context on this. What actions should the creator of this PR take to get their thing un-stuck? (I'm not convinced that we should be going into this other state if we have known blocking changes.)

jeresig · 2026-07-06T14:14:34Z

+    switch (input.event) {
+        case "APPROVE":
+            head = input.hasInlineComments
+                ? "Approved — see inline comments."


I mentioned this in the other PR but we can leave this empty as we're already leaving comments.

jwbron · 2026-07-06T17:13:46Z

Agreed on both. I'll add a precedence flip so known blocking findings always produce REQUEST_CHANGES (the hold state only replaces what would otherwise be an auto-approve on a run where the correctness or skill pass silently produced nothing), and update the hold body to say exactly how to get unstuck: which pass failed, how to re-trigger the run, and who to ping.

…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.

jeresig

Thanks for the changes!

jeresig · 2026-07-07T14:22:00Z

+ * verdict and no next action.
+ */
+const HOLD_UNSTUCK_LINES = [
+    "To get unstuck: push a new commit (or re-run the review workflow from the " +


This seems reasonable to me, for now!

… 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>

Formatting-only follow-up to b2c4d90: applies @khanacademy/eslint-config prettier rules to verdict.test.ts + render-comment.test.ts so the lint check passes on the changed files. No test logic or inline-snapshot content changed — 50/50 still green under node24; tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ema (tester NACK v1) Formatting-only; no logic change. Applies the repo lint gate's prettier 2.6.2 formatting (eslint-plugin-prettier@4.0.0 binds prettier 2.6.2) which had never been run against these files: - render-comment.ts / finding-schema.ts: string-arg + ternary line wrapping. - typeof-indexed-access type aliases dropped the paren form ((typeof X)[number] -> typeof X[number]) — prettier 2.6.2 emits the unparenthesized form; the parens were added by prettier 2.7.0. Semantics unchanged (verified typecheck-clean under the repo tsconfig). Verified: strict typecheck clean; verdict truth-table + render checks still pass; prettier diff against these files is now empty except the exact 2.6.2<->2.8.8 typeof-parens delta, which the pinned CI formatter (2.6.2) resolves in favour of the committed form. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…bodies) - Flip verdict precedence to REQUEST_CHANGES > HOLD_FOR_HUMAN > APPROVE: a blocking finding is actionable on its own, so the hold only ever replaces what would otherwise be an auto-approval. Missing-dimension and policy-conflict reasons are still recorded and rendered. - Empty review bodies when inline comments exist (APPROVE and REQUEST_CHANGES); a non-empty body remains only for comment-less reviews and skipped-dimension notes, matching review.md. - HOLD_FOR_HUMAN body now explains what failed and how to get unstuck (retry via push or workflow re-run, ask for a manual review, or opt out with the skip-ai-review label), and renders policy-conflict lines. - Drop planning identifiers from shipped comments.

jwbron added egg agent:orchestrator labels Jul 3, 2026

jwbron mentioned this pull request Jul 3, 2026

review: roster restructure, determinism boundary, eval suite #196

Closed

jwbron force-pushed the egg/pipeline-dcdad92d/slice-2 branch 2 times, most recently from 695fd8b to f578afc Compare July 3, 2026 21:42

egg and others added 7 commits July 3, 2026 14:43

Persist BRC history for slice-1 (#2548)

8826894

slice-1: prettier fixes + changeset

c02da14

slice-1: drop BRC history artifacts from slice PR

b119a01

jwbron force-pushed the egg/pipeline-dcdad92d/slice-1 branch from 1faaa0f to b119a01 Compare July 3, 2026 21:43

jwbron force-pushed the egg/pipeline-dcdad92d/slice-2 branch from f578afc to a2dee28 Compare July 3, 2026 21:43

jwbron marked this pull request as ready for review July 3, 2026 21:44

jwbron requested a review from jeresig July 3, 2026 21:44

khan-actions-bot requested review from a team and jaredly and removed request for a team July 3, 2026 21:44

jeresig requested changes Jul 6, 2026

View reviewed changes

jwbron force-pushed the egg/pipeline-dcdad92d/slice-2 branch from a2dee28 to aa98784 Compare July 6, 2026 18:19

jeresig approved these changes Jul 7, 2026

View reviewed changes

jwbron and others added 5 commits July 7, 2026 11:58

review: drop lib install step; scripts run via npx tsx

123a10f

egg-orchestrator and others added 4 commits July 7, 2026 11:58

Persist BRC history for slice-2 (#2548)

f618d2a

slice-2: changeset

6058d86

slice-2: drop BRC history artifacts from slice PR

da5b450

jwbron force-pushed the egg/pipeline-dcdad92d/slice-2 branch from aa98784 to 0950bf2 Compare July 7, 2026 18:59

Base automatically changed from egg/pipeline-dcdad92d/slice-1 to main July 8, 2026 16:33

jwbron merged commit 8feb84d into main Jul 8, 2026
4 checks passed

jwbron deleted the egg/pipeline-dcdad92d/slice-2 branch July 8, 2026 16:35

Navigation Menu

Uh oh!

[pipeline-dcdad92d][slice-2/12] Determinism boundary: computed... - #198

[pipeline-dcdad92d][slice-2/12] Determinism boundary: computed...#198
jwbron merged 17 commits into
mainfrom
egg/pipeline-dcdad92d/slice-2

jwbron commented Jul 3, 2026

Uh oh!

changeset-bot Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

jeresig left a comment

Uh oh!

jeresig Jul 6, 2026

Uh oh!

jeresig Jul 6, 2026

Uh oh!

jeresig Jul 6, 2026

Uh oh!

jwbron commented Jul 6, 2026

Uh oh!

jeresig left a comment

Uh oh!

jeresig Jul 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jwbron commented Jul 3, 2026

What's in this PR

This slice

Stack

Uh oh!

changeset-bot Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

jeresig left a comment

Choose a reason for hiding this comment

Uh oh!

jeresig Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

jeresig Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

jeresig Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

jwbron commented Jul 6, 2026

Uh oh!

jeresig left a comment

Choose a reason for hiding this comment

Uh oh!

jeresig Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Jul 3, 2026 •

edited

Loading