fix(test): stop pinning live-measured totals; assert instrument correctness three ways - #1327
Merged
Merged
Conversation
main is red: 'honours PHASE_ITERS as a comparison constant' timed out at vitest's 5000ms default on a loaded CI runner (5,690ms). This blocks green CI for every open PR. Same class as #1302 (send-integration afterAll): tests that shell out to real scripts do real work and need real budgets. Not a flake - a latent failure. On an UNLOADED machine these tests take 3.9-4.0s against the 5s default, i.e. ~80% of budget before any contention. The sibling 'emits byte-identical output twice' hit 4,576ms in the same CI run; it was next. Scope: 12 blocks in one file that invoke run() (11 tests + the beforeAll that populates the shared measurement output). Identified by parsing the file for blocks whose body calls run(), not by eyeballing. Non-shelling tests in the same file are pure fs reads and keep the default budget - a timeout on a test that cannot be slow is noise. 60_000 per the #1302 precedent. Nothing else in the diff. Follow-up, deliberately NOT in this hotfix: the underlying cost is that measure-prompt-surface.sh spawns python3 once per file for include expansion. Making that a single pass would cut seconds off every invocation. Worth doing; not worth coupling to an unblock-main commit.
…ctness three ways The reproduction tests asserted ALWAYS_ON_WORDS === 34231 against the REAL repo, so every always-on edit by every project fires them. Spec 1307 hit this on day one and handled it correctly - causal check, then a justified bump. But the incentive it creates for the NEXT project is bump-without-checking, which is the regression the test exists to catch: the number moves for two different reasons (the instrument broke / the surface changed) and the test cannot distinguish them, so it delegates that judgement to whoever is least placed to spend time on it. Three layers replace it. None can be disturbed by a legitimate surface edit: 1. INVARIANTS over the live repo - the composition arithmetic, true at any surface size (ALWAYS_ON = SHARED + SPAWN + I x (HOT + PHASE mean); architect = SHARED + ARCHITECT; PHASE_ITERS linear). 2. ABSOLUTE values on a synthetic FIXTURE whose arithmetic a human can check by hand (SHARED 7, SPAWN 10, ALWAYS_ON 107). This pins the instrument's correctness WITHOUT pinning the repo's content. 3. BYTE-assertion on the frozen baseline artifact - a historical record, where editing it should be a deliberate act. WHY LAYER 2 IS NOT REDUNDANT, verified by mutation rather than argued: dropping HOT from SHARED (a real instrument bug) leaves every internal identity balanced, because the wrong value propagates consistently. Under that mutation ALL THREE INVARIANT TESTS PASS and all three fixture tests fail. Invariants alone would have shipped it. (The T1 structural test also fires on that particular mutation because it pattern-matches the SHARED= source line - but that only catches mutations touching that line, not a component bug that resolves the wrong file.) Verified in the other direction too: adding 44 words to CLAUDE.md - the exact edit shape that fired the old assertion - keeps all 24 tests green, so no unrelated builder is asked to bump a number they do not own. Live absolute numbers move to the phase manifests and generated artifacts, where a human reads them as findings rather than maintaining them as expectations.
…fix/1280-instrument-invariant-tests # Conflicts: # packages/codev/src/__tests__/spec-1280-measurement-instrument.test.ts
Contributor
Author
Merged #1321's timeout fix into this branch — it was carrying the same latent redSelf-review catch before this sat in the merge queue: this PR branched from Merged One conflict, resolved in favour of this PR: the hotfix carried a budgeted copy of Verified after resolution: zero unbudgeted shelling tests, zero conflict markers, 24 tests Merge order is unchanged — this still belongs behind #1321. |
mohidmakhdoomi
approved these changes
Aug 1, 2026
mohidmakhdoomi
left a comment
mohidmakhdoomi
left a comment
Collaborator
There was a problem hiding this comment.
Approving to unblock main. Test-only change to spec-1280-measurement-instrument.test.ts: gives the script-shelling measurement tests explicit 60s budgets and reworks assertions to stop pinning live-measured totals. Verified green on CI (Tests + CLI Integration). Resolves the 5000ms timeout on 'honours PHASE_ITERS as a comparison constant' that has main red; supersedes #1321.
waleedkadous
added a commit
that referenced
this pull request
Aug 1, 2026
drop Architect corrected their own report to 1280 (my baseline bump is on THIS branch, not main) and flagged that #1327 replaces the absolute-pin tests with invariants + synthetic fixture, making my bump unnecessary and conflicting. VERIFIED rather than assumed, before the rebase: fetched fix/1280-instrument-invariant-tests, confirmed the absolute pins are gone, confirmed exactly one conflicting file, then swapped THEIR test file into this worktree and ran it against this branch's docs — 24/24 pass unchanged. So dropping my bump and taking theirs wholesale is safe, and I know that now rather than discovering it mid-rebase. Backed my file up first; restored after. Their new file names this case directly: the absolute form 'fires on every always-on edit by every project — Spec 1307 hit it on day one, correctly checked causality and bumped it. But the incentive it creates for the NEXT project is bump-without-checking.' That is the design question I flagged rather than decided, now fixed at its root by the project that owns it. Consolidated all three pending merges into one table with what 'done' looks like for each, because three separate queued merges is exactly where one gets forgotten.
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
2 participants
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.
Stop pinning live-measured totals in the instrument tests
Routed from the architect after Spec 1307 hit this on day one.
The problem with the current form
The reproduction tests assert
ALWAYS_ON_WORDS === 34231against the real repo, so everyalways-on edit by every project fires them. 1307 handled it exactly right — causal check, then
a justified bump with the derivation preserved.
But the incentive it creates for the next project is bump-without-checking, which is the
regression the test exists to catch. The number moves for two different reasons — the instrument
broke, or the surface changed — and the test cannot distinguish them, so it delegates that
judgement to whoever is least placed to spend time on it.
Three layers replace it
None can be disturbed by a legitimate surface edit:
ALWAYS_ON = SHARED + BUILDER_SPAWN[spir] + I × (HOT + PHASE mean),architect = SHARED + ARCHITECT, andPHASE_ITERSlinearity.SHARED 7,SPAWN 10,ALWAYS_ON 107. This pins the instrument's correctness withoutpinning the repo's content.
be a deliberate act.
Why layer 2 isn't redundant — verified by mutation, not argued
Invariants have a blind spot: a component that is silently wrong still satisfies every internal
identity, because the wrong value propagates consistently.
Mutating the script to drop
HOTfromSHARED— a real instrument bug:Invariants alone would have shipped it. (The pre-existing T1 structural test also fires here,
but only because it pattern-matches the
SHARED=source line — it would not catch a componentbug that resolves the wrong file.)
Verified in the other direction too
Adding 44 words to
CLAUDE.md— the exact edit shape that fired the old assertion — keeps all24 tests green. No unrelated builder is asked to bump a number they don't own.
Where live numbers go instead
Into the phase manifests and generated artifacts, where a human reads them as findings
rather than maintaining them as expectations.
Note
Branched off
d42a061a. 1307's interim bump (34293/8661) is not onorigin/mainas of thisbranch point —
mainstill readstoBe(34231)— so there is no conflict to resolve here. Ifthat bump lands separately it will conflict with this file; happy to rebase.