fix(apm-review-panel): guarantee non-empty turn exit via synchronous fan-out#1714
Merged
Merged
Conversation
…fan-out PR Review Panel runs could report success yet open a "No Safe Outputs Generated" failure: the orchestrator spawned the CEO synthesizer in background/detached mode and ended its turn before synthesis returned, so agent_output was empty, gh-aw skipped safe-output detection, and the add-comment job never ran. This hardens the apm-review-panel skill prose -- the correct (and only) layer, since no deterministic tool can force an orchestrator LLM to await its own subagent or keep its turn open: - New "Non-empty turn exit" invariant framed on gh-aw's actual detection mechanism: every turn ends with >=1 safe output -- the rendered comment, or an explicit `noop` (a real, configured safe-output) -- never empty. - New "Synchronous fan-out" invariant: every task spawn (panelists + CEO) is BLOCKING; describes the blocking-vs-detached affordance, not vendor syntax. - Step 5 (CEO spawn) point-of-use blocking anchor; checklist-intro anchor. - New step 9: guarantee a non-empty exit, with the `noop` fallback as the deterministic backstop. - New gotcha cataloguing background spawn-and-forget as the canonical empty-output cause. Supersedes the approach in #1712; the lockfile is unchanged (it tracks dependency resolution, not skill body content) and no workflow recompile is needed (the workflow pulls SKILL.md from main at runtime). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
12 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the apm-review-panel skill’s orchestration contract to prevent gh-aw “No Safe Outputs Generated” failures by making fan-out (panelists + CEO synthesizer) explicitly synchronous and by requiring a non-empty safe-output exit.
Changes:
- Adds architecture invariants for non-empty turn exit and synchronous fan-out (no background/detached
taskspawns). - Strengthens the execution checklist with blocking/awaiting requirements, plus a new step to ensure the turn ends with a safe output (comment or
noopfallback). - Documents the background spawn-and-forget failure mode in Gotchas for faster diagnosis.
Show a summary per file
| File | Description |
|---|---|
| packages/apm-review-panel/SKILL.md | Updates skill invariants/checklist/gotchas to enforce synchronous fan-out and guarantee non-empty safe-output emission. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
Comment on lines
+409
to
+413
| 9. **Guarantee a non-empty exit.** Your final action this turn MUST be a | ||
| safe output. In the normal path that is the single `add-comment` from | ||
| step 7 (the `remove-labels` sweep alone does NOT count -- it is not | ||
| the run's required output). Before ending the turn, confirm step 7 | ||
| actually issued the `add-comment` call and it did not error. If, after |
…urce The committed deployed copy under .agents/skills/apm-review-panel/ had drifted from packages/apm-review-panel/ (the source): it predated the performance-expert persona (SKILL.md 415 vs 446 lines; schema enum + description both missing performance-expert). Re-deployed via `apm install --target agent-skills` so the deployed copy matches source again -- this also carries the synchronous-fan-out fix from the parent commit. The deployed SKILL.md diff is larger than the source fix because it folds in the pre-existing performance-expert drift; both deployed files are now byte-identical to packages/. Scope notes: - Lockfile intentionally NOT touched. The committed apm.lock.yaml is a copilot-target lockfile (deployed_files: copilot-app-db://workflows/...) and does not govern .agents/skills/ at all -- which is precisely why the audit/integrity gate never caught this drift. Retargeting it to agent-skills here would be out of scope and would corrupt the evidence for the separate root-cause investigation. - Other pre-existing .agents/skills drift (e.g. an undeployed cut-release skill) is left for that investigation rather than folded in here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added a commit
that referenced
this pull request
Jun 9, 2026
Merge origin/main (PRs #1714, #1717, #1718, #1719) into the release branch. Only #1718 is user-facing (src/apm_cli/install drift + target-complete lockfile manifest); its entry was authored on main and now sits in the [0.19.0] Fixed block with the PR number appended. #1714, #1717, #1719 are maintainer-toolkit / test-only and dropped per the entry-sanitizer DROP list. Lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added a commit
that referenced
this pull request
Jun 9, 2026
…ckfile The APM Self-Check (apm audit --ci) content-integrity gate failed: apm.lock.yaml recorded a stale deployed_file_hashes entry (fe6a73...) for .agents/skills/apm-review-panel/SKILL.md while the deployed file and its packages/** source are both fc63e25... #1714 updated the skill content but deliberately left apm.lock.yaml untouched; #1718 then turned the drift gate live (dropped --no-drift), exposing the mismatch. apm install reconciles the recorded hash (and refreshes apm_version to 0.19.0). All 9 audit checks now pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added a commit
that referenced
this pull request
Jun 9, 2026
* chore: release v0.19.0 Bump pyproject.toml + uv.lock to 0.19.0 and move the [Unreleased] CHANGELOG block to [0.19.0] - 2026-06-09 with one 'so what' entry per merged PR. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.19.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: fold #1718 into v0.19.0 changelog after main merge Merge origin/main (PRs #1714, #1717, #1718, #1719) into the release branch. Only #1718 is user-facing (src/apm_cli/install drift + target-complete lockfile manifest); its entry was authored on main and now sits in the [0.19.0] Fixed block with the PR number appended. #1714, #1717, #1719 are maintainer-toolkit / test-only and dropped per the entry-sanitizer DROP list. Lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(self-check): reconcile stale apm-review-panel SKILL.md hash in lockfile The APM Self-Check (apm audit --ci) content-integrity gate failed: apm.lock.yaml recorded a stale deployed_file_hashes entry (fe6a73...) for .agents/skills/apm-review-panel/SKILL.md while the deployed file and its packages/** source are both fc63e25... #1714 updated the skill content but deliberately left apm.lock.yaml untouched; #1718 then turned the drift gate live (dropped --no-drift), exposing the mismatch. apm install reconciles the recorded hash (and refreshes apm_version to 0.19.0). All 9 audit checks now pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
pushed a commit
that referenced
this pull request
Jun 9, 2026
…e integrity) Sync with main through v0.19.0 (#1715, #1714, #1718, #1719, #1717). Conflicts resolved: - CHANGELOG.md: main cut the v0.19.0 release, moving prior Unreleased entries into a dated section. Took main's restructure; relocated the #1681 "Tightened Stage 2 code-complexity thresholds" entry to the new ## [Unreleased] section (released sections stay immutable per Keep a Changelog). Took main's #1702/#1710 close-ref for the lockfile entry. - src/apm_cli/install/services.py: #1718 added per-file skill-bundle integrity recording inline at a site my refactor had extracted into _log_skill_result. Kept the new _skill_bundle_file_entries helper in services.py (sibling of _deployed_path_entry); dropped main's duplicate inline _log_hook_display_payloads (already relocated to services_integrate.py and re-exported in my #1700 port); accepted the _log_skill_result delegator at the call site. - src/apm_cli/install/services_integrate.py: ported #1718's deployed.extend(_skill_bundle_file_entries(tp, ...)) into _log_skill_result's deployed-path loop (deployed aliases result["deployed_files"]), with a lazy import of the helper. Shadow gate: no markers, all src <=800 lines, ruff check + format clean, pylint R0801 EXIT=0, auth-signals clean; 11144 tests pass across the install/integration suites plus the #1718, #1700, and #1709 targeted sets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Supersedes #1712 (audited and rebuilt around a stronger contract).
PR Review Panel run 27194415500 reported
successyet opened a "No Safe Outputs Generated" failure. The orchestrator spawned the CEO synthesizer in background/detached mode and ended its turn before synthesis returned, so the singleadd-commentwas never emitted:Why prompt-hardening (and not a code fix)
The bug is a fan-out + synthesizer fan-in violation: the parent must WAIT at synthesis. There is no deterministic tool that can force an orchestrator LLM to await its own subagent or to keep its turn open — turn-management is inherently LLM-driven. Prompt-hardening the
apm-review-panelskill is the correct (and only) layer. The workflow pullsSKILL.mdfrommicrosoft/apm#mainat runtime, so no workflow recompile is needed.What changed (
packages/apm-review-panel/SKILL.md)noop), never empty. This is the robust backstop.taskspawn (panelists and CEO) is BLOCKING. Describes the blocking-vs-detached affordance (theagent_idbackground variant), not vendor-specific syntax.noop(a real, configured gh-aw safe-output) as the deterministic fallback.Deployed-copy re-sync (
.agents/skills/apm-review-panel/)The committed deployed copy had drifted from source — it predated the
performance-expertpersona (SKILL.md 415 vs 446 lines; schema enum + description also stale). Re-deployed viaapm install --target agent-skills; both deployed files are now byte-identical topackages/. The deployedSKILL.mddiff is larger than the source fix because it folds in that pre-existing drift.How this differs from #1712
#1712 leaned on LLM self-attestation ("verify you invoked the tool"). This version foregrounds the deterministic exit contract keyed off gh-aw's own detection (
agent_outputemptiness), keeps the synchronous-fan-out rule as the proximate cause, verified thatnoopis a genuine configured safe-output rather than asserting it from recall, re-syncs the drifted deployed copy, and omits the stray.github/aw/logs/debug files that #1712 mentioned.Type of change
Testing
Prompt + deployed-asset change; no code paths or tests exercise
SKILL.mdcontent. Verified:apm compile --validatepasses (41 primitives valid)..agents/skills/apm-review-panel/files byte-identical topackages/source.Spec conformance (OpenAPM v0.1)