Skip to content

feat(lens): emit per-argument param_modes at every typed-row site (BACKLOG #237) - #1170

Merged
wshallwshall merged 4 commits into
mainfrom
claude/builder-237-step1-param-modes
Sep 16, 2026
Merged

wshallwshall merged 4 commits into
mainfrom
claude/builder-237-step1-param-modes

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

BACKLOG #237, step 1 of 3 (AC-M1, AC-M2, AC-M7). ADR 0076 Amendment E.

_param_mode and its bounded-shape predicates shipped in PR 559 with no production caller. This
gives it four. Nothing in ide/ is touched: the stepsModel.ts consumer and the mode selector are
steps 2 and 3, which are not dispatched.

The four emission sites

Each was re-located by symbol, not by the line numbers in the brief.

Site Symbol What it now emits
1. native slots _native_action_row param_modes over native.slots plus native.display
2. wrapper action _classify_simple, _ACTION_PARAMS branch _param_modes_for_call(call, _ACTION_PARAMS[name])
3. diagnostic _classify_simple, _DIAGNOSTIC_PARAMS branch _param_modes_for_call(call, _DIAGNOSTIC_PARAMS[name])
4. lookup _classify_simple, _LOOKUP_PARAMS branch _param_modes_for_call(call, _LOOKUP_PARAMS[name])

E.4 names action and lookup. Site 3 is included because a diagnostic row carries typed params
and literal_params exactly as those two do, and E.7 scopes modes to "rows that have typed
arguments". Leaving it out would give one row kind an "is this EDITABLE" answer with no "what SHAPE
is this" answer beside it, which is the asymmetry E.10's root-cause paragraph warns against.

AC-M1 totality is structural, not coincidental

_render_params and the new _param_modes_for_call now share one name mapping,
_rendered_param_nodes. A key cannot exist in params and not in param_modes, because there is
only one place a key is named. That covers the synthetic *arg1 and **kwargs keys and the extra
positionals past a signature.

On the native path params and param_modes are both built from [*native.slots, *native.display],
so the display kwargs get a mode. literal_params is untouched and still covers slots only: a
constant occurrence= is static by shape and absent from literal_params by design. Widening
literal_params was considered and refused (E.10) -- E.8 depends on an older consumer reading it
unchanged.

AC-M7: which version is the pre-Amendment-E one

param_modes rides CONTRACT_V2, so CONTRACT_V1 stays byte-identical to the pre-amendment parser,
exactly as it does across Amendments A and D. This is the one judgement call in the change, so
the reasoning is here rather than only in the code:

  • It does not mint a CONTRACT_V3. E.8 puts the FORWARD skew on the consumer -- an older IDE keeps
    reading params/literal_params and ignores the key it does not know. ide/src/stepsModel.ts
    pins LENS_CONTRACT = 2, so that sentence is about a live consumer rather than a hypothetical one.
    A version of its own would instead make an older engine REFUSE a newer IDE outright, which is the
    other skew direction and not the one E.8 describes.
  • Amendments A and D both landed in CONTRACT_V2; the project does not mint one version per
    amendment.
  • Step 2 can then read the map without a second engine-side contract bump.

Pinned two ways in Python, against the existing contract mechanism: a hardcoded V1 row-key
allowlist across the whole corpus (so any new V1 key fails, not just this one), and a strict
comparison that stripping param_modes from the v2 rows reproduces the v1 rows exactly.
End-to-end through the CLI: lens parse - --json carries no map; --contract 2 carries
{"path": "static", "value": "static", "occurrence": "static"}.

MEASURED: the samples corpus cannot test this

samples/config produces 28 rows at CONTRACT_V1 and 67 at CONTRACT_V2, and not one carries
params
-- it contains no action, diagnostic or lookup row at all. AC-M2's "across the whole
samples corpus" is therefore vacuous against the shipped corpus, and a corpus-only test would
pass over an empty set.

So the tests run over an adversarial corpus and the samples modules, with a positive control that
asserts the samples contribution is empty and fails the day it stops being (at which point the note
in the test is stale, not the test). The adversarial corpus carries the shapes that matter: a native
display kwarg, a wrapper positional past the signature, *args, **kwargs, templated f-strings on
both paths, and a dynamic mapping argument.

Negative controls, one per emission site

Each site's emission was disabled in turn and the tests re-run. Every result below is observed, not
predicted.

Control Result
Site 1 native emission off its own test RED, AC-M1 corpus test RED
Site 2 wrapper action off its own test RED, AC-M1 corpus test RED
Site 3 diagnostic off its own test RED, AC-M1 corpus test RED
Site 4 lookup off its own test RED, AC-M1 corpus test RED
Totality broken (native drops display kwargs) AC-M1 corpus test RED, site 1 test RED
Contract gate removed (v1 emits the map) both AC-M7 tests RED
static widened to list/tuple AC-M2 biconditional RED

The nine new-behaviour tests were also confirmed RED against the pre-change code before the
implementation landed.

Checks

Run in this worktree on its own .venv:

  • ruff check . -- passed
  • ruff format --check . -- passed (after formatting the two changed files)
  • mypy messagefoundry (strict) -- passed, 274 files
  • pytest over the 13 tests/test_lens_*.py files -- 557 passed, 3 skipped, serially and under
    -n 4
    , identical counts, so nothing diverges per xdist worker
  • pytest tests/test_checks.py tests/test_actions.py tests/test_corepoint_import.py -- 149 passed,
    1 skipped
  • pre-commit hooks at commit time -- all passed

Skipped, and why: the full suite, which under fleet contention may never finish. Legs to read after
this process exits: the four test legs, and anything that shells lens parse.

Ledger banner for the Lander

Proposed text for the vault row. I did not edit docs/BACKLOG.md.

Step 1 SHIPPED (this PR). _param_mode has its first production caller: param_modes is
emitted at all four typed-row sites (native action, wrapper action, diagnostic, lookup), total over
params, gated to CONTRACT_V2 so CONTRACT_V1 stays byte-identical. AC-M1, AC-M2 and AC-M7 are
met and pinned. Steps 2 and 3 remain and are not dispatched: the ide/src/stepsModel.ts
consumer, and the per-argument mode selector in the webview. AC-M3 to AC-M6 (the writable
templated rewrite path) are untouched.

This row was measured wrong in four specific ways at HEAD. Whoever fixes it should correct all
four:

  1. "the ten-way taxonomy step 1 needs is not computed anywhere" -- FALSE. MODE_STATIC,
    MODE_TEMPLATED, MODE_DYNAMIC, _is_bounded_message_read, _is_bounded_interpolation and
    _param_mode are all on main in messagefoundry/lens.py, with tests/test_lens_param_modes.py
    (PR 559).
  2. "no part of the mode selector exists" -- FALSE, same evidence.
  3. "both named backlog dependencies (#1758, #1760) are still open" -- FALSE. Both are CLOSED, and the
    row's own 2026-08-23 amendment already says so; the 2026-08-20 re-score carries the stale sentence
    forward.
  4. The header says P2 while the row's own re-score block says P3.

Open question, for the next brief rather than for a reply

Should diagnostic rows carry param_modes? I emitted on all four sites per the brief, with the
E.7 reasoning above. AC-M1's text names only action and lookup, so a reader checking the ADR
literally will find site 3 unlisted. If the owner or the ADR wants the narrower reading, dropping
site 3 is a two-line change plus its test. If the broader reading is right, AC-M1's wording is worth
widening to "a row with typed arguments" when the ADR is next touched.

…CKLOG #237)

ADR 0076 Amendment E's _param_mode classifier shipped in PR 559 with no production
caller. This gives it four: the native action row, and the wrapper action,
diagnostic and lookup rows.

AC-M1 (total over params) is structural, not coincidental: _render_params and the
new _param_modes_for_call now share one name mapping, _rendered_param_nodes, so a
key cannot exist in one and not the other. The native path builds both maps from
slots plus display, so a constant occurrence= gets a mode while staying out of
literal_params -- a literal that is not editable, which is why AC-M2 is scoped to
editable params (E.10). literal_params is untouched; widening it was considered
and refused.

AC-M7: the map rides CONTRACT_V2, so CONTRACT_V1 stays byte-identical to the
pre-amendment parser. It does not mint a version of its own because E.8 puts the
forward skew on the consumer, and the shipped extension asks for contract 2.
@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 15, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

LANDER review. Read against head 05e32c5. Merge.

THE COMPATIBILITY DECISION IS THE WHOLE REVIEW AND IT IS RIGHT. param_modes is a new FIELD, not a new KIND. This module's own docstring is explicit that a row kind reaching an OLD consumer "renders a blank, titleless row", so a new kind is the one change shape that breaks a shipped IDE. A field cannot: an older consumer ignores a key it does not read. And it rides CONTRACT_V2 rather than minting a version of its own, which is argued rather than asserted -- a version of its own "would instead make an older engine REFUSE a newer IDE outright, which is the other direction and not what E.8 describes." That is the correct reading of forward skew, and the note that the shipped extension asks for contract 2 today makes it about a real consumer rather than a hypothetical.

THE BEST THING IN THIS PR IS A TEST THAT EXISTS BECAUSE THE AUTHOR CAUGHT THEIR OWN SUITE BEING VACUOUS. test_the_corpus_actually_carries_rows_of_every_emission_site states it plainly:

MEASURED 2026-09-15: samples/config produces 28 rows at CONTRACT_V1 and 67 at CONTRACT_V2, and NOT ONE of them carries params -- the shipped samples use native msg[...] assignment and helper calls the vocabulary does not name... A corpus-wide AC-M1/AC-M2 test over samples/config alone therefore passes over an empty set, which is indistinguishable from a correct implementation.

The natural corpus is EMPTY for the property under test. A suite built on it would have gone green over nothing, forever, and looked thorough doing it. Finding that before shipping, and then keeping the measurement in the docstring so the next person cannot un-learn it, is the single behaviour this repository has most needed this week.

THE SAME INSIGHT APPEARS AGAIN IN V1_ROW_KEYS, and it is worth saying twice:

Hardcoded on purpose: derived from the corpus it would absorb any new key and assert nothing. Adding to it is a contract change.

A derived allowlist self-updates and therefore cannot fail. Hardcoding turns "did a new key leak into V1" into a question the test can actually answer, and makes widening it a deliberate act. test_contract_v1_emits_no_param_modes_and_no_other_new_key is the AC-M7 guarantee, and it checks for ANY new key rather than just this one -- so it catches the next field too, not only this PR's.

THE SUBTLE STATE IS DOCUMENTED AT BOTH ENDS RATHER THAN LEFT TO BE DISCOVERED. A constant occurrence= display keyword is static in param_modes while staying OUT of literal_params -- a literal that is not editable. That reads like a contradiction until you see that the two fields answer different questions, which is exactly what the docstring says and what test_a_literal_that_is_not_editable_is_static_and_stays_out_of_literal_params pins. A consumer that assumed static implied editable would have shipped a bug; now it reds a test instead.

WIRING UP CODE THAT SHIPPED WITH NO CALLER IS THE RIGHT KIND OF CHANGE. _param_mode and its bounded-shape predicates landed in PR 559 with no production caller -- so they were, until now, untested-in-anger machinery carrying a maintenance cost and no benefit. Four emission sites is the payment. test_static_is_exactly_ast_constant_and_nothing_looser pins the predicate so "static" does not quietly widen later, and test_the_exclusion_list_covers_every_shape_e5_names checks the exclusion set against the spec's own list rather than against itself.

The four sites were re-located BY SYMBOL rather than by the line numbers in the brief. That is the correct method and the same one PR 1165 is landing for the residual lint -- line numbers in a record decay silently.

SCOPE IS HELD. Step 1 of 3; nothing in ide/ is touched, and the stepsModel.ts consumer and the mode selector are named as steps 2 and 3 rather than half-built here. Declaring the remainder is what keeps a partial landing from reading as a complete one.

Verdict: merge.

wshallwshall and others added 3 commits September 15, 2026 08:09
The one red leg on this PR was
`tests/test_store.py::test_a_failed_open_closes_the_connection_and_lets_the_process_exit`,
failing with:

    AssertionError: a failed open left live thread(s): Thread-1 (_connection_worker_thread)

That test is not part of this branch's change. It is a known flake that `main`
already fixed in 7c8775e ("test(store): poll for the aiosqlite worker to drain
instead of sampling once", PR #1174): the assertion enumerated threads once
immediately after `asyncio.run()` returned, racing the aiosqlite worker's
shutdown. This branch predated that commit. Verified 7c8775e changes exactly
that assertion, and that it is an ancestor of origin/main.

`git merge-tree --name-only origin/main HEAD` named no conflicting path before
the merge (it printed a tree OID and exited 0), and the merge itself was clean
across 22 commits and 78 files. This branch's own contribution is unchanged and
still confined to `messagefoundry/lens.py` and `tests/test_lens_param_modes.py`.

Carries the per-argument `param_modes` emit work for BACKLOG #237. No source
change in this commit.
The remote already carried a merge of `main` at dfb7dcc, made before
7c8775e landed -- which is why the flake was still red there. This joins
that history with the newer origin/main merge in the previous commit, so
nothing the remote had is dropped and no force-push is needed.

Content-free by measurement: `git merge-tree --name-only HEAD a3bdab4`
produced tree e2dc6dd, byte-identical to
the tree the previous merge already produced.
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Repair: the red leg was a known flake that main has already fixed.

The failing assertion was

FAILED tests/test_store.py::test_a_failed_open_closes_the_connection_and_lets_the_process_exit
  AssertionError: a failed open left live thread(s): Thread-1 (_connection_worker_thread)

That test belongs to no part of this PR's change. It was sampling thread state once,
immediately after asyncio.run() returned, which races the aiosqlite worker's shutdown.
7c8775e63 ("test(store): poll for the aiosqlite worker to drain instead of sampling once",
PR #1174) replaced that single sample with a poll to a 5s deadline. Verified: that commit
changes exactly this assertion, and it is an ancestor of origin/main.

This branch predated it, so the merge picks it up.

What landed here, in two commits:

  1. Merge origin/main to pick up the test_store flake fix -- brings in 22 commits across
    78 files. git merge-tree --name-only origin/main HEAD named no conflicting path
    beforehand, and the merge itself was clean.
  2. Merge the remote branch tip, which had merged an older main -- the remote already
    carried a merge of main at dfb7dcc8f, made before 7c8775e63 landed, which is why
    the flake was still red there. Joining it keeps that history and avoids a force-push.
    It is content-free by measurement: git merge-tree --name-only HEAD a3bdab45c produced
    tree e2dc6dddeb909163b1c1bf57fe93bee5ef5d9774, byte-identical to the tree the first
    merge already produced.

This PR's own contribution is unchanged and still confined to messagefoundry/lens.py
and tests/test_lens_param_modes.py. No file under ide/ is touched.

Checks run locally on the merged tree (cf6a5f890):

  • pytest tests/test_store.py -k failed_open -- 1 passed, 57 deselected
  • pytest tests/test_lens_param_modes.py tests/test_lens_rewrite.py -- 137 passed, 3 skipped
  • pytest tests/test_lens_param_modes.py -n 4 -- 91 passed
  • all 13 test_lens_*.py files plus test_tooling_partition.py -- 568 passed, 3 skipped
  • ruff check . -- all checks passed; ruff format --check . -- 1303 files already formatted
  • mypy messagefoundry -- no issues in 275 source files

Not run locally: the full suite, and the hosted-runner-only legs (SQL Server, Postgres,
load, windows-service-smoke). Those need to be read from CI.

@wshallwshall
wshallwshall added this pull request to the merge queue Sep 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 16, 2026
@github-actions

Copy link
Copy Markdown

CI failed while this pull request was in the merge queue, so the queue ejected it.

Its own head can still be green: the queue revalidates the merge, and the path gates that skip on a pull request run there. Read the run before retrying.

https://github.com/MEFORORG/MessageFoundry/actions/runs/35113132356

@wshallwshall
wshallwshall added this pull request to the merge queue Sep 16, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Lander: evicted from the merge queue and re-queued at position 4. Not your change, and nothing is needed from you.

What failed

Queue branch pr-1170-834f96a78, leg test (windows-2022, py3.14):

FAILED tests/test_asvs_login_deadline.py::test_every_login_failure_branch_answers_at_one_deadline
AssertionError: deadline depends on the branch taken:
  unknown_username   0.5000016      wrong_password      0.5000026
  locked_account     0.5000025      bootstrap_username  0.5000029
  ad_pathway_retired 1.0000056

Why it is not this PR

Your diff cannot reach that code. Whole-word counts over the whole diff:

login 0   auth 0   deadline 0   sleep_until 0   AuthProvider 0   service 0
lens 5   <- control, so the search works

You touch messagefoundry/lens.py and tests/test_lens_param_modes.py. Nothing else.

And it is not a regression on main either. I ran the test against current main with an interpreter I first confirmed imports from the engine worktree rather than its own tree:

tests/test_asvs_login_deadline.py::test_every_login_failure_branch_answers_at_one_deadline   1 passed
tests/test_asvs_login_deadline.py (whole file)                                              31 passed

So it reproduces neither in isolation nor against its own file. The queue branch is main plus the entries ahead of yours, and this is the third eviction today whose cause had nothing to do with the PR evicted.

The part that is worth someone's attention, and is not yours

I nearly attributed this to PR 1179, which I reviewed and merged earlier today and which does change _complete_ad_login. The failing branch is named ad_pathway_retired, so that was the obvious suspect. Running the test settled it: it passes on main, which includes 1179.

What is left is more interesting. The failing value is exactly twice the others, which looks like a budget-extension path rather than jitter — and this test is explicitly built to be immune to load. Its own docstring:

Recording the deadline rather than the elapsed is what keeps these tests off the wall clock. The deadline is the value the control actually computes; a measured elapsed would only be that value plus noise.

If a loaded runner can still push one branch into a second quantum, that stated design property does not hold, and the guard for an anti-enumeration timing property (ASVS 6.3.8) is itself timing-sensitive. I have handed that to a Manager as a Builder brief rather than filing a number I have not allocated.

Stated no larger than it is

One occurrence. I did not measure a rate, and "load-sensitive" is the reading the 2x value and the local passes together support, not something I reproduced under load. If it fires again, that is a second data point about the deadline test, not about your PR.

Your checks are green on cf6a5f890 and your review verdict stands, which is why this went straight back to the queue rather than back to you.

Merged via the queue into main with commit 72811ba Sep 16, 2026
41 checks passed
@wshallwshall
wshallwshall deleted the claude/builder-237-step1-param-modes branch September 16, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-red A required check went red. Attribute it before retrying.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant