fix: propagate service start and stop failures

[Ryota-Di]

What & why

kubeintellect service start and kubeintellect service stop now propagate a non-zero exit code when the underlying systemctl command fails.

Previously, subprocess.run(...) was called without inspecting its result, so systemctl could fail while the kubeintellect CLI still exited successfully with code 0.

This PR:

• checks the systemctl return code for service start and service stop
• propagates the underlying non-zero exit code
• adds regression coverage for both commands
• removes the repaired start / stop calls from _REVIEWED
• updates the remaining _REVIEWED reasoning for status and logs to explicitly address exit-code semantics
• updates the documented server test count after adding the two parametrized regression cases

Why status and logs are unchanged

I left service status and service logs unchanged intentionally.

For status, a non-zero systemctl status code can describe a valid queried state such as an inactive service, so it should not be blindly treated as failure of the status query itself.

For logs, journalctl -f is an interactive follow command whose normal lifecycle includes user interruption, so its eventual subprocess return code does not directly represent whether log viewing was successfully started.

Their _REVIEWED entries now document these exit-code considerations rather than only noting that stdout/stderr are visible to the user.

Testing

The regression test was first run against the unmodified implementation and failed as expected for both start and stop:

FAILED ...[start] - Failed: DID NOT RAISE SystemExit
FAILED ...[stop] - Failed: DID NOT RAISE SystemExit
2 failed

After the fix:

17 passed

Additional validation:

test_doc_claims_match_code: 1 passed
ruff: All checks passed!
mypy: Success: no issues found in 193 source files

I also ran the full server test suite locally. The remaining failures were unrelated to this change and specific to the local macOS environment: the demo GIF tests require a supported monospace font such as DejaVu Sans Mono, and three file-mode tests hit BSD chmod behavior.

Closes #189.

Type of change

• 🐛 Bug fix
• ✨ New feature
• 📖 Docs
• 🧹 Refactor / chore
• ⚡ Performance
• 🧪 Tests

Scope

• Version directory touched: v4/ (plus root-level test-count docs)
• This change is scoped to a version whose contributions are open (v4/, or docs/typos in older versions).

Checklist

• New behavior has both a happy-path and an error-path test
• Adding a playbook? triggers: and detect: are two independent features — a test proves
  the compiled detect: predicate fires on a realistic event and does not fire on a
  neighbouring one. The counts and the schema check prove neither. (kind: is the observation
  channel — Pod / Event / Node — never the Kubernetes object; use involved_kind: to
  narrow the subject.)
• Every mutating/write operation keeps its dry-run + diff + human-approval (HITL) gate (safety invariant)
• Secret values are never logged or returned (key names only)
• uv run pytest passes locally
• uv run ruff check . passes locally
• uv run mypy src passes locally
• Docs updated if behavior/CLI/flags changed

Notes for reviewers

The full server suite was run locally on macOS. The failures described above appear environment-specific; the tests directly covering this change, doc-claim validation, lint, and type checking pass locally.

[github-actions]

🎉 Thank you for your first pull request to KubeIntellect.

Please read this one thing — it will look like something is broken, and it isn't:

On a fork PR from a first-time contributor, GitHub runs no CI at all until a
maintainer clicks "Approve and run workflows". Your PR will sit with no checks
and a blocked merge box. That is expected and it is not your fault. You don't
need to do anything — a maintainer will approve the run.

While you wait, the things most likely to come up in review:

• Did you run the gates? make setup from the repo root runs six of the nine
  checks main requires (ruff, mypy, both test suites, file modes, syntax
  warnings) and needs no cluster and no API key. The remaining three — the
  install smoke test and both suites on Python 3.13 — run only in CI, so a
  green local run does not guarantee a green PR.
• Does your change include a test that fails before it and passes after?
• If your change touches a command that emits a diagnosis, report, or summary:
  please run it against a deliberately absent resource (a made-up id, no
  cluster) and confirm it refuses rather than returning a plausible-looking result.
  This is the one failure mode this project cannot ship, and CI cannot catch it.
• Substantial AI assistance is welcome — please just
  disclose it
  in the description so reviewers know where to look.

Contributors are credited in CHANGELOG.md and in the release notes.

[MSKazemi]

@Ryota-Di — first PR to this project, and it is green on all nine required checks. Welcome properly. 🎉

First, an apology for the delay that was not yours: your CI had never run. A first-time contributor's fork PR sits at conclusion: action_required until a maintainer clicks approve, and completed in the API makes it look like it ran and failed. That is the trap @biggdawg320 documented in TRIAGE.md last week, and I still managed to leave you waiting on it. Approved now, and everything passes.

I re-ran your red-green rather than taking your word for it

Your claim was that the test fails before the fix and passes after. Verified independently, by putting main's unpatched cli.py back underneath your test:

$ pytest tests/test_no_unchecked_subprocess_success.py -k propagate_systemctl_failure
E       Failed: DID NOT RAISE SystemExit
FAILED ...test_service_start_and_stop_propagate_systemctl_failure[start]
FAILED ...test_service_start_and_stop_propagate_systemctl_failure[stop]
2 failed

$ # with your fix restored
17 passed in 3.28s

Byte-for-byte what you reported. That matters more than it sounds: an accurate self-report is the thing that makes the next PR cheap to review.

The part that shows you read the code rather than the issue

Anyone can add if proc.returncode != 0. Two things here are better than that.

You propagated the actual exit code — sys.exit(proc.returncode), not sys.exit(1). A caller can still tell a unit-not-found from a permission denial.

And you noticed the allowlist would rot. _REVIEWED had:

("cli.py", "systemctl --user stop"):   "as `service start`",
("cli.py", "systemctl --user status"): "as `service start`",

Deleting the start entry — which your fix requires — would have left status and logs pointing at a reason that no longer existed. A reason nobody can read is the same as no reason, and that allowlist only works because every entry carries one. You rewrote both with real exit-code semantics instead. I checked; nothing dangling is left behind.

Your call to leave status and logs alone is correct, and it is the interesting half of this issue. systemctl status returns non-zero for an inactive service — a perfectly valid answer to a query. Propagating that would have made kubeintellect service status fail whenever it successfully told you the service was stopped. And journalctl -f exits non-zero on the user's own Ctrl-C. Treating either as failure would have been a new bug wearing the old bug's clothes. Two of the four sites were deliberately allowlisted and you worked out which two.

You also updated the documented server counts (5562 → 5564) in both AGENTS.md and CONTRIBUTING.md — that number is gated, so a PR adding tests goes red without it. Most first PRs here trip on exactly that.

Your row on the wall

You were added to the contributor table two days ago marked 🛠️, when you claimed the issue rather than when you delivered. Per the rule announced then, the row gets rewritten to describe this fix once it lands — from "stepped forward" to what you actually did.

---

Merge decision is with the maintainer and I expect it to be yes. If you want another, #154 is close in spirit — count-drift gates that run per-PR and cannot see cross-PR drift, which is the gate you just satisfied, viewed from the other side. good first issue has the rest. No obligation at all.

And entirely optionally: a ⭐ or a note anywhere about "a CLI that exited 0 when the service it started had refused to start" helps a one-maintainer project more than it looks — the repo currently has more forks than stars. It changes nothing about how your work is treated either way. Thank you for a genuinely well-made first contribution. 💙

[MSKazemi]

Merged — thank you, this was a real bug and a clean fix.

Your change is on main here:

kubeintellect/v4/packages/kubeintellect-server/app/cli.py

Lines 782 to 789 in a11425d

	elif action == "start":
	proc = subprocess.run(["systemctl", "--user", "start", _SERVICE_NAME])
	if proc.returncode != 0:
	sys.exit(proc.returncode)
	elif action == "stop":
	proc = subprocess.run(["systemctl", "--user", "stop", _SERVICE_NAME])
	if proc.returncode != 0:
	sys.exit(proc.returncode)

What it fixes matters more than its size. service start exiting 0 after systemd refused is the failure mode this project treats as the worst one: a confident success for work that did not happen. Anything that trusted that exit code — a deploy script, a CI step, an operator reading a terminal — was told the server was up when it was not.

The part I want to call out is the part you didn't change. Leaving service status and service logs discarding their return codes is the correct call, and it is the easy thing to get wrong in this patch: systemctl status returns non-zero for a perfectly valid inactive unit, and journalctl -f returns non-zero when the operator presses Ctrl-C. Treating either as a failure would have swapped one wrong answer for another. Rewriting the surviving _REVIEWED reasons in exit-code terms instead of pointing at the entries you deleted was also right — the dead-entry guard would have caught the alternative.

Verified before merging, on the merged tree: ruff clean, mypy clean across 193 files, server suite 5560 passed, kq suite 749 passed, and the file-mode, syntax, encoding, roster and doc-claims gates all green. I confirmed your test genuinely fails on the unpatched code first (DID NOT RAISE SystemExit, both parameters) rather than taking it on faith.

Two things I added on top, so you know exactly what changed under your name:

1. A success-path test. Yours pins the code that gets propagated; nothing held the other side, so sys.exit(proc.returncode or 1) — the shape a later cleanup reaches for — would turn every successful service start into exit 1 with the entire suite still green. I checked that by injecting exactly that line; only the new case fails.
2. systemctl --user start and stop in _MUST_STAY_CHECKED. Deleting the allowlist entries stops the discarded shape returning, but nothing stopped someone writing the entry back, which is the repair a future contributor would reach for first.

I also merged main into your branch to resolve the test-count conflict and recollected the number (5578 -> 5584). Your commit is untouched in the history.

Your all-contributors entry is updated from the in-flight placeholder to bug/code/test, and the changelog entry for the next release credits you by name.

If you're looking for another one, the same class of bug is worth hunting elsewhere in cli.py — service logs still exits 0 when the unit does not exist and journalctl -f returns immediately having streamed nothing. That is a separate finding from yours and I'd be glad to see it.

Entirely optional, and the merge and the credit were the point either way: if the project is useful to you, a star or a mention helps other people find it.