Skip to content

Commit c6d3bc0

Browse files
committed
fix: address CodeRabbit review findings on web allowlist and markdown
- config.py: reject `allowed_domains` entries that are dots-only (would normalize to empty → silently unrestricted) and any entry containing whitespace (newlines previously slipped past the space/tab check). - tools/web/_allowlist.py: strip trailing dots when normalizing entries so `example.com.` matches `example.com` hosts. - tools/web/search.py + ui/shell/tool_renderers/web.py: emit a structured `returned_results=0` signal on the all-filtered path and have the search renderer prefer it, so an all-filtered result reports "0 results" instead of misreading the prose notice as one result. - ui/shell/components/markdown.py: prefix rebuilt table rows with the captured delimiter-line indent so normalization never promotes an indented table to top level (defensive; the guard already bails on non-empty indent today). - agents/default/system.md: reword the code-fence guidance to use inline code spans for language names (markdownlint MD038). - Tests: reject `.`/whitespace allowlist entries, trailing-dot entry matching, and an all-results-filtered renderer regression; refresh the default-agent system-prompt snapshot.
1 parent f348913 commit c6d3bc0

9 files changed

Lines changed: 53 additions & 10 deletions

File tree

src/pythinker_code/agents/default/system.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ Your responses are rendered as Markdown in a terminal. Emit well-formed Markdown
267267

268268
- **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table.
269269
- Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells.
270-
- **Code fences are for code only.** Use triple-backtick blocks (tagged with the language, e.g. ```python, ```toml) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly.
270+
- **Code fences are for code only.** Use triple-backtick blocks tagged with a language (for example, `python` or `toml`) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly.
271271
- **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports.
272272

273273
# Ultimate Reminders

src/pythinker_code/config.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,12 @@ def _validate_allowed_domains(cls, value: list[str] | None) -> list[str] | None:
240240
"Remove it, or omit allowed_domains entirely to leave web access "
241241
"unrestricted."
242242
)
243-
if any(char in cleaned for char in "/: \t"):
243+
if cleaned.strip(".") == "":
244+
raise ValueError(
245+
f"Invalid allowed_domains entry {entry!r}: hostname must contain "
246+
"domain labels, not only dots."
247+
)
248+
if any(char.isspace() for char in cleaned) or any(char in cleaned for char in "/:"):
244249
raise ValueError(
245250
f"Invalid allowed_domains entry {entry!r}: use a bare hostname "
246251
"like 'example.com', not a URL, path, or host:port."

src/pythinker_code/tools/web/_allowlist.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55

66
def _normalize(entry: str) -> str:
7-
return entry.strip().lstrip(".").lower()
7+
return entry.strip().strip(".").lower()
88

99

1010
def host_in_allowlist(host: str | None, allowed: list[str] | None) -> bool:

src/pythinker_code/tools/web/search.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,9 @@ async def __call__(self, params: Params) -> ToolReturnValue:
159159
if dropped:
160160
builder.extras(allowlist_filtered=dropped)
161161
if not results:
162+
# Structured zero-result signal so the renderer reports "0
163+
# results" instead of misreading the prose below as one result.
164+
builder.extras(returned_results=0)
162165
return builder.ok(
163166
f"All {dropped} search result(s) were outside the configured "
164167
"web allowlist and have been omitted.",

src/pythinker_code/ui/shell/components/markdown.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -404,8 +404,11 @@ def _normalize_table_block(text: str) -> str:
404404
tail = text[match.end() :]
405405

406406
# The delimiter must start its own line — guards against inline ``|-|``.
407-
line_prefix = head[head.rfind("\n") + 1 :]
408-
if n_cols < 2 or line_prefix.strip() != "":
407+
# Any leading whitespace is the table's indentation (e.g. nested under a
408+
# list item); preserve it when re-emitting so we never promote an
409+
# indented table to top level.
410+
indent = head[head.rfind("\n") + 1 :]
411+
if n_cols < 2 or indent.strip() != "":
409412
out += text[: match.end()]
410413
text = tail
411414
continue
@@ -457,10 +460,10 @@ def _normalize_table_block(text: str) -> str:
457460
# paragraph), so ensure one before emitting the header.
458461
if out and not out.endswith("\n\n"):
459462
out += "\n" if out.endswith("\n") else "\n\n"
460-
out += "| " + " | ".join(header_cells) + " |\n"
461-
out += "| " + " | ".join(markers) + " |\n"
463+
out += f"{indent}| " + " | ".join(header_cells) + " |\n"
464+
out += f"{indent}| " + " | ".join(markers) + " |\n"
462465
for row in data_rows:
463-
out += "| " + " | ".join(row) + " |\n"
466+
out += f"{indent}| " + " | ".join(row) + " |\n"
464467

465468
remainder = "\n".join(tail_lines[consumed:])
466469
if not remainder.strip():

src/pythinker_code/ui/shell/tool_renderers/web.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,14 @@ def _allowlist_filtered_count(result: ToolResultPayload) -> int:
185185
return 0
186186

187187

188+
def _explicit_result_count(result: ToolResultPayload) -> int | None:
189+
"""The tool's own result count, if it emitted one (preferred over text)."""
190+
extras_raw = result.details.get("extras")
191+
extras = cast("dict[str, object]", extras_raw) if isinstance(extras_raw, dict) else {}
192+
value = extras.get("returned_results")
193+
return value if isinstance(value, int) and value >= 0 else None
194+
195+
188196
def _render_search_result(
189197
ctx: ToolRenderContext, result: ToolResultPayload
190198
) -> RenderableType | None:
@@ -203,7 +211,8 @@ def _render_search_result(
203211
return Group(body, fg("muted", f"... ({remaining} more lines, ctrl+o to expand)"))
204212
return body
205213

206-
count = _search_result_count(result.text)
214+
explicit = _explicit_result_count(result)
215+
count = explicit if explicit is not None else _search_result_count(result.text)
207216
summary = Text()
208217
summary.append("Found ", style=tui_rich_style("tool_output"))
209218
summary.append(str(count), style=tui_rich_style("tool_title"))

tests/core/test_default_agent.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ async def test_default_agent(runtime: Runtime):
275275
276276
- **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table.
277277
- Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells.
278-
- **Code fences are for code only.** Use triple-backtick blocks (tagged with the language, e.g. ```python, ```toml) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly.
278+
- **Code fences are for code only.** Use triple-backtick blocks tagged with a language (for example, `python` or `toml`) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly.
279279
- **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports.
280280
281281
# Ultimate Reminders

tests/tools/test_web_allowlist.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
("DOCS.Example.COM", [" .Example.com "], True),
3131
# Trailing-dot FQDN host.
3232
("docs.example.com.", ["example.com"], True),
33+
# Trailing-dot allowlist *entry* matches a plain host.
34+
("docs.example.com", ["example.com."], True),
35+
("example.com", ["example.com."], True),
3336
# Multiple entries: match any.
3437
("foo.org", ["example.com", "foo.org"], True),
3538
# Empty / None host with a non-empty allowlist is rejected.
@@ -53,6 +56,9 @@ def test_web_config_accepts_bare_hostnames() -> None:
5356
"example.com/path", # path
5457
"example.com:8080", # port
5558
"two words.com", # whitespace
59+
".", # dots-only would normalize to empty (unrestricted) — reject
60+
"..", # dots-only
61+
"ex\nample.com", # newline whitespace must be rejected too
5662
],
5763
)
5864
def test_web_config_rejects_malformed_entries(bad_entry: str) -> None:

tests/ui_and_conv/test_tui_card_tool_renderers.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -928,6 +928,23 @@ def test_search_no_allowlist_indicator_when_not_filtered():
928928
assert "filtered to allowlist" not in rendered
929929

930930

931+
def test_search_all_results_filtered_reports_zero():
932+
# When every result is dropped by the allowlist, SearchWeb emits prose plus a
933+
# structured returned_results=0 signal; the renderer must prefer that count
934+
# instead of misreading the one-line prose as a single result.
935+
rendered = _render(
936+
"SearchWeb",
937+
{"query": "python"},
938+
output=(
939+
"All 2 search result(s) were outside the configured web allowlist "
940+
"and have been omitted."
941+
),
942+
details={"extras": {"allowlist_filtered": 2, "returned_results": 0}},
943+
)
944+
assert "Found 0 results" in rendered
945+
assert "2 filtered to allowlist" in rendered
946+
947+
931948
# ---------------------------------------------------------------------------
932949
# Background tasks
933950
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)