fix(llm,cli): tell the truth about a missing ceiling and name unmerge (#440, #445) - #499
Merged
Conversation
…#440, #445) Two messages that misdescribed what happened. `OllamaGenerationCapped` interpolated `max_generation_tokens` unconditionally, so a client constructed without one raised "the configured max_generation_tokens ceiling (None)" -- self-contradictory, since nothing was configured. That branch is genuinely reachable: Ollama reports `done_reason == "length"` whenever generation stops for length reasons, including the model's own context window filling, not only when `num_predict` binds. The message now names the ceiling when there is one and otherwise attributes the cut-off to the backend's own limit. Adds the two `done_reason` edge cases nothing pinned: an unexpected string, and a non-string. `contradictions` named a merged-content condition without naming the verb that resolves it. That matters more here than for a pair verdict: a pair has two nodes an operator can open, while a merged-content verdict has ONE, and the disagreeing second body lives in the ledger where no ordinary read surfaces it. The report now prints `openkos unmerge <survivor> <absorbed>`, the two ids that verb takes. A pair verdict deliberately gets no such line. Also promotes the `## Merged content (` heading to `okf.MERGED_CONTENT_HEADING_PREFIX`, which was duplicated between the module that writes it and the module that finds it to cut a ledger snapshot back to the survivor's own content. Reviewed under lineage `review-b5b91450ae735c18`, high risk, canonical 4R, approved with receipt and zero blockers.
…ssage (#440, #445) Folds in the three findings from this branch's own four-lens review (lineage `review-b5b91450ae735c18`). The `next: openkos unmerge` hint added in 3ee11e8 could name an absorbed id belonging to a NON-tail merge-ledger entry. Verified against the source rather than taken on the reviewer's word: `_merged_body_candidates` raises one candidate per `merged_from` entry, not only the newest, while `plan_unmerge` refuses any `absorbed_id` that is not the ledger tail. On a survivor with two unreversed merges, the older verdict's suggested command would therefore refuse. The line now states that precondition instead of promising success. The unconfigured-ceiling test asserted only that "None" and "configured" were absent and "truncated" present -- assertions any vaguer message would also satisfy. It now pins the exact sentence raised, and keeps both absence assertions so a regression to "(None)" still fails loudly. The module-private alias of `okf.MERGED_CONTENT_HEADING_PREFIX` had one use site and a docstring restating the public constant's own. Removed; the use site references the public constant directly. Both sides stay pinned: the writer through the merged-document layout test, the reader through the ledger-snapshot truncation test. Reviewed under lineage `review-782bc636462d2286`, high risk, canonical 4R, approved with receipt, zero blockers.
jasonssdev
added a commit
that referenced
this pull request
Aug 9, 2026
) Closes #486. `next`'s rank-0 bootstrap tier gates on `signals.docs`, populated by a walk that silently drops any subdirectory it cannot list. `lint.collect_docs` reports the documents it could not READ, never the directories it could not LIST, so a bundle whose documents all live under an unreadable subtree produced an empty document list with no skip notice at all -- and `next` answered "ingest your first source", advice for a different bundle. `status` already folds those walk errors into "Needs attention"; `next` had no such pass. A memoized `walk_incomplete` signal reads `okf._walk_errors` and is consulted by the bootstrap tier alone, after its two zero-walk gates have already concluded the bundle looks empty. When the walk is provably incomplete the tier recommends `openkos status`, the verb that names which directory could not be read, instead of a first ingest. `NextAction.command`'s contract promised a command runnable verbatim while the bootstrap tier returns an `openkos ingest <path>` template. The contract now names that single exception and the rule for when it should become a structured field instead. The review of the first commit filed six findings, all folded into the second. Two mattered: the `except OSError` added around `okf._walk_errors` was dead code, because that helper hands `os.walk` an `onerror` collector so scan failures become data rather than exceptions -- and keeping any handler would have swallowed other failures into "the walk was complete", the exact claim the property exists to prevent. And the cost claim was overstated: an empty `docs` means zero ELIGIBLE documents, not a small tree. Also closes the cosmetic double-space item disclosed in #499; the real fix is the test, which now pins the whole rendered line rather than three separate substrings. Reviews: `review-2938f054fe29ac79` (high risk, canonical 4R) and `review-da8f29e16c16ef87` (medium risk, one focus lens, zero findings), both approved with receipts and zero blockers. The `pre-pr` gate denied on receipt scope, not review coverage: each receipt binds its own increment, so neither covers the branch union. Merged under explicit maintainer action, disclosed in the pull request body. Refs #386, #275, #445.
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 #440.
Closes #445.
Two messages that misdescribed what actually happened.
#440 — "the configured ceiling (None)"
OllamaGenerationCappedinterpolatedmax_generation_tokensunconditionally, so a client built without one raised "the configured max_generation_tokens ceiling (None)" — self-contradictory, since nothing was configured.That branch is genuinely reachable, not theoretical: Ollama reports
done_reason == "length"whenever generation stops for length reasons, including the model's own context window filling, not only whennum_predictbinds. The message now names the ceiling when there is one, and otherwise attributes the cut-off to the backend's own limit.Also adds the two
done_reasonedge cases nothing pinned: an unexpected string, and a non-string.#445 — naming the condition without naming the verb
contradictionsreported a merged-content verdict and never mentionedunmerge. That matters more here than for an ordinary pair verdict: a pair has two nodes an operator can open, while a merged-content verdict has one, and the disagreeing second body lives in the ledger where no ordinary read surfaces it.The report now prints the command with the two ids that verb takes. A pair verdict deliberately gets no such line — nothing was merged, and pointing at a reversal verb would be wrong.
Also promotes
## Merged content (tookf.MERGED_CONTENT_HEADING_PREFIX, which was duplicated between the module that writes it and the module that finds it to cut a ledger snapshot back to the survivor's own content.What the review caught, and why it was worth the cycle
The first review of this branch found a real defect in the fix itself: the
next: openkos unmerge …hint could name an absorbed id belonging to a non-tail merge-ledger entry. Verified against the source rather than taken on the reviewer's word —_merged_body_candidatesraises one candidate permerged_fromentry, not only the newest, whileplan_unmergerefuses anyabsorbed_idthat is not the ledger tail. On a survivor with two unreversed merges, the older verdict's suggested command would have refused. The line now states that precondition instead of promising success.Two more findings folded in the same commit: an assertion that only checked for the absence of two words (and so would have passed against any vaguer message) now pins the exact sentence; and a module-private alias that restated the public constant's own docstring was removed.
Verification
uv run pytest -q→ 4017 passed, 1 skipped.uv run mypy .→ Success, 179 files.ruff check/ruff formatclean and convergent.Native review, and the same gate caveat as #498
Two lineages, both approved with receipts, both zero blockers:
review-b5b91450ae735c18— the main change (3ee11e8). High risk, canonical 4R. Three informational findings, all folded rather than deferred.review-782bc636462d2286— the folded corrections (9672821). High risk, canonical 4R. Two SUGGESTIONs, both about one cosmetic item (below).pre-prdenies this PR, deliberately and disclosed. It measures the whole branch againstorigin/mainwhile each receipt binds only its own increment, so neither covers the union. Denial isreceipt-binding / candidate-or-paths-mismatch, actionexplicit-maintainer-action. Every changed line was reviewed and approved; what is missing is a single receipt whose scope equals the union — a binding gap, not a coverage gap.Known cosmetic item, recorded not fixed: the rendered hint carries a double space before its parenthetical (
…apatheia-2 (LIFO-enforced: …)), and the test asserts its three parts as separate substrings so spacing drift is unpinned. The candidate was frozen when the lenses reported it; folding a double space would cost another full four-lens cycle, so it is disclosed here instead of silently changed.