Skip to content

fix(docs): correct every row of the "What each one runs" preset table, and guard it - #142

Merged
OsherElhadad merged 2 commits into
mainfrom
fix/preset-doc-drift
Sep 1, 2026
Merged

fix(docs): correct every row of the "What each one runs" preset table, and guard it#142
OsherElhadad merged 2 commits into
mainfrom
fix/preset-doc-drift

Conversation

@amiddavid

@amiddavid amiddavid commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

The defect

Every row of the "What each one runs" table in docs/how-to/choose-a-preset.md was stale relative to the presets map in config/config.go:

  • codesmart, coding and general were documented as running toon — which was retired from them after it acted 0 times on 5,752 production requests and converted 0 candidates in 11.67M measured tokens.
  • Every row omitted components that do run: the lossless pair textclean and searchfold, and linecap.
  • Three presets in the map were never in the table at all: agentdiet, house, housellm.

Severity

docs/reference/presets.md was correct at the same moment, so the two documents contradicted each other — in the exact table a reader uses to decide what a preset will do to their context. That is worse than an out-of-date sentence: the answer was wrong in the direction that matters, naming a component that does not run and omitting three that do. A reader had no way to tell which document was lying.

The fix

  1. The table is regenerated from the presets map. Existing row order is preserved; the three never-listed presets are appended. Nothing else in the document is restructured.

  2. config/docdrift_test.go asserts set equality in both directions between the presets map and both doc tables (docs/how-to/choose-a-preset.md and docs/reference/presets.md): every preset has a row with the pipeline it runs, and nothing is documented that does not exist. It reads either formatting — one backtick pair around a comma list, or per-name backticks joined by .

    Revision after review: the first version iterated the rows it happened to find and stood in for completeness with a hand-tuned multi < 5 floor. That let a shipping preset with no row pass (seven of twelve rows deletable, codesmart among them), let a documented-but-nonexistent preset pass — which is a startup failure, config: config: unknown preset "cache", the skeleton/coding class — and silently skipped any row name with a digit or hyphen. Inverting the loop deleted that block and its magic number; docRow widened to `([a-z0-9_-]+)`.

Revert-verification

Every mutation was asserted to have landed in the source (anchor found exactly once, post-write re-read confirming it) before the test result was believed; an apply that fails its own check aborts the run rather than reporting a vacuous "ok".

(a) a preset that ships with no rowagentdiet's row deleted

--- FAIL: TestPresetDocsDoNotDrift (0.00s)
    docdrift_test.go:116: ../docs/how-to/choose-a-preset.md: preset "agentdiet" ships [format agentdiet cachesplit] but has no row in this table. An undocumented preset is the same defect as a misdocumented one: a reader cannot tell it exists, or what it would do to their context.
FAIL	github.com/rossoctl/context-guru/config	0.229s

(b) a preset documented that does not exist — a cache row added (the exact state a merge of #141 onto an unguarded table would produce)

--- FAIL: TestPresetDocsDoNotDrift (0.00s)
    docdrift_test.go:145: ../docs/how-to/choose-a-preset.md: preset "cache" is documented but does not exist in the presets map; `--preset cache` exits at startup with `unknown preset "cache"`.
FAIL	github.com/rossoctl/context-guru/config	0.169s

(c) the hyphen hole, tripping both directions at oncecodesmart renamed to code-smart

--- FAIL: TestPresetDocsDoNotDrift (0.00s)
    docdrift_test.go:116: ../docs/how-to/choose-a-preset.md: preset "codesmart" ships [format textclean searchfold dedup failed_run cmdfilter extract_llm extract linecap cachesplit] but has no row in this table. An undocumented preset is the same defect as a misdocumented one: a reader cannot tell it exists, or what it would do to their context.
    docdrift_test.go:145: ../docs/how-to/choose-a-preset.md: preset "code-smart" is documented but does not exist in the presets map; `--preset code-smart` exits at startup with `unknown preset "code-smart"`.
FAIL	github.com/rossoctl/context-guru/config	0.165s

(d) a wrong component listsafe cut to formatcachesplit in docs/reference/presets.md

--- FAIL: TestPresetDocsDoNotDrift (0.00s)
    docdrift_test.go:128: ../docs/reference/presets.md: preset "safe" is documented as running
          [format cachesplit]
        but ships
          [format textclean searchfold cachesplit]
        A reader uses this table to decide what a preset will do to their context; naming a component that does not run, or omitting one that does, is the failure this guard exists for.
FAIL	github.com/rossoctl/context-guru/config	0.172s

Restoring each mutation returns go test ./config/ -run Drift to ok. Full go test ./config/ passes (7.1s); gofmt -l . and go vet ./config/ are clean (eval box, Go 1.26.4). The earlier row-level mutations from the first round (stale codesmart row, dropped components) still fail as before.

Merge order with #141 (please merge this PR first)

Both PRs edit the same hunk of docs/how-to/choose-a-preset.md and git merge-tree confirms a content conflict. #141 added a | `cache` | `cachesplit` | row onto the stale table; this PR rewrites every row without knowing cache exists, so both mechanical resolutions lose something — taking this table drops the cache row, taking #141's restores all nine stale rows including toon.

Correct resolution: this PR's corrected rows plus #141's cache row placed after codesafe, in both guarded files.

#142 should merge first. With the inverted guard in place, forgetting the cache row becomes a CI failure — mutation (a) above is literally that scenario — instead of a silent regression. Merged the other way round, the omission is invisible. Note the guard also runs in the other direction, so #141 must carry a cache row in both docs/how-to/choose-a-preset.md and docs/reference/presets.md, and this PR deliberately does not add one: cache does not exist on main and a row for it would fail this PR's own "documented but does not exist" check.

Non-blocking review items, all addressed

  • Test renamed to TestPresetDocsDoNotDrift, so go test ./config/ -run Drift matches it (verified).
  • The "lossless repack first … cache directives last" sentence now carries the house/housellm exemption, pointing at config/config.go and the preset reference.
  • house/housellm are marked as the service configs and agentdiet as a published baseline rather than a recommendation, so nobody reads this how-to and sets --preset house.
  • "the presets map above" corrected to name config/config.go.

Why this is its own PR

Found while adding a row to this table during the local-distribution work. It is split out because a stale preset-pipeline table has nothing to do with distribution and should not wait on it.

Heads-up for whoever merges second: the sibling PR feat/local-distribution also touches this table (it adds one row for a new cache preset), so it will need a trivial rebase there — one row, no semantic conflict.

Not fixed here (out of scope, reported)

Other documents also carry stale pipeline listings in prose/snippets, outside the two preset tables this guard covers:

  • README.md:133-134codesmart and codesafe described with the pre-August pipelines (toon present, lossless pair and linecap absent).
  • docs/get-started/connect-ibm-service.md:22 — "Default pipeline" given as [format, toon, dedup, failed_run, cmdfilter, extract, cachesplit], which is not the house pipeline the map ships.

(docs/results/** intentionally records the pipelines as measured at the time and is correctly "stale".)

🤖 Generated with Claude Code

…, and guard it

Every row of the "What each one runs" table in docs/how-to/choose-a-preset.md
was stale relative to the `presets` map in config/config.go:

  - `codesmart`, `coding` and `general` were documented as running `toon`,
    which was RETIRED from them — it acted 0 times on 5,752 production
    requests and converted 0 candidates in 11.67M measured tokens.
  - every row omitted components that do run: the lossless pair `textclean`
    and `searchfold`, and `linecap`.
  - the map has presets the table never listed at all (`agentdiet`, `house`,
    `housellm`).

docs/reference/presets.md was correct at the same moment, so the two documents
contradicted each other — in the table a reader uses to decide what a preset
will do to their context. That is worse than an out-of-date sentence: the
answer was wrong in the direction that matters, naming a component that does
not run and omitting three that do.

The table is now regenerated from the map (existing row order kept, the
never-listed presets appended), and config/docdrift_test.go parses BOTH doc
tables and diffs each row against `presets`. It reads either formatting — one
backtick pair around a comma list (choose-a-preset.md) or per-name backticks
joined by arrows (presets.md) — and asserts coverage, because an earlier
version of this check looked for individually-backticked names only, matched
nothing in choose-a-preset.md, and so silently checked just the
single-component presets.

Revert-verified (each mutation asserted to have landed in the source before
the test was believed).

(a) codesmart's row in docs/how-to/choose-a-preset.md put back to its stale form:

    --- FAIL: TestDocumentedPresetPipelinesMatchTheShippedOnes (0.00s)
        docdrift_test.go:98: ../docs/how-to/choose-a-preset.md: preset "codesmart" is documented as running
              [format toon dedup failed_run cmdfilter extract_llm extract cachesplit]
            but ships
              [format textclean searchfold dedup failed_run cmdfilter extract_llm extract linecap cachesplit]
            A reader uses this table to decide what a preset will do to their context; naming a component that does not run, or omitting one that does, is the failure this guard exists for.

(b) `safe` reduced to `format` -> `cachesplit` in docs/reference/presets.md:

    --- FAIL: TestDocumentedPresetPipelinesMatchTheShippedOnes (0.00s)
        docdrift_test.go:98: ../docs/reference/presets.md: preset "safe" is documented as running
              [format cachesplit]
            but ships
              [format textclean searchfold cachesplit]

(c) the coverage assertion itself, checked non-vacuously by deleting 10 rows
    from the table:

    --- FAIL: TestDocumentedPresetPipelinesMatchTheShippedOnes (0.00s)
        docdrift_test.go:114: ../docs/how-to/choose-a-preset.md: this guard only checked 3 preset rows (2 of them multi-component). The table's shape must have changed and the check has silently stopped covering it — which is how the stale rows survived in the first place.

Restoring each mutation returns `go test ./config/` to ok. gofmt and
`go vet ./config/` are clean.

Found while adding a row to this table during unrelated local-distribution
work; split out because the defect has nothing to do with distribution.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@OsherElhadad

Copy link
Copy Markdown
Collaborator

Reviewed against main e88f5d8, in a clean worktree. Build, gofmt, go vet and the full
go test ./config/... all pass; mkdocs build --strict builds clean and the edited page is in
the nav at mkdocs.yml:115 with all seven outbound links resolving.

The correction itself is right: all 14 rows now match config/config.go, and three presets the
table never listed (agentdiet, house, housellm) are added. Confirmed the drift this fixes
was real — running the new guard against main's table fails 9 of 10 rows (every one except
summarize).

The guard's within-row coverage is genuinely solid. Adding, reordering and removing a
component in config.go each fail the test, in both documented files:

add     "mcp": +linecap             -> FAIL  documented [format textclean smartcrush cachesplit]
                                             but ships [format textclean smartcrush linecap cachesplit]
reorder "codesmart": swap textclean/searchfold -> FAIL
remove  "balanced": -linecap        -> FAIL

Two things to fix before this merges.

1. Blocking — the guard checks rows, not the preset set, so the drift class it was written for still passes

The loop iterates doc rows and asks "is this row correct?". It never iterates presets and
asks "does every preset have a row?". Standing in for that is a hand-tuned coverage floor at
config/docdrift_test.go:113:

len(seen) == 0 || multi < 5

Magic number 5, against an actual population of 12 multi-component presets. Three holes,
each verified by mutating a scratch copy and running the real test:

Mutation Guard
A preset documented in the table that does not exist in presets passes
A preset that ships but has no row passes
A row name containing a hyphen or digit (`code-smart`, `preset2`) silently skippeddocRow is ^|\s*`([a-z_]+)`\s*|

Hole B tolerates deleting 7 of the 12 multi-component rows before the floor trips on the 8th
— including codesmart itself, because the surviving rows satisfy multi >= 5.

This matters because it is the class the PR itself just fixed by hand. The commit message says
the map had "presets the table never listed at all (agentdiet, house, housellm)" — that
exact defect can recur tomorrow with CI green. It is also already live: #141 adds a cache
preset, and on a merge of both, the pipeline table has no cache row and the guard passes.

Hole A is not cosmetic. A documented preset that does not exist is a startup failure, not a
graceful degradation — confirmed directly:

$ PRESET=cache ./context-guru-proxy
config: config: unknown preset "cache"      # process exits

Same class as the skeleton/coding incident this file's own comments describe.

Fix — invert the loop. This deletes code: the seen/multi/multi < 5 block and its
magic number go away, and the guard then scales with the map instead of needing the floor
re-tuned.

documented := map[string][]string{}
for _, m := range docRow.FindAllStringSubmatch(string(b), -1) {
    name, row := m[1], m[2]
    cell := row
    if i := strings.Index(row, "|"); i >= 0 {
        cell = row[:i]
    }
    documented[name] = pipelineFromCell(cell)
}
for name, want := range presets {
    got, ok := documented[name]
    if !ok {
        t.Errorf("%s: preset %q ships %v but has no row in this table. "+
            "An undocumented preset is the same defect as a misdocumented one.", path, name, want)
        continue
    }
    if len(want) == 0 && len(got) == 0 {
        continue // `off`, which also subsumes the current strings.Contains(cell, "empty") sentinel
    }
    if strings.Join(got, ",") != strings.Join(want, ",") {
        t.Errorf(...) // message unchanged
    }
}
for name := range documented {
    if _, ok := presets[name]; !ok {
        t.Errorf("%s: preset %q is documented but does not exist; `--preset %s` exits at startup", path, name, name)
    }
}

Also widen docRow to `([a-z0-9_-]+)` so hole C closes.

2. Blocking — merge-order hazard with #141

Both PRs edit the same hunk of docs/how-to/choose-a-preset.md:

$ git merge-tree --write-tree --messages refs/remotes/pr141 HEAD
CONFLICT (content): Merge conflict in docs/how-to/choose-a-preset.md

#141 added a | `cache` | `cachesplit` | row onto the stale table; this PR rewrites all
rows without knowing cache exists. Both mechanical resolutions lose something — taking this
PR's table drops the cache row; taking #141's restores all 9 stale rows including toon.

Correct resolution: this PR's corrected rows plus #141's cache row after codesafe.
Landing fix 1 first makes forgetting it a CI failure instead of a silent regression, which is the
argument for merging #142 before #141.

Non-blocking

  • docs/how-to/choose-a-preset.md:41-45 — the new house/housellm rows contradict the
    unchanged sentence beneath them. It says "lossless repack first … cache directives last", but
    those pipelines run two offloaders before the lossless pair and put toolfilter after
    cachesplit. reference/presets.md:10 carries the exemption note ("the order is the
    operator's") and config/config.go:414-431 records both deviations at length so nobody
    "fixes" them — but that context did not travel with the rows. One clause on :44-45.
  • :41-42house/housellm are now listed as peers of codesmart in a user-facing
    how-to, with no service-only marker; the chooser (:12-23) and notes (:47+) mention neither
    them nor agentdiet. Someone sets --preset house and gets toon and no linecap.
  • Test namego test ./config/ -run Drift matches nothing (no tests to run). The file
    is docdrift_test.go but the test is TestDocumentedPresetPipelinesMatchTheShippedOnes.
    TestPresetDocsDoNotDrift would make the obvious selector work.
  • :16 — "the presets map above" — it is in config.go, not this file.

Out of scope

The preset facts stated outside the two guarded files are stale, and the guard's shape cannot
reach them (README's prose lists, the wrong default preset in five places). Filed as #145 rather
than loaded onto this PR.

Verdict

Needs changes — both blocking items are small, and fix 1 is a net deletion. Nothing here
touches an invariant or risks the running proxy. This is the best documentation work in the
current batch; the guard just needs to assert set equality rather than trust the rows it happens
to find.

…rectness

Review of #142 found the guard checked the rows it happened to find, never the
preset SET. It iterated doc rows asking "is this row right?" and stood in for
completeness with a hand-tuned floor, `multi < 5`, against a population of 12
multi-component presets. Three holes, all in the class this PR had just fixed
by hand:

  - a preset that SHIPS with no row passed (seven of twelve rows could be
    deleted, `codesmart` among them, before the floor tripped);
  - a preset DOCUMENTED that does not exist passed — and that is a startup
    failure, not a graceful degradation:
      $ PRESET=cache ./context-guru-proxy
      config: config: unknown preset "cache"      # process exits
    the same class as the skeleton/coding incident this file's comments
    describe;
  - a row name with a digit or hyphen was silently skipped by docRow.

The loop is inverted: it now walks `presets` asserting every preset has a row
with the pipeline it runs, then walks the rows asserting nothing is documented
that does not exist. That deletes the seen/multi/`multi < 5` block and its magic
number — the check scales with the map instead of needing the floor re-tuned —
and docRow widens to `([a-z0-9_-]+)` so an odd name is REPORTED rather than
skipped. Both loops iterate sorted keys so failures are stable.

Also: renamed to TestPresetDocsDoNotDrift so `go test -run Drift` matches it;
noted in choose-a-preset.md that `house`/`housellm` deviate from the
lossless-first ordering deliberately (config.go records why, reference/presets.md
carries the exemption) and that `house`/`housellm`/`agentdiet` are the service
configs and a published baseline, not peer recommendations, so nobody reads a
how-to and sets `--preset house`; corrected "the `presets` map above" to name
config/config.go.

Revert-verified again, each mutation asserted to have landed in the source
before its result was believed (an apply that fails its own anchor check aborts
the run rather than reporting a vacuous ok).

(a) a preset that ships with no row — `agentdiet`'s row deleted:

    --- FAIL: TestPresetDocsDoNotDrift (0.00s)
        docdrift_test.go:116: ../docs/how-to/choose-a-preset.md: preset "agentdiet" ships [format agentdiet cachesplit] but has no row in this table. An undocumented preset is the same defect as a misdocumented one: a reader cannot tell it exists, or what it would do to their context.

(b) a preset documented that does not exist — a `cache` row added:

    --- FAIL: TestPresetDocsDoNotDrift (0.00s)
        docdrift_test.go:145: ../docs/how-to/choose-a-preset.md: preset "cache" is documented but does not exist in the presets map; `--preset cache` exits at startup with `unknown preset "cache"`.

(c) the hyphen hole, which trips both directions at once — `codesmart` renamed
    to `code-smart`:

    --- FAIL: TestPresetDocsDoNotDrift (0.00s)
        docdrift_test.go:116: ../docs/how-to/choose-a-preset.md: preset "codesmart" ships [format textclean searchfold dedup failed_run cmdfilter extract_llm extract linecap cachesplit] but has no row in this table. ...
        docdrift_test.go:145: ../docs/how-to/choose-a-preset.md: preset "code-smart" is documented but does not exist in the presets map; `--preset code-smart` exits at startup with `unknown preset "code-smart"`.

(d) a wrong component list — `safe` cut to format -> cachesplit in
    docs/reference/presets.md:

    --- FAIL: TestPresetDocsDoNotDrift (0.00s)
        docdrift_test.go:128: ../docs/reference/presets.md: preset "safe" is documented as running
              [format cachesplit]
            but ships
              [format textclean searchfold cachesplit]
            A reader uses this table to decide what a preset will do to their context; naming a component that does not run, or omitting one that does, is the failure this guard exists for.

Restoring each mutation returns `go test ./config/ -run Drift` to ok; the full
`go test ./config/` passes, gofmt and `go vet ./config/` are clean.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@OsherElhadad
OsherElhadad merged commit 3697f3c into main Sep 1, 2026
5 checks passed
@github-project-automation github-project-automation Bot moved this from New/ToDo to Done in Rossoctl Issue Prioritization Sep 1, 2026
amiddavid added a commit that referenced this pull request Sep 1, 2026
None of this is distribution work. Every item is a defect in code or docs that already shipped,
found while doing #141, and split out at review request so it can be judged on its own — and so it
can land whether or not the funnel does.

## 1. The expand tool was advertised where no marker can exist

`expand.Inject` under `auto` gated on "the request declares tools" and "the store persists".
Nothing asked whether the pipeline could produce a `<<cg:HASH>>` marker at all, so an
offloader-free pipeline declared `context_guru_expand` to the provider — and every call against it
must fail, because there is nothing in the Store to resolve. Measured on the real gateway route:

  tools SENT by client    : [Read Bash]
  tools FORWARDED upstream: [Read Bash context_guru_expand]

Affected `safe` and any cachesplit-only configuration, and — the one that matters most — **`off`,
the A/B control arm**. A control that carries an extra tool declaration is not a control, and every
measurement taken against it was comparing two arms that differed by more than the pipeline.

The cost when it fires is a wasted round trip and a step of the user's turn: on a transcript
containing marker-shaped text (this repo's own docs contain literal `<<cg:HASH>>`), a model calls
the tool and gets "[expand: original for id ... is no longer available]".

It was also a code-vs-comment contradiction, which is why nobody noticed: `Options.InjectExpand`
documented the gate as requiring "an expandable marker", while `expand/inject.go` says "No marker
condition, deliberately" three lines from the code. Both now describe what happens.

`components.Pipeline.HasOffload()` answers by TYPE ASSERTION, not a list of component names: a
name list is a second copy of "which components are lossy" and drifts the moment somebody adds
one. `components.Offload` cannot be implemented by accident — it requires returning cache keys
proving the original was stashed. Marker independence is preserved (the property that keeps the
tools array byte-stable across a session, and hence the prefix cached): a pipeline does not change
turn to turn.

**Ten existing tests changed fixture.** Every test of the expand loop hand-seeds the Store to
simulate an offload, but built its handler with `pipeline: []` — which cannot offload anything.
Harmless while injection ignored the pipeline; now they use `offloadCapablePipeline` (`[linecap]`,
which does not act on their short bodies). No assertion was weakened; each fixture now matches its
own premise.

## 2. `POST /v1/messages/count_tokens` was not served

Absent it, a client asking how big its context is gets a 404 and falls back to working it out with
**inference requests** — billed calls, caused by a proxy whose purpose is to reduce them. Cheap to
add, and it costs every routed user, not only the funnel.

Forwarded verbatim, with no pipeline. Returning the compacted count would be smaller and would be
wrong in the dangerous direction: the client budgets its own transcript from this number, and
because every component fails open, the next request could forward the full body and take a 400.
Over-reporting is recoverable; under-reporting is a failed turn. The cost of that choice is now
documented in `docs/reference/routes.md`, where the route was absent entirely — a routed session
self-compacts earlier than it needs to (115,933 reported vs 32,802 forwarded on a measured body).

The hosted branch has tests, because that branch is the only thing standing between the
multi-tenant service and an unmetered open forwarder that would send OUR credential upstream.

## 3. Our own docs said the binary needs a C toolchain

`docs/setup.md`, `docs/hosted.md` and `docs/get-started/quickstart-proxy.md` all told evaluators to
install one. It is needed for `go test -race` and for the optional `cg_skeleton` tag, not for the
binary. setup.md went further and named **bifrost's tokenizer** as a cgo dependency, which it never
was — o200k_base is embedded (`internal/tokens/tokens.go`).

Asserted rather than re-claimed: a new `purego` CI job builds with `CGO_ENABLED=0` and
`CC=/nonexistent-c-compiler`, checks the artifact is statically linked, starts it and probes
/healthz. It also runs the packages whose behaviour depends on which components compile in —
because `build-test` runs exclusively with `CGO_ENABLED=1` (the race detector needs it), so
`TestEveryPresetBuilds` had **never executed in the configuration a user would build**. That guard
exists for exactly the `preset: coding` / `unknown component "skeleton"` breakage.

## 4. Preset facts stated outside the guarded files (#143, #145)

- The binary defaults to **`house`**; five sites said `codesmart` (README x3,
  `docs/reference/config.md`, `docs/get-started/quickstart-proxy.md` — the last is step 2 of the
  first page anyone runs). Anyone running the binary bare while reading those measured a different
  configuration than the published SWE-bench numbers describe.
- README's `codesmart`/`codesafe` pipeline lists and
  `docs/get-started/connect-ibm-service.md`'s "Default pipeline" were stale — naming `toon`,
  retired after acting 0 of 5,752 production requests, and omitting components that do run.
  The IBM page's omission of `toolfilter` matters most: that page is what a prospective hosted
  tenant reads to decide what the service does to their traffic.

All regenerated from the `presets` map. The two tables inside #142's drift guard are untouched
here; these are the sites that guard cannot reach.

## Verification

Five mutations, each proven to have landed in the source before its result was allowed to count:

  expand injection ungated        -> TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist FAIL
    on cachesplit-only, `safe`, and `off`
  HasOffload always false         -> same test FAIL on `mcp` and the offloader pipeline: "mints
    markers but no longer advertises the expand tool, so a model cannot recover what it offloaded"
  count_tokens route unregistered -> TestCountTokensIsServed FAIL (404)
  count_tokens rewrites the body  -> TestCountTokensIsServed FAIL
  hosted auth removed             -> TestCountTokensHostedRequiresAuth FAIL (502, want 401)

The second is the mirror-image check: it proves the gate did not trade one silent defect for
another, an offloader whose output nothing can expand.

Two things I got wrong on the way, recorded because both were caught by tests rather than by me:

- I first asserted `mcp` had no offloader. `smartcrush` implements `components.Offload`
  (`components/offload/smartcrush.go`), so that pipeline genuinely mints markers and genuinely
  needs the tool. The case now asserts the opposite, with the reason — and it is the argument for
  asking the interface rather than keeping a hand-written list.
- Copying `proxy/proxy.go` wholesale from the older distribution branch onto current main silently
  reverted #155's `effPreset`/`notePreset` work. `TestCompactRowNamesThePresetThatRan` — a test I
  had never read — failed with "the dashboard names a pipeline that did not run". The file was
  restored from main and the two edits re-applied on top; #155's change is intact.

`go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean.

One unrelated flake seen once and not reproduced: `TestConcurrentCallsDoNotRaceOnTheGateHistogram`
failed in a full-suite run with "no single-flight follower ran ... the race was never exercised",
then passed 8/8 in isolation and in two further full suites, and passes on clean main. Reported
separately rather than papered over.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
amiddavid added a commit that referenced this pull request Sep 1, 2026
Review of #161 found the same defect this PR is themed on — claims that are not true — in seven more
places, one of them in shipped code. All seven fixed, plus the `HasOffload` unit test the reviewer
raised without asking for.

Rebased onto current main first, so #142's preset-table guard and the preset pass below cannot
re-fix or re-break each other.

## Merge-blocking

**1. `proxy/proxy.go` named `mcp` as offloader-free.** It is not: `smartcrush` implements
components.Offload, which this PR's own test asserts (`wantAdd: true`) and its own mirror-image
mutation proves. I had corrected the test and left the comment wrong — the copy a future reader
actually trusts.

The fix deletes the list rather than correcting it. The comment now names the shapes affected
(`off`, `safe`, any cachesplit-only configuration) and then says why enumerating presets here is the
wrong move: a list in a comment is a second source of truth, and this one was wrong about `mcp` on
its first draft. That is the whole argument for gating on the interface.

**2. Five sites named the `cache` preset, which does not exist on this base.** It is #141's, and it
reached here in the wholesale copy of `proxy/proxy.go` this PR already admits to, then travelled
into the test files when they were split out of that branch. Reworded to name configurations that
exist here; the underlying defect they describe is unchanged and still reproduces on `off` and
`safe`.

**3. `proxy/counttokens_test.go` carried copy-paste artifacts vet and gofmt cannot see.** A
duplicated 3-line doc comment, and a 20-line orphan documenting a function that lives in
`expandgate_test.go` under a different name and citing a test that exists nowhere. Both from the
same cause: my splitter took each test's doc comment by scanning back to the previous blank line,
which swallowed the FOLLOWING test's comment as a trailing block. A third artifact the review did not
list is fixed too — `expandgate_test.go`'s doc comment still described "the preset's promise" and
cited `docs/how-to/install-plugin.md` and an install skill, both of which belong to #160.

## The rest

**4.** `docs/reference/config.md` and `docs/components.md` said `auto` injection has exactly two
conditions. It has three. Both now say so, and say what the third is for. The cache-stability
argument those passages make is unaffected — a pipeline does not change turn to turn either — so it
gained a member rather than needing a rewrite.

**5. `make build` now sets `CGO_ENABLED=0`.** The docs could claim "no C toolchain" all they liked
while step 1 of the quickstart was `make build`, which needed one because the Makefile exported
`CGO_ENABLED=1` for every target. Pointing readers at `build-static` would have fixed the sentence;
making the DEFAULT build pure Go makes the claim true of the command the docs tell people to run.
`CGO_ENABLED=1` stays for the test targets, where `-race` requires it, and the comment says exactly
that. Verified: `CC=/nonexistent make build` produces a statically linked binary.

README, CLAUDE.md and the quickstart no longer require a C toolchain. All five remaining
`codesmart`-is-the-default sites are corrected — including two in `config/config.go`, which is how
the claim spread to five documents: it sat three lines from the flag that disproves it.

**6.** `docs/setup.md` overstated its own evidence, which is the exact sin this PR is about. It
claimed CI removes the C compiler from `PATH` (with cgo off the toolchain never consults `CC`; that
variable is a tripwire, not the mechanism) and that cross-compilation to four targets is asserted,
when CI builds native linux/amd64 only. Now says what CI actually does, and states separately that
the other three targets were verified by hand and are asserted at release time. Same overstatement
fixed in the `ci.yaml` comment.

**7.** `ci.yaml` promised a linked issue and linked nothing, and named a different flake than the PR
body did. Both are real; the comment is about the campaign one, and now links #163.

## HasOffload unit tests

`./components`: nil-safe, empty pipeline (the A/B control arm), reformatters-only, and an offloader
in three positions. Revert-verified both ways — always-true fails the empty and reformatter cases,
always-false fails the offloader cases.

A registry-walking test was supposed to make it rot-proof, and **it skipped**: registrations happen
in `components/all`, so a test inside `components` can neither see them nor import the package that
does. A test that skips reads as coverage and is not, so it moved to `components/all`, where it
runs — 21 of 21 registered components, 13 implementing Offload. It fails if either count is zero,
because an all-false or all-true population would agree with a broken HasOffload.

Full `go test ./...`, `go vet ./...` and `gofmt -l` clean; doc link/anchor checker re-run over every
document touched.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
OsherElhadad pushed a commit that referenced this pull request Sep 1, 2026
…pand-tool gate, count_tokens, C-toolchain claim, preset facts) (#161)

* fix: defects that surfaced while building the distribution funnel

None of this is distribution work. Every item is a defect in code or docs that already shipped,
found while doing #141, and split out at review request so it can be judged on its own — and so it
can land whether or not the funnel does.

## 1. The expand tool was advertised where no marker can exist

`expand.Inject` under `auto` gated on "the request declares tools" and "the store persists".
Nothing asked whether the pipeline could produce a `<<cg:HASH>>` marker at all, so an
offloader-free pipeline declared `context_guru_expand` to the provider — and every call against it
must fail, because there is nothing in the Store to resolve. Measured on the real gateway route:

  tools SENT by client    : [Read Bash]
  tools FORWARDED upstream: [Read Bash context_guru_expand]

Affected `safe` and any cachesplit-only configuration, and — the one that matters most — **`off`,
the A/B control arm**. A control that carries an extra tool declaration is not a control, and every
measurement taken against it was comparing two arms that differed by more than the pipeline.

The cost when it fires is a wasted round trip and a step of the user's turn: on a transcript
containing marker-shaped text (this repo's own docs contain literal `<<cg:HASH>>`), a model calls
the tool and gets "[expand: original for id ... is no longer available]".

It was also a code-vs-comment contradiction, which is why nobody noticed: `Options.InjectExpand`
documented the gate as requiring "an expandable marker", while `expand/inject.go` says "No marker
condition, deliberately" three lines from the code. Both now describe what happens.

`components.Pipeline.HasOffload()` answers by TYPE ASSERTION, not a list of component names: a
name list is a second copy of "which components are lossy" and drifts the moment somebody adds
one. `components.Offload` cannot be implemented by accident — it requires returning cache keys
proving the original was stashed. Marker independence is preserved (the property that keeps the
tools array byte-stable across a session, and hence the prefix cached): a pipeline does not change
turn to turn.

**Ten existing tests changed fixture.** Every test of the expand loop hand-seeds the Store to
simulate an offload, but built its handler with `pipeline: []` — which cannot offload anything.
Harmless while injection ignored the pipeline; now they use `offloadCapablePipeline` (`[linecap]`,
which does not act on their short bodies). No assertion was weakened; each fixture now matches its
own premise.

## 2. `POST /v1/messages/count_tokens` was not served

Absent it, a client asking how big its context is gets a 404 and falls back to working it out with
**inference requests** — billed calls, caused by a proxy whose purpose is to reduce them. Cheap to
add, and it costs every routed user, not only the funnel.

Forwarded verbatim, with no pipeline. Returning the compacted count would be smaller and would be
wrong in the dangerous direction: the client budgets its own transcript from this number, and
because every component fails open, the next request could forward the full body and take a 400.
Over-reporting is recoverable; under-reporting is a failed turn. The cost of that choice is now
documented in `docs/reference/routes.md`, where the route was absent entirely — a routed session
self-compacts earlier than it needs to (115,933 reported vs 32,802 forwarded on a measured body).

The hosted branch has tests, because that branch is the only thing standing between the
multi-tenant service and an unmetered open forwarder that would send OUR credential upstream.

## 3. Our own docs said the binary needs a C toolchain

`docs/setup.md`, `docs/hosted.md` and `docs/get-started/quickstart-proxy.md` all told evaluators to
install one. It is needed for `go test -race` and for the optional `cg_skeleton` tag, not for the
binary. setup.md went further and named **bifrost's tokenizer** as a cgo dependency, which it never
was — o200k_base is embedded (`internal/tokens/tokens.go`).

Asserted rather than re-claimed: a new `purego` CI job builds with `CGO_ENABLED=0` and
`CC=/nonexistent-c-compiler`, checks the artifact is statically linked, starts it and probes
/healthz. It also runs the packages whose behaviour depends on which components compile in —
because `build-test` runs exclusively with `CGO_ENABLED=1` (the race detector needs it), so
`TestEveryPresetBuilds` had **never executed in the configuration a user would build**. That guard
exists for exactly the `preset: coding` / `unknown component "skeleton"` breakage.

## 4. Preset facts stated outside the guarded files (#143, #145)

- The binary defaults to **`house`**; five sites said `codesmart` (README x3,
  `docs/reference/config.md`, `docs/get-started/quickstart-proxy.md` — the last is step 2 of the
  first page anyone runs). Anyone running the binary bare while reading those measured a different
  configuration than the published SWE-bench numbers describe.
- README's `codesmart`/`codesafe` pipeline lists and
  `docs/get-started/connect-ibm-service.md`'s "Default pipeline" were stale — naming `toon`,
  retired after acting 0 of 5,752 production requests, and omitting components that do run.
  The IBM page's omission of `toolfilter` matters most: that page is what a prospective hosted
  tenant reads to decide what the service does to their traffic.

All regenerated from the `presets` map. The two tables inside #142's drift guard are untouched
here; these are the sites that guard cannot reach.

## Verification

Five mutations, each proven to have landed in the source before its result was allowed to count:

  expand injection ungated        -> TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist FAIL
    on cachesplit-only, `safe`, and `off`
  HasOffload always false         -> same test FAIL on `mcp` and the offloader pipeline: "mints
    markers but no longer advertises the expand tool, so a model cannot recover what it offloaded"
  count_tokens route unregistered -> TestCountTokensIsServed FAIL (404)
  count_tokens rewrites the body  -> TestCountTokensIsServed FAIL
  hosted auth removed             -> TestCountTokensHostedRequiresAuth FAIL (502, want 401)

The second is the mirror-image check: it proves the gate did not trade one silent defect for
another, an offloader whose output nothing can expand.

Two things I got wrong on the way, recorded because both were caught by tests rather than by me:

- I first asserted `mcp` had no offloader. `smartcrush` implements `components.Offload`
  (`components/offload/smartcrush.go`), so that pipeline genuinely mints markers and genuinely
  needs the tool. The case now asserts the opposite, with the reason — and it is the argument for
  asking the interface rather than keeping a hand-written list.
- Copying `proxy/proxy.go` wholesale from the older distribution branch onto current main silently
  reverted #155's `effPreset`/`notePreset` work. `TestCompactRowNamesThePresetThatRan` — a test I
  had never read — failed with "the dashboard names a pipeline that did not run". The file was
  restored from main and the two edits re-applied on top; #155's change is intact.

`go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean.

One unrelated flake seen once and not reproduced: `TestConcurrentCallsDoNotRaceOnTheGateHistogram`
failed in a full-suite run with "no single-flight follower ran ... the race was never exercised",
then passed 8/8 in isolation and in two further full suites, and passes on clean main. Reported
separately rather than papered over.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>

* ci(purego): run one package binary at a time

The new job runs `go test` over five package trees, and `go test` starts up to GOMAXPROCS package
binaries in parallel. On a 2-core CI runner that added a second heavily-parallel run of the proxy
package per PR, and under that contention a timing-sensitive control-plane test from #150
(TestCtlGetCampaignAggregatesPredictedAndRealPerTenant) failed on two unrelated PRs — then passed on
a re-run of the same commit, and passes 3/3 whole-package on a 16-core box against both main and the
affected branch. Filed as #163.

Hunting that flake is not this job's business. Not provoking it is: `-p 1` costs about a minute and
removes the contention this job introduced, without dropping any coverage.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>

* fix: make this PR's own prose true, and unit-test HasOffload

Review of #161 found the same defect this PR is themed on — claims that are not true — in seven more
places, one of them in shipped code. All seven fixed, plus the `HasOffload` unit test the reviewer
raised without asking for.

Rebased onto current main first, so #142's preset-table guard and the preset pass below cannot
re-fix or re-break each other.

## Merge-blocking

**1. `proxy/proxy.go` named `mcp` as offloader-free.** It is not: `smartcrush` implements
components.Offload, which this PR's own test asserts (`wantAdd: true`) and its own mirror-image
mutation proves. I had corrected the test and left the comment wrong — the copy a future reader
actually trusts.

The fix deletes the list rather than correcting it. The comment now names the shapes affected
(`off`, `safe`, any cachesplit-only configuration) and then says why enumerating presets here is the
wrong move: a list in a comment is a second source of truth, and this one was wrong about `mcp` on
its first draft. That is the whole argument for gating on the interface.

**2. Five sites named the `cache` preset, which does not exist on this base.** It is #141's, and it
reached here in the wholesale copy of `proxy/proxy.go` this PR already admits to, then travelled
into the test files when they were split out of that branch. Reworded to name configurations that
exist here; the underlying defect they describe is unchanged and still reproduces on `off` and
`safe`.

**3. `proxy/counttokens_test.go` carried copy-paste artifacts vet and gofmt cannot see.** A
duplicated 3-line doc comment, and a 20-line orphan documenting a function that lives in
`expandgate_test.go` under a different name and citing a test that exists nowhere. Both from the
same cause: my splitter took each test's doc comment by scanning back to the previous blank line,
which swallowed the FOLLOWING test's comment as a trailing block. A third artifact the review did not
list is fixed too — `expandgate_test.go`'s doc comment still described "the preset's promise" and
cited `docs/how-to/install-plugin.md` and an install skill, both of which belong to #160.

## The rest

**4.** `docs/reference/config.md` and `docs/components.md` said `auto` injection has exactly two
conditions. It has three. Both now say so, and say what the third is for. The cache-stability
argument those passages make is unaffected — a pipeline does not change turn to turn either — so it
gained a member rather than needing a rewrite.

**5. `make build` now sets `CGO_ENABLED=0`.** The docs could claim "no C toolchain" all they liked
while step 1 of the quickstart was `make build`, which needed one because the Makefile exported
`CGO_ENABLED=1` for every target. Pointing readers at `build-static` would have fixed the sentence;
making the DEFAULT build pure Go makes the claim true of the command the docs tell people to run.
`CGO_ENABLED=1` stays for the test targets, where `-race` requires it, and the comment says exactly
that. Verified: `CC=/nonexistent make build` produces a statically linked binary.

README, CLAUDE.md and the quickstart no longer require a C toolchain. All five remaining
`codesmart`-is-the-default sites are corrected — including two in `config/config.go`, which is how
the claim spread to five documents: it sat three lines from the flag that disproves it.

**6.** `docs/setup.md` overstated its own evidence, which is the exact sin this PR is about. It
claimed CI removes the C compiler from `PATH` (with cgo off the toolchain never consults `CC`; that
variable is a tripwire, not the mechanism) and that cross-compilation to four targets is asserted,
when CI builds native linux/amd64 only. Now says what CI actually does, and states separately that
the other three targets were verified by hand and are asserted at release time. Same overstatement
fixed in the `ci.yaml` comment.

**7.** `ci.yaml` promised a linked issue and linked nothing, and named a different flake than the PR
body did. Both are real; the comment is about the campaign one, and now links #163.

## HasOffload unit tests

`./components`: nil-safe, empty pipeline (the A/B control arm), reformatters-only, and an offloader
in three positions. Revert-verified both ways — always-true fails the empty and reformatter cases,
always-false fails the offloader cases.

A registry-walking test was supposed to make it rot-proof, and **it skipped**: registrations happen
in `components/all`, so a test inside `components` can neither see them nor import the package that
does. A test that skips reads as coverage and is not, so it moved to `components/all`, where it
runs — 21 of 21 registered components, 13 implementing Offload. It fails if either count is zero,
because an all-false or all-true population would agree with a broken HasOffload.

Full `go test ./...`, `go vet ./...` and `gofmt -l` clean; doc link/anchor checker re-run over every
document touched.

Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>

---------

Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants