fix(agent): harden reports and edit handling - #23
Merged
Conversation
Spec for hardening the existing Rich + prompt_toolkit TUI against a catalog of rendering bug classes. Treats the source spec as a behavioral contract rather than an implementation directive: the win condition is a deterministic, bug-class-mapped test suite, not a from-scratch renderer. Leads with Markdown + security/scan report rendering (Tier-1 capture tests grounded in the real 92-finding fixture), classifies every bug class by test tier, and keeps the remaining 11 subsystems as a shallow sequenced roadmap.
Bite-sized TDD plan for the lead phase of the contract-hardening design: shared capture/idempotency helpers, table + color contract tests, real 92-finding report fixture, regex-repair characterization, and one source fix (AST-based report-fence extraction so a nested ```report block inside a documentation fence is no longer promoted). Plan-only; execution gated on greenlight.
Empty-header guard and a width-parametrized long-cell guard. The long-cell test pins the data-integrity contract (no character truncated, wrap-insensitive) rather than word-contiguity, since the narrow stacked-record renderer folds mid-word without losing content. Logs that mid-word fold + missing continuation indent as deferred renderer-polish defect D1 in the design spec.
Replace the flat _REPORT_FENCE_RE regex with a markdown-it AST walk over top-level fence tokens, so a ```report block nested inside an outer documentation fence is not promoted to a report. render_agent_body slices by token.map line ranges, splitting on "\n" only (not str.splitlines, which also breaks on \f/\v/U+0085/U+2028/U+2029) to stay index-aligned with markdown-it and avoid leaking the closing fence. Parser is typed via TYPE_CHECKING import to match the sibling components/markdown.py pattern.
Registry maps each lead-phase bug class to its guard and records the two approved deviations (code-span-pipe source fix; long-cell data-integrity reframe + deferred defect D1) and the H1/H2/H3 outcomes. Also fixes a UP035 lint nit in the shared helper (Callable from collections.abc).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR hardens the TUI renderer's Markdown and report rendering with contract tests, refactors report fence detection from regex to markdown-it AST parsing, adds output-language guardrails for agents/subagents, escapes pipes in inline code within table cells, normalizes file-replace tool inputs, integrates a pre-start update prompt, and creates extensive test infrastructure for deterministic rendering validation across themes and widths. ChangesTUI Renderer Contract Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-29-tui-markdown-report-contract-hardening.md`:
- Around line 9-10: Remove the non-uv fallback command in the tech stack line:
delete the ".venv/bin/python -m pytest …" fallback and ensure the example only
shows the uv form (e.g., "uv run pytest …"); update the sentence to comply with
the repo policy so all Python/tool invocation examples use "uv" (or Make
targets) rather than direct .venv/python calls.
- Around line 751-752: The plan uses text.splitlines(keepends=True) to build
lines, which breaks the fence-token line-map contract and can desync indices;
change the split to text.split("\n") so the variable lines uses \n-only
splitting and matches the slicing semantics used by render_agent_body and tests,
ensuring fence delimiter indices remain consistent with the fence-token line-map
logic (update any related assumptions in the surrounding code that expect
keepends behavior to operate on the new lines array).
In `@src/pythinker_code/ui/shell/components/report.py`:
- Around line 71-101: Rename the new snake_case identifiers to camelCase to
match repo conventions: change _md_parser to _mdParser and _get_report_parser to
_getReportParser (and update all calls), rename _iter_report_payloads to
_iterReportPayloads, and rename local variables like blocks to blockList (and
any payload, cursor, rest instances mentioned at 286-303) while preserving
behavior; update all references/usages within the file so function names and
variables remain consistent and imports/return types are unchanged.
In `@tests/ui_and_conv/test_md_color_contract.py`:
- Around line 37-42: The test currently skips missing border glyphs and only
checks the first occurrence, allowing vacuous passes; update the loop over
frame_char to fail if the glyph is not found and to check every occurrence: for
each frame_char, scan through all indices in the string coloured (use str.find
with a start offset in a while loop) and for each found index compute window =
coloured[max(0, idx - 24): idx] and assert inline_fg not in window, and if no
occurrence was found for a given frame_char raise/assert (e.g., assert False or
assert idx_found_count > 0 with a helpful message) so missing glyphs do not
silently pass.
In `@tests/ui_and_conv/test_md_stream_idempotency.py`:
- Around line 36-40: The assertion uses "After" in "".join(committed) which can
be true from other slices and thus doesn't protect each individual slice; change
the guard to check the current slice_ instead: in the loop over committed[:-1]
assert that either the slice_.rstrip().endswith("|") is False or "After" in
slice_, so each slice is validated independently (use the existing variables
slice_ and committed and update the condition to `"After" in slice_`).
In `@tests/ui_and_conv/test_md_table_contract.py`:
- Around line 39-41: The current test uses a weak assertion that combines two
conditions with OR, allowing false positives; update the assertions in the test
that call render_plain(pythinker_markdown(md), width=80) so they explicitly
require the literal pipe to be present and the escaped form absent (i.e.,
replace the `assert "a | b" in out or "a \\| b" not in out` logic with two
separate assertions that assert "a | b" in out and assert "a \\| b" not in out),
keeping the existing check for "Col" unchanged.
In `@tests/ui_and_conv/test_report_fence_nesting.py`:
- Around line 15-22: The test introduces snake_case names (_NESTED, out, text) —
rename them to camelCase (for example _NESTED -> nestedReportMarkdown, out ->
output, text -> inputText) and update every reference in the file (including the
other occurrences around the same test) so imports, assertions, and helper calls
use the new camelCase identifiers; ensure constants that are intended to be
private still use a leading underscore if desired (e.g., _nestedReportMarkdown)
and run the test to confirm no reference errors.
In `@tests/ui_and_conv/test_report_realdata.py`:
- Around line 33-39: The test module introduces snake_case constants (_FIXTURE,
_VALID_SEVERITIES, _SEVERITY_ALIASES and other newly added locals across the
file) — rename these to camelCase (e.g., _fixture -> fixturePath or fixturePath,
_validSeverities, _severityAliases) and update every reference/usage (including
within functions and tests between the earlier added sections) to the new
camelCase identifiers; ensure imports, asserts, and any dict lookups or set
literals using these symbols are updated consistently and run tests to confirm
no unresolved names remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a0452b3-3c3d-421c-a9bd-31b0c61b1560
📒 Files selected for processing (27)
.pythinker/reports/validation-tui-renderer-contract-hardening.mddocs/superpowers/plans/2026-05-29-tui-markdown-report-contract-hardening.mddocs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.mdsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/subagents/core.pysrc/pythinker_code/subagents/runner.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/visualize/_live_view.pytests/core/test_default_agent.pytests/core/test_prepare_soul.pytests/tools/test_str_replace_file.pytests/ui_and_conv/README_contract_registry.mdtests/ui_and_conv/_md_contract_helpers.pytests/ui_and_conv/test_live_view_notifications.pytests/ui_and_conv/test_live_view_todos.pytests/ui_and_conv/test_md_color_contract.pytests/ui_and_conv/test_md_contract_helpers.pytests/ui_and_conv/test_md_render_authority.pytests/ui_and_conv/test_md_repair_characterization.pytests/ui_and_conv/test_md_stream_idempotency.pytests/ui_and_conv/test_md_table_contract.pytests/ui_and_conv/test_report_fence_nesting.pytests/ui_and_conv/test_report_realdata.pytests/ui_and_conv/test_shell_update.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ui_and_conv/test_md_render_authority.py (1)
31-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAST pattern misses
stdout.write()afterfrom sys import stdout/stderr.The check on line 34 requires
func.valueto be anAttribute, which matchessys.stdout.write()but misses the common pattern:from sys import stdout stdout.write("bypassed!")In that AST,
func.valueis aNamenode withid='stdout', not anAttribute, so the isinstance check fails and the bypass isn't detected.🔍 Proposed fix to catch direct stdout/stderr writes
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}") + if ( + isinstance(func, ast.Attribute) + and func.attr == "write" + and isinstance(func.value, ast.Name) + and func.value.id in {"stdout", "stderr"} + ): + offenders.append(f"std*.write at line {node.lineno}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui_and_conv/test_md_render_authority.py` around lines 31 - 37, The current AST check only detects sys.stdout.write by requiring func.value to be an ast.Attribute; modify the condition around the write-detection (the block that builds offenders and uses func, func.attr, func.value, and node.lineno) to also accept func.value being an ast.Name with id in {"stdout","stderr"} (in addition to the existing ast.Attribute case), so calls like stdout.write(...) or stderr.write(...) are appended to offenders the same way as sys.stdout.write(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/ui_and_conv/test_md_render_authority.py`:
- Around line 31-37: The current AST check only detects sys.stdout.write by
requiring func.value to be an ast.Attribute; modify the condition around the
write-detection (the block that builds offenders and uses func, func.attr,
func.value, and node.lineno) to also accept func.value being an ast.Name with id
in {"stdout","stderr"} (in addition to the existing ast.Attribute case), so
calls like stdout.write(...) or stderr.write(...) are appended to offenders the
same way as sys.stdout.write(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d70f746a-508b-407c-98a8-1ce82142b2ba
📒 Files selected for processing (2)
tests/tools/test_agent_tool.pytests/ui_and_conv/test_md_render_authority.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/ui_and_conv/test_md_render_authority.py (1)
23-38: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename variables to camelCase per repository guidelines.
Variables throughout this function (
offenders,tree,node,func,target,is_std_stream) use snake_case. As per coding guidelines, Python code in this repository must use camelCase for variable names.♻️ Suggested refactor
`@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}" + astTree = ast.parse((_SRC / filename).read_text()) + offenderList: list[str] = [] + for astNode in ast.walk(astTree): + if isinstance(astNode, ast.Call): + funcObj = astNode.func + if isinstance(funcObj, ast.Name) and funcObj.id == "print": + offenderList.append(f"print() at line {astNode.lineno}") + if isinstance(funcObj, ast.Attribute) and funcObj.attr == "write": + targetVal = funcObj.value + isStdStream = ( + isinstance(targetVal, ast.Attribute) and targetVal.attr in {"stdout", "stderr"} + ) or (isinstance(targetVal, ast.Name) and targetVal.id in {"stdout", "stderr"}) + if isStdStream: + offenderList.append(f"std*.write at line {astNode.lineno}") + assert not offenderList, f"{filename} bypasses the screen model: {offenderList}"As per coding guidelines, "**/*.{py,ts,tsx,js,jsx}: Use camelCase for variable names in Python and TypeScript code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui_and_conv/test_md_render_authority.py` around lines 23 - 38, Rename local variables in test_no_direct_terminal_writes_in_renderer to use camelCase per repo rules: change tree -> astTree (or similar), offenders -> offenderList, node -> currentNode, func -> funcNode, target -> targetNode, and is_std_stream -> isStdStream (update any related type hints like offenders: list[str] to offenderList: list[str]); update every occurrence within the function and the final assert message to use the new names so logic (ast.walk, isinstance checks, and appended messages) remains identical.tests/ui_and_conv/test_md_color_contract.py (1)
22-48: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename variables to camelCase per repository guidelines.
Variables throughout this test function (
colors,md,coloured,inline_fg,frame_char,found_count,start,idx,window) use snake_case. As per coding guidelines, Python code must use camelCase for variable names.♻️ Suggested refactor
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, ( + themeColors = get_markdown_colors("dark") + assert themeColors.code_block_border != themeColors.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) + markdownText = "Here is `inline` and a block:\n\n```python\nx = 1\n```\n" + renderedColoured = render_ansi(pythinker_markdown(markdownText), 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 + inlineFg = _sgr_fg(themeColors.inline_code) + for frameChar in ("╭", "╰", "─"): + foundCount = 0 + startPos = 0 while True: - idx = coloured.find(frame_char, start) - if idx == -1: + charIdx = renderedColoured.find(frameChar, startPos) + if charIdx == -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}" + foundCount += 1 + colorWindow = renderedColoured[max(0, charIdx - 24) : charIdx] + assert inlineFg not in colorWindow, "border frame inherited inline-code color" + startPos = charIdx + 1 + assert foundCount > 0, f"missing expected frame glyph {frameChar!r}"As per coding guidelines, "**/*.{py,ts,tsx,js,jsx}: Use camelCase for variable names in Python and TypeScript code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui_and_conv/test_md_color_contract.py` around lines 22 - 48, Rename the local variables in test_code_block_border_does_not_use_inline_code_color to camelCase: change colors -> themeColors (or similar), md -> markdownText, coloured -> renderedColoured, inline_fg -> inlineFg, frame_char -> frameChar, found_count -> foundCount, start -> startPos, idx -> charIdx, window -> colorWindow; update all usages inside the function including the calls to render_ansi(pythinker_markdown(...)) and the call to _sgr_fg(colors.inline_code) (use themeColors.inline_code) so identifiers match the new names and assertions/messages use the camelCase names (preserve the function name and test logic).tests/ui_and_conv/test_md_table_contract.py (1)
20-117: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename variables to camelCase per repository guidelines.
All variables across the test functions (
md,out,lines,code_row,plain_row,long_cell,stripped_cell,stripped_out,width) use snake_case. As per coding guidelines, Python code must use camelCase for variable names.♻️ Suggested refactor for sample test functions
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()] + markdownText = "| Expr | Meaning |\n| --- | --- |\n| `a | b` | bitwise or |\n| plain | text |\n" + renderedOut = render_plain(pythinker_markdown(markdownText), width=80) + outputLines = [line for line in renderedOut.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] + assert any("Expr" in line and "Meaning" in line for line in outputLines) + codeRow = [line for line in outputLines if "a | b" in line and "bitwise or" in line] + plainRow = [line for line in outputLines if "plain" in line and "text" in line] + assert codeRow + assert plainRow + assert codeRow[0] != plainRow[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 + markdownText = "| Col |\n| --- |\n| a \\| b |\n" + renderedOut = render_plain(pythinker_markdown(markdownText), width=80) + assert "a | b" in renderedOut # literal pipe preserved + assert "a \\| b" not in renderedOut + assert "Col" in renderedOutApply similar camelCase renames to all remaining test functions in this file.
As per coding guidelines, "**/*.{py,ts,tsx,js,jsx}: Use camelCase for variable names in Python and TypeScript code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui_and_conv/test_md_table_contract.py` around lines 20 - 117, The tests use snake_case local variable names which violate the repo's camelCase rule; in each test function (e.g., test_table_with_piped_inline_code_keeps_columns, test_table_with_escaped_pipes_keeps_literal_pipe, test_escape_code_span_pipes_*, test_table_empty_header_cell_does_not_mislabel, test_table_long_cell_wraps_without_dropping_content) rename locals like md, out, lines, code_row, plain_row, long_cell, stripped_cell, stripped_out, and the parametrized width variable to camelCase equivalents (e.g., md -> markdownInput, out -> renderedOut, lines -> nonEmptyLines, code_row -> codeRow, plain_row -> plainRow, long_cell -> longCell, stripped_cell -> strippedCell, stripped_out -> strippedOut, width -> testWidth) and update all uses and assertions accordingly; ensure helper reference to _escape_code_span_pipes stays intact and only local variable identifiers are changed, running tests to confirm no behavior change.tests/ui_and_conv/test_md_stream_idempotency.py (1)
10-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename variables to camelCase per repository guidelines.
All variables across the module (
chunks,stream,committed,ready,tail,full,slice_,reassembled,rendered,md,first,second) use snake_case. As per coding guidelines, Python code must use camelCase for variable names.♻️ Suggested refactor for _drain and test_streaming_table_is_not_committed_mid_row
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 + mdStream = PythinkerMarkdownStream() + committedSlices: list[str] = [] + for chunkText in chunks: + readyText = mdStream.push(chunkText) + if readyText: + committedSlices.append(readyText) + tailText = mdStream.flush() + if tailText: + committedSlices.append(tailText) + return committedSlices 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" + fullMarkdown = "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)) + committedSlices = _drain(list(fullMarkdown)) # 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" + for sliceText in committedSlices[:-1]: + if "---" in sliceText: + assert "| 1 | 2 |" in sliceText, "a header-only table was committed before data arrived" # Reassembled stream equals the original (no loss, no duplication). - assert "".join(committed) == full + assert "".join(committedSlices) == fullMarkdownApply similar camelCase renames to
test_h2_stream_slices_reassemble_without_duplicate_rowsandtest_h3_report_and_table_render_is_idempotent.As per coding guidelines, "**/*.{py,ts,tsx,js,jsx}: Use camelCase for variable names in Python and TypeScript code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui_and_conv/test_md_stream_idempotency.py` around lines 10 - 69, The reviewer wants all local variable names switched from snake_case to camelCase: rename variables in _drain (chunks -> chunksList, stream -> streamObj, committed -> committedList, ready -> readySlice, tail -> tailSlice), in test_streaming_table_is_not_committed_mid_row (full -> fullText, slice_ -> sliceText), in test_h2_stream_slices_reassemble_without_duplicate_rows (reassembled -> reassembledText, rendered -> renderedText), and in test_h3_report_and_table_render_is_idempotent (md -> markdownText, first -> firstRender, second -> secondRender); update every use of these symbols inside the functions _drain, test_streaming_table_is_not_committed_mid_row, test_h2_stream_slices_reassemble_without_duplicate_rows, and test_h3_report_and_table_render_is_idempotent to the new camelCase names and run tests to ensure no name collisions or missed references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/ui_and_conv/test_md_color_contract.py`:
- Around line 22-48: Rename the local variables in
test_code_block_border_does_not_use_inline_code_color to camelCase: change
colors -> themeColors (or similar), md -> markdownText, coloured ->
renderedColoured, inline_fg -> inlineFg, frame_char -> frameChar, found_count ->
foundCount, start -> startPos, idx -> charIdx, window -> colorWindow; update all
usages inside the function including the calls to
render_ansi(pythinker_markdown(...)) and the call to _sgr_fg(colors.inline_code)
(use themeColors.inline_code) so identifiers match the new names and
assertions/messages use the camelCase names (preserve the function name and test
logic).
In `@tests/ui_and_conv/test_md_render_authority.py`:
- Around line 23-38: Rename local variables in
test_no_direct_terminal_writes_in_renderer to use camelCase per repo rules:
change tree -> astTree (or similar), offenders -> offenderList, node ->
currentNode, func -> funcNode, target -> targetNode, and is_std_stream ->
isStdStream (update any related type hints like offenders: list[str] to
offenderList: list[str]); update every occurrence within the function and the
final assert message to use the new names so logic (ast.walk, isinstance checks,
and appended messages) remains identical.
In `@tests/ui_and_conv/test_md_stream_idempotency.py`:
- Around line 10-69: The reviewer wants all local variable names switched from
snake_case to camelCase: rename variables in _drain (chunks -> chunksList,
stream -> streamObj, committed -> committedList, ready -> readySlice, tail ->
tailSlice), in test_streaming_table_is_not_committed_mid_row (full -> fullText,
slice_ -> sliceText), in test_h2_stream_slices_reassemble_without_duplicate_rows
(reassembled -> reassembledText, rendered -> renderedText), and in
test_h3_report_and_table_render_is_idempotent (md -> markdownText, first ->
firstRender, second -> secondRender); update every use of these symbols inside
the functions _drain, test_streaming_table_is_not_committed_mid_row,
test_h2_stream_slices_reassemble_without_duplicate_rows, and
test_h3_report_and_table_render_is_idempotent to the new camelCase names and run
tests to ensure no name collisions or missed references.
In `@tests/ui_and_conv/test_md_table_contract.py`:
- Around line 20-117: The tests use snake_case local variable names which
violate the repo's camelCase rule; in each test function (e.g.,
test_table_with_piped_inline_code_keeps_columns,
test_table_with_escaped_pipes_keeps_literal_pipe, test_escape_code_span_pipes_*,
test_table_empty_header_cell_does_not_mislabel,
test_table_long_cell_wraps_without_dropping_content) rename locals like md, out,
lines, code_row, plain_row, long_cell, stripped_cell, stripped_out, and the
parametrized width variable to camelCase equivalents (e.g., md -> markdownInput,
out -> renderedOut, lines -> nonEmptyLines, code_row -> codeRow, plain_row ->
plainRow, long_cell -> longCell, stripped_cell -> strippedCell, stripped_out ->
strippedOut, width -> testWidth) and update all uses and assertions accordingly;
ensure helper reference to _escape_code_span_pipes stays intact and only local
variable identifiers are changed, running tests to confirm no behavior change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13cc31ff-368f-401c-b2ca-5da39c5421b3
📒 Files selected for processing (5)
docs/superpowers/plans/2026-05-29-tui-markdown-report-contract-hardening.mdtests/ui_and_conv/test_md_color_contract.pytests/ui_and_conv/test_md_render_authority.pytests/ui_and_conv/test_md_stream_idempotency.pytests/ui_and_conv/test_md_table_contract.py
Member
Author
|
Follow-up on CodeRabbit review for b8eec2c:
Local validation: |
CodeRabbit was suggesting camelCase for Python identifiers on .py files, sourced from an over-broad learning. The convention across the entire codebase is snake_case per PEP 8; ruff selects no pep8-naming (N) rules and neither CONTRIBUTING.md nor AGENTS.md mandate camelCase. Add an explicit **/*.py path instruction so the false positive stops recurring; camelCase remains scoped to TypeScript/JavaScript sources.
This was referenced May 30, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.pythinker/reports/report output in the default agent promptValidation
uv run ruff format --check tests/core/test_default_agent.py src/pythinker_code/tools/file/replace.py tests/tools/test_str_replace_file.pyuv run ruff check tests/core/test_default_agent.py src/pythinker_code/tools/file/replace.py tests/tools/test_str_replace_file.pyuv run pyright src/pythinker_code/tools/file/replace.pyuv run pytest tests/core/test_agent_spec.py tests/core/test_default_agent.py::test_default_agent tests/tools/test_str_replace_file.py -quv run pytest tests/ui_and_conv -qgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests