fix(cli): keel open says WHERE it looked, because that was the missing fact - #763
Merged
Conversation
…ing fact Found by an install that worked. Four console daemons bootstrapped, three wrote their records, and `keel open --port 8765` answered "no recorded keel server" -- so a success read as a failure. The command had been run from a source checkout, which is ITSELF a deployment root (`is_deployment_root` is deliberately generous, and the tree has a `keel.db` and a `config.yaml`). So it searched the checkout's `run/` while the servers wrote to `~/keel/run/`. The message offered three explanations and the true one was not among them. It could not be: the message never said where it had looked. Every refusal now names the directory, and the wording says why that directory was chosen. `keel open` takes no `--db` and no `--config`, so nothing in its signature hints that the answer depends on the working directory; the path is the hint, and the `--help` says the same thing in one line. The stale-record branch gets it too. A crashed server and a wrong directory are different problems, and an operator staring at either needs the same fact to tell them apart. Verified by running the command from the wrong tree and reading what it prints: it names the worktree it searched, which is exactly the sentence that would have ended the original confusion in a glance. 6,413 passed / 3 skipped, ruff and mypy clean. Three mutants killed: the path dropped from each refusal branch, and the help line removed. Recorded while here, since #756 rests on it: launchd DOES give `keel serve` a stdout that is not a tty. Three daemons, three records written. That was the one assumption nothing in CI could test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6
… is running Review of this PR's own diff, and the finding is the same class of fault the PR exists to remove: a message that states something false with confidence. Since #759 added the port probe, `live_record` has returned `None` for TWO reasons -- a dead pid, or a live one with nothing answering on the port -- and the refusal asserted the first regardless. Measured against a record whose pid was the running test process: recorded pid: 53702 (this process, alive) Error: ... but its process is gone -- it crashed or was killed. It said that about the very process printing it. A server alive but not yet listening, or bound to a host other than the one recorded, was told it had crashed -- which sends its operator to the wrong investigation entirely, on the surface whose whole job is to stop exactly that. `_refuse` re-checks the pid now and says which failure it is. The live-pid branch names the pid and the address nothing answered on, and points at the log, because that state is a bind failure or a half-started server: a keel server was recorded on port 58030 and its process (27325) is still running, but nothing answers on 127.0.0.1:58030. That is a description of what `com.keel.serve.live` was actually doing on the machine that reported the original bug. `_process_alive` becomes public for the second caller. `_refuse` is typed `NoReturn`, so the unreachable `return` after it is gone and a checker proves what a reader had been assuming. And the not-found message no longer says the path came from "this directory": `state_root` honours `KEEL_HOME` FIRST, so that was false for precisely the reader most likely to have set it. 6,417 passed / 3 skipped, ruff and mypy clean. Four mutants killed, each verified to have applied: always-claim-gone, `NoReturn` reverted, `KEEL_HOME` dropped from the message, and the pid no longer named. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6
Re-review of this PR. The previous commit taught the refusal to distinguish a
dead process from a live one that is not listening -- and to do it, parsed the
pid a SECOND time, bare:
ValueError: invalid literal for int() with base 10: 'not-a-number'
`live_record` wrapped its conversion in `try`/`except`; `_refuse` repeated it
without one. `read_record` admits such a record, because it validates only that
a token is present. So a malformed pid tracebacked out of the one code path
whose entire job is to fail gracefully -- the exact promise `read_record`'s
docstring makes ("`keel open` must not traceback at an operator whose server has
just died").
`runtime.recorded_pid` is now the single answer both callers ask for. Fixing the
duplicate rather than adding a second guard, because two conversions of one field
is what produced this: guarding the new copy would have left the shape that
generates the next one.
`0` for anything unparseable is the safe reading: `process_alive` refuses zero
before signalling -- `os.kill(0, ...)` addresses this process's own group -- so
an unreadable pid reports as not running rather than as something.
6,419 passed / 3 skipped, ruff and mypy clean. Three mutants killed: the guard
removed, `_refuse` parsing for itself again, and `recorded_pid` always answering
zero.
The first attempt at the guard-removal mutant DID NOT APPLY -- `ruff format` had
rewritten `except (A, B):` into PEP 758's unparenthesised form, so the anchor
missed and the suite went green over an unmutated file. Caught by the assertion
that the source actually changed, which is the third time that rewrite has voided
a patch string in this series.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by an install that worked. Four console daemons bootstrapped, three wrote their records, and
keel open --port 8765answered "no recorded keel server" — so a success read as a failure.The command had been run from a source checkout, which is itself a deployment root (
is_deployment_rootis deliberately generous, and the tree has akeel.dband aconfig.yaml). So it searched the checkout'srun/while the servers wrote to~/keel/run/.The message listed three explanations and the true one was not among them. It could not be — it never said where it had looked.
After
keel opentakes no--dband no--config, so nothing in its signature hints that the answer depends on the working directory. The path is the hint, and--helpnow says so in one line.The stale-record branch gets it too — a crashed server and a wrong directory are different problems, and an operator staring at either needs the same fact to tell them apart.
Checks
6,413 passed / 3 skipped, ruff and mypy clean. Three mutants killed: the path dropped from each refusal branch, and the help line removed. Verified by running the command from the wrong tree and reading what it prints.
One thing worth recording for #756
launchd does give
keel servea stdout that is not a tty. Three daemons, three records written. That was the single assumption the wholekeel opendesign rests on and that nothing in CI could test.🤖 Generated with Claude Code
https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6