docs(s31): close the review's honesty gap — narrow semantic claims to the shipped mechanism

This commit is contained in:
Kjell Tore Guttormsen 2026-07-25 13:00:13 +02:00
commit 9e149c6847
5 changed files with 81 additions and 36 deletions

View file

@ -35,19 +35,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
`costsim` seam note was reworded from the stale "fylles av S5.4 verdirapport" to a truthful
forward reference so `costsim`'s own output no longer claims the wiring is done.
- Semantic retrieval seam (S3.1): a new MAF-free `semretrieval.py` adds an `Embedder`/`Retriever`
pair and a `HybridRanker` blending brute-force numpy cosine over embedded proposal features with
the existing structural score, exposed as `--semantic-retrieval` (valid in both run modes).
**Off by default and additive**: with no retriever installed the store delegates to
pair and a `HybridRanker` blending a numpy cosine term over the embedded feature triple (sorted
cost codes, measure type, magnitude bucket) with the existing structural score, exposed as
`--semantic-retrieval`. **What ships is the seam, not better retrieval quality**: the bundled
`FakeEmbedder` is a deterministic sha256 projection with no semantics, so over a structural tie
the order is deterministic but arbitrary. A real embedder is selected from a CLOSED registry via
`--embedder-config` / `build_embedder` — deliberately never an import path, so a config file can
never name arbitrary code to load.
**Off by default and additive**: with no retriever passed the store delegates to
`StructuralRetriever`, which reproduces the pre-seam ranking exactly, so the text-excluded
default and every existing test are unchanged. Turning it on is a deliberate, gated reversal of
that default, since an embedder does see proposal text. Determinism is pinned rather than hoped
for: BLAS threads are fixed before numpy is imported, vectors are C-contiguous float64, and
ranking uses the total order `(-round(score, 9), id)`. Ships an optional, rebuildable
`vectors.npy` + `vectors.jsonl` store (byte-identical regardless of insertion order; fail-fast
on a row/line mismatch; missing loads as `None`). numpy is confined to `semretrieval.py` and
never enters `okf.py`, `retrieval.py` or `shared/`. The real embeddings client remains a
config-only extension point — the shipped `FakeEmbedder` is a deterministic hash projection with
no semantics, so this buys a scaling *seam*, not better retrieval quality.
default and every existing test are unchanged. The embedding excludes `description`, matching
`similarity` ("text is ignored by design") and the verdict-id hash — so a flag-on run reads no
surface text either, and a genuine expert verdict can no longer be outranked by the framework's
own echo of the query. The ranker is passed PER CALL, never assigned to the caller's store, so
the opt-in cannot outlive the run that asked for it. Accepted in both run modes; in
single-project mode it requires `--bundle-dir` and `--verdict-dir` and is refused — never
silently ignored — without them.
Determinism, precisely: ranking order rests on the total order `(-round(score, 9), id)`; the
BLAS thread pins set before numpy is imported (`VECLIB_MAXIMUM_THREADS` for Accelerate,
`OPENBLAS`/`MKL`/`OMP` for other backends) defend the narrower claim that vector artifacts are
byte-identical across environments. Ships an optional, rebuildable `vectors.npy` +
`vectors.jsonl` store (byte-identical regardless of insertion order; fail-fast on a row/line
mismatch; missing loads as `None`) — an authoring primitive with no caller in `src/`, offered to
extenders like `write_verdict` and `promote_verdict`. numpy is confined to `semretrieval.py` and
never enters `okf.py`, `retrieval.py` or `shared/`.
- Azure/Foundry offline preflight config gate (`preflight.py`, S4.1).
- Offline live-dry-run drill (`--live-dry-run`, S4.2): walks the whole path up to the eager
client build and stops before the first model call — zero chat calls.
@ -58,8 +69,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
`allow_egress` opt-in. Not auto-wired into `run.py`.
- `docs/knowledge-base-recipe.md` (S5.3, D-H item 1): the documented team process (technical +
domain expert) for building a knowledge base, with the honest 12 week expectation.
- Test suite: 488 passing tests (4 skips are live-provider-only), every wired seam covered by a
load-bearing test that goes red when the seam is detached.
- Test suite: 512 passing tests (4 skips are live-provider-only). Every wired seam is covered by a
load-bearing test that goes red when the seam is detached — including, since the S3.1 review
remediation, the `--semantic-retrieval` wiring at `main()` level. That claim did not hold for
S3.1 as first shipped: the flag was covered only below the CLI, so hardcoding it off left the
suite green. Each remediated seam now carries a recorded detach point, verified by mutation.
### Notes
- Licensed under the MIT License (see `LICENSE`).

View file

@ -122,14 +122,24 @@ when the seam is detached, so the loop cannot silently degrade into theater.
uv run python -m portfolio_optimiser.run --report --ledger ledger.json
```
`--semantic-retrieval` (S3.1) is an **opt-in** ranking change, valid in both run modes and
**off by default**. Off, prior verdicts are ranked exactly as before: a structural score over
the affected cost-code set, measure type and magnitude bucket, with surface text deliberately
excluded. On, that score is blended with brute-force cosine over embedded features, so a
semantically related prior verdict carrying a *different* code set can also reach the
hypothesis prompt. Since an embedder sees the proposal text, turning the flag on is a
deliberate, gated reversal of the text-excluded default — which is why it is a flag and not a
new default. Nothing about a flag-off run changes, and no savings claim depends on it.
`--semantic-retrieval` (S3.1) is an **opt-in** ranking change, **off by default**. Off, prior
verdicts are ranked exactly as before: a structural score over the affected cost-code set,
measure type and magnitude bucket, with surface text deliberately excluded. On, that score is
blended with a cosine term over the *same* structural triple, which lets a prior verdict on a
*different* cost-code set outrank one that ties structurally.
**What this ships is the seam, not better retrieval.** The bundled `FakeEmbedder` is a
deterministic sha256 projection carrying no semantics, so over a structural tie the resulting
order is deterministic but arbitrary. Retrieval *quality* depends entirely on injecting a real
embedder — `--embedder-config` selects one from a closed registry (never an import path; a
config file can never name arbitrary code to load), and `docs/extending.md` documents the
`Embedder` protocol. The embedding excludes `description`, matching the structural score and the
verdict-id hash, so a flag-on run reads no surface text either.
The flag is accepted in both run modes, but in single-project mode it **requires** `--bundle-dir`
and `--verdict-dir`: without them it cannot take effect, and the run is refused rather than
silently ignoring the flag. Nothing about a flag-off run changes, and no savings claim depends
on it.
The **prior-verdict fold — the learning step — happens only on the `--bundle-dir` path**; a
plain `--docs-dir`-only run is single-shot (no fold). `--decision`/`--rationale` apply to the

View file

@ -137,7 +137,15 @@ Note that `similarity` is **injected**, not imported by `semretrieval`. That is
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 config-only extension point — it is not built here.** The shipped
**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

View file

@ -283,11 +283,14 @@ async def run_project(
(S4.2, comparison protocol §4 pkt 2/3) is the offline drill: it walks the whole path up to the
EAGER client build, writes the run-config artefact (when ``outbox_dir`` is set), and returns a
``DryRunReport`` BEFORE the first model call (``debate.run``) zero chat calls.
``semantic_retrieval`` (S3.1) is the opt-in scaling seam: when true, the store's ranker is
swapped for a ``HybridRanker`` that blends brute-force cosine over embedded features with the
structural score, so a semantically related prior verdict carrying a DIFFERENT cost-code set
can reach the Step-1 fold. Default false keeps the structural, text-excluded ranking exactly
as before."""
``semantic_retrieval`` (S3.1) is the opt-in scaling SEAM the deliverable is the extension
point, not better retrieval. When true, a ``HybridRanker`` blends a cosine term over the
embedded feature triple (sorted cost codes, measure type, magnitude bucket) with the structural
score, which lets a prior verdict on a DIFFERENT cost-code set outrank one that ties
structurally. The shipped ``FakeEmbedder`` is a deterministic sha256 projection carrying NO
semantics, so over a structural tie the resulting order is deterministic but arbitrary;
retrieval *quality* arrives only with an embedder injected via ``embedder=`` or
``--embedder-config``. Default false keeps the structural ranking exactly as before."""
# 0. Fail-fast: an outbox write is byte-deterministic and keyed on run_id — no wall-clock default.
if outbox_dir is not None and run_id is None:
raise ValueError(
@ -390,8 +393,14 @@ async def run_project(
# hypothesis context BEFORE generation, keyed on the OKF bundle's candidate features (available
# pre-hypothesis). THIS is the one missing dataflow — previously ExpeL was computed
# post-generation into a discarded SessionContext (step 7 below), so a prior verdict could not
# reach the next hypothesis. Bundle-driven path with a populated store only; the road path is
# untouched (its post-hoc, proposal-keyed retrieval below is unchanged).
# reach the next hypothesis. Bundle-driven path with a populated store only.
#
# Scope of the --semantic-retrieval opt-in, stated precisely (an earlier version of this
# comment claimed "the road path is untouched", which the flag made false): the ranker built
# below is passed to ALL THREE retrievals this run performs — this fold, and the post-hoc
# ExpeLContextProvider + store.retrieve in step 7 — so the flag reaches the road path's
# proposal-keyed retrieval too. What IS untouched on the road path is the fold itself: it stays
# bundle-gated, so a --docs-dir-only run remains single-shot either way.
# S3.1 opt-in: build the hybrid ranker as a LOCAL, then pass it explicitly at each retrieval
# this run performs. It is deliberately not assigned to ``store.retriever``: the store is
# caller-owned (``run_portfolio`` threads one store across every project, and a library caller
@ -724,10 +733,13 @@ def main(argv: list[str] | None = None) -> int:
parser.add_argument(
"--semantic-retrieval",
action="store_true",
help="S3.1 opt-in scaling seam: rank prior verdicts with a hybrid of brute-force cosine "
"over embedded features and the structural score, so a semantically related verdict with a "
"DIFFERENT cost-code set can reach the hypothesis prompt. Valid in both modes; OFF by "
"default, and off means the structural, text-excluded ranking is unchanged",
help="S3.1 opt-in scaling SEAM: blend a cosine term over the embedded feature triple with "
"the structural score, so a prior verdict on a DIFFERENT cost-code set can outrank one that "
"ties structurally. The shipped embedder is a semantics-free sha256 projection — this buys "
"the extension point, not better retrieval; inject a real one with --embedder-config. "
"Accepted in both modes, but in single-project mode it REQUIRES --bundle-dir and "
"--verdict-dir (without them it cannot take effect, and is refused rather than ignored). "
"OFF by default, and off means the structural ranking is unchanged",
)
parser.add_argument("--decision", default="approved", choices=["approved", "rejected"])
parser.add_argument("--rationale", default="reviewed by expert")

View file

@ -92,8 +92,9 @@ _MAGNITUDE_BUCKETS = [(0.0, 1e5), (1e5, 5e5), (5e5, 1e6), (1e6, float("inf"))]
class Embedder(Protocol):
"""Maps structured proposal features to a fixed-length float64 vector.
The real implementation (an embeddings client) is a config-only extension point and is NOT
built here tests and the offline path use ``FakeEmbedder``."""
The real implementation (an embeddings client) is a CODE-LEVEL extension point and is NOT
built here tests and the offline path use ``FakeEmbedder``. Config selects from the closed
``build_embedder`` registry; it can never name a module to import (see ``EmbedderConfig``)."""
def __call__(self, features: ProposalFeatures) -> np.ndarray: ...