Repository navigation
feat(bench): complete spec 103 token-bench — live agent loop, reproducibility, publication draft - #1153
Merged
Conversation
…ite-up (60/64) US2, US3 and the US5 draft. Four units built in parallel, then an adversarial pass found five defects and three vacuous tests; all are fixed here. US2 — the live agent loop (bench/agentloop.go). Fully unit-testable with no suite, no model and no spend: the driver is parameterised over function types the way flipgate.go is, so every rule that decides what gets PUBLISHED is exercised by fakes. Classification implements the spec's binding Definitions and sees only an attempt sequence — no cell, no endpoint — so provably the same rule applies to the baseline arm and the proxy arms. The FR-020 baseline is enforced with two sentinels: a missing baseline is an error (a saving measured against nothing is fabricated, not smaller) and a "baseline" cell carrying an mcpproxy endpoint is a sixth proxy cell wearing the denominator's name. US3 — reproducibility. Private-recording provenance, partial-run blocking, run-local record retention that degrades to "records not retained" rather than dangling, and the reproduction procedure in the README including that setting TIKTOKEN_CACHE_DIR only NAMES a cache while a first run still downloads. DEFECTS FROM THE ADVERSARIAL PASS: 1. The regression rule fabricated a claim. cellHasCompletionFigure walked raw records while the completion rate it guarded came from an aggregate that drops a run WHOLESALE if any unit is partial. The two disagreed, so a cell where nothing was counted published regression:true and a "100.0 percentage points below baseline" reason — in the same row that said nothing was measured, with the JSON and the HTML disagreeing about it. Now gated on the aggregate, and on Withheld. 2. runs<4 was not enforced where it mattered. SavingVsBaseline — documented as the only sanctioned way to turn two cells into a percentage — never read Runs, so a caller could publish a 90% saving off a SINGLE run. It now refuses below MinHeadlineRuns, and the withheld check moved BEFORE the regression check so an unmeasured cell is never described as a regression. 3. A cell that measured nothing rendered 0.0% first-attempt success, 0 retries and ±0.0% spread, badged "measured". Only the two cost columns were inside the withheld guard. All derived columns now render em-dashes, and a single-run spread renders "spread undefined" rather than ±0.0%. 4. THE STRUCTURAL ONE: US2/US3 were libraries with NO production callers. cmd/bench never set ReportV2.AgentLoop, .RunStatus or .Inputs, so every replay report silently read as independently reproducible by omission. ReplayReport now stamps PrivateRecordingInputs — an outsider cannot obtain the operator's recording, and saying so is the difference between an honest limitation and an implied guarantee. 5. Three vacuous dashboard assertions, each proven to pass with the behaviour deleted: they matched STATIC TEMPLATE PROSE elsewhere on the page — the legend sentence contains "regression", the footnote contains "runs", and the page-wide legend supplies both provenance badges. Scoped to the row that must carry the mark via a new rowFor helper, and mutation-verified. Also: .pre-commit-config.yaml excludes .patch/.diff from trailing-whitespace. A unified diff encodes meaning in leading whitespace — a blank context line IS a lone space — and the hook silently rewrote 4 context lines of bench/mcpmark/mcpproxy_arm.patch, producing a patch git apply would reject. T049 and T055 are NOT ticked and are not done. T049 requires running a paid suite against third-party services; T055 requires an API key for the token-counting endpoint. Both are built up to that boundary and blocked only on spend and credentials. Gates: go build ./... and -tags server clean; go test ./bench/... green; -race green; golangci-lint v2 0 issues; serveredition tests green; make swagger diff-clean; git ls-files bench/results empty.
… bypassable (T064) Four findings from the opencode gpt-5.6-sol review, all verified and fixed. Two were High and both were the same shape: an invariant stated in one place and unenforced in the place that actually produces published numbers. 1. THE NEVER-SUM RULE WAS NOT ENFORCED ON THE LIVE PATH. AggregateTokenFigures was exercised by tests but no production path called it, while aggregateCell added token integers whose origin nothing checked and AgentLoopBlockFor then stamped the caller's provider onto the result — so a tokenizer-derived figure mixed into a provider run produced a plausible hybrid wearing a provider label. Two reviewers found this independently; the first pass raised it as a caveat and I did not act on it, which is why it survived to a second. UnitRecord now carries its own AccountingSource and aggregateCell refuses to add a record that disagrees with the aggregate. A cell whose records mixed sources is WITHHELD outright rather than published as a partial total, since a partial total presented as the cell's cost understates it. 2. SavingVsBaseline TRUSTED A MUTABLE FLAG. It is documented as the only sanctioned way to turn two cells into a percentage, but it read cell.Regression rather than deriving the regression from the completion rates sitting on both cells — so a block unmarshalled from a report.json or assembled by hand, with Regression:false and a 50-point completion deficit, returned a saving. It now derives the verdict, and the threshold it uses is serialised on the block so a figure recomputed from a saved report uses the number that run applied rather than whatever the default has since become. 3. Unmeasured figures serialised as measured zeros. FirstAttemptSuccessPct and SpreadPct are now POINTERS: with no determinate intents there is no rate, and with fewer than two per-run figures the spread is undefined. A serialised 0 rendered "0.0%" and "±0.0%" — a claim of perfect consistency from a single run, which is the opposite of what FR-021 asks the field to convey. 4. Per-row session provenance existed but never reached the report. ReportV2 still carried []SessionCostEstimate, so the row type that distinguishes a measured 0.0 retry rate from a defaulted one could not serialise or render. The envelope, the live path and the dashboard table now use SessionCostRow, and the table shows the retry-rate source and run count per row instead of asserting "these are estimates" in a footnote. Both High fixes are mutation-verified: disabling the mixed-source withholding or the derived regression fails its test, and both pass reverted. One test was comparing pointer ADDRESSES after the nullable change and would have silently always differed; it now compares values. T063 and T064 ticked. Gates: go build ./... and -tags server clean; go test ./bench/... green; -race green; golangci-lint v2 0 issues; all three schema validation tests PASS.
Deploying mcpproxy-docs with
|
| Latest commit: |
20806e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://de489c37.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://103-complete.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 33466200110 --repo smart-mcp-proxy/mcpproxy-go
|
Dumbris
enabled auto-merge (squash)
September 1, 2026 04:23
Unit Tests (windows-latest) failed on TestBaseline_CountToolWithSchemaParity in bench/arms with: load tiktoken encoding "cl100k_base": failed to rename temporary file: ... Access is denied. Not a generic flake, and not a regression from this branch — it is the reproducibility hazard spec 103 documents, biting CI. tiktoken-go downloads the BPE on first use and finishes with an atomic rename into a shared cache dir. `go test ./...` runs packages in parallel, so several test binaries race to download the same file; on Windows the loser fails its whole package on the rename. The test that fails is therefore whichever package lost the race, which is why it lands on files nobody touched. Setting TIKTOKEN_CACHE_DIR alone does NOT fix this — it names a cache, and an empty one still races. Populating it first does, because every later tokenizer construction is a cache hit that renames nothing. Adds bench/cmd/warmtiktoken and a step that runs it before both test steps, with TIKTOKEN_CACHE_DIR set for the sweep. The command doubles as the SC-004 offline prerequisite: an outsider reproducing the deterministic figures needs the vocabulary present, and "run this" is easier to follow than a paragraph explaining that the env var is necessary but not sufficient. The ubuntu failure in the same run was TestFastPathDoesNotOvertakeAQueue (internal/upstream/limiter) — a separate, documented, load-sensitive flake that this branch does not touch and that clears on rerun.
This was referenced Sep 1, 2026
Dumbris
added a commit
that referenced
this pull request
Sep 2, 2026
…st repeats (#1199) * chore: remove accidentally committed warmtiktoken binary, guard against repeats `warmtiktoken` (43 MB arm64 Mach-O, the build output of `go build ./bench/cmd/warmtiktoken` run from the repo root) landed at the repo root in #1153. Untrack it, ignore it alongside the other root binaries, and add two pre-commit guards so it cannot happen again: - `check-added-large-files` (8 MB cap, above the largest legitimate screenshot/report) - a local `no-native-binaries` hook that rejects Mach-O/ELF/PE files The blob stays in history; purging it needs a history rewrite, which is left as a separate decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * chore: replace real-looking e2e api_key fixture, ignore .osgrep/ `test/e2e-config.json` carried an auto-generated 64-hex api_key since #477. It only ever guarded a localhost test instance and scripts/test-api-e2e.sh extracts whatever key the core logs, so the value is irrelevant — but it trips every secret scanner (gitleaks: generic-api-key). Use an obviously synthetic value, matching the sandbox-integration workflow's "qa-sandbox-ci-test". Also ignore `.osgrep/`: `.osgrep/server.json` (local authToken) was committed on the unmerged pull/165 ref. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 2, 2026
… token-bench truth-sync (#1200) * chore(roadmap): raise mcp-2026-upgrade to P1 (mcp-go v1.0.0 stable), add activity-storage-bounds, sync token-bench - mcp-2026-upgrade: P3 -> P1. mark3labs/mcp-go v1.0.0 stable shipped 2026-09-02; go.mod still pins v0.57.0. Next step is speckit.plan (#532). - action-log-transparency: new task activity-storage-bounds tracking #1173/#1174 (response truncation never wired), #1176 (unbounded per-server tool_calls buckets) and #1175 (omitempty zero erasure). - token-bench-harness: in_progress -> done; #1141/#1147/#1151/#1153/#1160 are all merged. Clears both --check-github errors. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * chore(roadmap): keep merged token-bench PRs on the done task, not the in_progress epic Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes spec 103 to 62/64. The two unticked tasks need money or credentials, not engineering — detailed at the bottom.
What lands
US2 — the live agent loop (
bench/agentloop.go). Fully unit-testable with no suite, no model and no spend: the driver is parameterised over function types the wayflipgate.gois, so every rule that decides what gets published is exercised by fakes. Classification implements the spec's binding Definitions and sees only an attempt sequence — no cell, no endpoint — so provably the same rule applies to the baseline arm and the proxy arms.US3 — reproducibility. Private-recording provenance, partial-run blocking, run-local record retention that degrades to "records not retained" rather than dangling, and the reproduction procedure in the README.
US5 — the write-up draft, plus the MCPMark integration patch and example config.
Defects found and fixed
Two adversarial passes plus the Sol review found nine defects. The recurring species: an invariant stated in one place and unenforced where the numbers are actually produced.
The never-sum rule was not enforced on the live path.
AggregateTokenFigureswas exercised by tests but no production path called it, whileaggregateCelladded token integers whose origin nothing checked — so a tokenizer figure mixed into a provider run produced a plausible hybrid wearing a provider label.UnitRecordnow carries its own accounting source; a cell whose records disagree is withheld, not published as a partial total.Two reviewers found this independently. The first raised it as a caveat and I didn't act on it, which is why it survived to a second.
SavingVsBaselinetrusted a mutable flag. Documented as the only sanctioned way to produce a percentage, it readcell.Regressionrather than deriving it — so a block unmarshalled from areport.jsonwithRegression:falseand a 50-point completion deficit returned a saving. It now derives the verdict, and the threshold is serialised so a recomputed figure uses the number that run applied.A cell that measured nothing published claims about itself. The regression guard walked raw records while the rate it guarded came from an aggregate that drops whole runs; a cell where nothing was counted emitted
regression: truewith "100.0 percentage points below baseline" — in the same row that said nothing was measured, JSON and HTML contradicting each other.Unmeasured figures serialised as measured zeros.
FirstAttemptSuccessPctandSpreadPctare now pointers:±0.0%from a single run is a claim of perfect consistency, the opposite of what FR-021 asks the field to convey.Per-row session provenance never reached the report.
ReportV2still carried[]SessionCostEstimate, so the type distinguishing a measured 0.0 retry rate from a defaulted one couldn't serialise.Three vacuous dashboard tests passed by matching static template prose — the legend contains "regression", a footnote contains "runs". Deleting the entire provenance column left them green. Now row-scoped and mutation-verified.
A finding beyond the benchmark
T061 compared the Web-UI savings figure against the bench headline over one live fleet: +68.4% vs −32.5%, same 45 tools, opposite sign.
SavedTokens = totalTokens − avgQuerySizenever subtracts the proxy's own menu — 5783 tokens here, larger than the entire baseline it replaces — and negatives are clamped to zero, so the figure structurally cannot report that the proxy costs more.Written up in
specs/103-token-bench/findings/webui-savings-vs-bench.md. This is user-facing and worth its own fix.Incidental
The trailing-whitespace pre-commit hook corrupted the MCPMark patch, stripping the leading space from context lines — structural in a unified diff.
.patch/.diffare now excluded.Not done, and why
Both are built up to that boundary: the driver, the patch, the config and the tests exist.
Verification