Repository navigation
bug(ci): 8 of 12 hardcoded-value rule classes never block, and 9 unbaselined violations sit on the base right now #14914
Description
Found while building #14912 on 192a2d12c. The merged base carries 9 unbaselined hardcoded-value violations right now, and SSOT Configuration Compliance reports "status": "pass" over them.
Evidence
On a clean checkout of origin/Dev_new_gui (192a2d12c), no local modifications:
{ "status": "pass", "total_violations": 9, "ssot_violations": 0,
"other_violations": 9, "warnings": 0, "baselined": 1976 }The nine:
autobot-slm-backend/ansible/playbooks/remove-role.yml:194 "/opt/autobot/{{ role_target_dir }}
autobot-slm-backend/ansible/playbooks/remove-role.yml:206 '/opt/autobot/
autobot-slm-backend/ansible/playbooks/cleanup-nodes.yml:101 "/opt/autobot/{{ _cleanup_target.dir }}
… 128, 178, 205, 232, 262, 289 (same file, same shape)
All nine arrived with #14890. The detector found every one of them, printed every one of them, and the gate passed.
Why they do not block
Two independent places, both keyed on ssot_violations alone:
# pipeline-scripts/detect-hardcoded-values.sh
STATUS="pass"
[ "$SSOT_VIOLATIONS" -gt 0 ] && STATUS="fail"
# .github/workflows/ssot-coverage.yml — "Set job status"
if [ "$SSOT_VIOLATIONS" -gt "0" ]; then … exit 1; fiother_violations is computed, reported in the JSON, printed to the summary — and read by nothing that can fail.
Counted at the emit sites in scripts/lib/hardcoded-value-rules.sh:
| class | emit sites | rules | blocks? |
|---|---|---|---|
ssot |
4 | VM IPs, infra ports, model names | yes |
other |
8 | AutoBot paths, DB DSNs, URLs, account identities, roles, categories, timeouts, magic numbers | no |
Two thirds of the merged rule set is detected but unenforced.
Why this is worth filing now rather than shrugging at
The ssot/other split and the STATUS rule both predate #14883 — I preserved them deliberately to keep the JSON contract that ssot-coverage.yml consumes. But #14883 is what made the eight other rules actually run against the tree: before it, they lived in a pre-commit hook that only ever saw staged files, or in a fork nothing invoked. So the consolidation substantially increased the number of findings in the one class nothing acts on. The surface grew; the sink did not.
Compounding it, and tracked separately in #14912: SSOT Configuration Compliance is not one of the ten required contexts, so even the ssot half is advisory at the branch-protection layer.
Options, in preference order
- Make
otherblock, with the current 9 baselined first. Cheapest and consistent with how the rest of the gate works: add them topipeline-scripts/hardcoded_values_baseline.txt(or fix them — they are/opt/autobotliterals in ansible playbooks, which is exactly whatAUTOBOT_BASE_DIRexists for), then fail ontotal_violationsinstead ofssot_violations. - Promote per class. Some
otherrules are noisier than others; magic numbers and timeouts have the widest false-positive surface, paths and DSNs the narrowest. A per-class blocking policy would let the sharp ones bite now. - Leave advisory, but say so. If that is the deliberate answer, the JSON field and the workflow summary should say "advisory" so nobody reads a green
SSOT Configuration Complianceas "no hardcoded values".
Option 1 is my recommendation; the nine current findings are a genuine backlog item, not noise.
Acceptance criteria
- A decision recorded on whether
other-class findings block - If they do: the 9 current findings fixed or baselined, and the gate keyed on the chosen number
- Whatever is chosen, a green
SSOT Configuration Compliancemeans something a reader can state precisely - Mutation-proved: introduce one
other-class violation and confirm the gate does what the decision says
Related
- refactor(ci): consolidate duplicated, drifted and never-run CI guards (#14371, #13984, #12986, #14034) #14883 — the consolidation that made these rules run
- bug(ci): the blocking stale-baseline audit reds ssot-coverage on any PR touching 8% of the repo #14912 — prune ergonomics, and the required-context gap
- fix(provision): route every component removal through one data-guarded task, so an unknown fact cannot delete a database (#14856) #14890 — where the nine current findings came from
Activity
mrveiss commentedon Aug 24, 2026
Decision, for AC 1:
other-class findings block. All twelve emit sites do.The reasoning, in one line: the advisory axis already existed and it was never the class — it is the severity.
_hv_rule_magic_numberemitsWARNINGforoffset=0, warnings are counted separately and excluded fromtotal_violations, and that carve-out predates all of this. So a rule that should not block already says so in the rule.ssotvsotherdescribes what the fix looks like (a named config key vs a named family), not whether the value belongs in the source.Option 1 from this issue, with one change: the nine were fixed, not baselined.
check_baseline_no_growth.shhas no sanctioned route for a baseline addition, so baselining them meant weakening the guard. All nine are quoted/opt/autobotliterals in two ansible playbooks andinventory/group_vars/all.ymlalready definesautobot.base_diras the SSOT for that tree, so the fix was a substitution. Two further already-baselined occurrences in the same files were fixed alongside them, and--prune-baselineremoved their now-stranded entries — 1410 keys to 1408, shrink-only, guard untouched.That missing addition route is filed separately as #14919.
Also fixed while in there, same defect class: the workflow's count parse ends in
|| echo "0", so a detector that died (a missing scan directory is FATAL as of #14912) produced no JSON, parsed as zero, and passed. The workflow now enforces the detector'sstatusfield — one verdict, computed once — plus theexit_codeit was already writing to$GITHUB_OUTPUTand reading nowhere.AC 4, mutation-proved in both directions, on a copy of the tree outside the repo:
and the sink, with the
Set job statusstep extracted from the workflow and executed on those outputs: exit 1 with the mutation present, exit 0 with it removed.PR #14918.
fix(ci): resolve the install-root SSOT in the cleanup guard (#14914)
test(ci): pin that both hardcoded-value entry points block alike (#14914
docs(provision): drop a stale note from the cleanup summary (#14914)
fix(provision): make the component-dir sweep follow the install-root …
fix(ci): make every hardcoded-value class block, not one of two (#14914…
mrveiss commentedon Aug 24, 2026
Delivered and merged as
ecf4ddf1e(PR #14918).CANCELLEDis never counted green;started_atandcompleted_atare never sorted together).code-reviewerpasses — a full one, then a focused re-review of the four commits that landed afterwards, because those touched the CRITICAL(provision): re-running the wizard can delete live databases — an undefined role fact is the delete branch #14856 data-loss guard.Verified in base:
with the old
[ "$SSOT_VIOLATIONS" -gt 0 ]kept above it as a comment recording what it missed.All 12 emit classes now block. The advisory axis is severity, not class —
WARNING(onlyoffset=0) stays non-blocking and is excluded fromtotal_violations, so a rule that should not block declares itself in the rule. Corroborated in review by something not in the original argument: the pre-commit hook already blocked on anyVIOLATIONof either class, so this closes a gap where the CI tree-scan was more permissive than the commit-time hook.The 9 findings were fixed, not baselined.
check_baseline_no_growth.shhas no sanctioned addition route — and its own failure text claims one ("a decision for a reviewer to take deliberately") that does not exist. Baselining would have meant weakening the guard, so the literals were replaced withautobot.base_dirfrom the inventory SSOT. Filed as #14919 rather than worked around. The baseline only shrank: 1410 → 1408 keys, 2 removed, 0 added, 0 reordered, verified line-by-line in review.A second fail-open was closed on the way. The workflow parsed every count with
|| echo "0"andSTATUSwith|| echo "unknown", so a detector that died — newly possible since a missing scan dir became FATAL in #14915 — produced no JSON, parsed as zero, and passed. The workflow no longer re-derives a verdict; it enforces the detector'sstatusplus theexit_codethat was already being written to$GITHUB_OUTPUTand read by nothing.Three regressions this change introduced, all found and fixed before merge:
jinja2.exceptions.UndefinedError: 'autobot' is undefined. That is itself the fail-closed proof: an undefinedautobotraises rather than silently resolving to/._delegated_targetsrenderTPL/TPL, sotest_the_protected_config_dir_is_never_scheduled_for_removalkept passing with nothing left to match. Green and blind. Only the loud half made it visible at all._is_component_dirmatched onstartswith("/opt/"), so the tree-wide sweep would have gone quiet the moment anyone wrote{{ autobot.base_dir }}/foo, state: absent— a trap this PR's own header notes would have armed for the next author.All three resolved by having the guard resolve
autobot.base_dirfrominventory/group_vars/all.yml, never restating it. The classifier recognises the SSOT root and a fixed historical set (/opt/autobot,/opt), because legacy flat roots are a fact about already-deployed hosts, not a setting — collapsing the two would lose coverage exactly when a tree is mid-migration.Mutation evidence, both directions, against the pre-fix classifier:
{{ autobot.base_dir }}/autobot-backend/srv/autobot, unguarded removal there/opt/slm-agentMoving the root alone and observing green would have been false comfort — that is also what going blind looks like. The row that distinguishes them moves the root and plants a violation at the new location.
Caveats recorded, not hidden: no host evidence for the ansible change (static guards and CI only);
candidates_seen > 0is a backstop rather than the primary guarantee, the honest tree number being 3 of 68, with the real guarantee being an SSOT-derived classifier self-test that fails by name in both directions. Review also noted one test function crossed the decomposition threshold and one dict literal whose two keys collapse to the same string today — neither blocking.