Skip to content

ci(review): route Claude review through fleet - #142

Closed
kyle-sexton wants to merge 3 commits into
mainfrom
agent/issue-78-claude-review-fleet
Closed

ci(review): route Claude review through fleet#142
kyle-sexton wants to merge 3 commits into
mainfrom
agent/issue-78-claude-review-fleet

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

  • route the Claude review reusable caller through the repository's governed runner selector
  • pin the fleet-capable reusable workflow and pass the selector result with the established hosted fallback
  • remove the now-obsolete hosted-runner exception

This advances melodic-software/github-iac#78.

Dependency

Depends on #140 plus the generated runner-policy sync. Current old-policy CI may fail or queue until then. This is intentionally a dependency-blocked draft.

No managed files were edited; this changes only the repository-local caller and repository policy exception.

Validation

The prior routing-only fix cleared privileged-hosted-only but left
hosted-exception-required unresolved. Route claude-review.yml's review
job to always-hosted (ubuntu-24.04), drop the now-orphaned
select-runner job, and register a runner-policy.json exception
(privileged-control-plane) matching the existing
publish-packages.yml#publish precedent: the job mints a Claude token
and writes pull-request feedback with write authority, so it stays on
GitHub-hosted infrastructure.
…-review-fleet

# Conflicts:
#	.github/workflows/claude-review.yml
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 35s —— View job


PR Review

  • Gather context (PR metadata, branch state)
  • Inspect commits and diff
  • Read relevant files
  • Post review findings

Summary

This PR currently has zero net changes versus origin/main. All three branch-local commits cancel out by the time of the merge-sync (ab6fb82):

Commit Intent Net effect
efb268e Route review through fleet: add select-runner job, drop policy exception, bump pin +18 lines / −5 lines
7e821c7 Revert to always-hosted: re-add exception, drop select-runner, hardcode runner: ubuntu-24.04 Largely reverses efb268e
ab6fb82 Merge sync from origin/main (which carries df54d0e pin via related work) Final diff vs main = zero

Merging this PR as-is would be a no-op squash commit.


Findings

🔴 Important — PR description does not match the committed state

The PR body describes three outcomes that were explicitly reversed by 7e821c7:

  • route the Claude review reusable caller through the repository's governed runner selector
  • pass the selector result with the established hosted fallback
  • remove the now-obsolete hosted-runner exception

Commit 7e821c7's own message acknowledges the reversal: hosted-exception-required remained unresolved after the fleet-routing commit, so the approach was changed to always-hosted and the exception was restored. The PR description was not updated to reflect this pivot. If the PR is still meant to land the original fleet-routing design, the prerequisite (clearing hosted-exception-required in the upstream ci-workflows contract) needs to be met first. If the goal shifted to the always-hosted approach, the title and summary need to be rewritten to describe what 7e821c7 actually does.

🔴 Important — Zero diff makes the PR unmerge-able in a meaningful sense

After ab6fb82 merged origin/main (which itself received the df54d0e pin through related upstream work), the branch is byte-for-byte identical to main. The intended change — whichever form it takes after the description is reconciled — needs to be re-introduced on top of the current main HEAD before this PR can be reviewed on its substance.


Observations on 7e821c7's approach (for when the PR is re-worked)

The always-hosted reasoning in 7e821c7 is sound:

  • The privileged-control-plane exception category fits: the job mints a Claude OIDC token and writes PR feedback with pull-requests: write, matching the publish-packages.yml#publish precedent already in runner-policy.json.
  • Keeping the exception rather than routing through the fleet avoids a hosted-exception-required policy violation in the upstream contract — a constraint that would need to be cleared in ci-workflows before fleet routing is viable.

If the PR pivots to the always-hosted design, the changes in 7e821c7 (exception retained, runner: ubuntu-24.04 hardcoded, no select-runner job) are consistent and properly justified. The new pin (4dbb0df → df54d0e after sync) just needs to be the one the PR lands with, verified against the ci-workflows commit date comment convention.


Recommended next steps

  1. Decide: is the goal still fleet routing (requiring upstream ci-workflows work to clear hosted-exception-required), or always-hosted with a policy exception?
  2. Rewrite the PR title and summary to describe the chosen approach.
  3. Rebase the branch onto current main and re-introduce whichever change set matches the decision, so the PR carries a non-zero diff.

No blocking criteria from REVIEW.md (secrets, injection, authorization) apply to the zero-diff state; the security posture of the current file on main is unchanged.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closing as superseded — this branch has since converged to be byte-identical to main (zero diff after merging main in). #140 (allowedCallerPermissions validation for the exact ci-workflows SHA this PR pinned) and #148 (bumped the claude-review pin further to a newer, natively hosted-only SHA) both merged and independently arrived at the same conclusion this PR's own final commit had already reached: claude-review stays hosted-only, no fleet routing needed. Nothing left here to merge.

kyle-sexton added a commit that referenced this pull request Jul 17, 2026
…ier (#158)

## What

- Add a `select-review` job resolving `${{
vars.CI_REVIEW_SELF_HOSTED_LABEL }}` through the governed selector
(pinned `select-runner@cdc5917c`).
- Route the `review` job's runner from the selector output to the fleet
claude-review reusable (pinned `claude-review@4dbb0dfc`), permissions
exactly per the reviewed caller-perms waiver.
- Drop the `privileged-control-plane` hosted exception for
`claude-review.yml#review`.

## Why

Campaign A of the zero-hosted directive (epic
melodic-software/github-iac#78): automated review moves to the dedicated
capped review tier. Shape matches this repo's own runner-policy E2E
fixture (#155); `runner-policy.mjs --root .` passes locally with zero
findings.

Supersedes the closed draft #142.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01K3QehVwmWzkBLpKokNCkkt

---------

Co-authored-by: Claude Fable 5 (1M context) <noreply@anthropic.com>
@kyle-sexton
kyle-sexton deleted the agent/issue-78-claude-review-fleet branch July 18, 2026 21:59
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