51621a9 fix(web): address review findings · zut

M docs/extensions.md +5 -7


@@ -226,13 +226,11 @@ describing the tool's args (the same shape Anthropic and OpenAI accept).
226	226
227	227		Tool names live in the same namespace as built-in tools (`read`,
228	228		`write`, `edit`, `bash`, `create_worktree`, `grep`, `lsp`, `web_search`,
229		-	`web_open`, `web_find`, `web_click`, `skill`). Conflicts with active built-ins
230		-	are silently shadowed by the built-in. Every native public-web capability name
231		-	is reserved even when its setting disables the capability, so an extension
232		-	cannot claim or replace it. The `grep` name is
233		-	reserved even when `Args.NoTools`/SDK `Config.NoTools` or an explicit
234		-	`Args.Tools`/SDK `Config.Tools` allowlist disables the native search tool, so
235		-	an extension cannot claim or replace it.
	229	+	`web_open`, `web_find`, `web_click`, `schedule`, `update_goal`, `skill`).
	230	+	Conflicts with active built-ins are silently shadowed by the built-in. Every
	231	+	native public-web capability name, plus `grep`, `schedule`, and `update_goal`,
	232	+	is reserved even when it is unavailable, so an extension cannot claim or
	233	+	replace it.
236	234
237	235		Set `"deferred": true` to register a tool without advertising its definition initially. A loader tool can activate registered deferred tools by returning their names in `activate_tools`:
238	236


@@ -226,13 +226,11 @@ describing the tool's args (the same shape Anthropic and OpenAI accept).
226			226
227		Tool names live in the same namespace as built-in tools (`read`,	227		Tool names live in the same namespace as built-in tools (`read`,
228		`write`, `edit`, `bash`, `create_worktree`, `grep`, `lsp`, `web_search`,	228		`write`, `edit`, `bash`, `create_worktree`, `grep`, `lsp`, `web_search`,
229	-	`web_open`, `web_find`, `web_click`, `skill`). Conflicts with active built-ins	229	+	`web_open`, `web_find`, `web_click`, `schedule`, `update_goal`, `skill`).
230	-	are silently shadowed by the built-in. Every native public-web capability name	230	+	Conflicts with active built-ins are silently shadowed by the built-in. Every
231	-	is reserved even when its setting disables the capability, so an extension	231	+	native public-web capability name, plus `grep`, `schedule`, and `update_goal`,
232	-	cannot claim or replace it. The `grep` name is	232	+	is reserved even when it is unavailable, so an extension cannot claim or
233	-	reserved even when `Args.NoTools`/SDK `Config.NoTools` or an explicit	233	+	replace it.
234	-	`Args.Tools`/SDK `Config.Tools` allowlist disables the native search tool, so
235	-	an extension cannot claim or replace it.
236			234
237		Set `"deferred": true` to register a tool without advertising its definition initially. A loader tool can activate registered deferred tools by returning their names in `activate_tools`:	235		Set `"deferred": true` to register a tool without advertising its definition initially. A loader tool can activate registered deferred tools by returning their names in `activate_tools`:
238			236

M packages/agent/build.go +1 -1


@@ -100,7 +100,7 @@ func (r *Resolved) MergeExtensionTools(mgr ExtensionToolSource) {
100	100		// Web capability and other native names remain reserved even when their
101	101		// policy excludes the current session. An extension must not turn a
102	102		// normal CLI opt-out into a differently implemented capability.
103		-	if tools.IsWebCapabilityName(info.Name) \|\| info.Name == "grep" \|\| info.Name == "update_goal" {
	103	+	if tools.IsWebCapabilityName(info.Name) \|\| info.Name == "grep" \|\| info.Name == "schedule" \|\| info.Name == "update_goal" {
104	104		continue
105	105		}
106	106		if _, exists := r.ToolRegistry[info.Name]; exists {


@@ -100,7 +100,7 @@ func (r *Resolved) MergeExtensionTools(mgr ExtensionToolSource) {
100		// Web capability and other native names remain reserved even when their	100		// Web capability and other native names remain reserved even when their
101		// policy excludes the current session. An extension must not turn a	101		// policy excludes the current session. An extension must not turn a
102		// normal CLI opt-out into a differently implemented capability.	102		// normal CLI opt-out into a differently implemented capability.
103	-	if tools.IsWebCapabilityName(info.Name) \|\| info.Name == "grep" \|\| info.Name == "update_goal" {	103	+	if tools.IsWebCapabilityName(info.Name) \|\| info.Name == "grep" \|\| info.Name == "schedule" \|\| info.Name == "update_goal" {
104		continue	104		continue
105		}	105		}
106		if _, exists := r.ToolRegistry[info.Name]; exists {	106		if _, exists := r.ToolRegistry[info.Name]; exists {

M packages/agent/ext/ext.go +4 -4


@@ -421,10 +421,10 @@ func (e *Extension) Command(name, description string, fn CommandHandler) {
421	421		// into the agent's registry once the extension's ready frame fires.
422	422		//
423	423		// Conflicts with active built-in tool names (read, write, edit, bash,
424		-	// create_worktree, grep, lsp, web_search, web_open, web_find, web_click, and
425		-	// skill) are silently shadowed by the built-in. Every native public-web name
426		-	// and grep are reserved even when those built-ins are disabled, so an
427		-	// extension cannot claim or replace them.
	424	+	// create_worktree, grep, lsp, web_search, web_open, web_find, web_click,
	425	+	// schedule, update_goal, and skill) are silently shadowed by the built-in.
	426	+	// Every native public-web name, plus grep, schedule, and update_goal, is
	427	+	// reserved even when unavailable, so an extension cannot claim or replace it.
428	428		func (e *Extension) Tool(name, description string, schema json.RawMessage, fn ToolHandler) {
429	429		e.registerTool(name, description, schema, false, fn)
430	430		}


@@ -421,10 +421,10 @@ func (e *Extension) Command(name, description string, fn CommandHandler) {
421		// into the agent's registry once the extension's ready frame fires.	421		// into the agent's registry once the extension's ready frame fires.
422		//	422		//
423		// Conflicts with active built-in tool names (read, write, edit, bash,	423		// Conflicts with active built-in tool names (read, write, edit, bash,
424	-	// create_worktree, grep, lsp, web_search, web_open, web_find, web_click, and	424	+	// create_worktree, grep, lsp, web_search, web_open, web_find, web_click,
425	-	// skill) are silently shadowed by the built-in. Every native public-web name	425	+	// schedule, update_goal, and skill) are silently shadowed by the built-in.
426	-	// and grep are reserved even when those built-ins are disabled, so an	426	+	// Every native public-web name, plus grep, schedule, and update_goal, is
427	-	// extension cannot claim or replace them.	427	+	// reserved even when unavailable, so an extension cannot claim or replace it.
428		func (e *Extension) Tool(name, description string, schema json.RawMessage, fn ToolHandler) {	428		func (e *Extension) Tool(name, description string, schema json.RawMessage, fn ToolHandler) {
429		e.registerTool(name, description, schema, false, fn)	429		e.registerTool(name, description, schema, false, fn)
430		}	430		}

M packages/agent/modes/interactive_settings.go +8 -1


@@ -717,7 +717,14 @@ func (i *Interactive) stripWebSearchTool() {
717	717		return
718	718		}
719	719		tools := i.agent.ToolsSnapshot()
720		-	if _, ok := tools["web_search"]; !ok {
	720	+	hasWebCapability := false
	721	+	for _, name := range toolspkg.WebCapabilityNames {
	722	+	if _, ok := tools[name]; ok {
	723	+	hasWebCapability = true
	724	+	break
	725	+	}
	726	+	}
	727	+	if !hasWebCapability {
721	728		return
722	729		}
723	730		toolspkg.RemoveWebCapabilities(tools)

 		return
 	}
 	tools := i.agent.ToolsSnapshot()
-	if _, ok := tools["web_search"]; !ok {
+	hasWebCapability := false
+	for _, name := range toolspkg.WebCapabilityNames {
+		if _, ok := tools[name]; ok {
+			hasWebCapability = true
+			break
+		}
+	}
+	if !hasWebCapability {
 		return
 	}
 	toolspkg.RemoveWebCapabilities(tools)

M packages/agent/tools/web_page.go +37 -4


@@ -137,6 +137,18 @@ func (s BrowsingStore) addPage(page webDocument, generation uint64) (string, b
137	137		return id, id != ""
138	138		}
139	139
	140	+	func (s *BrowsingStore) remove(id string) {
	141	+	if s == nil {
	142	+	return
	143	+	}
	144	+	s.mu.Lock()
	145	+	if doc, ok := s.docs[id]; ok {
	146	+	delete(s.docs, id)
	147	+	s.used -= uint64(doc.size)
	148	+	}
	149	+	s.mu.Unlock()
	150	+	}
	151	+
140	152		func (s BrowsingStore) add(doc webDocument, generation uint64, checkGeneration bool) string {
141	153		if s == nil \|\| doc == nil {
142	154		return ""

@@ -288,7 +300,10 @@ func (t *WebOpenTool) Preview(_ context.Context, raw json.RawMessage) (core.Tool
288	300		if !found {
289	301		return webPageError(webPageUnknown), nil
290	302		}
291		-	return webPageValue(fmt.Sprintf("Open public web source %s. No request has been made.", doc.url), false), nil
	303	+	if doc.source {
	304	+	return webPageValue(fmt.Sprintf("Open public web source %s. No request has been made.", doc.url), false), nil
	305	+	}
	306	+	return webPageValue(fmt.Sprintf("Open stored web page %s. No request has been made.", doc.url), false), nil
292	307		}
293	308		func (t *WebClickTool) Preview(_ context.Context, raw json.RawMessage) (core.ToolResult, error) {
294	309		args, ok := parseWebClickArgs(raw)

@@ -559,7 +574,7 @@ func parsePlainWebPage(body []byte, finalURL string) *webDocument {
559	574		}
560	575		func extractHTMLWebPage(root html.Node, finalURL string) (webDocument, string) {
561	576		var title string
562		-	var content *html.Node
	577	+	var article, main, body *html.Node
563	578		stack := []struct {
564	579		n *html.Node
565	580		depth int

@@ -576,8 +591,19 @@ func extractHTMLWebPage(root html.Node, finalURL string) (webDocument, string)
576	591		if item.n.Data == "title" && title == "" {
577	592		title = boundPageText(nodePageText(item.n), 512)
578	593		}
579		-	if content == nil && (item.n.Data == "article" \|\| item.n.Data == "main" \|\| item.n.Data == "body") {
580		-	content = item.n
	594	+	switch item.n.Data {
	595	+	case "article":
	596	+	if article == nil {
	597	+	article = item.n
	598	+	}
	599	+	case "main":
	600	+	if main == nil {
	601	+	main = item.n
	602	+	}
	603	+	case "body":
	604	+	if body == nil {
	605	+	body = item.n
	606	+	}
581	607		}
582	608		}
583	609		for child := item.n.LastChild; child != nil; child = child.PrevSibling {

@@ -587,6 +613,13 @@ func extractHTMLWebPage(root html.Node, finalURL string) (webDocument, string)
587	613		}{child, item.depth + 1})
588	614		}
589	615		}
	616	+	content := article
	617	+	if content == nil {
	618	+	content = main
	619	+	}
	620	+	if content == nil {
	621	+	content = body
	622	+	}
590	623		if content == nil {
591	624		content = root
592	625		}

 	return id, id != ""
 }
+func (s *BrowsingStore) remove(id string) {
+	if s == nil {
+		return
+	}
+	s.mu.Lock()
+	if doc, ok := s.docs[id]; ok {
+		delete(s.docs, id)
+		s.used -= uint64(doc.size)
+	}
+	s.mu.Unlock()
+}
 func (s *BrowsingStore) add(doc *webDocument, generation uint64, checkGeneration bool) string {
 	if s == nil || doc == nil {
 		return ""
 	if !found {
 		return webPageError(webPageUnknown), nil
 	}
-	return webPageValue(fmt.Sprintf("Open public web source %s. No request has been made.", doc.url), false), nil
+	if doc.source {
+		return webPageValue(fmt.Sprintf("Open public web source %s. No request has been made.", doc.url), false), nil
+	}
+	return webPageValue(fmt.Sprintf("Open stored web page %s. No request has been made.", doc.url), false), nil
 }
 func (t *WebClickTool) Preview(_ context.Context, raw json.RawMessage) (core.ToolResult, error) {
 	args, ok := parseWebClickArgs(raw)
 }
 func extractHTMLWebPage(root *html.Node, finalURL string) (*webDocument, string) {
 	var title string
-	var content *html.Node
+	var article, main, body *html.Node
 	stack := []struct {
 		n     *html.Node
 		depth int
 			if item.n.Data == "title" && title == "" {
 				title = boundPageText(nodePageText(item.n), 512)
 			}
-			if content == nil && (item.n.Data == "article" || item.n.Data == "main" || item.n.Data == "body") {
+			switch item.n.Data {
-				content = item.n
+			case "article":
+				if article == nil {
+					article = item.n
+				}
+			case "main":
+				if main == nil {
+					main = item.n
+				}
+			case "body":
+				if body == nil {
+					body = item.n
+				}
 			}
 		}
 		for child := item.n.LastChild; child != nil; child = child.PrevSibling {
 			}{child, item.depth + 1})
 		}
 	}
+	content := article
+	if content == nil {
+		content = main
+	}
+	if content == nil {
+		content = body
+	}
 	if content == nil {
 		content = root
 	}

M packages/agent/tools/web_page_test.go +11 -10


@@ -3,6 +3,7 @@ package tools
3	3		import (
4	4		"context"
5	5		"crypto/tls"
	6	+	"crypto/x509"
6	7		"encoding/json"
7	8		"net"
8	9		"net/http"

@@ -148,13 +149,12 @@ func TestWebPageFetcherPreservesHTTPSHostnameForSNI(t *testing.T) {
148	149		server.StartTLS()
149	150		defer server.Close()
150	151		fetcher := testWebFetcher(t, server)
151		-	// httptest uses its own generated certificate. The unexported seam is used
152		-	// only to trust it for this local SNI assertion; production never supplies
153		-	// InsecureSkipVerify.
154		-	fetcher.tlsConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // test-only local certificate
155		-	body, _, _, message := fetcher.fetch(context.Background(), "https://public.test/")
	152	+	roots := x509.NewCertPool()
	153	+	roots.AddCert(server.Certificate())
	154	+	fetcher.tlsConfig = &tls.Config{RootCAs: roots}
	155	+	body, _, _, message := fetcher.fetch(context.Background(), "https://example.com/")
156	156		got, _ := serverName.Load().(string)
157		-	if message != "" \|\| string(body) != "secure" \|\| got != "public.test" {
	157	+	if message != "" \|\| string(body) != "secure" \|\| got != "example.com" {
158	158		t.Fatalf("HTTPS body=%q message=%q SNI=%q", body, message, got)
159	159		}
160	160		}

@@ -165,16 +165,17 @@ func TestWebPageFetcherPinsTheValidatedAddress(t *testing.T) {
165	165		_, _ = w.Write([]byte("pinned"))
166	166		}))
167	167		defer server.Close()
168		-	var dialed string
	168	+	var dialed atomic.Value
169	169		fetcher := testWebFetcher(t, server)
170	170		originalDial := fetcher.dial
171	171		fetcher.dial = func(ctx context.Context, network, address string) (net.Conn, error) {
172		-	dialed = address
	172	+	dialed.Store(address)
173	173		return originalDial(ctx, network, address)
174	174		}
175	175		body, _, _, message := fetcher.fetch(context.Background(), "http://public.test/")
176		-	if message != "" \|\| string(body) != "pinned" \|\| dialed != "8.8.8.8:80" {
177		-	t.Fatalf("pinned fetch body=%q message=%q dialed=%q", body, message, dialed)
	176	+	got, _ := dialed.Load().(string)
	177	+	if message != "" \|\| string(body) != "pinned" \|\| got != "8.8.8.8:80" {
	178	+	t.Fatalf("pinned fetch body=%q message=%q dialed=%q", body, message, got)
178	179		}
179	180		}
180	181

 import (
 	"context"
 	"crypto/tls"
+	"crypto/x509"
 	"encoding/json"
 	"net"
 	"net/http"
 	server.StartTLS()
 	defer server.Close()
 	fetcher := testWebFetcher(t, server)
-	// httptest uses its own generated certificate. The unexported seam is used
+	roots := x509.NewCertPool()
-	// only to trust it for this local SNI assertion; production never supplies
+	roots.AddCert(server.Certificate())
-	// InsecureSkipVerify.
+	fetcher.tlsConfig = &tls.Config{RootCAs: roots}
-	fetcher.tlsConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // test-only local certificate
+	body, _, _, message := fetcher.fetch(context.Background(), "https://example.com/")
-	body, _, _, message := fetcher.fetch(context.Background(), "https://public.test/")
 	got, _ := serverName.Load().(string)
-	if message != "" || string(body) != "secure" || got != "public.test" {
+	if message != "" || string(body) != "secure" || got != "example.com" {
 		t.Fatalf("HTTPS body=%q message=%q SNI=%q", body, message, got)
 	}
 }
 		_, _ = w.Write([]byte("pinned"))
 	}))
 	defer server.Close()
-	var dialed string
+	var dialed atomic.Value
 	fetcher := testWebFetcher(t, server)
 	originalDial := fetcher.dial
 	fetcher.dial = func(ctx context.Context, network, address string) (net.Conn, error) {
-		dialed = address
+		dialed.Store(address)
 		return originalDial(ctx, network, address)
 	}
 	body, _, _, message := fetcher.fetch(context.Background(), "http://public.test/")
-	if message != "" || string(body) != "pinned" || dialed != "8.8.8.8:80" {
+	got, _ := dialed.Load().(string)
-		t.Fatalf("pinned fetch body=%q message=%q dialed=%q", body, message, dialed)
+	if message != "" || string(body) != "pinned" || got != "8.8.8.8:80" {
+		t.Fatalf("pinned fetch body=%q message=%q dialed=%q", body, message, got)
 	}
 }

M packages/agent/tools/web_search.go +16 -1


@@ -192,14 +192,29 @@ func (t *WebSearchTool) Execute(ctx context.Context, raw json.RawMessage, progre
192	192		}
193	193		visible := results[:visibleCount]
194	194		if t.store != nil {
	195	+	refs := make([]string, len(visible))
195	196		for index := range visible {
196	197		refID, committed := t.store.addSourceAtGeneration(visible[index].URL, storeGeneration)
197	198		if !committed {
	199	+	for _, id := range refs {
	200	+	t.store.remove(id)
	201	+	}
198	202		return webSearchError("web search: unavailable in this session"), nil
199	203		}
	204	+	refs[index] = refID
200	205		visible[index].RefID = refID
201	206		}
202		-	text, _, _ = formatWebSearchResults(visible)
	207	+	initialTruncated := outputTruncated
	208	+	var finalTruncated bool
	209	+	text, visibleCount, finalTruncated = formatWebSearchResults(visible)
	210	+	outputTruncated = initialTruncated \|\| finalTruncated
	211	+	for _, id := range refs[visibleCount:] {
	212	+	t.store.remove(id)
	213	+	}
	214	+	visible = visible[:visibleCount]
	215	+	if visibleCount == 0 {
	216	+	return webSearchError("web search: no usable HTTP(S) results"), nil
	217	+	}
203	218		}
204	219		return webSearchResultValue(text, false, map[string]any{
205	220		"backend": "DuckDuckGo HTML",


@@ -192,14 +192,29 @@ func (t *WebSearchTool) Execute(ctx context.Context, raw json.RawMessage, progre
192		}	192		}
193		visible := results[:visibleCount]	193		visible := results[:visibleCount]
194		if t.store != nil {	194		if t.store != nil {
			195	+	refs := make([]string, len(visible))
195		for index := range visible {	196		for index := range visible {
196		refID, committed := t.store.addSourceAtGeneration(visible[index].URL, storeGeneration)	197		refID, committed := t.store.addSourceAtGeneration(visible[index].URL, storeGeneration)
197		if !committed {	198		if !committed {
			199	+	for _, id := range refs {
			200	+	t.store.remove(id)
			201	+	}
198		return webSearchError("web search: unavailable in this session"), nil	202		return webSearchError("web search: unavailable in this session"), nil
199		}	203		}
			204	+	refs[index] = refID
200		visible[index].RefID = refID	205		visible[index].RefID = refID
201		}	206		}
202	-	text, _, _ = formatWebSearchResults(visible)	207	+	initialTruncated := outputTruncated
			208	+	var finalTruncated bool
			209	+	text, visibleCount, finalTruncated = formatWebSearchResults(visible)
			210	+	outputTruncated = initialTruncated \|\| finalTruncated
			211	+	for _, id := range refs[visibleCount:] {
			212	+	t.store.remove(id)
			213	+	}
			214	+	visible = visible[:visibleCount]
			215	+	if visibleCount == 0 {
			216	+	return webSearchError("web search: no usable HTTP(S) results"), nil
			217	+	}
203		}	218		}
204		return webSearchResultValue(text, false, map[string]any{	219		return webSearchResultValue(text, false, map[string]any{
205		"backend": "DuckDuckGo HTML",	220		"backend": "DuckDuckGo HTML",