Skip to content

review: prompt disciplines and measured cost fixes#228

Merged
jwbron merged 3 commits into
mainfrom
review-prompt-disciplines
Jul 9, 2026
Merged

review: prompt disciplines and measured cost fixes#228
jwbron merged 3 commits into
mainfrom
review-prompt-disciplines

Conversation

@jwbron

@jwbron jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Third of three PRs split out of #224; stacked on #227 (retarget as the stack merges). Prompt text only (review.md + README); no code changes. With this PR the stack matches #224 apart from the changesets and one follow-up: the budget-exhaustion trigger was reworded (per review feedback) to key off observable proxies rather than spend, which the agent cannot see.

Quality disciplines:

  • Quote-the-rule. A skill or conventions violation may only be flagged when the exact rule text and the exact violating line can both be quoted (skill-auditor, conventions reviewer, and every lens-owned skill application); the validator never confirms a skill claim that cannot quote its rule. No spirit-of-the-doc inference.
  • Amplification nuance. A claim about a mechanism that predates the diff confirms only when the diff materially amplifies its consequence, and the finding must say so explicitly; the correctness reviewer states introduce-vs-amplify on every pre-existing-bug finding.
  • Method-angle procedures. The correctness reviewer works the diff through three named procedures instead of one flat checklist: a line scan (what input or state makes each line wrong), a removed-behavior audit (name the invariant every deleted line enforced; find where the new code re-establishes it), and a cross-file trace (callers and callees of every changed symbol).

Measured cost fixes:

  • Graceful budget exhaustion. Nearing the run's hard ceilings (per-run AI-credits cap, job timeout), the orchestrator sheds remaining work in a fixed order (opt-in reviewers/lenses first, then the risks/patterns comment, then validation) and submits the verdict from findings validated so far plus skipped-dimension notes, instead of dying at a ceiling with everything spent and nothing posted. Since the agent cannot observe its own credit spend, the trigger is defined in terms of observable proxies: elapsed wall-clock vs runBudget.maxWallClockMinutes, dispatch count vs maxReviewerInvocations, the shared investigation journal vs maxTotalToolCalls, and overall trajectory.
  • Batched safe-output tail. The orchestrator emits same-kind safe outputs together in as few calls and turns as possible; thread resolutions especially, which previously went out one turn apiece.

Prompt text carries no plan identifiers.

cc @jeresig

Testing

  • npx vitest run workflows/review: 452 tests green (prompt-only change; smoke set gated).

@changeset-bot

changeset-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 19164d2

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

@jeresig jeresig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general seems good - but I'm not sure about the budgeting aspect, I don't think that's something that it'll have access to?

Comment thread workflows/review/review.md Outdated
claims anyway, and surface the gap as a skipped dimension (`claim validation`) with the
note in Step 6, so the author knows they were not double-checked this run.

**Run out of budget gracefully: always land the review.** The run has a hard

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the agent even have insight into if it is running out of budget? Are messages being injected in to provide that context? I'm worried that this will make it paranoid and start to abandon steps due to imagined constraints.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, you're right that it can't see spend directly. There's no channel that reports credits consumed back to the agent mid-run. The cap is enforced in gh-aw's api-proxy, and cost only shows up in the step summary after the run.

What the orchestrator does have is the target and a set of proxies: the router JSON it parses in Step 3 carries runBudget.maxUsd and maxWallClockMinutes, elapsed time is checkable from bash, the investigation-cap CLI already counts tool calls against maxTotalToolCalls in code, and it knows how many reviewers are still pending. The wall-clock side is just as load-bearing anyway, since timeout-minutes: 20 kills the job as surely as the credit cap does, and time is the one signal it can measure accurately.

So the shedding mechanism works as written, but the trigger sentence overpromises by leading with "spend". I'll reword it to say spend isn't observable and to lean on the observable proxies (elapsed minutes vs maxWallClockMinutes, reviewers dispatched vs maxReviewerInvocations, cap-CLI totals, diff size) before this merges. I'll also fix the "hard AI-credits cap (frontmatter)" aside, since the per-run cap is actually gh-aw's baked-in default, not something the frontmatter declares.

jwbron added a commit that referenced this pull request Jul 9, 2026
## Summary

First of three PRs split out of #224 (stack: this PR, then #227 provenance gate, then #228 prompt disciplines). Each lands independently revertable; the three together are byte-identical to #224.

Every finding (not just blocking ones) now carries a concrete `failure_scenario`: the specific inputs or state and the wrong outcome they produce. The field is required in the structured finding schema (`FINDING_SCHEMA_VERSION` bumped to 2), emitted by every producer (label-shape reviewers and all 11 specialist lenses), and carried verbatim into `claims.json`. The claim-validator now attacks exactly that stated scenario: confirmed means the named scenario occurs on the cited lines, and a scenario too vague to check caps at plausible.

`FINDING_SCHEMA_VERSION` is now 2 (a required field invalidates previously serialized findings, per the schema module's own rule); every corpus fixture is migrated with a hand-written failure scenario.

cc @jeresig

## Testing

- `npx vitest run workflows/review`: 410 tests green, including the smoke set.
- `npx tsc --noEmit` clean.

Author: jwbron

Reviewers: jeresig, jaredly

Required Reviewers:

Approved By: jeresig

Checks: ✅ 3 checks were successful

Pull Request URL: #226
@jwbron jwbron changed the base branch from review-provenance-gate to main July 9, 2026 18:07
An error occurred while trying to automatically change base from review-provenance-gate to main July 9, 2026 18:07
jwbron added 2 commits July 9, 2026 11:32
Quote-the-rule: a skill or conventions violation may only be flagged
when the exact rule text and the exact violating line can both be
quoted (skill-auditor, conventions reviewer, and every lens-owned skill
application); the validator never confirms a skill claim that cannot
quote its rule.

Amplification nuance: a claim about a mechanism that predates the diff
confirms only when the diff materially amplifies its consequence, and
the finding must say so explicitly; the correctness reviewer states
introduce-vs-amplify on every pre-existing-bug finding.

Method-angle procedures: the correctness reviewer works the diff
through three named procedures instead of one flat checklist: a line
scan, a removed-behavior audit, and a cross-file trace.

Graceful budget exhaustion: nearing the AI-credits cap, the
orchestrator sheds remaining work in a fixed order and submits the
verdict from findings validated so far plus skipped-dimension notes.

Batched safe-output tail: same-kind safe outputs go out together in as
few calls and turns as possible; thread resolutions especially.
…able proxies, not spend

The orchestrator cannot see its own credit spend (nothing reports credits
consumed back mid-run), so the budget-exhaustion trigger now names the
signals it can actually observe: elapsed wall-clock vs
runBudget.maxWallClockMinutes (captured via date +%s at Step 1), dispatch
count vs maxReviewerInvocations, the shared investigation journal vs
maxTotalToolCalls, and trajectory. Also corrects the hard-cap aside: the
per-run AI-credits cap is gh-aw's baked-in default (the frontmatter only
disables the daily cap), and the job timeout is an equally fatal ceiling.
@jwbron jwbron force-pushed the review-prompt-disciplines branch from ac358f3 to 3acab3e Compare July 9, 2026 18:34
…tep 1, quantify the shed trigger

Review follow-ups: record date +%s as Step 1's first action instead of a
mid-document aside so the elapsed-time proxy is reliably captured; define
nearing a soft target as roughly three-quarters so shedding behavior is
consistent across runs; stop calling the budget guardrail a mechanical
gate in the README, since unlike the provenance gate it is a prompt
discipline backed in code only by the investigation-cap CLI.
@jwbron jwbron merged commit 11666ee into main Jul 9, 2026
3 checks passed
@jwbron jwbron deleted the review-prompt-disciplines branch July 9, 2026 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants