HYPERFLEET-991 - feat: add PR risk scoring presubmit job#79325
Conversation
WalkthroughAdds an optional ChangesProw configuration update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse auto-ack |
|
@rafabene: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![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 (3)
ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh (3)
56-56: ⚡ Quick winYAML parsing with sed is fragile.
The
sedcommand assumes specific YAML formatting and doesn't handle comments, varied indentation, or complex YAML structures. A malformed or unconventional.risk-config.yamlcould cause silent failures or incorrect parsing.Consider using
yqfor robust YAML parsing:♻️ Alternative implementation with yq
if [ -f .risk-config.yaml ]; then - CUSTOM=$(sed -n '/^sensitive_paths:/,/^[^ ]/{ /^ - /s/^ - //p }' .risk-config.yaml | tr '\n' ' ') + CUSTOM=$(yq eval '.sensitive_paths[]' .risk-config.yaml 2>/dev/null | tr '\n' ' ') [ -n "${CUSTOM}" ] && SENSITIVE_PATHS="${CUSTOM}" fiNote: Verify that
yqis available in thesrccontainer image, or fall back to the currentsedapproach with a comment acknowledging the limitation.🤖 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 `@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh` at line 56, The sed-based extraction assigned to CUSTOM (the line setting CUSTOM=$(sed -n '/^sensitive_paths:/,/^[^ ]/{ /^ - /s/^ - //p }' .risk-config.yaml | tr '\n' ' ')) is fragile for YAML; replace it with a robust yq-based parse to read the sensitive_paths array from .risk-config.yaml (e.g., using yq to extract .sensitive_paths[] and join with spaces) and fall back to the original sed command only if yq is not available, adding a comment that explains the fallback and the limitation; ensure the new logic preserves the same final format (space-separated values in CUSTOM) and references the CUSTOM variable and .risk-config.yaml so reviewers can find the change.
119-119: 💤 Low valueConsider using parameter expansion instead of sed.
Shellcheck SC2001 suggests a simpler alternative that doesn't require a subprocess.
♻️ Use parameter expansion
- encoded=$(echo "${old}" | sed 's|/|%2F|g') + encoded="${old//\//%2F}"🤖 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 `@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh` at line 119, Replace the sed subprocess used to percent-encode slashes with shell parameter expansion: instead of running sed on the variable old, assign encoded by expanding old with a global replacement of "/" to "%2F" (use the ${old//\//%2F} style) so you avoid spawning an external process; update the assignment that currently sets encoded from the sed command accordingly.
34-36: ⚡ Quick winConsider validating PULL_BASE_SHA before use.
While
PULL_BASE_SHAshould be present in PR contexts, adding an explicit check would make the script more defensive against unexpected CI environments.🛡️ Add validation
+if [ -z "${PULL_BASE_SHA:-}" ]; then + echo "ERROR: PULL_BASE_SHA not set." + exit 1 +fi + DIFF_FILES=$(git diff --name-only "${PULL_BASE_SHA}..HEAD" || true)🤖 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 `@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh` around lines 34 - 36, The script uses PULL_BASE_SHA to compute DIFF_FILES and LINES_CHANGED but doesn't validate it; add a guard that checks if the PULL_BASE_SHA variable is unset or empty before running git commands (reference PULL_BASE_SHA, DIFF_FILES, LINES_CHANGED), and if missing print a clear error message to stderr and exit with a non-zero status (or set safe defaults and continue if that behavior is preferred) so the git diff invocations are not run with an invalid SHA.
🤖 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
`@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh`:
- Line 61: The current conditional uses grep "^${path}" which treats ${path} as
a regex and can mis-match paths containing regex metacharacters; update the
check so ${path} is matched literally and still anchored to the start of each
DIFF_FILES line (e.g., use grep -F with an anchored start or iterate lines and
use shell prefix matching). Specifically, change the if that references
DIFF_FILES and path to perform a literal, prefix-only match (examples: pipe
DIFF_FILES through grep -F and then filter for lines starting with the literal
path, or read DIFF_FILES line-by-line and use [[ "$file" == "${path}"* ]] to
detect a prefix) so sensitive paths from .risk-config.yaml are not interpreted
as regexes.
---
Nitpick comments:
In
`@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh`:
- Line 56: The sed-based extraction assigned to CUSTOM (the line setting
CUSTOM=$(sed -n '/^sensitive_paths:/,/^[^ ]/{ /^ - /s/^ - //p }'
.risk-config.yaml | tr '\n' ' ')) is fragile for YAML; replace it with a robust
yq-based parse to read the sensitive_paths array from .risk-config.yaml (e.g.,
using yq to extract .sensitive_paths[] and join with spaces) and fall back to
the original sed command only if yq is not available, adding a comment that
explains the fallback and the limitation; ensure the new logic preserves the
same final format (space-separated values in CUSTOM) and references the CUSTOM
variable and .risk-config.yaml so reviewers can find the change.
- Line 119: Replace the sed subprocess used to percent-encode slashes with shell
parameter expansion: instead of running sed on the variable old, assign encoded
by expanding old with a global replacement of "/" to "%2F" (use the
${old//\//%2F} style) so you avoid spawning an external process; update the
assignment that currently sets encoded from the sed command accordingly.
- Around line 34-36: The script uses PULL_BASE_SHA to compute DIFF_FILES and
LINES_CHANGED but doesn't validate it; add a guard that checks if the
PULL_BASE_SHA variable is unset or empty before running git commands (reference
PULL_BASE_SHA, DIFF_FILES, LINES_CHANGED), and if missing print a clear error
message to stderr and exit with a non-zero status (or set safe defaults and
continue if that behavior is preferred) so the git diff invocations are not run
with an invalid SHA.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c00a1bc2-5c27-4451-a570-2cd4978ab5d1
⛔ Files ignored due to path filters (4)
ci-operator/jobs/openshift-hyperfleet/hyperfleet-adapter/openshift-hyperfleet-hyperfleet-adapter-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift-hyperfleet/hyperfleet-api/openshift-hyperfleet-hyperfleet-api-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift-hyperfleet/hyperfleet-broker/openshift-hyperfleet-hyperfleet-broker-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift-hyperfleet/hyperfleet-sentinel/openshift-hyperfleet-hyperfleet-sentinel-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/openshift-hyperfleet/hyperfleet-adapter/openshift-hyperfleet-hyperfleet-adapter-main.yamlci-operator/config/openshift-hyperfleet/hyperfleet-api/openshift-hyperfleet-hyperfleet-api-main.yamlci-operator/config/openshift-hyperfleet/hyperfleet-broker/openshift-hyperfleet-hyperfleet-broker-main.yamlci-operator/config/openshift-hyperfleet/hyperfleet-sentinel/openshift-hyperfleet-hyperfleet-sentinel-main.yamlci-operator/step-registry/hyperfleet/risk-scorer/OWNERSci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.shci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-ref.metadata.jsonci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-ref.yaml
| fi | ||
| SENSITIVE_HIT="" | ||
| for path in ${SENSITIVE_PATHS}; do | ||
| if echo "${DIFF_FILES}" | grep -q "^${path}"; then |
There was a problem hiding this comment.
Use literal matching for file path prefixes.
The grep "^${path}" treats path as a regex pattern. If custom sensitive paths from .risk-config.yaml contain regex metacharacters (e.g., ., [, *), they could match unintended files.
🔒 Use grep -F for literal matching
- if echo "${DIFF_FILES}" | grep -q "^${path}"; then
+ if echo "${DIFF_FILES}" | grep -qF "${path}"; then
SENSITIVE_HIT="${SENSITIVE_HIT} ${path}"
fiNote: This matches paths containing the literal string rather than anchoring to the start. If prefix-only matching is required, consider:
if echo "${DIFF_FILES}" | grep -F "${path}" | grep -q "^${path}"; thenor process paths line-by-line:
while IFS= read -r file; do
[[ "$file" == "${path}"* ]] && SENSITIVE_HIT="${SENSITIVE_HIT} ${path}" && break
done <<< "${DIFF_FILES}"🤖 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
`@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh`
at line 61, The current conditional uses grep "^${path}" which treats ${path} as
a regex and can mis-match paths containing regex metacharacters; update the
check so ${path} is matched literally and still anchored to the start of each
DIFF_FILES line (e.g., use grep -F with an anchored start or iterate lines and
use shell prefix matching). Specifically, change the if that references
DIFF_FILES and path to perform a literal, prefix-only match (examples: pipe
DIFF_FILES through grep -F and then filter for lines starting with the literal
path, or read DIFF_FILES line-by-line and use [[ "$file" == "${path}"* ]] to
detect a prefix) so sensitive paths from .risk-config.yaml are not interpreted
as regexes.
Add a new step-registry ref `hyperfleet-risk-scorer` that computes a deterministic risk score for PRs based on diff size, sensitive path changes, and test coverage. The job applies a risk/low, risk/medium, or risk/high GitHub label and posts a score breakdown comment. The job is configured as optional (never blocks merge) and uses the `hyperfleet-ci-bot` GitHub App for API access. Enabled for: hyperfleet-adapter, hyperfleet-api, hyperfleet-broker, hyperfleet-sentinel.
rafabene
force-pushed
the
HYPERFLEET-991-risk-scorer
branch
from
add5b0b to
7079a48
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@ci-operator/config/openshift-hyperfleet/hyperfleet-sentinel/openshift-hyperfleet-hyperfleet-sentinel-main.yaml`:
- Around line 40-44: The new tests entry (as: risk-scorer / ref:
hyperfleet-risk-scorer) was added only to ci-operator config and you must run
the generator to produce the corresponding Prow job; run the repository job-gen
step (make update) to regenerate ci-operator/jobs and prow configs, verify the
generated presubmit for the risk-scorer ref appears under ci-operator/jobs and
jobs/... , and commit those generated files alongside your changed
openshift-hyperfleet-hyperfleet-sentinel-main.yaml so the new presubmit is
actually wired up.
In
`@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh`:
- Around line 22-28: The script enables xtrace via "$WAS_TRACING && set -x" that
may run before the authenticated GitHub token is printed; to avoid leaking
GITHUB_TOKEN, ensure tracing is disabled around the token request: before the
curl that builds GITHUB_TOKEN (which uses JWT and GITHUB_APP_INSTALLATION_ID),
save the current tracing state, run the curl with tracing off (or wrap the
authenticated block with set +x), then restore the original tracing state only
after the last authenticated request; specifically modify the section that
defines GITHUB_TOKEN so tracing is disabled during the python/json token
extraction and only restored afterwards.
- Around line 132-133: The lookup for EXISTING_COMMENT_ID only checks the first
page of comments and can miss the marker on busy PRs; change the logic that uses
curl + python3 to page through issue comments (use the API params per_page and
page, or request sorted by updated with sort=updated) until a comment containing
${COMMENT_MARKER} is found or no more pages remain, then set EXISTING_COMMENT_ID
to that comment's id (refer to the existing variables AUTH, API, PULL_NUMBER,
COMMENT_MARKER and the EXISTING_COMMENT_ID assignment). Implement a loop that
increments the page number, breaks when the marker is located, and falls back to
empty string if not found to preserve idempotence.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 94be7e51-e878-4515-b662-b778f43289d0
⛔ Files ignored due to path filters (4)
ci-operator/jobs/openshift-hyperfleet/hyperfleet-adapter/openshift-hyperfleet-hyperfleet-adapter-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift-hyperfleet/hyperfleet-api/openshift-hyperfleet-hyperfleet-api-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift-hyperfleet/hyperfleet-broker/openshift-hyperfleet-hyperfleet-broker-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift-hyperfleet/hyperfleet-sentinel/openshift-hyperfleet-hyperfleet-sentinel-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/openshift-hyperfleet/hyperfleet-adapter/openshift-hyperfleet-hyperfleet-adapter-main.yamlci-operator/config/openshift-hyperfleet/hyperfleet-api/openshift-hyperfleet-hyperfleet-api-main.yamlci-operator/config/openshift-hyperfleet/hyperfleet-broker/openshift-hyperfleet-hyperfleet-broker-main.yamlci-operator/config/openshift-hyperfleet/hyperfleet-sentinel/openshift-hyperfleet-hyperfleet-sentinel-main.yamlci-operator/step-registry/hyperfleet/risk-scorer/OWNERSci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.shci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-ref.metadata.jsonci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-ref.yaml
✅ Files skipped from review due to trivial changes (2)
- ci-operator/step-registry/hyperfleet/risk-scorer/OWNERS
- ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-ref.metadata.json
🚧 Files skipped from review as they are similar to previous changes (4)
- ci-operator/config/openshift-hyperfleet/hyperfleet-adapter/openshift-hyperfleet-hyperfleet-adapter-main.yaml
- ci-operator/config/openshift-hyperfleet/hyperfleet-broker/openshift-hyperfleet-hyperfleet-broker-main.yaml
- ci-operator/config/openshift-hyperfleet/hyperfleet-api/openshift-hyperfleet-hyperfleet-api-main.yaml
- ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-ref.yaml
| - as: risk-scorer | ||
| optional: true | ||
| steps: | ||
| test: | ||
| - ref: hyperfleet-risk-scorer |
There was a problem hiding this comment.
Run make update and commit the generated Prow configs.
This tests entry alone does not register a new presubmit in Prow; I don’t see the corresponding generated ci-operator/jobs/... updates in the supplied diff. Without those generated files, the new risk-scorer job won’t actually be wired up.
As per coding guidelines, ci-operator/config/**/*.yaml: "For CI configuration, edit files in ci-operator/config/<org>/<repo>/ and run make update to generate downstream Prow job configs".
🤖 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
`@ci-operator/config/openshift-hyperfleet/hyperfleet-sentinel/openshift-hyperfleet-hyperfleet-sentinel-main.yaml`
around lines 40 - 44, The new tests entry (as: risk-scorer / ref:
hyperfleet-risk-scorer) was added only to ci-operator config and you must run
the generator to produce the corresponding Prow job; run the repository job-gen
step (make update) to regenerate ci-operator/jobs and prow configs, verify the
generated presubmit for the risk-scorer ref appears under ci-operator/jobs and
jobs/... , and commit those generated files alongside your changed
openshift-hyperfleet-hyperfleet-sentinel-main.yaml so the new presubmit is
actually wired up.
| GITHUB_TOKEN=$(curl -sf -X POST \ | ||
| -H "Authorization: Bearer ${JWT}" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| "https://api.github.com/app/installations/${GITHUB_APP_INSTALLATION_ID}/access_tokens" \ | ||
| | python3 -c "import sys,json; print(json.load(sys.stdin)['token'])") | ||
|
|
||
| $WAS_TRACING && set -x |
There was a problem hiding this comment.
Keep xtrace disabled for the authenticated GitHub calls.
If this step starts with -x, Line 28 turns tracing back on before every later curl that sends Authorization: token ${GITHUB_TOKEN}. That will leak the installation token into CI logs. Keep tracing off until the last authenticated request, or wrap each authenticated block with set +x / restore afterward.
As per coding guidelines, ci-operator/step-registry/**/*-commands.sh: "protect sensitive information (passwords, tokens, API keys, cluster URLs, kubeconfig contents) from leaking into CI logs" and "Disable shell tracing (set +x) temporarily when handling sensitive operations in step registry scripts, then restore it after".
🤖 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
`@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh`
around lines 22 - 28, The script enables xtrace via "$WAS_TRACING && set -x"
that may run before the authenticated GitHub token is printed; to avoid leaking
GITHUB_TOKEN, ensure tracing is disabled around the token request: before the
curl that builds GITHUB_TOKEN (which uses JWT and GITHUB_APP_INSTALLATION_ID),
save the current tracing state, run the curl with tracing off (or wrap the
authenticated block with set +x), then restore the original tracing state only
after the last authenticated request; specifically modify the section that
defines GITHUB_TOKEN so tracing is disabled during the python/json token
extraction and only restored afterwards.
| EXISTING_COMMENT_ID=$(curl -sf "${AUTH[@]}" "${API}/issues/${PULL_NUMBER}/comments?per_page=100" \ | ||
| | python3 -c "import sys,json; comments=[c for c in json.load(sys.stdin) if '${COMMENT_MARKER}' in c.get('body','')]; print(comments[0]['id'] if comments else '')" || true) |
There was a problem hiding this comment.
The existing-comment lookup is not actually idempotent on busy PRs.
This only inspects the first 100 issue comments. If a PR already has more than 100 comments before the scorer posts its marker comment, the next run will miss it and create a duplicate instead of updating in place. Page through comments until the marker is found, or query the most recently updated comments first.
🤖 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
`@ci-operator/step-registry/hyperfleet/risk-scorer/hyperfleet-risk-scorer-commands.sh`
around lines 132 - 133, The lookup for EXISTING_COMMENT_ID only checks the first
page of comments and can miss the marker on busy PRs; change the logic that uses
curl + python3 to page through issue comments (use the API params per_page and
page, or request sorted by updated with sort=updated) until a comment containing
${COMMENT_MARKER} is found or no more pages remain, then set EXISTING_COMMENT_ID
to that comment's id (refer to the existing variables AUTH, API, PULL_NUMBER,
COMMENT_MARKER and the EXISTING_COMMENT_ID assignment). Implement a loop that
increments the page number, breaks when the marker is located, and falls back to
empty string if not found to preserve idempotence.
|
/pj-rehearse auto-ack |
|
@rafabene: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@rafabene: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
hyperfleet-risk-scorerstep-registry ref that computes a deterministic risk score for PRsrisk/low,risk/medium, orrisk/highGitHub label automaticallyhyperfleet-ci-botGitHub App (installed on openshift-hyperfleet org) for API accessoptional: true— never blocks mergeTest plan
hyperfleet-ci-botis synced totest-credentialsnamespace/test risk-scorerOverview
Adds a reusable PR risk-scoring step and wires it into the CI pipelines for HyperFleet’s four component repositories (hyperfleet-adapter, hyperfleet-api, hyperfleet-broker, hyperfleet-sentinel). The job is optional and therefore does not block merges.
What changed (practical impact)
New step-registry component: hyperfleet-risk-scorer
CI pipeline integration
risk-scorertest job to the main CI pipeline for each of these repos: hyperfleet-adapter, hyperfleet-api, hyperfleet-broker, hyperfleet-sentinel.hyperfleet-risk-scorerstep-ref; marked optional: true so it never blocks merges.Ownership & metadata
Implementation artifacts
ci-operator/step-registry/hyperfleet/risk-scorer/
ci-operator/config/openshift-hyperfleet/
risk-scoreroptional test job entries in the pipeline config for hyperfleet-adapter, hyperfleet-api, hyperfleet-broker, and hyperfleet-sentinel.Security / operational notes
Testing checklist (as provided)
hyperfleet-ci-botis synced to CI namespace./test risk-scorer.