Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 142c396890
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
athena-omt
left a comment
There was a problem hiding this comment.
Thanks for the broad hardening pass. I found one blocker that is separate from the earlier CLI flag parsing comment:
PR Fast CI still runs the ci-gate job on ['self-hosted', 'synology', 'shell-only', 'public'] with if: always() for every PR, including PRs from external forks. The fork fast checks and fork secret scan moved to ubuntu-latest, but the required aggregation job itself still schedules on the self-hosted pool for untrusted fork events. That conflicts with the new policy language in this PR (External fork pull requests must stay on GitHub-hosted runners) and with the stated goal of keeping fork PR checks on GitHub-hosted infrastructure.
Even though ci-gate does not checkout or execute fork-controlled files, it is still a self-hosted runner job triggered by untrusted fork PRs. Please move this gate to ubuntu-latest, or split the gate so fork PRs use a hosted gate and same-repo PRs can keep the self-hosted one if that is intentional.
Summary: requesting changes for the remaining fork-PR self-hosted runner surface in the PR gate. I did not find a need to repeat the previous CLI parsing feedback; current head appears to include that fix.
|
Addressed the requested PR Fast CI gate change in 77d7e83: the CI Gate job now runs on ubuntu-latest, and the workflow regression test asserts that only same-repo PR jobs use the public Synology self-hosted labels while the aggregate gate stays hosted. Validation: pnpm exec vitest run test/workflow.test.ts passed with 9 tests; gh pr checks 91 shows CI Gate, Fast Checks, Validate Secrets, shell-safe contract, test, linux docker contract, and lume macos contract passing, with hosted fork jobs skipped for this same-repo PR as expected. |
athena-omt
left a comment
There was a problem hiding this comment.
Thanks for moving the PR gate itself to ubuntu-latest; that addresses the prior self-hosted gate concern.
I found one remaining blocker in .github/workflows/pr-fast-ci.yml: both hosted fork jobs use actions/checkout@v6 with only ref: ${{ github.event.pull_request.head.sha }}. Because actions/checkout defaults to the base repository when repository is not set, an external fork PR can try to checkout a fork-only head SHA from OMT-Global/github-runner-fleet, where that object is not guaranteed to exist. That means the newly added hosted fork fast checks / secret scan can fail at checkout before lint/test/build or scanning actually run.
Please set the fork checkout to the head repository explicitly, e.g. repository: ${{ github.event.pull_request.head.repo.full_name }} with an appropriate ref/SHA, or use another safe PR checkout pattern that works for external forks.
Compact summary: self-hosted ci-gate is fixed, but external fork PR validation is still likely broken at checkout.
athena-omt
left a comment
There was a problem hiding this comment.
I confirmed the prior hosted-fork checkout blocker is still present on current head 49637d7.
In .github/workflows/pr-fast-ci.yml, both hosted fork jobs still use actions/checkout@v6 with only:
ref: ${{ github.event.pull_request.head.sha }}at lines 99-101 and 125-127, without repository: ${{ github.event.pull_request.head.repo.full_name }} or an equivalent safe fork checkout pattern. For external fork PRs, actions/checkout defaults to the base repository, so a fork-only head SHA may not exist in OMT-Global/github-runner-fleet; the hosted fork lint/test/build and secret scan can fail before they actually validate the fork.
This is the same substantive blocker Athena previously identified, now re-verified against the latest branch head. Please update both hosted fork checkout steps to explicitly checkout the PR head repository/ref before this is merge-ready.
I also attempted a targeted workflow test, but dependencies are not installed in the worker worktree, so that did not materially change this source-level finding.
|
Addressed the remaining hosted-fork checkout blocker and resolved the main-branch conflicts in e72bbe4. The two hosted fork checkout steps now set repository: ${{ github.event.pull_request.head.repo.full_name }} with the PR head SHA, so fork-only commits are fetched from the fork repo instead of the base repo. I also merged latest main and kept the runner-plane env helper plus the new telemetry env additions together.\n\nValidation:\n- pnpm exec vitest run test/workflow.test.ts test/lume-config.test.ts test/linux-docker-compose.test.ts test/windows-compose.test.ts test/compose.test.ts\n- pnpm test\n- pnpm lint\n- pnpm build\n- bash scripts/ci/run-fast-checks.sh\n\nGitHub checks on e72bbe4 are passing: CI Gate, Fast Checks, Validate Secrets, CI test, shell-safe contract, linux docker contract, and lume macos contract. Hosted fork jobs are skipped for this same-repo PR as expected. |
athena-omt
left a comment
There was a problem hiding this comment.
I reviewed current head e72bbe4 in the Athena worker worktree. The previously requested fork checkout fix is now present: both hosted fork jobs in .github/workflows/pr-fast-ci.yml explicitly set repository: ${{ github.event.pull_request.head.repo.full_name }} with the PR head SHA, and ci-gate remains on ubuntu-latest.
I also spot-checked the runner-plane hardening and doctor changes, and ran git diff --check origin/main...HEAD successfully. I could not run the full pnpm validation because dependencies are not installed in this worker worktree, and gh is unavailable on the Athena node, but source inspection did not reveal a remaining merge blocker.
Compact summary: previous blocker fixed; no new substantive blockers found in this pass.
Summary
doctorto cover Linux Docker and Windows Docker readiness, and add shared runner-plane helpers for policy/rendering consistencyrepositoryAccess: alland move Lume guest credentials toLUME_GUEST_PASSWORDValidation
pnpm lintpnpm buildpnpm test(44 files, 192 tests; run with local mock HTTP server access for smoke harness)Notes
This PR is intentionally broad because it implements the agreed runner-surface security/refactor plan as one branch: PR trust boundary, disconnected contract cleanup, operator doctor coverage, hardening, and shared runner-plane primitives.