portfolio-optimiser/docs/extending.md
Kjell Tore Guttormsen 9668e17f2f feat(mcp): concrete MCP servers become tools the agents can call during a run
Krav 3, and the operator chose the run path explicitly: the external service must
be reachable WHILE the run works, not only when documents are ingested. Until now
the run path had one in-process tool against a local folder — and on the bundle
path the agents had no tools at all.

MAF already ships the client (MCPStdioTool / MCPStreamableHTTPTool, verified in
the pinned 1.9.0 with allowed_tools and request_timeout), so `mcp_tools.py` owns
only what MAF cannot decide for us: which servers a run may contact, which of
their tools it may call, how long it waits, and where the credential comes from.
This is a DIFFERENT seam from ingest_mcp.py on purpose — that one pulls source
documents before a run and speaks to null-argument tools. Same protocol, different
job.

Every refusal is a live hazard, not tidiness. An empty allowlist would let the far
end decide what the agents may call, so naming the tools is mandatory. A
non-positive timeout is an unbounded wait against a third party. An unknown field
is refused rather than ignored, which is also what keeps a literal secret from
being parked in the config — there is no field for one, only the NAME of an env
var. A named-but-unset credential refuses instead of calling anonymously, because
an anonymous call can succeed with the wrong scope.

Egress is declared, always. Every server and permitted tool is named in the run
announcement before the first call — including when no --mandate is given, which
was a real hole: the announcement only printed with a commission, so configuring
servers without one would have contacted third parties with nothing printed at
all. --live-dry-run still opens nothing, because the tools are entered after the
dry-run cut: the promise to stop before the first call now covers egress too.

Threaded through BOTH modes. A flag accepted in one mode and silently dropped in
the other is the defect class this CLI refuses by name.

Load-bearing MEASURED against the whole 744-test suite, four mutations all red:
build the tools but never hand them to the agents (2) · never enter the
AsyncExitStack, so they are constructed and useless (1) · never declare the egress
(2) · drop the allowlist on the built client (1).

Two live docs claimed MCP was unwired in the run path; both corrected rather than
left to rot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULCqjLF61rehj5cZmdUoR3
2026-08-05 16:53:07 +02:00

320 lines
22 KiB
Markdown

# Extending the framework (extension points)
portfolio-optimiser is a generic core with explicit **config seams** (D4/D5, 90 %-prinsippet):
you onboard a new project, a new data source, or a new model-map **without editing the core
`src/portfolio_optimiser/*.py`**. The three guides below name the exact seam for each.
> **Honesty note (rent teknisk rammeverk).** The bundled reference domain
> (`data/reference_projects.json` + `data/docs/<id>/`) is a set of **SYNTHETIC, AI-authored
> fixtures** — fictional construction-cost projects, dummy estimates, and placeholder
> `verdict_input` decisions. They are flagged in each file's `_note`. A production deployer
> **replaces the data source** with their own and supplies **Layer-2 verdicts via real HITL**
> (fageksperter), not static config. The static `verdict_input` field is a test-fixture
> convenience that stands in for the durable HITL verdict in the offline synthetic framework.
## Legg til eget prosjekt
A new project is **config + docs only** — no code change (this is exercised by the SC1 test
`test_e_new_project_flows_through_via_config_only` + the `test_f_no_hardcoded_project_ids_in_src`
guard, which fails if any project id leaks into `src/`).
1. Append an entry to `src/portfolio_optimiser/data/reference_projects.json` with the full key
set: `id`, `name`, `description`, `currency`, `cost_items`, **`docs_dir`**, and
**`verdict_input`** (`{"decision", "rationale"}`).
- `docs_dir` is a path **relative to the package `data/` root** (e.g. `"docs/MY-PROJECT"`);
the loader (`reference_domain.load_reference_projects`) resolves it to an absolute path.
- `verdict_input` carries the (synthetic) Layer-2 decision/rationale; flag it in the file's
`_note` as synthetic if it is not a real expert verdict.
2. Create the bundled docs folder `src/portfolio_optimiser/data/docs/<id>/` with at least one
text file whose content names the cost-saving measure/terms (so `retrieve_chunks` returns at
least one citable chunk).
3. Run the project: `run_portfolio(["MY-PROJECT"], "local", client_factory=...)` (or include it
in the default fan-out by passing no `project_ids`).
## Legg til egen datakilde
The retriever (`retrieval.py` / `datasource.py`) reads a **local docs folder** per project,
selected by the project's `docs_dir` in `reference_projects.json`. To point a project at your own
data, change its `docs_dir` to your folder and drop your cost documentation there — the citation
seam (`{file, locator, snippet, score}`) is identical on the in-process tool and MCP paths. The
folder is boundary-checked (fail-closed) against path traversal, so keep documents inside the
configured `docs_dir`. A real deployer swaps the bundled synthetic docs for their own source.
## Legg til egen modell-map
Model choice is **config, not code** (B12): `src/portfolio_optimiser/data/model_map.json` maps
`profile -> role -> model/deployment` (`resolve_model(profile, role)`). To use your own models:
- Edit the `local` block to your local model ids (Ollama/LM Studio), and/or
- Edit the `azure` block to your Foundry deployment names (the placeholders
`REPLACE-WITH-FOUNDRY-DEPLOYMENT` are tenant-specific — replace them or supply via env).
The role keys (`proposer`, `checker`, `default`) let you assign a distinct model per debate role;
`default` is the fallback when a role is unmapped.
## Legg til en ingest-kilde (http-familien som worked example)
The **ingest layer** (`ingest.py`, spec `shared/ingest-spec.md`) is a second, distinct source
seam from the retriever above: one JSON **manifest** per source coupling declares a `source.type`
and a list of extractions; `materialize(...)` runs the connector for that type and writes an OKF
bundle.
**An ingested bundle is not yet a runnable bundle — the chain does not close by itself.** Walked
from a fresh clone (2026-08-05): materializing `examples/ingest-golden-file/manifest.json` writes
exactly `index.md` plus one concept file per extraction, and pointing the run at it is refused:
```
run refused: IR projection not found in bundle: 'validator-input.json'
```
That is a clean fail-fast, not a crash — but nothing before this paragraph told you it was coming.
The run path additionally needs the bundle's **IR projection**, `validator-input.json` (the
candidate the deterministic validator judges — `shared/method-spec.md` §7.1), which the ingest
layer does **not** produce: ingest materializes *source documents*, while the IR projection states
the *candidate measure*, and no connector can infer one from the other. A bundle may also ship an
optional `cost-baseline.json`; without it the validator still runs, but unanchored to the
project's real cost lines. Both are hand-authored today. For the IR projection the shape reference
is `shared/examples/bygg-energi-mikro/validator-input.json`; **no bundled example ships a
`cost-baseline.json`** (checked), so its shape — `{code: {quantity, unit_cost}}` — comes from
`ir.CostBaseline` and the README. Writing them from ingested content is
unbuilt, and is not on the 90 %-principle side of the line: what candidate to propose is the
agents' job, not the connector's.
> **Where to change it (2026-07-20):** `ingest.py` is a thin adapter — Door A is implemented by
> the shared [`llm-ingestion-okf`](https://git.fromaitochitta.com/open/llm-ingestion-okf) library
> (git-pinned to `v0.3.2`), so connectors, materialization and index generation improve in ONE
> place across every consumer. `shared/ingest-spec.md` remains the normative spec — the library
> implements it, it does not replace it, and spec changes go via commons. The API below is
> unchanged; only the implementation moved. Note that Door A is **ungated**: it calls no
> security guard before writing to disk, so gating untrusted content is the caller's
> responsibility (see `docs/plan/2026-07-16-llm-ingestion-guard-inclusion.md`). The spec ships three source families — `file`/CSV (I2), `sql` (I4), and **`http`** (I6) —
and `http` is the framework's **worked extension-point example**: it shows exactly how a third,
network-transport family plugs into the same connector / materialization / gate contracts.
The `http` manifest contract (spec §4):
- `source.base_url` — the endpoint root; it **must not embed credentials** (userinfo is rejected
at schema validation).
- `source.credential_ref` — the **name of an environment variable** holding the secret, resolved
at run time (sent as `Authorization: Bearer <secret>`); the secret is never read from the
manifest, never logged, never stamped in the generated frontmatter. `null` → no auth.
- each extraction's `query` is joined onto `base_url` and its response body is rendered
**verbatim inside a fenced code block** (not a markdown table — a raw `|` or `\` survives
un-escaped); `max_rows` caps the response line count, fail-fast (never silent truncation).
**Network is a per-run grant, never a manifest field (spec §8).** `materialize` refuses an `http`
source unless the caller passes `allow_network=True`:
```python
# refused fail-fast — the manifest cannot grant itself network access:
materialize(manifest, bundle_dir, ingested_at="2026-07-04T12:00:00Z")
# opted in explicitly by the operator for this run:
materialize(manifest, bundle_dir, ingested_at="2026-07-04T12:00:00Z", allow_network=True)
```
This is the **local-only default, no silent egress** principle made mechanical: configuration is
data that cannot escalate its own authority; only the runtime `allow_network` grant can. The
transport itself is an **injectable seam**`materialize(..., http_get=<callable>)` swaps the GET
implementation (the default `_urllib_get` is the only socket path). The golden case
(`examples/ingest-golden-http/`) and every test inject a canned `get` over committed fixture
payloads, so the suite runs offline against a **local mock** — no live source, no credentials.
**The default transport is time-bounded (S2.4).** The library's `_urllib_get` calls the stdlib
opener without a timeout, which falls back to the process-wide default socket timeout — `None`
out of the box — so a source that accepts a connection and then never answers would hang a run,
contradicting the invariant that nothing runs unbounded. `materialize` therefore hands the library
that same socket path wrapped in `ingest.timeout_get`, which scopes `socket.setdefaulttimeout`
(`HTTP_TIMEOUT_SECONDS`, 30s) around the delegate call: the bound applies **without** a second
socket path and **without** duplicating the credential header, so the pinned library stays
untouched. An explicitly injected `http_get` is passed through **unwrapped** — a caller-supplied
transport owns its own timeout policy.
**Honest limit:** the default socket timeout is *process-global*. Under `concurrency=k` the runner
is asyncio on a single thread, so the scoping holds. Driving `read_http` from a thread-pool
executor would make it unsafe, and the bound would have to move to a per-call timeout argument —
i.e. to owning a socket path locally.
### D7 sibling hook — MCP as an extension of this family
The spec (§4) documents an **MCP-based connector as an extension of the `http` family**, not a new
client wired into the optimiser run path — that stays a **Non-Goal** here: the in-process
`FunctionTool` seam is the default in the run path, and MCP is demonstrated (via `build_mcp_server`
in `datasource.py`), not wired in.
**The connector (S2.2, 2026-08-02).** `ingest_mcp.py` implements it. There is **no fourth source
family and no schema change**: an MCP source is an ordinary `type: "http"` source whose transport is
declared by the `base_url` scheme, and whose two parts fall straight out of the join the library
already performs (`base_url` + `/` + `query`):
```json
{ "type": "http", "id": "docs", "base_url": "mcp+stdio://PORTFOLIO_DOCS_MCP" }
```
```python
from portfolio_optimiser.ingest import materialize
from portfolio_optimiser.ingest_mcp import mcp_get, stdio_call_tool
materialize(manifest, bundle_dir, ingested_at="2026-08-02T00:00:00Z",
allow_network=True, http_get=mcp_get(stdio_call_tool()))
```
`server_ref` is the **name of an environment variable** holding the server command — mirroring the
`sql` family's `connection_ref`, so the manifest carries a reference and never an executable path —
and the extraction's `query` is the tool name. Parsing is string-based, not `urlsplit`-based:
`urlsplit().hostname` lowercases the host, which would silently break a case-sensitive env lookup.
Two properties are worth stating because they are **inherited, not written**: the §8 network grant
covers MCP for free (an MCP source *is* `http`, so it is refused before any tool call unless
`allow_network=True`), as do the `max_rows` cap, verbatim fenced rendering, and the §7 provenance
stamp. Had MCP become a fourth family, each of those would have been ours to write — and ours to
forget. The transport discriminator **gates rather than labels**: `mcp_get` refuses a URL it does
not own, so an MCP transport can never quietly serve a plain `https://` manifest and leave the
bundle's provenance claiming a transport that was never used.
**Verified against a real MCP server subprocess (2026-08-03).** `stdio_call_tool` was previously
written but never executed end to end; `examples/ingest-golden-mcp/` + `tests/test_ingest_golden_mcp.py`
now run it against a live server process — byte-identical golden extraction, plus the tool-error and
missing-`server_ref` branches. Five detach mutations measured RED (unwrap, `initialize()`, error-code
identity, the `isError` branch, one body byte). A local subprocess costs no model tokens, so the cost
discipline is untouched; the contract tests still inject a canned tool and spawn nothing.
**What running it actually found — the error contract was broken.** `stdio_client` and
`ClientSession` are each an anyio task group, and anyio re-packages anything leaving one in a
`BaseExceptionGroup`. Every error raised inside the session (`mcp_tool_error`,
`mcp_non_text_content`) therefore reached callers as an exception group, never as the `IngestError`
the whole Door A path catches and switches on by `code`. Fixed by unwrapping the group and
re-raising the owned error; anything unowned is re-raised untouched. **No canned-tool test could
have caught this** — they never enter a task group. This is the case for running what you ship.
**A server on this path must expose a null-argument tool.** The URL carries both coordinates and the
tool is called with an empty argument dict, so `datasource.build_mcp_server` **cannot** serve ingest:
its `retrieve_cost_docs(query)` has a required parameter (verified — it returns an error result).
The two are separate seams by design: `build_mcp_server` serves the agents' retrieval path.
**No longer true (Trekk B, 2026-08-05):** MCP used to be unwired in the optimiser run path. It is
now wired, as an **opt-in**`--mcp-config` (or `run_project(mcp_servers=...)`) hands the agents
live tools from concrete external servers *during* the debate, built by `mcp_tools.py` on MAF's
`MCPStdioTool` / `MCPStreamableHTTPTool`. The in-process `FunctionTool` seam remains the default:
with no config, no network call is possible and the tool list is unchanged.
Three properties hold that config to the repo's data rules, and each is measured by a mutation:
the tool **allowlist is mandatory** (an empty one would let the far end decide what the agents may
call); every configured server and permitted tool is **named in the run announcement before the
first call**, so nothing is contacted undeclared — including when no `--mandate` is given; and
`--live-dry-run` still opens **nothing**, because the tools are entered after the dry-run cut.
This is a **different seam from the ingest path above**, deliberately: that one pulls source
documents into a bundle before a run and speaks to null-argument tools; this one hands live tools
to the agents while they work. They share a protocol, not a job.
**The timeout path moved, and it is covered.** This paragraph used to say the deadline was
`asyncio.wait_for` and that no test exercised it; both halves are now out of date. Measuring
against a genuinely hanging server showed `asyncio.wait_for` never produced a `TimeoutError` at
all — it cancels the call from *outside* the structure anyio owns (`stdio_client`, `ClientSession`),
and the two cancellation mechanisms do not compose; the observed outcome was a
`BrokenResourceError` inside an exception group. The deadline is therefore `anyio.fail_after`
nested *inside* both task groups, and only the scope that hit its own deadline
(`CancelScope.cancelled_caught`) earns the `mcp_timeout` code — any other `TimeoutError` is
re-raised untouched, since the builtin is also `socket.timeout` and `asyncio.TimeoutError`.
`tests/test_ingest_golden_mcp.py` pins both the deadline and that discriminator.
**Where the D7 sibling stands (målbilde §11 boundary).** The Claude Agent SDK sibling built the
**file/CSV and SQL** connectors — mirroring I3/I5 — with bit-identical golden extractions. **HTTP
and MCP are implemented on the MAF side only**; the sibling ships no network connector and no
live-source integration. `ingest_mcp.py` is deliberately MAF-free (it imports the open `mcp`
protocol client, never `agent_framework`), so the seam is portable to D7 unchanged. On D7 the
in-process server hook is `create_sdk_mcp_server(name, version="1.0.0", tools=...) ->
McpSdkServerConfig` (package `claude-agent-sdk`) — verified 2026-07-04 against the official Claude
Agent SDK Python docs — but that is a **documented hook a deployer would reach for, not a shipped D7
connector**; no D7 MCP session is planned. Nothing here contacts a live endpoint.
## Bytt ut henteren (Embedder / Retriever, S3.1)
How prior verdicts are ranked is a seam, not a hard-coded sort. `semretrieval.py` declares two
protocols and the store delegates to them:
- **`Retriever`** — `rank(query, candidates, k) -> list[Verdict]`. `VerdictStore.retriever`
defaults to `None`, which means `StructuralRetriever`: the same weighted structural score and
`(-similarity, id)` ordering the store used before the seam existed. Assign your own object
with a `rank` method to replace ranking wholesale.
- **`Embedder`** — `__call__(features) -> np.ndarray`. `HybridRanker(embedder, similarity, weight)`
blends `weight * cosine + (1 - weight) * structural`; both terms live in `[0, 1]`, so `weight`
means what it reads as (`SEMANTIC_WEIGHT_DEFAULT = 0.25`).
**Your vectors must be finite.** `cosine` raises `ValueError` on a NaN or infinite norm rather
than scoring it — a non-finite score compares `False` against everything, which leaves the
ranking in whatever order the input happened to arrive in and defeats the total order
`HybridRanker` otherwise guarantees. A zero vector is fine and scores `0.0`; the asymmetry is
deliberate, because zero is a state the shipped `FakeEmbedder` produces on purpose whereas
non-finite only ever means the embedder is broken. Coercing it to `0.0` would hide that as
"no semantic similarity" and let ranking proceed on a forged signal.
```python
store.retriever = HybridRanker(MyEmbedder(), similarity, weight=0.3)
```
Note that `similarity` is **injected**, not imported by `semretrieval`. That is deliberate:
`verdicts.py` imports `agent_framework`, and injecting the score keeps the retrieval layer free
of it (guarded by `tests/test_semretrieval_loadbearing.py`, which ranks in a subprocess and then
asserts `verdicts` never entered `sys.modules`). Keep that property if you extend the module.
**A real embeddings client is a code-level extension point — it is not built here.** Selection goes
through a CLOSED registry: `--embedder-config` names a type, `build_embedder` dispatches on it, and
an unknown type is refused rather than resolved. A dotted `module:Class` import path is
deliberately *not* supported — that would be arbitrary code execution at config-load time and would
hand a config file the ability to import something that opens a socket, routing around the
no-network guard (which is scoped to `semretrieval.py` and blind to a third module by
construction). Adding a real embedder therefore means adding a registry branch in code, plus any
capability opt-in as a factory kwarg — the same discipline `NotifierConfig` applies to egress. The
shipped
`FakeEmbedder` is a deterministic sha256 projection with no semantics; it exists so the seam is
exercisable offline at zero cost, and the load-bearing proof is that removing the cosine term
flips the ranking, not that the projection is meaningful. A deployer supplying a real client owns
its network egress, cost, and the fact that it embeds proposal **text** — which is why the hybrid
is opt-in (`--semantic-retrieval`) rather than the default. If you wire one in, note that
`semretrieval` currently imports no network module at all, and a guard test asserts exactly that;
a real client belongs behind the `Embedder` protocol in *your* module, not inside this one.
**Optional persistence — an unwired authoring primitive.** `save_vector_store(dir, verdicts,
embedder)` / `load_vector_store(dir)` write and read a byte-deterministic `vectors.npy` +
`vectors.jsonl` pair (sorted by verdict id, atomic replace). A missing store loads as `None`; a
row/line mismatch raises rather than silently mis-ranking. `*.npy` is gitignored.
They have **no caller in `src/`**, and ranking never reads a persisted matrix: `HybridRanker.rank`
re-invokes the embedder for every candidate on every call, so a persisted store would be bypassed
even if one existed. They are offered to extenders, on the same footing as `write_verdict`,
`promote_verdict` and `build_mcp_server` — public primitives the core deliberately does not wire
into `run_project`. Wiring the cache in only becomes worth anything once the ranker is changed to
consult it, which is a redesign rather than a hookup.
*Known limitation:* the empty-store branch hardcodes `EMBED_DIM`, so a third-party `Embedder` of a
different dimension writes a shape-inconsistent empty store. Stated, not fixed.
## Bevisst ikke bygget (90 %-kuttlista)
Per the design philosophy (a ~90 % generic core with clear extension points — we do not chase
the last 10 %), the following are **deliberate cuts**, not roadmap debt. Each is extension
territory for a deployer, with the seam named:
- **B10 — full verdict-conflict taxonomy.** Chosen minimal semantics (documented in
`verdicts.py` + README): the in-memory store is first-write-wins per verdict id; the disk
layers (`write_verdict`, `promote_verdict`) are last-write-wins per file. The full taxonomy
(rejection categories + a rule for conflicting expert verdicts) is deferred until real experts
produce conflicting verdicts.
- **B11 — expert notification.** `run_project(notify=...)` remains a plain-callable seam
(`run.py` auto-wires no default notifier), but the core now ships the declared `Notifier`
contract (`notify.py`, exported from the package top) with three implementations:
`ConsoleNotifier`, `FileNotifier` (byte-deterministic JSONL), and `WebhookNotifier` — plus a
fail-fast `build_notifier(config, *, allow_egress=...)` factory. The webhook is the ONLY egress
point and is fail-closed behind an explicit per-run `allow_egress=True` opt-in (a code kwarg,
never a config field — mirroring the ingest layer's `allow_network`). SSRF guards, HMAC
signing, and auth headers remain deployer-owned extension points on the injectable
`WebhookPost` transport seam. Webhook URLs must start with `http://` or `https://` — a
scheme-less URL is rejected fail-fast at config construction.
- **U12 — checkpointing / crash-survival of a run.** A run either completes or is re-run; the
async verdict inbox (step 7) is the resumable boundary, not intra-run state.
- **U14 — OpenTelemetry / observability.** Provenance stamping is the audit trail the core
ships; OTEL wiring is a deployer concern.
- **Concurrent fan-out.** `run_portfolio` iterates projects sequentially by design (fresh
per-project execution state; one threaded `VerdictStore`); parallel orchestration is left to
the deployer and would need budget-cap coordination.