feat: web domain allowlist and markdown report rendering - #15
Merged
Conversation
…ve tool feedback Introduces two new wire protocol events so the UI can distinguish the approval/hooks phase from actual tool execution, and stream incremental shell output before the final ToolResult arrives. - ToolExecutionStarted: emitted once per tool call after approval completes, before the tool body runs; file/MCP/external tools emit from the approval path while Shell and RunAgents emit it themselves at the right moment - ToolOutputPart: streamed from the Shell tool as stdout/stderr lines arrive TUI blocks now hold the execution-started spinner until ToolExecutionStarted lands (showing a calm "preparing" row before that), render streamed output as a live tail preview, and the Bash card shows "running" status when partial output is present. The composing _ContentBlock also gains a live Markdown preview while the model writes.
…n agents; pad markdown code blocks explore.yaml: require supplemental ruff checks (e.g. --select C901) to be labeled "outside project lint policy" so callers can distinguish enforced violations from advisory findings. plan.yaml: add context-gate rule to verify any lint/complexity finding is in the project's active select list before proposing a refactor — prevents plans driven by rules the project intentionally does not enforce. markdown.py: yield a blank_row() above and below each bordered code block so panels read as distinct sections rather than crowding surrounding prose. Test added to assert the blank-row framing is present.
…CallBlock Add three new state fields (_subagent_output_parts, _subagent_output_had_stderr, _subagent_execution_started), two new public methods (mark_sub_execution_started, append_sub_output_part), and cleanup in finish_sub_tool_call for the new fields. Also update append_sub_tool_call and append_sub_tool_call_part to recompose after mutations, readying the block for live subagent tool streaming in Task 2.
The committed explore.yaml carries lint/complexity-policy guidance (do not flag rules such as C901 that are absent from the project's configured rule set) that its inline snapshot in test_agent_spec.py never received, so the test failed on a clean checkout. Regenerate the snapshot to match HEAD.
- Frame fenced code blocks with a blank row above and below so they read as a distinct section instead of crowding surrounding prose. - Add a conservative pre-parse normalizer that repairs malformed GFM tables the model sometimes emits (header glued to prose, a blank line before the |---| delimiter, data rows crammed onto the delimiter line) which markdown-it would otherwise render as raw text. Anchored on the delimiter row, it rebuilds only regions whose header and data cell counts validate, preserves column alignment, and leaves ambiguous or fenced content untouched.
- Add an Output Formatting section to the default system prompt: emit
well-formed Markdown tables, reserve code fences for actual code (never
wrap prose reports, finding lists, or ASCII boxes in a fence), and use
status icons sparingly. This stops the model from producing the malformed
tables and emoji-laden boxes that rendered poorly in the TUI.
- Reword the Date and Time section so the injected ${PYTHINKER_NOW} is
framed as the authoritative present, anchoring the agent's sense of 'now',
recency, and 'latest' to it instead of a training-era year.
- Refresh the prompt snapshot and add coverage for both.
# Conflicts: # src/pythinker_code/config.py # src/pythinker_code/tools/web/fetch.py # tests/core/test_config.py
…into unified branch # Conflicts: # docs/superpowers/plans/2026-05-26-agent-live-tool-stream.md # src/pythinker_code/ui/shell/components/markdown.py # src/pythinker_code/ui/shell/visualize/_blocks.py # src/pythinker_code/ui/shell/visualize/_live_view.py # tests/ui/test_shell_markdown.py # tests/ui_and_conv/test_tool_call_block.py
Contributor
📝 WalkthroughWalkthroughAdds a top-level ChangesWeb Domain Allowlist Feature
Markdown Table Normalization
System Prompt and Documentation Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
Heads-up for review: the domain allowlist is enforced on the local-HTTP fetch path ( |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/tools/test_fetch_url.py (1)
20-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid module-wide mocking of private validator internals.
This autouse patch couples tests to
_validate_fetch_urlinternals and can mask real behavior regressions. Prefer exercising observable behavior via tool inputs/config and targeted local test servers.As per coding guidelines,
tests/**/*.py: Flag tests that mock internal implementation details rather than observable behavior.🤖 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/tools/test_fetch_url.py` around lines 20 - 28, The autouse fixture _bypass_ssrf_validation is patching the private function fetch_module._validate_fetch_url which couples tests to internals; replace it by removing the module-wide monkeypatch and instead make tests exercise observable behavior: start a local test server or use configured allowed-hosts/allowlist inputs and call the public fetch API, or if a bypass is absolutely needed create a non-autouse, test-scoped fixture that overrides only the public configuration/parameter that controls SSRF validation (e.g., pass an explicit allowlist or use a public setter) so you no longer monkeypatch fetch_module._validate_fetch_url directly; update tests to use that fixture or server accordingly.
🤖 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 `@src/pythinker_code/agents/default/system.md`:
- Line 270: The inline code span in the sentence beginning "Code fences are for
code only." contains spaced triple-backticks (e.g. "python, ```toml") which
triggers MD038; fix it by rewriting that fragment to use proper inline code
spans for language names (e.g. `python` and `toml`) and remove any embedded
backticks or spaces inside a single code span so the sentence reads like: Use
triple-backtick blocks tagged with a language (for example, `python` or `toml`)
solely for source, config, or commands.
In `@src/pythinker_code/config.py`:
- Around line 235-247: The current allowed_domains loop accepts entries like "."
and misses other whitespace; update the validation in the loop that processes
each entry (the cleaned variable used in that loop) to: after cleaned =
entry.strip() also reject entries that are empty or consist only of dots (e.g.,
cleaned == "" or all(ch == "." for ch in cleaned)) and reject any entry
containing any whitespace character using char.isspace() rather than only
checking " " and "\t"; keep the existing URL/port/path checks (e.g., characters
'/' or ':') but replace the whitespace check with a char.isspace() check and
update the ValueError messages accordingly so entries like "." or newline-only
strings are refused.
In `@src/pythinker_code/tools/web/_allowlist.py`:
- Around line 6-7: The _normalize function currently only removes leading dots
and whitespace which causes entries like "example.com." not to match
"example.com"; update _normalize(entry: str) to trim whitespace and strip both
leading and trailing dots (e.g., use entry.strip().strip(".").lower() or
equivalent) so that entries with trailing dots are normalized the same as
without them; keep the lowercasing behavior and reference the _normalize
function when making the change.
In `@src/pythinker_code/tools/web/search.py`:
- Around line 162-166: The all-filtered branch currently returns only prose via
builder.ok, which downstream rendering can miscount; update the return to
include a structured zero-result signal (e.g. add an extras or metadata field
like extras.returned_results = 0 or returned_results=0) alongside the existing
message and brief so renderers can prefer the explicit count; locate the branch
that calls builder.ok (the block returning "All {dropped} search result(s) ...")
and augment that builder.ok call to include the extras/returned_results field
set to 0.
In `@src/pythinker_code/ui/shell/components/markdown.py`:
- Around line 406-468: The code strips the original leading indentation
(captured in line_prefix) when re-emitting the normalized GFM table, which moves
indented tables to top-level; update the emission so every emitted table line
(header, marker row, and each data row) is prefixed with line_prefix (i.e.,
replace the out += "| " + ... joins with out += line_prefix + "| " + ...), and
ensure the blank-line insertion respects the same indentation by inserting
line_prefix + "\n" or line_prefix + "\n\n" as appropriate so the reconstructed
table preserves its original left padding; look for the variables line_prefix,
header_cells, markers, data_rows and the places that append header/marker/row
strings to out.
In `@tasks/todo.md`:
- Line 506: Update the typo in the tasks/todo.md entry: change the word
"unparseable" to "unparsable" in the string "fail-closed on unparseable hosts;
allowlist-before-DNS ordering." so the typo checker passes; locate and edit that
exact phrase in the file and commit the corrected spelling.
In `@tests/tools/test_web_allowlist_tools.py`:
- Line 13: The test is reaching into internal implementation by calling
_validate_fetch_url and patching new_client_session; instead, change the test to
exercise the public FetchURL behavior: configure a controlled allowlist/denylist
via the same config used by FetchURL, run a small local test HTTP server (or a
fixture) and assert FetchURL.fetch (or the public method used) accepts or
rejects URLs as expected; remove direct calls to _validate_fetch_url and the
new_client_session patch so the test validates observable behavior rather than
internal implementation.
In `@tests/ui_and_conv/test_tui_card_tool_renderers.py`:
- Around line 911-929: Add a regression test function in
tests/ui_and_conv/test_tui_card_tool_renderers.py that covers the "all results
filtered" payload branch: reuse the _render helper (as in
test_search_shows_allowlist_filtered_indicator) but pass an empty output and
details with extras indicating allowlist_filtered count and an
all-results-filtered flag (e.g., extras={"allowlist_filtered": X,
"all_results_filtered": True}); assert the rendered output shows zero results
(e.g., contains "Found 0 result" or "Found 0 results") and includes the UI
indicator/message for all-results filtered (e.g., contains "filtered to
allowlist" or an "all results filtered" phrasing) to lock in correct zero-result
rendering.
---
Outside diff comments:
In `@tests/tools/test_fetch_url.py`:
- Around line 20-28: The autouse fixture _bypass_ssrf_validation is patching the
private function fetch_module._validate_fetch_url which couples tests to
internals; replace it by removing the module-wide monkeypatch and instead make
tests exercise observable behavior: start a local test server or use configured
allowed-hosts/allowlist inputs and call the public fetch API, or if a bypass is
absolutely needed create a non-autouse, test-scoped fixture that overrides only
the public configuration/parameter that controls SSRF validation (e.g., pass an
explicit allowlist or use a public setter) so you no longer monkeypatch
fetch_module._validate_fetch_url directly; update tests to use that fixture or
server accordingly.
🪄 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: 1e76bf5c-4cb9-45f6-ab92-809dce188d60
📒 Files selected for processing (21)
docs/en/configuration/config-files.mdsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/config.pysrc/pythinker_code/tools/web/_allowlist.pysrc/pythinker_code/tools/web/fetch.mdsrc/pythinker_code/tools/web/fetch.pysrc/pythinker_code/tools/web/search.mdsrc/pythinker_code/tools/web/search.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/tool_renderers/web.pytasks/todo.mdtests/core/test_config.pytests/core/test_default_agent.pytests/core/test_load_agent.pytests/tools/test_fetch_url.pytests/tools/test_tool_descriptions.pytests/tools/test_web_allowlist.pytests/tools/test_web_allowlist_tools.pytests/ui/test_shell_markdown.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/utils/test_pyinstaller_utils.py
- 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.
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)
src/pythinker_code/tools/web/search.py (1)
159-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the “filtered by allowlist” return with
dropped > 0.This branch also runs when the backend returns zero results (so
dropped == 0), which produces a misleading “outside allowlist” message.Proposed fix
- if not results: + if dropped > 0 and not results: # Structured zero-result signal so the renderer reports "0 # results" instead of misreading the prose below as one result. builder.extras(returned_results=0) return builder.ok( f"All {dropped} search result(s) were outside the configured " "web allowlist and have been omitted.", brief="Filtered by allowlist", )🤖 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 `@src/pythinker_code/tools/web/search.py` around lines 159 - 169, The current branch returns a "Filtered by allowlist" message whenever results is empty, even if no items were dropped (dropped == 0); update the conditional so the allowlist-filtered return only happens when dropped > 0 and results is empty. Locate the block using the variables dropped and results and the calls builder.extras(...) and builder.ok(...), and change the logic to first set builder.extras(returned_results=0) when not results and then, if dropped > 0, call builder.extras(allowlist_filtered=dropped) and return the builder.ok(...) filtered-by-allowlist message; otherwise return the generic zero-results response.
🤖 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 `@src/pythinker_code/tools/web/search.py`:
- Around line 159-169: The current branch returns a "Filtered by allowlist"
message whenever results is empty, even if no items were dropped (dropped == 0);
update the conditional so the allowlist-filtered return only happens when
dropped > 0 and results is empty. Locate the block using the variables dropped
and results and the calls builder.extras(...) and builder.ok(...), and change
the logic to first set builder.extras(returned_results=0) when not results and
then, if dropped > 0, call builder.extras(allowlist_filtered=dropped) and return
the builder.ok(...) filtered-by-allowlist message; otherwise return the generic
zero-results response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0753b562-b993-4438-9f1a-94ef3fbe7973
📒 Files selected for processing (9)
src/pythinker_code/agents/default/system.mdsrc/pythinker_code/config.pysrc/pythinker_code/tools/web/_allowlist.pysrc/pythinker_code/tools/web/search.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/tool_renderers/web.pytests/core/test_default_agent.pytests/tools/test_web_allowlist.pytests/ui_and_conv/test_tui_card_tool_renderers.py
This was referenced May 30, 2026
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
Unifies two previously-local feature branches onto current
mainin a single PR:feat/web-allowed-domains) — adds a configurableweb.allowed_domainspolicy enforced byFetchURLandSearchWeb. A newtools/web/_allowlist.pyprovides label-aware, case-insensitive subdomain matching; an unconfigured (empty/None) allowlist imposes no restriction. Fetch validates the initial URL before opening any session and re-validates every redirect hop against both the allowlist and the existing SSRF guard. Search filters results to allowed hosts.fix/markdown-rendering-and-date-awareness) — improves TUI rendering of model-generated reports: normalizes glued/crammed GFM tables, leaves inline pipes and fenced code untouched, and renders priority-matrix code blocks as grouped rows.Merge notes
Both branches were far behind
main; conflicts were resolved by integrating, not overwriting:tools/web/fetch.py: combined the allowlist check and the SSRF IP check into the single_validate_fetch_url, and threadedallowed_domainsthrough the SSRF-safe redirect helper so each hop is checked for both.ui/shell/components/markdown.py: kept both table-fixing passes —_repair_crammed_markdown_tables(heading-glued-to-header) then_normalize_markdown_tables(general glued-table rebuild).mainwere kept; the older stubbed versions from the feature branch were dropped.hiddenimportssnapshot for the new_allowlistmodule.Verification
ruff check+ruff format --check: cleanpyright: 0 errorstests/green (incl. web allowlist, markdown rendering, pyinstaller)tests_e2e/greenSummary by CodeRabbit
New Features
Improvements
Tests & Docs