ci: publish research image from exact-main artifact - #332
Conversation
📝 WalkthroughWalkthroughThe PR adds source-selection logic for ACR publishing, authenticated research release manifests, exact-source artifact handling, and workflow assertions covering creation, verification, smoke consumption, and image revision binding. ChangesACR research release flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PredictionMarketsCI
participant SourceSelector
participant ReleaseArtifact
participant ACRPublish
participant Docker
PredictionMarketsCI->>SourceSelector: completed workflow result, head SHA, run ID
SourceSelector->>ACRPublish: publish_target, research_mode, source_sha, artifact_run_id
ACRPublish->>ReleaseArtifact: create or download and verify release
ReleaseArtifact->>ACRPublish: verified research binaries
ACRPublish->>Docker: build image with selected source revision
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
b291f5c to
098adf2
Compare
098adf2 to
80ba7f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/scripts/test-research-image-release-artifact.sh (1)
69-91: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise malformed manifest rejection.
These cases mutate binaries and verifier inputs, but never corrupt or remove required fields from the manifest emitted by
create. Add malformed-syntax and missing-binding/digest-field cases that must fail verification, preserving the fail-closed artifact contract.🤖 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/scripts/test-research-image-release-artifact.sh around lines 69 - 91, Extend assert_rejected and its invocation cases to corrupt the generated manifest itself, covering malformed syntax and missing required binding or digest fields. Ensure each mutated candidate is passed to artifact verify and fails closed, while preserving the existing binary, source, run, and lock mismatch cases..github/workflows/acr-publish.yml (1)
179-190: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueHarden the repeated
source_sha/run_idtemplate interpolation intorun:scripts.zizmor flags direct
${{ needs.selector.outputs.* }}expansion insiderun:at this site and recurring at lines 239-240, 376, 427, 485, and 505. The upstream regex validation (^[0-9a-f]{40}$/^[1-9][0-9]*$) inselect-acr-publish-source.shneutralizes real injection risk today, but as defense-in-depth it's cheap to route these throughenv:(as already done for thesource-jobs/sourcesteps) so a future relaxation of that regex can't reintroduce an injection vector.♻️ Example for this site
+ env: + SOURCE_SHA: ${{ needs.selector.outputs.source_sha }} + ARTIFACT_RUN_ID: ${{ needs.selector.outputs.artifact_run_id }} run: | ... - ../.github/scripts/research-image-release-artifact.sh create research-release \ - "${{ needs.selector.outputs.source_sha }}" \ - "${{ needs.selector.outputs.artifact_run_id }}" . + ../.github/scripts/research-image-release-artifact.sh create research-release \ + "$SOURCE_SHA" "$ARTIFACT_RUN_ID" .🤖 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/acr-publish.yml around lines 179 - 190, Harden the repeated template interpolations in the workflow’s run scripts by passing needs.selector.outputs.source_sha and artifact_run_id through step-level env variables, then reference those environment variables in the scripts. Apply this consistently to the current release-artifact command and the recurring sites at lines 239-240, 376, 427, 485, and 505, matching the existing source-jobs/source pattern while preserving the validated values and behavior.Source: Linters/SAST tools
🤖 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/acr-publish.yml:
- Around line 58-63: Update job_conclusion to explicitly check that matches
contains exactly one line before printing it; return a nonzero status when there
are zero or multiple matching jobs, so command substitution and the workflow
fail instead of writing an ambiguous GITHUB_OUTPUT value.
---
Nitpick comments:
In @.github/scripts/test-research-image-release-artifact.sh:
- Around line 69-91: Extend assert_rejected and its invocation cases to corrupt
the generated manifest itself, covering malformed syntax and missing required
binding or digest fields. Ensure each mutated candidate is passed to artifact
verify and fails closed, while preserving the existing binary, source, run, and
lock mismatch cases.
In @.github/workflows/acr-publish.yml:
- Around line 179-190: Harden the repeated template interpolations in the
workflow’s run scripts by passing needs.selector.outputs.source_sha and
artifact_run_id through step-level env variables, then reference those
environment variables in the scripts. Apply this consistently to the current
release-artifact command and the recurring sites at lines 239-240, 376, 427,
485, and 505, matching the existing source-jobs/source pattern while preserving
the validated values and behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbf0a199-54da-4d81-80bf-c27581c7980f
📒 Files selected for processing (8)
.github/scripts/research-image-release-artifact.sh.github/scripts/select-acr-publish-source.sh.github/scripts/test-acr-publish-workflow.sh.github/scripts/test-research-image-release-artifact.sh.github/workflows/acr-publish.yml.github/workflows/ploy-ci.ymldeployment/aliyun/test-polymarket-raw-ops-control-plane.shdeployment/aliyun/test-rust-lob-control-plane.sh
| job_conclusion() { | ||
| local name=$1 matches | ||
| matches=$(awk -F '\t' -v name="$name" '$1 == name {print $2}' <<<"$jobs") | ||
| [[ -n "$matches" && "$matches" != *$'\n'* ]] | ||
| printf '%s\n' "$matches" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does bash's set -epropagate into command substitution subshells by default, or does it requireshopt -s inherit_errexit?
💡 Result:
By default, Bash does not propagate the errexit (set -e) option into command substitution subshells [1][2]. When not in POSIX mode, Bash explicitly clears the errexit option within these subshells [1][2]. To change this behavior and force command substitution subshells to inherit the errexit setting from the parent shell, you must enable the inherit_errexit shell option using shopt -s inherit_errexit [3][4]. This option is automatically enabled when Bash is running in POSIX mode [3]. It is important to note that enabling inherit_errexit does not guarantee that the parent script will exit on every error within a command substitution [5][4]. The set -e option only causes the shell to exit when a command that fails is a "simple command" (or part of certain constructs) and its failure status is not otherwise handled or ignored [5]. If a command substitution is embedded within a larger command (e.g., echo $(false) or export var=$(false)), the containing command may still succeed because the command substitution's failure does not necessarily propagate to the status of the outer command [5][4]. To ensure the script halts on an error, the command substitution should typically be performed as a direct, standalone assignment (e.g., var=$(false)) [5][4].
Citations:
- 1: https://stackoverflow.com/questions/19054868/using-command-substitution-or-similar-but-still-having-script-exit-using-set
- 2: https://stackoverflow.com/questions/29532904/bash-subshell-errexit-semantics
- 3: https://www.gnu.org.cach3.com/software/bash/manual/html_node/The-Shopt-Builtin.html
- 4: https://unix.stackexchange.com/questions/613019/subshell-not-propagating-error-bash-4-4
- 5: https://unix.stackexchange.com/questions/674037/when-does-inherit-errexit-not-work
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant workflow lines around the function and its call.
sed -n '45,80p' .github/workflows/acr-publish.yml
# Check whether the workflow contains errexit, inherit_errexit, or POSIX mode settings.
rg -n 'set -e|errexit|inherit_errexit|POSIX|shopt' .github/workflows/acr-publish.yml || true
# Behavioral probe: demonstrate the bare guard inside command substitution under bash.
bash - <<'BASH'
job_conclusion() {
local name=$1 matches
local jobs=$'ok\tgood'
matches=$(awk -F '\t' -v name="$name" '$1 == name {print $2}' <<<"$jobs")
[[ -n "$matches" && "$matches" != *$'\n'* ]]
printf '%s\n' "$matches"
}
# This assignment succeeds even though the guard fails silently.
out=$(job_conclusion ok)
printf 'exit status and output for command-substitution call: exit=%s out=%q\n' "$?" "$out"
BASHRepository: proerror77/monday
Length of output: 2092
Make job_conclusion fail when there are zero or multiple matching jobs.
The bare condition inside $(job_conclusion ...) succeeds without stopping execution when awk returns no line or several lines, so $GITHUB_OUTPUT can receive an ambiguous value. Add explicit error handling around the guard.
🔧 Suggested fix
job_conclusion() {
local name=$1 matches
matches=$(awk -F '\t' -v name="$name" '$1 == name {print $2}' <<<"$jobs")
- [[ -n "$matches" && "$matches" != *$'\n'* ]]
+ [[ -n "$matches" && "$matches" != *$'\n'* ]] || {
+ printf 'ambiguous or missing job conclusion for %s\n' "$name" >&2
+ exit 1
+ }
printf '%s\n' "$matches"
}📝 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.
| job_conclusion() { | |
| local name=$1 matches | |
| matches=$(awk -F '\t' -v name="$name" '$1 == name {print $2}' <<<"$jobs") | |
| [[ -n "$matches" && "$matches" != *$'\n'* ]] | |
| printf '%s\n' "$matches" | |
| } | |
| job_conclusion() { | |
| local name=$1 matches | |
| matches=$(awk -F '\t' -v name="$name" '$1 == name {print $2}' <<<"$jobs") | |
| [[ -n "$matches" && "$matches" != *$'\n'* ]] || { | |
| printf 'ambiguous or missing job conclusion for %s\n' "$name" >&2 | |
| exit 1 | |
| } | |
| printf '%s\n' "$matches" | |
| } |
🤖 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/acr-publish.yml around lines 58 - 63, Update
job_conclusion to explicitly check that matches contains exactly one line before
printing it; return a nonzero status when there are zero or multiple matching
jobs, so command substitution and the workflow fail instead of writing an
ambiguous GITHUB_OUTPUT value.
2 participants
Change contract
A successful relevant main Prediction Markets CI run publishes one short-lived manifest-bound Research release artifact, and automated ACR packaging consumes exactly that authenticated run artifact without rebuilding.
Out of scope
Rust behavior, Docker image redesign, compiler/cache policy, cache-save timing, ACR Enterprise migration, self-hosted runners, collector deployment, and branch-protection policy.
Dependency / merge order
Depends on #316, merged via #329. No stacked PR remains; this PR targets
maindirectly.Focused validation
.github/scripts/test-research-image-release-artifact.sh.github/scripts/test-acr-publish-workflow.sh.github/scripts/test-select-rust-ci-scope.shbash -nand ShellCheck for all changed shell scriptsactionlintforploy-ci.ymlandacr-publish.ymlgit diff --check origin/main...HEADCounterexamples cover failed/PR/non-main/partial source runs, implicit manual rebuild, missing/extra/tampered binaries, source/run/Cargo.lock mismatch, and irrelevant skipped Research jobs.
Rollout / rollback impact
Automated Research ACR publication begins only after a successful relevant
mainPrediction Markets CI run and uses its exact run ID and head SHA. Artifacts retain for one day. Missing, expired, malformed, or mismatched artifacts fail closed with no rebuild. Manual Research publication requires the explicit rebuild checkbox. Rollback is a workflow/script revert; existing image contents and runtime deployments are unchanged.Scope exception
None. Eight files and 491 changed lines.
Closes #317
Summary by CodeRabbit
New Features
Bug Fixes
Tests