Repository navigation
refactor(tooling): base-ref resolution is duplicated across four CI guards while scripts/lib exists #13984
Description
Discovered while delivering #13876, #13879 and #13880. Partly self-inflicted — two of the four copies were added by that work.
The duplication
Every changed-files guard now resolves a base ref and status-checks its git calls, independently:
| File | base-resolution call sites |
|---|---|
pipeline-scripts/check-pre-commit-hook-pr.sh |
5 |
.github/workflows/code-quality.yml (inline) |
4 |
scripts/lint-conventions.sh |
1 |
scripts/verify-done.sh |
1 |
All four encode the same hard-won rules: on a pull_request the checkout is the merge commit so HEAD^1..HEAD is the PR's own changes; origin/<base> is the base's current tip and drags in other PRs' merges; an unresolvable ref must be fatal rather than read as "no changes". Those rules were learned once and written four times, so the next correction has to be applied four times or the copies drift.
scripts/lib/ already exists as the canonical home for shared shell helpers (branch-guards.sh, project_root.sh), each with a colocated *_test.sh. Neither new script sources it. That is a "consolidate, never fork" violation.
Second, related finding: function length
scripts/verify-done.sh's branch_state() is 113 lines against the repo's ≤30-line rule. It accumulated one guard per review round (seven rounds, thirteen defects) and each is individually justified, but the result is a single function holding six distinct decisions. Extracting them is the same refactor as the above — the guards become named, testable helpers.
Change
scripts/lib/git-scope.sh— one status-checked base/head resolver encoding the merge-commit rule, with a colocated test- All four call sites source it; the inline workflow copy becomes a call
branch_state()decomposed into named predicates (has_merge_unique_content,substantive_commits,content_in_base, …), each ≤30 lines and independently testable
Implementation order
Wave 1 — extract scripts/lib/git-scope.sh + tests from the existing four copies (no blockers)
Wave 2 — repoint check-pre-commit-hook-pr.sh and code-quality.yml (depends on wave 1)
Wave 3 — repoint lint-conventions.sh, decompose branch_state() (depends on wave 1)
Acceptance criteria
- One resolver in
scripts/lib/, sourced by all four call sites - Colocated
*_test.shor*_test.pycovering the merge-commit, shallow-clone and unresolvable-ref cases - No function in
scripts/verify-done.shexceeds 30 lines - Existing suites (
verify_done_test.py,lint_conventions_test.py,check-pre-commit-hook-pr_test.py) still pass unchanged
Activity
mrveiss commentedon Aug 11, 2026
Discovered during the #13874/#13876/#13879/#13880 session. Cross-linked from those issues' closure comments.
github-actions commentedon Aug 11, 2026
👋 Auto-triage could not confidently classify this issue (no confident signal). Could a maintainer add the appropriate labels (
frontend,backend,infrastructure,docs,testingandgood-first-issue,intermediate,advanced)?mrveiss commentedon Aug 23, 2026
Delivered in #14883 as part of the CI-guard cluster (#14371, #13984, #12986, #14034). #14181 was measured and deliberately excluded — see the comment there for where it actually stands. Discovered and filed rather than dropped: #14884, #14885.
refactor(tooling): one base-ref resolver in scripts/lib, sourced by e…
refactor(ci): consolidate duplicated, drifted and never-run CI guards (…
mrveiss commentedon Aug 23, 2026
Delivered and merged.
ac754f5e0a9d3c9805, including all 12python-suiteshards. Up to date with base at merge.code-reviewerpass. It reconstructed all three pre-merge detectors from the rebase target and diffed them byte-for-byte rather than trusting the PR's union table.Verified in base after merge:
Union preserved — rules a survivor-pick would have deleted:
(19|2[0-5])did not winPORT_PATTERN— declared and never grep'dUser=,chown, sudoers)offset=0as WARNING not VIOLATIONscripts/lib/hardcoded_value_rules_test.pyfails by name per former detector, so a substitution cannot pass as a merge.One HIGH finding was raised in review and fixed before merge, not deferred. The baseline suppressing 1977 findings had no anti-growth check: a PR could hardcode a value and append a matching baseline line in the same commit, and
ssot-coveragewould reportssot_violations=0.check_baseline_no_growth.shnow diffs the baseline against the base ref and fails on any new key or increased count, failing closed on an unresolvable base, an unreadable baseline, or a parse error. Mutation-proved in a scratch repo both directions — growth red, shrinkage green.A real credential exposure was found and fixed while doing it. Two
postgresql://user:pass@host/dbvalues were already sitting in the generated baseline, a tracked file that also reaches public CI output. One shared_hv_redact_userinfonow serves both the DSN and URL rules; scheme and host survive so findings stay actionable. Verified in base: zerouser:pass@patterns remain, and the baseline totals are unchanged at 1434 lines / 1977 — the redaction dropped no finding.Also fixed:
api-wiring— a required check — omittedautobot_shared/**/*.pyfrom its path filter whileaudit_api_wiring.pyimports two modules from that tree, so it reported green over an audit that never ran (#12986).#14181 was deliberately not closed by this PR — measured at 8/10 on one criterion with the remainder blocked by epic #7442. Follow-ups filed: #14884, #14885, #14891.