Out-of-scope finding from #6223 / PR #6677. Recorded per Prime Directive #10, unassigned.
The drift
packages/formula/src/parse-cel-to-ast.test.ts carries two comments that describe classifyError's pre-#6202 behaviour as if it were current.
:135-144, inside classifies the common syntax fault as parse``:
NOT asserted for ((record.a): cel-js phrases an unbalanced delimiter as Expected RPAREN, got EOF, which classifyError's /parse|unexpected|syntax/i does not match, so a genuine syntax fault is reported to the author as runtime. Pre-existing, out of scope for #4812, filed separately — asserting it here would enshrine it.
And :116-118:
The parity claim is the accept/reject verdict itself. compile's error classification is asserted separately below — cel-js does not phrase every syntax fault the same way, and classifyError reads the phrasing.
Both were accurate when written. Neither is now:
The /parse|unexpected|syntax/i regex quoted in the first comment no longer exists in the codebase.
Why it is worth a ticket rather than nothing
The comment does not merely go stale, it instructs: it tells the next author that asserting ((record.a) -> parse here "would enshrine it", i.e. that the correct verdict is a bug not to be pinned. That is the opposite of the current contract. The second comment gives a live reason ("classifyError reads the phrasing") for a test-design decision whose premise is gone, so a reader deciding whether to tighten that parity test is reasoning from a false fact about the classifier.
Class
Observation-class: comments only, no runtime behaviour, nothing a user hits. Filed plainly and left for triage to grade — the fix is a comment rewrite in one file, and possibly promoting the ((record.a) case from "deliberately not asserted" to an ordinary assertion now that the verdict is stable.
Related
#6133 / PR #6202 (the parse arm). #6223 / PR #6677 (the type / runtime arms, where this was noticed). #4812 (the issue the file was written for).
Out-of-scope finding from #6223 / PR #6677. Recorded per Prime Directive #10, unassigned.
The drift
packages/formula/src/parse-cel-to-ast.test.tscarries two comments that describeclassifyError's pre-#6202 behaviour as if it were current.:135-144, insideclassifies the common syntax fault asparse``:And
:116-118:Both were accurate when written. Neither is now:
ParseErrorarm structural, so((record.a)has been gradedparsesince it merged —cel-error-classification.test.tspins exactly that fixture.classifyErrordoes not read any phrasing.The
/parse|unexpected|syntax/iregex quoted in the first comment no longer exists in the codebase.Why it is worth a ticket rather than nothing
The comment does not merely go stale, it instructs: it tells the next author that asserting
((record.a) -> parsehere "would enshrine it", i.e. that the correct verdict is a bug not to be pinned. That is the opposite of the current contract. The second comment gives a live reason ("classifyErrorreads the phrasing") for a test-design decision whose premise is gone, so a reader deciding whether to tighten that parity test is reasoning from a false fact about the classifier.Class
Observation-class: comments only, no runtime behaviour, nothing a user hits. Filed plainly and left for triage to grade — the fix is a comment rewrite in one file, and possibly promoting the
((record.a)case from "deliberately not asserted" to an ordinary assertion now that the verdict is stable.Related
#6133 / PR #6202 (the parse arm). #6223 / PR #6677 (the type / runtime arms, where this was noticed). #4812 (the issue the file was written for).