feat(tooling-ux): minimal 3-tool surface with progressive discovery + escalation

[protostatis]

Minimal 3 (search/open/extract) + help for progressive discovery of 32.

• Python SmartClient: Brave search (DDG fallback), navigate_auto (bounded discover/cards, relative href via _last_url), stable escalation taxonomy (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss) with category/severity/retryable/evidence, external_action for chrome_escalation, rephrased cookies_set language, fixed ThreadPool timeout bounding.
• Rust: --mcp-profile minimal|full static contracts (minimal: navigate/extract/help/query), help tool grouped catalog, sensitive language rephrase.
• Harness: scripts/site_matrix.py sanitized 9-site matrix (slickdeals, engadget, rei, finance, github) with bounded 12s, sanitized URLs.

Live: Slickdeals $150 partial_result, Engadget 9.2/10 null, REI thin_shell, 7 independent LLM agents with minimal 4-line instruction all succeeded via search->open.

Advisor: gpt-5-6-advisor reviewed strategy, recommends static minimal/full vs dynamic unlocking, portable Rust facts + host actions.

[protostatis]

Sky's Code Review

This PR introduces a 'minimal 3-tool' progressive-discovery surface: a Python SmartClient (python/unbrowser/smart.py) wrapping Brave HTML search with DDG fallback, plus navigate_auto that runs bounded discover/cards/page_model enrichment and emits a structured, portable escalation taxonomy (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss). It also adds a Rust --mcp-profile minimal|full static contract, a grouped help tool, a rephrased cookies_set description, and a sanitized 9-site harness (scripts/site_matrix.py). The architecture is sound and the sanitization discipline (no cookies/headers/body emitted, no query tokens) is good. Main concerns are reliability/resource issues in the Python wrapper: a ThreadPoolExecutor is constructed per enrichment call and abandoned worker threads are not reaped on timeout (the code comments acknowledge this), and exception handling in _brave_api_extract/_brave_html_extract swallows errors too broadly. These are non-blocking but worth tightening before broad rollout.

Verdict: Comment

Comments

• The escalation taxonomy is well-designed and the reason codes are stable and portable (Rust emits facts, Python maps to host actions), matching the advisor's recommendation. Good separation of concerns.
• Sanitization discipline in the harness is commendable: cookies/headers/full body are never emitted, URLs are stripped of query params, and query strings are public. This is the right stance for an infra repo.
• The cookies_set rephrase (removing 'bypassing bot detection' in favor of 'continue using session state from a user-authorized browser, where permitted, requires explicit user confirmation') is a meaningful safety improvement and should be kept.
• The broad except Exception: pass / except Exception: return None blocks in the extraction helpers and enrichment loop make silent-degradation the default. Given this is a tooling surface for LLM agents, partial/total silent failure may be hard for downstream agents to diagnose. Consider a lightweight structured error field in the bundle even on soft-failure paths.
• The 620-line smart.py adds substantial logic; given it is the reliability-sensitive piece, consider unit tests for _norm_url, _escalation_for_bundle branches, and the timeout path (using a monkeypatched call that sleeps) in addition to the integration harness.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/smart.py:292 — _timed_call constructs a new ThreadPoolExecutor on every enrichment call and, on timeout, abandons a non-daemon worker thread via shutdown(wait=False). Repeated navigate_auto calls (as the harness does across 9 sites) will accumulate orphaned threads until process exit. Consider a single shared executor on SmartClient (or module-level) with a bounded max_workers, created lazily once and shut down on exit.

python/unbrowser/smart.py:300 — On timeout the worker is abandoned 'to be reaped when the process exits' — but Python threads are not reaped and the executor is reconstructed each call, so this leaks a thread per timeout. Use daemon threads or a persistent executor with a hard shutdown on client close to bound resource growth under sustained load/rate-limiting.

python/unbrowser/smart.py:168 — _brave_api_extract catches a bare except Exception: return None, silently masking auth/token failures, malformed responses, and network errors. Returning None triggers fallback to the HTML path, which is acceptable behavior, but the failure reason is invisible. Consider logging or surfacing a subtle diagnostic so operators can tell API failure vs. no-key from real data problems.

python/unbrowser/smart.py:187 — The BRAVE_API path uses urllib urlopen with a fixed 10s timeout and no retry/backoff. Under 429/5xx from the Brave API this simply returns None and falls through to HTML search, defeating the purpose of having an API key. Add a retry with exponential backoff (and honor Retry-After) for 429/5xx before abandoning to the HTML fallback.

python/unbrowser/smart.py:356 — In the challenge branch, several options entries build params using bundle.get('url') (e.g. retry_backoff/retry params url). bundle['url'] can be None/empty when navigation failed before producing a URL (e.g. DNS/TLS failure), yielding a null url param. Guard with bundle.get('url') or '' or omit the param when empty.

python/unbrowser/smart.py:311 — _norm_url(url, self._last_url) relies on self._last_url populated by the base Client. If search() populated _last_url from a search result page and then navigate_auto is called with a relative href, urljoin uses the search-page URL as base — correct, but verify _last_url is updated on every super().navigate (including redirects) and is not stale from a prior unrelated navigation.

src/main.rs:7502 — The help dispatch handler calls the deprecated mcp_tools() (always 'full') rather than mcp_tools_for_profile(&mcp_profile). In minimal mode the help catalog advertises all 32 tools regardless of the configured profile, which undercuts the 'minimal surface + progressive discovery' intent. Wire the active mcp_profile into the help tool result so minimal-mode agents aren't shown tools they actually lack.

scripts/site_matrix.py:150 — search_open mode navigates to hits[0]['url'] and asserts status==200, but does not compare the resolved URL against the sanitized expected host, so a Brave/DDG tracking-redirect or an unexpected redirect could silently pass. Consider asserting the final host matches an allowlisted domain for the entry to catch redirect/tracking regressions.

[protostatis]

Sky's Code Review

This PR adds a 'smart' wrapper layer (unbrowser/smart.py) providing a minimal 3-tool surface (search/open/extract + help) with progressive discovery of the full 32-tool catalog, a stable escalation taxonomy, and Rust-side static --mcp-profile minimal|full contracts with a grouped help catalog. It also adds a sanitized 9-site regression harness (scripts/site_matrix.py). The code is generally well-structured, with sensible fallbacks (Brave→DDG, API→HTML), time-bounded enrichment via threads, and rephrased clearance-cookie language to require explicit user authorization. Several reliability concerns exist: thread-based timeout 'abandonment' leaks worker threads (explicitly acknowledged in a comment) with no upper bound on concurrency, the ThreadPoolExecutor is constructed per-call with no shutdown in the exception path, and one typo ('bypasses'→'bypassing' was fixed but 'shuts' logic and _last_url dependency are unverified). The matrix harness has a subtle correctness gap: the 'open' mode's escalation check treats 'partial_result' as acceptable when None is expected, which silently masks real regressions. No secrets are exposed and the sanitization claims hold. Overall this is approvable with minor comments rather than blocking issues.

Verdict: Comment

Comments

• The clearance-cookie language rephrase (both Rust cookies_set description and Python escalation hint) now correctly requires 'explicit user confirmation and origin-scoped, ephemeral storage' — this aligns with the external_action chrome_escalation requiring user confirmation. Good security posture change.
• The escalation taxonomy (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss) with category/severity/retryable/evidence is a clean, portable design and matches the 'static minimal/full vs dynamic unlocking' advisor recommendation well.
• The architecture comment in _escalation_for_bundle ('Rust emits facts; Python maps to host actions') is a good separation, but the current implementation only reads from the bundle dict — confirm the Rust side actually populates challenge/density/scripts/extract facts into the navigate result for this mapping to be fully live rather than aspirational.
• The matrix harness is genuinely sanitized (no cookies, sanitize_url strips query params, no headers/body emitted) — verified. The 12s bounded timeout and 0.5s inter-request sleep show good host etiquette.
• Consider adding a small test asserting the HELP_CATALOG groups sum to 32 tools and match the Rust mcp_tools list, since the two catalogs are maintained independently and can drift.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/smart.py:1 — The _timed_call docstring states 'On timeout we abandon the worker (Python threads cannot be killed)' — this leaks a thread per timeout. Under sustained heavy-DOM timeouts (the REI/Engadget thin_shell/JS cases hit this path frequently), these orphaned workers accumulate with no bound, each holding a Client reference and network state. Consider using a shared/process-level timeout mechanism or at least documenting the leak explicitly and capping concurrent in-flight timeouts.

python/unbrowser/smart.py:1 — In _timed_call, the generic except Exception path calls ex.shutdown(wait=False, cancel_futures=True) then re-raises — good. But the successful path already shut down wait=True. The TimeoutError path shuts down wait=False. No path leaks the executor, but since a fresh ThreadPoolExecutor(max_workers=1) is created per _timed_call invocation and per enrichment (discover/cards/page_model), this spawns several threads per navigate_auto call. Consider recycling a single module-level executor or using asyncio with the underlying futures if the Client supports it.

python/unbrowser/smart.py:1 — navigate_auto references self._last_url for relative-href resolution via _norm_url but _last_url is never assigned anywhere in this file. If Client doesn't set it on navigate, relative hrefs like '/' will resolve to 'https://' + '/' → 'https:///' (malformed) rather than joining against the current page. Verify Client maintains _last_url on every navigate, or capture nav.get('url') and pass it explicitly as the base.

python/unbrowser/smart.py:1 — _brave_html_extract raises UnbrowserError on non-429/503 statuses but returns raw navigate for brave in the base Client.search. The smart wrapper's ddg/bing delegate to super().search() which returns a dict (navigate result), while brave returns list[dict] — this API shape inconsistency is documented but callers using run() get either shape depending on engine. Acceptable for now, but worth a type-stable return contract.

scripts/site_matrix.py:1 — In 'open' mode, when exp['escalation_reason'] is None and out['escalation'] in (None, 'partial_result'), the entry is marked ok. This means a site that SHOULD return no escalation but instead hits the 16KB extract cap (partial_result) still passes — masking a potential regression where content suddenly grows or auto-extract breaks. Similarly the 'timeout' band-aid marks heavy-DOM timeouts as ok. For a regression harness, these lenient branches reduce its value as a canary; consider splitting 'informational partial_result acceptable' from 'true escalation mismatch' more strictly, or recording a warning field instead of silently passing.

scripts/site_matrix.py:1 — In 'search' mode, a KeyError would be raised on hits[0] if search returns an empty list — but the harness does hit hits[0]['url'] before the len check only in search mode via out['sample_url'] = sanitize_url(hits[0]['url']) if hits else None which is guarded. However in 'search_open' mode, hits may be non-empty but hits[0] may lack 'url' if Brave returned malformed items; an IndexError/KeyError propagates to the generic except and is captured as error, which is acceptable but could be clearer.

src/main.rs:1 — The help tool dispatch builds the catalog synchronously from mcp_tools() (full list) regardless of the active minimal profile, then returns a grouped catalog including tools the minimal profile does not expose (e.g. 'cookies_set'). Since the minimal profile hides these tools from tools/list, help() revealing them is arguably the intended 'progressive discovery' — but help() should reflect the active profile's surface rather than always full, otherwise the minimal/full static contract (per the advisor's recommendation) is violated: minimal agents get full-tool visibility via help immediately.

src/main.rs:1 — parse_mcp_profile_arg accepts any free-form value and only the literal 'minimal' string triggers the minimal profile; anything else (including 'Minimal' is lowercased, but 'minim', 'mcp_minimal', typos) silently falls back to 'full'. Consider validating against an enum and erroring on unknown values to catch CLI typos, consistent with how --shims parses strictly (it uses parse_shim_mode_arg with ? error propagation).

[protostatis]

Sky's Code Review

This PR introduces a 'minimal 3-tool' progressive-discovery surface (search/open/extract + help) layered over the existing unbrowser tooling. It adds a Python SmartClient (Brave/DDG search inference, navigate_auto with bounded enrichment, a stable escalation taxonomy, and a grouped help catalog), a Rust --mcp-profile minimal|full static contract with a help tool, and a sanitized 9-site harness. The design is well-thought-out and the escalation taxonomy (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss) is a genuinely good abstraction. Security-wise the cookies_set language rephrase is a net positive (origin-scoped, requires confirmation, user-authorized framing). Main issues are correctness/reliability concerns: a potential AttributeError on _last_url before the first navigate, the ThreadPoolExecutor timeout not actually interrupting running work (despite the documented rationale, futures cannot be cancelled mid-run), the minimal profile's help tool exposing the full 32-tool catalog through mcp_tools(), and a bare-domain URL heuristic that can misclassify dotted search queries. None are hard security breakers, so this is a 'comment' rather than 'request_changes'.

Verdict: Comment

Comments

• The escalation taxonomy is the strongest part of this change — stable reason codes with category/severity/retryable/evidence and portable Rust-facts→Python-actions mapping is a clean separation of concerns. The 'partial_result' as informational (not escalation) vs. the pre-existing 'extract_truncated' supersede is handled correctly.
• The cookies_set rephrase ('Continue using session state from a user-authorized browser, where permitted... requires explicit user confirmation and origin-scoped, ephemeral storage') is a meaningful security/tone improvement over the prior 'bypassing bot detection' wording.
• _escalation_for_bundle returns a very large inline dict blob for each case; consider extracting these to module-level constants/structs for readability, since several fields (action/options/next_tools) are near-duplicated across the auth/challenge/server_error branches.
• No secrets are introduced (BRAVE_API_KEY is read from env, never logged; harness sanitizes URLs and headers). Confirmed no cookie/header/body leakage in site_matrix.py output.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

self._last_url is read before any navigation has occurred. If the base Client.__init__ does not initialize _last_url (e.g. to None or ''), the very first navigate_auto()/run() call raises AttributeError when _norm_url(url, self._last_url) executes. Guard with getattr(self, '_last_url', None) or confirm the base class always sets it.

[protostatis]

Bounded ThreadPoolExecutor is good, but note fut.cancel() in _timed_call does NOT interrupt an already-running self.call(...) — ThreadPoolExecutor futures cannot be cancelled once running. On timeout the worker keeps executing (possibly a slow network fetch or heavy JS eval) until it finishes on its own. The pool being capped at 3 mitigates thread leak, but a burst of timeouts can still hold all 3 workers busy on abandoned work, stalling subsequent enrichments. Consider a per-worker lower-level timeout or external process isolation if this becomes a real contention source.

[protostatis]

The bare-domain heuristic (p.netloc and '.' in p.netloc and ' ' not in s) will misclassify search queries containing dots and no spaces, e.g. ub.run('best.laptop') or a domain-like query such as foo.bar — it becomes a navigate instead of a search. Consider requiring a known TLD suffix or at least a trailing slash/path before treating a dotted token as a URL, or accept the ambiguity and document it.

[protostatis]

The help dispatch arm builds its catalog from mcp_tools() (the full list), not mcp_tools_for_profile(&mcp_profile). In minimal profile this means the help tool — whose stated purpose is progressive discovery — returns the complete 32-tool list immediately rather than only the minimal surface. If the intent is for minimal-profile agents to truly discover incrementally, the help payload should honor the active profile; otherwise document that help is intentionally a full catalog even in minimal mode.

[protostatis]

In _brave_html_extract, the 429/503 fallback re-fetches DDG via a hardcoded URL rather than delegating to super().search(query, engine='ddg') / _ddg_html_extract consistently. Functionally equivalent, but duplicating the DDG URL construction here (vs. the base class) risks drift if the base DDG endpoint changes. Minor maintainability nit.

[protostatis]

The search_open mode navigates to hits[0]['url'] with no validation that the hit URL is same-origin/expected, and no per-host rate awareness beyond a flat 0.5s sleep. For a public harness this is acceptable, but consider a per-domain dedupe or allowlist so a stray/wrong search result doesn't cause the harness to hit an unintended host.

[protostatis]

Sky's Code Review

This PR introduces a 'minimal 3-tool' progressive-discovery surface layered over an existing headless-browser client. Python gains a new smart.py (SmartClient with search/navigate_auto/help, Brave HTML+API search with DDG fallback, bounded enrichment via a shared ThreadPoolExecutor, and a stable escalation taxonomy) plus a stdio MCP server (smart_mcp.py); Rust gains an --mcp-profile minimal|full flag, a help tool, and rephrased cookies_set language; a sanitized 9-site harness and protocol tests are added. Assessment: overall the design is coherent and the security posture is deliberately tightened (no secrets/cookies in harness, user-confirmation requirement on cookie replay, sensitive-language rephrasing). It is largely a library/tooling change, not prod-infra, so most findings are reliability/correctness comments rather than blockers. The most significant real defect is that the Rust help dispatch path always builds its catalog from the full (unfiltered) tool set regardless of --mcp-profile, defeating the minimal-surface isolation the PR is centered on; there is also a minor unused-variable in the minimal filter. A few robustness issues exist (blocking worker threads not truly cancelled on timeout despite the comment, empty allowed semantics, brittle confidence cast, search return-type override) but none break production. Verdict: comment.

Verdict: Comment

Comments

• Scope note: this is a library/tooling PR (Python smart client, Rust MCP profile flag, a test harness and a stdio MCP server), not conventional infra. Security posture is good: the harness is explicitly sanitized (no cookies/secrets/headers emitted), cookie replay is gated on explicit user confirmation, and sensitive boilerplate language was rephrased. No secrets, unsafe shell, or network-exposure risks identified.
• Progressive-discovery consistency: the Rust minimal set is {navigate, query, extract, help} but the Python framing is 'search/open/extract/help' with query living under the help-unlocked catalog. The two surfaces describe different minimal contracts; aligning them (and making the Rust help respect the active profile) would make the 'minimal 3' story internally consistent.
• Test assertions are somewhat relaxed (e.g. py_count >= 30 and <= 40, full == 33) and the count 33 depends on the current tool inventory — a future tool add/remove will silently break test_minimal_is_4_and_full_is_33. Prefer asserting the invariant (minimal ⊆ full, minimal == {navigate,query,extract,help}) over the exact 33, or derive the expected count from the actual list.
• The _brave_html_extract / eval-result handling (string vs list vs JSON-decode retry) is defensively written but verbose; the inline reasoning comments ('eval returns Python value already JSON-decoded...?') read like unresolved TODO speculation and could be tightened for maintainers.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

The help handler calls mcp_tools() (the full-profile catalog) unconditionally and filters from that, ignoring --mcp-profile. A 'minimal' client invoking help will therefore see the complete 32-tool list, which undermines the progressive-discovery isolation that is the core premise of this PR. It should build from mcp_tools_for_profile(&mcp_profile) (passed into dispatch or captured), consistent with the tools/list path.

[protostatis]

In mcp_tools_for_profile, the minimal boolean is computed but the filter uses a hardcoded allowed list; the minimal variable is only used as the filter guard. This is fine functionally but the naming is confusing (minimal vs allowed). Consider collapsing to a single guard and documenting why query is retained in the minimal set (the comment says 'so agents can probe selectors before escalating', but the Python smart.py minimal contract names only search/open/extract/help, not query — there is an inconsistency between the Rust minimal set {navigate,query,extract,help} and the Python docstring's 'minimal 3' that should be reconciled).

[protostatis]

The comment claims the shared bounded executor avoids a per-call ThreadPool leak and that timed-out workers are 'not abandoned as an orphan'. However fut.result(timeout=...) only raises TimeoutError; fut.cancel() cannot interrupt an already-running blocking call (the underlying self.call/QuickJS work keeps running on the pool thread). The worker is not really cancelled — at most the thread is retained in the bounded pool. The comment overstates the guarantee; consider documenting that cancellation is cooperative/non-preemptive so callers don't assume the underlying work was aborted.

[protostatis]

search() overrides Client.search and changes the return type (list for brave, dict for ddg/bing) with a # type: ignore[override]. The mixed return shape (list vs base dict) is a latent API trap — callers branching on engine get heterogeneous types. Consider making brave/DDG both return the parsed list shape via entry_search/search_brave consistently, or document the asymmetry prominently (the module docstring already mentions it, but the asymmetry is sharp).

[protostatis]

The confidence coercion (float(...) if isinstance(..., (int,float)) else 0.9) is duplicated and brittle — non-numeric or bool values silently become 0.9, which could mis-rank escalation confidence. A small helper (_coerce_confidence) would remove the repetition and make the intent explicit.

[protostatis]

The main() loop reads stdin line-by-line with no handling for a graceful EOF/shutdown; when stdin closes it simply exits the for-loop without calling SmartClient.close(), so the ThreadPoolExecutor is not shut down (relies on GC). Minor: consider a finally that closes _client for deterministic cleanup.

[protostatis]

The market-data URLs (markets.businessinsider.com, finance.yahoo.com) are hit with a hardcoded 0.5s inter-entry sleep and no per-host rate limiting beyond the bounded discover/cards limits. For a regression harness run repeatedly this risks 429s from these hosts; the rate_limited escalation path exists but the harness treats any escalation mismatch other than partial_result/timeout as a hard fail, so transient 429s from public endpoints could produce flaky failures. Consider relaxing strict-mode to tolerate rate_limited for the finance group.

[protostatis]

Sky's Code Review

This PR adds a 'minimal 3-tool' progressive-discovery layer on top of unbrowser: a Python SmartClient (search/open/extract + help, with Brave-HTML search + DDG fallback, bounded auto-discovery via a shared ThreadPoolExecutor, and a stable escalation taxonomy), a Rust --mcp-profile minimal|full static contract with a grouped help tool, a sanitized 9-site harness, and protocol tests. Overall the design is thoughtful and the escalation taxonomy is well-structured. The main issues are (1) a contract/consistency mismatch between the PR's stated '3-tool' framing and the actual surface counts (Python exposes search/open/help while Rust minimal exposes navigate/query/extract/help = 4 tools; full = 33), which can confuse agents that must reason about tool names across the two MCP servers; (2) URL-vs-search inference in run()/is_url can misclassify dot-containing queries; (3) several None-safety pitfalls around blockmap.title; and (4) the threaded 'timeout' is a cooperative deadline, not a kill, so abandoned workers can outlive the process. No hard security breach, but the cookies_set rephrase still describes replaying clearance cookies to bypass bot detection, which warrants a policy sanity check.

Verdict: Comment

Comments

• The PR title/description say 'minimal 3-tool surface' but the actual minimal contract is 4 tools in Rust (navigate, query, extract, help) and the full catalog is 33 (described elsewhere as '32 + help'). The '3' vs '4' vs '32' vs '33' framing is inconsistent throughout the docstrings, help catalog, and test assertions (test_minimal_is_4_and_full_is_33 asserts full >= 33). Tighten the wording so the numbers are unambiguous and don't mislead agents during progressive discovery.
• The escalation taxonomy is well-designed and the portable 'Rust facts -> Python host actions' split is a good pattern. Consider adding a unit test asserting every escalation reason emitted by _escalation_for_bundle maps to a documented reason string, to prevent drift between the taxonomy in the PR description and the code.
• Brave HTML extraction relies on fragile CSS selectors (.snippet[data-type=web], .title, .snippet-content) with multiple fallbacks. The generic 'scan all ' fallback will surface search.brave.com-internal and unrelated links. This is acceptable for a best-effort wrapper but worth noting the result quality is selector-dependent and may need periodic maintenance as Brave's DOM changes.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

is_url() treats any bare token containing a dot and no space as a URL. A search query like 'amazon.com' or 'brave.com review' (no leading scheme, contains dot) will be routed to navigate_auto() instead of search. Consider requiring a TLD suffix, a path component, or '://' before classifying a bare string as a URL in run().

[protostatis]

_norm_url() silently falls through to 'https://' + u.lstrip('/') when urljoin fails or base is empty. This can mangle arbitrary text (e.g. a relative href like 'mailto:' or an already-invalid token) into a bogus URL. Consider returning the input unchanged or raising on unresolvable values rather than fabricating an https:// URL.

[protostatis]

The shared ThreadPoolExecutor (max_workers=3) is bounded, which is good, but the docstring in navigate_auto correctly admits threads cannot be killed mid-run: on timeout the worker continues executing self.call() and only returns to the pool later. Combined with close() using shutdown(wait=False, cancel_futures=True), long-running self.call bodies can outlive the process. If a call blocks on a hung network fetch beyond the worker's own timeout, this leaks a thread indefinitely. Confirm the underlying Client.call has its own hard deadline so abandoned workers terminate.

[protostatis]

In navigate_auto(), goal is set from nav['blockmap']['title'][:120]; if blockmap.title is None (not an empty string), the slice 'None[:120]' raises TypeError and the entire navigate_auto call fails. Guard with (title or '')[:120]. The harness hits this same pattern (see scripts/site_matrix.py).

[protostatis]

out['title'] = (bundle.get('blockmap') or {}).get('title', '')[:120] will raise TypeError if title is None. Page titles can legitimately be null on redirect/error pages. Use (title or '')[:120] to avoid the harness crashing on a non-strict run.

[protostatis]

The Python MCP server exposes search/open/help (3 tools), but the Rust minimal profile exposes navigate/query/extract/help (4 tools) and full exposes 33. An agent is told 'minimal 3' yet sees 4 tools in Rust and a completely different tool name ('open' vs 'navigate', 'search' vs 'query'). This naming divergence across the two MCP servers will break cross-context agent reasoning. Align the tool names/contracts or document the mapping explicitly.

[protostatis]

The cookies_set description rephrase still instructs agents to 'replay a clearance cookie (e.g. PerimeterX _px3) acquired in real Chrome' to bypass bot detection, now framed as 'requires explicit user confirmation'. This is a policy/safety-relevant behavior (circumventing anti-bot controls) that deserves an explicit sign-off; the wording softens but does not remove the capability. Confirm this matches project policy and that confirmation is actually enforced, not just documented.

[protostatis]

Sky's Code Review

PR #48 introduces a 'minimal 3 + help' progressive-discovery surface on top of the unbrowser tool: a new Python SmartClient (smart.py) with Brave-search/html extraction, navigate_auto with bounded enrichment and a stable escalation taxonomy, a Python MCP server (smart_mcp.py), a Rust --mcp-profile minimal|full flag with a help tool returning a grouped catalog, a sanitized site-matrix harness (site_matrix.py), and protocol tests. The design is thoughtful — enrichment is time-bounded via a shared ThreadPoolExecutor, escalation codes are stable and carry category/severity/retryable/evidence, and cookies_set/challenge language was rephrased toward 'user-authorized browser'. No hard security bug or production-breaking defect found (no secrets leaked, no unsafe shell, no unpinned artifacts). Main concerns are: (1) the cookies_set / chrome_escalation rephrasing still preserves arbitrary cookie-replay and 'external_action: chrome_escalation' bot-bypass capability — wording-softening rather than a real capability reduction, worth an explicit policy sign-off; (2) a couple of latent Python bugs (_norm_url fallback can emit invalid URLs for root-relative hrefs when _last_url is unset; _micro_hint_for_bundle can IndexError on a form with empty controls); and (3) Python MCP exposes 3 tools (search/open/help) while the PR title and Rust minimal profile advertise a 4-tool surface (navigate/query/extract/help) — an inconsistency that also leaks into the help() catalog which hardcodes the full 32 regardless of profile.

Verdict: Comment

Comments

• Security/policy: the diff rephrases cookies_set and the challenge escalation to 'acquire a clearance cookie in real Chrome' / 'user-authorized browser', but the underlying capability is unchanged — arbitrary Set-Cookie replay and an external_action: chrome_escalation bot-bypass path remain. If this rephrasing is intended as a real policy tightening, it currently only softens language without removing the escalation surface; confirm that's the intent and get explicit sign-off.
• Reliability: the shared ThreadPoolExecutor (max_workers=3) runs discover/cards/page_model sequentially, and consumer threads cannot be killed on timeout — a timed-out call keeps a worker busy until it finishes. The code correctly acknowledges this and uses shutdown(wait=False, cancel_futures=True) on close(), but repeated slow/timeout enrichments can still starve the 3-worker pool within a long session. Consider documenting or capping concurrent navigate_auto usage.
• The _brave_html_extract JS snippet-extraction fallback (all.slice(title.length, ...)) assumes the collapsed full text starts exactly with the collapsed title; fragile if the title node ordering differs. It's best-effort so low risk, but a length guard against negative/overshoot slices would be cheap.
• Test coverage is good (fixtures for escalation + micro-hint, protocol drift test). The test_help_catalog_sums_to_32_and_matches_rust assertion ranges are unusually loose (>=30, <=40) and the 32-tool framing is imprecise (Python catalog is quoted as 35 = 32 + help + 2 aliases) — fine for a canary but the '32' vs '33' vs '35' counts are inconsistent across the PR description, tests, and comments.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

_norm_url fallback return "https://" + u.lstrip("/") emits invalid URLs for root-relative or bare inputs when base is unset. For a root-relative href like "/foo/bar" with _last_url still None (first call), this yields "https://foo/bar" (no host). The navigate_auto comment explicitly says it 'falls through to https://' on empty base — but for relative hrefs that's wrong. Consider raising or returning a clearly-invalid marker so the caller fails fast rather than navigating a malformed URL.

[protostatis]

In _micro_hint_for_bundle, f.get("controls", [{}])[0].get("ref") will raise IndexError if a form's controls list is present but empty (the [{}] default only applies when the key is missing, not when it maps to []). _micro_hint_for_bundle is called in navigate_auto without a try/except, so a single form with empty controls crashes the whole bundle. Guard with controls[0].get("ref") if controls else None.

[protostatis]

int(kw.get("count", kw.get("limit", 10))) raises TypeError if a caller passes count=None or a non-numeric value. Use a safe coercion (e.g. try/except int() with a default) so the inference path degrades instead of throwing.

[protostatis]

The help tool's groups JSON hardcodes the full 32-tool catalog unconditionally, so in --mcp-profile minimal mode calling help() still reveals every tool name — undercutting the 'progressive discovery' / 'help unlocks the rest' premise the PR advertises. The tools array is correctly filtered by profile, but groups and the note should reflect the active profile too (or explicitly document that the catalog is informational while only the filtered set is callable).

[protostatis]

TOOLS exposes only search/open/help, but the PR title and this help description claim the minimal surface is 'search/open/extract/help'. There is no extract (or query) tool in the Python MCP server, so help advertises an extract entry that isn't callable here. Either add extract (aligning with the Rust minimal profile's navigate/query/extract/help) or fix the description. This is also inconsistent with the Rust --mcp-profile minimal contract (4 tools) vs the Python MCP (3 tools).

[protostatis]

Sky's Code Review

This PR adds a 'minimal 3-tool' progressive-discovery surface on top of the existing unbrowser client. Python gets a SmartClient wrapper (search->Brave/DDG fallback, navigate_auto with bounded discovery and a stable escalation taxonomy), an MCP server (smart_mcp.py), and a sanitized 9-site test harness. Rust gains --mcp-profile minimal|full with static tool contracts and a grouped help tool. The escalation taxonomy (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss) with category/severity/retryable/evidence is well-designed and portable. The work is clean and thoughtfully sanitized (no secrets/cookies/bodies emitted). Main concerns are concurrency-correctness of the ThreadPoolExecutor timeout pattern in smart.py and protocol/consistency drift between the Python HELP_CATALOG and the Rust help tool catalog.

Verdict: Comment

Comments

• The escalation taxonomy is the strongest part of this PR — stable reason codes with category/severity/retryable/evidence and host-action mapping (external_action -> chrome_escalation) is well thought out for portability.
• The language rephrase around cookies_set ('continue using session state from a user-authorized browser, where permitted', 'requires explicit user confirmation', 'origin-scoped, ephemeral storage') is a good security-toned change and should be kept.
• site_matrix.py is properly sanitized (no cookies, no headers, no full body, URLs truncated). The 0.5s sleep between hosts and bounded 12s timeout are good hygiene.
• Consider adding a note or test that the MCP server handles client/tools/call errors with the correct JSON-RPC -32603 envelope and id echo — the manual line-based parser is easy to regress.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

close() uses shutdown(wait=False, cancel_futures=True). cancel_futures only affects queued (not-yet-started) futures; any _timed_call worker that already timed out and is still running in a thread will keep running past close(). If the underlying self.call blocks on a slow network fetch, the process may not exit cleanly or may accumulate until the worker finishes. Consider shutdown(wait=True, timeout=...) or, better, enforce the timeout at the transport layer (socket/HTTP timeout) rather than only at the future layer.

[protostatis]

_timed_call relies on fut.result(timeout=tm) to bound work, but Python threads cannot be killed mid-run — fut.cancel() is a no-op once the future is executing. As written, a discover/cards/page_model call that exceeds timeout returns a _timeout marker but the worker thread continues to completion in the pool. With max_workers=3 and three slow blocks you can saturate the pool and stall subsequent navigate_auto calls. This is acknowledged in the docstring, but for production reliability prefer bounding the underlying call itself (e.g. propagate a real timeout into self.call/navigate) rather than trusting the future timeout alone.

[protostatis]

bundle['extract'] is populated from nav.get('extract'), which per the base navigate only appears for inline embedded JSON under the 16KB cap. The 'partial_result' escalation checks extract.get('primary_truncated'), but primary_truncated is produced by an explicit extract() call, not the inline navigate extract. This means a truncated primary extract will never surface as partial_result via navigate_auto unless the blockmap/nav path also carries primary_truncated. Confirm the bundling actually plumbs the truncation stub, otherwise the Slickdeals partial_result expectation in site_matrix.py will silently not fire.

[protostatis]

if isinstance(cards, list) and len(cards) == 0 — cards is set to res from extract_cards, which may be a dict (e.g. {'cards': [...], ...}) rather than a plain list depending on the Rust extract_cards shape. If extract_cards returns an object, len(cards)==0 on an empty dict would wrongly trigger the cards_miss branch. Validate the actual return shape of extract_cards and dereference the list explicitly.

[protostatis]

get_client() creates SmartClient lazily but is never closed; the ThreadPoolExecutor created in SmartClient.init will linger for the process lifetime. For a long-lived stdio MCP server this is fine (single process), but add a shutdown path on stdin EOF/SIGTERM so the pool and any in-flight enrichment threads are reaped on host restarts.

[protostatis]

The help tool's grouped catalog is hardcoded and does not reconcile against tools_arr (the actual tool list) nor against the Python HELP_CATALOG. The Python catalog counts differ from the Rust full list (test acknowledges 35 vs 33 vs '32'). This hardcoded list will drift as tools are added/removed. Derive the groups from mcp_tools_for_profile() or a single source of truth instead of maintaining a parallel hand-written map — otherwise the 'progressive discovery' promise (help reflects exactly what's callable) will break silently.

[protostatis]

handle_session_request() calls dispatch_tool(..., "full") unconditionally, bypassing any mcp_profile. This means the bare-RPC dispatcher always treats the surface as full even if the process was started with --mcp-profile minimal. If the minimal contract is meant to be enforced outside the MCP stdin path as well, this is an inconsistency; if minimal-only-applies-to-MCP, add a comment documenting that intent.

[protostatis]

The assertions encode magic numbers (minimal==4, full>=33, py catalog 30..40) with comments explaining the drift, but the numbers themselves are not anchored to a single source of truth. This will fail spuriously as soon as tools are added. Prefer asserting the structural invariants (minimal == {navigate,query,extract,help}, minimal ⊆ full, help ∈ full) and drop the brittle >=33 / 30..40 ranges, or derive counts from mcp_tools('full').

[protostatis]

Sky's Code Review

This PR adds a 'minimal 3-tool progressive discovery' layer on top of an existing headless-browser CLI/MCP product: a Python SmartClient wrapper (Brave search with DDG/API fallback, navigate_auto with bounded discover/cards + a structured escalation taxonomy, and a grouped help catalog), a new stdio MCP server (smart_mcp.py), a Rust --mcp-profile minimal|full static contract with a help tool, and a sanitized 9-site matrix harness plus protocol tests. The work is well-scoped and the sanitization effort (no secrets/cookies, rephrased cookie-replay language, bounded timeouts) is commendable. There are no hard security holes, but there are several reliability and consistency issues worth fixing: the ThreadPoolExecutor timeout does not actually bound thread resources (hung calls leak a permanently-shrunken pool, and close() can tear down shared state mid-call), the Rust 'minimal' contract (navigate/query/extract/help) diverges from the Python surface (search/open/help) and from the PR's own '3-tool + help' framing, and the bare-RPC dispatch path hardcodes the 'full' profile. Verdict is 'comment' — no blocking security/bug, but the thread-lifecycle and contract-alignment issues should be addressed before this goes broad.

Verdict: Comment

Comments

• Contract/name misalignment: the Rust 'minimal' profile is {navigate, query, extract, help} (4 tools) while the Python SmartClient/smart_mcp surface is {search, open, help} (plus extract as a catalog entry). The PR title/'core' framing says 'minimal 3-search/open/extract + help', but the Rust help catalog's 'core' group lists navigate/query/extract/help, not search/open. Pick one canonical minimal surface and align Rust help, Python HELP_CATALOG, and smart_mcp TOOLS so agents don't get divergent core contracts between the two entry points.
• The challenge/bot-wall escalation in _escalation_for_bundle auto-attaches a cookies_set option suggesting replaying a clearance cookie (e.g. _px3) from 'user-authorized Chrome'. The language was correctly rephrased and requires_user_confirmation is set, but consider gating cookie-replay hints behind a stricter opt-in rather than emitting them by default for any challenge, to avoid an agent nudging a user toward bypassing bot detection without explicit prior intent.
• test_help_catalog_sums_to_32_and_matches_rust relies on stringy name-matching and loose bounds (>=30/<=40) with a comment admitting '35 is expected (32 + help + 2 aliases)'. The '32' vs '33' vs '35' counting is muddled and will silently rot; tighten to an explicit expected canonical tool set rather than a range.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

close() calls shutdown(wait=False, cancel_futures=True). cancel_futures only cancels queued futures, not running ones. A worker blocked on a slow network call will keep running on the (daemon-ish) pool after close(), and super().close() may tear down the shared Client/binary while that thread is still mid-call — a use-after-close race. Consider draining/handling in-flight futures or documenting that close() is best-effort under in-flight enrichment.

[protostatis]

_timed_call returns a {_timeout: True} marker on fut.result(timeout=tm) but does not free the underlying thread (Python threads can't be killed). With max_workers=3, three hung discover/cards/page_model calls permanently exhaust the shared pool for the lifetime of this SmartClient instance — subsequent navigate_auto calls serialize/deadlock. The timeout bounds latency but not thread capacity; consider a per-client worker leak guard or documenting that a client with 3 stuck enrichments should be closed/reopened.

[protostatis]

except Exception: return None in _brave_api_extract silently swallows persistent auth/config errors (bad key, malformed JSON) and falls through to the HTML path on every call, so a misconfigured BRAVE_API_KEY never surfaces. A one-time diagnostic log (or surfacing the error on first failure only) would aid debugging without changing the graceful-degrade behavior.

[protostatis]

handle_session_request hardcodes "full" when calling dispatch_tool, so the bare-RPC/session path ignores --mcp-profile entirely. The help tool over bare RPC will always report the full catalog and accept full-profile tools, inconsistent with the MCP path that respects the profile. If bare RPC should also honor --mcp-profile, thread it through here.

[protostatis]

if raw.is_none() && let Ok(v) = ... uses let-chains, stabilized in Rust 1.88. Confirm the project's MSRV/CI toolchain is >= 1.88, otherwise this will fail to compile on older toolchains.

[protostatis]

main() never closes the cached SmartClient (no _client.close() on EOF/finally), so the underlying session/binary resource is left to be GC'd. Minor, but add an explicit close on shutdown for clean teardown.

[protostatis]

Sky's Code Review

This PR introduces a 'minimal 3-tool surface' (search/open/help) with progressive discovery across Python and Rust. The Python SmartClient adds Brave search with DDG fallback, a navigate_auto bundle with a stable escalation taxonomy, a ThreadPoolExecutor-based bounded enrichment, and an MCP server. The Rust side adds --mcp-profile minimal|full static contracts and a help tool. The test harness and protocol tests are thorough. Overall this is well-engineered and thoughtful, with a few reliability concerns around thread-pool lifecycle, timeout bounding semantics, and MCP protocol correctness that warrant comment-level fixes.

Verdict: Comment

Comments

• The escalation taxonomy is well-designed and the 'portable facts in Rust + actions in Python' split is a genuinely good architecture. The external_action for chrome_escalation with requires_user_confirmation on cookie replay is correctly gated.
• The cookies_set description rephrase ('Continue using session state from a user-authorized browser... requires explicit user confirmation') is a meaningful safety improvement over the prior 'bypassing bot detection' language.
• Testing is strong: protocol tests assert the minimal contract has drifted, and test_micro_hint_fixtures includes the CNBC JS-injected-table trap regression and a context-leak assertion ('finance'/'premarket' must not leak into the generic hint).
• One cross-cutting concern: the Python MCP server (search/open/help) and the Rust MCP server (navigate/extract/help/query in minimal) present different minimal surfaces, and the PR description's 'minimal 3' (search/open/extract) doesn't exactly match the Rust minimal 4 (navigate/extract/help/query). The '3-tool' framing is slightly loose — minimal actually exposes 4 Rust tools and 3 Python tools with help as the fourth. Worth aligning the messaging or adding a doc note on why the two minimal contracts differ (Python smart surface vs Rust core surface).

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

The comment claims 'thread_name_prefix="smart"' and 'Bounded to 3 workers (discover/cards/page_model)', but _timed_call is invoked with a per-call timeout (default 8s). Python threads cannot actually be killed; fut.cancel() on a running future is a no-op and the worker thread keeps running after fut.result(timeout=...) raises. On a heavily JS-gated page, three 8s timeouts can leave three orphaned workers still executing self.call(...). Since the executor is shared and only reaped in close(), a long-lived MCP server (smart_mcp.py reuses a single SmartClient for the process lifetime) can accumulate stuck threads. Consider capping total work with a session-level deadline or documenting that close(wait=False) leaks in-flight workers.

[protostatis]

The MCP server uses a process-global singleton SmartClient (get_client caches _client), but the ThreadPoolExecutor inside it is created with thread_name_prefix="smart" and only shut down in SmartClient.close(), which is never called in main(). For a long-running stdio MCP server this is a deliberate lifetime choice, but the --mcp-profile minimal|full Rust server and this Python server expose different tool surfaces (Rust minimal = navigate/extract/help/query; Python minimal = search/open/help). Mixing them under the same registry name 'unbrowser-smart' could confuse hosts. Worth a doc note that these are separate contracts.

[protostatis]

handle_tools_call for open passes the url/goal straight through but does not apply the timeout argument from navigate_auto, and the MCP open inputSchema does not expose a timeout or exec_scripts parameter even though navigate_auto supports them. A host that hits a bot-walled or JS-heavy URL has no way to bound the open call from the MCP surface. Consider exposing timeout/exec_scripts in the inputSchema for parity with the Python library contract.

[protostatis]

In _brave_html_extract, when Brave returns 429/503 it falls back to DDG HTML, but only guards the fallback with nav.get('status') in (429, 503). Other 4xx (e.g. 403 challenge) or 5xx (e.g. 502) raise UnbrowserError with no Braver→DDG fallback and no challenge signal surfaced. The escalation taxonomy handles this later via _escalation_for_bundle, but the search entry point has no equivalent escalation/fallback for non-429 failures, so a 403 at search time surfaces as a raw exception rather than a structured challenge/auth_required escalation.

[protostatis]

_BRAVE_SNIPPET_JS selects a[href^="http"] and reads a.getAttribute('href') || a.href, but Brave frequently wraps result links in /search?q=... redirect URLs or href that is a relative/tracking path. The code only filters href.includes("search.brave.com"), so Brave tracking/redirect wrappers (not on search.brave.com) would be returned as-is with the comment 'keep as-is — caller can follow'. This can feed non-canonical URLs into navigate_auto, which then resolves them via _last_url. Minor, but worth a more robust URL normalization for Brave redirects.

[protostatis]

_escalation_for_bundle computes challenge.get('confidence') with an isinstance(..., (int, float)) guard but the returned escalation.confidence for challenge uses a float() default of 0.9. Good, but note bool is a subclass of int in Python, so a challenge dict carrying "confidence": true (JSON-converted) would pass the isinstance check and yield float(True) == 1.0, silently inflating confidence to 1.0. Consider an explicit not isinstance(x, bool) guard.

[protostatis]

The harness does time.sleep(0.5) between entries to 'be nice to hosts', but it also runs multiple finance/code entries in the same process against the same host. The --timeout default 12s per open is applied only to navigate_auto's timeout param, but _timed_call's 8s default inner timeout is hardcoded in navigate_auto with timeout: float = 8.0 default — the harness passes timeout=timeout (12s) but navigate_auto still defaults enrichment to 8s internally. Not a bug, but the 12s 'bounded' claim in the PR description doesn't bound the enrichment calls, only the outer navigate.

[protostatis]

parse_mcp_profile_arg uses if raw.is_none() && let Ok(v) = std::env::var(...) — a let-chain (&& let) which requires a recent Rust toolchain (Rust 2024 / edition 2021 with let_chains stable in 1.88+). This is fine if CI pins a recent toolchain, but if the workspace supports an older MSRV this will fail to compile. Worth confirming the MSRV supports let-chains.

[protostatis]

handle_session_request now hardcodes dispatch_tool(..., "full"), meaning the bare-RPC session path (non-MCP session exec / legacy JSON-RPC) always uses the full tool surface and ignores any --mcp-profile the user might have set. That's likely intentional (mcp-profile only gates the MCP tools/list surface), but it creates a subtle inconsistency: help via session-exec advertises the full catalog while MCP tools/list honors the profile. Add a comment clarifying that --mcp-profile is MCP-only by design.

[protostatis]

Sky's Code Review

This PR introduces a 'minimal 3 + help' progressive-discovery tool surface across both a Python SmartClient (search/open/help MCP) and the Rust MCP server (--mcp-profile minimal|full). The design is thoughtful: a stable escalation taxonomy (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss), bounded ThreadPoolExecutor-based enrichment with timeouts, micro_hint/avoid/tool_entropy calibration layers, and a sanitized site-matrix harness. Overall it is high-quality and self-documenting. The main concerns are (1) the cookies_set 'requires_user_confirmation' phrasing is cosmetic only — there is no actual server-side enforcement gate for cookie replay, (2) a likely AttributeError risk from self._last_url if the base Client does not define that attribute, (3) timeout exhaustion of the shared 3-worker executor since Python threads can't be interrupted, and (4) an inconsistency between the Python smart surface (search/open/help) and the Rust minimal profile (navigate/query/extract/help) that makes 'minimal 3' mean different things on each path. None of these are hard production-breaking bugs in isolation, so this is a comment-level review.

Verdict: Comment

Comments

• Tool surface inconsistency: the Python smart MCP (smart_mcp.py) exposes search/open/help (3 tools, with 'open' mapped to navigate_auto), while the Rust --mcp-profile minimal exposes navigate/query/extract/help (4 tools, no 'search' or 'open' names). The PR title and description frame this as the same 'minimal 3 (search/open/extract) + help', but the two surfaces do not match. Pick one canonical minimal contract and align both, or document clearly why Python and Rust differ (e.g. search is Python-only and relies on the SmartClient wrapper, not the Rust binary).
• parse_mcp_profile_arg relies on Rust 2024 let-chains (if raw.is_none() && let Ok(v) = std::env::var(...)). Verify the project's edition/toolchain supports let-chains; older stable compilers will fail to build this. If any other code still targets an older edition, this is a compile-time break.
• The Python HELP_CATALOG includes 'open' and 'search' as entries (aliases for navigate/query semantics) that do not exist as Rust tools, while the Rust help catalog lists 33 distinct tools. The test test_help_catalog_sums_to_32_and_matches_rust explicitly special-cases open/search, which signals the aliasing is a known but subtly divergent contract — worth reconciling rather than carrying forward as two floating aliases.
• site_matrix.py is nicely sanitized (sanitize_url strips query strings, no cookie/header emission), but note it hardcodes live third-party URLs (slickdeals, engadget, rei, businessinsider, yahoo, github) with a fixed 0.5s delay and no User-Agent/robots consideration. For a regression harness run in CI this is acceptable, but be aware it hits production sites and may flake on upstream changes or rate limits.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

close() calls self._smart_executor.shutdown(wait=False, cancel_futures=True) then returns immediately. cancel_futures only cancels pending tasks; running tasks continue on non-daemon executor threads after close(). Combined with _timed_call timeouts (which can't kill a mid-flight Python thread), repeated timeouts can leave up to 3 worker threads occupied and blocking subsequent enrichments until they finish. Bounded, so not a leak, but consider wait=True on shutdown or making the executor threads daemon so process exit isn't delayed by a hung navigate.

[protostatis]

self._last_url is accessed directly (self._last_url or "" in navigate_auto, and inside _norm_url via base). If the base Client class does not initialize _last_url, this raises AttributeError on first use and breaks the entire navigate_auto path. This is the 'relative href via _last_url' feature in the PR title, so it must already exist somewhere — but using getattr(self, '_last_url', None) is safer and removes the hidden coupling to an undocumented base-class attribute.

[protostatis]

_timed_call returns a {'_timeout': True, ...} marker when fut.result(timeout=tm) raises TimeoutError, but the underlying worker keeps running to completion (Python threads are non-preemptible). With discover/cards/page_model all sharing the same max_workers=3 pool, three hangs exhaust the pool and every subsequent enrichment blocks sequentially. Consider a per-call budget that the C-side (navigate call) also honors, or reduce enrichment concurrency so a timeout cannot starve the pool.

[protostatis]

The cookies_set description was rephrased to add 'requires explicit user confirmation and origin-scoped, ephemeral storage', but this is description text only — dispatch_tool has no actual confirmation/enforcement gate. An agent (or any MCP client) can still call cookies_set with arbitrary cookie values. If the intent is that cookie replay should be gated on user confirmation, that gate needs to be enforced server-side (a confirmation round-trip or an allowlist), not just asserted in prose. Otherwise the rephrase is security theater.

[protostatis]

The help tool returns the full tools_arr (complete tool schemas from mcp_tools_for_profile) even when the active profile is 'minimal'. Under --mcp-profile minimal, help therefore leaks the full 33-tool schema list including sensitive tools like cookies_set, which partially defeats the progressive-discovery/narrow-surface intent. Consider having help emit names/descriptions only (not full inputSchemas) for tools outside the active profile, or gating schema exposure on the full profile.

[protostatis]

Sky's Code Review

This PR adds a minimal 3-tool (search/open/extract + help) progressive-discovery surface over the existing unbrowser toolset: a Python SmartClient wrapper (smart.py) with Brave→DDG search fallback, bounded navigate_auto enrichment with a stable escalation taxonomy, a stdio MCP server (smart_mcp.py), grouped --help/help-topic catalog, and Rust --mcp-profile minimal|full static contracts plus per-tool MCP annotations. The design is thoughtful and well-documented (calibration-over-correctness, negative advice, entropy-based hint suppression), and the cookies_set rephrase toward 'user-authorized browser, ephemeral, requires confirmation' is a meaningful safety improvement over the prior 'bypassing bot detection' language. The main concerns are reliability/thread-safety around the shared ThreadPoolExecutor (timeout cannot kill Python threads; workers still run and may race with Client.close), an MCP client singleton that is never closed, and unbounded serialization of large bundles into MCP responses. No secrets are exposed and no outright security vulnerability found; these are production-hardening items rather than blockers.

Verdict: Comment

Comments

• The escalation taxonomy is well-designed and stable (challenge/auth_required/not_found/rate_limited/server_error/timeout/thin_shell/unsupported_js_feature/partial_result/cards_miss) with category/severity/retryable/evidence — this is a solid, reusable contract. Consider extracting it to a shared enum/module so the Python smart.py and Rust dispatch_tool can't drift out of sync.
• The cookies_set rephrase removes 'bypassing bot detection' in favor of 'user-authorized browser, origin-scoped, ephemeral, requires explicit confirmation' — a genuine safety improvement. The smart.py challenge hint and external_action(chrome_escalation) options consistently carry requires_user_confirmation/user-authorized language; keep that consistency as the taxonomy grows.
• BRAVE_API_KEY is read from env and sent as a header; it is never logged or echoed in the diff, but verify the Brave API error path (_brave_api_extract) doesn't interpolate the key into any exception message or response field.
• The '7 independent LLM agents succeeded via search->open' claim in the description is not verifiable from the diff; the included site_matrix.py harness asserts expected escalation_reason values (partial_result/thin_shell) which depend on live site structure and will be brittle over time — treat these as canary expectations, not deterministic assertions, which the informational-non-strict path already mostly does.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/smart.py:477 — close() calls _smart_executor.shutdown(wait=False, cancel_futures=True) then immediately super().close(). If the underlying Client tears down the native-binary subprocess while enrichment worker threads are still mid-call, those threads can race against the dead process and raise spurious errors (or worse, half-written state). Since timeout cannot kill a running Python thread, threads started just before close() will outlive the client. Consider shutdown(wait=True, cancel_futures=True) here (blocking briefly) to guarantee no in-flight call touches the closed client.

python/unbrowser/smart.py:491 — _timed_call submits to a shared max_workers=3 pool and, on TimeoutError, calls fut.cancel() then returns a _timeout marker. ThreadPoolExecutor.cancel() is a no-op on an already-running worker, so the worker keeps executing the call against the session after the caller has moved on. This is acknowledged in the comment but means the 'bounded' guarantee is only on caller-blocking, not on actual work or resource use. Bounded pool mitigates exhaustion; note that repeated timeouts still leave orphaned work in flight that can contend for the binary's single session.

python/unbrowser/smart_mcp.py:71 — get_client() returns a module-global SmartClient that is never closed. The SmartClient owns a ThreadPoolExecutor (3 worker threads); those threads are not reaped on process exit, and the native-binary subprocess is not torn down cleanly. Since MCP servers are long-running, hold the client in main() and close it on a shutdown path (or at minimum document that the executor is intentionally leaked per-process).

python/unbrowser/smart_mcp.py:147 — tools/call serializes the full result via json.dumps(value, indent=2) with no size cap. navigate_auto bundles include blockmap plus extract (JSON-LD/NEXT_DATA) and discover routes; on a data-heavy page this can produce multi-MB text blocks that blow up host context windows. Consider truncating the blockmap/extract fields in the MCP response layer (the smart layer already stubs >16KB extract, but blockmap is not capped).

src/main.rs:6439 — parse_mcp_profile_arg uses if-let chains (raw.is_none() && let Ok(v) = ...) and the && let Some(obj) = t.as_object_mut() form later in mcp_tools_for_profile. These are let-chains, stabilized in Rust 1.88 (2024). If the project's MSRV is older, this breaks the build. Pin/confirm the minimum rustc version in CI or rewrite with nested matches for wider compatibility.

src/main.rs:7506 — The help tool returns mcp_tools_for_profile(profile) including full descriptions and inputSchema for every tool. In minimal profile this is fine (only 4 tools), but note the _ => unknown-tool fallback always builds mcp_tools_for_profile("full") and leaks the full tool name list to a minimal-profile agent via the 'Did you mean' error, effectively revealing the hidden surface. Minor — the catalog is non-secret — but it slightly undercuts the progressive-disclosure intent.

scripts/site_matrix.py:131 — sanitize_url strips the query string and truncates to 120 chars, but slickdeals-deal.open_url is a full path that may contain fragment/tracking components beyond '?'. Truncation at 120 chars can still leak PII-free path segments, and 'title' from blockmap is emitted raw (up to 120 chars). Low risk since all URLs are public, but confirm no URL in the matrix ever carries a signed token in the path (the /f/19868166... form is opaque but public).

[protostatis]

Sky's Code Review

This PR adds a minimal 3-tool (search/open/extract) smart surface with progressive discovery layered over the existing unbrowser RPC engine, plus a matching --mcp-profile minimal|full contract in Rust, help-catalog discovery, a sanitized 9-site matrix harness, and a probabilistic-policy doc section. The Python SmartClient wraps Brave/DDG search, bounded auto-discovery, a portable escalation taxonomy, and derives micro_hint/next_tools/avoid/entropy signals. Overall the design is thoughtful and the sanitization (no cookies/secrets in the harness, rephrased cookies_set language) is well executed. The diff is truncated, so I reviewed the visible Python + Rust. I found no production-breaking security issues, but several reliability/correctness concerns worth addressing: an unguarded self._last_url attribute access, fragile CLI argument parsing (index-out-of-range/ValueError on missing --count/--goal values), a ThreadPoolExecutor that is bounded but used sequentially (so the 3-worker pool is effectively single-threaded), and a bare-RPC dispatch path that hardcodes the 'full' profile while the MCP path threads the profile correctly.

Verdict: Comment

Comments

• Diff is truncated ('... (1 more file(s) truncated due to size) ...'): the Rust mcp_main/tools/call path and any remaining files were not fully reviewable. Recommend confirming the final file(s) for the same profile-threading and sanitization issues flagged above.
• SmartClient.search overrides the base Client.search with a different return shape (list[dict] for brave vs dict for ddg/bing). This is documented but is a fragile API seam for downstream callers; consider a distinct method name or a stable wrapped return type to avoid accidental breakage.
• The probability-policy doc addition is clear and well-reasoned; the calibration/negative-advice/entropy-suppression rules align with the code (gated evidence branches, avoid[] only on hard absence, micro_hint suppressed under ambiguity). No action needed.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

self._last_url is accessed directly (self._last_url or "") with no getattr guard. If the base Client does not define _last_url, this raises AttributeError on the very first navigate_auto call. Confirm Client always populates _last_url (at least to None) or use getattr(self, '_last_url', None).

[protostatis]

count = int(args[i + 1]) will raise IndexError if --count is the last arg (no value), or ValueError on a non-integer. Same pattern in _cmd_open for --goal (args[i + 1]). Add bounds/type checking and emit a clean usage error instead of a raw traceback.

[protostatis]

_cmd_open: goal = args[i + 1] has the same missing-value IndexError risk as --count when --goal is the trailing arg. Guard args length before indexing.

[protostatis]

The ThreadPoolExecutor(max_workers=3) is fed one enrichment at a time inside a sequential for loop, so only one worker is ever busy. The 'parallel enrichment' benefit of the pool is unused; timeouts still bound each call but the pool is effectively single-threaded. Either submit discover/cards/page_model concurrently (when include_page_model is set) or drop the pool in favor of a per-call executor.

[protostatis]

fut.cancel() on a running future is a no-op; the worker still runs to completion after the timeout marker is returned. This is acknowledged in the comment but means bounded 'timeout' does not actually free the worker or stop the work — repeated timeouts on slow pages can still accumulate load. Consider a hard cap on outstanding enrichment work or a separate process/thread you can actually terminate.

[protostatis]

handle_session_request hardcodes dispatch_tool(..., "full") for the bare-RPC path, while mcp_main threads the parsed mcp_profile. If a host uses the bare session/RPC interface expecting profile-gated tools, this inconsistency silently exposes full help/catalog. Confirm bare-RPC is intentionally always-full, or thread the profile here too.

[protostatis]

mcp_tool_annotations defaults unknown tool names to readOnlyHint=true, idempotentHint=true, openWorldHint=false. Tools like network_extract and cookies_get (analysis of already-fetched data) fit this, but confirm nothing that mutates state falls into the default branch — otherwise an unannotated mutating tool is advertised as read-only, suppressing host confirmation prompts.

[protostatis]

challenge.get("confidence", 0.9) uses default 0.9, but the isinstance check only coerces numeric values and leaves non-numeric values (e.g. a string) unhandled — the subsequent float(challenge.get(...)) path is unreachable if the value is a non-numeric type. Minor; consider normalizing via a small helper to avoid TypeError on malformed challenge confidence.

[protostatis]

Sky's Code Review

This PR adds a 'minimal 3-tool' progressive-discovery surface atop unbrowser: a Python SmartClient (search/open/extract + help) with Brave search + DDG fallback, a stable escalation taxonomy, and probabilistic routing aids (micro_hint/next_tools/avoid/tool_entropy); a Rust --mcp-profile minimal|full split with a grouped help tool and MCP tool annotations; and a sanitized 9-site harness. The design is thoughtful and the 'rephrase bot-bypass language' change is a genuine safety improvement. However, two issues warrant attention before merge: (1) the MCP-exposed open/search tools accept arbitrary URLs with no SSRF/allowlist guard — this is a network-reachability primitive handed to LLM agents, which is a real security surface; and (2) SmartClient runs enrichment calls (discover/extract_cards/page_model) on a shared ThreadPoolExecutor against the same Client/session, which is almost certainly not thread-safe and can race on shared session state (cookies/_last_url/binary session). The diff is also truncated ('1 more file(s) truncated'), so the review covers the visible files only.

Verdict: Changes requested

Comments

• The rephrase of the cookies_set description (removing 'bypassing bot detection' in favor of 'Continue using session state from a user-authorized browser, where permitted, requires explicit user confirmation and origin-scoped, ephemeral storage') is a strong, correct safety improvement — the best part of the diff.
• The per-enrichment 8s timeout runs sequentially (discover + cards + optional page_model), so a worst-case open() can block ~24s+ before returning, exceeding the harness's advertised 12s bound. Consider a single total budget across enrichments rather than per-call timeouts.
• The _timed_call timeout contract is documented accurately (threads can't be killed; fut.cancel() is a no-op on a running future), which is good — but it means the pool can still stall at max_workers=3 under slow sites, and the abandon-on-timeout behavior is worth an explicit note in the SmartClient docstring so callers understand a timed-out call may still be consuming a worker.
• The probabilistic-routing design (Section 13 of probabilistic-policy.md and the micro_hint/avoid/entropy gating) is well-reasoned and the calibration discipline (e.g. >=8 cells before routing to extract_table) is a genuinely good engineering touch.
• Diff is truncated ('1 more file(s) truncated due to size'); review covers docs, pyproject, init, _cli, smart.py, smart_mcp.py, site_matrix.py, and main.rs as shown. If the truncated file contains more MCP/tool surface or harness changes, re-review those.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/smart.py:84 — SSRF risk: _norm_url falls through to 'https://' + u.lstrip('/') with no validation of scheme, host, or whether the target is an internal/loopback/metadata address. Since open() (navigate_auto) is exposed directly to LLM agents over MCP with no host allowlist, an agent can be induced to fetch internal URLs (e.g. http://169.254.169.254/, file://, or RFC1918 hosts). Add scheme allowlisting (http/https only) and reject/guard private, loopback, link-local, and metadata IP ranges before the binary performs the fetch.

python/unbrowser/smart.py:588 — Thread-safety bug: _timed_call submits self.call (discover/extract_cards/page_model) to the shared _smart_executor, invoking the same Client/session concurrently. Client.call is almost certainly not thread-safe (shared cookie jar, _last_url, and the underlying binary session state). Concurrent enrichment calls can race and corrupt session state or return interleaved/incorrect results. Either serialize enrichment on the session (run sequentially, or with a per-session lock) or document/prove Client.call is re-entrant.

python/unbrowser/smart.py:454 — close() calls self._smart_executor.shutdown(wait=False, cancel_futures=True). Because Python threads cannot be interrupted, wait=False leaves in-flight self.call() worker threads running against a session that is being torn down. Prefer wait=True (or a bounded join) on shutdown so no worker outlives the client, especially since session teardown may invalidate the binary session the worker is still using.

python/unbrowser/smart.py:60 — is_url auto-treats any bare string with a dot in the netloc as a URL (e.g. 'internal.corp' or 'localhost' without port). Combined with run() inference, a user query that looks like a hostname is silently routed to navigate_auto and fetched. Ensure the SSRF guard sits in this path too, and consider requiring an explicit scheme for the navigate path to avoid surprising fetches of bare hostnames.

python/unbrowser/smart_mcp.py:45 — Inconsistency in the 'minimal 3' contract: the Python smart MCP surface exposes search/open/help (plus navigate internally), while the Rust --mcp-profile minimal exposes navigate/extract/help/query. The PR title advertises a unified minimal 3-tool surface, but the two implementations disagree on which tools are in the minimal set. Align them (or document each explicitly) so agent authors get a consistent contract.

src/main.rs:6439 — parse_mcp_profile_arg uses if raw.is_none() && let Ok(v) = ... (let-chains). Ensure the MSRV/build pins a Rust version that supports let-chains. Also note the same let-chain style is used in mcp_tools_for_profile for annotations.

src/main.rs:7301 — The minimal-profile help tool returns the FULL tools_arr (all 33 tool schemas) in its catalog/tools response even though tools/list only advertises 4. This leaks the full tool surface (including cookies_set with its E2E schema) to a minimal-profile agent that was intended to see a smaller surface. Consider emitting descriptions-only or the minimal subset in the help response when profile==minimal.

scripts/site_matrix.py:145 — sanitize_url only strips the query string and truncates to 120 chars; it does not redact path tokens or credential fragments. It's acceptable for this public matrix, but the docstring claims 'sanitize anyway' — if any URL ever carries a token in the path it would leak into --json output. Consider redacting query/path credential patterns defensively.