fix(slm): manage Redis by the unit the role installs, and fail a restore that cannot stop it by mrveiss · Pull Request #16061 · mrveiss/AutoBot-AI

mrveiss · 2026-09-08T13:55:51Z

Single-issue rationale: this changes a database restore path on a live fleet. It goes alone so the review is about one question — whether a restore can still proceed with the server running — rather than sharing a pass with unrelated work.

Thinking Path

The operator said twice that the deployment is supposed to use Redis Stack. It always was. Measured on the live node:

executable    : /opt/redis-stack/bin/redis-server
MODULE LIST   : ReJSON, timeseries, search

The instance is right. The code managing it was not. systemctl show -p LoadState reports not-found for both redis and redis-server on a provisioned node, and two services hardcode exactly those.

The reconciler searched ["redis-server", "redis"], so it could not find the service it is responsible for — while the services table carried redis-stack-server | running. Two components disagreeing about the name, and only one of them feeds the view.

The restore path is the urgent half. _run_command returns (success, output); _stop_redis_for_restore took neither. So the stop failed against a nonexistent unit, the failure was invisible, and execute_restore replaced the RDB under a running server — which overwrites it on its next save. The symmetric start is equally a no-op, so even the recovery step looked fine.

The failure and the success produce the same observable, which is why nobody saw it.

What Changed

role_registry.systemd_unit_for_role(role) — one source for the unit a role installs. The registry already held the answer; three copies had drifted from it.
Reconciler tries redis-stack-server first, legacy names retained as fallbacks for pre-Stack nodes rather than as equal candidates.
The restore raises when the stop fails.

Verification

4 tests, and both fixes mutation-checked:

mutation	result
reconciler back to `["redis-server", "redis"]`	1 failed
restore back to discarding `_run_command`'s result	1 failed
both restored	4 passed

The sweep assertion is the one that generalises: no systemctl command in the service layer may name a unit absent from a provisioned node. Keeping the legacy names as fallbacks stays legal — a pre-Stack node still has them; naming one in a command does not.

The first draft of that sweep flagged its own comment explaining the bug, because the comment contains the string systemctl stop redis-server. That is #16011's blindness — a line-oriented matcher that cannot tell code from prose — appearing in the guard written to catch a naming defect. Fixed by skipping comment lines, and recorded there.

The registry assertion parses the declaration rather than importing it: role_registry.py imports models.database, and what is being pinned is the declaration every caller resolves against.

Not in this PR

api/services.py:73 still lists redis-server and redis without redis-stack-server — fixed in #16027, open. api/deployments.py:83 and node-roles.ts:95 are operator-facing tool lists; #16060's criteria ask for them to be corrected or recorded as display-only, so a later sweep does not re-derive whether they matter.

Model Used

Opus 5

…ore that cannot stop it (#16060)

coderabbitai · 2026-09-08T13:56:00Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 21a39734-f74c-40d1-b6e2-10d647469683

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-09-08T13:58:06Z

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml.

…ue-16060-redis-unit # Conflicts: # autobot-slm-backend/services/role_registry.py

…ore that cannot stop it (#16061) * fix(slm): manage Redis by the unit the role installs, and fail a restore that cannot stop it (#16060) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * fix(slm): keep the redis-unit fix inside the size ceilings (#16060) --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

) (#15996) * test(hooks): guard that a non-deny permissionDecision exits 0 (#15956) PreToolUse parses a hook's decision JSON from stdout and only on exit 0. Exit 2 is a blocking error whose reason comes from stderr. So a hook printing "permissionDecision":"ask" and then exiting 2 has its payload discarded: the call is denied and the operator sees a refusal with no reason. Two hooks do this. protect-files.sh's ask() helper -- guarding both settings files, its only two ask sites against five deny sites -- and scan-secrets.sh at its single non-zero exit, where the author's intent is on the record in the reason string itself. The guard reads source rather than behaviour, because a runtime test would have to know which decision was intended before it could tell a denial from a discarded ask -- and that knowledge is what the bug destroys. It sweeps every hook rather than the two known ones, since a fix limited to the reported site is indistinguishable from one that checked its neighbours. This test FAILS until the two exit codes are corrected. That is deliberate: the defect is currently invisible and the guard is what makes it loud. Refs #15956 * fix(hooks): a non-deny permissionDecision must exit 0 to be read (#15956) PreToolUse parses a hook's decision JSON from stdout and only on exit 0. Exit 2 is a blocking error whose reason it takes from stderr. Both hooks below printed "permissionDecision":"ask" and then exited 2, so the payload was discarded: the call was denied outright and the operator saw a refusal with no reason, because nothing had been written to stderr. protect-files.sh: ask() now exits 0. Its two ask sites -- against five deny sites -- guard settings.json and settings.local.json, so the only path in that hook capable of reaching a human never did. Every deny() keeps exit 2; all five call sites verified unchanged. scan-secrets.sh: the same defect at its only non-zero exit, which made the whole scanner fail-hard rather than fail-ask. The author's intent is on the record in the reason string -- "Review carefully before allowing" is written for a human being asked a question, and none was ever shown it. It survived because a denial and a discarded ask produce an identical observable, so the hooks looked like they were working. Post-fix sweep over every hook: 6 decision emissions, 0 mismatches. The guard added in f05335a keeps that true for hooks added later. Refs #15956 * fix(hooks): a secret denies, and its reason goes to stderr (#15956, #12513) The previous commit made scan-secrets.sh honour its declared `ask` payload by exiting 0. That converted credential BLOCKS into ASKS for double-quoted and single-quoted literals, AWS access key ids and connection strings, and repo_tests/scan_secrets_hook_test.py caught it: 4 failures, assert 0 == 2. The test is a recorded ruling, not an artefact. Its docstring under #12513 reads "the secret-scan PreToolUse hook must block secrets and allow indirection" and "a secret scanner that fails open, silently, is worse than none: it is trusted". So the payload was the bug in this file, not the exit code -- the mirror image of protect-files.sh, where the exit code was the bug and `ask` was right. Two files, one symptom, opposite fixes; treating the shapes as the same defect is what produced the regression. `deny` + exit 2 alone would have restored the block and left the operator with "No stderr output" -- the very symptom #15956 exists to remove. PreToolUse takes an exit-2 reason from STDERR, so the reason is written there. Both rulings hold at once; they were never in conflict except through the channel. scan_secrets_hook_test.py 11 passed. Guard 7 passed. Pipe test: EXIT=2 with the reason on stderr. Refs #15956, #12513 * fix(hooks): a blocking decision writes its reason to stderr (#15956) The first pass fixed `ask` and left every `deny` discarding its reason. The comment stating the rule sat three lines below the function that broke it, and scan-secrets.sh in the same PR already did it correctly -- one file carried the remedy the other needed. `ask` and `deny` differ in EXIT CODE. They do not differ in which stream carries the explanation: on exit 2 the harness takes the reason from stderr, so a deny that writes only to stdout blocks with no explanation. Five deny sites in protect-files.sh plus its jq branch, which is the worst of them -- no jq means every edit is blocked and that sentence was the only explanation. The structural half is why this blocked the PR. EXPECTED_EXIT measures the exit code; once `ask` was corrected the defect moved into the CHANNEL, so the guard could no longer tell the fixed file from the broken one and pinned the broken half as correct. When a fix moves a defect from one dimension to another, the guard has to move with it. The new channel check found a THIRD hook on its first run: block-dangerous-commands.sh, both deny sites, never reported. Fixed here. Pipe-verified: a .env deny now exits 2 with its reason on stderr; the settings ask still exits 0 with stdout as its channel. Refs #15956 * test(llc): the blocked-by refusal reaches the caller as a 409 (#15931) (#16059) * test(llc): the blocked-by refusal reaches the caller as a 409 (#15931) The rule's whole history is a correct implementation nobody reached. The service-level tests in test_blocked_by_rule_fires_15931.py prove it fires when called the way production calls it; they cannot distinguish "the route enforces this" from "the route swallows the exception and returns 200", which is the failure that would restore the original defect with those tests still green. work_items.py:644 sits inside a try whose handlers translate exceptions, so the refusal reaching a caller is a claim about the handler, not the service. These assertions go through POST /work-items/{id}/transition with a real session and real rows, and assert on the status code and message a client sees. The service is not called directly anywhere in the file. Three cases: the refusal (409, reason intact, item unmoved), a resolved blocker (200), and no relations at all (200). The last two are the contrast - a refusal that fires for every BLOCKED item would pass the first assertion alone. * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * test(llc): the blocker fixture uses a status the enum defines (#15931) WorkItemStatus has no TODO member -- backlog/ready/in_progress/in_review/ done/cancelled/blocked. READY is the right choice: has_unresolved_blockers treats {done, cancelled} as terminal, so a READY blocker is genuinely unresolved and the refusal fires for the reason the test claims. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(llc): a status transition's comment reaches the store, not the floor (#16054) * fix(llc): a status transition's comment reaches the store, not the floor (#16017) * test(llc): assert the comment reaches the database, not that the handler mentions it (#16017) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * style(llc): condense the #16017 rationale to keep agent_api.py under its ceiling * test(llc): pin the ordering against the FIRST commit, and record the docstring hazard (#16017) * test(llc): a failed commit must take the comment with it (#16017 review) --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(slm): a healthy oneshot and a templated unit must not report unknown (#16027) * fix(slm): a healthy oneshot and a templated unit must not report unknown (#16019) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * fix(slm): mirror the agent status fix into the slm_agent role files (#16019) * fix(hooks): pre-push must not collect trees pytest.ini ignores (#16019) * refactor(slm): extract the service port map so services.py stays under its ceiling (#16019) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * fix(slm): a completed oneshot is counted as healthy, not dropped from the aggregates (#16019) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * refactor(slm): extract ServiceStatus so database.py stays under its ceiling (#16019) * fix(slm): register models.service_status with the conftest stub (#16019) * fix(slm): move ServiceStatus out of the models package so the migration runner can import it (#16019) * test(slm): assert the aggregate sums rather than overwrites, and share one bucketing helper (#16019 review) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * chore(ratchet): lower monitoring.py's ceiling to its post-extraction size (#16019) --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(slm): manage Redis by the unit the role installs, and fail a restore that cannot stop it (#16061) * fix(slm): manage Redis by the unit the role installs, and fail a restore that cannot stop it (#16060) * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * fix(slm): keep the redis-unit fix inside the size ceilings (#16060) --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * test(guards): record the hook-decision guard's *.sh sweep (#15956) repo_tests/hook_decision_exit_codes_15956_test.py sweeps `.claude/hooks/*.sh`, so it declares the root-relative `*.sh` glob and belongs in the record that tracks which guards depend on trees the python filter does not reach. Without the entry, glob_declared_reads_15900_test.py reports it as `declaring-but-not-recorded` -- correctly: a guard reading a tree nothing records is exactly what that record exists to surface. This is an ADDITION of a dependency this branch introduced, not a deletion. The `*.min.js` entries this branch was also failing on were stale base: #16012 removed them and the merge in this branch's parent commit took that in. Deleting them here would have re-litigated a landed cleanup, which is why the two failures needed opposite actions despite sharing a test file and a shard. Refs #15956 * test(hooks): read the decision's value, not just its exit and channel (#15956, #16079) The guard measured two dimensions of a hook decision and a third was unguarded: the value itself. DECISION_RE enumerated ask|allow|deny -- the valid values -- so an invalid one was invisible. A detector keyed on the set of correct answers cannot report a wrong answer; it reports absence. Found block-dangerous-commands.sh:347 emitting permissionDecision "warn", which is not in the contract and so does nothing, on stderr on an exit-0 path where stderr is not surfaced. Inert twice over, well-formed, beside three working siblings. Filed as #16079 and pinned as a ratchet entry rather than fixed: the remedy is a behaviour decision about what that advisory should do, and a guard that picked one would be answering a question nobody ruled on. The ratchet asserts the entry is still present and still invalid, so it fails when #16079 lands. Also refuses a pairing when the first exit is indented deeper than its decision. Taking the first textual exit lets a conditional exit 0 be credited to a decision that falls through to a later exit 2 -- proximity standing in for structure, in a guard whose whole subject is which exit follows which decision. Indentation rather than collecting every exit in the window, which false-positives on scan-secrets.sh:150, whose window holds both the deny's exit 2 and the script's terminal exit 0. Merges _decisions_with_exits and _emissions_with_context into one _scan: they computed the same window in two places with nothing forcing them to agree. Adds six detector fixtures -- matches a real emission, ignores prose, sees an out-of-contract value, refuses the fall-through shape, still pairs the same-indent shape the hooks use, closes the stderr window at the paired exit. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

fix(slm): manage Redis by the unit the role installs, and fail a rest…

1d6da53

…ore that cannot stop it (#16060)

github-actions Bot and others added 4 commits September 8, 2026 13:58

Auto-fix: code formatting

68d52a4

Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml.

fix(slm): keep the redis-unit fix inside the size ceilings (#16060)

65ab578

Merge remote-tracking branch 'origin/issue-16060-redis-unit' into iss…

db094e9

…ue-16060-redis-unit # Conflicts: # autobot-slm-backend/services/role_registry.py

Merge branch 'Dev_new_gui' into issue-16060-redis-unit

923079b

mrveiss mentioned this pull request Sep 8, 2026

Per-node service status: templated units are dropped and 'active (exited)' maps to unknown #16019

Closed

5 tasks

mrveiss merged commit 2845e68 into Dev_new_gui Sep 8, 2026
78 checks passed

mrveiss deleted the issue-16060-redis-unit branch September 8, 2026 17:31

mrveiss mentioned this pull request Sep 8, 2026

Redis is managed by two unit names that do not exist, and a failed restore-stop is silent #16060

Closed

2 of 5 tasks

Navigation Menu

Repository navigation

fix(slm): manage Redis by the unit the role installs, and fail a restore that cannot stop it - #16061

fix(slm): manage Redis by the unit the role installs, and fail a restore that cannot stop it#16061
mrveiss merged 5 commits into
Dev_new_guifrom
issue-16060-redis-unit

mrveiss commented Sep 8, 2026

Uh oh!

coderabbitai Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mrveiss commented Sep 8, 2026

Thinking Path

What Changed

Verification

Not in this PR

Model Used

Uh oh!

coderabbitai Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 8, 2026

✅ SSOT Configuration Compliance: Passing

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Sep 8, 2026 •

edited

Loading