From 73c756c430dfc8485606080a1c09c3f8581bdc72 Mon Sep 17 00:00:00 2001 From: Lanh Hoang Date: Wed, 12 Aug 2026 19:35:55 -0400 Subject: [PATCH 1/2] docs: define architecture deepening phases --- ...026-08-12-architecture-deepening-design.md | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-12-architecture-deepening-design.md diff --git a/docs/superpowers/specs/2026-08-12-architecture-deepening-design.md b/docs/superpowers/specs/2026-08-12-architecture-deepening-design.md new file mode 100644 index 0000000..45f3fa1 --- /dev/null +++ b/docs/superpowers/specs/2026-08-12-architecture-deepening-design.md @@ -0,0 +1,124 @@ +# Pi Tools Architecture Deepening Design + +## Status + +Approved design for a phased implementation plan. The work covers all five candidates from the whole-repository architecture review, ordered from simplest to most complex. Each phase is independently mergeable, has a usable result, preserves unrelated public behavior, and leaves the full test suite passing. + +## Objective + +Improve the highest-leverage seams in `@pi-vault/pi-tools` without turning the review into a broad rewrite. The implementation will concentrate policy, remove duplicated provider knowledge, and make runtime behavior observable through stable interfaces. + +The work will use existing TypeScript types, native `fetch`, the current provider registry, and Vitest. It will not add dependencies or split modules solely because they are long. + +## Current friction + +The review found five related opportunities: + +1. `SearchProvider` accepts domain and date filters even though several adapters ignore them silently. +2. Provider identity is repeated across provider metadata, configuration defaults, environment mappings, and dashboard discovery. +3. Provider selection, fallback/fusion execution, budget reservation, activity logging, and outcome recording have different paths for different capabilities. +4. `ConfigManager` refreshes provider instances during a session while optional tool registration, guidance, and combine settings are captured at session start. +5. Content extraction calls Jina and Gemini fallbacks directly while `web_fetch` has a separate registered-provider fallback; Jina has duplicate implementations. + +The deletion test rules out splitting `src/extract/pipeline.ts` merely because it is 548 lines: its small interface hides substantial routing, validation, and fallback behavior. The improvement belongs at the external adapter seam, not in arbitrary file subdivision. + +## Target architecture + +The intended dependency direction is: + +```text +config + ↓ +provider catalog + ↓ +provider registry + ↓ +provider operation policy + ↓ +search / fetch / docs / research tools + +web_fetch + ↓ +content extraction module + ↓ +ordered extraction adapters + ↓ +provider operation policy +``` + +The provider catalog owns built-in provider facts. User configuration remains the override implementation. The provider registry continues to own budget reservation and registered adapters. The provider operation policy owns capability-aware selection and execution behavior. The extraction module remains deep: it owns routing order, validation, and result shaping while external extraction adapters cross one explicit seam. + +## Locked design decisions + +- Five phases are required; none may be skipped because a later phase appears more valuable. +- Phase order is dependency-first and simplest-to-most-complex: + 1. search filter semantics; + 2. provider catalog; + 3. provider operation policy; + 4. configuration/tool lifecycle; + 5. content extraction seam. +- Every phase is independently mergeable and rollbackable. +- Every phase has a focused test gate and runs the full existing test suite before completion. +- Phase 1 does not change provider ordering or selection strategy. +- Phase 2 keeps user overrides in configuration and moves only built-in provider facts into the catalog. +- Phase 3 preserves current tier ordering for `auto`; `best-performing` becomes capability-aware. Budget reservation still happens before provider delegation. +- Phase 4 aligns tool availability and guidance with current session state without assuming that the host can unregister a tool dynamically. +- Phase 5 preserves the existing extraction order and Gemini fallback behavior while giving external adapters one seam and removing duplicate Jina behavior. +- No new dependency, SDK, generic framework, or speculative abstraction is part of the work. + +## Phase contracts + +### Phase 1 — explicit search filter semantics + +Usable result: a caller can determine whether requested domain/date filters are enforced. Each adapter either honors the filter, uses a safe central post-filter, or reports the unsupported condition explicitly. Silent filter loss is removed. Existing provider ordering and fallback behavior stay unchanged. + +### Phase 2 — single provider catalog + +Usable result: adding a provider’s identity no longer requires editing several independent metadata lists. The catalog supplies identity, tier, credential mapping, and built-in defaults to configuration, registration, and the dashboard. Project and global user overrides remain authoritative. + +### Phase 3 — capability-aware provider operation policy + +Usable result: all capabilities use one coherent selection and execution policy. Search keeps tier ordering under `auto`; other capabilities no longer accidentally rely on registration order when the selected strategy requires more. Fallback and fusion share activity and outcome recording, and fetch outcomes can feed performance metrics. + +### Phase 4 — aligned configuration and tool lifecycle + +Usable result: a refreshed configuration has one runtime source of truth. Provider availability, guidance, combine settings, and optional docs/research tool behavior no longer depend on stale session-start snapshots. The implementation remains compatible with a host that only supports registration at session initialization. + +### Phase 5 — single content extraction seam + +Usable result: `web_fetch` still follows its established extraction order, but Jina, Gemini, and registered fetch adapters are selected through one explicit extraction policy. Jina transport and result shaping have one implementation. External fallback behavior is covered through the seam rather than through duplicated direct calls. + +## Data flow and error rules + +- Configuration errors retain the current safe behavior: malformed non-strict configuration falls back where it currently does; strict refresh keeps the prior usable configuration when refresh fails. +- Budget errors remain `BudgetExceededError` and are not recorded as provider failures. +- Provider failures continue through `AggregateProviderError` and existing fallback semantics. +- Unsupported search filters must be explicit and must not silently claim a constrained result set. +- Runtime refresh must not make an already running tool use a partially updated configuration. +- Extraction must preserve SSRF validation, binary/size checks, redirect validation, cancellation, and existing `RetryableExtractionError` behavior. +- No phase may weaken trust gating, credential handling, or error sanitization. + +## Testing strategy + +Each phase plan will use the existing test style and add the smallest regression surface that proves its seam: + +- contract tests for filter support and unsupported behavior; +- catalog/config/registry integration tests for a provider’s single source of identity; +- selection, fallback, fusion, budget, activity, and metrics tests through the operation seam; +- post-refresh runtime tests for tool availability and current guidance/configuration; +- extraction routing, Jina deduplication, Gemini fallback, cancellation, and SSRF regression tests through the extraction seam. + +The baseline command is `pnpm check`. The current repository passes 87 test files and 1,410 tests, with pre-existing lint warnings and a Node engine warning because the environment is Node 23.11.0 while the package declares Node >=24.15.0. The plan will not include unrelated lint cleanup or an engine change. + +## Planned documents + +After this spec is reviewed, the implementation plan will be split into one main plan and five phase plans: + +- `docs/superpowers/plans/2026-08-12-architecture-deepening.md` +- `docs/superpowers/plans/2026-08-12-architecture-deepening-phase-1-filters.md` +- `docs/superpowers/plans/2026-08-12-architecture-deepening-phase-2-catalog.md` +- `docs/superpowers/plans/2026-08-12-architecture-deepening-phase-3-operation-policy.md` +- `docs/superpowers/plans/2026-08-12-architecture-deepening-phase-4-runtime-lifecycle.md` +- `docs/superpowers/plans/2026-08-12-architecture-deepening-phase-5-extraction-seam.md` + +The main plan will define cross-phase invariants, sequencing, rollout, rollback, and verification. Each phase plan will define its file map, atomic result, implementation tasks, tests, error rules, and completion gate. From de49fe22bad6def136328b38acb7ad7c62ad3192 Mon Sep 17 00:00:00 2001 From: Lanh Hoang Date: Wed, 12 Aug 2026 20:54:48 -0400 Subject: [PATCH 2/2] docs: add implementation plan and 5 phase plans --- ...-architecture-deepening-phase-1-filters.md | 433 +++++++++++++++++ ...-architecture-deepening-phase-2-catalog.md | 449 ++++++++++++++++++ ...ture-deepening-phase-3-operation-policy.md | 397 ++++++++++++++++ ...ure-deepening-phase-4-runtime-lifecycle.md | 303 ++++++++++++ ...cture-deepening-phase-5-extraction-seam.md | 418 ++++++++++++++++ .../2026-08-12-architecture-deepening.md | 99 ++++ 6 files changed, 2099 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-12-architecture-deepening-phase-1-filters.md create mode 100644 docs/superpowers/plans/2026-08-12-architecture-deepening-phase-2-catalog.md create mode 100644 docs/superpowers/plans/2026-08-12-architecture-deepening-phase-3-operation-policy.md create mode 100644 docs/superpowers/plans/2026-08-12-architecture-deepening-phase-4-runtime-lifecycle.md create mode 100644 docs/superpowers/plans/2026-08-12-architecture-deepening-phase-5-extraction-seam.md create mode 100644 docs/superpowers/plans/2026-08-12-architecture-deepening.md diff --git a/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-1-filters.md b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-1-filters.md new file mode 100644 index 0000000..6b4360a --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-1-filters.md @@ -0,0 +1,433 @@ +# Pi Tools Phase 1: Explicit Search Filter Semantics Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop search providers from silently ignoring requested domain/date filters. + +**Architecture:** Add a small filter-support contract to `SearchProvider`. Providers declare whether domain filters are native, locally post-filtered, or unsupported, and whether date filters are native or unsupported. `web_search` removes unsupported candidates before execution and applies the existing safe domain post-filter where declared. + +**Tech Stack:** TypeScript, native `URL`, existing filter helpers, Vitest. + +--- + +## Atomic result + +After this phase, an unfiltered search behaves exactly as before. A filtered search either uses a provider that enforces every requested filter, locally filters domains when the provider declares that mode, or returns an explicit error naming unsupported filter groups. Provider ordering, budgets, fallback, and fusion remain unchanged for eligible providers. + +## File map + +Modify: + +- `src/providers/types.ts` — define `SearchFilterMode`, `SearchFilterSupport`, and require `SearchProvider.filterSupport`. +- `src/utils/filters.ts` — add safe host-based domain post-filtering and unsupported-filter detection. +- `src/providers/http-adapter.ts` — carry filter-support metadata through the shared HTTP adapter. +- `src/providers/http-providers.ts` — declare support for each HTTP provider configuration. +- `src/providers/duckduckgo.ts` — declare query-domain post-filter support and unsupported date semantics. +- `src/providers/exa.ts` — declare native domain/date support. +- `src/providers/tavily.ts` — declare native domain support and unsupported date semantics. +- `src/providers/serper.ts` — declare native domain/date support. +- `src/providers/tinyfish.ts` — declare native domain/date support. +- `src/providers/firecrawl.ts`, `src/providers/jina.ts`, `src/providers/ollama.ts`, `src/providers/openai-codex.ts`, `src/providers/openai-web-search.ts`, `src/providers/parallel.ts`, `src/providers/searxng.ts`, `src/providers/sofya.ts` — declare unsupported filter groups instead of accepting silent no-ops. +- `src/providers/registry.ts` — preserve `filterSupport` when wrapping registered providers. +- `src/tools/web-search.ts` — reject or filter candidates before fallback/fusion and apply local domain filtering. +- Search-provider test fixtures in `tests/config-manager.test.ts`, `tests/commands/tools.test.ts`, `tests/commands/tools-actions.test.ts`, `tests/commands/tools-dashboard.test.ts`, `tests/providers/registry.test.ts`, `tests/providers/types.test.ts`, and `tests/tools/web-search.test.ts` — satisfy the required provider contract. + +Create: + +- `tests/providers/filter-support.test.ts` — support matrix and domain filtering contract tests. + +## Tasks + +### Task 1: Add the filter-support contract and failing tests + +**Files:** + +- Modify: `src/providers/types.ts` +- Modify: `src/utils/filters.ts` +- Create: `tests/providers/filter-support.test.ts` +- Modify: `tests/providers/types.test.ts` + +- [ ] **Step 1: Define the support types.** + +Add the following types beside `SearchFilters` in `src/providers/types.ts`: + +```ts +export type SearchFilterMode = "native" | "post-filter" | "unsupported"; + +export interface SearchFilterSupport { + domains: SearchFilterMode; + dates: Exclude; +} + +export const UNSUPPORTED_SEARCH_FILTERS: SearchFilterSupport = { + domains: "unsupported", + dates: "unsupported", +}; +``` + +Add `readonly filterSupport: SearchFilterSupport;` to `SearchProvider`. Requiring the field makes a production adapter’s filter contract visible at its interface; test doubles will use `UNSUPPORTED_SEARCH_FILTERS` when they do not model filters. + +- [ ] **Step 2: Add safe domain filtering and unsupported-group detection.** + +Append these functions to `src/utils/filters.ts`: + +```ts +import type { SearchFilterSupport, SearchFilters, SearchResult } from "../providers/types.ts"; + +function hostMatchesDomain(urlValue: string, domain: string): boolean { + try { + const host = new URL(urlValue).hostname.toLowerCase(); + const normalized = domain.trim().toLowerCase().replace(/^\.+|\.+$/g, ""); + return normalized.length > 0 && (host === normalized || host.endsWith(`.${normalized}`)); + } catch { + return false; + } +} + +export function filterResultsByDomains( + results: SearchResult[], + filters?: SearchFilters, +): SearchResult[] { + if (!filters?.includeDomains?.length && !filters?.excludeDomains?.length) return results; + + return results.filter((result) => { + const included = + !filters.includeDomains?.length || + filters.includeDomains.some((domain) => hostMatchesDomain(result.url, domain)); + const excluded = + filters.excludeDomains?.some((domain) => hostMatchesDomain(result.url, domain)) ?? false; + return included && !excluded; + }); +} + +export function unsupportedSearchFilters( + filters: SearchFilters | undefined, + support: SearchFilterSupport, +): string[] { + const unsupported: string[] = []; + if ((filters?.includeDomains?.length || filters?.excludeDomains?.length) && support.domains === "unsupported") { + unsupported.push("domains"); + } + if ((filters?.startDate || filters?.endDate) && support.dates === "unsupported") { + unsupported.push("dates"); + } + return unsupported; +} +``` + +Keep `applyDomainFilters()` unchanged; it remains useful for providers that translate domains into upstream query syntax. The new post-filter must compare exact hosts and subdomains so `notexample.com` does not satisfy `example.com`. + +- [ ] **Step 3: Write the failing contract tests.** + +Add tests to `tests/providers/filter-support.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; +import { + filterResultsByDomains, + unsupportedSearchFilters, +} from "../../src/utils/filters.ts"; +import { + UNSUPPORTED_SEARCH_FILTERS, + type SearchFilterSupport, + type SearchResult, +} from "../../src/providers/types.ts"; + +const results: SearchResult[] = [ + { title: "Docs", url: "https://docs.example.com/a", snippet: "docs" }, + { title: "Root", url: "https://example.com/b", snippet: "root" }, + { title: "Lookalike", url: "https://notexample.com/c", snippet: "other" }, +]; + +describe("search filter support", () => { + it("filters included domains by host and subdomain", () => { + expect(filterResultsByDomains(results, { includeDomains: ["example.com"] })).toEqual( + results.slice(0, 2), + ); + }); + + it("filters excluded domains without matching lookalike hosts", () => { + expect(filterResultsByDomains(results, { excludeDomains: ["example.com"] })).toEqual([ + results[2], + ]); + }); + + it("reports unsupported domain and date groups", () => { + expect( + unsupportedSearchFilters( + { + includeDomains: ["example.com"], + startDate: "2026-01-01", + }, + UNSUPPORTED_SEARCH_FILTERS, + ), + ).toEqual(["domains", "dates"]); + }); + + it("allows native domains and dates", () => { + const support: SearchFilterSupport = { domains: "native", dates: "native" }; + expect( + unsupportedSearchFilters( + { includeDomains: ["example.com"], endDate: "2026-08-12" }, + support, + ), + ).toEqual([]); + }); +}); +``` + +Update every `SearchProvider` test double to include `filterSupport: UNSUPPORTED_SEARCH_FILTERS`. Do not give a fixture native support unless that test is specifically testing filter behavior. + +- [ ] **Step 4: Run the focused tests and confirm they fail for the missing contract.** + +Run: + +```bash +pnpm exec vitest run tests/providers/filter-support.test.ts tests/providers/types.test.ts +``` + +Expected: collection/type failures until the new types and functions exist. + +### Task 2: Declare support on production adapters + +**Files:** + +- Modify: `src/providers/http-adapter.ts` +- Modify: `src/providers/http-providers.ts` +- Modify: `src/providers/duckduckgo.ts` +- Modify: `src/providers/exa.ts` +- Modify: `src/providers/tavily.ts` +- Modify: `src/providers/serper.ts` +- Modify: `src/providers/tinyfish.ts` +- Modify: `src/providers/firecrawl.ts` +- Modify: `src/providers/jina.ts` +- Modify: `src/providers/ollama.ts` +- Modify: `src/providers/openai-codex.ts` +- Modify: `src/providers/openai-web-search.ts` +- Modify: `src/providers/parallel.ts` +- Modify: `src/providers/searxng.ts` +- Modify: `src/providers/sofya.ts` + +- [ ] **Step 1: Make the shared HTTP adapter carry support metadata.** + +Add an optional field to `HttpSearchConfig` and copy it onto the returned provider: + +```ts +export interface HttpSearchConfig { + // existing fields... + filterSupport?: SearchFilterSupport; +} + +export function createHttpSearchProvider(apiKey: string, config: HttpSearchConfig): SearchProvider { + return { + name: config.name, + label: config.label, + filterSupport: config.filterSupport ?? UNSUPPORTED_SEARCH_FILTERS, + async search(/* existing parameters */) { + // existing request and parser implementation + }, + }; +} +``` + +Use a new object spread or a frozen constant if needed to avoid a provider mutating the shared unsupported object. + +- [ ] **Step 2: Mark native and post-filter adapters.** + +Use these exact declarations: + +```ts +const NATIVE_DOMAIN_DATE_FILTERS: SearchFilterSupport = { + domains: "native", + dates: "native", +}; + +const NATIVE_DOMAIN_FILTERS: SearchFilterSupport = { + domains: "native", + dates: "unsupported", +}; + +const POST_FILTER_DOMAIN_FILTERS: SearchFilterSupport = { + domains: "post-filter", + dates: "unsupported", +}; +``` + +Set `filterSupport` as follows: + +- Brave Search, Exa, Serper, and TinyFish: `NATIVE_DOMAIN_DATE_FILTERS`. +- Tavily: `NATIVE_DOMAIN_FILTERS`. +- DuckDuckGo: `POST_FILTER_DOMAIN_FILTERS`; its date timelimit is approximate and its end date is not enforced, so dates remain unsupported. +- Brave LLM, fastCRW, LangSearch, Linkup, Marginalia, Perplexity, WebSearchAPI, and You.com: `UNSUPPORTED_SEARCH_FILTERS`. +- Firecrawl, Jina, Ollama, OpenAI Codex, OpenAI web search, Parallel, SearXNG, and Sofya: `UNSUPPORTED_SEARCH_FILTERS`. + +For class-based providers, add `readonly filterSupport = ...` beside `name` and `label`. For HTTP providers, pass the value in each `createHttpSearchProvider` config. Import the constants as type-safe values from `src/providers/types.ts`. + +- [ ] **Step 3: Preserve metadata through registry wrappers.** + +In `ProviderRegistry.registerProvider()`, copy the source metadata into the wrapped search provider: + +```ts +provider: { + name: provider.name, + label: provider.label, + filterSupport: provider.filterSupport, + search: async (query, maxResults, signal, filters) => { + this.consume(options.name, { capability: "search", maxResults }); + return provider.search(query, maxResults, signal, filters); + }, +}, +``` + +Do not apply filtering in the registry; the registry should preserve provider policy metadata and budget behavior, while the web-search module decides how requested filters affect a call. + +- [ ] **Step 4: Run provider contract tests.** + +Run: + +```bash +pnpm exec vitest run tests/providers/filter-support.test.ts tests/providers/http-adapter.test.ts tests/providers/registry.test.ts tests/providers/types.test.ts +``` + +Expected: all focused tests pass. + +### Task 3: Enforce the contract in `web_search` + +**Files:** + +- Modify: `src/tools/web-search.ts` +- Modify: `tests/tools/web-search.test.ts` + +- [ ] **Step 1: Add the explicit unsupported-filter result shape.** + +Extend `WebSearchDetails` without changing existing fields: + +```ts +interface WebSearchDetails { + provider: string; + resultCount: number; + unsupportedFilters?: string[]; + fusionMeta?: { + providersUsed: string[]; + degraded: boolean; + results: Array<{ url: string; providers: string[] }>; + }; +} +``` + +After `buildFilters(params)`, partition candidates with `unsupportedSearchFilters(filters, provider.filterSupport)`. If a candidate has unsupported groups, remove it. If no candidates remain and the original candidate list was non-empty, return: + +```ts +{ + content: [{ type: "text", text: `Search error: no eligible provider supports requested filters: ${groups.join(", ")}` }], + details: { provider: "none", resultCount: 0, unsupportedFilters: groups }, +} +``` + +If the original candidate list is empty, preserve the existing `Search error: No search providers available` result. + +- [ ] **Step 2: Apply only declared local domain post-filtering.** + +Add a local function in `web-search.ts`: + +```ts +function executeSearch( + provider: SearchProvider, + query: string, + maxResults: number, + signal: AbortSignal | undefined, + filters: SearchFilters | undefined, +): Promise { + return provider.search(query, maxResults, signal, filters).then((results) => + provider.filterSupport.domains === "post-filter" + ? filterResultsByDomains(results, filters) + : results, + ); +} +``` + +Use `executeSearch()` in both the fusion candidate mapping and the ordinary `executeWithFallback()` mapping. Do not post-filter native providers; their upstream contract already enforces domains and local filtering could alter provider ranking unexpectedly. + +- [ ] **Step 3: Add regression tests for explicit and post-filter behavior.** + +Add tests to `tests/tools/web-search.test.ts` that prove: + +1. An unsupported date provider is not called and returns a message containing `dates`. +2. A provider with `domains: "post-filter"` returns only exact-domain/subdomain results. +3. A fusion call excludes unsupported candidates before execution and still fuses the eligible providers. +4. An unfiltered call still sends the same candidate list and returns the same result formatting. + +Use providers shaped like this in the tests: + +```ts +const unsupported: SearchProvider = { + name: "unsupported", + label: "Unsupported", + filterSupport: UNSUPPORTED_SEARCH_FILTERS, + search: vi.fn().mockResolvedValue([]), +}; + +const postFiltered: SearchProvider = { + name: "post-filtered", + label: "Post-filtered", + filterSupport: { domains: "post-filter", dates: "unsupported" }, + search: vi.fn().mockResolvedValue([ + { title: "good", url: "https://docs.example.com/a", snippet: "good" }, + { title: "bad", url: "https://other.example/a", snippet: "bad" }, + ]), +}; +``` + +- [ ] **Step 4: Run focused tool tests.** + +Run: + +```bash +pnpm exec vitest run tests/tools/web-search.test.ts tests/providers/filter-support.test.ts +``` + +Expected: all tests pass. + +### Task 4: Complete fixture updates and commit the phase + +**Files:** + +- Modify: `tests/config-manager.test.ts` +- Modify: `tests/commands/tools.test.ts` +- Modify: `tests/commands/tools-actions.test.ts` +- Modify: `tests/commands/tools-dashboard.test.ts` +- Modify: `tests/providers/registry.test.ts` +- Modify: `tests/providers/types.test.ts` + +- [ ] **Step 1: Update all remaining SearchProvider fixtures.** + +Add `filterSupport: UNSUPPORTED_SEARCH_FILTERS` to each fixture returned or inline-constructed in the listed tests. Keep the field explicit rather than using a cast, so future test providers cannot silently omit the contract. + +- [ ] **Step 2: Run the complete phase gate.** + +Run: + +```bash +pnpm exec vitest run tests/providers/filter-support.test.ts tests/providers/http-adapter.test.ts tests/providers/registry.test.ts tests/providers/types.test.ts tests/tools/web-search.test.ts tests/commands/tools.test.ts tests/commands/tools-actions.test.ts tests/commands/tools-dashboard.test.ts tests/config-manager.test.ts +pnpm check +git diff --check +``` + +Expected: focused tests and the full suite pass; only the documented pre-existing Biome and Node-engine warnings remain. + +- [ ] **Step 3: Commit the atomic phase.** + +```bash +git add src/providers src/utils/filters.ts src/tools/web-search.ts tests +git commit -m "feat: make search filter support explicit" +``` + +The commit must not include catalog, runtime lifecycle, operation-policy, or extraction changes. + +## Phase completion gate + +- Filter support is declared on every production `SearchProvider`. +- Unsupported filtered calls fail explicitly before a provider request. +- Post-filter domains use exact host/subdomain matching. +- Unfiltered search behavior is unchanged. +- Focused tests, `pnpm check`, and `git diff --check` pass. diff --git a/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-2-catalog.md b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-2-catalog.md new file mode 100644 index 0000000..88c6a65 --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-2-catalog.md @@ -0,0 +1,449 @@ +# Pi Tools Phase 2: Single Provider Catalog Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Give provider identity, built-in defaults, and fallback environment names one source of truth. + +**Architecture:** Add a provider catalog beside the provider adapters. Each catalog entry owns a `ProviderMeta`, its default `ProviderConfigEntry`, and its fallback environment variable when one exists. `config.ts`, `ConfigManager`, the provider barrel, and the `/tools` dashboard consume catalog projections. Gemini remains a non-provider service entry in the same catalog module so its existing fallback environment mapping stays centralized without pretending Gemini is a registry provider. + +**Tech Stack:** TypeScript, existing provider metadata factories, existing config merge/validation, Vitest. + +--- + +## Atomic result + +After this phase, adding or removing a built-in provider requires changing one catalog entry and its adapter import. The catalog’s provider-name set, default-config set, and fallback-env set are checked against one another. User configuration still overrides catalog defaults through the existing merge path, and provider registration still skips disabled, unkeyed, or failing providers exactly as before. + +## File map + +Create: + +- `src/providers/catalog.ts` — provider catalog entries plus derived provider/default/fallback projections. +- `tests/providers/catalog.test.ts` — catalog consistency and projection tests. + +Modify: + +- `src/providers/all.ts` — re-export catalog projections for compatibility. +- `src/config.ts` — consume catalog defaults and fallback environment values; remove duplicate provider maps. +- `src/config-manager.ts` — receive the catalog’s provider metadata projection. +- `src/index.ts` — use catalog metadata for registration and `/tools` display. +- `src/providers/searxng.ts` — use the resolved key injected by `ConfigManager`, removing the config-to-provider value import. +- `tests/config.test.ts` — preserve fallback/default behavior while importing the catalog-backed export. +- `tests/config-manager.test.ts` — use catalog-shaped provider fixtures and verify the injected key path. +- `tests/providers/all.test.ts` — assert the compatibility barrel and catalog contain the same provider set. +- `tests/extract/config-video.test.ts` — retain the `gemini` fallback assertion from the catalog-backed map. + +Do not modify provider request behavior, budgets, selection order, or user-config validation in this phase. + +## Tasks + +### Task 1: Define the catalog and its consistency tests + +**Files:** + +- Create: `src/providers/catalog.ts` +- Create: `tests/providers/catalog.test.ts` +- Modify: `src/providers/all.ts` + +- [ ] **Step 1: Define the catalog entry shape.** + +Add these types and imports in `src/providers/catalog.ts`: + +```ts +import type { ProviderConfigEntry } from "../config.ts"; +import type { ProviderMeta } from "./types.ts"; + +export interface ProviderCatalogEntry { + readonly meta: ProviderMeta; + readonly defaultConfig: ProviderConfigEntry; + readonly fallbackEnv?: string; +} + +interface ServiceEnvironmentEntry { + readonly name: string; + readonly fallbackEnv: string; +} +``` + +Use a type-only import for config types. This keeps the catalog from importing config runtime code while `config.ts` imports catalog values. + +- [ ] **Step 2: Move the provider facts into one ordered catalog.** + +Move the provider imports currently assembled in `src/providers/all.ts` into `src/providers/catalog.ts`. Preserve the current order so registration and dashboard output remain stable. Resolve imported metadata by name, then define one ordered catalog literal containing each provider’s default config and fallback environment. This keeps identity, defaults, and fallback facts in one entry instead of three parallel maps. + +Define the metadata array first, using the current `all.ts` order: + +```ts +const providerMetas: readonly ProviderMeta[] = [ + ...httpProviders, + context7, + duckduckgo, + exa, + firecrawl, + jina, + ollama, + openaiCodex, + openaiWebSearch, + parallel, + searxng, + serper, + sofya, + tavily, + tinyfish, +]; +``` + +Then use a lookup helper that fails at module load if an adapter import and catalog entry diverge: + +```ts +const providerMetaByName = new Map( + providerMetas.map((meta) => [meta.name, meta] as const), +); + +function catalogEntry( + name: string, + defaultConfig: ProviderConfigEntry, + fallbackEnv?: string, +): ProviderCatalogEntry { + const meta = providerMetaByName.get(name); + if (!meta) throw new Error(`Missing provider metadata for ${name}`); + return Object.freeze({ meta, defaultConfig, fallbackEnv }); +} +``` + +The implementation must contain all 23 current provider defaults, including disabled and local/unlimited providers, with these exact values: + +```ts +const providerCatalog: readonly ProviderCatalogEntry[] = [ + catalogEntry("brave", { + enabled: true, + budget: { mode: "hard", limit: 5, period: "month", unit: "usd", pool: "brave" }, + apiKey: "BRAVE_API_KEY", + }, "BRAVE_API_KEY"), + catalogEntry("brave-llm", { + enabled: true, + budget: { mode: "hard", limit: 5, period: "month", unit: "usd", pool: "brave" }, + apiKey: "BRAVE_API_KEY", + }, "BRAVE_API_KEY"), + catalogEntry("context7", { + enabled: true, + budget: { mode: "hard", limit: 1000, period: "month", unit: "request" }, + apiKey: "CONTEXT7_API_KEY", + }, "CONTEXT7_API_KEY"), + catalogEntry("duckduckgo", { enabled: true, budget: { mode: "unlimited" } }), + catalogEntry("exa", { + enabled: true, + budget: { mode: "hard", limit: 10, period: "month", unit: "usd", pool: "exa" }, + apiKey: "EXA_API_KEY", + }, "EXA_API_KEY"), + catalogEntry("fastcrw", { + enabled: false, + budget: { mode: "hard", limit: 500, period: "lifetime", unit: "credit" }, + apiKey: "FASTCRW_API_KEY", + }, "FASTCRW_API_KEY"), + catalogEntry("firecrawl", { + enabled: true, + budget: { mode: "hard", limit: 1000, period: "month", unit: "credit" }, + apiKey: "FIRECRAWL_API_KEY", + }, "FIRECRAWL_API_KEY"), + catalogEntry("jina", { enabled: true, budget: { mode: "managed" } }, "JINA_API_KEY"), + catalogEntry("langsearch", { + enabled: false, + budget: { mode: "hard", limit: 1000, period: "day", unit: "request" }, + apiKey: "LANGSEARCH_API_KEY", + }, "LANGSEARCH_API_KEY"), + catalogEntry("linkup", { + enabled: false, + budget: { mode: "hard", limit: 20, period: "month", unit: "usd" }, + apiKey: "LINKUP_API_KEY", + }, "LINKUP_API_KEY"), + catalogEntry("marginalia", { enabled: false, budget: { mode: "managed" } }, "MARGINALIA_API_KEY"), + catalogEntry("ollama", { + enabled: false, + budget: { mode: "unlimited" }, + apiKey: "OLLAMA_API_KEY", + }, "OLLAMA_API_KEY"), + catalogEntry("openai-codex", { enabled: true, budget: { mode: "managed" } }), + catalogEntry("openai-web-search", { + enabled: true, + budget: { mode: "managed" }, + apiKey: "OPENAI_API_KEY", + }, "OPENAI_API_KEY"), + catalogEntry("parallel", { + enabled: false, + budget: { mode: "managed" }, + apiKey: "PARALLEL_API_KEY", + }, "PARALLEL_API_KEY"), + catalogEntry("perplexity", { + enabled: true, + budget: { mode: "managed" }, + apiKey: "PERPLEXITY_API_KEY", + }, "PERPLEXITY_API_KEY"), + catalogEntry("searxng", { + enabled: false, + budget: { mode: "unlimited" }, + instanceUrl: "http://localhost:8080", + }), + catalogEntry("serper", { + enabled: false, + budget: { mode: "hard", limit: 2500, period: "lifetime", unit: "request" }, + apiKey: "SERPER_API_KEY", + }, "SERPER_API_KEY"), + catalogEntry("sofya", { + enabled: false, + budget: { mode: "managed" }, + apiKey: "SOFYA_API_KEY", + }, "SOFYA_API_KEY"), + catalogEntry("tavily", { + enabled: false, + budget: { mode: "hard", limit: 1000, period: "month", unit: "credit" }, + apiKey: "TAVILY_API_KEY", + }, "TAVILY_API_KEY"), + catalogEntry("tinyfish", { + enabled: true, + budget: { mode: "unlimited" }, + apiKey: "TINYFISH_API_KEY", + }, "TINYFISH_API_KEY"), + catalogEntry("websearchapi", { + enabled: false, + budget: { mode: "hard", limit: 2000, period: "month", unit: "credit" }, + apiKey: "WEBSEARCHAPI_API_KEY", + }, "WEBSEARCHAPI_API_KEY"), + catalogEntry("youcom", { + enabled: false, + budget: { mode: "hard", limit: 100, period: "lifetime", unit: "usd" }, + apiKey: "YOUCOM_API_KEY", + }, "YOUCOM_API_KEY"), +]; + +const serviceEnvironmentEntries: readonly ServiceEnvironmentEntry[] = [ + { name: "gemini", fallbackEnv: "GEMINI_API_KEY" }, +]; +``` + +Derive all consumer projections from `providerCatalog` and keep the service fallback in the same environment map: + +```ts +export { providerCatalog }; + +export const allProviders: ProviderMeta[] = providerCatalog.map(({ meta }) => meta); + +export const defaultProviderConfigs: Record = Object.fromEntries( + providerCatalog.map(({ meta, defaultConfig }) => [meta.name, defaultConfig]), +); + +export const fallbackEnvMap: Record = Object.fromEntries([ + ...providerCatalog.flatMap(({ meta, fallbackEnv }) => + fallbackEnv ? [[meta.name, fallbackEnv] as const] : [], + ), + ...serviceEnvironmentEntries.map(({ name, fallbackEnv }) => [name, fallbackEnv] as const), +]); +``` + +Before exporting projections, validate that every catalog metadata name is unique, every imported metadata name has one catalog entry, and Gemini is not accidentally treated as a provider. A module-load error is preferable to silently dropping a provider from registration: + +```ts +const catalogNames = new Set(providerCatalog.map(({ meta }) => meta.name)); +if (catalogNames.size !== providerCatalog.length) throw new Error("Duplicate provider catalog name"); +if (catalogNames.size !== providerMetas.length) + throw new Error("Provider metadata and catalog entry counts differ"); +for (const meta of providerMetas) { + if (!catalogNames.has(meta.name)) throw new Error(`Provider ${meta.name} is missing from catalog`); +} +``` + +Do not export the mutable source tables. Export the catalog and projections only. + +- [ ] **Step 3: Add catalog consistency tests before consumers change.** + +Create `tests/providers/catalog.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; +import { + allProviders, + defaultProviderConfigs, + fallbackEnvMap, + providerCatalog, +} from "../../src/providers/catalog.ts"; + +describe("provider catalog", () => { + it("has one entry per provider and no duplicate names", () => { + const names = providerCatalog.map(({ meta }) => meta.name); + expect(names.length).toBe(23); + expect(new Set(names).size).toBe(names.length); + expect(new Set(allProviders.map(({ name }) => name))).toEqual(new Set(names)); + }); + + it("has a valid default config for every provider", () => { + for (const { meta, defaultConfig } of providerCatalog) { + expect(defaultProviderConfigs[meta.name]).toBe(defaultConfig); + expect(typeof defaultConfig.enabled).toBe("boolean"); + expect(defaultConfig.budget.mode).toMatch(/^(hard|managed|unlimited)$/); + } + }); + + it("keeps fallback env mappings in the catalog projection", () => { + expect(fallbackEnvMap.brave).toBe("BRAVE_API_KEY"); + expect(fallbackEnvMap.exa).toBe("EXA_API_KEY"); + expect(fallbackEnvMap.gemini).toBe("GEMINI_API_KEY"); + expect(fallbackEnvMap["openai-codex"]).toBeUndefined(); + }); +}); +``` + +Run: + +```bash +pnpm exec vitest run tests/providers/catalog.test.ts +``` + +Expected: the new catalog tests pass before the existing consumers are switched. + +- [ ] **Step 4: Keep the compatibility barrel.** + +Replace the provider assembly in `src/providers/all.ts` with: + +```ts +export { + allProviders, + providerCatalog, +} from "./catalog.ts"; +``` + +Existing imports of `allProviders` must continue to compile. Do not make callers import individual adapter modules just to access provider metadata. + +### Task 2: Make config consume catalog projections + +**Files:** + +- Modify: `src/config.ts` +- Modify: `src/providers/searxng.ts` +- Modify: `tests/config.test.ts` +- Modify: `tests/extract/config-video.test.ts` + +- [ ] **Step 1: Replace duplicate maps in `config.ts`.** + +Import the value projections from the catalog: + +```ts +import { defaultProviderConfigs, fallbackEnvMap } from "./providers/catalog.ts"; +``` + +Replace the manually maintained `FALLBACK_ENV_MAP` object with: + +```ts +export const FALLBACK_ENV_MAP = fallbackEnvMap; +``` + +Replace the literal `DEFAULT_CONFIG.providers` object with: + +```ts +providers: defaultProviderConfigs, +``` + +Leave `DEFAULT_GITHUB_CONFIG`, `DEFAULT_COMBINE_CONFIG`, `DEFAULT_DEEP_RESEARCH_CONFIG`, Gemini, YouTube, Video, and PDF defaults in `config.ts`; they are not provider catalog entries. Preserve the existing `loadMergedConfig` merge order and validation. + +- [ ] **Step 2: Break the provider-to-config runtime cycle in SearXNG.** + +`src/providers/searxng.ts` currently imports `resolveApiKey` as a runtime value. `ConfigManager` already resolves `providerConfig.apiKey` before calling `meta.create`, so pass the injected `key` directly: + +```ts +create: (key, providerConfig) => ({ + search: new SearXNGProvider({ + instanceUrl: providerConfig?.instanceUrl, + apiKey: key, + }), +}), +``` + +Remove the `resolveApiKey` import. This preserves explicit keys, env indirection, shell commands, and fallback env behavior because those paths still run through `ConfigManager` and `resolveProviderKey` at their existing boundaries. + +- [ ] **Step 3: Prove defaults and fallback behavior did not drift.** + +Keep the existing `FALLBACK_ENV_MAP` assertions and add a set comparison in `tests/config.test.ts` between `Object.keys(loadMergedConfig(...).providers)` and the catalog provider names. Retain the Gemini assertions in `tests/extract/config-video.test.ts`. + +Run: + +```bash +pnpm exec vitest run tests/providers/catalog.test.ts tests/providers/all.test.ts tests/config.test.ts tests/extract/config-video.test.ts +``` + +Expected: provider count, names, built-in budgets, fallback env resolution, and Gemini config defaults remain unchanged. + +### Task 3: Route registration and dashboard metadata through the catalog + +**Files:** + +- Modify: `src/config-manager.ts` +- Modify: `src/index.ts` +- Modify: `tests/config-manager.test.ts` +- Modify: `tests/providers/all.test.ts` + +- [ ] **Step 1: Pass the catalog projection into `ConfigManager`.** + +Keep `ConfigManager`’s provider metadata argument typed as `readonly ProviderMeta[]` and pass `allProviders` from the catalog. Do not make `ConfigManager` know about individual provider modules. If the constructor is widened from `ProviderMeta[]` to `readonly ProviderMeta[]`, update tests and callers without changing registration behavior. + +- [ ] **Step 2: Build dashboard identity from `providerCatalog`.** + +In `src/index.ts`, import `providerCatalog` alongside `allProviders` and derive the tier map and names from the catalog entries: + +```ts +const tierMap = new Map(providerCatalog.map(({ meta }) => [meta.name, meta.tier])); +const allProviderNames = providerCatalog.map(({ meta }) => meta.name); +``` + +Keep `allProviders` for the `ConfigManager` constructor until phase 3 changes the operation-policy boundary. The dashboard must continue showing all built-ins, including disabled providers. + +- [ ] **Step 3: Add the SearXNG injection regression test.** + +In `tests/config-manager.test.ts`, instantiate the real `searxng` metadata with a config key and spy on the resulting provider request headers. Assert that the resolved key is used without importing `config.ts` from the provider module. Keep the test deterministic by using the existing mocked `resolveApiKey`. + +- [ ] **Step 4: Run the registration and dashboard tests.** + +Run: + +```bash +pnpm exec vitest run tests/config-manager.test.ts tests/providers/all.test.ts tests/commands/tools.test.ts tests/commands/tools-actions.test.ts tests/commands/tools-dashboard.test.ts +``` + +Expected: all provider registration, name/tier display, and configuration command tests pass. + +### Task 4: Complete the phase gate and commit + +- [ ] **Step 1: Search for stale duplicate sources.** + +Run: + +```bash +rg -n "const (FALLBACK_ENV_MAP|DEFAULT_CONFIG)|export const allProviders|providerMeta as" src/config.ts src/providers src/index.ts +``` + +Expected: the provider default map and fallback map exist only as catalog projections; `all.ts` only re-exports catalog values; adapter imports remain in `catalog.ts`. + +- [ ] **Step 2: Run the complete phase gate.** + +```bash +pnpm exec vitest run tests/providers/catalog.test.ts tests/providers/all.test.ts tests/config.test.ts tests/config-manager.test.ts tests/extract/config-video.test.ts tests/commands/tools.test.ts tests/commands/tools-actions.test.ts tests/commands/tools-dashboard.test.ts +pnpm check +git diff --check +``` + +Expected: focused tests and the full suite pass with only the documented pre-existing Biome and Node-engine warnings. + +- [ ] **Step 3: Commit the atomic phase.** + +```bash +git add src/providers/catalog.ts src/providers/all.ts src/providers/searxng.ts src/config.ts src/config-manager.ts src/index.ts tests +git commit -m "refactor: centralize provider catalog" +``` + +The commit must not include operation execution, tool re-registration, or extraction fallback changes. + +## Phase completion gate + +- Every built-in provider has exactly one catalog entry and one default config. +- Fallback env names, including the Gemini service fallback, are catalog-derived. +- `allProviders`, `ConfigManager`, config loading, and the dashboard consume catalog projections. +- User overrides and provider registration behavior are unchanged. +- Focused tests, `pnpm check`, and `git diff --check` pass. diff --git a/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-3-operation-policy.md b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-3-operation-policy.md new file mode 100644 index 0000000..3db9a08 --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-3-operation-policy.md @@ -0,0 +1,397 @@ +# Pi Tools Phase 3: Capability-Aware Provider Operation Policy Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make provider selection, budgets, activity logging, fallback, fusion, and outcome metrics follow one capability-aware policy. + +**Architecture:** Keep provider registration and budget ownership in `ProviderRegistry`. Store tier metadata for every registered capability, not only search. Give fetch, code-search, and docs selection the same tier/performance policy as search. Extract one attempt runner from `executeWithFallback`; use it for fallback, fusion, code-search, docs, and research so activity and outcome hooks cannot drift between tools. + +**Tech Stack:** TypeScript, native `AbortSignal`, existing activity monitor and budget registry, Vitest. + +--- + +## Atomic result + +After this phase: + +- `auto` selects eligible providers in tier order for search, fetch, code-search, and docs. +- `best-performing` ranks every capability that has active metrics; explicit provider selection still wins when requested and eligible. +- Hard budgets are consumed by the registered wrapper before delegation for every capability. +- A successful attempt, ordinary provider failure, budget rejection, cancellation, and activity record have the same semantics in fallback, fusion, and direct capability tools. +- Search fusion keeps its targeted/all batching and RRF output unchanged. + +No tool is re-registered dynamically in this phase; that is phase 4. + +## File map + +Create: + +- None. Reuse `ProviderRegistry`, `execute.ts`, and `fusion.ts` as the operation-policy seam. + +Modify: + +- `src/providers/types.ts` — define the capability type used by selection helpers. +- `src/providers/registry.ts` — retain tier metadata for all capability maps, centralize eligible/performance selection, and expose execution hooks. +- `src/providers/execute.ts` — add the shared single-attempt runner and use it in fallback execution. +- `src/providers/fusion.ts` — use the shared attempt runner without changing fusion batching. +- `src/tools/web-search.ts` — accept the shared execution hooks while preserving filter behavior from phase 1. +- `src/tools/web-fetch.ts` — pass hooks through provider fallback. +- `src/tools/code-search.ts` — execute through the shared attempt runner. +- `src/tools/web-docs-search.ts` — execute through the shared attempt runner. +- `src/tools/web-docs-fetch.ts` — execute through the shared attempt runner. +- `src/tools/web-research.ts` — execute each Exa request through the shared attempt runner while preserving the budget callback. +- `src/index.ts` — construct one registry hook set and pass it to every tool factory. +- `tests/providers/registry.test.ts` — test selection order and metrics for every capability. +- `tests/providers/execute.test.ts` — test the shared attempt runner and fallback delegation. +- `tests/providers/fusion.test.ts` — test shared activity/outcome behavior in fusion. +- `tests/tools/web-fetch.test.ts`, `tests/tools/code-search.test.ts`, `tests/tools/web-docs-search.test.ts`, `tests/tools/web-docs-fetch.test.ts`, `tests/tools/web-research.test.ts` — cover direct capability execution. +- `tests/index.test.ts`, `tests/index-strategy.test.ts` — verify one hook set and strategy routing. + +## Tasks + +### Task 1: Give every registered capability a common selection policy + +**Files:** + +- Modify: `src/providers/types.ts` +- Modify: `src/providers/registry.ts` +- Modify: `tests/providers/registry.test.ts` + +- [ ] **Step 1: Define the capability union once.** + +Add this type beside `ProviderOperation` in `src/providers/types.ts`: + +```ts +export type ProviderCapability = ProviderOperation["capability"]; +``` + +Use it for selection helpers and hook operation labels. Do not replace the discriminated `ProviderOperation`; its operation-specific fields still drive usage-cost calculation. + +- [ ] **Step 2: Store tier metadata for every capability map.** + +Replace the search-only registration shape with a reusable internal shape: + +```ts +interface RegisteredProvider { + provider: T; + tier: ProviderTier; +} + +private searchProviders = new Map>(); +private fetchProviders = new Map>(); +private codeSearchProviders = new Map>(); +private docsProviders = new Map>(); +``` + +When `registerProvider()` stores fetch, code-search, or docs instances, store `{ provider, tier: options.tier }`. Keep the existing wrapper functions and their `consume()` calls. Phase 1’s `filterSupport` metadata must remain on wrapped search providers. + +- [ ] **Step 3: Factor eligibility and scoring without changing the search formula.** + +Keep `isEligible()` and the existing score weights (`successRate * 0.5 + speed * 0.3 + quality * 0.2`). Generalize the implementation over a map of `RegisteredProvider` and the provider-name key: + +```ts +private selectByTier( + entries: Map>, + name?: string, +): T[] { + if (name && name !== "auto") { + const entry = entries.get(name); + return entry && this.isEligible(name) ? [entry.provider] : []; + } + + return ([1, 2, 3] as const).flatMap((tier) => + [...entries].flatMap(([providerName, entry]) => + entry.tier === tier && this.isEligible(providerName) ? [entry.provider] : [], + ), + ); +} +``` + +Add a generic performance sorter that uses the map key for metrics and returns only budget-eligible entries. Providers without active samples retain the existing neutral score and stable map order. Do not score a provider from metrics belonging to a different capability operation if the registered provider name is absent from the capability map. + +- [ ] **Step 4: Preserve existing search APIs and extend other capabilities.** + +Keep these existing methods as compatibility wrappers: + +```ts +selectSearchCandidates(name?: string): SearchProvider[]; +selectSearchByPerformance(name?: string): SearchProvider | undefined; +selectSearchByPerformanceAll(): SearchProvider[]; +selectSearchForFusion(strategy: SelectionStrategy, name?: string): SearchProvider[]; +``` + +Update them to use the generalized helpers. Add strategy-aware methods for the other registered capabilities: + +```ts +selectFetchCandidates(strategy: SelectionStrategy = "auto"): FetchProvider[]; +selectCodeSearch(strategy: SelectionStrategy = "auto"): CodeSearchProvider | undefined; +selectDocs(name?: string): DocsProvider | undefined; +selectDocsByStrategy( + strategy: SelectionStrategy, + name?: string, +): DocsProvider | undefined; +``` + +For `best-performing`, `selectFetchCandidates()` returns all scored eligible fetch providers, while `selectCodeSearch()` and `selectDocsByStrategy()` return the first scored eligible provider. For `auto`, all methods use tier order. `selectDocs(name?)` remains the compatibility wrapper for the current auto/name API, and an explicit docs name is returned only when that provider is eligible. No new strategy is added. + +- [ ] **Step 5: Test capability selection and budget eligibility.** + +Add registry tests with three providers at tiers 1, 2, and 3, each exposing search/fetch/code/docs. Assert: + +1. `auto` returns `1 → 2 → 3` for fetch and search even when registration order differs. +2. `best-performing` returns the provider with the highest existing metrics for fetch and code-search. +3. An over-budget provider is absent from every capability selection, including an explicit name. +4. `selectDocsByStrategy("best-performing")` uses metrics and `selectDocsByStrategy("auto", "named")` retains the explicit provider only when eligible; the old `selectDocs("named")` call remains compatible. +5. Existing search fusion strategy tests still return the same candidate order. + +Run: + +```bash +pnpm exec vitest run tests/providers/registry.test.ts +``` + +Expected: focused registry tests pass with the existing selection behavior preserved for search. + +### Task 2: Centralize one provider-attempt execution path + +**Files:** + +- Modify: `src/providers/execute.ts` +- Modify: `src/providers/fusion.ts` +- Modify: `src/providers/registry.ts` +- Modify: `tests/providers/execute.test.ts` +- Modify: `tests/providers/fusion.test.ts` + +- [ ] **Step 1: Extract the single-attempt contract.** + +Export the existing candidate shape and add an execution hook type: + +```ts +export interface FallbackCandidate { + name: string; + execute: () => Promise; +} + +export interface ExecutionHooks { + onSuccess?: (providerName: string, latencyMs: number) => void; + onFailure?: (providerName: string) => void; +} + +export interface ExecuteAttemptOptions extends ExecutionHooks { + candidate: FallbackCandidate; + operation: string; + signal?: AbortSignal; + activityQuery?: string; +} +``` + +Implement `executeAttempt()` so it: + +1. checks `signal.throwIfAborted()` before work; +2. logs one API activity entry using `activityQuery ?? operation`; +3. measures latency and calls `onSuccess` only after the result and post-result abort check succeed; +4. logs completion with status 200 on success; +5. logs the sanitized activity error and calls `onFailure` for ordinary errors; +6. never calls `onFailure` for `BudgetExceededError`; and +7. rethrows the original error so the caller decides whether to continue, aggregate, or render it. + +The implementation must not catch and convert cancellation into a provider failure. Keep `BudgetExceededError` imported from the registry and preserve the existing `AggregateProviderError` messages. + +- [ ] **Step 2: Make fallback use the shared attempt.** + +Reduce `executeWithFallback()` to candidate iteration and aggregation: + +```ts +for (const candidate of candidates) { + try { + const result = await executeAttempt({ + candidate, + operation, + signal, + onSuccess, + onFailure, + }); + return { result, providerName: candidate.name }; + } catch (error) { + errors.push({ + provider: candidate.name, + error: error instanceof Error ? error.message : String(error), + }); + } +} +``` + +Leave the empty-candidate `AggregateProviderError` and final aggregation unchanged. + +- [ ] **Step 3: Make fusion use the shared attempt.** + +Add an optional `signal?: AbortSignal` to `FusionOptions`, pass it through `executeWithFusion()` to `executeTargeted()`, and check it before each batch. Replace the duplicated activity/timing/try-catch wrapper with `executeAttempt({ candidate: { name: candidate.name, execute: () => candidate.execute(perProvider) }, operation: "search", activityQuery: `fusion:${candidate.name}`, signal, onSuccess, onFailure })`. + +Preserve fusion’s existing behavior: batches run concurrently, empty results are successful but not usable, budget rejections are recorded in errors but not `providersFailed`, and a no-usable-result batch throws `AggregateProviderError("search", errors)`. Do not route fusion through sequential `executeWithFallback`; concurrency is part of the fusion contract. + +- [ ] **Step 4: Expose registry hooks instead of repeating outcome wiring.** + +Import `ExecutionHooks` as a type in `registry.ts` and add: + +```ts +getExecutionHooks(): ExecutionHooks { + return { + onSuccess: (providerName, latencyMs) => + this.recordOutcome(providerName, { success: true, latencyMs }), + onFailure: (providerName) => + this.recordOutcome(providerName, { success: false }), + }; +} +``` + +Return fresh closures so tests can replace registry instances without sharing state. Keep `recordResultQuality()` separate because it is search-result-specific, not generic provider execution. + +- [ ] **Step 5: Test one activity/outcome event per attempt.** + +Add tests that prove: + +1. fallback success logs one start/complete pair and one success hook; +2. fallback failure then success logs one error and one success, with one failure hook; +3. `BudgetExceededError` is aggregated without a failure hook; +4. cancellation is rethrown and does not call a failure hook; and +5. fusion uses the same hooks while preserving concurrent batch behavior and `providersFailed` semantics. + +Run: + +```bash +pnpm exec vitest run tests/providers/execute.test.ts tests/providers/fusion.test.ts +``` + +Expected: the focused execution tests pass with no duplicate activity entries. + +### Task 3: Route direct capability tools through the policy + +**Files:** + +- Modify: `src/tools/web-search.ts` +- Modify: `src/tools/web-fetch.ts` +- Modify: `src/tools/code-search.ts` +- Modify: `src/tools/web-docs-search.ts` +- Modify: `src/tools/web-docs-fetch.ts` +- Modify: `src/tools/web-research.ts` +- Modify: `src/index.ts` +- Modify: `tests/tools/web-fetch.test.ts` +- Modify: `tests/tools/code-search.test.ts` +- Modify: `tests/tools/web-docs-search.test.ts` +- Modify: `tests/tools/web-docs-fetch.test.ts` +- Modify: `tests/tools/web-research.test.ts` +- Modify: `tests/index.test.ts` +- Modify: `tests/index-strategy.test.ts` + +- [ ] **Step 1: Pass one hook set to tool factories.** + +Update factories that execute providers to accept `ExecutionHooks` as an optional argument. Keep existing argument order where possible and update tests at the call site. In `src/index.ts`, create the hooks once after the registry: + +```ts +const executionHooks = registry.getExecutionHooks(); +``` + +Pass it to web search, web fetch, code search, docs search, docs fetch, and web research. Keep the existing result-quality callback only on web search. + +- [ ] **Step 2: Use shared attempts in `web_search` and `web_fetch`.** + +Pass `executionHooks` to both `executeWithFallback()` and `executeWithFusion()` in `web-search.ts`. Keep phase 1 filter eligibility/post-filtering before the candidate’s `execute` function. + +In `web-fetch.ts`, pass `executionHooks` into the existing provider fallback. Do not move provider fallback into the extraction pipeline yet; phase 5 owns that seam. The current retryable pipeline error and sanitized combined error remain unchanged. + +- [ ] **Step 3: Wrap code-search and docs calls.** + +Replace direct provider calls with `executeAttempt()`: + +```ts +const results = await executeAttempt({ + candidate: { + name: provider.name, + execute: () => provider.codeSearch(params.query, maxResults, signal ?? undefined), + }, + operation: "code-search", + signal: signal ?? undefined, + ...executionHooks, +}); +``` + +Use operation names `docs-search` and `docs-fetch` for Context7. Preserve each tool’s current error contract: code-search renders a sanitized error result, docs tools allow the provider error to propagate as a failed tool result, and missing-provider messages remain unchanged. + +- [ ] **Step 4: Wrap each Exa research request.** + +Keep `beforeResearch` as the budget reservation callback, but place it inside the candidate execution passed to `executeAttempt()`: + +```ts +responses.push( + await executeAttempt({ + candidate: { + name: "exa", + execute: () => { + beforeResearch?.({ + capability: "research", + type: mode.type, + maxResults: mode.numResults, + contentTypes: 2 + (mode.summaryQuery ? 1 : 0), + }); + return client.deepResearch(request, signal ?? undefined); + }, + }, + operation: "research", + signal: signal ?? undefined, + ...executionHooks, + }), +); +``` + +Preserve the existing query deduplication, output paths, report rendering, and Exa key checks. A budget rejection must remain a budget rejection rather than becoming a provider failure metric. + +- [ ] **Step 5: Use strategy-aware selection for non-search tools.** + +Update resolver callbacks in `src/index.ts` to pass `configManager.current.selectionStrategy` to fetch, code-search, and `selectDocsByStrategy`. Keep the compatibility `selectDocs()` wrapper for callers that do not need an explicit strategy. Keep search’s existing explicit/default/combine behavior, and pass the tool `signal` through `executeWithFusion`. Add tests proving a runtime strategy of `best-performing` reaches each capability resolver. + +Run: + +```bash +pnpm exec vitest run tests/tools/web-fetch.test.ts tests/tools/code-search.test.ts tests/tools/web-docs-search.test.ts tests/tools/web-docs-fetch.test.ts tests/tools/web-research.test.ts tests/index.test.ts tests/index-strategy.test.ts +``` + +Expected: direct tools retain their result/error shapes while recording outcomes through the same hooks. + +### Task 4: Complete the phase gate and commit + +- [ ] **Step 1: Search for duplicate execution instrumentation.** + +Run: + +```bash +rg -n "activityMonitor\.log(Start|Complete|Error)|recordOutcome\(" src/providers src/tools src/index.ts +``` + +Expected: attempt lifecycle logging is implemented in `executeAttempt()`; tool code only supplies hooks or search quality recording. The fusion module contains no second timing/callback implementation. + +- [ ] **Step 2: Run the complete phase gate.** + +```bash +pnpm exec vitest run tests/providers/registry.test.ts tests/providers/execute.test.ts tests/providers/fusion.test.ts tests/tools/web-search.test.ts tests/tools/web-fetch.test.ts tests/tools/code-search.test.ts tests/tools/web-docs-search.test.ts tests/tools/web-docs-fetch.test.ts tests/tools/web-research.test.ts tests/index.test.ts tests/index-strategy.test.ts +pnpm check +git diff --check +``` + +Expected: focused tests and the full suite pass with only the documented pre-existing Biome and Node-engine warnings. + +- [ ] **Step 3: Commit the atomic phase.** + +```bash +git add src/providers src/tools src/index.ts tests +git commit -m "refactor: unify provider operation policy" +``` + +The commit must not include ConfigManager listeners, tool re-registration, or extraction fallback changes. + +## Phase completion gate + +- Tier and performance selection is capability-aware and budget-aware. +- Fallback, fusion, direct capability calls, and research share one attempt lifecycle. +- Budget rejections and cancellations preserve their existing non-failure semantics. +- Search fusion concurrency and output behavior are unchanged. +- Focused tests, `pnpm check`, and `git diff --check` pass. diff --git a/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-4-runtime-lifecycle.md b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-4-runtime-lifecycle.md new file mode 100644 index 0000000..5df3e51 --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-4-runtime-lifecycle.md @@ -0,0 +1,303 @@ +# Pi Tools Phase 4: Aligned Configuration and Tool Lifecycle Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make provider refresh, current configuration, and registered tool definitions share one session lifecycle. + +**Architecture:** Add a change listener to `ConfigManager`. `index.ts` owns one `registerTools()` closure that reads the manager’s current config and re-registers definitions when the manager applies a valid change. Pi replaces a tool definition by name, so re-registration updates prompt guidance and schemas without accumulating duplicates. Optional docs/research tools are registered at session start and check current availability during execution. + +**Tech Stack:** TypeScript, Pi `ExtensionAPI.registerTool`, existing config TTL refresh, Vitest. + +--- + +## Atomic result + +After this phase: + +- Provider enable/disable/key/config changes are visible to selection and tool definitions during the same session. +- Guidance changes are reflected by replacing the existing tool definition under the same name. +- `web_docs_search`, `web_docs_fetch`, and `web_research` have stable definitions from session start; without credentials they return their existing unavailable messages, and a later config/key refresh makes them usable without restart. +- A malformed config reload leaves the prior providers, config, and tool definitions untouched. +- Re-registering a tool does not create duplicate active definitions. + +Phase 4 does not change provider selection algorithms or move extraction fallbacks; phases 3 and 5 own those concerns. + +## File map + +Modify: + +- `src/config-manager.ts` — emit a callback after a valid changed config is applied. +- `src/index.ts` — own one refreshable tool-registration closure and register optional tools unconditionally. +- `src/tools/web-search.ts` — read combine settings through a current-config getter when provided. +- `src/tools/web-research.ts` — read deep-research settings through a current-config getter. +- `tests/helpers.ts` — model Pi’s same-name replacement behavior. +- `tests/config-manager.test.ts` — test change callbacks and no-op/malformed reloads. +- `tests/index.test.ts` — test stable tool names, unavailable-to-available transitions, and no duplicates. +- `tests/index-guidance.test.ts` — test guidance replacement after a config refresh. +- `tests/index-strategy.test.ts` — test current selection strategy after refresh. +- `tests/tools/web-search.test.ts` — cover dynamic combine settings. +- `tests/tools/web-research.test.ts` — cover dynamic deep-research enablement. + +No new runtime service or dependency is needed. The existing `ConfigManager` and Pi registration API are the seam. + +## Tasks + +### Task 1: Add a precise ConfigManager change callback + +**Files:** + +- Modify: `src/config-manager.ts` +- Modify: `tests/config-manager.test.ts` + +- [ ] **Step 1: Define the listener type and constructor option.** + +Add a local callback type: + +```ts +type ConfigChangeListener = (config: PiToolsConfig) => void; +``` + +Store an optional listener on `ConfigManager`. Keep the existing constructor arguments working by adding the listener after `modelRegistry`: + +```ts +constructor( + cwd: string, + registry: ProviderRegistry, + providerMetas: readonly ProviderMeta[], + modelRegistry?: ModelRegistry, + onChange?: ConfigChangeListener, +) { + // existing initialization + this.onChange = onChange; +} +``` + +The listener is not called during initial registration. Initial tool registration remains explicit in `index.ts`. + +- [ ] **Step 2: Detect both provider and non-provider behavior changes.** + +Keep `diffConfig()` as the provider registration diff. In `refresh()`, compute a `changed` flag before assigning `_config`: + +```ts +const changeSet = diffConfig(this._config, nextConfig, resolveApiKey); +const providerChanged = + changeSet.added.length > 0 || + changeSet.removed.length > 0 || + changeSet.changed.length > 0; +const configChanged = providerChanged || JSON.stringify(this._config) !== JSON.stringify(nextConfig); + +this.applyChanges(changeSet, nextConfig); +this._config = nextConfig; +this.cacheTime = now; +if (configChanged) this.onChange?.(this._config); +``` + +The explicit provider flag matters when an environment-backed key changes while the serialized config remains identical; `diffConfig()` already detects that resolved-key transition. Do not call the listener for a TTL hit, a no-op reload, or a malformed reload. Keep the malformed-config branch’s previous-config behavior. + +- [ ] **Step 3: Test callback timing and no-op behavior.** + +Add tests that: + +1. receive the new config after a provider enable/disable or key/structure change; +2. receive the new config after guidance, selection strategy, combine, or deep-research changes even when providers are unchanged; +3. are not called when the reloaded config is identical; +4. are not called when `loadMergedConfig()` throws; and +5. observe that the registry has already applied provider changes when the callback runs. + +Use `expireTtlForTest()` and the existing mocked `loadMergedConfig` rather than sleeping. + +Run: + +```bash +pnpm exec vitest run tests/config-manager.test.ts +``` + +Expected: focused ConfigManager tests pass. + +### Task 2: Make tool registration idempotent and refreshable + +**Files:** + +- Modify: `src/index.ts` +- Modify: `src/tools/web-search.ts` +- Modify: `src/tools/web-research.ts` +- Modify: `tests/helpers.ts` +- Modify: `tests/index.test.ts` +- Modify: `tests/index-guidance.test.ts` +- Modify: `tests/tools/web-search.test.ts` +- Modify: `tests/tools/web-research.test.ts` + +- [ ] **Step 1: Add one registration closure in `index.ts`.** + +Inside `createExtension`, keep the existing store/registry/cache setup and add: + +```ts +let registerTools: () => void = () => {}; +``` + +Inside `initializeSession`, create the manager with a callback that invokes the closure: + +```ts +configManager = new ConfigManager( + ctx.cwd, + registry, + allProviders, + ctx.modelRegistry, + () => registerTools(), +); +``` + +Define `registerTools` after the manager exists. It must create and call `pi.registerTool()` for all tool definitions using `configManager.current` for guidance and current config getters for execution. Call it once at the end of session initialization. + +Do not call `configManager.refresh()` from inside `registerTools()`. Refresh is triggered by resolver/getter callbacks; registration must consume the already-current config and remain side-effect free. + +- [ ] **Step 2: Keep resolver functions dynamic and register every stable tool name.** + +Keep candidate resolvers closing over `configManager` and pass the phase 3 strategy-aware selection methods. Replace conditional registration with stable registration for: + +```text +web_search +web_fetch +web_read +code_search +web_docs_search +web_docs_fetch +web_research +``` + +Docs and research resolvers continue to return unavailable when credentials or enablement are absent. Their definitions must still be present after `session_start`, so a later refresh can activate them. + +Use the current config for each execution boundary: + +```ts +const getCombineConfig = () => { + configManager.refresh(); + return configManager.current.combine; +}; + +const getDeepResearchConfig = () => { + configManager.refresh(); + return configManager.current.deepResearch; +}; +``` + +Pass `getCombineConfig` and `getDeepResearchConfig` to the factories. Keep guidance as registration-time data because the listener re-registers definitions whenever guidance changes. + +- [ ] **Step 3: Make `web_search` use current combine configuration.** + +Add an optional `getCombineConfig?: () => CombineConfig` factory argument. At execution start, use the getter when supplied: + +```ts +const activeCombineConfig = getCombineConfig?.() ?? combineConfig; +const combineActive = params.combine ?? activeCombineConfig?.enabled === true; +``` + +Use `activeCombineConfig` for fusion mode, target backends, and `k`. Preserve the current positional `combineConfig` fallback for direct tests and callers that do not provide a getter. + +- [ ] **Step 4: Make `web_research` use current deep-research configuration.** + +Change the factory to accept `getDeepResearchConfig?: () => DeepResearchConfig` in addition to the existing config argument. At execution start, after resolving the API key, read: + +```ts +const activeConfig = getDeepResearchConfig?.() ?? deepResearchConfig; +if (!activeConfig.enabled) { + throw new Error("web_research is disabled via deepResearch.enabled config."); +} +``` + +Use `activeConfig.modeDefaults` and `activeConfig`’s current output schema/guidance inputs wherever the captured config was previously used. Keep the key resolver and existing unavailable error unchanged. + +- [ ] **Step 5: Model same-name replacement in the test host.** + +Change `createMockPi().registerTool()` from append-only behavior to replacement by `tool.name`: + +```ts +registerTool(tool) { + const index = tools.findIndex((current) => current.name === tool.name); + if (index === -1) tools.push(tool); + else tools[index] = tool; +}, +``` + +This matches Pi’s `ExtensionAPI.registerTool()` contract and makes duplicate registration regressions visible. Add an assertion that `new Set(pi.tools.map((tool) => tool.name)).size === pi.tools.length` after session start and after a forced config refresh. + +### Task 3: Verify live transitions without weakening trust or error behavior + +**Files:** + +- Modify: `tests/index.test.ts` +- Modify: `tests/index-guidance.test.ts` +- Modify: `tests/index-strategy.test.ts` +- Modify: `tests/tools/web-search.test.ts` +- Modify: `tests/tools/web-research.test.ts` + +- [ ] **Step 1: Test optional tools are stable but availability is dynamic.** + +Start with no Exa or Context7 key. Assert all seven tool names are registered. Execute `web_research` and `web_docs_search` and assert their existing unavailable messages. + +Then make the mocked trusted config expose literal Exa/Context7 keys, advance/expire the manager TTL through a tool resolver or the `/tools` reload handler, and assert: + +1. the same tool names remain present exactly once; +2. the re-registered definitions contain the configured guidance; and +3. the provider resolver now returns the configured provider instead of the unavailable result. + +Do not expose a production-only test accessor just to reach the manager; use the existing refresh-triggering command/resolver path. + +- [ ] **Step 2: Test guidance replacement.** + +Load a config with a custom `guidance.web_search.promptSnippet`, start a session, and assert the registered definition uses it. Reload with a different snippet and assert the same named tool now has the new snippet, with no duplicate tool in `MockPi.tools`. + +- [ ] **Step 3: Test strategy and combine changes take effect.** + +Start with `selectionStrategy: "auto"` and combine disabled. Change the config to `best-performing` with combine enabled, trigger refresh, and assert the next search uses the performance selector and current fusion settings. Existing explicit `params.combine` overrides must continue to win over config. + +- [ ] **Step 4: Preserve trust gating.** + +Keep the existing trusted/untrusted project tests. For an untrusted project, credentials remain stripped and the stable optional tools return unavailable; no new path may reintroduce sensitive API keys, base URLs, browser cookies, or SSRF allow ranges. + +Run: + +```bash +pnpm exec vitest run tests/index.test.ts tests/index-guidance.test.ts tests/index-strategy.test.ts tests/tools/web-search.test.ts tests/tools/web-research.test.ts +``` + +Expected: tool replacement, dynamic configuration, guidance, strategy, combine, and trust tests pass. + +### Task 4: Complete the phase gate and commit + +- [ ] **Step 1: Check for conditional registration and append-only test behavior.** + +Run: + +```bash +rg -n "if \(.*docs|if \(.*research|tools\.push\(|registerTool\(" src/index.ts tests/helpers.ts +``` + +Expected: `index.ts` has one registration closure and stable optional tool names; `MockPi` replaces same-name entries. + +- [ ] **Step 2: Run the complete phase gate.** + +```bash +pnpm exec vitest run tests/config-manager.test.ts tests/index.test.ts tests/index-guidance.test.ts tests/index-strategy.test.ts tests/tools/web-search.test.ts tests/tools/web-research.test.ts tests/commands/tools.test.ts tests/commands/tools-actions.test.ts tests/commands/tools-dashboard.test.ts +pnpm check +git diff --check +``` + +Expected: focused tests and the full suite pass with only the documented pre-existing Biome and Node-engine warnings. + +- [ ] **Step 3: Commit the atomic phase.** + +```bash +git add src/config-manager.ts src/index.ts src/tools/web-search.ts src/tools/web-research.ts tests +git commit -m "refactor: align config refresh with tool lifecycle" +``` + +The commit must not include content extraction or provider catalog changes. + +## Phase completion gate + +- Config changes notify the session only after a valid changed reload. +- Tool definitions are replaced by name, not accumulated. +- Optional tools are stable at startup and dynamically available when current config permits. +- Guidance, strategy, combine, trust, and error behavior remain correct. +- Focused tests, `pnpm check`, and `git diff --check` pass. diff --git a/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-5-extraction-seam.md b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-5-extraction-seam.md new file mode 100644 index 0000000..1b36da8 --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-architecture-deepening-phase-5-extraction-seam.md @@ -0,0 +1,418 @@ +# Pi Tools Phase 5: Single Content Extraction Seam Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make registered fetch providers part of one content-extraction fallback seam and remove the duplicate direct Jina Reader implementation. + +**Architecture:** Keep routing and trust-sensitive HTTP handling in `src/extract/pipeline.ts`. Move shared result types into `src/extract/types.ts`. Add `src/extract/fallbacks.ts` as the seam that adapts registered `FetchProvider`s and Gemini extractors to the ordered extraction chain. `web_fetch` supplies the already registered, budget-wrapped fetch candidates and phase 3 execution hooks. The Jina transport remains only in `src/providers/jina.ts`. + +**Tech Stack:** TypeScript, native `fetch`, existing `executeWithFallback`, SSRF utilities, Gemini extractors, Vitest. + +--- + +## Atomic result + +After this phase, the normal HTML path is: + +```text +validated HTTP response + → Readability + → RSC + → registered fetch-provider fallback + → Gemini URL context + → Gemini Web + → raw text +``` + +The structured routes (GitHub, PDF, YouTube, local video, raw mode), SSRF validation, per-hop redirect validation, cancellation, cache/storage behavior, and retryable transport errors remain intact. A provider fallback is budgeted and instrumented through the registry wrapper and shared execution policy. `src/extract/jina-reader.ts` and its duplicate tests are removed. + +## File map + +Create: + +- `src/extract/types.ts` — shared `ExtractedContent`, `VideoFrame`, and `ImageBlock` types. +- `src/extract/fallbacks.ts` — ordered extraction fallback adapter and registered-fetch adapter. +- `tests/extract/fallbacks.test.ts` — fallback ordering, empty-result, cancellation, and provider-adapter tests. + +Modify: + +- `src/extract/pipeline.ts` — use shared types and fallback seam; remove direct Jina Reader call. +- `src/providers/jina.ts` — keep one Jina Reader transport and normalize empty/short content for the provider seam. +- `src/tools/web-fetch.ts` — pass registered fetch candidates and execution hooks into `extractContent`; remove the outer duplicate fallback. +- `tests/extract/pipeline.test.ts` — preserve normal, retryable, raw, PDF, and HTTP behavior with the new options. +- `tests/extract/pipeline-routing.test.ts` — replace direct Jina markers with registered-fetch seam markers and test ordering. +- `tests/extract/pipeline-ssrf.test.ts` — verify provider fallback is reached only after original URL validation. +- `tests/extract/cloudflare-retry.test.ts` — preserve challenge retry behavior. +- `tests/providers/jina.test.ts` — test the single Jina provider fetch path. +- `tests/tools/web-fetch.test.ts` — verify provider fallback, ordering, hooks, and error behavior through the pipeline. + +Delete: + +- `src/extract/jina-reader.ts` — duplicate direct Jina Reader transport. +- `tests/extract/jina-reader.test.ts` — tests for the deleted duplicate module; equivalent coverage moves to `fallbacks.test.ts` and `providers/jina.test.ts`. + +## Tasks + +### Task 1: Define the extraction fallback seam without changing routing + +**Files:** + +- Create: `src/extract/types.ts` +- Create: `src/extract/fallbacks.ts` +- Create: `tests/extract/fallbacks.test.ts` +- Modify: `src/extract/pipeline.ts` +- Modify: `src/tools/web-fetch.ts` + +- [ ] **Step 1: Move shared extraction result types.** + +Create `src/extract/types.ts` with the data currently declared in `pipeline.ts`: + +```ts +import type { PdfPageImage } from "./pdf-ocr.ts"; + +export interface VideoFrame { + data: string; + mimeType: string; + timestamp: string; +} + +export interface ExtractedContent { + text: string; + title?: string; + url: string; + extractionChain: string[]; + chars: number; + truncated: boolean; + contentId?: string; + thumbnail?: { data: string; mimeType: string }; + frames?: VideoFrame[]; + images?: PdfPageImage[]; + duration?: number; +} + +export type ImageBlock = { type: "image"; data: string; mimeType: string }; +``` + +In `pipeline.ts`, import these types and re-export them: + +```ts +export type { ExtractedContent, ImageBlock, VideoFrame } from "./types.ts"; +``` + +This keeps existing tool/test imports working while allowing the new fallback module to depend on the result contract without importing pipeline runtime code. + +- [ ] **Step 2: Define an ordered fallback adapter.** + +Create `src/extract/fallbacks.ts`: + +```ts +import type { ExecutionHooks } from "../providers/execute.ts"; +import { executeWithFallback } from "../providers/execute.ts"; +import type { FetchProvider } from "../providers/types.ts"; +import type { ExtractedContent } from "./types.ts"; + +export interface ExtractionFallback { + name: string; + run(): Promise; +} + +export function createFetchProviderFallback( + providers: readonly FetchProvider[], + url: string, + signal: AbortSignal | undefined, + hooks?: ExecutionHooks, +): ExtractionFallback | undefined { + if (providers.length === 0) return undefined; + + return { + name: "fetch-provider", + async run() { + const { result, providerName } = await executeWithFallback({ + candidates: providers.map((provider) => ({ + name: provider.name, + execute: async () => { + const fetched = await provider.fetch(url, signal); + const text = fetched.text.trim(); + if (text.length === 0) throw new Error(`${provider.name} returned empty content`); + return { fetched, providerName: provider.name }; + }, + })), + operation: "fetch", + signal, + ...hooks, + }); + + const text = result.fetched.text.trim(); + return { + text, + title: result.fetched.title, + url, + extractionChain: [`fetch-provider:${result.providerName}`], + chars: text.length, + truncated: false, + }; + }, + }; +} + +export async function runExtractionFallbacks( + fallbacks: readonly ExtractionFallback[], + chain: string[], + signal?: AbortSignal, +): Promise { + for (const fallback of fallbacks) { + signal?.throwIfAborted(); + try { + const result = await fallback.run(); + signal?.throwIfAborted(); + if (!result) { + chain.push(`${fallback.name}:fail`); + continue; + } + + chain.push(result.extractionChain[0] ?? fallback.name); + return { ...result, extractionChain: chain }; + } catch (error) { + signal?.throwIfAborted(); + chain.push(`${fallback.name}:error`); + // The next adapter gets a chance. executeWithFallback has already + // aggregated and instrumented individual registered provider failures. + } + } + return undefined; +} +``` + +The implementation may use a narrower internal return type, but it must preserve these rules: + +- empty provider text advances to the next provider; +- `AbortError`/caller cancellation is rethrown, never converted into a fallback failure; +- registered provider attempts use `executeWithFallback` so budgets, activity, metrics, and aggregate errors remain centralized; and +- a failed adapter adds a chain marker but does not expose raw provider credentials or unsanitized errors. + +- [ ] **Step 3: Add seam tests before pipeline wiring.** + +In `tests/extract/fallbacks.test.ts`, add tests that prove: + +1. providers are tried in the supplied order; +2. an empty first result advances to the second provider; +3. a successful provider returns `fetch-provider:` and preserves title/chars/url; +4. all provider failures allow the next Gemini-style adapter to run; +5. a caller abort rejects with the abort error and does not call later adapters; and +6. execution hooks receive provider success/failure while `BudgetExceededError` remains excluded from failure metrics. + +Mock `executeWithFallback` only if needed for deterministic adapter tests; retain at least one test with real registered-style provider functions so the budget/execution boundary is exercised by the phase gate. + +- [ ] **Step 4: Add provider candidates and hooks to `ExtractOptions`.** + +Extend the existing options in `pipeline.ts`: + +```ts +import type { ExecutionHooks } from "../providers/execute.ts"; +import type { FetchProvider } from "../providers/types.ts"; + +export interface ExtractOptions { + // existing fields... + fetchProviders?: readonly FetchProvider[]; + executionHooks?: ExecutionHooks; +} +``` + +Do not make the extraction module select raw provider registry entries. It receives already selected, registry-wrapped candidates from `web_fetch`. + +- [ ] **Step 5: Pass the seam inputs from `web_fetch`.** + +In `src/tools/web-fetch.ts`: + +- keep `resolveFetchCandidates` as the existing resolver boundary; +- resolve candidates immediately before calling `extractContent`; +- pass `fetchProviders: resolveFetchCandidates?.() ?? []` and the phase 3 execution hooks; and +- remove the `RetryableExtractionError` catch that separately calls `executeWithFallback`. + +The `fetchUrl()` body should have one extraction call and one cache write. Keep the current cache key, content store writes, multi-URL concurrency, image blocks, and sanitized `errorResult` behavior. + +Run: + +```bash +pnpm exec vitest run tests/extract/fallbacks.test.ts tests/tools/web-fetch.test.ts +``` + +Expected: the new seam and existing provider fallback tests pass after the initial pipeline integration. + +### Task 2: Wire the ordered seam into the pipeline and remove duplicate Jina + +**Files:** + +- Modify: `src/extract/pipeline.ts` +- Modify: `src/providers/jina.ts` +- Delete: `src/extract/jina-reader.ts` +- Delete: `tests/extract/jina-reader.test.ts` +- Modify: `tests/extract/pipeline.test.ts` +- Modify: `tests/extract/pipeline-routing.test.ts` +- Modify: `tests/providers/jina.test.ts` + +- [ ] **Step 1: Preserve all pre-fallback validation and direct routes.** + +Keep these pipeline sections in their current order and behavior: + +1. frame extraction options; +2. local video routing; +3. YouTube routing; +4. original URL SSRF validation; +5. GitHub structured extraction; +6. HEAD probe and binary/size checks; +7. validated HTTP GET with per-hop redirect validation; +8. Cloudflare retry; and +9. raw/PDF/direct HTML/RSC extraction. + +Do not pass unvalidated URLs to a registered fetch provider. The original `validateUrl()` must still run before any provider fallback, and `fetchValidated()` must continue validating every redirect hop for direct HTTP requests. + +- [ ] **Step 2: Replace the direct Jina tier with registered adapters.** + +After `chain.push("rsc:no-match")`, construct the fallback list: + +```ts +const fallbacks = [ + createFetchProviderFallback( + options?.fetchProviders ?? [], + url, + signal, + options?.executionHooks, + ), + { + name: "gemini-url-context", + run: () => extractWithUrlContext(url, signal), + }, + { + name: "gemini-web", + run: () => extractWithGeminiWeb(url, signal), + }, +].filter((fallback): fallback is NonNullable => Boolean(fallback)); + +const fallbackResult = await runExtractionFallbacks(fallbacks, chain, signal); +if (fallbackResult) return fallbackResult; +``` + +Then keep the existing raw-text fallback and final error, using the accumulated `chain`. Remove `extractViaJinaReader` import and all `jina-reader` chain markers. For a call with no registered providers, the chain should identify the skipped provider group consistently (for example `fetch-provider:skipped`) before Gemini; choose one marker and update tests to assert that exact contract. + +- [ ] **Step 3: Preserve retryable transport fallback semantics.** + +When the initial validated GET or Cloudflare retry fails with a network error, 429, or 5xx, call only the registered fetch-provider fallback with the original URL before throwing. If a provider succeeds, return its normalized `ExtractedContent`. If all providers fail, throw a `RetryableExtractionError` so callers and tests can still distinguish retryable transport failure from a permanent HTTP error. + +Do not use the raw/Gemini HTML path when there is no response body. Preserve SSRF errors as `SSRFError`, preserve caller cancellation, and do not turn `BudgetExceededError` into a retryable network error. The error message may include a short sanitized fallback summary, but the error class and URL redaction rules must remain intact. + +- [ ] **Step 4: Make the provider own Jina Reader transport.** + +Keep `JinaProvider.fetch()` as the only `r.jina.ai` request. Normalize its text before returning: + +```ts +const text = (await response.text()).trim(); +if (text.length < 100) { + throw new Error("Jina reader returned insufficient content"); +} +return { text }; +``` + +Retain its authorization header behavior and existing search behavior. Do not add a second direct fetch helper in the extraction module. The provider is already wrapped by `ProviderRegistry`, so its fetch budget and outcome hooks apply automatically. + +- [ ] **Step 5: Update extraction-chain and Jina tests.** + +Replace direct Jina-reader unit tests with: + +- fallback seam tests for adapter ordering and normalized `fetch-provider:` output; +- provider tests for Jina success, non-2xx error, short-content rejection, and authorization; and +- pipeline routing tests that inject a mock registered fetch provider and assert it runs before Gemini. + +Keep the existing thin HTML, Gemini URL context, Gemini Web, raw text, 4xx, 429, 5xx, and cancellation assertions. Only change assertions that named the deleted direct `jina-reader` implementation. + +Run: + +```bash +pnpm exec vitest run tests/extract/fallbacks.test.ts tests/extract/pipeline.test.ts tests/extract/pipeline-routing.test.ts tests/providers/jina.test.ts tests/tools/web-fetch.test.ts +``` + +Expected: focused extraction and provider tests pass, with no import or request path left for `src/extract/jina-reader.ts`. + +### Task 3: Prove security, structured-route, and multi-URL invariants + +**Files:** + +- Modify: `tests/extract/pipeline-ssrf.test.ts` +- Modify: `tests/extract/cloudflare-retry.test.ts` +- Modify: `tests/tools/web-fetch.test.ts` +- Modify: `tests/tools/web-fetch-video.test.ts` +- Modify: `tests/extract/pipeline.test.ts` + +- [ ] **Step 1: Test SSRF and redirect boundaries with provider candidates.** + +Add a test that supplies a mock fetch provider while requesting a private, link-local, or disallowed URL. Assert `extractContent` rejects with `SSRFError` before the provider’s `fetch` function is called. Keep the existing tests for redirect-to-private-IP and redirect-loop rejection; direct HTTP requests must still validate every hop. + +- [ ] **Step 2: Test fallback ordering and no duplicate Jina request.** + +For thin HTML, provide a mock provider and mocked Gemini adapters. Assert: + +1. Readability and RSC are attempted first; +2. the registered provider is called before either Gemini adapter; +3. Gemini is not called after a successful provider; and +4. there is no direct `r.jina.ai` request outside the Jina provider mock. + +For a provider failure, assert the next provider/Gemini adapter runs and the final extraction chain records the failure and success labels in order. + +- [ ] **Step 3: Test retryable HTTP and budget behavior.** + +For 429/5xx/network failure, assert a successful registered provider result is returned. When all registered providers fail, assert `RetryableExtractionError`. When the registry wrapper throws `BudgetExceededError`, assert no provider failure metric is recorded and the retryable class remains intact where no provider can serve the request. + +- [ ] **Step 4: Test structured routes bypass generic fallbacks.** + +Keep tests proving provider candidates are not called for successful YouTube, local video, GitHub, PDF, and raw-mode routes. Existing video frame and PDF OCR behavior must remain unchanged. Do not make provider fetch adapters handle binary content, frames, or PDFs. + +- [ ] **Step 5: Test cache and multi-URL behavior.** + +Keep the existing `web_fetch` tests for cache hits, `fresh`, duplicate URLs, concurrency limit, content IDs, image blocks, and partial failures. Assert the resolver is called for each actual uncached URL and the provider fallback result is stored exactly like direct extraction content. + +Run: + +```bash +pnpm exec vitest run tests/extract/pipeline-ssrf.test.ts tests/extract/cloudflare-retry.test.ts tests/extract/pipeline.test.ts tests/tools/web-fetch.test.ts tests/tools/web-fetch-video.test.ts +``` + +Expected: security and structured-route tests pass without weakening validation or changing cache/storage contracts. + +### Task 4: Complete the phase gate and commit + +- [ ] **Step 1: Search for duplicate Jina and outer fallback paths.** + +Run: + +```bash +rg -n "extractViaJinaReader|jina-reader|r\.jina\.ai|executeWithFallback|RetryableExtractionError" src/extract src/tools/web-fetch.ts +``` + +Expected: `r.jina.ai` appears only in `src/providers/jina.ts` and its provider tests; `web-fetch.ts` no longer owns a second provider fallback; `executeWithFallback` appears in the shared extraction adapter and provider execution policy. + +- [ ] **Step 2: Run the complete phase gate.** + +```bash +pnpm exec vitest run tests/extract/fallbacks.test.ts tests/extract/pipeline.test.ts tests/extract/pipeline-routing.test.ts tests/extract/pipeline-ssrf.test.ts tests/extract/cloudflare-retry.test.ts tests/providers/jina.test.ts tests/tools/web-fetch.test.ts tests/tools/web-fetch-video.test.ts +pnpm check +git diff --check +``` + +Expected: focused tests and the full suite pass with only the documented pre-existing Biome and Node-engine warnings. + +- [ ] **Step 3: Commit the atomic phase.** + +```bash +git add src/extract src/providers/jina.ts src/tools/web-fetch.ts tests +git commit -m "refactor: unify content extraction fallbacks" +``` + +The commit may delete `src/extract/jina-reader.ts` and its test, but must not change provider catalog, configuration lifecycle, or unrelated extraction routes. + +## Phase completion gate + +- Registered fetch providers are the only provider-backed extraction adapters. +- Direct Jina Reader transport is deleted from `src/extract`. +- Fallback order, retryable error class, budgets, activity, cancellation, SSRF, and structured routes are covered by tests. +- Cache, content storage, multi-URL, and rendering behavior remain intact. +- Focused tests, `pnpm check`, and `git diff --check` pass. diff --git a/docs/superpowers/plans/2026-08-12-architecture-deepening.md b/docs/superpowers/plans/2026-08-12-architecture-deepening.md new file mode 100644 index 0000000..25f361e --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-architecture-deepening.md @@ -0,0 +1,99 @@ +# Pi Tools Architecture Deepening Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Deepen all five reviewed architectural seams in `@pi-vault/pi-tools` through independently mergeable phases ordered from simplest to most complex. + +**Architecture:** Keep `src/extract/pipeline.ts` as the deep content-extraction module, but make its external fallback adapters explicit. Make provider metadata the catalog consumed by configuration, registration, and the dashboard. Concentrate capability selection, fallback/fusion execution, budgets, activity, and outcome recording behind the existing registry/execution seams, then align those seams with a refreshable session runtime. + +**Tech Stack:** TypeScript, Node native `fetch`, Pi extension APIs, TypeBox, Vitest, Biome, pnpm. + +--- + +## Execution order + +Implement and merge these phase plans in order: + +1. [Phase 1 — explicit search filter semantics](2026-08-12-architecture-deepening-phase-1-filters.md) +2. [Phase 2 — single provider catalog](2026-08-12-architecture-deepening-phase-2-catalog.md) +3. [Phase 3 — capability-aware provider operation policy](2026-08-12-architecture-deepening-phase-3-operation-policy.md) +4. [Phase 4 — aligned configuration and tool lifecycle](2026-08-12-architecture-deepening-phase-4-runtime-lifecycle.md) +5. [Phase 5 — single content extraction seam](2026-08-12-architecture-deepening-phase-5-extraction-seam.md) + +Each phase must be a separate mergeable change. A phase is complete only when its focused tests and `pnpm check` pass. Do not combine phase commits to hide a failing intermediate state. + +## Cross-phase invariants + +Keep these behaviors unchanged unless the phase plan explicitly names the change: + +- SSRF validation still runs before direct external HTTP fetches, every redirect hop is validated, and trust gating still strips sensitive project configuration. +- Hard-budget usage is reserved and persisted before provider delegation. A failed persistence write rolls usage back. `BudgetExceededError` is not a provider failure. +- Provider errors still use the existing fallback and `AggregateProviderError` semantics. +- Tool result error sanitization remains in place. +- `web_fetch` still supports one URL, multiple URLs, raw mode, PDFs, GitHub URLs, YouTube, local video, cache, session content storage, and `web_read` retrieval. +- No new package or SDK is added. Use existing TypeScript, native `fetch`, and test helpers. +- Do not split a file solely because it is long. A new file is justified only when it owns a concrete interface at a seam. +- Do not change the Node engine declaration or clean unrelated Biome warnings. + +## Baseline and verification + +Before each phase, run: + +```bash +pnpm check +``` + +Expected baseline in the current environment: + +- 87 test files pass. +- 1,410 tests pass. +- Biome reports pre-existing warnings. +- pnpm reports the environment is Node 23.11.0 while `package.json` declares Node `>=24.15.0`. + +The implementation environment should use Node `>=24.15.0` when available. If the environment remains on Node 23, record the engine warning but do not change the project requirement as part of this work. + +At every phase gate run the focused command listed in that phase plan, then: + +```bash +pnpm check +git diff --check +git status --short +``` + +`git status --short` must contain only intended phase files before committing. + +## Rollout and rollback + +Use one branch or worktree per phase. Merge only after the phase gate passes. If a phase must be reverted, revert its phase commit as a whole; do not manually undo only half of a seam change. The previous phase must continue to compile and pass its tests after a later phase is reverted. + +## Phase contracts + +### Phase 1 result + +Requested search domain/date filters are either enforced natively, applied through a safe local domain post-filter, or rejected with an explicit unsupported-filter error. A provider that silently ignores a requested filter is no longer eligible for that filtered call. Provider ordering is unchanged for unfiltered calls. + +### Phase 2 result + +Provider identity facts, fallback environment names, and built-in provider defaults have one catalog source. `config.ts`, `ConfigManager`, `allProviders`, and the dashboard consume that catalog. User config remains the higher-priority override. + +### Phase 3 result + +Search, fetch, code-search, docs, and research use coherent capability-aware selection/execution rules. Search `auto` retains tier ordering. Performance selection can be applied to every capability that has metrics. Fallback and fusion share activity and outcome recording. + +### Phase 4 result + +The session has one refreshable runtime source. Provider refresh and tool definitions use the same current config. Optional docs/research definitions are registered at session start so later key/config changes are usable; their execution checks current availability. Changed guidance is re-registered through Pi’s name-replacing `registerTool` path. + +### Phase 5 result + +`web_fetch` passes registered fetch adapters into one extraction fallback seam. The extraction order remains direct HTTP/structured extraction, registered fetch adapters, Gemini fallbacks, and raw text. The duplicate direct Jina implementation is removed; Jina transport is owned by the provider adapter. + +## Completion checklist + +- [ ] Phase 1 merged and its focused filter tests pass. +- [ ] Phase 2 merged and catalog/config/registration tests pass. +- [ ] Phase 3 merged and provider policy tests pass. +- [ ] Phase 4 merged and runtime refresh/tool registration tests pass. +- [ ] Phase 5 merged and extraction/fallback/SSRF tests pass. +- [ ] Final `pnpm check` passes with only documented pre-existing warnings. +- [ ] README or changelog updates are made only where a user-visible behavior or configuration contract changed; no architecture essay is added.