From 4bc90af43cc7bada33b5be1710223f5418cf69d3 Mon Sep 17 00:00:00 2001 From: Filip Klosowski Date: Wed, 26 Aug 2026 16:45:30 +0200 Subject: [PATCH 1/3] feat: introduce UnconventionalTagSyntax check to demote tolerated tag spellings - Added a new check for tag syntax that platformOS parses correctly but does not match documented syntax, allowing these constructs to be used without blocking writes. - Refactored existing checks to separate blocking and non-blocking tag syntax errors, specifically moving three common tag spellings to the new UnconventionalTagSyntax check. - Updated tests to ensure that the new check behaves as expected, allowing tolerated syntax while still blocking genuinely erroneous constructs. - Enhanced documentation and configuration files to include the new check and its settings. --- ...ugh-the-platform-handles-them-correctly.md | 204 ++++++++++++++++++ ...t-so-a-shipped-file-is-a-blocking-error.md | 99 +++++++++ ...ated-tag-syntax-is-not-a-blocking-error.md | 26 +++ .../src/checks/index.ts | 2 + .../checks/InvalidTagSyntax.ts | 9 + .../checks/tolerated-tag-markup.ts | 48 +++++ .../unconventional-tag-syntax/index.spec.ts | 159 ++++++++++++++ .../checks/unconventional-tag-syntax/index.ts | 51 +++++ .../platformos-check-node/configs/all.yml | 3 + .../configs/recommended.yml | 3 + .../src/result/blocking.spec.ts | 10 + .../result/tolerated-tag-syntax-gate.spec.ts | 93 ++++++++ 12 files changed, 707 insertions(+) create mode 100644 .backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md create mode 100644 .backlog/tasks/task-97 - An-angle-bracketed-URI-in-text-is-parsed-as-an-HTML-element-so-a-shipped-file-is-a-blocking-error.md create mode 100644 .changeset/tolerated-tag-syntax-is-not-a-blocking-error.md create mode 100644 packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts create mode 100644 packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts create mode 100644 packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.ts create mode 100644 packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts diff --git a/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md b/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md new file mode 100644 index 00000000..c879dd2c --- /dev/null +++ b/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md @@ -0,0 +1,204 @@ +--- +id: TASK-96 +title: >- + Three tag-markup spellings in real deployed code are blocking errors although + the platform handles them correctly +status: In Progress +assignee: [] +created_date: '2026-08-26 13:49' +updated_date: '2026-08-26 14:41' +labels: + - check-common + - false-block + - measured + - blocking-check + - liquid-html-parser +dependencies: [] +references: + - >- + packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts + - >- + packages/platformos-check-common/src/checks/liquid-html-syntax-error/index.ts + - packages/platformos-mcp-supervisor/src/result/blocking.ts + - packages/platformos-check-node/scripts/generate-factory-configs.js + - supervisor-tests/auto-eval/results/ROUND-2026-08-26/FINDINGS.md + - >- + supervisor-tests/auto-eval/reports/a-stray-colon-makes-a-private-cache-global.md + - >- + supervisor-tests/auto-eval/reports/what-does-the-syntax-annotation-promise.md +documentation: + - supervisor-tests/auto-eval/suites/18-value-collapse.mjs + - supervisor-tests/auto-eval/suites/13-cli-parity.mjs +priority: high +ordinal: 71000 +--- + +## Description + + +## The defect + +`InvalidTagSyntax` fires whenever a known tag's strict grammar rule fails, because the tolerant parser then keeps the markup as a raw **string**. It reports under `LiquidHTMLSyntaxError`, which is `Severity.ERROR` and a member of `BLOCKING_CHECKS`, so the supervisor returns `must_fix_before_write: true` and an agent cannot write the file at all. `pos-cli check` and the language server report the same error. + +Three spellings reached by that path occur in real deployed code and the platform handles each **correctly**. Measured over a 2,768-file production application (`supervisor-tests/auto-eval/substrate-large`): + +| spelling | files | occurrences | +|---|---|---| +| `{% capture 'name' %}` — quoted target | 23 | 43 | +| `{% case x: %}` — trailing colon | 4 | — | +| `{% parse_json o %%}` — trailing `%` | 1 | — | + +Each was rendered on a live instance (`fk-docs.ps-01-platformos.com`) and produces the intended result: + +``` +{% assign g = 1 %}{% case g: %}{% when 1 %}ONE{% endcase %} -> ONE (correct branch) +{% capture 'cs' %}HI{% endcapture %}{{ cs }} -> HI +{% parse_json d o %%}{"k":2}{% endparse_json %}{{ d }} -> {"k":2} +``` + +`capture` with a quoted name is the single most frequently blocked construct found in real code, so this is the largest measured false-block surface in the toolchain. + +## Why these are safe and other spellings are NOT + +The platform's tags do not validate markup against a fixed shape. `Liquify::Tags::BaseTagMethods#parse_main_value` (`base_tag_methods.rb:35`) uses an **unanchored** `markup =~ syntax`, so it takes the first value-shaped token it finds and treats the rest as attributes. For the three spellings above the token it finds happens to be the right one. For others it is not, and the result is silently wrong rather than merely tolerated. Two such cases are measured and MUST keep blocking: + +- `{% cache: k %}` and `{% cache expire: 30 %}` — the key collapses to a constant (`":"`, `"expire:"`), and `cache_tag.rb:54` composes the full cache key with no user or session component. Two distinct keys then share one entry instance-wide, so one user's rendered fragment is served to another. Measured: two blocks with distinct keys rendered one body, and a later request with an unrelated key returned the earlier request's content. +- `{% log: x %}` — records the literal `":"` instead of the author's message. + +This is why a blanket demotion of `InvalidTagSyntax`, or a demotion keyed on the tag name, or a rule such as "tolerate a leading separator", are all wrong: each would unblock a silent defect. `{% capture %}` with empty markup also **raises** on the platform, so demoting the `capture` tag as a whole would approve a fatal error. + +The safe direction is therefore: keep blocking by default, and demote only specific spellings measured to behave correctly. An unmeasured spelling must stay blocking by construction. + +## Why not widen the grammar instead + +These constructs survive `prettier-plugin-liquid` today **because** their markup stays a raw string and the printer emits raw strings verbatim. Making them parse means the printer must learn to print them, or the author's code is silently rewritten on the next format — the data-loss trap in CLAUDE.md's "Changing the grammar". Widening also removes the diagnostic entirely, and these spellings are still worth advising against; they work by accident, not by design. + +## Notes for whoever picks this up + +- `Problem` carries no per-offense severity: severity comes from the check's `meta.severity`. A finding cannot be demoted without moving it to its own check code. +- Adding a check requires registering it in `src/checks/index.ts` **and** regenerating the factory configs (`node packages/platformos-check-node/scripts/generate-factory-configs.js`), or `all.yml` / `recommended.yml` will not list it. +- Blocking is not severity: `blocksWrite` requires `severity: error` **and** membership of `BLOCKING_CHECKS`, so a new check is non-blocking by default. +- If the set of findings the MCP server reports changes, `transport/instructions.ts` must be updated in the same change; its claims are pinned by `validate-code.spec.ts`. +- Regression fixtures for the measured spellings, and the runtime oracles, live in `supervisor-tests/auto-eval/suites/18-value-collapse.mjs` and `suites/13-cli-parity.mjs`. + + +## Acceptance Criteria + +- [x] #1 The three measured spellings (`{% capture 'name' %}`, `{% case x: %}`, `{% parse_json o %%}`) no longer set must_fix_before_write, asserted end to end against the supervisor +- [x] #2 Each of the three still produces a diagnostic, so the construct is advised against rather than silently accepted +- [x] #3 `{% cache: k %}`, `{% cache expire: 30 %}` and `{% log: x %}` still block, asserted in the same test file as the demoted spellings so the split cannot drift apart unnoticed +- [x] #4 `{% capture %}` with empty markup still blocks, since the platform raises on it +- [x] #5 A spelling that reaches the same code path but is not on the measured-safe list still blocks, so the default is fail-safe and a future spelling is not admitted by omission +- [x] #6 Every affected buffer round-trips through prettier unchanged, including the demoted spellings +- [x] #7 Deliberately reverting the change makes the new tests fail (sabotage-verified), recorded in the task notes +- [x] #8 `pos-cli check` over the 2,768-file corpus reports no offense that it did not report before, other than the intended severity change on the three spellings +- [x] #9 If a new check code is introduced it is registered and the factory configs are regenerated, so `all.yml` and `recommended.yml` list it +- [x] #10 Any change to what the MCP server reports is reflected in transport/instructions.ts in this same change + + +## Implementation Plan + + +## Approved plan + +**Branch:** `fix/tolerable-tag-syntax-is-not-a-blocking-error` + +### Shape + +One predicate, two checks, so they are mutually exclusive by construction and cannot both fire or both go silent. + +1. `checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts` — add one exported predicate answering "is this tag markup a spelling measured to behave correctly on the platform?", covering exactly: + - `capture` whose markup is a quoted valid identifier + - `case` whose markup is a value followed by a trailing colon + - `parse_json` whose markup is a value followed by a trailing `%` + `detectInvalidTagSyntax` returns `undefined` for those, so `LiquidHTMLSyntaxError` (Severity.ERROR, in BLOCKING_CHECKS) stays silent on them. + +2. New check definition at `Severity.WARNING`, absent from `BLOCKING_CHECKS`, reporting exactly what the predicate admits. Non-blocking by default per `blocksWrite` — no change to `blocking.ts` needed. + +3. Register in `checks/index.ts` and regenerate factory configs (`node packages/platformos-check-node/scripts/generate-factory-configs.js`) so `all.yml` / `recommended.yml` list it. Add the check's docs page. + +Precedent: the TASK-83 per-shape `ValidFrontmatter` split. + +### Why not the alternatives + +- **Not a grammar widening.** These spellings survive prettier today *because* their markup stays a raw string and the printer emits raw strings verbatim; making them parse means the printer must learn to print them or the author's file is silently rewritten (CLAUDE.md "Changing the grammar", layer 4). Widening also removes the diagnostic, and these work by accident rather than design. +- **Not a blanket or per-tag demotion.** Empty `{% capture %}` raises on the platform, and `{% cache: k %}` / `{% cache expire: 30 %}` / `{% log: x %}` are silently wrong. Default must stay blocking; only measured-safe spellings are demoted. + +### Order of work + +1. Corpus baseline BEFORE any change: `pos-cli check` over the 2,768-file application, offense totals and the `LiquidHTMLSyntaxError` count recorded, so AC #8 is measured rather than asserted. +2. Predicate + its unit tests, including the negative spellings. +3. New check definition + tests; the blocking spellings asserted in the SAME file as the demoted ones so the split cannot drift apart. +4. End-to-end supervisor assertions for `must_fix_before_write`. +5. Prettier round-trip over every affected buffer. +6. Sabotage each direction and record what failed. +7. Corpus diff against the step-1 baseline. +8. Registration + factory configs + docs; `transport/instructions.ts` only if the reported set changes. + + +## Implementation Notes + + +## Implemented on `fix/tolerable-tag-syntax-is-not-a-blocking-error` + +27 insertions across 5 existing files, plus three new files. No grammar or printer change. + +### Shape as built + +- `liquid-html-syntax-error/checks/tolerated-tag-markup.ts` (new) — the shared allowlist predicate, keyed on `(tag name, markup shape)`. +- `InvalidTagSyntax.ts` — returns early when the predicate matches, so `LiquidHTMLSyntaxError` goes silent on exactly that set. +- `unconventional-tag-syntax/` (new) — `Severity.WARNING`, absent from `BLOCKING_CHECKS`, reports exactly what the predicate admits. `blocking.ts` needed no change. +- Registered in `checks/index.ts`; `all.yml` / `recommended.yml` regenerated at severity 1. The generator `require`s the BUILT package, so check-common must be built first — run against source it silently produces no change. + +### The allowlist was corrected by measurement, not reasoning + +Three of the first draft's regexes were wrong. Boundaries measured on the instance: + +| spelling | platform | first draft | final | +|---|---|---|---| +| `case g :` (spaced colon) | renders correctly | blocked | admitted | +| `case g::` (double colon) | renders correctly | blocked | admitted | +| `capture 'a-b'` | captures into `a-b` | blocked | admitted | +| `capture 'cs' extra` | captures into `cs` | blocked | **still blocked** — which token is the target is ambiguous; 0 corpus occurrences | +| `capture '123'` | **inconclusive** — the probe read `{{ 123 }}`, a numeric literal, not a variable read | blocked | **still blocked** — unmeasured, so not admitted | + +### Corpus diff (AC #8), same tree before and after + +``` +total offenses 13065 -> 13065 unchanged +files with offenses 1950 -> 1950 unchanged +LiquidHTMLSyntaxError 122 -> 88 -34 +UnconventionalTagSyntax 0 -> 34 +34 +24 of 26 other check codes unchanged +``` + +-34 matches the enumeration exactly (32 capture + 1 case + 1 parse_json), so the delta is fully attributable. NOTE: a first hand-rolled baseline returned 0 offenses — `pos-cli check run -f json` needs the project dir as an argument and fails silently without it. Use `lib/poscli-check.mjs`. + +### Sabotage (AC #7) — 9 mutations, all bite + +Predicate always TRUE / always FALSE / blocking check stops deferring: 11 tests fail each. The six boundary mutations (capture admits a space, a digit-leading name, a trailing token; case drops the name requirement, admits a trailing token; parse_json drops the name requirement) each fail exactly the one test that pins them. + +An earlier run reported two mutations as "not biting" when the mutation had never applied — a Python raw string searched for `['\"]` while the file holds `['"]`. The harness now asserts the mutation landed before trusting the result. + +### AC #6 nuance, recorded rather than glossed + +No buffer is MANGLED, but none is byte-identical either: prettier reformats Liquid generally (reflow / whitespace-control markers), and it normalises `{% parse_json d %%}` to `{% parse_json d % %}`. Verified inert — both spellings parse to markup `"d %"`. Measured for all nine demoted spellings across prettier 2 and 3: each is still tolerated after formatting, so **a save can never turn a warning into a blocked write**. The post-format spelling is pinned as its own fixture. + +The printer is provably untouched by this change: `prettier-plugin-liquid` depends on `liquid-html-parser` only, never on `platformos-check-common`. + +### AC #10 + +No change needed, verified rather than assumed. `instructions.ts` derives its coverage line from `allChecks.length` and `validate-code.spec.ts` compares against the same derived value, so adding a check moves both together. Nothing in the SILENCES section claims anything about tag syntax, and a non-blocking warning is already covered by the existing prose that `errors[]` being non-empty does not imply a block. + +### Verification + +Full monorepo: 357 test files / 4,535 tests pass; `type-check`, `build` and `format:check` all clean. New tests: 29 in `unconventional-tag-syntax/index.spec.ts`, 25 in `tolerated-tag-syntax-gate.spec.ts` (real lint through the real gate, no mocks), 1 added to `blocking.spec.ts`. + +### FOLLOW-UP — upstream docs page does not exist yet + +`meta.docs.url` points at `documentation.platformos.com/.../checks/unconventional-tag-syntax`. All 53 existing checks carry a URL in this pattern and the pages are authored upstream, so including it follows the repo convention — but this one 404s until someone publishes it, and `see_also` will carry the dead link to agents meanwhile. Either publish the page or drop the field before release. + +### Unrelated finding, recorded so it is not lost + +`RollbackOutsideTransaction` already exists and works correctly: a **page** with a bare `{% rollback %}` is reported; a **partial** deliberately is not, because `RollbackTag` checks `AfterCommitEverywhere.in_transaction?` at runtime and the caller decides. The eval's `S13-FN-rollback-outside-transaction` finding probes a partial, so it measures a designed silence — a fixture artifact of the harness, not a gap in the checks. Belongs to the eval, not to this task. + diff --git a/.backlog/tasks/task-97 - An-angle-bracketed-URI-in-text-is-parsed-as-an-HTML-element-so-a-shipped-file-is-a-blocking-error.md b/.backlog/tasks/task-97 - An-angle-bracketed-URI-in-text-is-parsed-as-an-HTML-element-so-a-shipped-file-is-a-blocking-error.md new file mode 100644 index 00000000..d27eafdd --- /dev/null +++ b/.backlog/tasks/task-97 - An-angle-bracketed-URI-in-text-is-parsed-as-an-HTML-element-so-a-shipped-file-is-a-blocking-error.md @@ -0,0 +1,99 @@ +--- +id: TASK-97 +title: >- + An angle-bracketed URI in text is parsed as an HTML element, so a shipped file + is a blocking error +status: To Do +assignee: [] +created_date: '2026-08-26 13:49' +updated_date: '2026-08-26 13:59' +labels: + - liquid-html-parser + - grammar + - false-block + - measured + - blocking-check +dependencies: [] +references: + - packages/liquid-html-parser/grammar/liquid-html.ohm + - >- + packages/platformos-check-common/src/checks/liquid-html-syntax-error/index.ts + - supervisor-tests/auto-eval/results/ROUND-2026-08-26/FINDINGS.md + - supervisor-tests/auto-eval/suites/13-cli-parity.mjs +priority: medium +ordinal: 72000 +--- + +## Description + + +## The defect + +A URI written in angle brackets inside body text is refused as a syntax error, and the platform renders it correctly. + +```liquid +{% capture m %}see {% endcapture %}OK[{{ m }}] +``` + +`LiquidHTMLSyntaxError` fires; the supervisor returns `must_fix_before_write: true`, so an agent cannot write the file. On a live instance the same buffer renders `OK[see <https://example.com/a|Name>]` — the brackets are escaped and printed, which is the intended result. Confirmed by O1c: `pos-cli deploy --dry-run` accepts it with its control accepted. + +This is the Slack-link idiom (``). It occurs in one file of a 2,768-file production application, `app/api_calls/send_slack_message.liquid`, and it blocks that entire file. + +## Why this is a different problem from the tag-markup false blocks + +The other measured false blocks come from a tag's strict markup rule failing, so the markup is kept as a raw string and `InvalidTagSyntax` reports it. This one does not go through that path at all: `` is consumed by the **HTML element** layer, which sees an opening tag whose name is `https:` and never finds a close. The mechanism, the fix and the risk are unrelated, which is why it is tracked separately. + +## Bound the blast radius before changing anything + +Loosening what counts as an HTML element name is the obvious fix and the dangerous one — it risks turning genuinely malformed HTML into silence, which trades a false block for a missed detection. Measure before narrowing: + +- Which strings after `<` are currently treated as an element name, and which of those the platform actually parses as HTML. +- Whether a real unclosed element (`
` with no `
`) still reports after the change. That case must not go quiet. +- Whether the construct behaves the same outside `capture` — in plain page body text, in an `{% if %}` branch, and inside an HTML attribute value — since the reproduction above only establishes the `capture` position. +- Whether the printer round-trips the buffer unchanged both before and after. It survives formatting today because the surrounding text is emitted verbatim; confirm that still holds. + +A scheme-like prefix followed by `//` is a plausible discriminator (an HTML element name cannot contain `:` followed by `//`), but it is a hypothesis to measure rather than a design to implement on sight. + +## Evidence + +- Round `ROUND-2026-08-26`, finding `S13-FB-angle-uri-in-capture`, severity FALSE_BLOCK, confirmed by O1c. +- Reproduction and control are in `supervisor-tests/auto-eval/suites/13-cli-parity.mjs`, in the `REDUCED` table. That row declares `expectSameOutput: false`, because its control deliberately removes the brackets and therefore renders different text — do not treat the output difference as evidence of misbehaviour. + + +## Acceptance Criteria + +- [ ] #1 `{% capture m %}see {% endcapture %}` is accepted and does not set must_fix_before_write +- [ ] #2 The same construct is measured in at least three positions beyond capture (plain body text, inside a conditional branch, inside an HTML attribute value) and each measured-accepted position is covered by a fixture +- [ ] #3 A genuinely unclosed HTML element still reports, asserted in the same test file so the loosening cannot silently swallow it +- [ ] #4 A malformed construct that is NOT a scheme-like URI still reports, so the change admits the measured shape rather than everything after a `<` +- [ ] #5 The buffer round-trips through prettier unchanged +- [ ] #6 Deliberately reverting the change makes the new tests fail (sabotage-verified), recorded in the task notes +- [ ] #7 `pos-cli check` over the 2,768-file corpus reports no offense it did not report before, other than the intended change on this construct +- [ ] #8 The measurement that bounds the change is recorded in the task notes, including any position where the platform does NOT accept the construct + + +## Implementation Notes + + +## Bounding measurement (2026-08-26) — carried out, and it CHANGES the fix + +Every position rendered on `fk-docs.ps-01-platformos.com` via `/api/app_builder/liquid_exec`, one construct per request: + +| position | platform | +|---|---| +| `{% capture m %}see {% endcapture %}` | renders (brackets escaped) | +| plain body text | renders | +| inside an `{% if %}` branch | renders | +| inside an HTML attribute value | renders | +| **`
` with no close (control)** | **renders** | +| **`` — no scheme (control)** | **renders** | + +**The platform validates no HTML whatsoever.** Liquid passes markup through as text, so the two controls render exactly like the URI case. Two consequences for this task: + +1. **Platform parity gives NO guidance on where to draw the line.** "The platform accepts it" is true of every malformed HTML string, so it cannot be the discriminator — using it would justify deleting HTML checking entirely. +2. **AC #3 and AC #4 are not parity requirements, they are OUR value-add.** An unclosed `
` is a real defect in the author's HTML and worth reporting even though the platform renders it. `` renders too, so a fix keyed on "does it render" would admit it as well. + +So the change must be keyed on the SHAPE (a scheme-like prefix followed by `//`, which no HTML element name can contain), narrowly, and explicitly NOT on platform acceptance. This is a judgement about where our strictness belongs, not a parity fix — decide it before writing grammar. + +This also means the task is **not ready to implement as filed**: the original description offered the scheme-prefix idea as a hypothesis to measure, and the measurement has now removed the parity justification for it while leaving the shape argument standing. + diff --git a/.changeset/tolerated-tag-syntax-is-not-a-blocking-error.md b/.changeset/tolerated-tag-syntax-is-not-a-blocking-error.md new file mode 100644 index 00000000..bfce17f2 --- /dev/null +++ b/.changeset/tolerated-tag-syntax-is-not-a-blocking-error.md @@ -0,0 +1,26 @@ +--- +'@platformos/platformos-check-common': minor +'@platformos/platformos-check-node': minor +'@platformos/platformos-mcp-supervisor': patch +--- + +Stop refusing three tag spellings that platformOS parses as intended + +`{% capture 'name' %}`, `{% case x: %}` and `{% parse_json v %%}` were reported by +`InvalidTagSyntax`, which lands under `LiquidHTMLSyntaxError` — `Severity.ERROR` and a member +of the MCP supervisor's blocking set — so an agent was told not to write the file at all. On a +2,768-file production application these accounted for **34 of the 122** `LiquidHTMLSyntaxError` +offenses, 32 of them `{% capture 'name' %}`, the most frequently refused construct in real +code. Every one was rendered on a live instance and produces the author's intended result. + +They now report as `UnconventionalTagSyntax` at `warning`, which is outside the blocking set: +still advised against, no longer fatal. Corpus totals are otherwise identical — 13,065 offenses +across 1,950 files before and after, with `LiquidHTMLSyntaxError` 122 → 88 and the 34 moving to +the new check. + +The admitted set is a deliberate allowlist, not a relaxation of `InvalidTagSyntax`. The +platform matches tag markup with an unanchored regex, so it also accepts spellings that then do +the wrong thing **silently** — a mistyped `{% cache: k %}` collapses the cache key to a +constant, and because the full key carries no user component, distinct keys share one entry +across the instance and one user's rendered fragment is served to another. Those keep blocking, +and are asserted alongside the demoted ones so the two halves cannot drift apart. diff --git a/packages/platformos-check-common/src/checks/index.ts b/packages/platformos-check-common/src/checks/index.ts index b205d322..4111b4d6 100644 --- a/packages/platformos-check-common/src/checks/index.ts +++ b/packages/platformos-check-common/src/checks/index.ts @@ -57,6 +57,7 @@ import { JsonLiteralQuoteStyle } from './json-literal-quote-style'; import { MissingContentForLayout } from './missing-content-for-layout'; import { YAMLSyntaxError } from './yaml-syntax-error'; import { UnsupportedStringEscape } from './unsupported-string-escape'; +import { UnconventionalTagSyntax } from './unconventional-tag-syntax'; export const allChecks: (LiquidCheckDefinition | GraphQLCheckDefinition | YAMLCheckDefinition)[] = [ DeprecatedFilter, @@ -68,6 +69,7 @@ export const allChecks: (LiquidCheckDefinition | GraphQLCheckDefinition | YAMLCh ImgWidthAndHeight, ImplicitIncludeArguments, LiquidHTMLSyntaxError, + UnconventionalTagSyntax, MatchingTranslations, MissingAsset, MissingDocParam, diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts index 31fba68c..670cb547 100644 --- a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/InvalidTagSyntax.ts @@ -1,5 +1,6 @@ import { LiquidTag, NamedTags, TAGS_WITHOUT_MARKUP } from '@platformos/liquid-html-parser'; import { Problem, SourceCodeType, TagEntry } from '../../..'; +import { isToleratedTagMarkup } from './tolerated-tag-markup'; /** * Tags that use no markup at all — they are valid as `{% else %}`, `{% break %}`, etc. @@ -77,6 +78,14 @@ export function detectInvalidTagSyntax( return; } + // Spellings the platform handles CORRECTLY are reported by UnconventionalTagSyntax at + // warning severity instead, so they no longer block the write. One shared predicate, so + // the two checks cannot both fire or both go silent — see tolerated-tag-markup.ts for why + // each shape is admitted and why the list must stay an allowlist. + if (isToleratedTagMarkup(node)) { + return; + } + // Build a helpful hint from the docset if available const tagEntry = tags.find((t) => t.name === tagName); const syntaxHint = tagEntry?.syntax ? ` Expected syntax: ${tagEntry.syntax}` : ''; diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts new file mode 100644 index 00000000..1608e62d --- /dev/null +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts @@ -0,0 +1,48 @@ +import { LiquidTag, NamedTags } from '@platformos/liquid-html-parser'; + +/** + * Tag markup the grammar refuses that platformOS parses AS INTENDED, measured on a live + * instance. The single source of truth for the split between `LiquidHTMLSyntaxError` + * (error, blocking — silent on everything here) and `UnconventionalTagSyntax` (warning, + * non-blocking — reports exactly this). + * + * An ALLOWLIST, and it must stay one. The platform matches tag markup with an unanchored + * regex, so it also "accepts" spellings that then do the wrong thing silently — a mistyped + * `{% cache: k %}` collapses the key to a constant and shares one cache entry across the + * whole instance. "The platform accepts it" is therefore never sufficient grounds to add a + * shape here; it must be measured to produce the AUTHOR'S intended result. + * + * Do not key on the tag name (`capture` is admitted for one shape and raises on empty + * markup) nor on "starts with a stray separator" (`{% cache expire: 30 %}` has no leading + * separator and collapses identically). + */ + +/** `{% capture 'name' %}` — 32 corpus occurrences. Capture::Syntax finds the name inside the quotes. */ +const CAPTURE_QUOTED_TARGET = /^\s*(['"])([A-Za-z_][\w-]*)\1\s*$/; + +/** + * `{% case x: %}` — trailing colon(s), optionally spaced. VariableLookup scans `[\w-]+` and + * drops them. A name is required: `{% case : %}` looks up nil and every `when` misses. + */ +const CASE_TRAILING_COLON = /^\s*[A-Za-z_][\w-]*(?:\.[\w-]+)*\s*:+\s*$/; + +/** `{% parse_json v %%}` — a stray `%` the unanchored SYNTAX never reaches. A name is required. */ +const PARSE_JSON_TRAILING_PERCENT = /^\s*[A-Za-z_][\w-]*\s*%+\s*$/; + +const TOLERATED: Partial> = { + [NamedTags.capture]: CAPTURE_QUOTED_TARGET, + [NamedTags.case]: CASE_TRAILING_COLON, + [NamedTags.parse_json]: PARSE_JSON_TRAILING_PERCENT, +}; + +/** + * Callers must already have established that `node.markup` is a string — the only situation + * in which either check runs. + */ +export function isToleratedTagMarkup(node: LiquidTag): boolean { + if (typeof node.markup !== 'string') return false; + const shape = TOLERATED[node.name]; + return shape !== undefined && shape.test(node.markup); +} + +export const TAGS_WITH_TOLERATED_MARKUP: readonly string[] = Object.keys(TOLERATED); diff --git a/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts new file mode 100644 index 00000000..4033aa24 --- /dev/null +++ b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts @@ -0,0 +1,159 @@ +import { describe, expect, it } from 'vitest'; +import { UnconventionalTagSyntax } from '.'; +import { LiquidHTMLSyntaxError } from '../liquid-html-syntax-error'; +import { runLiquidCheck } from '../../test'; +import { Severity } from '../../types'; + +/** + * Both halves of the split live here on purpose. The risk is not that this check fails to + * fire; it is that it fires too widely, because several spellings the platform also accepts + * are silently WRONG, and demoting one turns a refused write into a shipped defect. + * + * Every row's platform behaviour was rendered on a live instance (round ROUND-2026-08-26). + */ + +/** Warned about, never blocked. */ +const TOLERATED = [ + { what: 'capture, single-quoted target', source: `{% capture 'cs' %}HI{% endcapture %}` }, + { what: 'capture, double-quoted target', source: `{% capture "cs" %}HI{% endcapture %}` }, + { what: 'capture, hyphen in the quoted name', source: `{% capture 'a-b' %}HI{% endcapture %}` }, + { what: 'case, trailing colon', source: `{% case g: %}{% when 1 %}ONE{% endcase %}` }, + { what: 'case, dotted path then colon', source: `{% case g.type: %}{% when 1 %}A{% endcase %}` }, + { what: 'case, space before the colon', source: `{% case g : %}{% when 1 %}ONE{% endcase %}` }, + { what: 'case, double colon', source: `{% case g:: %}{% when 1 %}ONE{% endcase %}` }, + { what: 'parse_json, stray percent', source: `{% parse_json d %%}{"k":2}{% endparse_json %}` }, + { + what: 'parse_json, several percents', + source: `{% parse_json d %%%}{"k":2}{% endparse_json %}`, + }, + // Prettier normalises `%%}` to `% %}`; both leave markup `d %`, so a save must not turn a + // warning into a blocked write. + { + what: 'parse_json, as prettier reprints it', + source: `{% parse_json d % %}{"k":2}{% endparse_json %}`, + }, +]; + +/** Must keep blocking: each either raises on the platform, or runs while doing the wrong thing. */ +const MUST_STILL_BLOCK = [ + { + what: 'cache with a leading colon — key collapses to ":" and is shared instance-wide', + source: `{% cache: k, expire: 30 %}BODY{% endcache %}`, + }, + { + what: 'cache with the key omitted — collapses to "expire:" with no leading separator', + source: `{% cache expire: 30 %}BODY{% endcache %}`, + }, + { what: 'log with a leading colon — the message becomes ":"', source: `{% log: o, type: 'E' %}` }, + { + what: 'response_headers whose nested quotes truncate the argument', + source: `{% response_headers '{ "CSP" : "frame-ancestors 'none'" }' %}`, + }, + { + what: 'capture with empty markup — the platform raises', + source: `{% capture %}x{% endcapture %}`, + }, + { + what: 'capture with a space inside the quotes — the regex would take only `a`', + source: `{% capture 'a b' %}x{% endcapture %}`, + }, + { + what: 'capture with a trailing token — which of the two is the target is ambiguous', + source: `{% capture 'cs' extra %}x{% endcapture %}`, + }, + { + what: 'capture with a digit-leading quoted name — unmeasured, so not admitted', + source: `{% capture '123' %}x{% endcapture %}`, + }, + { + what: 'case with a colon and no name — looks up nil, every when misses', + source: `{% case : %}{% when 1 %}ONE{% endcase %}`, + }, + { + what: 'case with a colon then another token', + source: `{% case g : : %}{% when 1 %}ONE{% endcase %}`, + }, + { + what: 'parse_json with a percent and no name — the platform raises', + source: `{% parse_json %%}{"k":2}{% endparse_json %}`, + }, +]; + +/** Well-formed markup: the grammar parses it, so neither check may speak. */ +const WELL_FORMED = [ + `{% capture cs %}HI{% endcapture %}`, + `{% case g %}{% when 1 %}ONE{% endcase %}`, + `{% parse_json d %}{"k":2}{% endparse_json %}`, + `{% cache 'k', expire: 30 %}BODY{% endcache %}`, + `{% log o, type: 'E' %}`, +]; + +async function bothChecks(source: string) { + return { + unconventional: await runLiquidCheck(UnconventionalTagSyntax, source), + invalidTagSyntax: (await runLiquidCheck(LiquidHTMLSyntaxError, source)).filter((o) => + /Invalid syntax for tag/.test(o.message), + ), + }; +} + +describe('UnconventionalTagSyntax', () => { + describe('demotes the measured-safe spellings', () => { + for (const { what, source } of TOLERATED) { + it(`warns, and nothing blocks: ${what}`, async () => { + const { unconventional, invalidTagSyntax } = await bothChecks(source); + expect(unconventional).toHaveLength(1); + expect(unconventional[0].check).toBe('UnconventionalTagSyntax'); + expect(unconventional[0].severity).toBe(Severity.WARNING); + expect(invalidTagSyntax).toEqual([]); + }); + } + }); + + describe('leaves the dangerous spellings blocking', () => { + for (const { what, source } of MUST_STILL_BLOCK) { + it(`still an error, and not demoted: ${what}`, async () => { + const { unconventional, invalidTagSyntax } = await bothChecks(source); + expect(unconventional).toEqual([]); + expect(invalidTagSyntax.length).toBeGreaterThan(0); + expect(invalidTagSyntax.every((o) => o.severity === Severity.ERROR)).toBe(true); + }); + } + }); + + describe('says nothing about well-formed markup', () => { + for (const source of WELL_FORMED) { + it(`silent: ${source}`, async () => { + const { unconventional, invalidTagSyntax } = await bothChecks(source); + expect(unconventional).toEqual([]); + expect(invalidTagSyntax).toEqual([]); + }); + } + }); + + it('never fires on the same construct as the blocking check', async () => { + for (const { source } of [...TOLERATED, ...MUST_STILL_BLOCK]) { + const { unconventional, invalidTagSyntax } = await bothChecks(source); + expect( + unconventional.length > 0 && invalidTagSyntax.length > 0, + `both fired on ${source}`, + ).toBe(false); + } + }); + + it('covers every tag in the allowlist, so a tag cannot be added without a fixture', async () => { + const covered = new Set(); + for (const { source } of TOLERATED) { + const [offense] = await runLiquidCheck(UnconventionalTagSyntax, source); + covered.add(/\{%\s*([a-z_]+)/.exec(offense.message.replace(/^`/, ''))?.[1] ?? ''); + } + expect([...covered].sort()).toEqual(['capture', 'case', 'parse_json']); + }); + + it('does not report inside {% raw %}, whose body the parser keeps as text', async () => { + const { unconventional } = await bothChecks( + `{% raw %}{% capture 'cs' %}HI{% endcapture %}{% endraw %}`, + ); + expect(unconventional).toEqual([]); + }); +}); diff --git a/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.ts b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.ts new file mode 100644 index 00000000..0272bef1 --- /dev/null +++ b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.ts @@ -0,0 +1,51 @@ +import { Severity, SourceCodeType, LiquidCheckDefinition } from '../../types'; +import { isToleratedTagMarkup } from '../liquid-html-syntax-error/checks/tolerated-tag-markup'; + +/** + * Tag markup the grammar refuses that platformOS parses as intended. + * + * A separate check code only because `Problem` carries no per-offense severity: these findings + * come from the same detection as `InvalidTagSyntax`, which reports under + * `LiquidHTMLSyntaxError` at `Severity.ERROR` and blocks the write. 34 of the 122 syntax errors + * on a 2,768-file production app were these spellings, all of which run correctly. + * + * Stays a warning rather than disappearing: the spellings work by accident, and widening the + * grammar instead would move the burden to the prettier printer, which today emits this markup + * verbatim only because it is still a raw string. + * + * The admitted set is the allowlist in `tolerated-tag-markup.ts` — read it before adding a shape. + */ +export const UnconventionalTagSyntax: LiquidCheckDefinition = { + meta: { + code: 'UnconventionalTagSyntax', + name: 'Report tag syntax the platform tolerates but does not document', + docs: { + description: + 'Reports tag markup that platformOS parses correctly even though it does not match the ' + + 'documented syntax, so the construct can be tidied without the write being refused.', + url: 'https://documentation.platformos.com/developer-guide/platformos-check/checks/unconventional-tag-syntax', + recommended: true, + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.WARNING, + schema: {}, + targets: [], + }, + + create(context) { + return { + async LiquidTag(node) { + if (!isToleratedTagMarkup(node)) return; + + context.report({ + message: + `\`{% ${node.name} ${node.markup} %}\` is not the documented syntax for ` + + `'${node.name}', but platformOS parses it as intended. It runs correctly; ` + + `rewrite it in the documented form when convenient.`, + startIndex: node.position.start, + endIndex: node.position.end, + }); + }, + }; + }, +}; diff --git a/packages/platformos-check-node/configs/all.yml b/packages/platformos-check-node/configs/all.yml index 846eeed1..15d55d3d 100644 --- a/packages/platformos-check-node/configs/all.yml +++ b/packages/platformos-check-node/configs/all.yml @@ -112,6 +112,9 @@ TranslationKeyExists: UnclosedHTMLElement: enabled: true severity: 1 +UnconventionalTagSyntax: + enabled: true + severity: 1 UndefinedObject: enabled: true severity: 1 diff --git a/packages/platformos-check-node/configs/recommended.yml b/packages/platformos-check-node/configs/recommended.yml index 846eeed1..15d55d3d 100644 --- a/packages/platformos-check-node/configs/recommended.yml +++ b/packages/platformos-check-node/configs/recommended.yml @@ -112,6 +112,9 @@ TranslationKeyExists: UnclosedHTMLElement: enabled: true severity: 1 +UnconventionalTagSyntax: + enabled: true + severity: 1 UndefinedObject: enabled: true severity: 1 diff --git a/packages/platformos-mcp-supervisor/src/result/blocking.spec.ts b/packages/platformos-mcp-supervisor/src/result/blocking.spec.ts index f5f22977..72d02275 100644 --- a/packages/platformos-mcp-supervisor/src/result/blocking.spec.ts +++ b/packages/platformos-mcp-supervisor/src/result/blocking.spec.ts @@ -145,6 +145,16 @@ describe('Unit: blocksWrite', () => { expect(blocksWrite([at('MissingAsset'), at('ReservedVariableName')])).toBe(false); }); + it('does not block tag syntax the platform parses as intended', () => { + // Split out of LiquidHTMLSyntaxError so it could be non-blocking at all: 34 of the 122 + // syntax errors on a real 2,768-file app were spellings measured to run correctly. + expect(BLOCKING_CHECKS.has('UnconventionalTagSyntax')).toBe(false); + expect(blocksWrite([at('UnconventionalTagSyntax', 'warning')])).toBe(false); + // Its blocking sibling must be unaffected — the split has to keep one side gating. + expect(BLOCKING_CHECKS.has('LiquidHTMLSyntaxError')).toBe(true); + expect(blocksWrite([at('LiquidHTMLSyntaxError')])).toBe(true); + }); + it('blocks the JSON literal quote style, which is fatal at runtime AND on deploy', () => { expect(BLOCKING_CHECKS.has('JsonLiteralQuoteStyle')).toBe(true); expect(blocksWrite([at('JsonLiteralQuoteStyle')])).toBe(true); diff --git a/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts b/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts new file mode 100644 index 00000000..36af2cb0 --- /dev/null +++ b/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts @@ -0,0 +1,93 @@ +/** + * End to end: a real buffer through the real engine, then through the real write gate. + * + * `blocking.spec.ts` asserts the gate's membership logic over synthetic diagnostics. This + * asserts the thing an agent actually experiences — that these buffers produce diagnostics + * whose codes and severities let the write through, and that the dangerous spellings still + * do not. Both halves are here so demoting one can never silently demote the other. + */ +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { runBatchLint } from '../lint/lint-batch.js'; +import { blocksWrite } from './blocking.js'; + +let projectDir: string; + +beforeAll(() => { + projectDir = mkdtempSync(join(tmpdir(), 'mcp-sup-tolerated-')); + mkdirSync(join(projectDir, '.git')); +}); + +afterAll(() => { + rmSync(projectDir, { recursive: true, force: true }); +}); + +const FILE = 'app/views/partials/probe.liquid'; + +async function gate(source: string) { + const { diagnostics } = await runBatchLint({ + projectDir, + buffers: [{ filePath: FILE, content: source }], + }); + const found = diagnostics.get(FILE) ?? []; + return { + codes: [...new Set(found.map((d) => d.check))].sort(), + severities: Object.fromEntries(found.map((d) => [d.check, d.severity])), + blocks: blocksWrite(found.filter((d) => d.severity === 'error')), + }; +} + +/** Measured on a live instance to produce the author's intended result. */ +const TOLERATED = [ + { what: 'capture with a quoted target', source: `{% capture 'cs' %}HI{% endcapture %}` }, + { what: 'case with a trailing colon', source: `{% case g: %}{% when 1 %}ONE{% endcase %}` }, + { + what: 'parse_json with a stray percent', + source: `{% parse_json d %%}{"k":2}{% endparse_json %}`, + }, +]; + +/** Measured to raise, or to run while doing something other than what was written. */ +const BLOCKED = [ + { what: 'cache with a leading colon', source: `{% cache: k, expire: 30 %}B{% endcache %}` }, + { what: 'cache with the key omitted', source: `{% cache expire: 30 %}B{% endcache %}` }, + { what: 'log with a leading colon', source: `{% log: o, type: 'E' %}` }, + { what: 'capture with empty markup', source: `{% capture %}x{% endcapture %}` }, +]; + +describe('The write gate lets tolerated tag syntax through', () => { + it.each(TOLERATED)('$what', async ({ source }) => { + const { codes, severities, blocks } = await gate(source); + expect(codes).toContain('UnconventionalTagSyntax'); + expect(severities.UnconventionalTagSyntax).toBe('warning'); + expect(codes).not.toContain('LiquidHTMLSyntaxError'); + expect(blocks).toBe(false); + }); +}); + +describe('and still refuses the spellings that misbehave', () => { + it.each(BLOCKED)('$what', async ({ source }) => { + const { codes, severities, blocks } = await gate(source); + expect(codes).toContain('LiquidHTMLSyntaxError'); + expect(severities.LiquidHTMLSyntaxError).toBe('error'); + expect(codes).not.toContain('UnconventionalTagSyntax'); + expect(blocks).toBe(true); + }); +}); + +describe('and says nothing about the well-formed spellings', () => { + it.each([ + { what: 'capture', source: `{% capture cs %}HI{% endcapture %}` }, + { what: 'case', source: `{% case g %}{% when 1 %}ONE{% endcase %}` }, + { what: 'parse_json', source: `{% parse_json d %}{"k":2}{% endparse_json %}` }, + { what: 'cache', source: `{% cache 'k', expire: 30 %}B{% endcache %}` }, + ])('$what', async ({ source }) => { + const { codes, blocks } = await gate(source); + expect(codes).not.toContain('UnconventionalTagSyntax'); + expect(codes).not.toContain('LiquidHTMLSyntaxError'); + expect(blocks).toBe(false); + }); +}); From 4513e20456a2dd4faf3a8a33c78776dd172dfac9 Mon Sep 17 00:00:00 2001 From: Filip Klosowski Date: Wed, 26 Aug 2026 17:24:46 +0200 Subject: [PATCH 2/3] fix: ensure consistent verdicts for malformed statements in liquid bodies and tags --- ...dy-is-silently-unreported-for-some-tags.md | 40 +++++++++++++++++++ .../unconventional-tag-syntax/index.spec.ts | 27 +++++++++++++ 2 files changed, 67 insertions(+) diff --git a/.backlog/tasks/task-80 - A-statement-whose-markup-fails-inside-a-liquid-body-is-silently-unreported-for-some-tags.md b/.backlog/tasks/task-80 - A-statement-whose-markup-fails-inside-a-liquid-body-is-silently-unreported-for-some-tags.md index d115bf06..ac9a7531 100644 --- a/.backlog/tasks/task-80 - A-statement-whose-markup-fails-inside-a-liquid-body-is-silently-unreported-for-some-tags.md +++ b/.backlog/tasks/task-80 - A-statement-whose-markup-fails-inside-a-liquid-body-is-silently-unreported-for-some-tags.md @@ -6,6 +6,7 @@ title: >- status: To Do assignee: [] created_date: '2026-08-18 12:02' +updated_date: '2026-08-26 14:51' labels: - check-common - liquid-html-parser @@ -38,3 +39,42 @@ Consequence: the same construct blocks as a tag and is approved inside a {% liqu - [ ] #2 A statement with raw markup inside a liquid body is reported whichever tag it names - [ ] #3 assign ((( = 9, echo ((( and assign h .k = 9 inside a liquid body all block, with the tag-form messages unchanged + +## Implementation Notes + + +## Re-measured 2026-08-26 against master + TASK-96 (build `01f9acc` + the tolerated-tag-syntax split) + +Still reproduces, and the scope is **narrower than filed in one place and misattributed in another**. Swept 20 malformed statements, each in tag form and in `{% liquid %}` body form, through `runBatchLint` (real lint, no mocks). + +### Confirmed divergences — 2, both `assign` + +| statement | tag form | `{% liquid %}` body | +|---|---|---| +| `assign h .k = 9` | BLOCKS | **silent** | +| `assign x ((( = 9` | BLOCKS | **silent** | + +These are the real defect: the identical statement is refused in one spelling and approved in the other, so moving working-looking code into a liquid block loses the diagnostic. + +### Correction to the task as filed + +`echo (((` is listed above as a liquid-body silence. It is **silent in BOTH forms**, so it is not a tag-vs-liquid divergence at all — it is a uniform missed detection and belongs to a different fix. AC #3 should be split accordingly: the `assign` rows are a consistency bug, the `echo` row is a coverage gap. + +### Consistent, and therefore out of scope + +16 of the 20 swept statements reach the same verdict in both forms, including every one that already blocks: `assign = 9`, `assign x =`, `echo | upcase`, `hash_assign h ['k'] = 9`, `function r ['k'] = 'lib/x'`, `log: o`, `response_status: 404`, `return (((`, `increment (((`, `render 'a': b: 1`, `include 'a', b`, `cache: k`, `if (((`, `unless (((`. + +### TASK-96 did not widen this, verified rather than assumed + +The three spellings demoted to `UnconventionalTagSyntax` reach the SAME verdict in both forms — `warning`, non-blocking, in tag form and in a liquid body. Pinned by a parity fixture in `unconventional-tag-syntax/index.spec.ts` so a demoted spelling can never join the divergence list. `log: o` still blocks in both forms, so the demotion did not leak into this path. + +### Sweep methodology warning for whoever picks this up + +Three false divergences appeared in earlier runs of this sweep, all fixture errors rather than defects: + +- a BLOCK tag (`capture`, `case`, `cache`, `if`, `unless`, `for`) written with no closing tag blocks on "never closed", which is a different error than a markup rejection; +- a block body written as a bare token (`X`) inside a `{% liquid %}` body is not a valid statement there, so the error comes from the body and not from the statement under test — use `echo 'X'`; +- the interactive MCP supervisor process does not reload when `dist` is rebuilt, so spot-checks through it report the PREVIOUS build. Import `dist/lint/lint-batch.js` directly, or restart the client. + +Each of those produced a plausible, wrong conclusion before being caught. + diff --git a/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts index 4033aa24..2f100bc0 100644 --- a/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts +++ b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts @@ -156,4 +156,31 @@ describe('UnconventionalTagSyntax', () => { ); expect(unconventional).toEqual([]); }); + + /** + * TASK-80: the same malformed statement can reach different verdicts as a tag and inside a + * `{% liquid %}` body, and `assign` still does. A demoted spelling must not join that list — + * an author moving working code into a liquid block would otherwise gain a blocking error. + */ + describe('reaches the same verdict inside a {% liquid %} body', () => { + it.each([ + { + what: 'capture', + tag: `{% capture 'cs' %}X{% endcapture %}`, + liquid: `{% liquid\n capture 'cs'\n echo 'X'\n endcapture\n%}`, + }, + { + what: 'case', + tag: `{% case g: %}{% when 1 %}A{% endcase %}`, + liquid: `{% liquid\n case g:\n when 1\n echo 'A'\n endcase\n%}`, + }, + ])('$what', async ({ tag, liquid }) => { + const asTag = await bothChecks(tag); + const inBody = await bothChecks(liquid); + expect(asTag.unconventional).toHaveLength(1); + expect(inBody.unconventional).toHaveLength(1); + expect(asTag.invalidTagSyntax).toEqual([]); + expect(inBody.invalidTagSyntax).toEqual([]); + }); + }); }); From b6b500c68b43c7adf41533aa7cf1b503c3d1529f Mon Sep 17 00:00:00 2001 From: Filip Klosowski Date: Wed, 26 Aug 2026 18:44:03 +0200 Subject: [PATCH 3/3] feat: implement baseTagValue function to extract values from Liquid tag markup and enhance tolerated tag checks --- ...ugh-the-platform-handles-them-correctly.md | 38 +++- ...-the-platform-sets-the-header-correctly.md | 181 ++++++++++++++++++ .../checks/base-tag-value.ts | 81 ++++++++ .../checks/tolerated-tag-markup.ts | 38 +++- .../unconventional-tag-syntax/index.spec.ts | 122 ++++++++++-- .../result/tolerated-tag-syntax-gate.spec.ts | 11 ++ 6 files changed, 450 insertions(+), 21 deletions(-) create mode 100644 .backlog/tasks/task-98 - response_headers-with-balanced-nested-quotes-is-a-blocking-error-and-the-platform-sets-the-header-correctly.md create mode 100644 packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/base-tag-value.ts diff --git a/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md b/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md index c879dd2c..1bce651d 100644 --- a/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md +++ b/.backlog/tasks/task-96 - Three-tag-markup-spellings-in-real-deployed-code-are-blocking-errors-although-the-platform-handles-them-correctly.md @@ -6,7 +6,7 @@ title: >- status: In Progress assignee: [] created_date: '2026-08-26 13:49' -updated_date: '2026-08-26 14:41' +updated_date: '2026-08-26 15:56' labels: - check-common - false-block @@ -201,4 +201,40 @@ Full monorepo: 357 test files / 4,535 tests pass; `type-check`, `build` and `for ### Unrelated finding, recorded so it is not lost `RollbackOutsideTransaction` already exists and works correctly: a **page** with a bare `{% rollback %}` is reported; a **partial** deliberately is not, because `RollbackTag` checks `AfterCommitEverywhere.in_transaction?` at runtime and the caller decides. The eval's `S13-FN-rollback-outside-transaction` finding probes a partial, so it measures a designed silence — a fixture artifact of the harness, not a gap in the checks. Belongs to the eval, not to this task. + +## Post-implementation audit — every claim re-measured against the TAG ITSELF + +Triggered by finding that one justification had been measured through a PROXY. Re-ran all 21 constructs directly, one request each. + +### The allowlist is sound — all 10 verified INTENDED + +Three of them had only been REASONED about before and are now measured: `capture "cs"` (double-quoted) → `[HI]`; `case h.t:` (dotted path) → `[ONE]`; `parse_json d % %}` (the post-format spelling) → `[2]`. No change to the predicate was needed. + +### Three blocked-case JUSTIFICATIONS were wrong — all corrected in the spec + +| construct | claimed | measured | +|---|---|---| +| `response_headers '{…'none'…}'` | "receives 27 chars of malformed JSON" | **sets the header correctly, quotes intact** — a genuine FALSE BLOCK | +| `capture '1x'` | "unmeasured, so not admitted" | **runs correctly**, captures into `1x` | +| `case g : :` | no reason given | **runs correctly**, takes the right branch | + +The `response_headers` error was a proxy measurement: it was taken through `{% assign s = %}`, which truncates the literal to 27 characters. A `Base` tag matches `QuotedFragment`, which platformOS redefines to be escape-aware (`app/lib/liquid/quoted_string_escapes.rb`), so the tag receives the whole argument. **`assign` is not a proxy for a tag's own parsing.** Filed as TASK-98 with the balance boundary measured (a balanced inner pair works; an unbalanced apostrophe still fails with HTTP 501). + +### Three justifications verified CORRECT + +`capture 'a b'` → `[a=HI][b=]`, captures into `a` and drops ` b`. `case :` → `[FELL]`, takes the else branch silently — the genuinely dangerous one. `parse_json %` → raises. + +### What changed, and what did not + +The fix is **spec-only**: `git diff --name-only` lists one file, `unconventional-tag-syntax/index.spec.ts`. No source file changed, so behaviour is provably identical. The blocked set now labels each row `RAISES` / `WRONG` / `NOT ADMITTED` so a reader can tell a platform refusal from a silent misbehaviour from a knowingly-accepted false block — they are not interchangeable, and the flat list invited exactly the error above. + +`NOT ADMITTED` is stated as a deliberate trade: `capture '1x'` and `case g : :` run correctly and are still refused, because each has zero corpus occurrences and the allowlist is kept minimal. Widen with data, not sympathy. + +### Re-verified after the edits + +Full monorepo 357 files / 4,537 tests pass; `type-check` and `format:check` clean. All 9 sabotage mutations still bite (baseline 31/31). Corpus diff re-run and byte-identical: 13,065 offenses / 1,950 files unchanged, `LiquidHTMLSyntaxError` 122 → 88, `UnconventionalTagSyntax` 0 → 34. + +### Eval side, corrected in the same pass + +`suites/13-cli-parity.mjs`: the `valueFidelity` oracle was removed entirely — its only use was the invalid proxy above, so keeping it would have been dead code inviting reuse. The row now carries a "do not re-add a valueFidelity proxy here; read the header" note. `S13-FB-response-headers-nested-quotes` is restored as a FALSE_BLOCK and `S13-FB-log-colon` remains correctly retracted — verified by re-running the suite (3 findings, 1 retraction). diff --git a/.backlog/tasks/task-98 - response_headers-with-balanced-nested-quotes-is-a-blocking-error-and-the-platform-sets-the-header-correctly.md b/.backlog/tasks/task-98 - response_headers-with-balanced-nested-quotes-is-a-blocking-error-and-the-platform-sets-the-header-correctly.md new file mode 100644 index 00000000..e1b2f453 --- /dev/null +++ b/.backlog/tasks/task-98 - response_headers-with-balanced-nested-quotes-is-a-blocking-error-and-the-platform-sets-the-header-correctly.md @@ -0,0 +1,181 @@ +--- +id: TASK-98 +title: >- + response_headers with balanced nested quotes is a blocking error, and the + platform sets the header correctly +status: In Progress +assignee: [] +created_date: '2026-08-26 15:56' +updated_date: '2026-08-26 16:43' +labels: + - check-common + - false-block + - measured + - blocking-check +dependencies: + - TASK-96 +references: + - >- + packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts + - >- + packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.ts + - supervisor-tests/auto-eval/suites/13-cli-parity.mjs + - supervisor-tests/auto-eval/lib/runtime.mjs +priority: medium +ordinal: 73000 +--- + +## Description + + +## The defect + +```liquid +{% response_headers '{ "Content-Security-Policy" : "frame-ancestors 'none'" }' %} +``` + +`InvalidTagSyntax` reports this under `LiquidHTMLSyntaxError` — `Severity.ERROR`, in the supervisor's `BLOCKING_CHECKS` — so an agent is told not to write the file. Measured on a live instance, **the platform sets the header correctly, quotes intact**: + +``` +baseline (no tag) content-security-policy: (absent) +{% response_headers '{"CSP":"a"}' %} header set to "a" +the buffer above content-security-policy: frame-ancestors 'none' +``` + +One occurrence in a 2,768-file production application, in `app/views/layouts/application.html.liquid` — a file every page loads. So the entire layout of that application is currently unwritable by an agent. + +## The shape boundary, measured + +Nesting is not the discriminator; **balance** is. + +| argument | result | +|---|---| +| `'{ "CSP" : "frame-ancestors 'none'" }'` — balanced nested pair | HTTP 200, header set correctly | +| `'{ "X-Ae" : "a 'b' c" }'` — balanced, non-CSP header | HTTP 200, header set to `a 'b' c` | +| `'{"X-Ae": "va'lue"}'` — unbalanced apostrophe | **HTTP 501**, header not set | + +platformOS redefines `QuotedFragment` to be escape-aware (`app/lib/liquid/quoted_string_escapes.rb`), which is why a balanced inner pair survives. An unbalanced apostrophe still breaks the argument and must keep blocking. + +## How to measure it — do NOT use a proxy + +This construct was previously retracted as "the argument does not survive parsing", measured through `{% assign s = %}`. **That is a different parsing path** — `assign` has its own value parser, a `Base` tag matches `QuotedFragment`. `assign` truncates the literal to 27 characters; the tag does not. The retraction was wrong and has been reverted. + +The correct instrument, and it needs no deploy: `/api/app_builder/liquid_exec` **carries a real controller**, so `response_headers` actually sets the HTTP header on the liquid_exec response. Read `res.headers`, and diff against a no-tag baseline so a header the instance always sends is not credited to the tag. `lib/runtime.mjs`'s `probe()` discards both the status and the headers, which is why this went unseen. + +## Scope note + +Admitting this to the tolerated allowlist (`liquid-html-syntax-error/checks/tolerated-tag-markup.ts`) requires a balanced-quote predicate, which is the whole risk of this task: too loose and an unbalanced argument becomes a false approval on a security header. The allowlist mechanism and its blocking counterpart already exist — see `UnconventionalTagSyntax` and TASK-96 — so this is one shape added to an established seam, not new machinery. + + +## Acceptance Criteria + +- [x] #1 The corpus construct `{% response_headers '{ "Content-Security-Policy" : "frame-ancestors 'none'" }' %}` no longer sets must_fix_before_write, asserted end to end +- [x] #2 It still produces a diagnostic, so the unconventional spelling is advised against rather than silently accepted +- [x] #3 An UNBALANCED nested quote (`'{"X-Ae": "va'lue"}'`) still blocks, asserted in the same test file so the balance boundary cannot drift +- [x] #4 Balance is tested at more than one arity: zero nested pairs, one pair, and two pairs, each with its measured platform outcome recorded +- [x] #5 An escaped quote inside the argument, if platformOS supports one, is measured and its verdict pinned — or recorded as unmeasured and left blocking +- [x] #6 The measurement reads the actual HTTP response header and diffs against a no-tag baseline, so a header the instance always sends is never credited to the tag +- [x] #7 The buffer round-trips through prettier unchanged +- [x] #8 Deliberately reverting the change makes the new tests fail (sabotage-verified), recorded in the task notes +- [x] #9 `pos-cli check` over the 2,768-file corpus reports no offense it did not report before, other than the intended severity change on this construct + + +## Implementation Plan + + +## Plan, corrected by measurement + +**Branch:** `fix/tolerable-tag-syntax-is-not-a-blocking-error` (stacked on TASK-96; both target files were created there and are ABSENT on master). + +### The filed premise was wrong + +The description says admitting this "requires a balanced-quote predicate". Measured: **no quote-counting predicate is safe.** `'{ "X-Ae" : "a' 'b" }'` has an EVEN number of quotes and fails with HTTP 501, so a parity rule admits a construct the platform refuses — a false approval on a security header, which is precisely this task's stated risk. + +### What the platform actually does, read from source + +`app/lib/liquid/quoted_string_escapes.rb` redefines: + +```ruby +QuotedString = /"(?:\\.|[^\\"])*"|'(?:\\.|[^\\'])*'/ +QuotedFragment = /#{QuotedString}|(?:[^\s,\|'"]|#{QuotedString})+/o +``` + +So an unescaped delimiter TERMINATES the literal, and `QuotedFragment+` stops at whitespace, comma or pipe. `Base::SYNTAX` takes the first such run as the value; anything after it becomes attributes and is silently dropped. `Liquid::Expression.parse` then strips only the OUTER delimiter pair and unescapes the delimiter and backslash — a `\"` inside a `'…'` literal survives into the JSON parse. + +### The predicate + +Not syntactic. **Extract the value the platform will receive, and require that it parses as a JSON object.** That is exactly the condition `ResponseHeadersParser` needs, and it is the reason the platform 501s when it fails. + +### Differential result, 26 argument shapes + +The model was validated against the live instance, comparing BOTH the accept/reject verdict and the resulting header value: + +``` +false approvals: 0 false blocks: 0 value mismatches: 0 +``` + +Three earlier candidate models were discarded, each by a measured counterexample: quote parity (admits `"a' 'b"`, which fails), full-consumption (admits `"{'k':'v'}"`, which is not JSON), and unescape-everything (rejects `"say \"hi\""`, which works). + +### Work + +1. Generalise the allowlist in `tolerated-tag-markup.ts` from `RegExp` to a predicate function, wrapping the three existing regexes unchanged. +2. New sibling module modelling the platform's extraction (`QuotedFragment+` scan, outer-delimiter strip, delimiter unescape). It mirrors a specific platform implementation, so it is its own module with the source reference on it. +3. Admit `response_headers` when the extracted value parses as a JSON object. +4. Fixtures from the differential: valid/invalid at zero, one and two nested pairs; the unbalanced apostrophe; the escaped-quote case; a dropped-tail case; non-object JSON (`[1,2]`, a bare string); single-quoted JSON keys. +5. Sabotage each direction, asserting the mutation applied. +6. Corpus diff. Expected: `LiquidHTMLSyntaxError` 88 -> 87 and `UnconventionalTagSyntax` 34 -> 35, one occurrence, in `app/views/layouts/application.html.liquid`. + +### Known imperfection, deliberately not fixed here + +For an argument that is NOT valid JSON the verdict is correct (blocking) but the message still reads `Invalid syntax for tag 'response_headers' Expected syntax: …`, which describes the wrong problem. Rewording it needs a dedicated detector and is out of scope. + + +## Implementation Notes + + +## Implemented on `fix/tolerable-tag-syntax-is-not-a-blocking-error` + +### Built + +- `base-tag-value.ts` (new) — models what a `Base`-derived tag receives: the first `QuotedFragment+` run, outer delimiters stripped, delimiter and backslash unescaped. Mirrors `quoted_string_escapes.rb` + `base_tag_methods.rb`, cited on the module. +- `tolerated-tag-markup.ts` — allowlist generalised from `RegExp` to a predicate function; the three existing regexes wrapped unchanged. `response_headers` admitted when the extracted value parses as a JSON **object**. + +### Corpus diff + +``` +total 13065 -> 13065 files 1950 -> 1950 (unchanged) +LiquidHTMLSyntaxError 122 -> 87 (-35, was -34 after TASK-96) +UnconventionalTagSyntax 0 -> 35 (+35, was +34) +response_headers now warned in: layouts/application.html.liquid +``` + +Exactly the one occurrence the task named, in the layout every page loads. + +### Sabotage — 15 mutations, all bite + +S1/S2 predicate always true/false, S3 blocking check stops deferring, S4-S9 the six capture/case/parse_json boundaries, S10 header predicate always true, S11 drops the object guard, S12 drops the array guard, S13 skips value extraction, S14 extraction keeps the dropped tail, S15 extraction stops unescaping. Baseline 45/45. + +S15 initially did NOT bite — no fixture reached the unescape branch, because the case that needed it (`'{"k":"say \\"hi\\""}'`) turns out to be PARSED by the grammar and so never has raw markup. Two fixtures with an escaped delimiter beside an unescaped one were added, both measured; the branch is load-bearing (without it `{ "X-Ae" : "a \\'b" }` fails JSON.parse and a working construct blocks). + +The array guard was checked the same way rather than assumed: `'[{"k":"a 'b' c"}]'` IS raw markup, extracts to a valid JSON array, and the platform 501s — so the guard is reachable and pinned. + +### Verification + +Full monorepo 357 files / 4,553 tests; `type-check`, `build`, `format:check` clean. Prettier round-trip over all eight nested-quote buffers across prettier 2 and 3: **0 mangled, 0 crashed** (all whitespace-only reflow). End-to-end gate coverage added in `tolerated-tag-syntax-gate.spec.ts` for both directions. + +## Three PRE-EXISTING false approvals found, NOT fixed here + +The grammar PARSES these, so no check ever sees them and this change cannot reach them. Each is refused by the platform: + +| argument | our verdict | platform | +|---|---|---| +| `"{'X-Ae':'plain'}"` | silent | HTTP 501 | +| `'[1,2]'` | silent | HTTP 501 | +| `'not json'` | silent | HTTP 501 | + +A well-formed quoted string that is not a JSON object is approved and 501s at runtime. Pinned as `KNOWN_UNCHECKED_BY_ANY_CHECK` in the spec so the gap is visible rather than implicit, per the repo's existing idiom. Worth its own task — the same `baseTagValue` + JSON-object test would answer it, but it needs a check that runs on PARSED markup, which is a different seam from the allowlist. + +## Harness defect fixed along the way + +The sabotage script crashed mid-run and left the working tree SABOTAGED, because its self-check ran outside a `finally` and its "mutation applied" assertion was wrong for a replacement that CONTAINS the old text (prepending an early return). Rewritten: verify the file content changed rather than that the old text is gone, and restore in `finally` with a final assertion that the tree matches the backup. It also correctly reported two stale patterns instead of silently passing — the guard added in TASK-96 doing its job. + diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/base-tag-value.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/base-tag-value.ts new file mode 100644 index 00000000..8bb866f2 --- /dev/null +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/base-tag-value.ts @@ -0,0 +1,81 @@ +/** + * What value a `Liquify::Tags::Base`-derived tag actually receives for given raw markup. + * + * Mirrors two pieces of platformOS, and is only correct while they are: + * + * app/lib/liquid/quoted_string_escapes.rb + * QuotedString = /"(?:\\.|[^\\"])*"|'(?:\\.|[^\\'])*'/ + * QuotedFragment = /QuotedString|(?:[^\s,\|'"]|QuotedString)+/ + * app/lib/liquify/tags/base_tag_methods.rb + * SYNTAX matched UNANCHORED; group 1 is the value, the rest becomes attributes. + * + * Two consequences drive everything here. An unescaped delimiter TERMINATES a literal, so + * `'a'b'` is three tokens rather than one string; and a run ends at whitespace, comma or + * pipe, so anything after it is silently dropped rather than reported. + * + * Validated against a live instance over 26 argument shapes, comparing both the accept/reject + * verdict and the resulting value: 0 false approvals, 0 false blocks, 0 value mismatches. + * Three simpler models were discarded, each by a counterexample — quote parity admits + * `'{ "k" : "a' 'b" }'` which fails; full-consumption admits `"{'k':'v'}"` which is not JSON; + * unescaping every `\x` rejects `'{"k":"say \"hi\""}'` which works. + */ + +/** End index of the literal opening at `start`, or -1 when it is unterminated. */ +function literalEnd(markup: string, start: number): number { + const quote = markup[start]; + let i = start + 1; + while (i < markup.length) { + if (markup[i] === '\\') { + i += 2; + continue; + } + if (markup[i] === quote) return i + 1; + i += 1; + } + return -1; +} + +/** The first `QuotedFragment+` run — what `Base::SYNTAX` captures as the value. */ +function firstRun(markup: string): string { + let i = 0; + while (i < markup.length) { + const char = markup[i]; + if (char === "'" || char === '"') { + const end = literalEnd(markup, i); + if (end < 0) break; + i = end; + continue; + } + if ( + char === ' ' || + char === '\t' || + char === '\n' || + char === '\r' || + char === ',' || + char === '|' + ) + break; + i += 1; + } + return markup.slice(0, i); +} + +/** + * The value the tag receives, or `undefined` when the markup yields none. + * + * A run wrapped in matching delimiters is a string literal, so only the OUTER pair is + * removed. Only the delimiter and backslash are unescaped: a `\"` inside a `'…'` literal is + * not an escape and must survive, which is what lets `'{"k":"say \"hi\""}'` reach a JSON + * parser intact. + */ +export function baseTagValue(markup: string): string | undefined { + const run = firstRun(markup.trim()); + if (run.length === 0) return undefined; + + const quote = run[0]; + const isLiteral = (quote === "'" || quote === '"') && run.length >= 2 && run.endsWith(quote); + if (!isLiteral) return run; + + const inner = run.slice(1, -1); + return inner.replace(quote === "'" ? /\\(['\\])/g : /\\(["\\])/g, '$1'); +} diff --git a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts index 1608e62d..1a1276a4 100644 --- a/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts +++ b/packages/platformos-check-common/src/checks/liquid-html-syntax-error/checks/tolerated-tag-markup.ts @@ -1,4 +1,5 @@ import { LiquidTag, NamedTags } from '@platformos/liquid-html-parser'; +import { baseTagValue } from './base-tag-value'; /** * Tag markup the grammar refuses that platformOS parses AS INTENDED, measured on a live @@ -29,10 +30,35 @@ const CASE_TRAILING_COLON = /^\s*[A-Za-z_][\w-]*(?:\.[\w-]+)*\s*:+\s*$/; /** `{% parse_json v %%}` — a stray `%` the unanchored SYNTAX never reaches. A name is required. */ const PARSE_JSON_TRAILING_PERCENT = /^\s*[A-Za-z_][\w-]*\s*%+\s*$/; -const TOLERATED: Partial> = { - [NamedTags.capture]: CAPTURE_QUOTED_TARGET, - [NamedTags.case]: CASE_TRAILING_COLON, - [NamedTags.parse_json]: PARSE_JSON_TRAILING_PERCENT, +/** + * `{% response_headers '{ "K" : "a 'b' c" }' %}` — nested quotes. + * + * NOT a shape test. No syntactic rule works here: `'{ "K" : "a' 'b" }'` has an even number of + * quotes and fails with HTTP 501, so quote parity admits what the platform refuses. The tag + * needs one thing — an argument that parses as a JSON object — so that is what is checked, on + * the value {@link baseTagValue} says the tag will actually receive. + * + * Measured over 26 argument shapes against a live instance: 0 false approvals, 0 false blocks. + */ +function isParseableHeaderJson(markup: string): boolean { + const value = baseTagValue(markup); + if (value === undefined) return false; + let parsed: unknown; + try { + parsed = JSON.parse(value); + } catch { + return false; + } + return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed); +} + +const matches = (shape: RegExp) => (markup: string) => shape.test(markup); + +const TOLERATED: Partial boolean>> = { + [NamedTags.capture]: matches(CAPTURE_QUOTED_TARGET), + [NamedTags.case]: matches(CASE_TRAILING_COLON), + [NamedTags.parse_json]: matches(PARSE_JSON_TRAILING_PERCENT), + [NamedTags.response_headers]: isParseableHeaderJson, }; /** @@ -41,8 +67,8 @@ const TOLERATED: Partial> = { */ export function isToleratedTagMarkup(node: LiquidTag): boolean { if (typeof node.markup !== 'string') return false; - const shape = TOLERATED[node.name]; - return shape !== undefined && shape.test(node.markup); + const admits = TOLERATED[node.name]; + return admits !== undefined && admits(node.markup); } export const TAGS_WITH_TOLERATED_MARKUP: readonly string[] = Object.keys(TOLERATED); diff --git a/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts index 2f100bc0..cc494025 100644 --- a/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts +++ b/packages/platformos-check-common/src/checks/unconventional-tag-syntax/index.spec.ts @@ -32,49 +32,121 @@ const TOLERATED = [ what: 'parse_json, as prettier reprints it', source: `{% parse_json d % %}{"k":2}{% endparse_json %}`, }, + + // response_headers: admitted when the value the tag RECEIVES parses as a JSON object. + // Every row below was rendered on the instance and its header value read back. + { + what: 'response_headers, one nested pair (the corpus construct)', + source: `{% response_headers '{ "Content-Security-Policy" : "frame-ancestors 'none'" }' %}`, + }, + { + what: 'response_headers, two nested pairs', + source: `{% response_headers '{ "X-Ae" : "a 'b' c 'd' e" }' %}`, + }, + { + what: 'response_headers, nested pair with nothing around it', + source: `{% response_headers '{ "X-Ae" : "'x'" }' %}`, + }, + { + what: 'response_headers, adjacent empty pair', + source: `{% response_headers '{"X-Ae":"''"}' %}`, + }, + { + // Escaped delimiter alongside an unescaped one, so the grammar leaves raw markup AND the + // unescape matters: without it the value holds `\\'`, an invalid JSON escape, and a + // working construct would be blocked. Measured: header `a 'b`. + what: 'response_headers, escaped delimiter beside an unescaped one', + source: `{% response_headers '{ "X-Ae" : "a \\'b" }' %}`, + }, + { + // Measured: header `it's 'q' ok`. + what: 'response_headers, escaped and unescaped delimiters mixed', + source: `{% response_headers '{"X-Ae":"it\\'s 'q' ok"}' %}`, + }, + { + // The run ends at the space, so ` extra` is dropped — but what the tag receives is still + // valid JSON, and the platform sets the header. Admitted for that reason, not by accident. + what: 'response_headers, a dropped tail that leaves valid JSON behind', + source: `{% response_headers '{"X-Ae":"x"}' extra %}`, + }, ]; -/** Must keep blocking: each either raises on the platform, or runs while doing the wrong thing. */ +/** + * Must keep blocking. Three reasons appear below, and they are NOT interchangeable: + * + * RAISES the platform refuses it. + * WRONG it runs and does something other than what was written — the dangerous class. + * NOT ADMITTED it runs correctly, and is still refused. The allowlist is deliberately + * minimal: a shape earns a place by occurring in real code, not merely by + * working. Each of these is a knowingly-accepted false block on a construct + * with zero corpus occurrences; widen with data, not with sympathy. + */ const MUST_STILL_BLOCK = [ { - what: 'cache with a leading colon — key collapses to ":" and is shared instance-wide', + what: 'WRONG: cache with a leading colon — the key collapses to ":" and is shared instance-wide', source: `{% cache: k, expire: 30 %}BODY{% endcache %}`, }, { - what: 'cache with the key omitted — collapses to "expire:" with no leading separator', + what: 'WRONG: cache with the key omitted — collapses to "expire:", with no leading separator', source: `{% cache expire: 30 %}BODY{% endcache %}`, }, - { what: 'log with a leading colon — the message becomes ":"', source: `{% log: o, type: 'E' %}` }, { - what: 'response_headers whose nested quotes truncate the argument', - source: `{% response_headers '{ "CSP" : "frame-ancestors 'none'" }' %}`, + what: 'WRONG: log with a leading colon — the message becomes ":" and the payload is lost', + source: `{% log: o, type: 'E' %}`, + }, + { + // Measured HTTP 501: the run ends at the unescaped quote inside `va'lue`, so the tag gets + // `{"X-Ae": "va` — not valid JSON. Quote PARITY would admit this class; JSON validity does + // not, which is why the predicate is semantic rather than syntactic. + what: 'WRONG: response_headers whose unbalanced apostrophe truncates the JSON', + source: `{% response_headers '{"X-Ae": "va'lue"}' %}`, + }, + { + // Measured HTTP 501. EVEN number of quotes, and still broken — the counterexample that + // killed the quote-parity predicate. + what: 'WRONG: response_headers with an even quote count that still truncates', + source: `{% response_headers '{ "X-Ae" : "a' 'b" }' %}`, + }, + { + // Measured HTTP 501: raw markup, and the extracted value IS valid JSON — but an array is + // not a header map. This is what makes the object check load-bearing rather than defensive. + what: 'WRONG: response_headers whose value is a valid JSON array', + source: `{% response_headers '[{"k":"a 'b' c"}]' %}`, }, { - what: 'capture with empty markup — the platform raises', + what: 'RAISES: capture with empty markup', source: `{% capture %}x{% endcapture %}`, }, { - what: 'capture with a space inside the quotes — the regex would take only `a`', + // Measured: `[a=HI][b=]` — it captures into `a` and silently drops ` b`. + what: 'WRONG: capture with a space inside the quotes captures into `a` and drops the rest', source: `{% capture 'a b' %}x{% endcapture %}`, }, { - what: 'capture with a trailing token — which of the two is the target is ambiguous', + // Measured: `[cs=HI][extra=]` — it captures into `cs`. Which token was meant is unknowable. + what: 'WRONG: capture with a trailing token — the second token is silently dropped', source: `{% capture 'cs' extra %}x{% endcapture %}`, }, { - what: 'capture with a digit-leading quoted name — unmeasured, so not admitted', + // Measured with a readable name (`'1x'`): it captures into `1x` and works. An all-digit + // name cannot be read back at all — `{{ 123 }}` is a numeric literal, not a lookup — so + // the earlier "unmeasured" note was replaced by this, not by an admission. + what: 'NOT ADMITTED: capture with a digit-leading quoted name, which runs correctly', source: `{% capture '123' %}x{% endcapture %}`, }, { - what: 'case with a colon and no name — looks up nil, every when misses', + // Measured: renders the `else` branch. VariableLookup gets a nil name, so every `when` + // misses and control falls through with no error. The dangerous case in this family. + what: 'WRONG: case with a colon and no name silently takes the else branch', source: `{% case : %}{% when 1 %}ONE{% endcase %}`, }, { - what: 'case with a colon then another token', + // Measured: takes the correct branch. Refused only because nothing writes it. + what: 'NOT ADMITTED: case with a colon then another token, which runs correctly', source: `{% case g : : %}{% when 1 %}ONE{% endcase %}`, }, { - what: 'parse_json with a percent and no name — the platform raises', + what: 'RAISES: parse_json with a percent and no name', source: `{% parse_json %%}{"k":2}{% endparse_json %}`, }, ]; @@ -86,6 +158,20 @@ const WELL_FORMED = [ `{% parse_json d %}{"k":2}{% endparse_json %}`, `{% cache 'k', expire: 30 %}BODY{% endcache %}`, `{% log o, type: 'E' %}`, + `{% response_headers '{"X-Ae":"plain"}' %}`, + `{% response_headers '{"X-Ae":"say \\"hi\\""}' %}`, +]; + +/** + * Arguments the GRAMMAR parses, so neither check runs — and the platform still refuses them + * with HTTP 501. A pre-existing false approval, unrelated to this check and unaffected by it: + * these never had raw markup, so the split cannot reach them. Pinned so the gap stays visible + * rather than implicit; tracked separately. + */ +const KNOWN_UNCHECKED_BY_ANY_CHECK = [ + { what: 'single-quoted JSON keys', source: `{% response_headers "{'X-Ae':'plain'}" %}` }, + { what: 'a JSON array, not an object', source: `{% response_headers '[1,2]' %}` }, + { what: 'not JSON at all', source: `{% response_headers 'not json' %}` }, ]; async function bothChecks(source: string) { @@ -147,7 +233,15 @@ describe('UnconventionalTagSyntax', () => { const [offense] = await runLiquidCheck(UnconventionalTagSyntax, source); covered.add(/\{%\s*([a-z_]+)/.exec(offense.message.replace(/^`/, ''))?.[1] ?? ''); } - expect([...covered].sort()).toEqual(['capture', 'case', 'parse_json']); + expect([...covered].sort()).toEqual(['capture', 'case', 'parse_json', 'response_headers']); + }); + + describe('is silent where the grammar already parsed the markup', () => { + it.each(KNOWN_UNCHECKED_BY_ANY_CHECK)('$what', async ({ source }) => { + const { unconventional, invalidTagSyntax } = await bothChecks(source); + expect(unconventional).toEqual([]); + expect(invalidTagSyntax).toEqual([]); + }); }); it('does not report inside {% raw %}, whose body the parser keeps as text', async () => { diff --git a/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts b/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts index 36af2cb0..28e20ce1 100644 --- a/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts +++ b/packages/platformos-mcp-supervisor/src/result/tolerated-tag-syntax-gate.spec.ts @@ -48,6 +48,12 @@ const TOLERATED = [ what: 'parse_json with a stray percent', source: `{% parse_json d %%}{"k":2}{% endparse_json %}`, }, + { + // The construct from a real application layout. Measured: the platform sets + // `Content-Security-Policy: frame-ancestors 'none'` correctly. + what: 'response_headers with nested quotes that leave valid JSON', + source: `{% response_headers '{ "Content-Security-Policy" : "frame-ancestors 'none'" }' %}`, + }, ]; /** Measured to raise, or to run while doing something other than what was written. */ @@ -56,6 +62,11 @@ const BLOCKED = [ { what: 'cache with the key omitted', source: `{% cache expire: 30 %}B{% endcache %}` }, { what: 'log with a leading colon', source: `{% log: o, type: 'E' %}` }, { what: 'capture with empty markup', source: `{% capture %}x{% endcapture %}` }, + { + // Measured HTTP 501: the run stops at the unescaped quote, so the tag gets truncated JSON. + what: 'response_headers whose argument truncates to invalid JSON', + source: `{% response_headers '{"X-Ae": "va'lue"}' %}`, + }, ]; describe('The write gate lets tolerated tag syntax through', () => {