Gate tenant smoke tests only for pull requests#1988
Conversation
📝 WalkthroughWalkthroughThe smoke-test workflow now runs from trusted branch and pull request events, validates execution context, gates pull requests through a separate approval environment, pins privileged checkout to the workflow SHA, and broadens MT.1068 result validation. ChangesSmoke Test Gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Preflight
participant PRApproval
participant SmokeTest
participant ReportPublisher
GitHub->>Preflight: Trigger workflow
Preflight->>Preflight: Validate trusted ref and PR target
Preflight->>PRApproval: Continue pull request flow
PRApproval->>PRApproval: Evaluate maester-smoke-test-pr
PRApproval->>SmokeTest: Permit approved or non-PR execution
SmokeTest->>ReportPublisher: Publish report from github.sha
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/maester-action-smoke-test.yaml:
- Line 37: Update the job-level if condition in the workflow to stop excluding
draft pull requests by removing the github.event.pull_request.draft check.
Preserve the existing pull_request_target versus other-event logic so preflight
runs for opened or synchronized external PRs and allows approve-pr to proceed.
- Around line 79-82: Update the smoke job’s credentialed gate condition to be
cancellation-aware by requiring !cancelled(), allow always() only for
non-pull_request_target runs, and require needs.approve-pr.result == 'success'
for pull_request_target events. Preserve the existing preflight success
requirement while ensuring skipped approval cannot grant credential access to
PR-triggered runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad84a08d-20b4-4d34-a724-175887aeec26
📒 Files selected for processing (2)
.github/workflows/maester-action-smoke-test.md.github/workflows/maester-action-smoke-test.yaml
| name: Validate trusted ref | ||
| if: github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success' | ||
| name: Validate trusted execution context | ||
| if: github.event_name != 'pull_request_target' || !github.event.pull_request.draft |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file =="
if [ -f .github/workflows/maester-action-smoke-test.yaml ]; then
cat -n .github/workflows/maester-action-smoke-test.yaml
else
echo "workflow file not found"
fi
echo "== relevant docs references in repo =="
rg -n "draft|pending approval|approval|opened|synchronized|pull_request_target" -S . || true
echo "== git diff stat/name =="
git diff --stat || true
git diff -- .github/workflows/maester-action-smoke-test.yaml || trueRepository: maester365/maester
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow existence =="
git ls-files | grep -F '.github/workflows/maester-action-smoke-test.yaml' || true
echo "== workflow file first 140 lines =="
if [ -f .github/workflows/maester-action-smoke-test.yaml ]; then
sed -n '1,140p' .github/workflows/maester-action-smoke-test.yaml | cat -n
fi
echo "== job dependency/dependency-related docs references in workflow =="
if [ -f .github/workflows/maester-action-smoke-test.yaml ]; then
rg -n "needs:|if:|always\\(|preflight|approval|smoke|pull_request_target|draft|on:" .github/workflows/maester-action-smoke-test.yaml
fiRepository: maester365/maester
Length of output: 8501
Align draft PR handling with the documented trigger contract.
pull_request_target includes draft PRs (github.event.pull_request.draft exists), but this condition skips preflight, and approve-pr depends on it via needs. Draft PRs therefore do not create the approval/smoke run described for opened or synchronized external PRs. Remove !github.event.pull_request.draft or explicitly document that draft PRs are intentionally excluded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/maester-action-smoke-test.yaml at line 37, Update the
job-level if condition in the workflow to stop excluding draft pull requests by
removing the github.event.pull_request.draft check. Preserve the existing
pull_request_target versus other-event logic so preflight runs for opened or
synchronized external PRs and allows approve-pr to proceed.
| if: >- | ||
| always() && | ||
| needs.preflight.result == 'success' && | ||
| (needs.approve-pr.result == 'success' || needs.approve-pr.result == 'skipped') |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate workflow =="
fd -a 'maester-action-smoke-test.yaml' . || true
echo "== workflow around gated step =="
if [ -f .github/workflows/maester-action-smoke-test.yaml ]; then
wc -l .github/workflows/maester-action-smoke-test.yaml
sed -n '1,140p' .github/workflows/maester-action-smoke-test.yaml | cat -n
fi
echo "== related workflow files =="
fd -a '\.yaml$|\.yml$' .github/workflows | sed 's#^\./##' | sort
echo "== search approve and tenant access references =="
rg -n "approve-pr|approve|maester-action|tenant|secrets|permissions:|pull_request_target|cancel-in-progress|if: >-|cancelled\(" .github/workflows || trueRepository: maester365/maester
Length of output: 19797
🌐 Web query:
GitHub Actions workflow syntax always() skipped jobs cancel-in-progress jobs.context needs result skipped
💡 Result:
In GitHub Actions, jobs are skipped by default if any of their dependencies (defined in the needs property) fail or are skipped [1][2]. This behavior applies to the entire dependency chain from the point of failure or skip onwards [1][3][2]. To force a job to run regardless of the status of its dependencies—including when they are skipped—you must use the always() conditional function in the job-level if expression [1][2]. Key Considerations: 1. Default Behavior: By default, if a job has needs dependencies, an implicit success() status check is applied [4][5]. If any job in the dependency chain is skipped, the dependent job is also skipped because the default success() check evaluates to false [1][2][5]. 2. Using always: When you include if: ${{ always() }}, it overrides this default behavior, allowing the job to execute even if preceding jobs were skipped or failed [1][3][2]. 3. Context Access: You can check the specific status of dependencies using the needs context, which includes the result property [6]. Possible values for needs.<job_id>.result are success, failure, cancelled, or skipped [6]. If you need a job to run only under specific conditions (e.g., only if a previous job was skipped), you can combine always() with a check on the result property [7][4][5]: if: ${{ always && needs.job_a.result == 'skipped' }} 4. Dependency Chains: If you have a long chain of jobs, simply using always() on the final job is often insufficient if intermediate jobs in the chain were also skipped [3]. Every job in a dependency chain that you intend to run after a skip or failure typically requires its own if: ${{ always() }} condition to ensure it is not automatically skipped by the dependency chain rules [3]. Top results supporting this information: [1][3][2][6][4]
Citations:
- 1: https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/use-jobs
- 2: https://docs.github.com/actions/using-jobs/using-jobs-in-a-workflow
- 3: Clarify expressions and provide examples for workflows with skipped jobs github/docs#28762
- 4: Add missing 'skipped' option to
needsgithub/docs#411 - 5: Jobs skipped when NEEDS job ran successfully actions/runner#2205
- 6: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
- 7: Job-level "if" condition not evaluated correctly if job in "needs" property is skipped actions/runner#491
🌐 Web query:
site:docs.github.com/en/actions/using-jobs/using-jobs-in-a-workflow always() cancel-in-progress
💡 Result:
GitHub Docs say always() can make a dependent job run even if earlier jobs fail, but during workflow cancellation GitHub re-evaluates if conditions and a job with if: always() will continue running instead of being canceled. cancel-in-progress: true is part of concurrency and cancels in-progress runs in the same concurrency group. So always() and cancel-in-progress can conflict if you expect a job to stop on cancellation. (docs.github.com)
If you want, I can show the exact YAML pattern to avoid this.
Citations:
Make the credentialed gate fail closed and cancellation-aware.
smoke currently reaches tenant secrets whenever approve-pr is skipped, which the workflow currently allows for non-PR events. Include always() only for non-PR runs, require needs.approve-pr.result == 'success' for pull_request_target, and guard the whole condition with !cancelled() so PR cancellation does not let privileged jobs continue or start.
Proposed fix
- if: >-
- always() &&
- needs.preflight.result == 'success' &&
- (needs.approve-pr.result == 'success' || needs.approve-pr.result == 'skipped')
+ if: >-
+ ${{ !cancelled() &&
+ needs.preflight.result == 'success' &&
+ (github.event_name != 'pull_request_target' ||
+ needs.approve-pr.result == 'success') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: >- | |
| always() && | |
| needs.preflight.result == 'success' && | |
| (needs.approve-pr.result == 'success' || needs.approve-pr.result == 'skipped') | |
| if: >- | |
| ${{ !cancelled() && | |
| needs.preflight.result == 'success' && | |
| (github.event_name != 'pull_request_target' || | |
| needs.approve-pr.result == 'success') }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/maester-action-smoke-test.yaml around lines 79 - 82,
Update the smoke job’s credentialed gate condition to be cancellation-aware by
requiring !cancelled(), allow always() only for non-pull_request_target runs,
and require needs.approve-pr.result == 'success' for pull_request_target events.
Preserve the existing preflight success requirement while ensuring skipped
approval cannot grant credential access to PR-triggered runs.
Deploying maester with
|
| Latest commit: |
b80ab49
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://abf22638.maester.pages.dev |
| Branch Preview URL: | https://codex-gate-smoke-pr-runs.maester.pages.dev |
1 participant
Summary
main, including merged pull requestscore-moduleapproval only for pull request-triggered smoke runsmainrunsMT.1068result instances while continuing to reject any unexpected or incomplete executed testEnvironment configuration applied
maester-smoke-test: no required reviewer,mainonly, admin bypass disabled; retains the tenant and report credentialsmaester-smoke-test-pr:core-moduleis the sole required reviewer, self-approval allowed,mainonly, admin bypass disabled; contains no secrets or variablesNo additional Microsoft Entra federated credential or GitHub token permission is required.
Validation
actionlint1.7.12git diff --checkThe live run exposed two completed
MT.1068result instances on each OS. Its original single-instance assertion failed, and this PR now validates that all executed instances areMT.1068and complete normally instead.Summary by CodeRabbit
main, pull requests targetingmain, scheduled runs, and manual dispatches.