Skip to content

Fix PR Review Panel: emit advisory comment via add_comment tool, not a shell heredoc - #1844

Closed
Daniel Meppiel (danielmeppiel) with Copilot wants to merge 3 commits into
mainfrom
copilot/sergio-sisternes-epamfeat-installation-analytics-h
Closed

Fix PR Review Panel: emit advisory comment via add_comment tool, not a shell heredoc#1844
Daniel Meppiel (danielmeppiel) with Copilot wants to merge 3 commits into
mainfrom
copilot/sergio-sisternes-epamfeat-installation-analytics-h

Conversation

Copilot AI commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Description

The PR Review Panel workflow's agent job terminated unexpectedly (run 27815857237, 33m / 144 turns, no comment posted). gh aw audit shows the CEO synthesizer produced a valid needs_rework recommendation, but the orchestrator then staged the rendered comment body in a temp file with a shell heredoc:

cat > /tmp/gh-aw/agent/panel-comment.md << 'COMMENT_EOF'
... correctness bugs silently kill the feature's display layer ...

The copilot engine's bash command-safety parser scans command text (heredoc bodies included) and read a word-wrapped line beginning with kill as a real command, refusing it: "Command not executed. The 'kill' command must specify at least one numeric PID." The comment never emitted and the run failed. The add_comment safe-output tool already takes the body as a structured argument, so panel prose never needs to touch the shell.

Changes (prose-only in the skill):

  • apm-review-panel SKILL step 7 — mandate emitting the single comment by passing the rendered markdown directly to the add_comment tool's body argument; forbid staging/piping it through the shell (cat <<EOF, printf, echo, temp .md files), with the kill/rm/sudo parser hazard called out.
  • New Gotcha documenting the failure mode and citing run 27815857237.
  • Mirrored both edits into the installed .agents/skills/apm-review-panel/SKILL.md copy (kept byte-identical).
  • Removed accidentally-committed gh aw audit artifacts under .github/aw/logs/ and gitignored that path.

No workflow recompile needed — the workflow imports the skill from microsoft/apm#main at runtime, so .lock.yml is unaffected.

Type of change

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

Testing

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

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 and others added 2 commits June 19, 2026 10:18
Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
…edoc

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix workflow failure in PR review panel Fix PR Review Panel: emit advisory comment via add_comment tool, not a shell heredoc Jun 19, 2026
Daniel Meppiel (danielmeppiel) added a commit that referenced this pull request Sep 9, 2026
Carry the distinct #1844 safeguard into the canonical panel package and deployed ledger without recreating historical mirrors. Keep safe outputs fail-closed, prove transport clauses and provenance, and distinguish legacy unpack from plugin install in the actual pack handoff.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

Copy link
Copy Markdown
Collaborator

Superseded by #2897, which preserves this proposal's distinct structured-comment safeguard.

At replacement commit 3ca8dd44efb55a66ee30b3345a65574c2b3fd374, the canonical panel skill requires complete final markdown directly in structured add_comment.body: no shell staging, wrapper, or intermediate comment file. Missing, failed, or uncertain configured safeoutputs do not authorize a direct-write fallback. Incident run 27815857237 remains documented.

The deployed skill, canonical and compatibility hashes in apm.lock.yaml, and transport/provenance regression preserve the current package authority without recreating historical mirror paths.

Replacement evidence: the final current-head advisory, exact-head CI, and published APM 0.30.0 panel-resource roundtrip. The same nine retained reviewers and final CEO synthesis are complete. Local real-process restoration preserved all 12 actual package files; hosted deterministic compatibility covers four required files. Neither local CLI advisory delivery nor those checks is presented as hosted AI safe-output execution.

Closing this superseded proposal only, under the recovered preservation-and-linking authority. #2897 is ready for human review, not accepted or merged: CODEOWNER and independent last-push approvals remain outstanding. The fresh trusted-main #2741 AI exercise remains a post-merge human rollout obligation.

Original branch copilot/sergio-sisternes-epamfeat-installation-analytics-h and commit e00f905f97eefa6347013ab7a51f83152ce9e898 remain intact. No merge or branch deletion.

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