feat(contract): non-convergence stop criterion in sub-agent contract - #679
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0d77635f4
ℹ️ 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".
|
|
||
| Embed the schema at the top of every sub-agent's prompt and require results in that exact shape. Instruct each sub-agent explicitly: "Return ONLY the schema fields. No preamble, no analysis prose, no explanation — begin your response with the first schema field." When sub-agents return, validate field-by-field. If any artifact is missing, malformed, or wrapped in prose, re-dispatch only the failing sub-agent with the gap cited. Merge only schema-valid responses. | ||
|
|
||
| Also instruct each sub-agent to stop on non-convergence: if repeated attempts at the same sub-goal stop making progress after a few tries, do not keep retrying — return the best partial result via `failure_modes` with what could not be resolved. Activity is not progress. |
There was a problem hiding this comment.
Add a schema-valid non-convergence path
When this contract is loaded by /review, the child return format is the Finding schema in src/bundled-plugins/awa-bundled/skills/review/SKILL.md:111, which does not include a failure_modes field. In a non-converging review child, this new instruction tells the agent to put its partial result in failure_modes, while the contract immediately above still requires exact schema-valid responses and re-dispatches malformed/missing artifacts; that makes the new stop criterion unusable for the review loops it is meant to break. Add failure_modes or another blocked/partial field to the affected return schemas, or phrase this as a schema requirement rather than referencing a field those schemas do not return.
Useful? React with 👍 / 👎.
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
Reviewed at head ref b0d7763 (base main). Light-regime review (prose/convention change, 4 files, 6 insertions / 2 deletions, no executable code) across security, api-compat, correctness, spec-compliance, test-coverage, and perf-observability, plus a mechanical integrity pass on the pinned hashes and copy-identity. No blocking, critical, high, or medium findings. Three low/nit items below; none blocks merge.
Mechanically verified at ref b0d7763
- Pinned-hash integrity — both match, independently recomputed.
sha256(src/bundled-plugins/awa-bundled/skills/contract/SKILL.md) = a91fc354…f3545equals the new pin atbundled.test.ts:43;sha256(src/skills/_agents/prompts/contract.md) = 623bae5a…2ccbequals the new pin atvendored.test.ts:14. The old pins (748eaf01…,0b7febaf…) are correctly retired. - Copy-identity holds. The two in-repo convention copies differ by exactly one line (
context: loadfrontmatter); the new non-convergence paragraph is byte-identical in both, at the same position (after "Merge only schema-valid responses."). Matches the PR's byte-identical claim. - The new clause points at a real field.
failure_modesis defined in the convention (SKILL.md:15— "how to report blocked or partial work"), so "return the best partial result viafailure_modes" anchors to an existing, always-present schema field — not a phantom. - Diff is exactly the 4 declared files — no scope creep (
git diff --stat= 2 markdown copies + 2 test hash bumps, nothing else).
Dimension results (read against ref b0d7763)
- security — clean. Documentation-only prose added to a convention file; no executable code, no secrets, no injection surface, no privilege change.
- api-compat — clean (additive). No exported symbol, function signature, tool allowlist, or field name changed. The clause reuses the pre-existing
failure_modesfield; it adds and removes no schema field. - correctness — clean. The clause is internally consistent: it routes non-convergence output through the
failure_modesschema field, so it does not contradict the "Return ONLY the schema fields. No preamble, no analysis prose" directive (SKILL.md:18). See low finding #3 on channel overlap. - spec-compliance — intent met in full, no scope creep. Every clause of the stated intent has an implementing change: the paragraph added to both copies byte-identically, both pins bumped, no code touched. No unmet clause; the diff is confined to the four declared files.
- test-coverage — clean. The pinned-hash tests genuinely guard both files: each recomputes the on-disk sha256 and asserts
toBe(PINNED_HASHES[name]), so any unintended future edit fails until re-pinned. A skill-registration sentinel exists atbundled.test.ts:427-438. See low finding #2. - perf-observability — clean. No runtime, latency, logging, or metric impact. Semantically the clause improves observability by pushing a non-converging sub-agent to emit a
failure_modespayload rather than silently looping — the stated purpose.
Non-blocking findings
- nit · spec-compliance ·
src/skills/_agents/vendored.test.ts:14— the vendored guard is byte-equal and pinscontract.mdalone; there is no in-repo test asserting the vendored clause text equals the bundledSKILL.mdclause. Byte-identity across the two copies is confirmed out-of-band (diff), and no in-repo test asserts cross-file equality for any skill. Accept as-is. - low · test-coverage ·
src/skills/_agents/vendored.test.ts:12-16—vendored.test.tsis a fixed 3-entry enumeration with noreaddirSyncdirectory-scan sentinel (unlikebundled.test.ts:427-438), so a future new vendored prompt would not be force-registered intoPINNED_HASHES. This PR adds no new prompt, so the gap does not affect this change. Suggestion: add a directory-scan sentinel in a separate hardening PR. - low · correctness ·
src/bundled-plugins/awa-bundled/skills/contract/SKILL.md:20— the clause routes partial results "viafailure_modes", while the sibling epistemic-confidence block routes non-completion signals throughcoverage_gaps/boundary_flag, giving two overlapping channels for "what could not be resolved" with no stated precedence. Resolved:failure_modesis a required core field andcoverage_gapsis opt-in, sofailure_modesis the correct always-present channel. No fix required.
What was not checked
- Wave 1 read against branch HEAD
b0d77635f40eb499a5e57a93e6666ab3f6697241; everyfile:linecitation was read at that ref, and the two pinned hashes were independently recomputed there (both match). - The test suite was not executed — the PR's "12718 passed / 0 failed" claim is unverified here (read-only review; hash integrity and file contents verified by reading at the ref, not by observing a green run). CI on this PR is green.
- Out of scope (not flagged): the companion
awa-privatemarketplace copy and the privateafk-workshopvendored copy — the PR states all three are byte-identical, but those repos are outside this public-repo review. The in-repo drift-comparison test is skipped here becauseexample-pluginis not co-located (documented behavior, not a defect). - Stated intent: PR #679 title + body — spec-compliance assessed against it.
Read-only automated review. No approval / request-changes was cast; a maintainer will make the merge call.
…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).
What
Adds a non-convergence stop criterion to the sub-agent
/contractconvention (a new paragraph after the "Instruct each sub-agent explicitly" directive, tied to the existingfailure_modesfield):Applied to both in-repo copies (kept byte-identical):
src/bundled-plugins/awa-bundled/skills/contract/SKILL.mdandsrc/skills/_agents/prompts/contract.md, with the two pinned hashes bumped (bundled.test.ts,vendored.test.ts).Why
/contractis loaded by the orchestration skills (review, devils-advocate, shadow-verify, research, refactor, ship, …) and its directives get baked into the dispatch prompts those orchestrators write. So this clause reaches the orchestration-dispatched research-agent leaves — the population that actually trapped in the #669 incident (review dimension agents verifying citations) — a population thegeneral-purpose-prompt convergence clause (#675) does not reach.Layer map for the "trapped subagent" root cause (agents have an activity signal but no convergence signal):
general-purposeprompt (feat(agents): self-reported convergence/stop criterion for general-purpose #675) — direct/ad-hocgeneral-purposedispatches./contract) — orchestration-dispatched leaves, via the orchestrator's dispatch prompt.general-purposeround cap (feat(agents): bound general-purpose subagent with a generous tool-use ceiling #673) and the idle watchdog (feat(subagent): progress-aware idle watchdog for forked turns #672).It's a soft (prompt-compliance) signal by nature — the same mechanism that already makes critics return the contract's I/O schema.
Companion change
The identical clause is mirrored into the upstream
awa-privatemarketplace (plugins/awa-dev/skills/contract/SKILL.md) in a companion PR — byte-identical (verified: same sha256 across all three copies), so the bundled↔upstream drift comparison stays clean. The afk-workshop (private superset) vendored copy is the remaining routine mirror.Test / verification
pnpm lint— PASSpnpm test src/bundled-plugins/awa-bundled/bundled.test.ts— 17 passed / 12 skipped (drift-comparison skips:example-pluginnot co-located; the pinned-hash guard still runs)pnpm test src/skills/_agents/vendored.test.ts— 20 passedpnpm test— 12718 passed / 14 skipped / 0 failed (689 files)context: loadfrontmatter line.