Skip to content

fix(apm-review-panel): require synchronous fan-out so the panel never ends its turn with empty safe outputs#1712

Closed
Copilot wants to merge 4 commits into
mainfrom
copilot/fix1690-installer-safety
Closed

fix(apm-review-panel): require synchronous fan-out so the panel never ends its turn with empty safe outputs#1712
Copilot wants to merge 4 commits into
mainfrom
copilot/fix1690-installer-safety

Conversation

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Description

PR Review Panel run 27194415500 reported success yet opened a "No Safe Outputs Generated" failure. gh aw audit traced the chain: the orchestrator spawned the CEO synthesizer in background/detached mode and ended its turn before the synthesis returned, so the single add-comment was never emitted.

agent-stdio.log:  "Agent started in background with agent_id: ceo-synthesizer"  → exit 0
agent_output.json: {"items":[]}
safe_outputs:      needs.detection.result == 'success'  → false  (detection skipped → add-comment job never runs)

The apm-review-panel skill never stated that subagent task spawns are blocking, nor that the turn must not end before the comment is written. Fix is prompt-hardening in packages/apm-review-panel/SKILL.md (the copy the workflow pulls from microsoft/apm#main; no workflow recompile needed).

  • New invariant — "Synchronous fan-out, never spawn-and-forget": every task spawn (panelists and CEO) must use synchronous mode, never the background/async agent_id variant; documents the empty-output failure chain and the noop fallback.
  • Step 5 (CEO synthesizer) + checklist intro: require BLOCKING execution and awaiting the JSON return.
  • New step 9 — emission confirmation: verify the add-comment safe-output tool was actually invoked (and did not error) before ending the turn.
  • New gotcha: background spawn-and-forget as the canonical "No Safe Outputs Generated" cause.
  • Untracked gh aw debug logs accidentally captured under .github/aw/logs/.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

Prompt-only change to a skill primitive; no code paths or tests exercise SKILL.md content. Verified the checklist still reads coherently (steps 1–9, cross-references intact).

Spec conformance (OpenAPM v0.1)

If this PR changes behaviour that an OpenAPM v0.1 req-XXX covers,
confirm the three-step ritual (see CONTRIBUTING.md "Adding or
changing a normative requirement"):

  • Spec edit: docs/src/content/docs/specs/openapm-v0.1.md updated
    (new/changed <a id="req-XXX"></a> anchor + prose + Appendix C
    row).
  • Manifest edit: docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.yml
    updated.
  • Test edit: a @pytest.mark.req("req-XXX") test under
    tests/spec_conformance/ added or extended.
  • CONFORMANCE.{md,json} regenerated via
    uv run --extra dev python -m tests.spec_conformance.gen_statement
    and committed.
  • N/A -- this PR does not change OpenAPM-observable behaviour.

Copilot AI linked an issue Jun 9, 2026 that may be closed by this pull request
Copilot AI and others added 3 commits June 9, 2026 14:28
Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
…s turn with empty safe outputs

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
…ion-confirmation in skill guidance

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix workflow failure in PR review panel fix(apm-review-panel): require synchronous fan-out so the panel never ends its turn with empty safe outputs Jun 9, 2026
Copilot AI requested a review from danielmeppiel June 9, 2026 14:32
@danielmeppiel

Copy link
Copy Markdown
Collaborator

Superseded by #1714, which audited this fix and rebuilt it around a stronger contract: a deterministic "Non-empty turn exit" invariant keyed off gh-aw's own agent_output emptiness detection (with the real, configured noop safe-output as the fallback), rather than LLM self-attestation. Same proximate cause (synchronous fan-out / no spawn-and-forget) is retained. Closing in favor of #1714.

danielmeppiel added a commit that referenced this pull request Jun 9, 2026
…fan-out (#1714)

* fix(apm-review-panel): guarantee non-empty turn exit via synchronous 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>

* chore(apm-review-panel): re-sync deployed .agents/skills copy with source

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>

---------

Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

[aw] PR Review Panel failed

2 participants