fix(cli): bridge two vocabularies, move a late warning, stop a contradictory PASS (#389) - #502
Merged
Merged
Conversation
…dictory PASS (#389) Three of the four remaining bullets. The fourth needed no change: `doctor`'s Ollama remediation already distinguishes "not installed" from "not running" -- it checks PATH and, when the binary is absent, points at the download page and the macOS app instead of a `serve` command that does not exist there. Verified in the source and traced to the commit that added it. `reconcile` reported "recorded 'X' as superseding 'Y'" while `list` shows `deprecated` in its STATUS column, so the operator met two words for the action just performed and its effect, with nothing connecting them. The line now names the status the superseded concept will carry. `init`'s embedding stickiness note arrived after the choice it informs AND below "Next: run openkos ingest" -- past both moments it exists for. A note now precedes the picker, and the concrete one naming the resolved tag was moved above the call to action. `doctor` printed a green `[PASS] Backend host locality` directly beneath `[FAIL] Ollama reachable`. It reports configuration, not liveness, which is correct -- but the reader has no way to know the two answer different questions, so it read as a contradiction. It is now `[SKIP]` when Ollama is unreachable, following the same rule the model and embedding checks already use, with the configured host still named in the detail so the fact survives. Reviewed under lineage `review-a592b741d66c0577`, high risk, canonical 4R, approved with receipt, zero blockers.
…ng nothing (#389) Folds in the three findings from this branch's own review. Moving a warning earlier is worthless if the reader then meets the same sentence again a few lines down, which is exactly what the previous commit did on an interactive run. A flag now records whether the picker note already carried the explanation: if it did, the later line only confirms which tag is sticky; if it did not, the later line carries the explanation itself. All four combinations of interactive-or-not and flag-or-not print it exactly once. The sharpest finding was that a test added one commit ago was VACUOUS. It claimed to prove the note precedes a rendered embedding picker, but patching the installed-model probe to return an empty list makes that picker take its documented SILENT fallback and print nothing -- so the test passed for the same weaker reason as its non-interactive sibling. It now reports an allowlisted model as installed and asserts the picker text actually appeared. The header comment above `doctor`'s locality check still asserted it has no skip branch, which the previous commit had just made false. Reviewed under lineage `review-e278673103fe353a`, high risk, canonical 4R, approved with receipt, zero blockers.
#389) The doctor docstring described its backend-host-locality check as having no `[SKIP]` branch and, unlike every other check, always passing. Two commits ago that check gained a skip branch for the unreachable case, and the inline comment beside the code was corrected then -- this copy was missed, so the function docstring and the comment beside the code said opposite things about the same check. Verified against the implementation before rewriting: the status is literally pass when reachable and skip otherwise, the result is never critical, the exit-code gate fires only on a critical failure, and a non-empty detail is produced on both branches. Reviewed under lineage `review-160212498ddbc097`, high risk, canonical 4R, approved with receipt, zero blockers.
…ocstrings (#389) The reasoning for why doctor's backend-host-locality check skips when Ollama is unreachable had ended up in three places: the command docstring and two inline comment blocks. The docstring keeps it; the inline blocks say only what is local to their position. Two docstrings in the test module still described the old always-passes behavior, which this branch made false three commits ago. Reviewed under lineage `review-fa0fac70d7a87702`, high risk, canonical 4R, approved with receipt, zero blockers.
…389) The previous commit added a sentence saying the locality check's reasoning is not repeated beside the code, and left the paragraph that repeated it directly underneath. Two lenses caught it independently. That is the same class of false claim this branch spent several rounds removing, so it goes rather than drifts. The block now states only what is local to its position: which client it reuses, why it skips when Ollama is unreachable despite being able to answer without it, and why it must never become a failure. Comment-only. Reviewed under lineage `review-6eecbecf76705e3b`, high risk, canonical 4R, approved with receipt, zero findings across all four lenses.
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
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.
Closes #389 — the last of its bullets. One needed no change; the rest are here.
Already fixed, verified rather than assumed
doctor's Ollama remediation already distinguishes "not installed" from "not running": it checks PATH and, when the binary is absent, points at the download page and mentions the macOS app instead of printing aservecommand that does not exist on that machine. Traced to the commit that introduced it rather than taken from the issue text.The three real ones
Vocabulary.
reconcilereported "recorded 'X' as superseding 'Y'" whilelistshowsdeprecatedin its STATUS column — two words for the action just performed and its effect, with nothing connecting them. The line now names the status the superseded concept will carry.Placement.
init's embedding stickiness note arrived after the choice it informs and below "Next: run openkos ingest" — past both moments it exists for. A note now precedes the picker; the concrete one naming the resolved tag sits above the call to action.A green line under a red one.
doctorprinted[PASS] Backend host localitydirectly beneath[FAIL] Ollama reachable. It reports configuration, not liveness, which is correct — but the reader has no way to know the two answer different questions. It is now[SKIP]when Ollama is unreachable, following the same rule the model and embedding checks already use, with the configured host still in the detail so the fact survives.What the reviews caught — three rounds, all of it mine
Round 1 fixed the placement and immediately created a new problem: an interactive run now printed the stickiness explanation twice, once before the picker and once after. Moving a warning earlier is worthless if the reader meets the same sentence again five lines down. A flag now selects: if the picker note carried the explanation, the later line only confirms which tag it applies to.
Round 1 also caught that a test added in that same commit was vacuous. It claimed to prove the note precedes a rendered embedding picker, but patching the installed-model probe to return an empty list makes that picker take its documented silent fallback — printing nothing at all. It passed for the same weak reason as its non-interactive sibling. It now reports an allowlisted model as installed and asserts the picker text actually appeared.
Rounds 2 and 3 were about prose telling lies: the
doctordocstring still said the locality check has no skip branch, which round 1 had just made false; then my consolidation added a sentence saying the reasoning "is not repeated here" while leaving the repeat directly below it. Two lenses caught that one independently.Verification
uv run pytest -q→ 4032 passed, 1 skipped.uv run mypy .→ Success, 179 files.ruff check/ruff formatclean and convergent.Native review
Five lineages, all approved with receipts, all zero blockers. The last one returned zero findings across all four lenses.
pre-prdenies this PR, same structural reason as #498 through #501: it measures the whole branch againstorigin/mainwhile each receipt binds its own increment.receipt-binding / candidate-or-paths-mismatch, actionexplicit-maintainer-action. Every changed line was reviewed and approved.