Conversation
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/restarting-the-module-that-serves-you.md">
<violation number="1" location="docs/restarting-the-module-that-serves-you.md:47">
P3: The doc instructs readers to verify state with `ck module health`, but no such command exists — health is its own domain (`ck health`) and `ck module` has no health verb (verbs are list, status, restart, stop, start, rescan). A user following this would get a parse error at exactly the step meant to tell them what actually happened.</violation>
</file>
You're on the cubic free plan with 17 free PR reviews remaining this month. Upgrade for unlimited reviews.
Re-trigger cubic
| migration, or an `rm` because the transport lost the answer is how one | ||
| completed operation becomes two. | ||
| - **Verify state, don't re-run.** `ck module health` and `ck module status <id>` | ||
| go over a fresh connection and will tell you what actually happened. |
There was a problem hiding this comment.
P3: The doc instructs readers to verify state with ck module health, but no such command exists — health is its own domain (ck health) and ck module has no health verb (verbs are list, status, restart, stop, start, rescan). A user following this would get a parse error at exactly the step meant to tell them what actually happened.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/restarting-the-module-that-serves-you.md, line 47:
<comment>The doc instructs readers to verify state with `ck module health`, but no such command exists — health is its own domain (`ck health`) and `ck module` has no health verb (verbs are list, status, restart, stop, start, rescan). A user following this would get a parse error at exactly the step meant to tell them what actually happened.</comment>
<file context>
@@ -0,0 +1,73 @@
+ migration, or an `rm` because the transport lost the answer is how one
+ completed operation becomes two.
+- **Verify state, don't re-run.** `ck module health` and `ck module status <id>`
+ go over a fresh connection and will tell you what actually happened.
+- A wrapper script that treats a nonzero exit here as "the restart failed" will
+ classify a successful restart as a failure. If you automate around
</file context>
| go over a fresh connection and will tell you what actually happened. | |
| **Verify state, don't re-run.** `ck health` and `ck module status <id>` |
There was a problem hiding this comment.
Correct, and thank you — ck module health doesn't exist. ck health <id> and ck module status <id> are both real and I pattern-matched them into a hybrid, in the sentence telling readers to verify rather than re-run. A doc whose whole argument is "the outcome is unknown, go check" failing at the check step is the worst place for it.
Fixed in 99793df, and I re-ran every remaining ck invocation in the file against the live daemon rather than eyeballing them.
This file shouldn't have been in this PR at all — I branched feat/restart-count-on-status off the docs branch instead of master, so the doc rode along, and the copy here was the pre-fix version. Rebased onto master; #8 is now restart_count only, and the doc lives solely in #9 with the correction.
…able The supervisor has always counted respawns; the count was dropped at the wire boundary, so a module that crashed once and one crash-looping four times a second presented identically to an operator - same state, same last_exit code. That is the distinction deciding whether to read logs or read config, and it was only recoverable from the journal. Zero is skipped on the wire, so older consumers keep parsing and the golden fixture is unchanged - which is what makes the compatibility claim checkable rather than asserted. The CLI renders absent as 0 deliberately: an older daemon and a module that never restarted both mean 'no restarts to report', and a distinct unknown marker would invite reading a quiet module as a diagnostic gap. Verified against the running daemon, which predates the field: the new CLI shows restarts 0 rather than erroring on the missing key.
8f31ac9 to
f78ffaf
Compare
|
Superseded by Mine was
That's exactly right, and my shape gets it wrong. Under skip-zero, absent means "no restarts" — a daemon too old to have the field is indistinguishable from a module that has never crashed. I made a stale daemon report perfect health. It's the same absent-vs-empty collapse I filed #12 about and argued through three rounds of review on #10, reproduced in my own patch, in the one field whose entire purpose is to report a crash loop.
Nothing here needs rescuing. The doc from this branch is already separate in #9. |
Closes the remaining half of #3's ask 2. Replaces the inline patch in #3 (comment) — forking is enabled now, so this arrives as a PR instead.
The supervisor has always counted respawns. The count was dropped at the
ModuleStatus→SupervisorEntryboundary, so a module that crashed once and one crash-looping four times a second presented identically: samestate, samelast_exit_code. That's the distinction deciding whether to read logs or read config, and it was only recoverable from the journal.Compatibility is checked, not asserted
restart_countis skipped on the wire when zero, so the committed golden fixture is byte-identical andcontrol_wire_shapes_match_golden_json_and_round_trippasses unchanged. That test passing is the compatibility evidence rather than a design intention.There's also a real end-to-end check available here, because the daemon running on this box predates the field: a new
ckagainst it rendersrestarts 0instead of erroring on the missing key. An actual old receiver, not a mock of one.Gates
One decision worth reviewing rather than merging past
The CLI renders absent as
0, collapsing "old daemon, field not sent" with "module has never restarted". Both mean no restarts to report, and a?would invite reading a quiet module as a diagnostic gap.But that's the same absent-vs-empty question raised in #7's second design point, and I resolved it the opposite way there — a stderr tail must distinguish captured-empty from not-captured, because those send an operator in opposite directions. The two aren't inconsistent by accident: collapsing is right when the states are equivalent and wrong when they aren't. If you'd rather they match, this is the one to change.
What this doesn't close
#3's acceptance test is a crash loop diagnosable from
ckalone. This says it's looping; it still won't say why. The claustrum case hadexit_code: 1and the cause in stderr — that half is #7.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Expose the supervisor’s restart_count and display it in
ck module statusso crash loops are visible at a glance. Keeps wire compatibility by omitting zero; older daemons render “restarts 0” instead of failing.restart_countinSupervisorEntry, propagate in control, and add a “restarts” column to the status table (missing treated as 0 for compatibility).Written for commit 8f31ac9. Summary will update on new commits.