Skip to content

Fix(ci): publish governance warnings on pull requests from forks - #7876

Open
Critsium-xy wants to merge 1 commit into
deepmodeling:developfrom
Critsium-xy:fix/agent-governance-fork-pr-comment
Open

Fix(ci): publish governance warnings on pull requests from forks#7876
Critsium-xy wants to merge 1 commit into
deepmodeling:developfrom
Critsium-xy:fix/agent-governance-fork-pr-comment

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

Reminder

  • I have read AGENTS.md and docs/developers_guide/agent_governance.md.
  • I have linked an issue or explained why this PR does not need one.
  • I have added adequate unit tests and/or case tests, or explained why not.
  • I have listed the exact verification commands run and their results.
  • I have described user-visible behavior changes, including INPUT parameter changes.
  • I have explained core-module impact for ESolver, HSolver, ElecState, Hamilt, Operator, Psi, or other source/ changes.
  • I have requested any needed governance exception below.

Linked Issue

No issue. This is a self-contained CI defect found by reading
.github/workflows/agent_governance.yml; happy to open one first if maintainers
prefer that order.

The problem

Agent Governance runs on pull_request. For a pull request opened from a
fork, GitHub caps GITHUB_TOKEN at read-only regardless of the permissions:
block, so the comment step cannot post. The workflow guards against the
resulting failure with:

github.event.pull_request.head.repo.full_name == github.repository

The effect is that the warning comment is skipped for every fork pull request —
which is essentially all external contributions. Warnings land only in the run
summary, where contributors do not look. The check is quietly least useful
exactly where it was meant to help.

What's changed?

Split the check into the standard two-workflow pattern.

Trigger Token Job
Agent Governance pull_request contents: read runs the checker, uploads summary + PR number as an artifact
Agent Governance Comment (new) workflow_run pull-requests: write downloads that artifact, posts/updates the comment

workflow_run runs from the base branch with the base repository's
permissions, so the comment now lands on fork pull requests too. The
head.repo.full_name guard is gone.

Also caps the comment body under the 65536-character API limit, which the
previous version could exceed on a large diff, and appends a pointer to the run
summary when truncated.

Why this is not a pull_request_target style hole

The privileged workflow deliberately does not become an arbitrary-code-execution
surface:

  • It never checks out pull request code and never runs anything from the head.
  • Its only input is the text artifact uploaded by the analysis run.
  • Untrusted text reaches the API through files and jq -Rs, never through shell
    interpolation.
  • The PR number is re-derived with tr -dc '0-9' before it is used in an API
    path.
  • The analysis half is downgraded from pull-requests: write + issues: write
    to contents: read, so the half that does touch PR code now holds strictly
    fewer permissions than before.

Unit Tests and/or Case Tests for my changes

There is no test harness for workflow files, so the shell logic was extracted
from the shipped YAML and exercised directly against stubbed gh and jq,
over real agent_governance_check.py --format markdown output.

  • Commands run:
    • python tools/03_code_analysis/agent_governance_check.py --staged --format markdown
    • YAML parse of both workflow files
    • extracted Collect comment payload, Download comment payload and
      Comment governance warnings run: blocks, executed under bash with a
      gh/jq stub
  • Result summary:
    • governance checker: No findings., exit 0
    • both workflows parse
    • 25/25 shell assertions pass, covering: payload collection with and without a
      summary file, artifact present vs. absent, new comment (POST) vs. update of
      an existing comment (PATCH), clean summary posting nothing, incomplete
      payload, non-numeric PR number, and oversized-summary truncation staying
      under 65536 bytes
  • Checks not run, with reason:
    • No live end-to-end run. workflow_run workflows are read from the default
      branch, so Agent Governance Comment cannot execute from a PR branch by
      design. It becomes active only once this is merged into develop, and the
      first fork PR afterwards is the real end-to-end test. This is inherent to
      the pattern, not something this PR can work around.

Governance Notes

  • INPUT/docs changes: no INPUT change. docs/developers_guide/agent_governance.md
    gains a paragraph describing the two-workflow split and the merge-before-active
    caveat.
  • Core module impact: none. No source/ file is touched.
  • Exceptions requested: none.

Follow-up, not in this PR

The comment condition is grep -qi '^| warning |', so error-severity findings
still produce no comment even though they fail the job. That looks unintended,
but it is orthogonal to the fork permission bug and is left alone here to keep
this diff reviewable. Happy to send it separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RYWRHYxWwMgxoyWVYRHFn1

`Agent Governance` runs on `pull_request`, where a pull request from a fork
only ever receives a read-only `GITHUB_TOKEN`. The comment step therefore
guarded itself with

    github.event.pull_request.head.repo.full_name == github.repository

so the warning comment was skipped for every fork pull request, which is
almost all external contributions. Warnings were only visible in the run
summary, where contributors do not look.

Split the check into the standard two-workflow pattern:

- `Agent Governance` keeps running on `pull_request`, now with a `contents:
  read` token only. It uploads the rendered summary and the pull request
  number as an artifact.
- `Agent Governance Comment` is triggered by `workflow_run`, so it runs from
  the base branch with the base repository's permissions and can comment on
  fork pull requests.

The privileged workflow never checks out pull request code and never executes
anything from the head. Its only input is the uploaded text artifact, and
pull request supplied text reaches the API through files and `jq -Rs` rather
than shell interpolation, so this does not become a `pull_request_target`
style privilege escalation.

Also cap the comment body below the 65536 character API limit, which the
previous version could exceed on a large diff.

Verification:
- `python tools/03_code_analysis/agent_governance_check.py --staged
  --format markdown` -> "No findings.", exit 0
- Both workflow files parse as YAML.
- Extracted the three shipped shell blocks and ran them against stubbed `gh`
  and `jq` over the checker's real markdown output: 25/25 assertions pass,
  covering new comment vs. update of an existing comment, clean summary,
  missing or incomplete artifact, non-numeric pull request number, and
  oversized summary truncation.

Note: `workflow_run` workflows are read from the default branch, so the
comment half starts working only after this is merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RYWRHYxWwMgxoyWVYRHFn1
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.

1 participant