Repository navigation
fix(diagrams): preserve literal SVG comments in CDATA (FU-1015) - #1835
Conversation
Exercise the public stripper with literal expected SVG strings and retain the observed removal behavior for malformed comment openers. Test-First: docs/diagrams/strip-comments.test.mjs::stripComments preserves literal comment text in CDATA Test-First: docs/diagrams/strip-comments.test.mjs::stripComments retains removal behavior outside CDATA Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Protect original CDATA spans before applying the existing fixed-point comment removal to surrounding markup. Ignore CDATA-looking text in real comments, and keep removing openers assembled by earlier removals. Test-First: docs/diagrams/strip-comments.test.mjs::stripComments preserves literal comment text in CDATA Test-First: docs/diagrams/strip-comments.test.mjs::stripComments retains removal behavior outside CDATA Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Cuarroc
added
the
review-ok
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 417eec28c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,8 +1,21 @@ | |||
| // Separate from export-svg.mjs so tests can import it without Chromium or argv. | |||
| export function stripComments(s) { | |||
There was a problem hiding this comment.
When this implementation lands, docs/PLAN.md:365 will still describe FU-1015 as “Geplant: nicht disponiert,” even though this commit implements its #1581 K1 follow-up. Update the row in this commit so the canonical plan reflects the post-merge state; the repository explicitly treats this mismatch as a finding that must be resolved before acceptance.
AGENTS.md reference: AGENTS.md:L139-L139
Useful? React with 👍 / 👎.
| @@ -1,8 +1,21 @@ | |||
| // Separate from export-svg.mjs so tests can import it without Chromium or argv. | |||
| export function stripComments(s) { | |||
There was a problem hiding this comment.
Before accepting or merging this Tier-B change, obtain and record the required second independent review. The commit’s review disposition names only one non-author reviewer (glm-5.2:cloud) before declaring acceptance, while Tier B requires two reviewers from different non-author families, using the documented subscription/OpenRouter pairing or its E1 substitution.
AGENTS.md reference: AGENTS.md:L28-L33
Useful? React with 👍 / 👎.
| const parts = []; | ||
| let start = 0; | ||
| // Match comments too, so CDATA-looking text inside a comment is not protected. | ||
| for (const match of s.matchAll(/<!\[CDATA\[[\s\S]*?\]\]>|<!--(?:[\s\S]*?-->)?/g)) { |
There was a problem hiding this comment.
For a valid SVG such as <svg><?probe <![CDATA[ ?><!-- remove --><![CDATA[x]]></svg>, the matcher treats the text inside the processing instruction as a real CDATA opener and consumes through the later genuine CDATA terminator. Consequently <!-- remove --> is returned unchanged even though it is an actual markup comment; processing instructions must be consumed as opaque regions before recognizing CDATA starts.
Useful? React with 👍 / 👎.
|
Root 14:55Z: CI re-trigger only (no code change, head stays 417eec2). Attempt 1 of run 39790632897 hung 52 min in setup-linux and was cancelled; the attempt-2 rerun ended startup_failure, because the rerun of the cancelled run also re-ran the main-only main-red-guard job. A reopen starts a fresh pull_request run on the same head; review-ok stays bound to 417eec2. |
Cuarroc
removed
the
review-ok
|
@Mergifyio dequeue |
Merge Queue Status
This pull request spent 13 minutes 36 seconds in the queue, with no time running CI. ReasonPull request #1835 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Waiting for
All conditions
Requeued — the merge queue status continues in this comment ↓. |
Assert literal public API output for CDATA-looking processing instruction content before real comments and genuine CDATA. Regression-For: 417eec2 Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Consume processing instructions as opaque spans before recognizing CDATA. Remove following markup comments and preserve genuine CDATA bytes. Reconcile only the FU-1015 plan row with the partial delivery and outstanding siblings. Regression-For: 417eec2 Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cf8738d1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parts = []; | ||
| let start = 0; | ||
| // Consume processing instructions and comments before interpreting CDATA. | ||
| for (const match of s.matchAll(/<\?[\s\S]*?\?>|<!\[CDATA\[[\s\S]*?\]\]>|<!--(?:[\s\S]*?-->)?/g)) { |
There was a problem hiding this comment.
When the input is a valid SVG with an internal DTD subset, such as <!DOCTYPE svg [<!ENTITY marker "<![CDATA[">]><svg><!-- remove --><![CDATA[x]]></svg>, this matcher treats the quoted entity value as a real CDATA opener and consumes through the document's genuine CDATA terminator. As a result, <!-- remove --> survives unchanged; lex the DOCTYPE so quoted values cannot open CDATA, and add this sibling case to the regression coverage.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
Import the unchanged stripper test module from the existing export entrypoint. Preserve historical Test-First paths and all assertions. No-Test: test wiring only, assertions unchanged Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Cuarroc
added
the
review-ok
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 5 minutes 45 seconds in the queue, including 5 minutes 7 seconds running CI. ReasonThe merge conditions cannot be satisfied due to failing checks
Failing checks: HintYou may have to fix your CI before adding the pull request to the queue again. Waiting for
All conditions
Requeued — the merge queue status continues in this comment ↓. |
|
@Mergifyio requeue |
Merge Queue Status
This pull request spent 11 minutes 11 seconds in the queue, including 10 minutes 26 seconds running CI. Required conditions to merge
|
Die drei vorhandenen Stripper-Tests laufen jetzt im bestehenden Diagramm-Export-Gate mit.
Ihre Datei, Testnamen und Assertions sind unverändert erhalten.
Die vollständige Prepush-Bahn und der Push-Hook sind grün; unabhängiges Delta-Review, Roots Abnahme und Merge stehen noch aus.
Report
Plan-ID: FU-1015
Current test-wiring correction (2026-10-22)
Review: ausstehend (Stufe B). This worker launches no reviews. Root owns the bounded
9cf8738d..fc1d4662delta review by glm-5.3 and deepseek-v4.1-flash (round two, last), the GPT observer workflow, acceptance and Mergify. The parent FU-1015 goal stays open.Predecessor / correction base:
9cf8738d1b5afc7a597697b3cfa193c9898ca9a8, verified clean on the assigned branch before editing.Final checked / pushed head:
fc1d4662856fa7cb3234010d1f73c4428ad6e9ac.Observed main tip:
b0d92b112ec77cc396aeb34079aaf62f70348496(fetch at entry and remote read after publication).Actual package merge-base:
88c10c7716ad2dca629930adbcd8a291baf5d858; no freshness merge or history rewrite.Raw package size: 78 additions + 2 deletions = 80 lines, four files, including all prior implementation, tests and PLAN changes. This correction adds exactly one line in one file.
Change: Add
import './strip-comments.test.mjs';todocs/diagrams/export-svg.test.mjs. No other tracked file changed in this correction. Runtime stripper code, PLAN, all existing tests/options/assertions, CI scripts, fixtures, dependencies and generated SVGs are unchanged. Architecture drift: existing gate passed, no new finding or allowlist change.Method adaptation / preserved history: Root accepted R1835-G2 / R1835-D3 because the canonical
diagrams-exportgate runs onlynode --test docs/diagrams/export-svg.test.mjs. The original proposal was to move the cases and delete their separate file. Inspection found inheritedTest-First: docs/diagrams/strip-comments.test.mjs::<name>trailers in botha3a1eab6and417eec28; stock red-first validates their HEAD paths even in plan mode. Deletion would invalidate mandatory historical evidence. The authorized replacement is this static side-effect import: the same tests register under the existing entrypoint, without copying, moving, deleting, filtering or adding tests. Remaining acceptance is independent delta closure and Root acceptance; no gate change or acceptance relaxation was made.Byte proof:
docs/diagrams/strip-comments.test.mjsSHA-256 before and after is6a40888d37269b2ae0d2e0112d47ac03f0dd369faa2d3c6e97214b6302523bcd. Exact-byte comparison against9cf8738dpassed. A literal diff assertion confirms the export suite differs only by the import.Canonical gate proof: Both the explicit full prepush and the normal push hook printed and passed all three names below, together with the six existing export-suite tests: 9 passed, 0 failed, 0 skipped. Only the canonical export entrypoint was invoked for this proof; the two test files were not supplied together.
stripComments treats processing instructions as opaque before CDATAstripComments preserves literal comment text in CDATAstripComments retains removal behavior outside CDATAgit diff --checknode --test docs/diagrams/export-svg.test.mjsbefore commitbash scripts/ci/secret-scan.shbefore the normal commitgit commit -F <message-file>No-Test: test wiring only, assertions unchanged, observed-model coauthor.bash scripts/ci/red-first.sh --planat final headcount=6,specs=4; inherited paths remain valid.bash scripts/ci/red-first.shat final headbash scripts/ci/gates.sh lane prepushat final head in the assigned worktreediagrams-exportincludes all three names, 9/9 pass. Rust: 2,320 passed, 7 existing skips.git push -u origin codex/fu1015-svg-commentsgit ls-remote origin codex/fu1015-svg-commentsfc1d4662856fa7cb3234010d1f73c4428ad6e9ac.Admission evidence: Fresh start-check returned 0 before every heavy phase; adjacent one-minute loads were 1.50 (targeted browser suite), 1.73 (commit hooks), 3.09 (full prepush) and 4.89 (push hook), all below 8. MemAvailable exceeded 14,300 MiB; Cargo count was zero at each admission. Commands were awaited in the foreground. The predecessor conflict worker's log ends with
JOB-EXIT=0, corroborated by its dated completion event before this run. Earlier operational caveats below remain historical and unresolved by this evidence, including the earlier push at load 16.00 and the predecessor's missing commit-adjacent load sample.NICHT ABGEDECKT — current correction
#[cfg(windows)]tests or Windows clippy branches; the Windows lane runs in the merge queue.lage-jetztmirror were unavailable; bounded exact-source inspection was used. Offline PLAN checking does not establish global online plan freshness. This correction has no PLAN ownership and makes no PLAN edit.Current accepted-finding disposition and handover
fc1d4662; all three unchanged cases pass under the canonical gate. Independent closure pending Root's last delta review.No unrelated disposition was changed. PR ready/draft state, labels, queue and merge actions were left as found. The worker's bounded native goal covers this checked handover only; Root owns all remaining parent duties. All prior RED evidence, review findings/dispositions and resource caveats are preserved below as historical records bound to their stated heads.
Historical correction report and Root dispositions (preserved from the predecessor)
Plan-ID: FU-1015
Package: FU-1015 / R1581-K1, accepted correction on existing PR #1835. Tier: B.
Goal / benefit: Fix the public
stripCommentsregression introduced at417eec28: a CDATA-looking opener inside a processing instruction must not protect a later real XML comment. Preserve original PI and genuine CDATA bytes and existing malformed-comment behavior.Parent / owner: Ticket
3913f1fa-1ba7-448d-baa6-e188dc26b544, same execution owner; Root owns qualified reviews, acceptance and Mergify. The worker's nativeget_goalreturned null on entry;create_goalcreated the bounded delivery goal, and milestoneget_goalcalls confirmed it active during implementation/publication. No predecessor goal was replaced.Scope: Only
docs/diagrams/strip-comments.mjs, its test file, and the existing FU-1015 row indocs/PLAN.md. No new dependencies, generated SVG changes, export-test edits, app launch, review launch or merge action.Observed PR base tip:
main@9cc16b859fa2ec6be20ab95185a32b460a710376.Actual merge-base:
88c10c7716ad2dca629930adbcd8a291baf5d858; the original report's base is historical, not a live main-tip claim.Reviewed predecessor:
417eec28c40acde3a83a31059dd96d612dfca354.RED-test commit:
9208233c0c67a25c7dd3b71df32001a9464d5b57.Fix / checked / remote head:
9cf8738d1b5afc7a597697b3cfa193c9898ca9a8.Raw diff against merge-base: 77 additions + 2 deletions = 79 lines, three files, including the prior 59 lines. Fix-round delta against
417eec28: 26 raw lines.What changed and test boundary
<?...?>processing instructions as opaque spans before interpreting CDATA. The existing fixed-point comment removal still handles surrounding markup.72ae3f75); K4/K5 separately pending fix(ci): accept equivalent step indentation and cover guards #1839; unresolved feat(settings): add a brief changelog to the version section (KR-09m) #1551/feat(terminal): use design system search controls (KR-08a) #1556/ci: split Linux gates and isolate native Windows tests #1560/feat(design): complete motion tokens and layout-animation contract (DS-01) #1561/feat(leitstand): replace Fluss with Fabrik and preserve old links #1568/feat(leitstand): mount orchestrator chat in Verlauf (KR-04a) #1569 and the existing dependencies remain. The umbrella is not DONE. Live GitHub confirmed test(diagrams): pin exact SVG fixture output (FU-1015) #1833 merged and fix(ci): accept equivalent step indentation and cover guards #1839 open.Evidence (Linux, 2026-10-22)
node --test docs/diagrams/strip-comments.test.mjs, added regression before changing production code at417eec28<svg><?probe <![CDATA[ ?><!-- remove --><![CDATA[x]]></svg>versus expected<svg><?probe <![CDATA[ ?><![CDATA[x]]></svg>; 2 existing tests pass. Committed as9208233c.node --test docs/diagrams/strip-comments.test.mjsat final headnode --test docs/diagrams/strip-comments.test.mjs docs/diagrams/export-svg.test.mjsafter the fixgit show 88c10c77:docs/diagrams/strip-comments.mjsbash scripts/ci/red-first.sh --plancount=6,specs=4(inherited duplicate Test-First trailers plus two Regression-For trailers).bash scripts/ci/red-first.shRegression-For: 417eec28c40acde3a83a31059dd96d612dfca354resolves to a head ancestor.bash scripts/ci/secret-scan.shbefore each of the two normal commits, also repeated by their hooksgit commithooks for9208233cand9cf8738dbash scripts/ci/gates.sh lane prepushin the assigned original worktree at9cf8738dgit push -u origin codex/fu1015-svg-commentsgit ls-remote origin codex/fu1015-svg-comments9cf8738d1b5afc7a597697b3cfa193c9898ca9a8.gitleaks git --config .gitleaks.toml --redact --log-opts=88c10c7716ad2dca629930adbcd8a291baf5d858..HEAD .git diff --check;git status --short; own-row-only PLAN diff assertionBoth new commits truthfully use
Regression-For, with no new Test-First claim against the frozen merge-base. Originala3a1eab6/417eec28evidence remains historical: original CDATA regression RED, then eight targeted/integration tests GREEN; the prior report's full prepush and publication were bound to417eec28and are superseded by the checks above.NICHT ABGEDECKT
#[cfg(windows)]tests or Windows clippy branches; the Windows lane runs in the merge queue.lage-jetztmirror were unavailable. Bounded exact-source/caller inspection was used. The offline PLAN gate plus bounded live own-row / test(diagrams): pin exact SVG fixture output (FU-1015) #1833 / fix(ci): accept equivalent step indentation and cover guards #1839 checks do not prove global online PLAN freshness.Current findings and review disposition
Review: ausstehend (Stufe B). Root commissions two qualified non-author families (
glm-5.3or permittedglm-5.3-flash, plusdeepseek-v4.1-flash) and the GPT observer. Historical glm-5.2 is NOT eligible and does not count. No independent QA/delegation/reviewer was launched by this worker.9cf8738d, compiling candidate RED committed first in9208233c; public literal expectations now pass. Closure remains subject to Root's review of417eec28..9cf8738d.9cf8738d, only the existing FU-1015 row changed; unresolved siblings retained. Independent closure pending.This is the accepted-finding correction delta
417eec28..9cf8738d; Root owns the bounded round-two verification and final merge/split/drop decision. No readiness/draft toggling, labels,review-ok, queue, merge or new PR action was taken.Checked handover
Implemented, tested, pushed and reported at the final head. Qualified independent review, Root acceptance, merge and end-user usability acceptance remain open. Next owner: Root for review/acceptance/Mergify; the dispatcher releases the sole PLAN editing lane after this worker ends, then #1839 may proceed. If it later conflicts on the same row, Root coordinates a genuine conflict merge; no freshness merge/rebase was performed. The parent goal remains open.
Historical review record (superseded; preserved verbatim)
Review disposition (Root, 2026-10-22 ~13:55Z, head 417eec2)
Tier B (scripts/docs code with test). Author family OpenAI (Codex gpt-6.1-sol). Reviewer: glm-5.2:cloud (Zhipu) via
scripts/review/run-local.sh 1835 --via ollama, verdict approve with conditions.strip-comments.test.mjs:38expected value wrong, test would failnode --test docs/diagrams/strip-comments.test.mjsexit 0, 2/2 pass. The reviewer's trace misses the repeat-until-stable loop.<!-- <![CDATA[x]]>now protected<![CDATA[x]]>); only the<!--opener is removed in both.Root decision: accept; no open high finding.
review-okbound to 417eec2.Review disposition, amendment (Root, date -u ~15:25Z, head 417eec2) — acceptance WITHDRAWN
Root removed
review-ok, which takes the PR out of the queue. Root had applied an outdated tier-B rule.AGENTS.mdat main 9cc16b8, rule 5 plus lines 133-137, requires two non-author families via the approved Ollama credit (glm-5.3,glm-5.3-flash,deepseek-v4.1-flash), a GPT observer, and the PLAN row updated in the same PR. glm-5.2 is not on that list, so the earlier review does not count.docs/PLAN.md:365) stalestripComments('<svg><?probe <![CDATA[ ?><!-- remove --><![CDATA[x]]></svg>')returns the input unchanged, so<!-- remove -->survives (the base removed it). Fix: treat processing instructions as opaque, with a red test first.Next: NACHBESSERN on this branch (Stabschef dispatches), then the review of the delta and the new head by glm-5.3 and deepseek-v4.1-flash.
Review — Root disposition (2026-10-22 ~18:10Z, head 9cf8738, base origin/main b0d92b1)
Tier B. Reviewers outside the author family (OpenAI): glm-5.3 (Zhipu) "reject" (based only on G1, rejected below), deepseek-v4.1-flash (DeepSeek) approve with conditions.
<![CDATA[<!-- keep -->]]>); Root rannode --test docs/diagrams/strip-comments.test.mjson the head files: exit 0, pass 3, fail 0<?x > <!-- secret --> ?>unchanged); unsafe if SVG is inlined into HTML<?before CDATA-like text keeps the commentdocs/diagrams/strip-comments.test.mjsis run by no gate (gates.sh:164 runs only export-svg.test.mjs)diagrams-exportgate (move them into export-svg.test.mjs, no scripts/ci change), red-first irrelevant (test wiring), prove with gate outputAfter the NACHBESSERN: one delta review of the changed test file by glm-5.3 + deepseek-v4.1-flash (round 2, last).
NACHBESSERN delta — Root, 2026-10-22T18:38Z, 9cf8738..fc1d466
docs/diagrams/export-svg.test.mjsimports./strip-comments.test.mjs, so the gated commandnode --test docs/diagrams/export-svg.test.mjs(gates.sh:164, diagrams-export) now runs the stripper cases. Root ran it on the head files: exit 0, tests 9, pass 9, fail 0, incl. the three stripComments tests.Acceptance — Root, 2026-10-22T18:43Z, head fc1d466
<!DOCTYPE svg [<!ENTITY m "<![CDATA[">]>…) is treated as real CDATA, so a later comment survivesPR checks on fc1d466: all green (gh pr checks --watch exit 0, incl. CodeQL and red-first). Tier B: glm-5.3 + deepseek-v4.1-flash on 9cf8738, delta fc1d466 tier C (test wiring). No open high. Root decision: merge.