-
Notifications
You must be signed in to change notification settings - Fork 1
review: the submission plan; Steps 4-6 as code in scripted mode (orchestrator slice 4) #284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
48e1320
ad58197
6580d13
20a9693
0de59f3
563dccb
d95d65a
863c261
462d9d1
765ea3c
4ad02a7
2671732
97ca53d
43b63b0
ca877da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "review": minor | ||
| --- | ||
|
|
||
| The submission plan (deterministic-orchestrator slice 4, the production probe of the end state): in scripted dispatch mode, Steps 4-6 become code. After the dispatcher returns, the orchestrator runs `lib/submission.ts` once; it drops any claim anchored on a line the thread-reconciler flagged in `skipLines` (Step 5's defer-to-open-human-threads rule, applied before the verdict so a deferred claim neither posts nor counts), computes the verdict mechanically (Step 4's blocking-label rule via `computeVerdict`, plus the reduced-depth flip floor from `rereview.json`'s `keptBlockingCount` over a prior REQUEST_CHANGES stamp), renders every inline comment from the surviving claims (post-validation labels, rule quotes, suggestion blocks), renders the full review body (verdict head, the code-rendered accountability section, the dispatcher's note lines, the depth/tripwire notes, the fingerprint stamp as the final line, pr-level claims folded in), and stages `submission-plan.json`. The orchestrator's remaining model role is emitting safe outputs that match the plan verbatim, and the dispatch-conformance gate gains the plan-match rule: when a plan is staged, the queued event, body, and inline comments must match it under a sanitizer-tolerant normalization, so any splice, omission, or "improvement" blocks the submission and fails the run (the #244 accountability-splice check, as code). Queueing nothing is permitted only for the exact Step 6 redundant-approval shape: an APPROVE plan with no comments whose body is the bare comment-less approve line, so an approval carrying a shed disclosure can never be dropped on the floor silently. The safe-output emission itself is the remaining seam code cannot own under gh-aw today: the queue is a run-local JSONL append that needs no credentials, but the agent sandbox mounts `${RUNNER_TEMP}/gh-aw` read-only, so only the safeoutputs MCP container can write it; removing the seam wants a writable path into the queue (an upstream mount change, or a post-agent step on the host), neither tested yet. Rides the same live-trial gate as scripted dispatch; nothing changes in task mode. | ||
|
|
||
| Audit hardening for the plan-match rule this changeset introduces: an audit of gh-aw v0.81.6's ingest sanitizer against the rule-7 normalization found unabsorbed transforms that false-block a byte-faithful submission. The deployed URL policy is allowed-only and runs inside code regions, so any cited non-allowlisted https URL (MDN, StackOverflow) came back "(host/redacted)" against the plan's `<url>` placeholder: a guaranteed mismatch on ordinary review prose. The normalization (now its own module, `lib/sanitizer-normalize.ts`) folds https URLs and the domain-redaction token to a host-bearing placeholder, so the sanitizer's own rewrites pass while a link splice that swaps the host still blocks; it also absorbs neutralizeTemplateDelimiters' escaping backslashes (unbackticked `${{ }}` in prose), the bidi-control strip, and the C0/DEL control strip. Every fold applies to both sides, so splice detection loses nothing. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,10 @@ | |
| * (`out/thread-reconciler.json` `resolve`); the deficit direction is | ||
| * reported as executed-vs-decided accounting, never blocked (slice 3; | ||
| * the #244 ledger). | ||
| * 7. When a submission plan is staged (`submission-plan.json`, scripted | ||
| * mode, slice 4), the queued event, body, and inline comments must | ||
| * match it under a sanitizer-tolerant normalization; any splice or | ||
| * omission blocks (the #244 accountability-splice check, as code). | ||
| * | ||
| * Violation behavior: strip every posting/mutating item from the queue | ||
| * (keeping the diagnostics and the `out/` artifact upload so the evidence | ||
|
|
@@ -71,9 +75,10 @@ | |
| */ | ||
|
|
||
| import {extractJsonValue} from "./agent-json"; | ||
| import {isBlockingLabel} from "./render-comment"; | ||
| import {isBlockingLabel, renderReviewBody} from "./render-comment"; | ||
| import {parseLeadingLabel} from "./rereview"; | ||
| import {findLatestStamp, stampFromCacheMemory} from "./rereview-mode"; | ||
| import {normalizeBody} from "./sanitizer-normalize"; | ||
|
|
||
| /* -------------------------------------------------------------------------- */ | ||
| /* Types */ | ||
|
|
@@ -103,7 +108,8 @@ export type DispatchGateViolationCode = | |
| | "shed-undisclosed" | ||
| | "approve-with-blocking-comment" | ||
| | "flip-vetoed-kept-blocking" | ||
| | "resolve-not-decided"; | ||
| | "resolve-not-decided" | ||
| | "submission-plan-mismatch"; | ||
|
|
||
| export type DispatchGateViolation = { | ||
| /** Fixed-format code (never prose). */ | ||
|
|
@@ -147,6 +153,8 @@ export type DispatchGateInput = { | |
| cacheMemory?: unknown; | ||
| /** Parsed `rereview.json` (the accountability result; `keptBlockingCount`). */ | ||
| rereviewAccounting?: unknown; | ||
| /** Parsed `submission-plan.json` (scripted mode; slice 4). */ | ||
| submissionPlan?: unknown; | ||
| }; | ||
|
|
||
| /* -------------------------------------------------------------------------- */ | ||
|
|
@@ -508,6 +516,143 @@ export const evaluateDispatchConformance = ( | |
| } | ||
| } | ||
|
|
||
| // Rule 7 (scripted mode, slice 4): when a submission plan is staged, the | ||
| // queued outputs must match it. gh-aw's ingest sanitizer rewrites what | ||
| // the agent queued before the gate sees it, so bodies are compared under | ||
| // `normalizeBody` (sanitizer-normalize.ts, which documents every absorbed | ||
| // transform); anything beyond that is a splice (#244) and blocks. The | ||
| // rule also owns the NO-submission shapes: queued comments with no submit | ||
| // would land as a COMMENT review, and a silently-dropped plan would | ||
| // withhold a REQUEST_CHANGES verdict (or a disclosure), so only | ||
| // an APPROVE plan with no comments and a bare approve body may | ||
| // legitimately queue nothing (the Step 6 redundant-approval skip, whose | ||
| // shape the skip branch below checks in full). | ||
| // Defensive over agent-writable staged input, like every sibling parse in | ||
| // this file. `readJsonIfPresent` passes `JSON.parse("null")` straight | ||
| // through, so a `submission-plan.json` containing literal `null` reaches | ||
| // here as `null` — which a bare `!== undefined` guard admits, and the | ||
| // property reads below then throw. That throw escapes to the CLI entry | ||
| // catch, which exits 0 with the queue untouched: not a rule-7 failure but | ||
| // a fail-open of ALL SEVEN rules. The sibling `priorReviews` parse was | ||
| // hardened against exactly this shape; this one has to match it. | ||
| const planStaged = | ||
| typeof input.submissionPlan === "object" && | ||
| input.submissionPlan !== null | ||
| ? (input.submissionPlan as { | ||
| event?: unknown; | ||
| body?: unknown; | ||
| comments?: unknown; | ||
| }) | ||
| : undefined; | ||
| if (planStaged !== undefined && submit === undefined) { | ||
| const planComments = Array.isArray(planStaged.comments) | ||
|
khan-actions-bot marked this conversation as resolved.
|
||
| ? planStaged.comments | ||
| : []; | ||
| if (commentCount > 0) { | ||
| violations.push({ | ||
| code: "submission-plan-mismatch", | ||
| dimension: "verdict", | ||
| detail: `${commentCount} inline comment(s) queued with no review submission (they would land as an ungated COMMENT review); the staged plan requires a ${String( | ||
| planStaged.event, | ||
| )} submission`, | ||
| }); | ||
| } else { | ||
| // review.md's redundant-approval skip is narrower than "APPROVE | ||
| // with no comments": the body must ALSO carry no `Note:` lines | ||
| // and no accountability section, i.e. it is exactly the bare | ||
| // comment-less-approve line (the fingerprint stamp is an HTML | ||
| // comment, which normalizeBody already drops). Without the body | ||
| // check, an APPROVE that shed a lens (carrying a mandatory | ||
| // "Note: <lens> not assessed this run" disclosure) could be | ||
| // dropped on the floor and still pass the gate green, silently | ||
| // withholding both the disclosure and the approval. | ||
| const bareApprove = normalizeBody( | ||
| renderReviewBody({event: "APPROVE", hasInlineComments: false}), | ||
| ); | ||
| const planBody = | ||
| typeof planStaged.body === "string" ? planStaged.body : ""; | ||
| if ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): The no-submission skip gates only on the plan shape (bare comment-less APPROVE), not the prior-review-APPROVED half Lower-confidence (1)
|
||
| planStaged.event !== "APPROVE" || | ||
| planComments.length > 0 || | ||
| normalizeBody(planBody) !== bareApprove | ||
| ) { | ||
| violations.push({ | ||
| code: "submission-plan-mismatch", | ||
| dimension: "verdict", | ||
| detail: `nothing queued but the staged plan is ${String( | ||
| planStaged.event, | ||
| )} with ${ | ||
| planComments.length | ||
| } comment(s); only an APPROVE plan with no comments and a bare "${renderReviewBody( | ||
| {event: "APPROVE", hasInlineComments: false}, | ||
| )}" body may skip the submission`, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| if (planStaged !== undefined && submit !== undefined) { | ||
|
khan-actions-bot marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note (non-blocking): Rule 7 binds the queued event, body, and comments to the plan but never compares queued resolves against |
||
| if ( | ||
| typeof planStaged.event === "string" && | ||
| verdictEvent !== planStaged.event | ||
| ) { | ||
| violations.push({ | ||
| code: "submission-plan-mismatch", | ||
| dimension: "verdict", | ||
| detail: `queued event ${ | ||
| verdictEvent || "(none)" | ||
| } does not match the staged submission plan's ${ | ||
| planStaged.event | ||
| }`, | ||
| }); | ||
| } | ||
| if ( | ||
| typeof planStaged.body === "string" && | ||
| normalizeBody(body) !== normalizeBody(planStaged.body) | ||
| ) { | ||
| violations.push({ | ||
| code: "submission-plan-mismatch", | ||
| dimension: "review body", | ||
| detail: "queued review body does not match the staged submission plan (normalized comparison)", | ||
| }); | ||
| } | ||
| if (Array.isArray(planStaged.comments)) { | ||
| const planned = planStaged.comments | ||
| .filter( | ||
| ( | ||
| comment, | ||
| ): comment is {path: string; line: number; body: string} => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick (non-blocking): The plan-comment filter predicate narrows to |
||
| typeof (comment as {path?: unknown}).path === | ||
| "string" && | ||
| typeof (comment as {body?: unknown}).body === "string", | ||
| ) | ||
| .map( | ||
| (comment) => | ||
| `${comment.path}:${comment.line}:${normalizeBody( | ||
| comment.body, | ||
| )}`, | ||
| ) | ||
| .sort(); | ||
| const queued = input.items | ||
| .filter((item) => item.type === COMMENT_TYPE) | ||
| .map( | ||
| (item) => | ||
| `${ | ||
| typeof item["path"] === "string" ? item["path"] : "" | ||
| }:${String(item["line"] ?? "")}:${normalizeBody( | ||
| typeof item.body === "string" ? item.body : "", | ||
| )}`, | ||
| ) | ||
| .sort(); | ||
| if (JSON.stringify(planned) !== JSON.stringify(queued)) { | ||
| violations.push({ | ||
| code: "submission-plan-mismatch", | ||
| dimension: "inline comments", | ||
| detail: `queued inline comments (${queued.length}) do not match the staged submission plan (${planned.length})`, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| conformant: violations.length === 0, | ||
| violations, | ||
|
|
@@ -652,6 +797,10 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { | |
| `/tmp/gh-aw/cache-memory/pr-${prContext.number}.json`, | ||
| ) | ||
| : undefined; | ||
| const submissionPlan = readJsonIfPresent( | ||
| fs, | ||
| `${REVIEW_DIR}/submission-plan.json`, | ||
| ); | ||
|
|
||
| const evaluation = evaluateDispatchConformance({ | ||
| items, | ||
|
|
@@ -661,6 +810,7 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { | |
| priorReviews, | ||
| rereviewAccounting, | ||
| cacheMemory, | ||
| submissionPlan, | ||
| }); | ||
| evaluation.notes.unshift(...notes); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /** | ||
| * The ingest sanitizer's transforms, mirrored so the dispatch gate's rule 7 | ||
| * can compare a staged submission plan against the queued safe outputs | ||
| * without false-blocking a byte-faithful transcription. | ||
| * | ||
| * gh-aw v0.81.6 rewrites everything the agent queues before the gate sees it | ||
| * (`sanitize_content_core.cjs`): HTML comments vanish, unicode is hardened, | ||
| * template delimiters gain escaping, and URLs are redacted under an | ||
| * allowed-only policy. The plan is composed BEFORE that pass, so the two | ||
| * sides differ on a conforming run. {@link normalizeBody} applies every | ||
| * absorbed transform to BOTH sides, so tolerance costs no splice detection: | ||
| * anything the sanitizer would not have done is still a mismatch (#244). | ||
| * | ||
| * Documented-not-absorbed residuals, each of which needs a pathological body | ||
| * and fails red rather than silently: HTML entity decoding, XML tag | ||
| * conversion, the percent-decode side effect, homoglyph folds, the 65k | ||
| * truncation, markdown link titles, and tilde fences. | ||
| */ | ||
|
|
||
| /** | ||
| * Fold one body to its sanitizer-tolerant comparison form. Applied to the | ||
| * plan and the queued text alike; never to text that gets posted. | ||
| */ | ||
| export const normalizeBody = (text: string): string => | ||
| text | ||
| // The ingest sanitizer deletes ALL XML/HTML comments | ||
| // (removeXmlComments), so the queued body can never carry the | ||
| // plan's fingerprint stamp; comparing modulo comments is what | ||
| // "sanitizer-tolerant" requires (trial run 29893634730 blocked | ||
| // a byte-faithful transcription on exactly this). | ||
| .replace(/<!--[\s\S]*?-->/g, "") | ||
| // The sanitizer's hardenUnicodeText applies NFKC and strips | ||
| // zero-width characters (gh-aw sanitize_content_core.cjs), which | ||
| // rewrites compatibility characters: trial run 29903306596 | ||
| // blocked a jq-verbatim emission because one reviewer-authored | ||
| // ellipsis came back as three dots (NFKC). Apply the same | ||
| // normalization on both sides, plus the typographic quote/dash | ||
| // folds NFKC does not cover. | ||
| .normalize("NFKC") | ||
| .replace(/\u034f/g, "") | ||
| // Zero-width, bidi-control (sanitizer step 4), C0/DEL (its | ||
| // control-strip): all deleted on the queued side only, so | ||
| // delete them on both. | ||
| .replace(/[\u00ad\u200b-\u200f\u2060-\u2064\ufeff]/g, "") | ||
| .replace(/[\u202a-\u202e\u2066-\u2069]/g, "") | ||
| // eslint-disable-next-line no-control-regex | ||
| .replace(/[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/g, "") | ||
| .replace(/[\u2018\u2019\u201a\u201b]/g, "'") | ||
| .replace(/[\u201c\u201d\u201e\u201f]/g, '"') | ||
| .replace(/[\u2012\u2013\u2014\u2015]/g, "-") | ||
| .toLowerCase() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note (non-blocking): |
||
| .replace(/`/g, "") | ||
| // neutralizeTemplateDelimiters escapes {{ ${ {% {# <%= outside | ||
| // code regions; drop the escaping backslashes on both sides. | ||
| .replace(/\\(?=[{$%#<])/g, "") | ||
| // URL sanitization applies even inside code regions under the | ||
| // deployed allowed-only policy: a non-allowlisted domain or a | ||
| // non-https scheme is rewritten to "(host/redacted)" or | ||
| // "(redacted)" (sanitizeUrlDomains / sanitizeUrlProtocols, | ||
| // gh-aw v0.81.6). Domain redaction KEEPS the host, so an https | ||
| // URL folds to a host-bearing placeholder that both sides | ||
| // agree on and a cross-host link splice still trips the check. | ||
| // Protocol redaction drops the host entirely, so every other | ||
| // scheme folds hostless (matching the bare "(redacted)"). | ||
| // Deliberate residual: path and query ARE wildcarded, so swapping | ||
| // one same-host deep link for another passes rule 7; the splice | ||
| // check covers the prose and the link's host, not which page on | ||
| // that host is cited. | ||
| .replace(/https:\/\/([a-z0-9.-]+)\S*/g, "<url:$1>") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): |
||
| .replace(/[a-z][a-z0-9+.-]*:\/\/\S+/g, "<url>") | ||
| .replace( | ||
| /(?:mailto|javascript|vbscript|data|about|tel|magnet):\S+/g, | ||
| "<url>", | ||
| ) | ||
| .replace(/\(([a-z0-9.-]+)\/redacted\)/g, "<url:$1>") | ||
| .replace(/\(redacted\)/g, "<url>") | ||
| .replace(/\s+/g, " ") | ||
| .trim(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): A present-but-malformed
submission-plan.json(literalnull, a scalar, or an object with no bindableevent/body/comments) folds toplanStaged === undefined, so rule 7 is skipped silently and the gate still reports conformant — unlike therouting.jsonparse (dispatch-gate.ts:777-781), which records apresent but unparseablenote. Run forensics then cannot distinguishplan matchedfromplan staged but never bound. Consider pushing a note when the plan file exists but parses to undefined/null, mirroring the routing.json precedent. Non-blocking: the null fold is the right fail-open direction and an agent that can rewrite the plan can also delete it, so this is observability, not a new bypass.