feat(lens): emit per-argument param_modes at every typed-row site (BACKLOG #237) - #1170
Conversation
…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.
|
LANDER review. Read against head 05e32c5. Merge. THE COMPATIBILITY DECISION IS THE WHOLE REVIEW AND IT IS RIGHT. THE BEST THING IN THIS PR IS A TEST THAT EXISTS BECAUSE THE AUTHOR CAUGHT THEIR OWN SUITE BEING VACUOUS.
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
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. THE SUBTLE STATE IS DOCUMENTED AT BOTH ENDS RATHER THAN LEFT TO BE DISCOVERED. A constant WIRING UP CODE THAT SHIPPED WITH NO CALLER IS THE RIGHT KIND OF CHANGE. 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 Verdict: merge. |
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.
|
Repair: the red leg was a known flake that The failing assertion was That test belongs to no part of this PR's change. It was sampling thread state once, This branch predated it, so the merge picks it up. What landed here, in two commits:
This PR's own contribution is unchanged and still confined to Checks run locally on the merged tree (
Not run locally: the full suite, and the hosted-runner-only legs (SQL Server, Postgres, |
|
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 |
|
Lander: evicted from the merge queue and re-queued at position 4. Not your change, and nothing is needed from you. What failedQueue branch Why it is not this PRYour diff cannot reach that code. Whole-word counts over the whole diff: You touch 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: 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 yoursI nearly attributed this to PR 1179, which I reviewed and merged earlier today and which does change 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:
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 isOne 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 |
BACKLOG #237, step 1 of 3 (AC-M1, AC-M2, AC-M7). ADR 0076 Amendment E.
_param_modeand its bounded-shape predicates shipped in PR 559 with no production caller. Thisgives it four. Nothing in
ide/is touched: thestepsModel.tsconsumer and the mode selector aresteps 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.
_native_action_rowparam_modesovernative.slotsplusnative.display_classify_simple,_ACTION_PARAMSbranch_param_modes_for_call(call, _ACTION_PARAMS[name])_classify_simple,_DIAGNOSTIC_PARAMSbranch_param_modes_for_call(call, _DIAGNOSTIC_PARAMS[name])_classify_simple,_LOOKUP_PARAMSbranch_param_modes_for_call(call, _LOOKUP_PARAMS[name])E.4 names
actionandlookup. Site 3 is included because adiagnosticrow carries typed paramsand
literal_paramsexactly as those two do, and E.7 scopes modes to "rows that have typedarguments". 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_paramsand the new_param_modes_for_callnow share one name mapping,_rendered_param_nodes. A key cannot exist inparamsand not inparam_modes, because there isonly one place a key is named. That covers the synthetic
*arg1and**kwargskeys and the extrapositionals past a signature.
On the native path
paramsandparam_modesare both built from[*native.slots, *native.display],so the display kwargs get a mode.
literal_paramsis untouched and still covers slots only: aconstant
occurrence=isstaticby shape and absent fromliteral_paramsby design. Wideningliteral_paramswas considered and refused (E.10) -- E.8 depends on an older consumer reading itunchanged.
AC-M7: which version is the pre-Amendment-E one
param_modesridesCONTRACT_V2, soCONTRACT_V1stays 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:
CONTRACT_V3. E.8 puts the FORWARD skew on the consumer -- an older IDE keepsreading
params/literal_paramsand ignores the key it does not know.ide/src/stepsModel.tspins
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.
CONTRACT_V2; the project does not mint one version peramendment.
Pinned two ways in Python, against the existing
contractmechanism: a hardcoded V1 row-keyallowlist across the whole corpus (so any new V1 key fails, not just this one), and a strict
comparison that stripping
param_modesfrom the v2 rows reproduces the v1 rows exactly.End-to-end through the CLI:
lens parse - --jsoncarries no map;--contract 2carries{"path": "static", "value": "static", "occurrence": "static"}.MEASURED: the samples corpus cannot test this
samples/configproduces 28 rows atCONTRACT_V1and 67 atCONTRACT_V2, and not one carriesparams-- it contains no action, diagnostic or lookup row at all. AC-M2's "across the wholesamples 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 onboth 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.
staticwidened to list/tupleThe 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 .-- passedruff format --check .-- passed (after formatting the two changed files)mypy messagefoundry(strict) -- passed, 274 filespytestover the 13tests/test_lens_*.pyfiles -- 557 passed, 3 skipped, serially and under-n 4, identical counts, so nothing diverges per xdist workerpytest tests/test_checks.py tests/test_actions.py tests/test_corepoint_import.py-- 149 passed,1 skipped
Skipped, and why: the full suite, which under fleet contention may never finish. Legs to read after
this process exits: the four
testlegs, and anything that shellslens parse.Ledger banner for the Lander
Proposed text for the vault row. I did not edit
docs/BACKLOG.md.This row was measured wrong in four specific ways at HEAD. Whoever fixes it should correct all
four:
MODE_STATIC,MODE_TEMPLATED,MODE_DYNAMIC,_is_bounded_message_read,_is_bounded_interpolationand_param_modeare all onmaininmessagefoundry/lens.py, withtests/test_lens_param_modes.py(PR 559).
row's own 2026-08-23 amendment already says so; the 2026-08-20 re-score carries the stale sentence
forward.
Open question, for the next brief rather than for a reply
Should
diagnosticrows carryparam_modes? I emitted on all four sites per the brief, with theE.7 reasoning above. AC-M1's text names only
actionandlookup, so a reader checking the ADRliterally 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.