Make payload projection selector errors atomic - #84
Conversation
rickardvh
left a comment
There was a problem hiding this comment.
Blocking review on d4134ca:
-
The error path still constructs the selector catalog.
_selector_validation_error()calls_available_selectors_for_payload(payload)and retains the resulting list before sampling it. That violates the linked issue's requirement that the full inventory never be materialized on the ordinary error route. Because list traversal is capped at ten entries per list,available_countis also not the true total for larger payloads. The response remains unbounded by request size throughrequested_selectors,unknown_selectors, suggestion keys, and selector token lengths. -
Validation still follows projection work. Valid selectors are resolved and stored before a later invalid selector is detected. In the generated Python runtime,
_field_by_path()deep-copies selected values, so a large valid subtree can be copied and discarded on atomic failure. Use a first validation pass that performs existence checks without copying or serializing, followed by projection only after all selectors validate. -
The generic primitive invents host CLI semantics. The docs assign inventory/detail commands to the host, but the implementation synthesizes
--select ... --format jsonand--verbose --format json. Those flags are not guaranteed generic command surfaces. Exact discovery/detail commands should be host-declared data that the primitive passes through unchanged. -
The semver classification is too weak. This unconditionally replaces the documented selected-output shape (
values,missing,available_selectors) with a different kind and field contract. That is not a patch-compatible change unless introduced through an opt-in/versioned contract. -
Generated-target conformance is incomplete. The changed tests cover the in-process executor, not the generated Python and TypeScript runtimes through one canonical contract fixture. There is already a parity difference: Python
str.replace()replaces every matching segment, while TypeScriptString.replace()replaces only the first. Add shared target conformance for atomic failure, hard bounds, host-provided commands, custom kinds, and accurate counts.
Required direction: implement a bounded/streaming inventory summary with exact count, fixed sample and suggestions; bound selector count and token length in the shared parser; prevalidate before projection; move command text into host declarations; and prove identical behavior through generated Python and TypeScript targets.
rickardvh
left a comment
There was a problem hiding this comment.
Re-review of 15fbaf6: still blocked.
The follow-up resolves several earlier findings in the in-process executor: selector existence is checked before projection, host-supplied discovery/detail commands replace synthesized CLI flags, inventory counts are exact, and custom-kind replacement is limited to the first matching segment.
Remaining blockers:
-
Selector limits silently alter the request.
_projection_selectors()materializes all tokens, then keeps only 32 and truncates each to 256 characters. An unknown 33rd selector can disappear and produce success; an overlong selector is validated under a different name. Limit violations must return an explicit bounded request-validation error rather than silently dropping or mutating input. -
Generated Python and TypeScript do not implement the same bounded contract. They still accept unbounded selector counts and lengths, return every requested/unknown selector and suggestion entry, and emit empty discovery/detail commands. Generated Python also still replaces every
/selected-output/segment while the executor and TypeScript replace only the first. The source executor and both generated targets must share one behaviorally identical implementation contract. -
The inventory walker remains inventory-sized in memory for wide payloads.
_selector_inventory_summary()avoids retaining the final recursive catalog, but constructs completeentriescollections for each object/list and pushes all siblings ontopending; TypeScript similarly creates full mapped/reversed arrays. Use iterator-based traversal that retains only traversal depth/state plus the fixed-size sample. -
Cross-target conformance is still missing. The added tests exercise the in-process executor, and
primitive_conformance.pyrepeats that same surface. Add one canonical contract case executed through the executor, generated Python runtime, and generated TypeScript runtime, covering limit rejection, atomicity, exact counts, bounded suggestions, host commands, custom kinds, and output-size bounds. -
Release classification remains incompatible. Changing the established missing-selector result shape unconditionally is a breaking primitive-contract change.
semver:minoris insufficient unless the new behavior is introduced through an opt-in or explicitly versioned contract.
The new head currently has no attached CI/check status, is non-mergeable, and is behind master. Rebase, run the repository-required primitive/public API conformance lanes, and attach exact-head generated-target proof after correcting the remaining contract gaps.
15fbaf6 to
6342cf3
Compare
rickardvh
left a comment
There was a problem hiding this comment.
Re-review of 6342cf3: still blocked on two remaining contract issues.
The previous architectural findings are otherwise resolved: selector-request limit violations now fail explicitly, validation precedes projection, the inventory summary retains only a fixed sample, host discovery/detail commands pass through unchanged, generated Python/TypeScript parity coverage exists, CI is green, and semver:major is correct.
Remaining blockers:
-
The accepted request bounds do not guarantee the upstream 6 KB error-envelope hard cap. The contract accepts up to 32 selectors of 256 characters each. For unknown selectors, the response emits those names in both
requested_selectorsandunknown_selectors, plus suggestion keys and inventory metadata. Maximum accepted input can therefore produce well over 16 KB of selector-name content alone. The new generated-target test asserts only< 12000bytes with short selector/sample strings, so it does not prove the actual selected-output error hard cap from AW #1899. Add a cumulative selector-request byte/character budget chosen to guarantee the envelope cap, and also bound host command/sample path contributions or enforce a final construction-time envelope budget. Prove worst-case emitted UTF-8 JSON remains below 6 KB through the executor and both generated targets. -
Python and TypeScript define selector length differently for Unicode. Python uses
len(token)(Unicode code points), while TypeScript usestoken.length(UTF-16 code units). A selector containing 129 astral characters is accepted by Python as length 129 but rejected by TypeScript as length 258. The parity fixture covers only ASCII. Define the contract in one cross-language unit—preferably UTF-8 bytes or Unicode code points—and implement and test it identically in both targets.
After these two points are corrected, the generator-side contract appears ready for the dependent AW regeneration and host-level budget proof.
rickardvh
left a comment
There was a problem hiding this comment.
Re-review of 86442cb: the previous two blockers are resolved, but one cross-target determinism issue remains.
What is now correct:
- selector limits are defined in UTF-8 bytes in the interpreter and generated TypeScript runtime;
- cumulative selector-name input is capped at 512 bytes;
- the final validation envelope is measured as compact UTF-8 JSON and fitted below 6 KB;
- source command, detail commands, selector samples, and suggestions have explicit budgets;
- generated Python/TypeScript parity tests cover the 6 KB cap, cumulative overflow, and astral Unicode boundaries;
- exact-head CI and the semver-label check pass, the branch is mergeable, and
semver:majorremains correct.
Remaining blocker:
The bounded selector sample is traversal-order dependent, so Python and TypeScript can still emit different validation packets for the same JSON payload.
record_sample() checks the cumulative sample-byte budget before adding a candidate. Once the current traversal has filled that budget, a later lexicographically smaller path is rejected rather than replacing a larger retained path. Python traverses mappings in insertion order, while JavaScript object enumeration moves integer-index keys into numeric order.
A concrete shape is an object whose root keys were inserted as "4", "3", "2", "1", "0", with each value containing one nested selector path close to the 96-byte per-path limit. Python visits branches 4→0; TypeScript visits 0→4. With the 384-byte sample budget, each runtime retains nested paths from different branches. selector_inventory.sample therefore differs, and suggestions can differ with it.
This conflicts with the deterministic bounded-sample contract and with the purpose of the generated-target parity fixture.
Required direction: make sample membership independent of traversal order while retaining O(sample-limit) memory. For example, skip individually overlong paths, maintain only the lexicographically smallest sample_limit candidates during traversal, then apply the cumulative byte budget to that sorted bounded candidate set after traversal. Add one executor/generated-Python/generated-TypeScript fixture using integer-like object keys and near-budget nested paths.
No other blockers found on this head.
rickardvh
left a comment
There was a problem hiding this comment.
Recheck of d1c7ceb: still blocked on one cross-target determinism issue.
The integer-like key traversal case is fixed, exact-head CI and semver checks pass, the PR is mergeable, and it is now ready for review.
Remaining blocker:
The selector sample still uses language-native string ordering. Python calls sample_candidates.sort(), which orders by Unicode code point. TypeScript calls sampleCandidates.sort(), whose default ordering compares UTF-16 code units. Those differ outside the BMP, so the same JSON payload can still yield different bounded samples—and therefore different suggestions and validation packets—between the interpreter/generated Python runtime and generated TypeScript runtime.
A concrete boundary case is U+E000 versus U+10000: Python orders U+E000 first, while JavaScript orders U+10000 first because its leading surrogate is below U+E000. With eight lower ASCII candidates plus these two paths at the sample cutoff, the retained eighth path diverges.
Define and implement one explicit shared comparator, preferably lexicographic UTF-8 byte ordering to match the rest of this contract, and add that exact non-BMP boundary case to the interpreter/Python/TypeScript conformance fixture. Once that passes, the generator-side contract is ready for the dependent AW regeneration and host-level closure proof.
Summary
payload.projectreject unknown selectors atomically instead of returning partialvaluesValidation
uv run pytest tests/test_primitive_executor.py -k payload_project -quv run ruff check src/command_generation/primitive_executor.py src/command_generation/targets/python.py src/command_generation/targets/typescript.py tests/test_primitive_executor.py tests/primitive_conformance.pyuv run python tests/primitive_conformance.pyStacked dependency for Agentic Workspace #2259.