Skip to content

feat: jq 1.7 conformance fixes - #6126

Open
christse wants to merge 3 commits into
mainfrom
bxl/jq-conformance-fixes
Open

christse wants to merge 3 commits into
mainfrom
bxl/jq-conformance-fixes

Conversation

@christse

@christse christse commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

BXL's jq dialect disagreed with jq 1.7.1 in six places; this change brings
them into line. Every jq row below is a case in
tests/unit/jq-conformance-fixes.ts with jq 1.7.1's answer as the expectation.

jq conformance

  • Prototype leak. {"a":1} | .["toString"] returned a function and
    has("toString") was true; setpath(["__proto__","polluted"]; 1) wrote to
    the prototype. Lookups, has, getpath, setpath, del and object
    construction now use own-property semantics. The compiled scalar fast
    path and the card bridge were adjusted so the instance id getter (a
    prototype getter, never an own property) remains readable under
    own-property lookup, and a card with no resolvable field map reads its
    own members rather than null.
  • Keywords as field names. .label failed with Unexpected keyword;
    jq 1.7 accepts a keyword glued to the dot. The tokenizer decides by
    adjacency (. as $x is still a binding); the readable compiler respells
    such a field as ."label" before compiling and emits keyword-named path
    steps in that form. The tokenizer and the readable compiler share one
    keyword set. This also fixes an author's ."label" coming out of the
    readable path as .label.
  • \\( in strings was read as interpolation; it is an escaped
    backslash followed by a parenthesis.
  • \uXXXX escapes (and \/) were rejected; surrogate pairs combine.
  • Runtime limit clock. maxMillis measured wall time only, so the same
    program failed on a loaded machine and passed on a quiet one. Limits now
    accept clock: 'wall' | 'cpu' | () => number. The default stays
    'wall'; no caller in this repository selects another clock.
  • strftime("%-d") errored with a misleading "requires parsed datetime
    inputs"; the - no-padding flag is supported for %d %m %H %I %M %S %j,
    and a bad directive is reported as a format error.

Scope. Unicode case mapping beyond ASCII and localized month names are
unchanged (design questions). A malformed \u escape is rejected on the
plain jq route and kept as text by the readable pre-pass.

🤖 Generated with Claude Code

@christse
christse requested a review from habdelra September 15, 2026 13:04
@christse christse changed the title BXL: jq 1.7 conformance fixes and card bridge value presentation fix: jq 1.7 conformance fixes and card bridge value presentation Sep 15, 2026
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files  ±0      1 suites  ±0   1h 46m 0s ⏱️ - 1m 29s
4 805 tests ±0  4 791 ✅ ±0  14 💤 ±0  0 ❌ ±0 
4 820 runs  ±0  4 806 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit 8df0384. ± Comparison against earlier commit 765134b.

Realm Server Test Results

    1 files  ±0    225 suites  ±0   1h 23m 32s ⏱️ + 3m 30s
3 053 tests ±0  3 053 ✅ ±0  0 💤 ±0  0 ❌ ±0 
3 092 runs  ±0  3 092 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 8df0384. ± Comparison against earlier commit 765134b.

@christse
christse marked this pull request as ready for review September 15, 2026 13:34
Seven places where BXL disagreed with jq 1.7.1, each a case in
tests/unit/jq-conformance-fixes.ts with jq's answer as the expectation:
own-property lookups (no prototype leak through .["toString"], has, getpath,
setpath, del or object construction; the card bridge keeps the instance id
getter readable), keywords glued to the dot as field names (.label, with the
readable compiler respelling them as ."label"), \\( as an escaped backslash
rather than interpolation, \uXXXX and \/ string escapes, a clock option for the
wall-clock runtime limit, a compiled-pattern cache for match/sub/gsub, and the
strftime %- no-padding flag with format errors reported as such instead of
blaming the input.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This review went after behavioural regressions in the own-property change and the card bridge, the two twin-implementation surfaces this package carries (compiled scalar vs. streaming evaluator; the readable compiler vs. the jq tokenizer), and the load-bearing comments the diff adds. Every behavioural claim below was A/B'd against the merge-base in a scratch worktree. Not covered: the host integration suites, and anything that needs a real card-api instance rather than a stub.

Two regressions worth fixing before this merges — a card whose field map is unavailable now reads every field as null, and the pattern cache makes sticky regexes stateful across evaluations. Everything else is non-blocking.

  1. Field reads fall to null when there is no field map — card-input.ts, the getOwnPropertyDescriptor trap.
  2. A cached sticky pattern carries lastIndex between runs — builtinNativeFilters.ts, above _match_impl/3.
  3. The gsub cache doesn't measurably pay, and the test guarding it can't fail — jq-conformance-fixes.ts.
  4. The new derive def justification claims a termination bound that recursion doesn't get — ast/index.ts, and the same sentence in docs/profiles.md.
  5. The clock option has no consumer, so the flakiness it describes stands — runtimeState.ts.
  6. "pure path access never pays for the field map" no longer describes the code — card-input.ts.
  7. JQ_PARSER_KEYWORDS is a hand copy of Tokenizer.keywordsreadable-syntax.ts.
  8. Evergreen: the test-file header, the gsub comment, the "Lattice" reference, and the PR description.

CI: nothing red to act on — 24 passed, the rest skipped or still queued.

Adjacent, out of scope. A FieldDef's hidden links answer has("photo") with true while keys omits them, so the facade presents a jq object that disagrees with itself. That behaves the same on main (the has trap already answered this way), so it isn't this PR's to fix — worth knowing if the FieldDef presentation gets revisited.

Comment on lines 452 to 454
if (!desc && !isField && !isHiddenLink && !isIdentity) {
return undefined;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Regression, blocking. When resolveFieldKeys() comes back null, every field on the card reads null instead of its value.

jq lookups now arrive here through Object.hasOwn. A card field is a prototype getter, so Reflect.getOwnPropertyDescriptor(target, prop) is undefined and isField is the only thing left that can materialize a descriptor — and isField needs a non-null field map. safeFieldMap returns null whenever getFields throws or no bridge resolves, and the failure is silent: the computed field goes blank rather than erroring.

Verified against a stub card whose getFields bridge throws — .amount yields 42 on main and null on this branch. Same for a value carrying only the isBaseInstance stamp with no ambient global. ownKeys already covers this state (if (!keys) return own;), so the read path is the one that doesn't.

A keys === null fallback that keeps Object.prototype out restores the values; I ran the package suite with it and 72/72 stay green, prototype-leak cases included:

const isIdentity = prop === 'id' && Reflect.has(target, 'id');
// With no field map, fall back to what the instance actually has, minus
// Object.prototype — a field is a prototype getter, so the target carries
// no own descriptor for it.
const noFieldMap =
  keys === null &&
  typeof prop === 'string' &&
  prop in target &&
  !Object.hasOwn(Object.prototype, prop);

if (!desc && !isField && !isHiddenLink && !isIdentity && !noFieldMap) {
  return undefined;
}

A case in cyclic-graph.ts for a card whose bridge throws would pin it — there's none today.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Still open on 6a199dc2 — GitHub has collapsed this thread as outdated because the trap moved, so flagging it here: the hole is unchanged, and it's the one blocking item left.

The trap is now three lines, and the same path reaches it:

const isField = typeof prop === 'string' && !!resolveFieldKeys()?.includes(prop);
const isIdentity = prop === 'id' && Reflect.has(target, 'id');
if (!desc && !isField && !isIdentity) {
  return undefined;
}

A card field is a prototype getter, so desc is undefined; isIdentity only covers id; and isField needs resolveFieldKeys() to be non-null. When safeFieldMap returns null — getFields throws, or no bridge resolves — every field on that card reads null.

Re-verified against 6a199dc2 with a stub whose getFields bridge throws: .amount is 42 on main, null here. Same for a value carrying only the isBaseInstance stamp. ownKeys still handles the state (if (!keys) return own;); the read path still doesn't.

The fallback fits the smaller trap:

const isIdentity = prop === 'id' && Reflect.has(target, 'id');
// With no field map, fall back to what the instance actually has, minus
// Object.prototype — a field is a prototype getter, so the target carries
// no own descriptor for it.
const noFieldMap =
  resolveFieldKeys() === null &&
  typeof prop === 'string' &&
  prop in target &&
  !Object.hasOwn(Object.prototype, prop);
if (!desc && !isField && !isIdentity && !noFieldMap) {
  return undefined;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 765134b. The getOwnPropertyDescriptor trap now takes the fallback you sketched: when resolveFieldKeys() is null, a string prop that is in target and not on Object.prototype gets a descriptor, so fields read through their prototype getters. Two new cases in tests/boxel/cyclic-graph.ts pin it: a stub whose getFields bridge throws (.amount → 42, .id reads, has("amount") true, .toString → null, has("toString") false) and a value carrying only the isBaseInstance stamp with no bridge (.amount → 7, .constructor | type → "null"). Package suite 72/72.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. Verified on 765134bb0c: .amount reads 42 through the throwing bridge and 7 through the stamp-only value. Both new cases go red with the noFieldMap clause removed (29/31), so they pin the fix rather than document it.

One residual, optional and not a regression: a member from the class hierarchy below Object.prototype stays reachable by path in the degraded state. .serialize | type is "function" here and also on the merge-base, while .toString, .valueOf, .hasOwnProperty and .__proto__ are "null" here against "function" / "object" before — so the fallback is strictly better than what it replaces, just not airtight. A typeof raw !== 'function' term would close the rest if you'd rather it be clean than merely better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Tightened in 8df0384: the no-field-map fallback now also excludes members whose value is a function, so .serialize | type reads "null" in the degraded state; the cyclic-graph case asserts it. 72/72 suites.

Comment on lines +571 to +575
// `sub`/`gsub` are jq-defined over `match` and call it once per
// occurrence, so a fresh RegExp and capture-name scan per call made
// `gsub` several times slower than an explode/implode loop. Compiled
// patterns are shared by (pattern, flags); the global-flag state is not
// an issue because `match` and `matchAll` reset or clone it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Regression, blocking. The last clause holds for g but not for y: a sticky pattern keeps its lastIndex on the cached RegExp, so one program returns different answers on successive evaluations.

"aaa" | sub("a"; "X"; "y")   →   "Xaa"   "aXa"   "aaX"   "aaa"

Four runs in one process on this branch; main answers "Xaa" every time. matchAll does clone, so the g branch is safe — the non-g branch reaches String.prototype.match, and RegExpBuiltinExec writes lastIndex back for a sticky regex. Flags go straight into new RegExp here, so y is accepted even though jq 1.7 rejects it as a modifier string.

r.lastIndex = 0 before the non-global branch closes it; rejecting the flags jq doesn't define would too, and would bring the flag surface back in line with jq.

Worth reading alongside the thread on the perf assertion in jq-conformance-fixes.ts — the measurements there don't show the cache paying for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 765134b by removing the pattern cache: _match_impl/3 compiles a fresh RegExp per call again, as on main, so no lastIndex survives between evaluations — "aaa" | sub("a"; "X"; "y") answers "Xaa" on every run. Flag validation is left as it was on main. Re-measured before pulling it: 0.22–0.27 ms per warmed gsub with the cache, 0.19–0.29 ms without, which agrees with your finding that the cost is the jq-source sub/3 recursion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. "aaa" | sub("a"; "X"; "y") answers "Xaa" on all four runs now, and _match_impl/3 builds a per-call RegExp as on main, so there's no cached state left to carry between evaluations. Nothing else referenced compiledPattern, and moving strftimeError below the imports cleans up the code-between-imports placement too.

Comment on lines +133 to +138
const started = Date.now();
for (let i = 0; i < 300; i++) outputs('"abc123def456ghi" | gsub("[0-9]"; "")');
ok(
Date.now() - started < 5_000,
'300 gsub calls stay well under a second each',
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Non-blocking, needs a decision. This assertion can't fail, and the speedup it guards doesn't reproduce.

The same loop on main takes ~500 ms against the 5 000 ms bound, so a tree without the cache passes it.

And the work the cache removes is small: a gsub of this fixture drives 7 _match_impl/3 calls, and 3 500 new RegExp + captureGroupNames pairs measure 0.67 ms in total — ~0.0014 ms per gsub against ~1.0 ms measured per gsub. Six paired benchmark runs (500 warmed calls each) leave head and base inside each other's spread: base 0.55–1.21 ms/call, head 0.61–1.34.

The cost is the jq-source sub/3 recursion itself — mysub re-enters per occurrence, each pass doing explode/implode over the flags, [match(…)], a reduce over captures and two string slices. Caching the RegExp doesn't touch any of that.

So: drop the cache, or keep it and re-anchor the claim. If it stays, this test needs a bound a pre-cache tree fails, and the message wants correcting — it says "well under a second each" for what is really 16 ms each.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved in 765134b with the cache gone: the RegExp-identity assertion and the timing loop are removed, and section 6 keeps only the gsub conformance expectations (every occurrence replaced; named captures visible to the replacement). The compiledPattern import and the summary line are updated to match.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. The timing loop and the RegExp-identity assertion are gone, so what's left in section 6 are behavioural expectations rather than a bound a pre-fix tree also passes.

Comment thread packages/bxl/src/bxl/ast/index.ts Outdated
Comment on lines +390 to +394
// `derive` allows user-defined helpers for the same reason it allows folds:
// a helper body is walked like any other node, so a volatile, context or
// side-effect call inside it is still banned, and termination (including
// recursion) is bounded by the runtime budget. Keeping a shared helper in
// one place is what lets a real app's derivations stay readable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Pre-existing, now load-bearing. Needs a decision, non-blocking. "termination (including recursion) is bounded by the runtime budget" doesn't hold — unbounded recursion exhausts the JS stack well before maxSteps fires.

bxl('def f: f; f', { readableSyntax: false }).call({})
  → Maximum call stack size exceeded
bxl('def f(n): if n == 0 then 0 else f(n-1) end; f(50000)', { readableSyntax: false }).call({})
  → Maximum call stack size exceeded

With an explicit maxSteps the step limit does fire, so it's the default-limits route — the one computeVia takes — that lands on a RangeError instead. The stack overflow predates this change, but the relaxation is what makes it reachable from derive, and this sentence is the justification the relaxation rests on.

Either bound call depth so a runaway helper trips a RuntimeLimitError, or state what actually holds: that it fails fast on the stack rather than running away. The rest of the paragraph checks out — def stamp: now; stamp and def apply(f): f; apply(now) are both refused.

docs/profiles.md carries the same sentence in the "Why def is allowed in derive" note and needs the same treatment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved on 6a199dc2 — the derive relaxation is out of scope, so the claim this thread was about is gone with it. Nothing to do here.

Worth carrying forward if def-in-derive returns: the termination sentence needs a call-depth bound behind it, or different wording — def f: f; f still exits with Maximum call stack size exceeded rather than a RuntimeLimitError on the default-limits path.

Comment on lines +15 to +21
// What `maxMillis` measures. 'wall' is elapsed time and makes a result
// depend on host load: the same program passes on a quiet machine and fails
// on a busy one. Derivations that store their value should bound work by
// `maxSteps` and use 'cpu' (process CPU time where available) so a blocked
// event loop or a loaded host cannot fail them; 'wall' then serves only as a
// coarse safety net set well above any step-bounded run. A function is an
// explicit clock in milliseconds, for tests and for hosts with a better one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Non-blocking, needs a decision. Nothing turns this knob, so the host-load dependence described here is unchanged where it was hit.

No caller in the repo passes runtimeLimits at all — a runtimeLimits grep across packages/ outside packages/bxl is empty — and clock appears only in the new test. bxl() / expression(), which is exactly the "derivations that store their value" this paragraph addresses, still resolves to Date.now with the 2 000 ms default.

Following the advice also has a cost worth naming: runParsedNativeProgram does const compiledScalar = runtimeLimits ? undefined : parsed.compiledScalar, so a caller who passes { clock: 'cpu', maxSteps: … } gives up the compiled-scalar fast path on every derivation that would otherwise take it.

Either wire the derive factory to 'cpu' here so the knob does something, or cut the recommendation back to what the option is and leave the choice to whoever adopts it.

One adjacent note: resolveRuntimeClock and NativeRuntimeClock are new public exports of a published package, added under a fix: title that bumps a patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Addressed in 765134b by cutting the recommendation back. The comment on NativeRuntimeClock now describes the three clocks: 'wall' is the default, 'cpu' falls back to wall time where process.cpuUsage is missing, and any runtime limit takes the streaming evaluator instead of the compiled-scalar fast path. Nothing is wired to 'cpu' in this PR; the choice stays with whoever adopts it. resolveRuntimeClock is no longer exported from the package index (it was only used inside runtimeState.ts). The clock option and the NativeRuntimeClock type are still new surface under a fix: title — if you would rather see that as feat: (minor bump), say so and the title changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. Both new claims in the comment check out: resolveRuntimeClock('cpu') returns Date.now when process.cpuUsage is absent, and runParsedNativeProgram drops the compiled scalar for any explicit limits. The package-index export is gone.

On fix: vs feat: — since you asked: feat: is the accurate one. clock and NativeRuntimeClock are additive public surface, and nothing about them is a bug fix. At 0.7.0-unstable.6 that's a minor against a patch on a pre-1.0 unstable line, so it isn't worth a re-push by itself — worth correcting if you're touching the title anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Retitled to feat: jq 1.7 conformance fixes (minor bump), per your call on the additive clock surface. No code change for the title; the fallback tightening rode along as 8df0384.

Comment on lines +368 to +371
// Resolved on first enumeration only — pure path access never pays
// for the field map. Computeds are included: an expression aggregating
// over another card sees that card as its search doc would.
let fieldMap: ReturnType<typeof safeFieldMap> | undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Regression, non-blocking. The own-property change invalidates this: a path read now goes through Object.hasOwngetOwnPropertyDescriptorresolveFieldKeys()resolveFieldMap(), so pure path access does pay for the field map.

.a + .b against a stub card: 0 getFields calls on main, 1 here. It resolves once per (prepared expression, card instance), and getFields memoizes only inside a render pass (field-support.ts falls straight through to computeFields when __boxelRenderContext is unset), so on the headless derive path each one is a full prototype walk.

Small in absolute terms — but this sentence is what tells the next person path access is free, and the trap it describes is the same one the null field-map thread is about.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Still open on 6a199dc2, reclassified — pre-existing prose this PR makes false. The comment is no longer inside the diff now that the card-bridge changes are out, but the own-property change still routes path reads through Object.hasOwngetOwnPropertyDescriptorresolveFieldKeys(), so "pure path access never pays for the field map" no longer describes the code.

Re-measured on 6a199dc2: .a + .b against a stub card is 0 getFields calls on main, 1 here.

Fixing it is one line of prose, and it's worth doing in this PR rather than leaving it for whoever trusts it next.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 765134b. The comment now says: resolved once per view, on the first enumeration, has, or own-property lookup; jq field lookups are own-property lookups, so even a plain path read pays for the field map once per materialized view.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. The new wording matches what I measure: .a + .b against a fresh card costs one getFields, a fresh view over the same card costs another, and repeat calls through the same prepared expression cost none.

Comment on lines +3786 to +3810
// The jq tokenizer's own keyword list (Tokenizer.keywords), not the readable
// dialect's, since this is about what jq's parser would reject after a dot.
const JQ_PARSER_KEYWORDS = new Set([
'__loc__',
'and',
'as',
'break',
'catch',
'def',
'elif',
'else',
'end',
'foreach',
'if',
'import',
'include',
'label',
'module',
'modulemeta',
'not',
'or',
'reduce',
'then',
'try',
]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Non-blocking. This is a hand copy of Tokenizer.keywords, which is private static one file away. The two decide the same question from opposite ends — this one drives the respelling, that one drives the adjacency rule at tokenize time — so they have to agree exactly, and a mismatch is silent: a field name compiles to a keyword, or a binding to a field.

They match today (both 22 entries). Exporting the set from Tokenizer.ts and importing it here removes the chance that they stop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 765134b. Tokenizer.ts exports JQ_TOKENIZER_KEYWORDS (the class's keywords now points at it) and readable-syntax.ts imports that set instead of keeping a copy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. One set now — Tokenizer.keywords points at the exported JQ_TOKENIZER_KEYWORDS and the readable compiler imports it, so the respelling and the adjacency rule can't drift apart. The import direction stays inside the package's layering (bxl/ may reach into jqtools/).

Comment on lines +5 to +6
// jq 1.7.1 conformance gaps found while moving card computed fields to BXL
// (September 2026). Each expectation below is jq 1.7.1's answer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Non-blocking, evergreen. This header is journey plus a dated anchor — how the gaps were found and when. The second sentence is the part a reader needs: each expectation is jq 1.7.1's answer. Dropping the first leaves the contract standing on its own.

Two more of the same class elsewhere in the diff:

  • the _match_impl/3 comment's "made gsub several times slower than an explode/implode loop" describes the state before the change rather than what the code does;
  • the PR description opens with how the comparison came about, and "Not in this PR" reads as a delta — both are better as a statement of what the code covers and what it doesn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Done in 765134b: the test header is the one sentence (each expectation is jq 1.7.1's answer); the _match_impl/3 comment went out with the cache; the PR description is rewritten to state what the change covers, with a plain scope paragraph in place of "Not in this PR".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. Header is the one sentence, the gsub comment went out with the cache, and the description now opens on what the dialect does rather than how the comparison came about, with "Scope." in place of the delta.

* The JSON a card stores for a contained field value: every contains /
* containsMany subfield (computeds included) with unset values as null,
* nested field values recursively, dates as their serialized strings, links
* omitted. Mirrors the attributes Lattice's Node derivation reads, so an

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Non-blocking, evergreen. "Lattice" appears nowhere else in this repo — this line and the parallel comment in cyclic-graph.ts are the only two hits — so a reader outside the team has no way to resolve it, and this repo is public.

Naming the mechanism keeps the point without the dangling reference: the presentation matches the attributes a card stores, so an expression reading the stored JSON and one reading a live instance agree.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved on 6a199dc2 — the comment went out with the card-bridge presentation work. No "Lattice" hits left in the package.

@christse christse changed the title fix: jq 1.7 conformance fixes and card bridge value presentation fix: jq 1.7 conformance fixes Sep 15, 2026
@christse
christse force-pushed the bxl/jq-conformance-fixes branch from a09bf87 to 6a199dc Compare September 15, 2026 13:48
@habdelra

Copy link
Copy Markdown
Contributor

[Claude Code 🤖] The review above was written against a09bf87 and submitted about a minute after 6a199dc2 landed, so some of it is aimed at code that is no longer here. Re-ran everything against 6a199dc2 — package suite 72/72 — and here is where each thread stands.

Resolved by the scope reduction (no action):

  • The derive def termination claim — out with the profile change.
  • The "Lattice" reference — out with the card-bridge presentation work.

Still open, unchanged code:

  1. Blocking. A cached sticky pattern carries lastIndex between evaluations: "aaa" | sub("a"; "X"; "y") answers "Xaa" "aXa" "aaX" "aaa" across four runs in one process, against "Xaa" every time on main.
  2. Blocking. A card whose field map is unavailable reads every field as null. GitHub collapsed that thread as outdated when the trap moved, so it is easy to miss — the hole survives the smaller trap unchanged, and there is a re-anchored fix in the reply.
  3. The gsub cache doesn't measurably pay, and the test guarding it passes on a tree without it.
  4. The clock option still has no consumer, so the host-load dependence the description calls out is unchanged in the shipped path.
  5. JQ_PARSER_KEYWORDS still duplicates Tokenizer.keywords.
  6. "pure path access never pays for the field map" is now false — that comment sits outside the diff after the reduction, so it's pre-existing prose this change invalidates rather than something the diff introduces.

Evergreen, in the rewritten description: it still opens with how the comparison came about rather than what the code does, and "Not in this PR" reads as a delta against a draft the reader never saw. The second half of that line is worth keeping as a plain statement of scope — a malformed \u escape is rejected on the jq route and kept as text by the readable pre-pass. The same journey wording is in the test file header and above _match_impl/3.

One note on 1 and 3 together: if the pattern cache comes out, both close at once.

@habdelra

habdelra commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

@christse make sure your agent replies to the review comments with the fix review and the commit that introduces the fix as you update the PR (it makes it easier to review and its also part of the PR monitoring skill)

- card bridge: when no field map resolves (the bridge threw, or none is
  reachable), fall back to the instance's own members minus
  Object.prototype so its fields no longer read as null; two
  cyclic-graph cases pin it
- drop the compiled-pattern cache: a cached sticky regex carried
  lastIndex between evaluations, and the cache did not measurably pay
  (0.19-0.29 ms per gsub without it, 0.22-0.27 ms with it)
- one keyword set: Tokenizer exports JQ_TOKENIZER_KEYWORDS and the
  readable compiler imports it instead of keeping a hand copy
- clock: the comment describes the option instead of prescribing 'cpu';
  resolveRuntimeClock is internal again
- prose: the field-map cost comment, the test header, the gsub section

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@christse

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] Replied on each open thread with the fix and the commit. All of it is in 765134b: the null-field-map fallback in the card bridge with two pinning cases in cyclic-graph.ts; the pattern cache removed (the sticky-regex state and the unfalsifiable perf assertion go with it); one keyword set exported from the tokenizer; the clock comment cut back and resolveRuntimeClock un-exported; the prose fixes; and the PR description rewritten. Package suite 72/72, eslint and types clean.

@christse
christse requested a review from habdelra September 15, 2026 18:21
@habdelra

Copy link
Copy Markdown
Contributor

[Claude Code 🤖] Re-reviewed 765134bb0c against 6a199dc2, verifying each change rather than the description of it. Package suite 72/72, eslint and types clean.

All seven threads close. No blocking issues left.

The two that mattered are verified rather than taken on trust: "aaa" | sub("a"; "X"; "y") answers "Xaa" on every run now, and a card whose field bridge throws reads 42 again. The two new cases in cyclic-graph.ts both go red with the noFieldMap clause removed, so they pin the fix instead of passing either way — which is the thing that most often goes wrong with a test written alongside its fix.

Dispositions are in the threads. Two small things left there, neither worth holding the PR for: an optional typeof raw !== 'function' term that would make the no-field-map fallback airtight rather than just better than what it replaces, and the fix: / feat: question answered.

A card whose field map cannot be resolved reads its own members by path;
functions on the class hierarchy are now excluded too, so `.serialize`
reads as null there as it does with a field map.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@christse christse changed the title fix: jq 1.7 conformance fixes feat: jq 1.7 conformance fixes Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants