fix(release): harden branch-protection drift signal classification#22205
fix(release): harden branch-protection drift signal classification#22205BrianCLong wants to merge 3 commits intomainfrom
Conversation
BrianCLong
added
the
codex
ChatGPT Codex Connector
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the branch-protection drift detection script by introducing a distinct 'unknown' status for cases where GitHub metadata is inaccessible, preventing false drift positives. It adds a --fail-on-unknown flag to control exit behavior in these scenarios and includes a new regression test suite. The review feedback recommends using trap in the test script to ensure temporary directories are cleaned up even if tests fail.
| test_unknown_when_gh_unavailable() { | ||
| setup | ||
|
|
||
| local out_dir="${TEMP_DIR}/out" | ||
| mkdir -p "${out_dir}" | ||
|
|
||
| # Ensure gh is not discoverable for this test process. | ||
| PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ | ||
| "${RELEASE_SCRIPTS}/check_branch_protection_drift.sh" \ | ||
| --repo BHG/summit \ | ||
| --branch main \ | ||
| --out-dir "${out_dir}" >/dev/null 2>&1 || true | ||
|
|
||
| local report="${out_dir}/branch_protection_drift_report.json" | ||
| local drift_status | ||
| local drift_detected | ||
| drift_status=$(jq -r '.drift_status' "${report}") | ||
| drift_detected=$(jq -r '.drift_detected' "${report}") | ||
|
|
||
| assert_eq "unknown" "${drift_status}" "drift_status is unknown when gh metadata is inaccessible" | ||
| assert_eq "false" "${drift_detected}" "drift_detected is false when status is unknown" | ||
|
|
||
| teardown | ||
| } |
There was a problem hiding this comment.
The current test structure can leak temporary directories if a test fails before teardown is called. This can happen if a command fails and the script exits due to set -e. To make the test more robust, you can use trap to ensure the cleanup logic in teardown is always executed when the function returns. This also allows you to remove the explicit call to teardown at the end of the function.
test_unknown_when_gh_unavailable() {
setup
trap teardown RETURN
local out_dir="${TEMP_DIR}/out"
mkdir -p "${out_dir}"
# Ensure gh is not discoverable for this test process.
PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \
"${RELEASE_SCRIPTS}/check_branch_protection_drift.sh" \
--repo BHG/summit \
--branch main \
--out-dir "${out_dir}" >/dev/null 2>&1 || true
local report="${out_dir}/branch_protection_drift_report.json"
local drift_status
local drift_detected
drift_status=$(jq -r '.drift_status' "${report}")
drift_detected=$(jq -r '.drift_detected' "${report}")
assert_eq "unknown" "${drift_status}" "drift_status is unknown when gh metadata is inaccessible"
assert_eq "false" "${drift_detected}" "drift_detected is false when status is unknown"
}| test_fail_on_unknown_exits_nonzero() { | ||
| setup | ||
|
|
||
| local out_dir="${TEMP_DIR}/out" | ||
| mkdir -p "${out_dir}" | ||
|
|
||
| set +e | ||
| PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ | ||
| "${RELEASE_SCRIPTS}/check_branch_protection_drift.sh" \ | ||
| --repo BHG/summit \ | ||
| --branch main \ | ||
| --fail-on-unknown \ | ||
| --out-dir "${out_dir}" >/dev/null 2>&1 | ||
| local exit_code=$? | ||
| set -e | ||
|
|
||
| assert_eq "1" "${exit_code}" "--fail-on-unknown exits with status 1 when status is unknown" | ||
|
|
||
| teardown | ||
| } |
There was a problem hiding this comment.
To improve robustness and prevent resource leaks, this test should also use trap to guarantee that teardown is called. This ensures the temporary directory is cleaned up even if an error causes the test to exit prematurely.
| test_fail_on_unknown_exits_nonzero() { | |
| setup | |
| local out_dir="${TEMP_DIR}/out" | |
| mkdir -p "${out_dir}" | |
| set +e | |
| PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ | |
| "${RELEASE_SCRIPTS}/check_branch_protection_drift.sh" \ | |
| --repo BHG/summit \ | |
| --branch main \ | |
| --fail-on-unknown \ | |
| --out-dir "${out_dir}" >/dev/null 2>&1 | |
| local exit_code=$? | |
| set -e | |
| assert_eq "1" "${exit_code}" "--fail-on-unknown exits with status 1 when status is unknown" | |
| teardown | |
| } | |
| test_fail_on_unknown_exits_nonzero() { | |
| setup | |
| trap teardown RETURN | |
| local out_dir="${TEMP_DIR}/out" | |
| mkdir -p "${out_dir}" | |
| set +e | |
| PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ | |
| "${RELEASE_SCRIPTS}/check_branch_protection_drift.sh" \ | |
| --repo BHG/summit \ | |
| --branch main \ | |
| --fail-on-unknown \ | |
| --out-dir "${out_dir}" >/dev/null 2>&1 | |
| local exit_code=$? | |
| set -e | |
| assert_eq "1" "${exit_code}" "--fail-on-unknown exits with status 1 when status is unknown" | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c329f1a02
ℹ️ 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".
| PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ | ||
| "${RELEASE_SCRIPTS}/check_branch_protection_drift.sh" \ |
There was a problem hiding this comment.
The regression test claims to make gh undiscoverable, but this PATH still includes standard bins (/usr/local/bin, /usr/bin, etc.) where gh is commonly installed. In environments with GitHub CLI available, the script will take the live API path instead of the intended unknown path, so the assertions become flaky and no longer validate the new unknown-state behavior deterministically.
Useful? React with 👍 / 👎.
|
Queue note: branch-local issues have been reduced to the dependency on |
|
Closing in favor of #22241, which absorbs the drift-classification fix into the clean convergence train. |
1 participant
Motivation
Description
scripts/release/check_branch_protection_drift.shto detect missinggh, add a--fail-on-unknownflag, and emit adrift_statusenum (in_sync|drift_detected|unknown) while preservingdrift_detectedfor real mismatches.unknownverification state and improved API error text whenghor permissions are unavailable.scripts/release/tests/check_branch_protection_drift.test.shto validateunknownbehavior and the--fail-on-unknownexit semantics.docs/roadmap/STATUS.jsonwith a new initiativebranch-protection-drift-signal-hardeningdocumenting the change.Testing
./scripts/release/tests/check_branch_protection_drift.test.sh, which executed 3 tests and all passed (validatingdrift_status=unknownwhenghis inaccessible and--fail-on-unknownexit behavior)../scripts/release/check_branch_protection_drift.sh --repo BHG/summit --branch main --out-dir artifacts/release-train --verbose, which produceddrift_status: unknownand wroteartifacts/release-train/branch_protection_drift_report.jsonas expected in environments withoutgh.jq '{drift_status,drift_detected,api_accessible,api_error}' artifacts/release-train/branch_protection_drift_report.jsonto confirm fields and error messaging.Codex Task