Make POST /__aimock/reset the canonical full reset and converge llmock.reset() - #358
Merged
Conversation
commit: |
jpr5
force-pushed
the
reset-route-honest-naming
branch
3 times, most recently
from
August 6, 2026 02:43
a024cb2 to
e4fbf67
Compare
jpr5
added a commit
that referenced
this pull request
Aug 6, 2026
…artifact is pinned (#359) Two follow-ups on `main` from the adversarial review of #357. ## 1. The rejection/dedup query is already saturated — silently `gh pr list --limit N` is a window, not a ceiling: it returns the newest N rows and drops the rest with no flag, no warning and no count. The marker self-heal — the thing that makes a human's rejection survive a body edit — was a plain `--state all --limit 200` whose state filter is applied *client-side*, so every PR in the repo competed for those 200 slots and the merged ones won. Measured live on this repo, 2026-08-05: ``` gh pr list --state all --limit 200 -> 200 [MERGED=184 CLOSED=14 OPEN=2] #125..#358 gh pr list --state closed --limit 200 -> 200 [MERGED=186 CLOSED=14] gh pr list --state all --limit 200 --search "is:unmerged" -> 28 [CLOSED=26 OPEN=2] #1..#358 ``` Two things the review did not have: - **`--state closed` is not a fix.** gh maps it to CLOSED-**or**-MERGED — 186 merged against 14 closed, just as full. - **Twelve closed PRs are already invisible.** The repo has 26 unmerged-closed PRs; the current listing sees 14. The lost set is `#1, 30, 50, 56, 59, 61, 63, 94, 95, 96, 119, 120`, each confirmed CLOSED by `gh pr view`. None is a drift PR, so no rejection is lost *today* — but the mechanism is running now, and MERGED is the population that grows daily. **Fix.** Both self-heal listings additionally query `--search "is:unmerged"` and union the two views. That excludes merged PRs server-side: 200/200 slots consumed becomes 28/200, and 28 is the *complete* unmerged population back to #1. `is:unmerged` is a STATE predicate, so unlike `<key> in:body` it cannot be defeated by the very body edit the self-heal repairs, and a just-closed PR matches under either indexed state. The plain listing is unioned rather than replaced, so the index-free view stays and plain entries win on collision. Headroom is not a proof, so saturation is also made loud: `assert_listing_complete` refuses any listing that comes back full at its `--limit` rather than deciding on it. `--limit 200` also stopped being a magic number — each step binds `PR_LIST_LIMIT` once and the audit reads the same variable. ## 2. "the one unpinned executable in the job" was false `89e5a07` pinned `ollama.com/install.sh`. That script then streams `ollama.com/download/ollama-linux-<arch>.tar.zst` — mutable, unversioned, no digest — through `zstd -d` into `sudo tar -x` under `/usr/local`. Run verbatim out of the pinned script's own bytes, with curl serving attacker content: ``` STEP_EXIT=0 SUDO_RAN argv=tar -xf - -C ./dest TAR_RAN argv=-xf - -C ./dest what tar was handed: ATTACKER-CONTROLLED TARBALL BYTES ``` A compromised tarball plants a root-owned `git`/`gh`/`node` on PATH exactly as well as a compromised script would, and every later step holds a `contents:write` + `pull-requests:write` app token those binaries can reach. It cannot be fixed in place — install.sh pipes the download straight into `tar`, so it never holds the file and has nothing to verify; `OLLAMA_VERSION` only appends `?version=`, a version pin rather than a byte pin. **Fix.** install.sh is not used. The release artifact is fetched from an immutable release tag and sha256-checked before anything unpacks it. Digest agreed by three independent sources: the v0.32.6 release's `sha256sum.txt`, the GitHub release API's asset `digest` field, and sha256 of the 1,420,686,963 downloaded bytes. Archive layout confirmed by listing it (`bin/ollama` + `lib/ollama/*`, so `-C /usr/local` puts the binary on PATH). Dropping install.sh also drops its NVIDIA CUDA repo adds and `$PACKAGE_MANAGER -y install`; the step already ran `ollama serve` itself rather than using its systemd unit. The false claim is corrected in the workflow comment, the suite's header comment, its describe name and its failure messages. Two further stale claims about the self-heal listing `--state open` (false since #357) are corrected too. The commit message that first asserted it is in history and cannot be rewritten. ## Red-green The `gh` stub could not previously express item 1's bug — it ignored `--limit` and returned the whole population, so a saturated window looked exactly like a healthy one. It now models what real gh does. **RED (watched).** The existing rejection scenario plus 200 newer MERGED PRs — both PR-open steps re-proposed the rejected changeset: ``` FAIL a rejection is still found once MERGED PRs have filled the listing window stdio: Pushed branch main-1111222233334444 … Matched PR #56 FAIL needs_human_pr: a rejection survives a full window there too stdio: Pushed needs-human branch … Opened needs-human PR #57 ``` **GREEN.** 154 passed, exit 0. **RE-BREAK (watched), each mutation restored and md5-verified:** | mutation | tests red | |---|---| | neuter the union so unmerged rows are dropped | 2 | | delete all four `assert_listing_complete` calls | 2 | | raise the audit threshold out of reach | 1 | | replace the tarball digest comparison with `if false` | 1 | | point the fetch back at `ollama.com/install.sh` | 1 | Item 2's guard now watches `tar`, not `sh`, and the positive control asserts `tar` was handed exactly the verified bytes rather than merely that it ran. ## Gates `pnpm build` 0 · `pnpm test` 0 (**5048 passed / 171 files**) · `pnpm test:drift` 0 · `tsc --noEmit` 0 · `pnpm lint` 0 · `pnpm test:exports` 0 · prettier clean · `actionlint` 0 · `zizmor` 0 findings · `bash -n` on all 16 `run:` bodies 0 · commitlint 0. ## Not fixed here `.github/workflows/test-drift.yml:177` does the same `sh install.sh` with **no pin at all**, in a job holding five provider API keys, with provisioning ahead of the steps that use them. Same defect, different file — flagged, not touched.
jpr5
force-pushed
the
reset-route-honest-naming
branch
from
August 6, 2026 03:15
c56c40b to
b3a98b0
Compare
…reset() store coverage
…he /reset/fixtures alias The full reset was reachable under two names, and the deprecation pointed at the dishonest one: /reset/fixtures clears fixtures, journal entries, fixture match-counts, video and fal job state, and the Gemini counters. /reset is now the canonical route; /reset/fixtures keeps working unchanged but carries the Deprecation header, the deprecated/deprecation body fields, and a log warning. performFixturesReset is renamed performFullReset and exported, and LLMock.reset() now calls it — previously the in-process reset left veoVideoJobs, grokVideoJobs and the Gemini interaction/event-id counters populated.
The Route Overview table and the per-route sections now name the full reset's real blast radius — fixtures, journal entries, fixture match-counts (sequence position), video + fal.ai job state, and the Gemini interaction and event-id counters — and put the deprecation on /reset/fixtures. aimock-pytest: reset() and reset_fixtures() both call POST /__aimock/reset. Both already performed a full reset, so observable behaviour is unchanged; they just no longer trip the deprecated alias.
… the changelog Four stores were unguarded — dropping falJobs.clear(), falQueueStates.clear() or videoStates.clear(), or swapping journal.clear() for clearEntries(), all left the suite green. Adds a test per store, each verified to go red under its own mutation. The match-count one guards the sequence-position rewind the docs now headline. The changelog claimed the Python client no longer emits a deprecation warning. That is false while _version.py pins 1.38.0, whose /reset IS the deprecated alias; the pin bump is now recorded as a release follow-up. Also drops the unqualified 'behaviour is unchanged' for /reset/fixtures — its reset semantics are unchanged but its response body is additively extended.
…set gaps The alias's deprecation assertion could not fail: every substring it checked is present in a message pointing back at itself. It now asserts the whole string. Three paths had no coverage at all — the search/rerank/moderation clear that is the documented in-process divergence, the pre-start path where performFullReset takes a null target, and match-counts under a non-default testId, which let a default-only clear strand every other tenant. Docs: a full reset does not always yield no_fixture_match — in record mode an unmatched request is proxied to the real provider, which costs real money. The changelog now files the route change under Deprecated, per the 1.29.0 precedent, and both it and the reset() docblock record that reset() clears MODULE-GLOBAL state, so one instance's reset rewinds another's Gemini ids.
…gauge signals The Python client's switch to the canonical route was the only behavior change in the diff with no guard: both routes full-reset, so test_reset_clears_fixtures passes either way, and reset_fixtures() had no test at all. Both now spy on the control call and assert the canonical route's deprecation-free response, with the alias asserted alongside so the discriminator is anchored rather than vacuous. Also covers the two remaining deprecation signals — the log warning, in both directions, and the fixtures-loaded gauge that reset() now re-zeroes. Drops a weaker duplicate of the alias test, scopes the seeder handle to try/finally, orders the changelog Changed-before-Deprecated, and states precisely that the replay 404 and strict 503 both carry code: no_fixture_match.
… message "POST /__aimock/reset" is a prefix of the alias's own name, so the anchor assertion held even for a message pointing back at /reset/fixtures. It now keys on "use POST /__aimock/reset (full reset)", which a self-referential message does not contain.
jpr5
force-pushed
the
reset-route-honest-naming
branch
from
August 6, 2026 03:25
b3a98b0 to
1101d2b
Compare
jpr5
marked this pull request as ready for review
August 6, 2026 04:05
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.
What
POST /__aimock/reset/fixtureswas never a fixtures reset. It clears the fixtures array, the journal (entries and per-test fixture match-counts, across every testId),videoStates,openRouterVideoJobs,veoVideoJobs,grokVideoJobs,falJobs,falQueueStates, the Gemini interaction and event-id counters, and re-zeroes theaimock_fixtures_loadedgauge.POST /__aimock/resetdoes exactly the same thing — and it was the one marked deprecated. The deprecation pointed at the dishonest name.This inverts it and converges the in-process path onto the same implementation.
POST /__aimock/resetis canonical: plain{ "reset": true }, noDeprecationheader, nodeprecated/deprecationbody fields, no log warning.POST /__aimock/reset/fixturesis the deprecated alias, carrying the identical deprecation mechanics the old route used.performFixturesReset→performFullReset, exported and shared./reset/journalandDELETE /fixturesare untouched.aimock-pytest'sreset()andreset_fixtures()both target the canonical route.LLMock.reset()was not equivalent to the HTTP full reset — it leftveoVideoJobs,grokVideoJobsand both Gemini counters populated. It now calls the sameperformFullReset, so the two cannot drift again.One deliberate, commented difference:
reset()also clears the search / rerank / moderation fixtures. Those are registrable only throughLLMock; the control API has no route that creates them.Compatibility
/reset/fixturesreset semantics are unchanged — it clears exactly what it always did, so existing callers keep working. Its response body is additively extended:deprecatedanddeprecationnow accompanyreset, plus aDeprecation: trueheader. A caller doing strict equality on the old{ "reset": true }body must relax that assertion; a caller readingbody.resetis unaffected.LLMock.reset()clears state that is module-global, not per-instance — the Gemini counters and the fal.ai job/queue maps. With twoLLMockinstances live in one process,a.reset()rewinds the Gemini id sequencebis mid-way through, sobre-emitsaimock-int-0/evt_1, ids it already handed out. The fal maps were already global; the counters are newly reached. Documented in thereset()docblock and the CHANGELOG.Release follow-up (required, not done here)
aimock-pytestcallsPOST /__aimock/reset, but_version.pypinsAIMOCK_VERSION = "1.38.0"— the published, pre-fix server, where/resetis the deprecated alias. The client therefore trips a deprecation on every reset until the pin moves. BumpAIMOCK_VERSIONto the first npm release containing this change before publishing the nextaimock-pytest. The Python suite cannot catch this on its own becausetests/conftest.pyredirectsAIMOCK_CLI_PATHto the local build.Coverage
Every store
performFullResettouches has a guard, and every guard was verified to fail under a mutation that removes exactly what it claims to protect — then reverted and re-confirmed green./resetcarries no deprecationexpected 'true' to be undefined/reset/fixturessignals deprecationexpected undefined to be 'true'Expected: "…use POST /__aimock/reset (full reset)…"/Received: "…migrate to POST /__aimock/reset/fixtures…"warnto canonical/resetexpected "warn" to not be called at all, but actually been called 1 timesgrokVideoJobs.clear()expected 200 to be 404veoVideoJobs.clear()expected 200 to be 404videoStates.clear()expected 200 to be 404falJobs.clear()expected 200 to be 404falQueueStates.clear()expected 200 to be 404resetInteractionCounter()expected 'aimock-int-2' to be 'aimock-int-0'resetEventIdCounter()expected 'evt_6' to be 'evt_1'clearEntries()+clearMatchCounts("__default__")expected 2 to be +0expected [ { title: 'Weather Report', …(2) } ] to have a length of +0 but got 1targets === null) pathexpected 200 to be 404aimock_fixtures_loadedre-zerosetGaugeexpected '# TYPE aimock_fixtures_loaded gauge\n…' to contain 'aimock_fixtures_loaded{} 0'reset()back at/reset/fixturesassert 'http://…/__aimock/reset/fixtures'.endswith('/__aimock/reset')The match-count guard drives both the default scope and
x-test-id: tenant-a, using twosequenceIndexsiblings and re-adding the same fixture objects after the reset (counts are keyed by object identity). A default-only clear leaves every other tenant parked mid-sequence, so it is asserted explicitly.The Python guards spy on
requests.post, let the real call go out, and assert on the server's own deprecation signal — the only thing that distinguishes two routes with identical reset semantics. They anchor that discriminator by asserting the alias does signal deprecation, keyed onuse POST /__aimock/reset (full reset), the half of the message a self-referential variant would not contain.Suites
pnpm build && pnpm test: 172 files, 5079 passed, 0 failed, 0 skipped. The build matters — without itcli.test.tsisdescribe.skipIf(!CLI_AVAILABLE)and 45 tests silently skip.pytest tests/ -qagainst the locally builtdist/cli.js: 22 passed, 0 failed, 0 skipped, on CPython 3.12.13 (barepython3on the dev box is 3.9.6, belowrequires-python = ">=3.10"). Installed editable so the tests exercise the edited source.prettier --check,eslint,tsc --noEmit,commitlintall clean.pnpm test:driftnot run — it exercises live provider APIs and is unrelated to this change.Docs
The control-API page states what each route clears, and corrects a claim with a spend consequence: a full reset does not universally yield
no_fixture_match. In replay mode the next request fails404and in strict mode503(both carryingcode: "no_fixture_match"), but in record mode with a provider key configured it is proxied to the real provider — live upstream calls and real spend, not an error.Deliberately not done
aimock-pytest'sreset_fixtures()is as dishonestly named as the route was — it is a full reset. Kept as an alias forreset(); renaming a published Python API does not belong in this change.router.ts. Several are log strings asserted verbatim by tests, so rewording them is a behavior-adjacent change for its own PR. The phrase describes the concept accurately; only the danglingperformFixturesResetsymbol reference was a real defect, and that is fixed.🤖 Generated with Claude Code
https://claude.ai/code/session_017ATs7KdGhg9zhE1AkEMmo6