MP-161 + MP-170: error text is TEXT, escaped once at the root - #69
Merged
Conversation
The single message whose whole job is to unstick a blocked user was corrupted into
the command they had just run:
in : The OpenAI SDK is not installed. Install it with: pip install 'modelpin[providers]'
out: error: The OpenAI SDK is not installed. Install it with: pip install 'modelpin'
`[M]` `_fail` rendered its message through rich markup unescaped, so `[providers]`
was parsed as a style tag and eaten. It is the FIRST error a stranger without the
extra hits, and it sends them in a loop.
The second failure mode is worse in kind and is why this closes MP-170 too:
in : scenario [bold]alpha[/] could not be replayed
out: error: scenario alpha could not be replayed
No exception, no signal -- a plausible message naming something that does not
exist. Scenario ids have no pattern validator and provider messages are remote
text, so neither input is ours to trust.
`[M]` This class had been found FIVE times at individual call sites, and four
separate comments claimed the last one was closed. `_fail_no_scenarios` even
escaped its own argument and carried a comment explaining the hazard -- the symptom
patched one function away from the cause. The escape now lives in `_fail`, and that
local workaround is deleted in the same commit: keeping both would print a literal
backslash at the one call site that had been careful. `[M]` Verified across all 30
`_fail` call sites that none passes intentional markup.
Swept the rest of the error surface as the row required. Numeric interpolations are
safe; the two that were not are the Report write paths, where `--output-dir` is
user-supplied and an OSError carries a path -- a directory named `[wip]` vanished
from the one line saying where the Report went. Model ids in markup are a different
surface and stay filed as MP-174.
The test asserts on RENDERED console output, not on the markup string, and pins the
hint against `providers/openai.py` so a guard cannot outlive the message it guards.
`[M]` Both halves mutation-checked: reverting the escape fails 5 of 6; restoring the
local double-escape fails 1.
`[M]` /verify READY -- 747 passed + 4 xfailed (751 collected), 94% of 2083, ruff +
black clean, mypy 5 = baseline.
…ing the CLI
The previous commit escaped `_fail` and implied the unescaped-markup class was
finally closed. That claim has now been wrong FIVE times in this codebase, and the
first-run gate refuted it again -- by execution, not by reading:
`[M]` `modelpin scan` on a repo containing `src [experimental]/app.py` printed
| gpt-4o-mini | src \app.py | 1 |
A rich Table parses cell content as MARKUP by default, and these cells are paths
discovered in someone else's repo. `scan` named a file that does not exist. Any
Next.js repo -- whose route folders are literally `[slug]` -- gets every row
corrupted, on one of the first commands a stranger runs.
`[M]` `modelpin init --demo "target [wip] dir"` reported writing to
`target dir\...` -- the tool misdescribing files it had just created, using the
argument the user typed a second earlier. Four print sites, all silent.
Fixed both, plus the assertion-key line where the ids were escaped and the keys
beside them were not -- escaping one and not the other on the same line is the
inconsistency that hides the next one.
The Table cells are `rich.text.Text`, not `_rich_escape`d strings, because `Text`
cannot be re-parsed downstream.
Also corrected, all mine, all from the same gate:
A neighbouring comment still told the reader the root cause was OPEN, one commit
after it was closed. That is how this file accumulated four comments each claiming
the last unescaped site was fixed; left alone it sends the next reader hunting a
bug that is not there.
`[M]` "all 30 `_fail` call sites" was a grep artifact, not a count -- `grep -c`
reports 29 including the definition itself. It is 28, plus two via wrappers. Under
this project's evidence rule a wrong `[M]` number is worse than no number.
The test harness re-rendered captured output through a Console the TEST built,
so it measured a console of its own design rather than `cli.console`. It uses
`Console.capture()` on the real object now -- which immediately exposed that the
assertions were also measuring line wrapping, since the real console wraps at 80
and breaks a long path mid-token.
`[M]` Both new guards mutation-checked: revert the Table to raw strings, or the
init path to unescaped, and one test fails each time.
`[M]` /verify READY -- 749 passed + 4 xfailed (753 collected), 95% of 2085, ruff +
black clean, mypy 5 = baseline.
`[M] 2026-09-02` The two new tests compared raw substrings against console output and passed on Windows, then failed on Linux CI: the longer `/tmp/pytest-of-runner/...` prefix pushed the wrap INSIDE the path, splitting `target [wip] dir` as `target [wip] ` + `dir`. This module already defines `_unwrapped` for precisely this, added an hour earlier after the same failure. I wrote the helper and then did not use it two functions below. Third time today an assertion measured where the line broke rather than what the product printed -- the previous two being the same fix in `test_report_archive.py` (which had been red on CI and blocking two PRs) and in this file's own path assertion. The product output was correct in every one of those cases.
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.
Two commits. Closes MP-161 and MP-170 — they turned out to be the same defect, because MP-161's own prescribed fix was MP-170's root cause.
The user-visible bug
[M]A user whose install lacks the SDK extra was told to run the command they had just run:_failrendered its message through rich markup unescaped, so[providers]was parsed as a style tag and eaten. It is the first error a stranger without the extra hits, and it is a loop.The worse half
[M]scenario [bold]alpha[/] could not be replayedprinted asscenario alpha could not be replayed. No exception, no signal — a plausible message naming something that does not exist. Scenario ids have no pattern validator and provider messages are remote text, so neither input is ours to trust.Why the escape is in
_fail, not at the call site[M]This class had been found at five call sites across five rows, each patched locally, and four separate comments claimed the last one was closed._fail_no_scenarioseven escaped its own argument and carried a comment explaining the hazard — the symptom patched one function from the cause. That local workaround is deleted in the same commit; keeping both would print a literal backslash at the one call site that had been careful.The second commit exists because the gate refuted the first
The first commit implied the class was finally closed. The first-run gate disproved it by running the CLI:
[M]modelpin scanon a repo containingsrc [experimental]/app.pyprinted| gpt-4o-mini | src \app.py | 1 |— a richTableparses cell content as markup, and those cells are paths found in someone else's repo.scannamed a file that does not exist. Any Next.js repo, whose route folders are literally[slug], gets every row corrupted.[M]modelpin init --demo "target [wip] dir"reported writing totarget dir\...— the tool misdescribing files it had just created, using the argument the user typed a second earlier.749 tests passed the whole time. Table cells are
rich.text.Textnow, not escaped strings, becauseTextcannot be re-parsed downstream.Also corrected, all mine
[M]"all 30_failcall sites" was a grep artifact —grep -creports 29 and counts the definition. It is 28 plus two wrappers.cli.console. It usesConsole.capture()on the real object now — which immediately exposed that the assertions were also measuring line wrapping.[M]Every guard mutation-checked./verifyREADY — 749 passed + 4 xfailed (753 collected), 95% of 2085, ruff + black clean, mypy 5 = baseline.Filed, not fixed: MP-174 (model ids in markup — a different surface), MP-177 (the no-key check fires before the missing-SDK check, so this fixed message sits behind a door most strangers never open).