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

22 KiB

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 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:

# 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 seammaterialize(..., 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):

{ "type": "http", "id": "docs", "base_url": "mcp+stdio://PORTFOLIO_DOCS_MCP" }
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:

  • Retrieverrank(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.

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.