fix(review): bound git-investigator to one batched pass per agent - #669
fix(review): bound git-investigator to one batched pass per agent#669griffinwork40 wants to merge 1 commit into
Conversation
A FULL review dispatched 2 research-agents -> 2 git-investigators running 52+23 git commands (each hitting the 50-round cap), then both dimensions 429'd with no output. Add a "Bounded git access" directive to the citation block: re-read only file-state citations (skip diff-context), batch all re-reads into a single git-investigator dispatch, at most one per agent. Bump the pinned hash in bundled.test.ts. Load-reduction mitigation (lowers 429 pressure); does NOT fix the underlying throttled-call/idle-watchdog hang (tracked separately in subagent.ts). Citation integrity preserved (Wave 1.5 backstop). BACK-PORT GAP: mirror into upstream example-plugin review SKILL.md. Parallel private edit: awa-private#75.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3833bc3bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **Bounded git access (per agent — overrides the general "multiple parallel queries" dispatch guidance for reviews).** Keep citation verification to one short pass: | ||
| - Re-read **only** `file-state` citations at the reviewed ref. A `diff-context` citation is already grounded in the diff hunk — do **not** issue a git re-read for it. | ||
| - Collect every needed re-read and resolve them in a **single** `git-investigator` dispatch (batch the queries). Never re-dispatch `git-investigator` per finding, and never run more than one `git-investigator` per agent. | ||
| - This bounds a full review to one brief git pass per agent instead of an open-ended archaeology loop — the pattern that let a review balloon to tens of minutes and exhaust the API rate limit with no output. |
There was a problem hiding this comment.
In a workspace where example-plugin is co-located, the drift test compares this bundled file to example-plugin/plugins/example-plugin/skills/review/SKILL.md after stripping only the current INTENTIONAL_DIFFS.review entries; that allowlist still covers only the contract namespace, context: fork, and argument-hint divergences (bundled.test.ts:314-333). Since the hash comment calls this new block a BACK-PORT GAP (bundled.test.ts:93-94), those non-allowlisted lines will make the review drift test fail whenever the upstream copy is present. Land the upstream mirror with this change or classify the new lines in INTENTIONAL_DIFFS if they are intentionally bundled-only.
Useful? React with 👍 / 👎.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Merge decision: ✅ MERGE
fix(review): bound git-investigator to one batched pass per agent · reviewed at branch HEAD b3833bc · 2 files, +13/-1 (one bundled SKILL.md prompt edit + a pinned-hash bump in a Vitest drift test). Regime: light (hotfix, prompt-only, no runtime code path). No blocking findings. Stated intent (PR #669 title+body) was assessed — every clause has an implementing, ref-verified change; no scope creep.
The change does what it says: it adds a "Bounded git access" directive to the review skill's citation-requirement block that caps citation re-reads to one short pass per dimension agent — re-read only file-state citations (a diff-context citation is already grounded in the diff hunk, so no git re-read), and resolve every needed re-read in a single git-investigator dispatch (never per-finding, never more than one per agent), explicitly overriding the inherited "multiple parallel queries" guidance for the review use case. The pinned hash of the bundled review SKILL.md in bundled.test.ts is bumped to match. This is a load-reduction mitigation for the observed failure (a FULL review ran ~43 min because two dimension agents each drove a git-investigator into the 50-round cap, then both 429'd and returned zero output) — and the PR is honest that it does not fix the underlying throttled-call / idle-watchdog hang, which is tracked separately and untouched here.
Blocking findings
None.
Non-blocking findings
NIT · spec-compliance · src/bundled-plugins/awa-bundled/bundled.test.ts:93-94 (ref b3833bc) — The back-port to the upstream example-plugin review SKILL.md is deferred to a parallel PR (awa-private#75), so the bundled and upstream review skills are intentionally divergent until that lands and is mirrored.
Evidence:
// BACK-PORT GAP: mirror this into the upstream example-plugin review SKILL.md
// (not co-located here, so the drift test skips) — see workflow above.
Suggestion: none required — the gap is disclosed in-code and in the PR body per the documented hash-bump workflow (bundled.test.ts:23-29); tracking the back-port is the only follow-up.
NIT · correctness · src/bundled-plugins/awa-bundled/skills/review/SKILL.md:58 (ref b3833bc) — The directive states it overrides "the general 'multiple parallel queries' dispatch guidance," but that is a paraphrase, not a verbatim quote of the guidance it overrides — the real referent is the research-agent system prompt (src/skills/_agents/prompts/research-agent.md:29, "dispatch them in parallel in one wave"), which Wave-1 review agents inherit because they run as research-agent.
Evidence: **Bounded git access (per agent — overrides the general "multiple parallel queries" dispatch guidance for reviews).**
Suggestion: none required — the referent is real and the reference is unambiguous; the quotation marks read as scare-quotes, not a literal cite.
What was verified (evidence)
- Test change is correct and complete. The new pinned hash
6ae1d3b4…c5ais exactly the SHA-256 of the reviewSKILL.mdcontent at the ref (independently recomputed:git show b3833bc:…/review/SKILL.md | shasum -a 256→6ae1d3b4…c5a, matchingbundled.test.ts:95). The old hashf313e377…has 0 occurrences at the ref; the new hash appears once. Bumping only the pinned hash is the documented registration path for an authored SKILL.md edit (bundled.test.ts:18-29); no logic change is needed. The pinned-hash snapshot test (bundled.test.ts:428-429) will pass — consistent with the all-green CI on this head. - Drift test correctly SKIPS, not fails. The namespace-normalized drift test guards on
it.skipIf(!upstreamAvailable(name))(bundled.test.ts:469);reviewis in the mirrorable set with anUPSTREAM_PATHS['review']entry (bundled.test.ts:145), so whenexample-pluginis not co-located (standalone clone / CI) the test skips — exactly as the PR body claims. Adding a newINTENTIONAL_DIFFSallowlist entry now (pre-back-port) would be wrong: it would mask the very drift the skip-gate exists to surface once the two copies are co-located. - Citation integrity is preserved. Bounding the agent's first-pass re-reads does not weaken verification: (1) a
diff-contextcitation is defined as needing no re-read (SKILL.md:55) and the diff is in every agent's input; (2) file-state re-reads are still mandated — the directive caps dispatch count, not coverage (SKILL.md:60, "resolve every needed re-read… in a single dispatch"); (3) Wave 1.5 Check A independently re-runsgit show <ref>:<file>on every blocking/critical/high citation after Wave 1 and drops fabricated ones (SKILL.md:82-85), a backstop downstream of and independent from the agent's own re-read. - No internal contradiction. No "per-finding re-read required" directive exists in the file (
git grep -n "per-finding" b3833bc -- …/review/SKILL.md→ 0 matches); the other re-read/re-dispatch mentions (SKILL.md:55,59,60) are consistent with the new batching rule. The overridden guidance lives in the external research-agent prompt, so the override is a scoped narrowing of an inherited instruction, not a self-contradiction. - Spec-compliance satisfied, no scope creep. Every intent clause maps to a change: file-state-only re-read (
SKILL.md:59), single batched git-investigator dispatch / ≤1 per agent / no per-finding (SKILL.md:60), explicit override of the parallel-queries guidance (SKILL.md:58), pinned-hash bump (bundled.test.ts:95), load-reduction-not-hang-fix framing (no runtime/watchdog code touched —git show --statis exactly these 2 files), citation integrity preserved, back-port gap disclosed. The commit touches only the 2 named files (+13/-1). - security / api-compat / perf clean. Prompt+test-only change — no auth/secret/injection/network surface, no exported symbol or public interface changed (
PINNED_HASHESis a test-internal const). Net perf-positive: the directive's whole purpose is to cut the git-investigator fan-out that hit the round cap and exhausted the API rate limit.
What was NOT checked
- Read against branch HEAD
b3833bc— the load-bearing claims (SHA-256 hash equality, old-hash-absent/new-hash-present, drift-test skip logic, the "per-finding" absence grep, the 2-file diff scope) were re-derived at that ref. No critical/high findings, so/shadow-verifywas not required. - Runtime behavioral effect is not mechanically verifiable. That the directive actually reduces Wave-1 git-investigator dispatches at runtime is an LLM-adherence claim from a static prompt edit, not a deterministic guarantee — and the PR frames it as a mitigation, not a fix.
- The underlying hang is out of scope (throttled model call with no idle/progress watchdog, tracked separately in
subagent.ts, unmodified here) — not assessed. - The override referent
src/skills/_agents/prompts/research-agent.md:29was read from the working tree, notgit show b3833bc:…(it is not part of the PR); its text is stable across worktree copies, so ref-presence is a safe inference but is unverified atb3833bcspecifically. It backs only a nit. - The upstream example-plugin review
SKILL.mdis not co-located in this checkout, so the back-port (awa-private#75) could not be verified from here. - Tests were not executed; the pass conclusion rests on the recomputed SHA-256 equality + git-grep re-derivation, and CI on this head is all-green (Lint & Build, Test, real-PTY scrollback, Docs, Publish Bundle Smoke).
Net: a correct, spec-compliant, low-risk prompt hardening that reduces the review skill's API-rate-limit exposure without weakening citation integrity. The two nits (disclosed back-port deferral; paraphrased override-referent) are non-blocking. Safe to merge.
…hannel, not a fixed `failure_modes` field Addresses codex P2 on #679. The non-convergence clause told sub-agents to return partial results via `failure_modes`, but ~7 of 9 /contract consumers (review, research, shadow-verify, devils-advocate, ship, simplify, parallelize) define no such field — review uses an `unverified —` channel. Since /contract requires schema-valid returns and re-dispatches malformed ones, the clause was weakest exactly where it was aimed (review's #669 dimension agents). Reword to reference whatever failure/blocked/unverified channel each agent's schema defines, keeping `failure_modes` as the canonical example. Applied to both byte-identical copies; pinned hashes bumped (bundled.test.ts, vendored.test.ts).
…679) * feat(contract): add non-convergence stop criterion to sub-agent contract * fix(contract): point non-convergence clause at the schema's failure channel, not a fixed `failure_modes` field Addresses codex P2 on #679. The non-convergence clause told sub-agents to return partial results via `failure_modes`, but ~7 of 9 /contract consumers (review, research, shadow-verify, devils-advocate, ship, simplify, parallelize) define no such field — review uses an `unverified —` channel. Since /contract requires schema-valid returns and re-dispatches malformed ones, the clause was weakest exactly where it was aimed (review's #669 dimension agents). Reword to reference whatever failure/blocked/unverified channel each agent's schema defines, keeping `failure_modes` as the canonical example. Applied to both byte-identical copies; pinned hashes bumped (bundled.test.ts, vendored.test.ts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3833bc3bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 3. If the line does not exist at that ref, omit the finding entirely — do not paraphrase or reconstruct from memory. | ||
|
|
||
| **Bounded git access (per agent — overrides the general "multiple parallel queries" dispatch guidance for reviews).** Keep citation verification to one short pass: | ||
| - Re-read **only** `file-state` citations at the reviewed ref. A `diff-context` citation is already grounded in the diff hunk — do **not** issue a git re-read for it. |
There was a problem hiding this comment.
When reviewing a PR or commit that is not checked out, an agent that finds no candidate finding still needs git show <reviewed-ref>:<file> to inspect the post-merge file; its Read tool only sees the current worktree. Restricting re-reads to file-state citations leaves such an agent with no permitted way to satisfy the later requirement to report no issues found only after reading the file at the reviewed ref, so clean dimensions must be returned as unverified. Permit the single batched git-investigator pass to include ref reads needed for review and clean-read confirmation, not only citations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3833bc3bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 3. If the line does not exist at that ref, omit the finding entirely — do not paraphrase or reconstruct from memory. | ||
|
|
||
| **Bounded git access (per agent — overrides the general "multiple parallel queries" dispatch guidance for reviews).** Keep citation verification to one short pass: | ||
| - Re-read **only** `file-state` citations at the reviewed ref. A `diff-context` citation is already grounded in the diff hunk — do **not** issue a git re-read for it. |
There was a problem hiding this comment.
When a high-severity finding cites a deleted line visible in the diff, this rule tells both the Wave 1 reviewer and the Wave 1.5 verifier not to read that citation from the reviewed ref. That prevents Wave 1.5 Check A (SKILL.md:85-90) from determining that the evidence is diff-only and applying its required downgrade, so a finding based on code absent from the post-merge file can reach synthesis at its original severity. Scope this ban to Wave 1 dimension agents or explicitly allow Wave 1.5 to verify every high-severity citation.
Useful? React with 👍 / 👎.
1 participant
What
Bounds the bundled
/reviewskill's git-investigator usage to one short batched pass per dimension agent. Adds a "Bounded git access" directive to the citation-requirement block inawa-bundled/skills/review/SKILL.md:file-statecitations at the reviewed ref (adiff-contextcitation is already grounded in the diff hunk — no re-read).git-investigatordispatch; never re-dispatch per finding, never more than onegit-investigatorper agent. Explicitly overrides the general "multiple parallel queries" guidance for the review use case.Pinned hash in
bundled.test.tsbumped accordingly (per the documented workflow).Why
A FULL review of a small PR was observed running ~43 min: 2 review dimension agents each dispatched a git-investigator that ran 52 / 23 git commands (each hit the 50-round
READONLY_AGENT_MAX_TOOL_USE_ITERATIONScap), and both dimensions then hit HTTP 429 on a single account and returned zero output.Scope (honest): this is a load-reduction mitigation — fewer git-investigator dispatches + fewer
git showre-reads per review lowers tokens/requests-per-review, so the rate-limit is far less likely to trigger. It does not fix the underlying hang, which is a throttled model call with no idle/progress watchdog (SUBAGENT_DEFAULT_TIMEOUT_MSis a blunt 45-min wall-clock; the progress-aware idle watchdog noted insubagent.tsis the real fix and is tracked separately). Citation integrity is preserved — verification still runs (Wave 1.5 remains the backstop); only redundant diff-context re-reads and iterative per-finding dispatches are removed.Test / risk
pnpm test src/bundled-plugins/awa-bundled/bundled.test.ts→ 17 passed, 12 skipped (drift-comparison tests skip when example-plugin isn't co-located).Back-port gap
The same change should be mirrored into the upstream
example-pluginreview SKILL.md (not co-located in this checkout, so the drift test skips). Noted inline at the hash entry. A parallel private-plugin PR (awa-private#75) carries the equivalent edit.