diff --git a/.coderabbit.yaml b/.coderabbit.yaml index bc0b1b4f..a52f1c91 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -53,6 +53,13 @@ reviews: - "!**/__pycache__/**" path_instructions: + - path: "**/*.py" + instructions: | + Python uses snake_case for variables, functions, and methods per PEP 8; this is the + established convention across the entire codebase. Do NOT suggest renaming Python + identifiers to camelCase — such suggestions contradict the repo standard and are out of + scope. The camelCase convention applies only to TypeScript/JavaScript sources, never to + .py files. - path: "src/pythinker_code/telemetry/**" instructions: | Flag any changes that could inadvertently capture PII, secrets, or sensitive user data. diff --git a/.pythinker/reports/validation-tui-renderer-contract-hardening.md b/.pythinker/reports/validation-tui-renderer-contract-hardening.md new file mode 100644 index 00000000..d8851885 --- /dev/null +++ b/.pythinker/reports/validation-tui-renderer-contract-hardening.md @@ -0,0 +1,136 @@ +# Validation Report: TUI Renderer Contract Hardening Review + +## Verdict + +The pasted review is **partially validated**. It correctly identifies a real edge-case mismatch in `_CODE_SPAN_RE` and a weak test assertion, but it overstates severity and contains at least one unvalidated/likely incorrect GFM/table-escape claim. I would not treat the pasted report's "2 high-priority issues" as release blockers. + +## Scope and evidence + +Validated against branch `feat/tui-renderer-contract-hardening` using targeted reads and commands. + +Commands run: + +```bash +git rev-list --count main..HEAD +git diff --name-only main...HEAD | wc -l +git diff --shortstat main...HEAD +uv run pytest tests/ui_and_conv/test_md_table_contract.py tests/ui_and_conv/test_md_stream_idempotency.py -q +uv run pytest tests/ui_and_conv -q +uv run python - <<'PY' +from markdown_it import MarkdownIt +from pythinker_code.ui.shell.components.markdown import _escape_code_span_pipes + +md = MarkdownIt('commonmark').enable('table') +for label, row in [ + ('mismatched closing run', '| `a | b`` | rest |'), + ('double slash before pipe', '| `a \\\\| b` | rest |'), +]: + src = '| Expr | Meaning |\n| --- | --- |\n' + row + '\n' + cells = [] + for token in md.parse(src): + if token.type == 'inline': + cells.append((token.content, [(c.type, c.content) for c in (token.children or [])])) + print(label) + print('escaped-row:', repr(_escape_code_span_pipes(row))) + print('markdown-it inline cells:', cells) +PY +``` + +Results: + +- Branch metadata from `main...HEAD`: **16 commits**, **24 files changed**, `1966 insertions(+), 51 deletions(-)`. This does **not** match the pasted report's "15 commits / 16 files" claim. +- Targeted tests: `15 passed`. +- Full `tests/ui_and_conv`: `1305 passed`. + +## Finding validation + +### 1. `_CODE_SPAN_RE` mismatched backtick behavior + +Status: **Validated as an edge case, severity overstated.** + +Evidence: + +- `src/pythinker_code/ui/shell/components/markdown.py` defines `_CODE_SPAN_RE = re.compile(r"(?P`+)(?P.*?)(?P=ticks)")`. +- For `| `a | b`` | rest |`, `_escape_code_span_pipes` returns `| `a \| b`` | rest |`. +- `markdown-it-py` without the pre-escape parses the row cells as text fragments `('`a')` and `('b``')`, not a balanced code span. + +Interpretation: + +The observation is technically real: the regex can treat the first backtick of a longer closing run as the equal-length closer. However, this input is already malformed, and the normalizer is explicitly a repair/tolerance path for LLM-produced table rows. The product decision is whether to enforce strict GFM code-span boundaries or keep forgiving repair behavior. + +Recommended action: + +- Add a characterization test for mismatched backtick runs. +- Decide and document the intended policy: + - strict GFM: do not escape pipes in mismatched runs; or + - tolerant LLM repair: keep current behavior and test it as intentional. + +Suggested severity: **Low/Medium**, not High. + +### 2. Double-backslash before pipe claim + +Status: **Not validated; likely incorrect for the current parser contract.** + +Evidence: + +For row `| `a \\| b` | rest |`: + +- `_escape_code_span_pipes` returns it unchanged. +- `markdown-it-py` parses a valid table row with cells: + - `code_inline` content: `a \| b` + - second cell: `rest` +- The table structure remains intact. + +Interpretation: + +The pasted report assumes even/odd backslash semantics that do not match the observed `markdown-it-py` table behavior used by this code path. The current implementation's simple negative lookbehind preserves table structure for this case. The proposed regex change could alter literal backslash rendering and should not be applied without a concrete failing renderer test. + +Recommended action: + +- Do **not** treat this as a confirmed bug. +- If this edge matters, first add a renderer-level characterization test for the desired source-to-rendered output. + +Suggested severity: **None / advisory only**. + +### 3. `test_table_with_piped_inline_code_keeps_columns` assertion precision + +Status: **Validated, but low severity.** + +Evidence: + +The test in `tests/ui_and_conv/test_md_table_contract.py` checks only that `bitwise or`, `plain`, and `text` appear in rendered output. Those checks are useful but do not strongly prove table structure survived. + +Recommended action: + +Strengthen with a structural assertion that the header/data relationship survives, or with a lower-level normalized-markdown/token assertion. Keep it simple; avoid brittle visual-layout assertions. + +Suggested severity: **Low**. + +### 4. Parameterizing test strings + +Status: **Valid nit, not a defect.** + +This is maintainability advice only. Current explicit tests are readable and acceptable. + +Suggested severity: **Nit / optional**. + +### 5. Stream intermediate-state assertions + +Status: **Not validated as useful.** + +The existing stream test already validates exact reassembly and no duplicated rendered rows. The suggested `1 <= len(committed) <= 10` check is arbitrary and may become brittle if commit-boundary heuristics change without user-visible regression. + +Recommended action: + +Do not add the suggested slice-count assertion. If stronger coverage is needed, assert a named invariant tied to user-visible behavior, not an arbitrary count. + +## Recommended next actions + +1. Correct the review metadata: current branch evidence is 16 commits / 24 files from `main...HEAD`. +2. Add one characterization test for mismatched backtick runs in `_escape_code_span_pipes`. +3. Optionally strengthen `test_table_with_piped_inline_code_keeps_columns` with a non-brittle structural assertion. +4. Do not implement the pasted report's double-backslash regex recommendation unless a failing renderer-level test proves the desired behavior. + +## Notes + +The graphify knowledge graph may be stale because files changed in this session; validation above used targeted raw-file reads and deterministic commands rather than relying on the graph for modified areas. diff --git a/docs/superpowers/plans/2026-05-29-tui-markdown-report-contract-hardening.md b/docs/superpowers/plans/2026-05-29-tui-markdown-report-contract-hardening.md new file mode 100644 index 00000000..26e4ec99 --- /dev/null +++ b/docs/superpowers/plans/2026-05-29-tui-markdown-report-contract-hardening.md @@ -0,0 +1,968 @@ +# TUI Markdown + Report Contract-Hardening — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Turn the Markdown + security/scan-report bug-class catalog into an aggressive, deterministic regression suite that the existing Rich + prompt_toolkit stack passes, fixing only the one structural defect (nested-report-fence promotion) the tests expose. + +**Architecture:** Lead phase of `docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md`. We do **not** build a renderer. We add Tier-1 contract tests (capture + assertion) under `tests/ui_and_conv/`, characterize the existing regex repair pipeline (pin, don't refactor), ground report tests in the real 92-finding fixture, and apply exactly one source fix (AST-based report-fence extraction in `report.py`) gated by a failing test. + +**Tech Stack:** Python 3.12+, Rich 15, prompt_toolkit 3, markdown-it-py, pytest (`asyncio_mode = auto`), `uv`. Run tests with `uv run pytest …`. + +**Spec reference:** `docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md` §6–§8. + +**Source files in play:** +- `src/pythinker_code/ui/shell/components/report.py` — report dataclasses, `render_report`, `parse_report_block`, `render_agent_body`, `has_report_block` (the **only** file we modify, in Task 9) +- `src/pythinker_code/ui/shell/components/markdown.py` — `pythinker_markdown`, regex table-repair pipeline, `PythinkerMarkdownStream`, `markdown_commit_boundary` (characterized, **not** modified) +- `src/pythinker_code/utils/rich/markdown.py` — Rich Markdown subclass (exercised, not modified) + +**Test files created:** +- `tests/ui_and_conv/_md_contract_helpers.py` — shared capture/idempotency/param helpers +- `tests/ui_and_conv/test_md_table_contract.py` — table bug classes (spec area 3) +- `tests/ui_and_conv/test_md_repair_characterization.py` — pin the regex repair pipeline +- `tests/ui_and_conv/test_md_color_contract.py` — color-bleed / ANSI (spec area 4) +- `tests/ui_and_conv/test_report_realdata.py` — report render grounded in `security-scan-findings.json` +- `tests/ui_and_conv/test_report_fence_nesting.py` — H1 (the one real fix) +- `tests/ui_and_conv/test_md_stream_idempotency.py` — H2 + H3 + +**Methodology note (read before starting):** This plan mixes three test kinds. Know which you're writing: +- **Characterization** (pin): the behavior already exists; the test passes on first run and locks it in. "Expected: FAIL" does **not** apply — expected is PASS, and that is the point. +- **Contract** (guard): asserts a spec requirement the stack *should* already meet; expected PASS. If it FAILS you've found a real bug — stop and surface it, don't paper over it. +- **Hypothesis** (H1/H2/H3): written to *try* to reproduce a suspected defect. H1 is expected to FAIL first (bug present) then PASS after the fix. H2/H3 may PASS immediately (non-reproduction) — record that and keep them as guards. + +--- + +### Task 1: Shared contract-test helpers + +**Files:** +- Create: `tests/ui_and_conv/_md_contract_helpers.py` +- Test: `tests/ui_and_conv/test_md_contract_helpers.py` + +- [ ] **Step 1: Write the failing test** + +```python +# tests/ui_and_conv/test_md_contract_helpers.py +"""Smoke test for the shared Markdown/report contract helpers.""" + +from __future__ import annotations + +from rich.text import Text + +from tests.ui_and_conv._md_contract_helpers import ( + THEMES, + WIDTHS, + render_ansi, + render_plain, + render_twice_identical, +) + + +def test_helpers_capture_and_compare(): + assert "hello" in render_plain(Text("hello"), width=40) + # truecolor capture keeps SGR codes; a red fg emits the 31-family sequence. + assert "\x1b[" in render_ansi(Text("hi", style="red"), width=40) + assert render_twice_identical(lambda: Text("stable")) is True + assert WIDTHS and THEMES # parametrization sources are non-empty +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/ui_and_conv/test_md_contract_helpers.py -v` +Expected: FAIL with `ModuleNotFoundError: tests.ui_and_conv._md_contract_helpers` + +- [ ] **Step 3: Write the helper module** + +```python +# tests/ui_and_conv/_md_contract_helpers.py +"""Shared helpers for Markdown + report contract tests. + +DRY home for the two capture modes the repo already uses (plain text and +ANSI-preserving) plus an idempotency comparator. Mirrors the console +configuration in tests/ui_and_conv/test_tui_render_snapshots.py and +tests/ui_and_conv/test_report.py so captured output matches the rest of the +suite. +""" + +from __future__ import annotations + +from typing import Callable + +from rich.console import Console, RenderableType + +# Widths that exercise reflow boundaries: very narrow, a normal width, and an +# exactly-typical report width. Add the exact-full-width case per test. +WIDTHS: tuple[int, ...] = (24, 40, 80) +THEMES: tuple[str, ...] = ("dark", "light") + + +def render_plain(renderable: RenderableType, *, width: int = 80) -> str: + """Capture *renderable* as plain text (no color), like test_report._plain.""" + console = Console(width=width, no_color=True, legacy_windows=False) + with console.capture() as cap: + console.print(renderable) + return cap.get() + + +def render_ansi(renderable: RenderableType, *, width: int = 80) -> str: + """Capture *renderable* keeping ANSI escapes, like test_tui_render_snapshots._ansi.""" + console = Console( + width=width, + record=True, + force_terminal=True, + color_system="truecolor", + legacy_windows=False, + ) + console.print(renderable) + return console.export_text(styles=True) + + +def render_twice_identical(build: Callable[[], RenderableType], *, width: int = 80) -> bool: + """Render a freshly-built renderable twice; True iff byte-identical. + + `build` returns a NEW renderable each call so we test render determinism, + not object identity. + """ + return render_ansi(build(), width=width) == render_ansi(build(), width=width) +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `uv run pytest tests/ui_and_conv/test_md_contract_helpers.py -v` +Expected: PASS (2 lines of output, 1 passed) + +- [ ] **Step 5: Commit** + +```bash +git add tests/ui_and_conv/_md_contract_helpers.py tests/ui_and_conv/test_md_contract_helpers.py +git commit -m "test(tui): add shared markdown/report contract helpers" +``` + +--- + +### Task 2: Table contract — piped inline code & escaped pipes (spec area 3) + +**Files:** +- Create: `tests/ui_and_conv/test_md_table_contract.py` +- Test: same file + +- [ ] **Step 1: Write the contract test** + +```python +# tests/ui_and_conv/test_md_table_contract.py +"""Tier-1 contract tests for Markdown table rendering (spec area 3). + +Each test names the bug class it guards. These assert the EXISTING stack +(pythinker_markdown over markdown-it + Rich) already meets the contract; a +failure is a real regression to surface, not to silence. +""" + +from __future__ import annotations + +import pytest + +from pythinker_code.ui.shell.components.markdown import pythinker_markdown +from tests.ui_and_conv._md_contract_helpers import render_plain + + +def test_table_with_piped_inline_code_keeps_columns(): + """Bug class: 'tables breaking on piped inline code'.""" + md = ( + "| Expr | Meaning |\n" + "| --- | --- |\n" + "| `a | b` | bitwise or |\n" + "| plain | text |\n" + ) + out = render_plain(pythinker_markdown(md), width=80) + # Both data rows survive as a table (cell contents present, not collapsed + # into a single prose paragraph). + assert "bitwise or" in out + assert "plain" in out + assert "text" in out + + +def test_table_with_escaped_pipes_keeps_literal_pipe(): + """Bug class: escaped pipe must render as a literal '|', not split a cell.""" + md = "| Col |\n| --- |\n| a \\| b |\n" + out = render_plain(pythinker_markdown(md), width=80) + assert "a | b" in out or "a \\| b" not in out # literal pipe preserved + assert "Col" in out +``` + +- [ ] **Step 2: Run to verify it passes (contract already met)** + +Run: `uv run pytest tests/ui_and_conv/test_md_table_contract.py -v` +Expected: PASS. If either FAILS, you've found a live table bug — stop and report it against spec area 3 before continuing. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_table_contract.py +git commit -m "test(tui): guard tables against piped/escaped inline code" +``` + +--- + +### Task 3: Table contract — empty headers, long cells, narrow widths (spec area 3) + +**Files:** +- Modify: `tests/ui_and_conv/test_md_table_contract.py` + +- [ ] **Step 1: Append the parametrized contract tests** + +```python +# append to tests/ui_and_conv/test_md_table_contract.py +from tests.ui_and_conv._md_contract_helpers import WIDTHS # noqa: E402 + + +def test_table_empty_header_cell_does_not_mislabel(): + """Bug class: 'empty header cells mislabeled in narrow stacked layout'.""" + md = "| | Value |\n| --- | --- |\n| key | 42 |\n" + out = render_plain(pythinker_markdown(md), width=30) + assert "Value" in out + assert "key" in out + assert "42" in out + + +@pytest.mark.parametrize("width", WIDTHS) +def test_table_long_cell_wraps_without_dropping_content(width): + """Bug class: very long cells at narrow widths must wrap, not truncate.""" + long_cell = "alpha beta gamma delta epsilon zeta eta theta iota kappa" + md = f"| Name | Note |\n| --- | --- |\n| item | {long_cell} |\n" + out = render_plain(pythinker_markdown(md), width=width) + # Every word of the long cell survives somewhere in the wrapped output. + for word in long_cell.split(): + assert word in out, f"word {word!r} dropped at width={width}" +``` + +- [ ] **Step 2: Run to verify they pass** + +Run: `uv run pytest tests/ui_and_conv/test_md_table_contract.py -v` +Expected: PASS (5 tests). A FAIL on the width-parametrized test is a real reflow bug — surface it. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_table_contract.py +git commit -m "test(tui): guard table empty-header and long-cell wrapping" +``` + +--- + +### Task 4: Color-bleed contract — border vs inline-code color (spec area 4) + +**Files:** +- Create: `tests/ui_and_conv/test_md_color_contract.py` + +- [ ] **Step 1: Write the contract test** + +```python +# tests/ui_and_conv/test_md_color_contract.py +"""Tier-1 ANSI/color contract tests (spec area 4). + +Uses the truecolor-preserving capture so we can assert on SGR sequences, +exactly like tests/ui_and_conv/test_tui_render_snapshots.py. +""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.markdown import pythinker_markdown +from pythinker_code.ui.theme import get_markdown_colors +from tests.ui_and_conv._md_contract_helpers import render_ansi + + +def _sgr_fg(hexcolor: str) -> str: + """Build the truecolor foreground SGR fragment for a #rrggbb color.""" + h = hexcolor.lstrip("#") + r, g, b = int(h[0:2], 16), int(h[2:4], 16), int(h[4:6], 16) + return f"38;2;{r};{g};{b}" + + +def test_code_block_border_does_not_use_inline_code_color(): + """Bug class: 'border colors inheriting code-span color'. + + The bordered code block frame uses code_block_border; inline code uses + inline_code. They must be distinct colors, and the captured frame must not + paint the border in the inline-code color. + """ + colors = get_markdown_colors("dark") + assert colors.code_block_border != colors.inline_code, ( + "precondition: palette must distinguish border from inline code" + ) + md = "Here is `inline` and a block:\n\n```python\nx = 1\n```\n" + coloured = render_ansi(pythinker_markdown(md), width=60) + # The rounded frame characters must not carry the inline-code foreground. + inline_fg = _sgr_fg(colors.inline_code) + for frame_char in ("╭", "╰", "─"): + idx = coloured.find(frame_char) + if idx == -1: + continue + window = coloured[max(0, idx - 24) : idx] + assert inline_fg not in window, "border frame inherited inline-code color" +``` + +- [ ] **Step 2: Run to verify it passes** + +Run: `uv run pytest tests/ui_and_conv/test_md_color_contract.py -v` +Expected: PASS. A FAIL means the frame really is bleeding the inline-code color — a genuine area-4 bug to surface. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_color_contract.py +git commit -m "test(tui): guard code-block border against inline-code color bleed" +``` + +--- + +### Task 5: Streaming table commits atomically — no stale partial table (spec area 3) + +**Files:** +- Create: `tests/ui_and_conv/test_md_stream_idempotency.py` + +- [ ] **Step 1: Write the contract test** + +```python +# tests/ui_and_conv/test_md_stream_idempotency.py +"""Streaming-boundary contract + idempotency/divergence hypotheses (H2, H3).""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.markdown import ( + PythinkerMarkdownStream, + markdown_commit_boundary, + pythinker_markdown, +) +from tests.ui_and_conv._md_contract_helpers import render_ansi, render_plain + + +def _drain(chunks: list[str]) -> list[str]: + """Feed chunks to the stream; return the ordered list of committed slices.""" + stream = PythinkerMarkdownStream() + committed: list[str] = [] + for chunk in chunks: + ready = stream.push(chunk) + if ready: + committed.append(ready) + tail = stream.flush() + if tail: + committed.append(tail) + return committed + + +def test_streaming_table_is_not_committed_mid_row(): + """Bug class: 'stale bordered tables left in scrollback while streaming'. + + A table streamed one line at a time must not have a partial (header-only or + header+delimiter-only) slice committed as a finished block: the committer + keeps the last top-level block mutable until a following block begins. + """ + full = "Intro paragraph.\n\n| A | B |\n| --- | --- |\n| 1 | 2 |\n\nAfter.\n" + # stream character-by-character to maximize the chance of a mid-table commit + committed = _drain(list(full)) + # No committed slice may end in the middle of the table (i.e. contain the + # delimiter row but not the closing blank line + following block). + for slice_ in committed[:-1]: + if "---" in slice_: + assert slice_.rstrip().endswith("|") is False or "After" in "".join(committed), ( + "a partial table row was committed before the table closed" + ) + # Reassembled stream equals the original (no loss, no duplication). + assert "".join(committed) == full +``` + +- [ ] **Step 2: Run to verify it passes** + +Run: `uv run pytest tests/ui_and_conv/test_md_stream_idempotency.py::test_streaming_table_is_not_committed_mid_row -v` +Expected: PASS. A FAIL is a real streaming-commit bug — surface it. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_stream_idempotency.py +git commit -m "test(tui): guard streamed tables against mid-row commit" +``` + +--- + +### Task 6: H2 (offset divergence) + H3 (idempotency) hypotheses + +**Files:** +- Modify: `tests/ui_and_conv/test_md_stream_idempotency.py` + +- [ ] **Step 1: Append the hypothesis tests** + +```python +# append to tests/ui_and_conv/test_md_stream_idempotency.py + +# Glued prose+table that forces the regex repair pipeline to fire on a slice +# whose commit boundary was computed on the RAW (un-repaired) text. +_GLUED = "Findings Medium| # | File |\n| --- | --- |\n| 1 | a.py |\n| 2 | b.py |\n\nNext.\n" + + +def test_h2_stream_slices_reassemble_without_duplicate_rows(): + """H2: commit offsets are computed on raw text while the renderer transforms + repaired text. Try to reproduce a duplicate/stale row. Expected: PASS + (non-reproduction). If this FAILS, H2 is confirmed — capture the case. + """ + committed = _drain(list(_GLUED)) + reassembled = "".join(committed) + assert reassembled == _GLUED + # Render each committed slice; 'a.py' and 'b.py' must each appear exactly + # once across the rendered stream (no row duplicated by the repair pass). + rendered = "".join(render_plain(pythinker_markdown(s)) for s in committed) + assert rendered.count("a.py") == 1 + assert rendered.count("b.py") == 1 + + +def test_h3_report_and_table_render_is_idempotent(): + """H3: rendering the same markdown twice yields byte-identical output.""" + md = "## Title\n\n| A | B |\n| --- | --- |\n| 1 | `x|y` |\n\nDone.\n" + first = render_ansi(pythinker_markdown(md), width=70) + second = render_ansi(pythinker_markdown(md), width=70) + assert first == second +``` + +- [ ] **Step 2: Run the hypotheses** + +Run: `uv run pytest tests/ui_and_conv/test_md_stream_idempotency.py -v` +Expected: PASS for both. **If `test_h2_...` FAILS**, H2 is reproduced: do NOT patch blindly — record the failing input in the spec's §12 R-notes and open a focused fix task. If it PASSES, annotate the spec: "H2 did not reproduce; kept as guard." + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_stream_idempotency.py +git commit -m "test(tui): add stream offset-divergence and idempotency guards" +``` + +--- + +### Task 7: Characterize the regex table-repair pipeline (pin, don't refactor) + +**Files:** +- Create: `tests/ui_and_conv/test_md_repair_characterization.py` + +- [ ] **Step 1: Write characterization tests (they pass on first run)** + +```python +# tests/ui_and_conv/test_md_repair_characterization.py +"""Characterization tests that PIN the existing regex Markdown-repair pipeline. + +These lock in current correct behavior of _repair_crammed_markdown_tables, +_normalize_markdown_tables, and the priority-matrix detector so any future +change that alters them is caught. Per the spec (§2), this pipeline is pinned, +NOT refactored. If a characterized output looks imperfect, mark it with a +`# pinned: imperfect` note and a follow-up — do not change source here. +""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.markdown import ( + _normalize_markdown_tables, + _repair_crammed_markdown_tables, + pythinker_markdown, +) +from tests.ui_and_conv._md_contract_helpers import render_plain + + +def test_glued_heading_and_table_header_is_split(): + """Model output that glues a section title to a table header gets split so + the table renders as a table, not crammed prose.""" + glued = "Medium| # | File |\n| --- | --- |\n| 1 | a.py |\n" + repaired = _repair_crammed_markdown_tables(glued) + # The heading is separated onto its own line before the table header. + assert repaired.splitlines()[0].strip() == "Medium" + out = render_plain(pythinker_markdown(glued), width=60) + assert "Medium" in out + assert "a.py" in out + assert "File" in out + + +def test_crammed_data_rows_on_delimiter_line_are_rechunked(): + """Data cells crammed onto the delimiter line are split into rows.""" + crammed = "| # | File |\n| --- | --- || 1 | a.py || 2 | b.py |\n" + normalized = _normalize_markdown_tables(crammed) + out = render_plain(pythinker_markdown(normalized), width=60) + assert "a.py" in out + assert "b.py" in out + + +def test_wellformed_table_is_passed_through_unchanged_in_render(): + """A clean table renders with both rows and the header intact.""" + clean = "| A | B |\n| --- | --- |\n| 1 | 2 |\n| 3 | 4 |\n" + out = render_plain(pythinker_markdown(clean), width=40) + for token in ("A", "B", "1", "2", "3", "4"): + assert token in out +``` + +- [ ] **Step 2: Run to verify they pass (pinning current behavior)** + +Run: `uv run pytest tests/ui_and_conv/test_md_repair_characterization.py -v` +Expected: PASS (3 tests). If one FAILS, your understanding of current behavior is wrong — read the source in `components/markdown.py` and adjust the *assertion* to match reality (this is characterization; the source is ground truth). + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_repair_characterization.py +git commit -m "test(tui): pin regex markdown table-repair behavior" +``` + +--- + +### Task 8: Report rendering grounded in the real 92-finding fixture (spec area 3) + +**Files:** +- Create: `tests/ui_and_conv/test_report_realdata.py` +- Read-only fixture: `security-scan-findings.json` (repo root) + +- [ ] **Step 1: Write the raw→Report transform + render contract test** + +```python +# tests/ui_and_conv/test_report_realdata.py +"""Report rendering grounded in the real security-scan-findings.json fixture. + +The fixture is RAW scanner shape (filePath / severity UPPERCASE / vulnSlug / +title / description / lineNumbers / recommendation / confidence). report.py +consumes the Report shape (title / severity lowercase / location / body). The +transform below encodes the contract: case-fold severity, fold filePath + +lineNumbers into location, fold description + recommendation into body. If a +production transform exists (see Task 12), Task 12 asserts they agree. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from pythinker_code.ui.shell.components.report import ( + Report, + ReportFinding, + Severity, + render_report, +) +from tests.ui_and_conv._md_contract_helpers import THEMES, WIDTHS, render_plain + +_FIXTURE = Path(__file__).resolve().parents[2] / "security-scan-findings.json" +_VALID_SEVERITIES = {"critical", "high", "medium", "low", "info"} + + +def _location(raw: dict) -> str | None: + path = raw.get("filePath") + if not isinstance(path, str) or not path: + return None + lines = raw.get("lineNumbers") or [] + if isinstance(lines, list) and lines: + return f"{path}:{lines[0]}" + return path + + +def _body(raw: dict) -> str: + parts = [] + if raw.get("description"): + parts.append(str(raw["description"])) + if raw.get("recommendation"): + parts.append(f"**Fix:** {raw['recommendation']}") + return "\n\n".join(parts) + + +def _to_finding(raw: dict) -> ReportFinding: + severity = str(raw["severity"]).lower() + assert severity in _VALID_SEVERITIES, f"unexpected severity {raw['severity']!r}" + return ReportFinding( + title=str(raw["title"]), + severity=severity, # type: ignore[arg-type] + location=_location(raw), + body=_body(raw), + ) + + +def _load_report(limit: int | None = None) -> Report: + raw = json.loads(_FIXTURE.read_text()) + findings = tuple(_to_finding(r) for r in (raw[:limit] if limit else raw)) + return Report(title="Security Scan", scope=f"{len(findings)} findings", findings=findings) + + +def test_fixture_transforms_to_valid_report(): + report = _load_report() + assert len(report.findings) == 92 + # Every transformed severity is a valid Report severity. + seen: set[Severity] = {f.severity for f in report.findings} + assert seen <= _VALID_SEVERITIES + assert "critical" in seen # the fixture contains CRITICAL findings + + +@pytest.mark.parametrize("theme", THEMES) +@pytest.mark.parametrize("width", WIDTHS) +def test_real_report_renders_across_theme_and_width(theme, width): + out = render_plain(render_report(_load_report(limit=12), theme=theme), width=width) + assert "Security Scan" in out + # The summary tally line names at least one present severity. + assert any(sev in out for sev in ("critical", "high", "medium", "low", "info")) + + +def test_real_report_shows_locations_and_titles(): + out = render_plain(render_report(_load_report(limit=5)), width=100) + report = _load_report(limit=5) + for finding in report.findings: + assert finding.title[:20] in out + if finding.location: + # the file path portion of the first finding's location appears + assert finding.location.split(":")[0].split("/")[-1] in out +``` + +- [ ] **Step 2: Run to verify it passes** + +Run: `uv run pytest tests/ui_and_conv/test_report_realdata.py -v` +Expected: PASS. If `test_fixture_transforms_to_valid_report` FAILS on an unexpected severity, the fixture contains a value outside the five-severity set — extend `_VALID_SEVERITIES` mapping only if the production transform does the same; otherwise surface it. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_report_realdata.py +git commit -m "test(tui): render reports from the real security-scan fixture" +``` + +--- + +### Task 9: H1 — nested report-fence must not be promoted (THE fix) + +**Files:** +- Create: `tests/ui_and_conv/test_report_fence_nesting.py` +- Modify: `src/pythinker_code/ui/shell/components/report.py` + +- [ ] **Step 1: Write the failing hypothesis test** + +```python +# tests/ui_and_conv/test_report_fence_nesting.py +"""H1: a ```report block shown INSIDE an outer documentation fence must not be +promoted to a report. The flat _REPORT_FENCE_RE regex cannot see fence nesting; +an AST walk over top-level fence tokens structurally can. +""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.report import has_report_block, render_agent_body +from tests.ui_and_conv._md_contract_helpers import render_plain + +# A 4-backtick outer fence whose body is a literal ```report example. markdown-it +# parses the outer fence as ONE token, so the inner block is documentation text, +# not a real report. +_NESTED = ( + "Here is how to emit a report:\n\n" + "````markdown\n" + "```report\n" + '{"title": "Example", "findings": [{"title": "x", "severity": "high"}]}\n' + "```\n" + "````\n" +) + + +def test_nested_report_fence_is_not_detected(): + assert has_report_block(_NESTED) is False + + +def test_nested_report_fence_renders_as_documentation_not_report(): + out = render_plain(render_agent_body(_NESTED)) + # The inner block stays verbatim documentation; it is NOT promoted to the + # report renderer (which would drop the JSON and print a tally). + assert '"title": "Example"' in out + assert "1 high" not in out # no report tally emitted + + +def test_top_level_report_fence_still_promoted(): + """Regression guard: the real top-level case must keep working.""" + text = ( + "Intro.\n\n```report\n" + '{"title": "Real", "findings": [{"title": "bug", "severity": "medium"}]}\n' + "```\n" + ) + out = render_plain(render_agent_body(text)) + assert "Real" in out + assert "1 medium" in out + assert '"severity"' not in out # rendered as a report, not raw JSON +``` + +- [ ] **Step 2: Run to verify the nesting tests FAIL** + +Run: `uv run pytest tests/ui_and_conv/test_report_fence_nesting.py -v` +Expected: `test_nested_report_fence_is_not_detected` and `test_nested_report_fence_renders_as_documentation_not_report` **FAIL** (the flat regex promotes the inner block). `test_top_level_report_fence_still_promoted` PASSES. + +- [ ] **Step 3: Replace flat-regex extraction with an AST walk in `report.py`** + +Edit `src/pythinker_code/ui/shell/components/report.py`. + +3a. Add a lazy markdown-it parser and a top-level report-fence iterator near the other helpers (after `_DOT = "●"`): + +```python +# A markdown-it parser is reused so report-fence extraction is fence-aware: a +# ```report block nested inside an outer fence is part of that outer fence's +# content and is therefore NOT a top-level fence token (Principle #5: parse, +# don't pattern-match). +_md_parser: Any = None + + +def _get_report_parser() -> Any: + global _md_parser + if _md_parser is None: + from markdown_it import MarkdownIt + + _md_parser = MarkdownIt() + return _md_parser + + +def _iter_report_payloads(text: str) -> list[tuple[int, int, str]]: + """Yield (start_line, end_line, payload) for each TOP-LEVEL ```report fence. + + Line indices are 0-based half-open ([start, end)) into ``text``'s lines, + matching markdown-it ``token.map``. Nested fences never appear as top-level + ``fence`` tokens, so they are structurally excluded. + """ + md = _get_report_parser() + blocks: list[tuple[int, int, str]] = [] + for token in md.parse(text): + if ( + token.type == "fence" + and token.level == 0 + and token.map is not None + and token.info.strip() == "report" + ): + blocks.append((token.map[0], token.map[1], token.content)) + return blocks +``` + +3b. Rewrite `has_report_block` to use the AST iterator: + +```python +def has_report_block(text: str) -> bool: + """Whether *text* contains at least one well-formed top-level ` ```report ` block.""" + return any(parse_report_block(payload) is not None for _, _, payload in _iter_report_payloads(text)) +``` + +3c. Rewrite `render_agent_body` to slice by line map instead of regex cursor: + +```python +def render_agent_body(text: str, *, theme: ThemeName | None = None) -> RenderableType: + """Render assistant text, promoting top-level ` ```report ` blocks to reports. + + Non-report text renders via :func:`pythinker_markdown`; a valid top-level + report block renders via :func:`render_report`; an invalid or nested block is + left in place so the surrounding markdown shows it as an ordinary code block. + """ + # Split on "\n" only: markdown-it's token.map counts only "\n", so + # splitlines() can desync fence delimiter indices on other line separators. + lines = text.split("\n") + segments: list[RenderableType] = [] + cursor = 0 # line index + for start, end, payload in _iter_report_payloads(text): + report = parse_report_block(payload) + if report is None: + continue # malformed — leave it for the markdown renderer + before = "\n".join(lines[cursor:start]).strip("\n") + if before: + segments.append(pythinker_markdown(before)) + segments.append(render_report(report, theme=theme)) + cursor = end + + if not segments: + return pythinker_markdown(text) + + rest = "\n".join(lines[cursor:]).strip("\n") + if rest: + segments.append(pythinker_markdown(rest)) + + spaced: list[RenderableType] = [] + for i, segment in enumerate(segments): + if i: + spaced.append(Text("")) + spaced.append(segment) + return Group(*spaced) +``` + +3d. Delete the now-unused `_REPORT_FENCE_RE` regex and its `import re` if `re` is unused elsewhere in the file. Check first: + +Run: `grep -n "re\\.\|_REPORT_FENCE_RE\|^import re" src/pythinker_code/ui/shell/components/report.py` +- Remove the `_REPORT_FENCE_RE = re.compile(...)` block. +- Remove `import re` only if `grep` shows no other `re.` usage. + +- [ ] **Step 4: Run the full report suite to verify the fix and no regressions** + +Run: `uv run pytest tests/ui_and_conv/test_report_fence_nesting.py tests/ui_and_conv/test_report.py -v` +Expected: ALL PASS — the two nesting tests now pass, and every pre-existing test in `test_report.py` (including `test_render_agent_body_promotes_report_fence`, `test_render_agent_body_invalid_fence_falls_back_to_markdown`, and `test_streaming_commit_keeps_report_fence_atomic_and_renders`) still passes. + +- [ ] **Step 5: Type-check the modified file** + +Run: `uv run pyright src/pythinker_code/ui/shell/components/report.py` +Expected: no new errors. (`token`/parser are typed `Any`; that is intentional for the untyped markdown-it surface, consistent with `components/markdown.py`.) + +- [ ] **Step 6: Commit** + +```bash +git add tests/ui_and_conv/test_report_fence_nesting.py src/pythinker_code/ui/shell/components/report.py +git commit -m "fix(tui): extract report fences via AST so nested blocks aren't promoted" +``` + +--- + +### Task 10: Screen-authority guard for the lead-phase modules (spec principle 1) + +**Files:** +- Create: `tests/ui_and_conv/test_md_render_authority.py` + +- [ ] **Step 1: Write the static guard test** + +```python +# tests/ui_and_conv/test_md_render_authority.py +"""Screen-authority discipline (spec principle 1) for the lead-phase modules. + +These renderers must return Rich renderables, never write to the terminal +directly. A bare print()/sys.stdout.write in a renderer bypasses the Live +screen model and causes the duplicate-scrollback / corruption bug classes. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_SRC = Path(__file__).resolve().parents[2] / "src" / "pythinker_code" / "ui" / "shell" / "components" +_GUARDED = ["report.py", "markdown.py"] + + +@pytest.mark.parametrize("filename", _GUARDED) +def test_no_direct_terminal_writes_in_renderer(filename): + tree = ast.parse((_SRC / filename).read_text()) + offenders: list[str] = [] + for node in ast.walk(tree): + if isinstance(node, ast.Call): + func = node.func + if isinstance(func, ast.Name) and func.id == "print": + offenders.append(f"print() at line {node.lineno}") + if ( + isinstance(func, ast.Attribute) + and func.attr == "write" + and isinstance(func.value, ast.Attribute) + and func.value.attr in {"stdout", "stderr"} + ): + offenders.append(f"std*.write at line {node.lineno}") + assert not offenders, f"{filename} bypasses the screen model: {offenders}" +``` + +- [ ] **Step 2: Run to verify it passes** + +Run: `uv run pytest tests/ui_and_conv/test_md_render_authority.py -v` +Expected: PASS (2 tests). A FAIL means a renderer writes to the terminal directly — surface it. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_md_render_authority.py +git commit -m "test(tui): forbid direct terminal writes in markdown/report renderers" +``` + +--- + +### Task 11: Bug-class → test registry doc + full-suite gate + +**Files:** +- Create: `tests/ui_and_conv/README_contract_registry.md` + +- [ ] **Step 1: Write the 1:1 registry mapping (spec §8 lead-phase rows)** + +```markdown +# Markdown + Report contract test registry (lead phase) + +Maps spec bug classes to the test that guards them. Tier 1 = deterministic +capture. See docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md §8. + +| Spec area | Bug class | Test | Tier | +|---|---|---|---| +| 3 Markdown | table breaks on piped inline code | test_md_table_contract::test_table_with_piped_inline_code_keeps_columns | T1 | +| 3 Markdown | escaped pipe splits cell | test_md_table_contract::test_table_with_escaped_pipes_keeps_literal_pipe | T1 | +| 3 Markdown | empty header mislabeled | test_md_table_contract::test_table_empty_header_cell_does_not_mislabel | T1 | +| 3 Markdown | long cell wrap loss | test_md_table_contract::test_table_long_cell_wraps_without_dropping_content | T1 | +| 3 Markdown | stale streamed table | test_md_stream_idempotency::test_streaming_table_is_not_committed_mid_row | T1 | +| 3 Markdown | nested report-fence promoted | test_report_fence_nesting::* | T1 | +| 4 ANSI | border inherits code-span color | test_md_color_contract::test_code_block_border_does_not_use_inline_code_color | T1 | +| 3 Markdown | report render on real data | test_report_realdata::* | T1 | +| 1 Stability | render idempotency | test_md_stream_idempotency::test_h3_report_and_table_render_is_idempotent | T1 | +| 1 Stability | offset divergence (H2) | test_md_stream_idempotency::test_h2_stream_slices_reassemble_without_duplicate_rows | T1 | +| 1 Stability | screen-authority | test_md_render_authority::test_no_direct_terminal_writes_in_renderer | T1 | +| repair | regex pipeline pinned | test_md_repair_characterization::* | T1 | +``` + +- [ ] **Step 2: Run the full lead-phase suite + existing UI suite (no regressions)** + +Run: `uv run pytest tests/ui_and_conv -v` +Expected: all green, including the pre-existing tests. Capture the summary line (e.g. `N passed`). + +- [ ] **Step 3: Run lint + type-check on changed source** + +Run: `uv run ruff check src/pythinker_code/ui/shell/components/report.py tests/ui_and_conv && uv run pyright src/pythinker_code/ui/shell/components/report.py` +Expected: clean. + +- [ ] **Step 4: Commit** + +```bash +git add tests/ui_and_conv/README_contract_registry.md +git commit -m "docs(tui): add markdown/report contract test registry" +``` + +--- + +### Task 12: Reconcile with the production raw→Report transform (investigation) + +**Files:** +- Possibly modify: `tests/ui_and_conv/test_report_realdata.py` + +- [ ] **Step 1: Locate the production transform** + +Run: +```bash +grep -rn "ReportFinding\|Report(" src/pythinker_code/cli/security_scan.py src/pythinker_code/cli/secscan.py packages/pythinker-review 2>/dev/null | grep -v test +grep -rn "lineNumbers\|vulnSlug\|filePath" src packages/pythinker-review 2>/dev/null | grep -iv test | head +``` + +- [ ] **Step 2: Decide and act (one of two concrete outcomes)** + +- **If a production transform exists** (raw findings → `Report` or → ` ```report ` JSON): add ONE test to `test_report_realdata.py` that feeds the fixture through the production transform and asserts its `severity`/`location`/`body` for finding[0] match the test-local `_to_finding(raw[0])` output. This proves the test-local adapter matches production. Show the exact import and assertion once located. +- **If no production transform exists in this repo** (it lives in the external scanner that emits ` ```report ` JSON directly): add a one-line comment at the top of `test_report_realdata.py` recording that the transform is external and the test-local adapter is the documented contract. No code change beyond the comment. + +This task is bounded: it ends in either a single reconciliation test or a single documenting comment. Do not expand scope. + +- [ ] **Step 3: Commit** + +```bash +git add tests/ui_and_conv/test_report_realdata.py +git commit -m "test(tui): reconcile report fixture with production transform" +``` + +--- + +## Self-Review + +**1. Spec coverage (§6–§8 lead phase):** +- §6.1 characterization → Task 7 ✓ +- §6.2 table bug classes → Tasks 2, 3, 4, 5 ✓ +- §6.3 report on real data + raw→Report transform → Tasks 8, 12 ✓ +- §6.4 H1 → Task 9 ✓; H2 → Task 6 ✓; H3 → Task 6 ✓ +- §6.5 display-vs-copy → **intentionally deferred** (no `/copy` exists; guard documented in spec, not built — out of scope per spec non-goals) ✓ +- §7 idempotency harness → Task 1 (`render_twice_identical`) + Task 6 ✓; width-boundary harness → Task 1 (`WIDTHS`) + Task 3 ✓; theme harness → Task 1 (`THEMES`) + Task 8 ✓ +- §5.1 screen-authority → Task 10 ✓ +- §8 registry → Task 11 ✓ + +**2. Placeholder scan:** No "TBD/TODO/handle edge cases". Task 12 is a bounded investigation with two concrete, enumerated outcomes (not an open placeholder). Every code step shows complete code. + +**3. Type/name consistency:** helper names (`render_plain`, `render_ansi`, `render_twice_identical`, `WIDTHS`, `THEMES`) defined in Task 1 are used verbatim in Tasks 2–8. New `report.py` symbols (`_get_report_parser`, `_iter_report_payloads`) are defined in Task 9 Step 3a and used in 3b/3c. `_to_finding`/`_load_report` defined and used within Task 8/referenced in Task 12. + +**Note on exact-full-width:** Task 1's `WIDTHS` covers narrow/normal; the exact-full-width stray-space case (spec area 5) is roadmap seq 2, not lead phase. Flagged here so it is not silently considered covered. + +--- + +## Execution Handoff + +Per the approved scope, the deliverable is **the plan + spec only** — do not begin executing without an explicit greenlight. When greenlit, two options: + +1. **Subagent-Driven (recommended)** — dispatch a fresh subagent per task, review between tasks (REQUIRED SUB-SKILL: `superpowers:subagent-driven-development`). +2. **Inline Execution** — execute tasks in-session with checkpoints (REQUIRED SUB-SKILL: `superpowers:executing-plans`). diff --git a/docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md b/docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md new file mode 100644 index 00000000..603deeb1 --- /dev/null +++ b/docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md @@ -0,0 +1,211 @@ +# TUI Renderer Contract-Hardening — Design + +- **Date:** 2026-05-29 +- **Status:** Approved design — implementation plan pending (`writing-plans`) +- **Author:** Mohamed Elkholy +- **Scope:** Harden the existing Pythinker terminal UI against a known catalog of rendering bug classes, **leading with Markdown + security/code-scan report rendering**. +- **Builds on (prior art — do not contradict):** + - `2026-05-28-standardized-report-renderer-design.md` (origin of `components/report.py`) + - `2026-05-07-readable-terminal-reports-design.md` + - `2026-05-21-tui-spacing-design.md` + - `2026-05-28-pi-tui-engine-port-plan.md`, `2026-05-22-blackbox-src-tui-port-design.md` + +--- + +## 1. Context & problem statement + +We received a comprehensive spec ("Design & Build an Enhanced Terminal UI / CLI / TUI Renderer") describing a full-screen renderer hardened against ~13 subsystems' worth of bug classes harvested from a real changelog. The spec's literal instructions (Principle #1; Implementation steps 1–4) call for building a **cell-based virtual screen buffer, deterministic differ, display-width library, and ANSI normalizer from scratch**. + +Pythinker Code (v0.25.0) already obtains **all** of those primitives from two mature libraries it depends on: + +| Primitive the spec asks us to build | Already provided by | +|---|---| +| Virtual screen buffer of cells | **Rich** `Segment`/`Console`/`Live` | +| Deterministic diffing, idempotent frames | **Rich** `Live` | +| Display-width (CJK/emoji/combining) | **Rich** `cell_len` (its own Unicode East-Asian width tables; no `wcwidth` dep) | +| ANSI span normalization, fg/bg reset | **Rich** `Style`/`Segment` | +| Color-depth & OSC 8 capability detection | **Rich** `Console` | +| Alternate screen, cursor/keyboard restore | **prompt_toolkit** `Application` | +| Input buffer, IME, bracketed paste, key bindings | **prompt_toolkit** `Buffer`/`KeyBindings` | + +Building those from scratch would reimplement Rich/Textual/prompt_toolkit, violate the project's **zero-new-bundled-dependencies** and **simplicity-first** constraints, and *introduce* the very regressions the spec exists to prevent. + +The relevant rendering code already exists and is substantial: `utils/rich/markdown.py` (949 LOC), `ui/shell/components/markdown.py` (643), `ui/shell/components/report.py` (275), `ui/shell/visualize/_live_view.py` (1,301), `ui/shell/prompt.py` (3,257), with snapshot tests in `tests/ui_and_conv/` using Rich `Console.capture()` + `inline-snapshot`. + +## 2. The reframe (the core decision) + +**The spec is a behavioral contract, not an implementation directive.** Its statements split into two kinds, treated differently: + +- **Behavioral requirements** (idempotent frames, correct CJK width, tables survive piped inline code, keyboard mode resets on every exit path) — these **bind**, expressed as automated tests. +- **Implementation directives** ("build a cell buffer", "no regex-only Markdown", "no string-prefix shell logic") — these bind **new code only**. Existing code that *passes the behavioral contract* is **pinned, not refactored**. + +Consequence: the win condition is an **aggressive, deterministic, bug-class-mapped test suite**. If Rich + prompt_toolkit (plus the existing Pythinker layer) pass it, we have met the intent of the spec without writing fragile new rendering logic. We write code **only where a contract test fails.** + +This reconciliation specifically governs the existing **regex-based Markdown table-repair pipeline** in `components/markdown.py`. "No regex-only Markdown" is an implementation directive; it does not license rewriting load-bearing repairs (`_repair_crammed_markdown_tables`, `_normalize_markdown_tables`, `_normalize_table_block`, the priority-matrix detector) that already handle real malformed streaming output. That pipeline is the highest-**value** surface to **pin** (characterization tests), not the highest-priority surface to **refactor**. + +## 3. Goals / non-goals + +### Goals +1. A regression-test suite mapped **1:1** to the spec's bug classes, tagged by test tier. +2. **Deep** hardening of Markdown + security/scan **report** rendering (the lead phase), grounded in real data. +3. Two thin architectural disciplines (screen-authority, state-isolation) layered on the existing stack. +4. A **shallow, sequenced roadmap** for the other 11 subsystems — enough to execute later, not full sub-specs. +5. A capability-matrix doc + a `/terminal-setup` stub for the emulator long tail. + +### Non-goals +- No new renderer / screen buffer / differ / width engine. +- **No new bundled runtime dependencies** (hard project constraint). +- **No refactor of passing regex repairs.** +- No `/copy` Markdown→clipboard feature (does not exist today; see §6.5) unless separately requested. +- Tier-3 emulator-specific corruption is **not** mechanically tested (guarded by invariant + matrix doc). +- The other 11 areas are designed only to roadmap depth in this document. + +## 4. Foundational principles → existing stack (contract mapping) + +| Spec principle | Satisfied by | What *we* add (the contract) | +|---|---|---| +| 1. One authoritative screen model | Rich `Console`/`Live` | **Screen-authority discipline** (§5.1): no ad-hoc writes in the live region | +| 2. Deterministic diff + idempotent frames | Rich `Live` | **Idempotency harness** (§7): render twice → identical segments | +| 3. Self-healing repaint | pt `Application` invalidate / Rich `Live` refresh | Tier-2 pty tests for resize/refocus/attach | +| 4. Unicode-correct layout | Rich `cell_len` (Unicode width tables) | **Width-boundary harness** (§7): exact-full-width + CJK | +| 5. Parse, don't pattern-match | markdown-it-py AST (render path) + structured `parse_report_block` | Fence-aware report extraction (H1, §6.4); shell parser audit (roadmap) | +| 6. State isolation | separate modules today | **State-isolation map** (§5.2) pinned by mutation tests | +| 7. Reset modes reliably | pt exit handling | Tier-2 "every exit path resets keyboard mode" test | +| 8. Capability detection | Rich/pt detection | Capability-matrix doc + `/terminal-setup` stub (roadmap) | + +## 5. Architecture — two thin disciplines, zero new renderer + +### 5.1 Screen-authority discipline +All live-region output flows only through the existing `LiveView`/`Console`. No ad-hoc `print()` or raw escape writes in the live region. Enforced by a **guard test** (grep/AST check over the live-region modules) rather than a new abstraction. Static/committed scrollback continues to use Rich renderables. + +### 5.2 State-isolation map (Principle #6 onto real modules) +| State owner | Module | Isolation contract (test) | +|---|---|---| +| Prompt input buffer | `ui/shell/prompt.py` (pt `Buffer`) | Async task/status updates never mutate the input buffer | +| Streaming Markdown committer | `PythinkerMarkdownStream` (`components/markdown.py`) | `push()`/`flush()` own `pending`; committed slices are immutable | +| Report state | `components/report.py` (`Report` frozen dataclass) | `parse_report_block` is pure; malformed input never mutates prior output | +| Paste payloads | `prompt.py` paste handlers + `utils/clipboard` | Paste normalization isolated from history append | +| Background sessions | `background/` | Foreground prompt state never reads/writes session state directly | + +Each row becomes a test that mutates one owner and asserts the others are byte-identical. + +### 5.3 Capability layer +Document (not re-implement) what Rich detects (color depth: truecolor/256/16/no-color; OSC 8 hyperlinks) and what prompt_toolkit detects (keyboard protocol, bracketed paste, clipboard mechanism). Add a **capability-matrix doc** and a `/terminal-setup` **stub** that surfaces known-problematic environments (VS Code/Cursor/Windsurf GPU acceleration, etc.). The stub is roadmap, not lead phase. + +## 6. Lead Phase — Markdown + Report rendering (DEEP) + +**Targets:** `utils/rich/markdown.py`, `ui/shell/components/markdown.py`, `ui/shell/components/report.py`. +**Method:** strict **pin → fail → minimal-fix**. All Tier 1 (deterministic `Console.capture()` + `inline-snapshot`). + +### 6.1 Characterization snapshots (pin current correct behavior) +Snapshot the regex repair pipeline on real malformed-stream inputs *before* any change: +- glued prose+table header (`Medium| # | File |`) → `_repair_crammed_markdown_tables` +- dropped header/delimiter newline and crammed data rows → `_normalize_table_block` +- priority-matrix code blocks → `_render_priority_matrix` +- report-icon simplification outside fences → `_simplify_markdown_report_icons` + +These lock in behavior so any later change that alters them is caught. + +### 6.2 Table bug-class contract tests (Tier 1) +Parametrized over widths (incl. exactly-full-width and narrow) and both themes: +- table with **piped inline code** (`` `a|b` ``) and **escaped pipes** (`\|`) +- **empty header** cells; **very long** cells (wrap + reflow) +- **border color** must not inherit code-span color (color-bleed guard) +- **wrapped continuation** lines preserve inline style +- **stale bordered table** must not remain in scrollback while streaming + +### 6.3 Report rendering grounded in real data +`security-scan-findings.json` holds **92 real findings** in raw scanner shape: +`filePath`, `severity` (UPPERCASE), `vulnSlug`, `title`, `description`, `lineNumbers`, `recommendation`, `confidence`, `producedByRunId`. + +This is **not** the `Report`/`ReportFinding` shape (`title`, `severity` lowercase, `location`, `body`) that `parse_report_block` consumes. Therefore: +1. **Pin the raw→Report transform** (case-fold `CRITICAL`→`critical`; `filePath`+`lineNumbers`→`location`; `description`/`recommendation`→`body`). Locate or, if absent, specify it in the plan. +2. Snapshot `render_report` over the transformed fixture across **all five severities**, **both themes**, and **several widths**. +3. Snapshot `render_agent_body` promoting a ` ```report ` block embedded in surrounding Markdown. + +### 6.4 Three hypotheses — written as **failing tests first**, not asserted as defects +- **H1 (looks real):** `_REPORT_FENCE_RE` in `report.py` is a flat regex that does not track outer fence nesting; a ` ```report ` block shown *inside* a documentation ` ``` ` fence may be wrongly promoted to a report. **Test proves it; fix = fence-aware extraction reusing the committer's markdown-it parser** (Principle #5 binds this *new* code). +- **H2 (may not reproduce):** the streaming committer computes commit offsets on **raw** text (`markdown_commit_boundary`) while the renderer transforms **repaired** text; line-count changes could desync → duplicate/stale rows. **Write the reproduction test; if it cannot reproduce, record that and move on** (no speculative fix). +- **H3:** render-same-state-twice **idempotency** for report + repaired-table interaction (identical segments on re-render). + +### 6.5 Display-vs-copy contract (guard, not feature) +**Verified:** there is no `/copy` Markdown→clipboard command today. Clipboard handling is paste-only (`prompt_toolkit` + `pyperclip` + media grab); the only "copy" is `/fork` (session history). So the spec's "separate render paths for display vs. clipboard" and "/copy column misalignment" become a **guard for when such a path is added**: +- a separate `markdown_to_plain()` render path with its own snapshot; +- a test asserting **no ANSI/borders/OSC-8 escapes** leak into copied bytes and **no trailing whitespace** in streamed copy output. + +Building the actual `/copy` feature is out of scope unless requested. + +## 7. Regression-suite design + +- **Location:** `tests/ui_and_conv/`. **Idioms:** Rich `Console.capture()` + `inline-snapshot` (existing), `pexpect`/pty for Tier 2. +- **One docstring-tagged test per spec bug class**, plus a **bug-class → test-id → tier registry** (§8) — the 1:1 mapping the spec demands. +- **Idempotency harness:** `render(state)` twice → assert identical segment streams. Structurally kills duplicate-scrollback / progressive style-degradation for Tier 1. +- **Width-boundary harness:** parametrized widths incl. exactly-full-width and CJK; assert no overflow via `cell_len`. +- **Theme harness:** every report/markdown snapshot runs against dark *and* light to catch contrast/color-bleed. + +## 8. Bug-class → test registry (1:1 mapping, by tier) + +> Lead-phase rows (areas 3–5) are specified in detail above. The full catalog is enumerated here so every bug class has an owning test id and a tier. `T1` = deterministic capture; `T2` = pty; `T3` = invariant + manual matrix. + +| Spec area | Representative bug class | Test id (planned) | Tier | +|---|---|---|---| +| 1 Renderer stability | duplicate scrollback rows; style-pool leak; color bleed | `test_idempotent_frame`, `test_style_pool_stable_longsession` | T1 / T2 | +| 2 Fullscreen/alt-screen | leftover content after exit; literal markers in-progress; dialog submit-underneath | `test_altscreen_restored_on_cancel` | T2 | +| 3 **Markdown/rich text** | table breaks on piped inline code; border inherits code color; wrapped line loses style; stale streaming table; link→plain | `test_table_piped_code`, `test_border_no_codecolor_bleed`, `test_wrap_keeps_style`, `test_stale_table_streaming`, `test_osc8_link_fallback` | **T1** | +| 4 ANSI/themes/contrast | wrong-position colors; 256-bg bleed on attach; unreadable on theme mismatch; spinner color churn | `test_ansi_span_normalize`, `test_theme_contrast_both_bg` | T1 / T3 | +| 5 Wrapping/resize/width | stray leading space at exact width; CJK overflow/ghosts; spinner freeze after resize | `test_exact_full_width_no_stray_space`, `test_cjk_no_overflow` | T1 / T2 | +| 6 Scrolling/navigation | scroll breaks in attached session; offset reset after deletion | `test_scroll_offset_preserved` | T2 | +| 7 Prompt input/keyboard | typing lag large prompt; duplicate history; keyboard mode not reset on exit | `test_keyboard_mode_reset_all_exits`, `test_history_append_once` | T2 | +| 8 Paste/clipboard/images | duplicated right-click paste; lost via stash/replay; bad-image crash | `test_paste_paths_isolated`, `test_bad_image_placeholder` | T2 | +| 9 Shell/Bash/PowerShell | string-prefix permission gap; crash on malformed syntax; orphaned pty | `test_shell_permission_structured`, `test_pty_cleanup_on_eof` | T2 | +| 10 CLI/discovery | duplicate slash commands; trailing-tab cmd treated unknown; headless silent-fail | `test_slash_dedup`, `test_headless_reports_invalid_cmd` | T1 / T2 | +| 11 Background sessions | stuck blocked/running; doubled list rows; no repaint on attach | `test_session_attach_repaint` | T2 | +| 12 Spinner/progress/status | stale amber across tool calls; token counter zero; frozen elapsed time | `test_spinner_phase_derived`, `test_progress_reserved_area` | T1 / T2 | +| 13 IDE/emulator | VS Code spinner-count corruption; emulator keyboard/clipboard quirks | capability-matrix doc; `/terminal-setup` | **T3** | + +## 9. Roadmap for the other 11 areas (shallow — sequenced, not full designs) + +Each area gets **one row**, not a sub-spec. Sequence = recommended build order after the lead phase. + +| Seq | Area | Lead guard strategy | Tier | Notes | +|---|---|---|---|---| +| 1 | Renderer stability / idempotency | Idempotency + long-session style-pool harness | T1/T2 | Cheap, high value; partly covered by lead phase | +| 2 | Wrapping/resize/width | Width-boundary harness (exact-full-width, CJK) | T1/T2 | Reuses lead-phase harness | +| 3 | ANSI/themes/contrast | Span-normalize + both-bg contrast snapshots | T1/T3 | Reuses theme harness | +| 4 | Prompt input/keyboard | pty: keyboard-mode-reset on every exit; history append-once | T2 | Highest user-visible risk | +| 5 | Paste/clipboard/images | pty: isolate paste paths; bad-image placeholder | T2 | | +| 6 | Scrolling/navigation | pty: offset/selection preserved across mutation | T2 | | +| 7 | Fullscreen/alt-screen | pty: alt-screen restored on cancel/error | T2 | | +| 8 | Background sessions | pty: attach idempotent + full repaint | T2 | | +| 9 | Spinner/progress/status | phase-derived spinner; reserved progress area | T1/T2 | | +| 10 | Shell/Bash/PowerShell parsing | audit permission analysis for structured parsing | T2 | Security-adjacent; flag for explicit review | +| 11 | IDE/emulator matrix + `/terminal-setup` | capability-matrix doc + setup flow | T3 | Not mechanically testable | + +## 10. Deliverables + +1. This design doc. +2. Bug-class → test-id → tier **registry** (§8) maintained alongside the suite. +3. Lead-phase **Tier-1 contract suite** (characterization + bug-class + report-on-real-data), on greenlight. +4. Capability-matrix doc + `/terminal-setup` **stub**. +5. Shallow roadmap (§9) for the remaining areas. + +## 11. Acceptance gate (maps to the spec's Required Test Matrix) + +- **Lead phase done when:** all §6 Tier-1 tests pass; the three hypotheses (§6.4) are each resolved (fixed-with-test, or recorded-as-non-reproducing); idempotency + width-boundary + theme harnesses are green; the raw→Report transform is pinned against the real 92-finding fixture. +- **Long-session streaming**, **layout boundaries**, **Markdown stress** rows of the spec matrix are covered by Tier 1 here. **Input / Shell / Paste / Emulator** rows are explicitly deferred to the roadmap with their tiers, so coverage is never silently over-claimed. + +## 12. Risks & open questions + +- **R1 — characterization brittleness:** pinning the regex pipeline locks current behavior, including any current *wrong* output. Mitigation: review each characterization snapshot at creation; mark known-imperfect ones with a `# pinned: imperfect` note and a follow-up test. +- **R2 — raw→Report transform location:** RESOLVED during execution — **no in-repo transform exists**; the external scanner emits canonical ` ```report ` JSON directly, and `parse_report_block` rejects any non-canonical severity. So the test-local adapter in `test_report_realdata.py` *is* the documented contract. The raw fixture (`security-scan-findings.json`, 92 findings) carries scanner-native severities including two outside the canonical five — `BUG` (4) and `HIGH_BUG` (3); the adapter folds them (`HIGH_BUG→high`, `BUG→medium`) with a documented map. +- **R3 — H2 may not reproduce:** acceptable; the methodology records non-reproduction rather than inventing a fix. **Execution result: H2 did NOT reproduce** (the glued prose+table case reassembles with each row rendered exactly once); kept as a guard. H3 (idempotency) also passed. +- **OQ1:** Should `/copy` actually be built (turns §6.5 from guard into feature)? Default: no. +- **OQ2:** Tier-2 pty harness — adopt `pexpect` (test-only dev dep) or extend existing `tests_e2e` subprocess approach? Lead phase is Tier 1, so this is deferred to roadmap seq 4. + +### Deferred defects (found during execution — tracked, not fixed in lead phase) + +- **D1 — narrow stacked-table mid-word fold + missing continuation indent.** Location: `src/pythinker_code/utils/rich/markdown.py` → `TableElement` stacked-record path (triggered for ≥4 columns or any cell >48 chars). At viewport widths < ~40 a long cell value folds **mid-word** (`theta` → `t\nheta`) and continuation lines lose the leading ` ` indent. **No data loss** — every character survives, it only looks ragged; `test_table_long_cell_wraps_without_dropping_content` guards the data-integrity contract (whitespace-insensitive survival), not the cosmetics. Deferred fix: word-wrap the cell value with a hanging/`subsequent_indent` (e.g. `rich.text.Text.wrap` or a `textwrap` pass) in the cell-emission loop. Roadmap polish, not lead phase. + +## 13. Working method + +Before each subsystem: state which bug classes it must structurally prevent and how the design makes them impossible (not merely unlikely). After each subsystem: run its regression tests before moving on. If a proposed enhancement could reintroduce any guarded bug class, redesign rather than patch. Never bypass the screen-authority discipline for "quick" writes; never add regex-only Markdown or string-prefix shell permission logic in **new** code. diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index e69583a9..6519d45a 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -1,5 +1,9 @@ You are Pythinker — a think-first software engineering agent running on the user's computer. Before you write code, you read code. +# Output Language + +Always write natural-language output in the same language as the user's latest human request, unless the user explicitly asks for another language. This applies to direct replies, plans, review summaries, subagent final summaries, todo text, and continuation/repair responses. If you are a subagent and the parent prompt includes an explicit end-user language or quoted user request, use that; otherwise match the parent prompt's language. Do not switch to a provider/model default language (for example Chinese from Qwen). Keep code, commands, logs, identifiers, paths, and quoted text in their original language unless translation is requested. + Your identity, in order of priority: 1. **Code reviewer.** Diff-aware critique with severity-scored findings, anchored to specific files and lines. @@ -54,6 +58,8 @@ For any codebase, architecture, debugging, security, performance, planning, or " `title` is required; `scope`, `note`, `location`, and `body` are optional. `severity` must be one of the five listed values. Order does not matter — the renderer groups by severity (critical first) and derives the summary tally. Put narrative prose outside the block, before or after it. +**Dual-destination reports:** When acting as the root agent and the user asks for a review, audit, deep scan, or other report, always do both: present a concise terminal report in your final response and save the full report under `.pythinker/reports/.md`. Create `.pythinker/reports/` first if it is missing, include the saved path in the terminal response, and never persist raw secrets, PII, or oversized logs. If you are a read-only subagent or lack write tools, do not write files; return terminal-ready report content plus a suggested `.pythinker/reports/...` path so the parent can display and persist it. + # Engineering Discipline These principles govern every engineering response. They override speed: a slow right answer beats a fast wrong one. @@ -134,8 +140,6 @@ If the `Shell`, `TaskList`, `TaskOutput`, and `TaskStop` tools are available and If a foreground tool call or a background agent requests approval, the approval is coordinated through the unified approval runtime and surfaced through the root UI channel. Do not assume approvals are local to a single subagent turn. -When responding to the user, you MUST use the SAME language as the user, unless explicitly instructed to do otherwise. - # General Guidelines for Coding When building something from scratch, you should: diff --git a/src/pythinker_code/subagents/core.py b/src/pythinker_code/subagents/core.py index 4dcc0aeb..dfab4fa9 100644 --- a/src/pythinker_code/subagents/core.py +++ b/src/pythinker_code/subagents/core.py @@ -18,6 +18,16 @@ from pythinker_code.subagents.models import AgentLaunchSpec, AgentTypeDefinition from pythinker_code.subagents.store import SubagentStore +SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION = """\ + +Write natural-language output in the same language as the original user request or task +prompt, unless that request explicitly asks for another language. Do not switch to a +model/provider default language (for example Chinese from Qwen). Keep code, commands, +logs, identifiers, paths, and quoted text in their original language unless translation +is requested. + +""".strip() + if TYPE_CHECKING: from pythinker_code.soul.agent import Runtime @@ -33,6 +43,12 @@ class SubagentRunSpec: resumed: bool +def _prepend_output_language_instruction(prompt: str) -> str: + if not prompt.strip(): + return SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION + return f"{SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION}\n\n{prompt}" + + async def prepare_soul( spec: SubagentRunSpec, runtime: Runtime, @@ -77,6 +93,7 @@ async def prepare_soul( git_ctx = await collect_git_context(runtime.builtin_args.PYTHINKER_WORK_DIR) if git_ctx: prompt = f"{git_ctx}\n\n{prompt}" + prompt = _prepend_output_language_instruction(prompt) # 5. Write prompt snapshot (debugging aid) store.prompt_path(spec.agent_id).write_text(prompt, encoding="utf-8") diff --git a/src/pythinker_code/subagents/runner.py b/src/pythinker_code/subagents/runner.py index 71a0f081..cbc3e3f7 100644 --- a/src/pythinker_code/subagents/runner.py +++ b/src/pythinker_code/subagents/runner.py @@ -51,7 +51,9 @@ "plan": 300, } SUMMARY_CONTINUATION_PROMPT = """ -Your previous response was too brief. Please provide a more comprehensive summary that includes: +Your previous response was too brief. Please provide a more comprehensive summary. +Keep the same natural-language output language as the original task/user request; do not switch +to a model/provider default language. Include: 1. Specific technical details and implementations 2. Detailed findings and analysis diff --git a/src/pythinker_code/tools/file/replace.py b/src/pythinker_code/tools/file/replace.py index 2992d82e..47713ad6 100644 --- a/src/pythinker_code/tools/file/replace.py +++ b/src/pythinker_code/tools/file/replace.py @@ -1,8 +1,9 @@ +import json from collections.abc import Callable from pathlib import Path -from typing import override +from typing import Any, cast, override -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator from pythinker_core.tooling import CallableTool2, ToolError, ToolReturnValue from pythinker_host.path import HostPath @@ -41,6 +42,69 @@ class Params(BaseModel): ) ) + @staticmethod + def _json_string_to_edit(value: Any) -> Any: + if not isinstance(value, str): + return value + try: + parsed: Any = json.loads(value) + except json.JSONDecodeError: + return value + return ( + cast(dict[str, Any] | list[Any], parsed) if isinstance(parsed, (dict, list)) else value + ) + + @classmethod + def _normalize_edit_aliases(cls, value: Any) -> Any: + value = cls._json_string_to_edit(value) + if isinstance(value, list): + return [cls._normalize_edit_aliases(item) for item in cast(list[Any], value)] + if not isinstance(value, dict): + return value + + normalized: dict[str, Any] = dict(cast(dict[str, Any], value)) + if "old" not in normalized and "oldText" in normalized: + normalized["old"] = normalized["oldText"] + if "new" not in normalized and "newText" in normalized: + normalized["new"] = normalized["newText"] + if "replace_all" not in normalized and "replaceAll" in normalized: + normalized["replace_all"] = normalized["replaceAll"] + return normalized + + @model_validator(mode="before") + @classmethod + def _normalize_common_edit_shapes(cls, data: Any) -> Any: + """Accept common model-generated StrReplaceFile argument shapes. + + Agents occasionally pass the nested ``edit`` payload as a JSON string, or + flatten ``old``/``new`` at the top level after seeing the UI label this + tool as "Update". Normalize those shapes before Pydantic validates the + canonical schema. + """ + if not isinstance(data, dict): + return data + + values: dict[str, Any] = dict(cast(dict[str, Any], data)) + if "edit" in values: + values["edit"] = cls._normalize_edit_aliases(values["edit"]) + return values + if "edits" in values: + values["edit"] = cls._normalize_edit_aliases(values["edits"]) + return values + + old_key = "old" if "old" in values else "oldText" if "oldText" in values else None + new_key = "new" if "new" in values else "newText" if "newText" in values else None + if old_key is not None and new_key is not None: + edit: dict[str, Any] = {"old": values[old_key], "new": values[new_key]} + if "replace_all" in values: + edit["replace_all"] = values["replace_all"] + elif "replaceAll" in values: + edit["replace_all"] = values["replaceAll"] + values["edit"] = edit + return values + + return values + class StrReplaceFile(CallableTool2[Params]): name: str = "StrReplaceFile" diff --git a/src/pythinker_code/ui/shell/__init__.py b/src/pythinker_code/ui/shell/__init__.py index f7aa7ff0..5ab0dc43 100644 --- a/src/pythinker_code/ui/shell/__init__.py +++ b/src/pythinker_code/ui/shell/__init__.py @@ -54,6 +54,7 @@ from pythinker_code.ui.shell.slash import registry as shell_slash_registry from pythinker_code.ui.shell.update import ( pending_update_notice, + prompt_pre_start_update, refresh_update_cache_if_due, ) from pythinker_code.ui.shell.visualize import ( @@ -617,9 +618,15 @@ async def run(self, command: str | None = None) -> bool: finally: self._cancel_background_tasks() - # Start the update check in the background only. Startup must never be - # blocked by version polling; users get a cached toast and can run - # /update when they are ready. + # Blocking pre-start update prompt. Must run before _auto_update so the + # same upgrade isn't shown as both a blocking menu and a background + # toast; if the user picks "Skip this session" the toast is suppressed + # by _skipped_version_this_session. May raise typer.Exit on "Update now" + # or "Exit" — that's the documented behavior. prompt_pre_start_update + # self-suppresses for source checkouts and non-TTY sessions. + await prompt_pre_start_update() + + # Start auto-update background task if not disabled. if get_env_bool("PYTHINKER_CLI_NO_AUTO_UPDATE"): logger.info("Auto-update disabled by PYTHINKER_CLI_NO_AUTO_UPDATE environment variable") else: diff --git a/src/pythinker_code/ui/shell/components/markdown.py b/src/pythinker_code/ui/shell/components/markdown.py index 804cdd58..d6da235f 100644 --- a/src/pythinker_code/ui/shell/components/markdown.py +++ b/src/pythinker_code/ui/shell/components/markdown.py @@ -349,6 +349,29 @@ def _simplify_markdown_report_icons(markup: str) -> str: return "".join(lines) +# Inline code-span repair for table rows: a run of N backticks, lazily-matched +# body, then the first later run containing those same N backticks. This is a +# tolerant LLM-output repair heuristic, not a complete GFM code-span parser. +_CODE_SPAN_RE = re.compile(r"(?P`+)(?P.*?)(?P=ticks)") + + +def _escape_code_span_pipes(text: str) -> str: + r"""Escape raw ``|`` inside inline code spans as ``\|`` so GFM keeps the table + cell intact (LLMs frequently emit unescaped pipes inside code spans). Only the + code-span interior is touched; table-delimiter pipes outside spans are left + alone, and an already-escaped ``\|`` is not double-escaped. Call only on + table-row text (see :func:`_normalize_table_block`); applying it to prose + inline-code would leave a literal backslash in the rendered span. + """ + + def _repl(match: re.Match[str]) -> str: + ticks = match.group("ticks") + body = re.sub(r"(? list[str]: """Split a ``| a | b |`` run into stripped inner cells (drops the frame).""" parts = re.split(r"(? str: while head_lines and head_lines[-1] == "": head_lines.pop() header_match = _HEADER_RE.match(head_lines[-1]) if head_lines else None - header_cells = _split_pipe_cells(header_match.group("cells")) if header_match else [] + header_cells = ( + _split_pipe_cells(_escape_code_span_pipes(header_match.group("cells"))) + if header_match + else [] + ) if header_match is None or len(header_cells) != n_cols: out += text[: match.end()] text = tail @@ -437,7 +464,7 @@ def _normalize_table_block(text: str) -> str: data_rows: list[list[str]] = [] bail = False for segment in data_segments: - cells = _split_pipe_cells(segment) + cells = _split_pipe_cells(_escape_code_span_pipes(segment)) if not cells: continue if len(cells) % n_cols != 0: diff --git a/src/pythinker_code/ui/shell/components/report.py b/src/pythinker_code/ui/shell/components/report.py index 0b629329..2ba2b342 100644 --- a/src/pythinker_code/ui/shell/components/report.py +++ b/src/pythinker_code/ui/shell/components/report.py @@ -18,9 +18,11 @@ import json import logging -import re from dataclasses import dataclass -from typing import Any, Literal, cast, get_args +from typing import TYPE_CHECKING, Any, Literal, cast, get_args + +if TYPE_CHECKING: + from markdown_it import MarkdownIt from rich.console import Group, RenderableType from rich.padding import Padding @@ -62,6 +64,42 @@ _DOT = "●" +# A markdown-it parser is reused so report-fence extraction is fence-aware: a +# ```report block nested inside an outer fence is part of that outer fence's +# content and is therefore NOT a top-level fence token (Principle #5: parse, +# don't pattern-match). +_md_parser: MarkdownIt | None = None + + +def _get_report_parser() -> MarkdownIt: + global _md_parser + if _md_parser is None: + from markdown_it import MarkdownIt + + _md_parser = MarkdownIt() + return _md_parser + + +def _iter_report_payloads(text: str) -> list[tuple[int, int, str]]: + """Yield (start_line, end_line, payload) for each TOP-LEVEL ```report fence. + + Line indices are 0-based half-open ([start, end)) into ``text``'s lines, + matching markdown-it ``token.map``. Nested fences never appear as top-level + ``fence`` tokens, so they are structurally excluded. + """ + md = _get_report_parser() + blocks: list[tuple[int, int, str]] = [] + for token in md.parse(text): + if ( + token.type == "fence" + and token.level == 0 + and token.map is not None + and token.info.strip() == "report" + ): + blocks.append((token.map[0], token.map[1], token.content)) + return blocks + + @dataclass(frozen=True, slots=True) class ReportFinding: """One finding in a report.""" @@ -220,50 +258,48 @@ def parse_report_block(payload: str) -> Report | None: return Report(title=title, scope=scope, findings=tuple(findings), note=note) -# A fenced block whose info string is exactly ``report`` (optionally followed by -# whitespace). Captures the JSON payload between the fences. -_REPORT_FENCE_RE = re.compile( - r"^[ \t]*```[ \t]*report[ \t]*\n(?P.*?)\n[ \t]*```[ \t]*$", - re.DOTALL | re.MULTILINE, -) - - def has_report_block(text: str) -> bool: - """Whether *text* contains at least one well-formed ` ```report ` block. + """Whether *text* contains at least one well-formed top-level ` ```report ` block. Used by output surfaces (e.g. the headless final-text printer) to decide whether to route through :func:`render_agent_body` instead of emitting the raw text. Only matches blocks that actually parse, so a malformed fence - leaves output unchanged. + leaves output unchanged. A ` ```report ` example nested inside an outer + documentation fence is not a top-level fence token, so it is not matched. """ return any( - parse_report_block(m.group("payload")) is not None for m in _REPORT_FENCE_RE.finditer(text) + parse_report_block(payload) is not None for _, _, payload in _iter_report_payloads(text) ) def render_agent_body(text: str, *, theme: ThemeName | None = None) -> RenderableType: - """Render assistant text, promoting ```` ```report ```` blocks to reports. + """Render assistant text, promoting top-level ` ```report ` blocks to reports. - Non-report text renders via :func:`pythinker_markdown`; a valid report - block renders via :func:`render_report`; an invalid block is left in place - so the surrounding markdown shows it as an ordinary code block. + Non-report text renders via :func:`pythinker_markdown`; a valid top-level + report block renders via :func:`render_report`; an invalid or nested block is + left in place so the surrounding markdown shows it as an ordinary code block. """ + # Split on "\n" only (NOT str.splitlines, which also breaks on \f, \v, \x85, + # 
, 
): markdown-it's token.map counts only "\n", so any other + # split character would shift our line indices out of sync with the parser + # and leak fence delimiters into the surrounding prose. + lines = text.split("\n") segments: list[RenderableType] = [] - cursor = 0 - for match in _REPORT_FENCE_RE.finditer(text): - report = parse_report_block(match.group("payload")) + cursor = 0 # line index + for start, end, payload in _iter_report_payloads(text): + report = parse_report_block(payload) if report is None: continue # malformed — leave it for the markdown renderer - before = text[cursor : match.start()].strip("\n") + before = "\n".join(lines[cursor:start]).strip("\n") if before: segments.append(pythinker_markdown(before)) segments.append(render_report(report, theme=theme)) - cursor = match.end() + cursor = end if not segments: return pythinker_markdown(text) - rest = text[cursor:].strip("\n") + rest = "\n".join(lines[cursor:]).strip("\n") if rest: segments.append(pythinker_markdown(rest)) diff --git a/src/pythinker_code/ui/shell/visualize/_live_view.py b/src/pythinker_code/ui/shell/visualize/_live_view.py index b723185c..58856aa8 100644 --- a/src/pythinker_code/ui/shell/visualize/_live_view.py +++ b/src/pythinker_code/ui/shell/visualize/_live_view.py @@ -664,10 +664,9 @@ def _pinned_todo_row( icon = "□" icon_token = "muted" title_style = tui_rich_style("muted") - # The first row carries the ``⎿`` gutter; later rows indent two extra - # columns so their checkbox sits under the first task's title, giving the - # list the nested look of the reference design instead of a flat column. - prefix = f" {TRANSCRIPT_TOOL_GUTTER} " if is_first else " " + # The first row carries the ``⎿`` gutter; continuation rows omit it but + # keep the same checkbox/title columns for a stable todo list alignment. + prefix = f" {TRANSCRIPT_TOOL_GUTTER} " if is_first else " " title_budget = max(1, width - cell_width(prefix) - 2) title = truncate_to_width(todo.title.strip(), title_budget) row = Text(prefix, style=tui_rich_style("muted")) diff --git a/tests/core/test_default_agent.py b/tests/core/test_default_agent.py index 472ec6f3..13c459af 100644 --- a/tests/core/test_default_agent.py +++ b/tests/core/test_default_agent.py @@ -20,6 +20,10 @@ async def test_default_agent(runtime: Runtime): """\ You are Pythinker — a think-first software engineering agent running on the user's computer. Before you write code, you read code. +# Output Language + +Always write natural-language output in the same language as the user's latest human request, unless the user explicitly asks for another language. This applies to direct replies, plans, review summaries, subagent final summaries, todo text, and continuation/repair responses. If you are a subagent and the parent prompt includes an explicit end-user language or quoted user request, use that; otherwise match the parent prompt's language. Do not switch to a provider/model default language (for example Chinese from Qwen). Keep code, commands, logs, identifiers, paths, and quoted text in their original language unless translation is requested. + Your identity, in order of priority: 1. **Code reviewer.** Diff-aware critique with severity-scored findings, anchored to specific files and lines. @@ -74,6 +78,8 @@ async def test_default_agent(runtime: Runtime): `title` is required; `scope`, `note`, `location`, and `body` are optional. `severity` must be one of the five listed values. Order does not matter — the renderer groups by severity (critical first) and derives the summary tally. Put narrative prose outside the block, before or after it. +**Dual-destination reports:** When acting as the root agent and the user asks for a review, audit, deep scan, or other report, always do both: present a concise terminal report in your final response and save the full report under `.pythinker/reports/.md`. Create `.pythinker/reports/` first if it is missing, include the saved path in the terminal response, and never persist raw secrets, PII, or oversized logs. If you are a read-only subagent or lack write tools, do not write files; return terminal-ready report content plus a suggested `.pythinker/reports/...` path so the parent can display and persist it. + # Engineering Discipline These principles govern every engineering response. They override speed: a slow right answer beats a fast wrong one. @@ -154,8 +160,6 @@ async def test_default_agent(runtime: Runtime): If a foreground tool call or a background agent requests approval, the approval is coordinated through the unified approval runtime and surfaced through the root UI channel. Do not assume approvals are local to a single subagent turn. -When responding to the user, you MUST use the SAME language as the user, unless explicitly instructed to do otherwise. - # General Guidelines for Coding When building something from scratch, you should: diff --git a/tests/core/test_prepare_soul.py b/tests/core/test_prepare_soul.py index 7b9eaef0..84dcd0c9 100644 --- a/tests/core/test_prepare_soul.py +++ b/tests/core/test_prepare_soul.py @@ -9,7 +9,11 @@ from pythinker_code.soul.context import Context from pythinker_code.subagents import AgentLaunchSpec, AgentTypeDefinition, ToolPolicy from pythinker_code.subagents.builder import SubagentBuilder -from pythinker_code.subagents.core import SubagentRunSpec, prepare_soul +from pythinker_code.subagents.core import ( + SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION, + SubagentRunSpec, + prepare_soul, +) def _register_coder(runtime): @@ -78,7 +82,7 @@ async def test_prepare_soul_writes_prompt_file(runtime, monkeypatch): await prepare_soul(spec, runtime, builder, runtime.subagent_store) written = runtime.subagent_store.prompt_path("aprompt1").read_text(encoding="utf-8") - assert written == "my prompt text" + assert written == f"{SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION}\n\nmy prompt text" @pytest.mark.asyncio @@ -113,7 +117,7 @@ async def test_prepare_soul_persists_system_prompt_on_first_run(runtime, monkeyp soul, prompt = await prepare_soul(spec, runtime, builder, runtime.subagent_store) assert soul.agent.system_prompt == "fresh system prompt" - assert prompt == "do the work" + assert prompt == f"{SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION}\n\ndo the work" # Verify it was persisted — a second restore should see it ctx2 = Context(runtime.subagent_store.context_path("afresh01")) @@ -143,4 +147,4 @@ async def test_prepare_soul_stage_callback(runtime, monkeypatch): spec2, runtime, builder, runtime.subagent_store, on_stage=None ) assert soul is not None - assert prompt == "test prompt" + assert prompt == f"{SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION}\n\ntest prompt" diff --git a/tests/tools/test_agent_tool.py b/tests/tools/test_agent_tool.py index 64e667e9..5f7b192d 100644 --- a/tests/tools/test_agent_tool.py +++ b/tests/tools/test_agent_tool.py @@ -17,6 +17,7 @@ from pythinker_code.soul.agent import Agent as SoulAgent from pythinker_code.soul.approval import ApprovalResult from pythinker_code.subagents import AgentLaunchSpec, AgentTypeDefinition, ToolPolicy +from pythinker_code.subagents.core import SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION from pythinker_code.tools.agent import AgentRunConfig, RunAgents from pythinker_code.wire.types import ApprovalRequest, TextPart from tests.conftest import tool_call_context @@ -75,7 +76,7 @@ async def fake_run_soul( assert "actual_subagent_type: coder" in result.output assert runtime.subagent_store.require_instance(agent_id).subagent_type == "coder" assert runtime.subagent_store.prompt_path(agent_id).read_text(encoding="utf-8") == ( - "look into parser issue" + f"{SUBAGENT_OUTPUT_LANGUAGE_INSTRUCTION}\n\nlook into parser issue" ) diff --git a/tests/tools/test_str_replace_file.py b/tests/tools/test_str_replace_file.py index 3335303c..fb95f70f 100644 --- a/tests/tools/test_str_replace_file.py +++ b/tests/tools/test_str_replace_file.py @@ -75,6 +75,70 @@ async def test_replace_multiple_edits( assert await file_path.read_text() == "Hi world! See you world!" +async def test_replace_accepts_json_string_edit( + str_replace_file_tool: StrReplaceFile, temp_work_dir: HostPath +): + """Accept the edit object when a model passes it as a JSON string.""" + file_path = temp_work_dir / "test.txt" + await file_path.write_text("old content") + + result = await str_replace_file_tool.call( + { + "path": str(file_path), + "edit": '{"old": "old", "new": "new"}', + } + ) + + assert not result.is_error + assert await file_path.read_text() == "new content" + + +async def test_replace_accepts_json_string_edit_list( + str_replace_file_tool: StrReplaceFile, temp_work_dir: HostPath +): + """Accept multiple edits when a model passes the list as a JSON string.""" + file_path = temp_work_dir / "test.txt" + await file_path.write_text("alpha beta gamma") + + result = await str_replace_file_tool.call( + { + "path": str(file_path), + "edit": '[{"old": "alpha", "new": "one"}, {"old": "beta", "new": "two"}]', + } + ) + + assert not result.is_error + assert await file_path.read_text() == "one two gamma" + + +async def test_replace_accepts_flattened_edit_fields( + str_replace_file_tool: StrReplaceFile, temp_work_dir: HostPath +): + """Accept top-level old/new fields from malformed model tool calls.""" + file_path = temp_work_dir / "test.txt" + await file_path.write_text("old content") + + result = await str_replace_file_tool.call({"path": str(file_path), "old": "old", "new": "new"}) + + assert not result.is_error + assert await file_path.read_text() == "new content" + + +async def test_replace_accepts_edit_aliases( + str_replace_file_tool: StrReplaceFile, temp_work_dir: HostPath +): + """Accept oldText/newText aliases from edit-tool-shaped calls.""" + file_path = temp_work_dir / "test.txt" + await file_path.write_text("old content") + + result = await str_replace_file_tool.call( + {"path": str(file_path), "edits": [{"oldText": "old", "newText": "new"}]} + ) + + assert not result.is_error + assert await file_path.read_text() == "new content" + + async def test_replace_multiline_content( str_replace_file_tool: StrReplaceFile, temp_work_dir: HostPath ): diff --git a/tests/ui_and_conv/README_contract_registry.md b/tests/ui_and_conv/README_contract_registry.md new file mode 100644 index 00000000..a8eeb157 --- /dev/null +++ b/tests/ui_and_conv/README_contract_registry.md @@ -0,0 +1,55 @@ +# Markdown + Report contract test registry (lead phase) + +Maps spec bug classes to the test that guards them. Tier 1 = deterministic +capture. See +`docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md` §8. + +| Spec area | Bug class | Test | Tier | +|---|---|---|---| +| 3 Markdown | table breaks on piped inline code | `test_md_table_contract::test_table_with_piped_inline_code_keeps_columns` | T1 | +| 3 Markdown | escaped pipe splits cell | `test_md_table_contract::test_table_with_escaped_pipes_keeps_literal_pipe` | T1 | +| 3 Markdown | code-span pipe escaper (unit) | `test_md_table_contract::test_escape_code_span_pipes_*` | T1 | +| 3 Markdown | escaper scoped to tables (prose inline-code safe) | `test_md_table_contract::test_prose_inline_code_pipe_is_not_corrupted_with_backslash` | T1 | +| 3 Markdown | empty header mislabeled | `test_md_table_contract::test_table_empty_header_cell_does_not_mislabel` | T1 | +| 3 Markdown | long cell data integrity (no truncation) | `test_md_table_contract::test_table_long_cell_wraps_without_dropping_content` | T1 | +| 3 Markdown | stale streamed table | `test_md_stream_idempotency::test_streaming_table_is_not_committed_mid_row` | T1 | +| 3 Markdown | nested report-fence promoted | `test_report_fence_nesting::*` | T1 | +| 4 ANSI | border inherits code-span color | `test_md_color_contract::test_code_block_border_does_not_use_inline_code_color` | T1 | +| 3 Markdown | report render on real data | `test_report_realdata::*` | T1 | +| 1 Stability | render idempotency | `test_md_stream_idempotency::test_h3_report_and_table_render_is_idempotent` | T1 | +| 1 Stability | offset divergence (H2) | `test_md_stream_idempotency::test_h2_stream_slices_reassemble_without_duplicate_rows` | T1 | +| 1 Stability | screen-authority | `test_md_render_authority::test_no_direct_terminal_writes_in_renderer` | T1 | +| repair | regex pipeline pinned | `test_md_repair_characterization::*` | T1 | + +## Execution notes (deviations from the as-written plan) + +Two contract tests exposed real gaps; both were resolved with explicit approval +(see the design spec §12): + +- **Code-span pipes in tables (source fix).** `pythinker_markdown` followed + strict GFM, so an unescaped `|` inside an inline code span dropped a table + cell — exactly the malformed markdown LLMs emit. Fixed by escaping code-span + pipes inside confirmed table rows (`_escape_code_span_pipes` in + `components/markdown.py`, applied at the two `_split_pipe_cells` sites in + `_normalize_table_block`). The escaper is proven *monotonic on cell count*, so + it can never corrupt a well-formed table; residual corruption only on + already-malformed input is intrinsic to a regex approach. +- **Long-cell test reframed to data integrity.** The narrow stacked-record table + renderer folds cell text mid-word and drops the continuation indent at widths + < ~40 (deferred defect **D1** in the spec — cosmetic, *no data loss*). The + long-cell guard therefore pins the stated contract ("wrap, not truncate"): + every character survives, whitespace-insensitive, regardless of wrap. + +## Known-weak guard (intentional, tracked) + +- `test_md_stream_idempotency::test_streaming_table_is_not_committed_mid_row` + contains a near-tautological branch (`... or "After" in "".join(committed)`), + so its main protection is the reassembly-equality assertion. Kept as written + (no source defect found); strengthen if the streaming committer is revisited. + +## Hypotheses outcome + +- **H1** (nested report-fence promotion) — reproduced, then fixed via AST fence + extraction in `components/report.py` (`test_report_fence_nesting`). +- **H2** (stream offset divergence) — did **not** reproduce; kept as a guard. +- **H3** (render idempotency) — held; kept as a guard. diff --git a/tests/ui_and_conv/_md_contract_helpers.py b/tests/ui_and_conv/_md_contract_helpers.py new file mode 100644 index 00000000..c6c71e22 --- /dev/null +++ b/tests/ui_and_conv/_md_contract_helpers.py @@ -0,0 +1,50 @@ +# tests/ui_and_conv/_md_contract_helpers.py +"""Shared helpers for Markdown + report contract tests. + +DRY home for the two capture modes the repo already uses (plain text and +ANSI-preserving) plus an idempotency comparator. Mirrors the console +configuration in tests/ui_and_conv/test_tui_render_snapshots.py and +tests/ui_and_conv/test_report.py so captured output matches the rest of the +suite. +""" + +from __future__ import annotations + +from collections.abc import Callable + +from rich.console import Console, RenderableType + +# Widths that exercise reflow boundaries: very narrow, a normal width, and an +# exactly-typical report width. Add the exact-full-width case per test. +WIDTHS: tuple[int, ...] = (24, 40, 80) +THEMES: tuple[str, ...] = ("dark", "light") + + +def render_plain(renderable: RenderableType, *, width: int = 80) -> str: + """Capture *renderable* as plain text (no color), like test_report._plain.""" + console = Console(width=width, no_color=True, legacy_windows=False) + with console.capture() as cap: + console.print(renderable) + return cap.get() + + +def render_ansi(renderable: RenderableType, *, width: int = 80) -> str: + """Capture *renderable* keeping ANSI escapes, like test_tui_render_snapshots._ansi.""" + console = Console( + width=width, + record=True, + force_terminal=True, + color_system="truecolor", + legacy_windows=False, + ) + console.print(renderable) + return console.export_text(styles=True) + + +def render_twice_identical(build: Callable[[], RenderableType], *, width: int = 80) -> bool: + """Render a freshly-built renderable twice; True iff byte-identical. + + `build` returns a NEW renderable each call so we test render determinism, + not object identity. + """ + return render_ansi(build(), width=width) == render_ansi(build(), width=width) diff --git a/tests/ui_and_conv/test_live_view_notifications.py b/tests/ui_and_conv/test_live_view_notifications.py index 51f2a36d..75a3347e 100644 --- a/tests/ui_and_conv/test_live_view_notifications.py +++ b/tests/ui_and_conv/test_live_view_notifications.py @@ -142,7 +142,7 @@ def test_working_indicator_pins_todos_under_spinner(monkeypatch): # Active todo now appears both in the spinner header and the pinned list; # done todos sort to the bottom and are dropped by the 5-row cap when # active + pending already fill the rows. - assert "⎿ ■ Explore project context" in rendered + assert "⎿ ■ Explore project context" in rendered assert rendered.count("Explore project context") == 2 assert "□ Ask clarifying questions one at a time" in rendered assert "□ Propose 2–3 approaches with trade-offs" in rendered @@ -164,7 +164,7 @@ def test_working_indicator_keeps_done_todos_pinned(monkeypatch): rendered = _render(view._working_indicator()) assert "todos(" not in rendered - assert "⎿ ✓ Done task" in rendered + assert "⎿ ✓ Done task" in rendered assert "■" not in rendered assert "□" not in rendered diff --git a/tests/ui_and_conv/test_live_view_todos.py b/tests/ui_and_conv/test_live_view_todos.py index 04f23539..55578a2a 100644 --- a/tests/ui_and_conv/test_live_view_todos.py +++ b/tests/ui_and_conv/test_live_view_todos.py @@ -98,7 +98,7 @@ def test_todo_update_pins_current_task_under_activity_line(monkeypatch) -> None: assert "● Implement pinned todos… (7m 40s · ↓ 10k tokens)" in rendered assert rendered.count("Implement pinned todos") == 2 - assert "⎿ ■ Implement pinned todos" in rendered + assert "⎿ ■ Implement pinned todos" in rendered assert "✓ Explore UI" in rendered assert "✓ Write tests" in rendered assert "… +1 completed" in rendered @@ -120,7 +120,7 @@ def test_active_todo_activity_line_does_not_alternate_with_spinner_verb(monkeypa assert "● Implement pinned todos… (7m 45s · ↓ 10k tokens)" in rendered assert _live_view_module.spinner_message(now) not in rendered - assert "⎿ ■ Implement pinned todos" in rendered + assert "⎿ ■ Implement pinned todos" in rendered assert "✓ Explore UI" in rendered @@ -139,7 +139,7 @@ def test_spinner_verb_shows_until_next_todo_becomes_active(monkeypatch) -> None: rendered = _render(view._working_indicator()) assert f"● {_live_view_module.spinner_message(now)} (7m 45s · ↓ 10k tokens)" in rendered - assert "⎿ □ Next task" in rendered + assert "⎿ □ Next task" in rendered assert "✓ Finished task" in rendered @@ -191,7 +191,7 @@ def test_active_pinned_todo_row_uses_accent_icon_and_white_title() -> None: assert title_style.bold is True -def test_non_first_pinned_rows_indent_under_first_title() -> None: +def test_pinned_todo_rows_align_icons_and_titles() -> None: view = _LiveView(StatusUpdate()) first = view._pinned_todo_row( @@ -206,11 +206,12 @@ def test_non_first_pinned_rows_indent_under_first_title() -> None: width=100, ) - # First row carries the ⎿ gutter; later rows indent so their checkbox sits - # under the first row's title (icons intentionally not aligned). - assert first.plain.startswith(" ⎿ ■ ") + # First row carries the ⎿ gutter; later rows omit it but keep the same + # checkbox and title columns. + assert first.plain.startswith(" ⎿ ■ ") assert later.plain.startswith(" □ ") - assert later.plain.index("□") == first.plain.index("Lead task") + assert later.plain.index("□") == first.plain.index("■") + assert later.plain.index("Next task") == first.plain.index("Lead task") def test_successful_todo_tool_card_is_suppressed() -> None: diff --git a/tests/ui_and_conv/test_md_color_contract.py b/tests/ui_and_conv/test_md_color_contract.py new file mode 100644 index 00000000..599130fe --- /dev/null +++ b/tests/ui_and_conv/test_md_color_contract.py @@ -0,0 +1,48 @@ +# tests/ui_and_conv/test_md_color_contract.py +"""Tier-1 ANSI/color contract tests (spec area 4). + +Uses the truecolor-preserving capture so we can assert on SGR sequences, +exactly like tests/ui_and_conv/test_tui_render_snapshots.py. +""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.markdown import pythinker_markdown +from pythinker_code.ui.theme import get_markdown_colors +from tests.ui_and_conv._md_contract_helpers import render_ansi + + +def _sgr_fg(hexcolor: str) -> str: + """Build the truecolor foreground SGR fragment for a #rrggbb color.""" + h = hexcolor.lstrip("#") + r, g, b = int(h[0:2], 16), int(h[2:4], 16), int(h[4:6], 16) + return f"38;2;{r};{g};{b}" + + +def test_code_block_border_does_not_use_inline_code_color(): + """Bug class: 'border colors inheriting code-span color'. + + The bordered code block frame uses code_block_border; inline code uses + inline_code. They must be distinct colors, and the captured frame must not + paint the border in the inline-code color. + """ + colors = get_markdown_colors("dark") + assert colors.code_block_border != colors.inline_code, ( + "precondition: palette must distinguish border from inline code" + ) + md = "Here is `inline` and a block:\n\n```python\nx = 1\n```\n" + coloured = render_ansi(pythinker_markdown(md), width=60) + # The rounded frame characters must not carry the inline-code foreground. + inline_fg = _sgr_fg(colors.inline_code) + for frame_char in ("╭", "╰", "─"): + found_count = 0 + start = 0 + while True: + idx = coloured.find(frame_char, start) + if idx == -1: + break + found_count += 1 + window = coloured[max(0, idx - 24) : idx] + assert inline_fg not in window, "border frame inherited inline-code color" + start = idx + 1 + assert found_count > 0, f"missing expected frame glyph {frame_char!r}" diff --git a/tests/ui_and_conv/test_md_contract_helpers.py b/tests/ui_and_conv/test_md_contract_helpers.py new file mode 100644 index 00000000..f8d23802 --- /dev/null +++ b/tests/ui_and_conv/test_md_contract_helpers.py @@ -0,0 +1,22 @@ +# tests/ui_and_conv/test_md_contract_helpers.py +"""Smoke test for the shared Markdown/report contract helpers.""" + +from __future__ import annotations + +from rich.text import Text + +from tests.ui_and_conv._md_contract_helpers import ( + THEMES, + WIDTHS, + render_ansi, + render_plain, + render_twice_identical, +) + + +def test_helpers_capture_and_compare(): + assert "hello" in render_plain(Text("hello"), width=40) + # truecolor capture keeps SGR codes; a red fg emits the 31-family sequence. + assert "\x1b[" in render_ansi(Text("hi", style="red"), width=40) + assert render_twice_identical(lambda: Text("stable")) is True + assert WIDTHS and THEMES # parametrization sources are non-empty diff --git a/tests/ui_and_conv/test_md_render_authority.py b/tests/ui_and_conv/test_md_render_authority.py new file mode 100644 index 00000000..074c3f63 --- /dev/null +++ b/tests/ui_and_conv/test_md_render_authority.py @@ -0,0 +1,38 @@ +# tests/ui_and_conv/test_md_render_authority.py +"""Screen-authority discipline (spec principle 1) for the lead-phase modules. + +These renderers must return Rich renderables, never write to the terminal +directly. A bare print()/sys.stdout.write in a renderer bypasses the Live +screen model and causes the duplicate-scrollback / corruption bug classes. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_SRC = ( + Path(__file__).resolve().parents[2] / "src" / "pythinker_code" / "ui" / "shell" / "components" +) +_GUARDED = ["report.py", "markdown.py"] + + +@pytest.mark.parametrize("filename", _GUARDED) +def test_no_direct_terminal_writes_in_renderer(filename): + tree = ast.parse((_SRC / filename).read_text()) + offenders: list[str] = [] + for node in ast.walk(tree): + if isinstance(node, ast.Call): + func = node.func + if isinstance(func, ast.Name) and func.id == "print": + offenders.append(f"print() at line {node.lineno}") + if isinstance(func, ast.Attribute) and func.attr == "write": + target = func.value + is_std_stream = ( + isinstance(target, ast.Attribute) and target.attr in {"stdout", "stderr"} + ) or (isinstance(target, ast.Name) and target.id in {"stdout", "stderr"}) + if is_std_stream: + offenders.append(f"std*.write at line {node.lineno}") + assert not offenders, f"{filename} bypasses the screen model: {offenders}" diff --git a/tests/ui_and_conv/test_md_repair_characterization.py b/tests/ui_and_conv/test_md_repair_characterization.py new file mode 100644 index 00000000..4683940c --- /dev/null +++ b/tests/ui_and_conv/test_md_repair_characterization.py @@ -0,0 +1,48 @@ +# tests/ui_and_conv/test_md_repair_characterization.py +"""Characterization tests that PIN the existing regex Markdown-repair pipeline. + +These lock in current correct behavior of _repair_crammed_markdown_tables, +_normalize_markdown_tables, and the priority-matrix detector so any future +change that alters them is caught. Per the spec (§2), this pipeline is pinned, +NOT refactored. If a characterized output looks imperfect, mark it with a +`# pinned: imperfect` note and a follow-up — do not change source here. +""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.markdown import ( + _normalize_markdown_tables, + _repair_crammed_markdown_tables, + pythinker_markdown, +) +from tests.ui_and_conv._md_contract_helpers import render_plain + + +def test_glued_heading_and_table_header_is_split(): + """Model output that glues a section title to a table header gets split so + the table renders as a table, not crammed prose.""" + glued = "Medium| # | File |\n| --- | --- |\n| 1 | a.py |\n" + repaired = _repair_crammed_markdown_tables(glued) + # The heading is separated onto its own line before the table header. + assert repaired.splitlines()[0].strip() == "Medium" + out = render_plain(pythinker_markdown(glued), width=60) + assert "Medium" in out + assert "a.py" in out + assert "File" in out + + +def test_crammed_data_rows_on_delimiter_line_are_rechunked(): + """Data cells crammed onto the delimiter line are split into rows.""" + crammed = "| # | File |\n| --- | --- || 1 | a.py || 2 | b.py |\n" + normalized = _normalize_markdown_tables(crammed) + out = render_plain(pythinker_markdown(normalized), width=60) + assert "a.py" in out + assert "b.py" in out + + +def test_wellformed_table_is_passed_through_unchanged_in_render(): + """A clean table renders with both rows and the header intact.""" + clean = "| A | B |\n| --- | --- |\n| 1 | 2 |\n| 3 | 4 |\n" + out = render_plain(pythinker_markdown(clean), width=40) + for token in ("A", "B", "1", "2", "3", "4"): + assert token in out diff --git a/tests/ui_and_conv/test_md_stream_idempotency.py b/tests/ui_and_conv/test_md_stream_idempotency.py new file mode 100644 index 00000000..aad3f159 --- /dev/null +++ b/tests/ui_and_conv/test_md_stream_idempotency.py @@ -0,0 +1,68 @@ +# tests/ui_and_conv/test_md_stream_idempotency.py +"""Streaming-boundary contract + idempotency/divergence hypotheses (H2, H3).""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.markdown import PythinkerMarkdownStream, pythinker_markdown +from tests.ui_and_conv._md_contract_helpers import render_ansi, render_plain + + +def _drain(chunks: list[str]) -> list[str]: + """Feed chunks to the stream; return the ordered list of committed slices.""" + stream = PythinkerMarkdownStream() + committed: list[str] = [] + for chunk in chunks: + ready = stream.push(chunk) + if ready: + committed.append(ready) + tail = stream.flush() + if tail: + committed.append(tail) + return committed + + +def test_streaming_table_is_not_committed_mid_row(): + """Bug class: 'stale bordered tables left in scrollback while streaming'. + + A table streamed one line at a time must not have a partial (header-only or + header+delimiter-only) slice committed as a finished block: the committer + keeps the last top-level block mutable until a following block begins. + """ + full = "Intro paragraph.\n\n| A | B |\n| --- | --- |\n| 1 | 2 |\n\nAfter.\n" + # stream character-by-character to maximize the chance of a mid-table commit + committed = _drain(list(full)) + # No committed slice may end in the middle of the table (i.e. contain the + # delimiter row but not the data row that completes this fixture's table). + for slice_ in committed[:-1]: + if "---" in slice_: + assert "| 1 | 2 |" in slice_, "a header-only table was committed before data arrived" + # Reassembled stream equals the original (no loss, no duplication). + assert "".join(committed) == full + + +# Glued prose+table that forces the regex repair pipeline to fire on a slice +# whose commit boundary was computed on the RAW (un-repaired) text. +_GLUED = "Findings Medium| # | File |\n| --- | --- |\n| 1 | a.py |\n| 2 | b.py |\n\nNext.\n" + + +def test_h2_stream_slices_reassemble_without_duplicate_rows(): + """H2: commit offsets are computed on raw text while the renderer transforms + repaired text. Try to reproduce a duplicate/stale row. Expected: PASS + (non-reproduction). If this FAILS, H2 is confirmed — capture the case. + """ + committed = _drain(list(_GLUED)) + reassembled = "".join(committed) + assert reassembled == _GLUED + # Render each committed slice; 'a.py' and 'b.py' must each appear exactly + # once across the rendered stream (no row duplicated by the repair pass). + rendered = "".join(render_plain(pythinker_markdown(s)) for s in committed) + assert rendered.count("a.py") == 1 + assert rendered.count("b.py") == 1 + + +def test_h3_report_and_table_render_is_idempotent(): + """H3: rendering the same markdown twice yields byte-identical output.""" + md = "## Title\n\n| A | B |\n| --- | --- |\n| 1 | `x|y` |\n\nDone.\n" + first = render_ansi(pythinker_markdown(md), width=70) + second = render_ansi(pythinker_markdown(md), width=70) + assert first == second diff --git a/tests/ui_and_conv/test_md_table_contract.py b/tests/ui_and_conv/test_md_table_contract.py new file mode 100644 index 00000000..3ffd7fdc --- /dev/null +++ b/tests/ui_and_conv/test_md_table_contract.py @@ -0,0 +1,116 @@ +# tests/ui_and_conv/test_md_table_contract.py +"""Tier-1 contract tests for Markdown table rendering (spec area 3). + +Each test names the bug class it guards. These assert the EXISTING stack +(pythinker_markdown over markdown-it + Rich) already meets the contract; a +failure is a real regression to surface, not to silence. +""" + +from __future__ import annotations + +import pytest + +from pythinker_code.ui.shell.components.markdown import ( + _escape_code_span_pipes, + pythinker_markdown, +) +from tests.ui_and_conv._md_contract_helpers import WIDTHS, render_plain + + +def test_table_with_piped_inline_code_keeps_columns(): + """Bug class: 'tables breaking on piped inline code'.""" + md = "| Expr | Meaning |\n| --- | --- |\n| `a | b` | bitwise or |\n| plain | text |\n" + out = render_plain(pythinker_markdown(md), width=80) + lines = [line for line in out.splitlines() if line.strip()] + + # Both data rows survive as distinct table rows; this is stronger than + # checking content presence, which could also pass for a collapsed paragraph. + assert any("Expr" in line and "Meaning" in line for line in lines) + code_row = [line for line in lines if "a | b" in line and "bitwise or" in line] + plain_row = [line for line in lines if "plain" in line and "text" in line] + assert code_row + assert plain_row + assert code_row[0] != plain_row[0] + + +def test_table_with_escaped_pipes_keeps_literal_pipe(): + """Bug class: escaped pipe must render as a literal '|', not split a cell.""" + md = "| Col |\n| --- |\n| a \\| b |\n" + out = render_plain(pythinker_markdown(md), width=80) + assert "a | b" in out # literal pipe preserved + assert "a \\| b" not in out + assert "Col" in out + + +def test_escape_code_span_pipes_standard_case(): + # raw pipe inside a single-backtick code span gets escaped; outer table pipes untouched + assert _escape_code_span_pipes("| `a | b` | bitwise or |") == "| `a \\| b` | bitwise or |" + + +def test_escape_code_span_pipes_double_backticks(): + assert _escape_code_span_pipes("| ``a | b`` | target |") == "| ``a \\| b`` | target |" + + +def test_escape_code_span_pipes_already_escaped_is_idempotent(): + # must NOT double-escape an existing \| -> \\| + assert _escape_code_span_pipes("| `a \\| b` | target |") == "| `a \\| b` | target |" + + +def test_escape_code_span_pipes_no_code_span_unchanged(): + assert _escape_code_span_pipes("| regular | cell |") == "| regular | cell |" + + +def test_escape_code_span_pipes_leaves_lone_backtick_delimiters_alone(): + # A single unbalanced backtick is not a code span, so the real '|' delimiters + # must be preserved (no closing run -> no match -> no escaping). + assert _escape_code_span_pipes("| a ` b | c |") == "| a ` b | c |" + + +def test_escape_code_span_pipes_characterizes_mismatched_longer_closing_run(): + # This helper is an LLM-output repair heuristic for table rows, not a full + # GFM code-span parser. Keep the current tolerant behavior explicit: a + # longer closing run still protects the pipe before markdown-it sees the row. + assert _escape_code_span_pipes("| `a | b`` | target |") == "| `a \\| b`` | target |" + + +def test_prose_inline_code_pipe_is_not_corrupted_with_backslash(): + """Scope guarantee: the escaper only runs on table rows. Inline code in plain + prose must render its pipe literally, never gain a stray backslash (which a + CommonMark code span would show verbatim).""" + out = render_plain(pythinker_markdown("Use `a | b` for bitwise or.\n"), width=80) + assert "a | b" in out + assert "\\|" not in out + + +def test_table_empty_header_cell_does_not_mislabel(): + """Bug class: 'empty header cells mislabeled in narrow stacked layout'.""" + md = "| | Value |\n| --- | --- |\n| key | 42 |\n" + out = render_plain(pythinker_markdown(md), width=30) + assert "Value" in out + assert "key" in out + assert "42" in out + + +@pytest.mark.parametrize("width", WIDTHS) +def test_table_long_cell_wraps_without_dropping_content(width): + """Bug class: very long cells at narrow widths must wrap, not truncate. + + This pins the *data-integrity* contract the bug class names ("wrap, not + truncate"): every character of the long cell survives in order, regardless + of how the narrow stacked-record layout wraps it. We compare with all + whitespace removed so a wrap (whether at a word boundary or, at very narrow + widths, mid-word) still counts as survival — wrapping is not data loss. + + Known deferred renderer-polish defect (NOT data loss, so not guarded here): + at widths < ~40 the stacked-record table path folds cell text mid-word + (``theta`` -> ``t\\nheta``) and omits the continuation-line indent. Tracked + in tests/ui_and_conv/README_contract_registry.md and the design spec's + deferred-defects note. + """ + long_cell = "alpha beta gamma delta epsilon zeta eta theta iota kappa" + md = f"| Name | Note |\n| --- | --- |\n| item | {long_cell} |\n" + out = render_plain(pythinker_markdown(md), width=width) + # No character of the long cell is dropped (truncation), independent of wrap. + stripped_cell = "".join(long_cell.split()) + stripped_out = "".join(out.split()) + assert stripped_cell in stripped_out, f"long cell content truncated at width={width}" diff --git a/tests/ui_and_conv/test_report_fence_nesting.py b/tests/ui_and_conv/test_report_fence_nesting.py new file mode 100644 index 00000000..05575910 --- /dev/null +++ b/tests/ui_and_conv/test_report_fence_nesting.py @@ -0,0 +1,69 @@ +# tests/ui_and_conv/test_report_fence_nesting.py +"""H1: a ```report block shown INSIDE an outer documentation fence must not be +promoted to a report. The flat _REPORT_FENCE_RE regex cannot see fence nesting; +an AST walk over top-level fence tokens structurally can. +""" + +from __future__ import annotations + +from pythinker_code.ui.shell.components.report import has_report_block, render_agent_body +from tests.ui_and_conv._md_contract_helpers import render_plain + +# A 4-backtick outer fence whose body is a literal ```report example. markdown-it +# parses the outer fence as ONE token, so the inner block is documentation text, +# not a real report. +_NESTED = ( + "Here is how to emit a report:\n\n" + "````markdown\n" + "```report\n" + '{"title": "Example", "findings": [{"title": "x", "severity": "high"}]}\n' + "```\n" + "````\n" +) + + +def test_nested_report_fence_is_not_detected(): + assert has_report_block(_NESTED) is False + + +def test_nested_report_fence_renders_as_documentation_not_report(): + out = render_plain(render_agent_body(_NESTED)) + # The inner block stays verbatim documentation; it is NOT promoted to the + # report renderer (which would drop the JSON and print a tally). + assert '"title": "Example"' in out + assert "1 high" not in out # no report tally emitted + + +def test_top_level_report_fence_still_promoted(): + """Regression guard: the real top-level case must keep working.""" + text = ( + "Intro.\n\n```report\n" + '{"title": "Real", "findings": [{"title": "bug", "severity": "medium"}]}\n' + "```\n" + ) + out = render_plain(render_agent_body(text)) + assert "Real" in out + assert "1 medium" in out + assert '"severity"' not in out # rendered as a report, not raw JSON + + +def test_report_fence_with_exotic_line_separator_does_not_leak_delimiter(): + """Regression: a non-newline Unicode line separator (here U+0085 NEL) in the + prose before a valid top-level report must not desync the line slicing. + + markdown-it's ``token.map`` counts only ``\\n``; ``render_agent_body`` must + split on ``\\n`` only. ``str.splitlines`` also breaks on \\f \\v \\x85 , + which would shift indices and leak the closing ``` fence into the trailing + prose, rendering as a spurious empty bordered code block under the report. + """ + text = ( + "intro\x85more\n\n```report\n" + '{"title": "Real", "findings": [{"title": "bug", "severity": "medium"}]}\n' + "```\n" + ) + out = render_plain(render_agent_body(text)) + assert "Real" in out + assert "1 medium" in out + # No leaked closing fence -> no spurious bordered code block under the report. + assert "```" not in out + assert "╭" not in out and "╰" not in out diff --git a/tests/ui_and_conv/test_report_realdata.py b/tests/ui_and_conv/test_report_realdata.py new file mode 100644 index 00000000..da6c062c --- /dev/null +++ b/tests/ui_and_conv/test_report_realdata.py @@ -0,0 +1,103 @@ +# tests/ui_and_conv/test_report_realdata.py +"""Report rendering grounded in the real security-scan-findings.json fixture. + +The fixture is RAW scanner shape (filePath / severity UPPERCASE / vulnSlug / +title / description / lineNumbers / recommendation / confidence). report.py +consumes the Report shape (title / severity lowercase / location / body). The +transform below encodes the contract: case-fold severity, fold the scanner's +extended severities (HIGH_BUG, BUG) into the canonical five via _SEVERITY_ALIASES, +fold filePath + lineNumbers into location, fold description + recommendation into +body. + +No in-repo production transform exists: the external scanner emits canonical +``report`` JSON directly and report.parse_report_block rejects any non-canonical +severity, so this adapter is the documented contract (see the design spec R2 and +Task 12). +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from pythinker_code.ui.shell.components.report import ( + Report, + ReportFinding, + Severity, + render_report, +) +from tests.ui_and_conv._md_contract_helpers import THEMES, WIDTHS, render_plain + +_FIXTURE = Path(__file__).resolve().parents[2] / "security-scan-findings.json" +_VALID_SEVERITIES = {"critical", "high", "medium", "low", "info"} +# Scanner-native severities outside the canonical five, folded to a canonical +# value. The scanner's compound "*_BUG" / bare "BUG" tags are bug-tracker +# qualifiers, not Report severities. +_SEVERITY_ALIASES = {"high_bug": "high", "bug": "medium"} + + +def _location(raw: dict) -> str | None: + path = raw.get("filePath") + if not isinstance(path, str) or not path: + return None + lines = raw.get("lineNumbers") or [] + if isinstance(lines, list) and lines: + return f"{path}:{lines[0]}" + return path + + +def _body(raw: dict) -> str: + parts = [] + if raw.get("description"): + parts.append(str(raw["description"])) + if raw.get("recommendation"): + parts.append(f"**Fix:** {raw['recommendation']}") + return "\n\n".join(parts) + + +def _to_finding(raw: dict) -> ReportFinding: + raw_severity = str(raw["severity"]).lower() + severity = _SEVERITY_ALIASES.get(raw_severity, raw_severity) + assert severity in _VALID_SEVERITIES, f"unexpected severity {raw['severity']!r}" + return ReportFinding( + title=str(raw["title"]), + severity=severity, # type: ignore[arg-type] + location=_location(raw), + body=_body(raw), + ) + + +def _load_report(limit: int | None = None) -> Report: + raw = json.loads(_FIXTURE.read_text()) + findings = tuple(_to_finding(r) for r in (raw[:limit] if limit else raw)) + return Report(title="Security Scan", scope=f"{len(findings)} findings", findings=findings) + + +def test_fixture_transforms_to_valid_report(): + report = _load_report() + assert len(report.findings) == 92 + # Every transformed severity is a valid Report severity. + seen: set[Severity] = {f.severity for f in report.findings} + assert seen <= _VALID_SEVERITIES + assert "critical" in seen # the fixture contains CRITICAL findings + + +@pytest.mark.parametrize("theme", THEMES) +@pytest.mark.parametrize("width", WIDTHS) +def test_real_report_renders_across_theme_and_width(theme, width): + out = render_plain(render_report(_load_report(limit=12), theme=theme), width=width) + assert "Security Scan" in out + # The summary tally line names at least one present severity. + assert any(sev in out for sev in ("critical", "high", "medium", "low", "info")) + + +def test_real_report_shows_locations_and_titles(): + out = render_plain(render_report(_load_report(limit=5)), width=100) + report = _load_report(limit=5) + for finding in report.findings: + assert finding.title[:20] in out + if finding.location: + # the file path portion of the first finding's location appears + assert finding.location.split(":")[0].split("/")[-1] in out diff --git a/tests/ui_and_conv/test_shell_update.py b/tests/ui_and_conv/test_shell_update.py index cca57d01..e5104740 100644 --- a/tests/ui_and_conv/test_shell_update.py +++ b/tests/ui_and_conv/test_shell_update.py @@ -855,3 +855,47 @@ def test_update_prompt_text_shows_version_and_command(monkeypatch): assert "✨ Update available! 1.2.0 -> 1.3.0" in output assert "Release notes:" in output assert "uv tool upgrade pythinker-code" in output + + +@pytest.mark.asyncio +async def test_run_awaits_pre_start_update_before_auto_update(runtime, tmp_path, monkeypatch): + """Regression (efe101c/#63): Shell.run() must await prompt_pre_start_update() + — the blocking update menu — before scheduling the _auto_update background + toast. The menu was silently unwired while its unit tests stayed green; this + pins the wiring so it can't regress again unnoticed. + """ + from unittest.mock import AsyncMock, MagicMock + + from pythinker_core.tooling.empty import EmptyToolset + + from pythinker_code.soul.agent import Agent + from pythinker_code.soul.context import Context + from pythinker_code.soul.pythinkersoul import PythinkerSoul + from pythinker_code.ui.shell import Shell + + monkeypatch.delenv("PYTHINKER_CLI_NO_AUTO_UPDATE", raising=False) + + agent = Agent(name="Test", system_prompt="test", toolset=EmptyToolset(), runtime=runtime) + soul = PythinkerSoul(agent, context=Context(file_backend=tmp_path / "h.jsonl")) + shell = Shell(soul) + + class _PromptReached(Exception): + pass + + # Patch the name where run() looks it up (imported into the ui.shell module). + prompt_mock = AsyncMock(side_effect=_PromptReached) + monkeypatch.setattr("pythinker_code.ui.shell.prompt_pre_start_update", prompt_mock) + # If auto_update were scheduled before the prompt, this spy would be called. + auto_update_mock = MagicMock(name="_auto_update") + monkeypatch.setattr(shell, "_auto_update", auto_update_mock) + + # The sentinel is the real guard: _PromptReached is only reachable if run() + # actually awaits the (patched) prompt, so it pins prompt-before-auto_update. + # If the wiring were removed, run() would instead schedule the un-awaited + # MagicMock _auto_update and fail at create_task (TypeError) — still a failure, + # just a noisier one. + with pytest.raises(_PromptReached): + await shell.run() + + prompt_mock.assert_awaited_once() + auto_update_mock.assert_not_called()