Repository navigation
harness v2: spec-compliant parser, suites + CommonMark conformance gate in CI - #2
Conversation
…ites + conformance gate in CI The 27 review rounds on PR #1 were all Markdown-parsing edge cases in a hand-rolled parser. This replaces that parser with markdown-it-py (CommonMark 0.31.2 preset) and compares the two copies of each lesson by their rendered, visibility-normalised HTML. - harness/check-knowledge.py: 1,345 -> 551 lines; same check names, messages and CLI contract - harness/knowledge_md.py (37 lines): the one parser, with two patches that align markdown-it-py 4.0.0 with CommonMark 0.31.2: link labels capped at 999 characters; declaration blocks start at any ASCII letter and HTML-block patterns take only space/tab as whitespace (not NBSP) - harness/conformance/: differential gate. The parser must render all 652 spec examples and 36 edge cases like commonmark.js 0.31.2 (the reference); two reviewed exceptions where commonmark.js departs from the spec text on NBSP (cmark and cmark-gfm agree with the spec) - harness/tests/: the 199 negative-test cases from the PR #1 review, repo-relative, plus runner; four assertions corrected where the reference proves the old reading wrong (neg18 5, neg19 1, neg19 10, neg22 F1), two replacement cases added -> 201 cases - CI: dependency pin, suites job, conformance job - README: lesson feed documented as paused (retired 2026-09-27), harness layout, how to run; CONTRIBUTING: harness change rules, ledger open->merged convention - verified/2026-09-26_claude-surfaces_v3.md: proof repo renamed to cloud-launch-pad; v2 bannered - LEDGER: PR #1 rows flipped to merged (7946224), this PR's row Evidence (local): check-knowledge.py RESULT: PASS — 12 pass, 0 warn, 0 fail; run.sh TOTAL: PASS — 17 suites, 201 cases, 0 failing; conformance RESULT: PASS — 652/652 spec, 34/36 cases + 2 reviewed exceptions; mutation check: 6/6 deliberately broken checkers caught (1–70 cases red each); markdownlint-cli2 0.23.3: 0 issues. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @lookathim010-source, your pull request is larger than the review limit of 150,000 diff characters
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
![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: 78d12a9eca
ℹ️ 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".
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78d12a9eca
ℹ️ 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".
…ields, comments in labels, --json without deps, split confidence word) Five Codex findings on 78d12a9, each reproduced first (harness/tests/neg28.sh failed 8 of 17 cases before this change, passes 17 of 17 after): - visible_html() shortened the string that PROTECT_RE.finditer was walking, so after a <pre> every later code span was sliced at stale offsets and the text right after it dropped out of the comparison key (`y`A == `y`B). The whitespace after </pre> is now stripped from the next segment instead. - A field whose only content is invisible (`Why it matters here: <!-- x -->`, or a comment block under `Do:`) counted as non-empty. Emptiness now comes from the visible text after the label and from visible blocks only. - `Why<!-- note --> it matters here:` renders as the label but was not recognised. Labels now match the paragraph's leading plain text with invisible raw HTML skipped; code, images or other markup still break it. - `--json` printed text lines when markdown-it-py was missing. The deps error now honours the mode, and a parser that fails to load for any reason (incompatible release) is reported the same way. - confi`code`dence 80% was read as a confidence label because code spans were dropped from the text after the link. Visible non-text content (code, images, displayed raw HTML) now splits words; formatting delimiters and invisible markup stay zero-width. Reference first: six new cases in harness/conformance/cases.json pin the parses these tests rely on (commonmark.js 0.31.2 agrees on all six). Also: shellcheck + pyflakes CI job (.shellcheckrc documents the two exclusions; run.sh gets `cd … || exit 1`), `raw = state.src` spacing in knowledge_md.py, and the LEDGER row's line counts corrected (the helper module is 52 lines, not 37). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 28 fixes pushed in d2be01d (replies on each thread name the test that reproduces it). Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2be01d5c4
ℹ️ 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".
…urce-link text, silent mutation failures, `<?>` closing) + browser-accurate invisible markup Second Codex review of 78d12a9; every finding reproduced first by harness/tests/neg29.sh (13 of 20 cases failed on d2be01d, 20 of 20 pass now): - <textarea>a b</textarea> vs a b compared equal. A lesson holding raw HTML that shows whitespace as written (textarea, xmp, listing, plaintext) or can (a style element or attribute) is now compared exactly; Chromium 141 reports white-space pre / pre-wrap for all of these. - Source: [](https://…) was accepted. The source link now needs visible link text (code or an image counts; whitespace or a comment does not). - A `pos` case whose mutation failed ran against the untouched repo and passed. Every suite now applies mutations through mutate(): the mutation runs under set -e, and on any failure the copy's checker is replaced by a stub printing MUTATION DID NOT APPLY, so the case cannot pass. The full run after this change confirms all 218 earlier mutations were applying. Temp dirs are now removed on exit in every suite. - `<?>` (and `<!-->`) as an HTML block was reported as never closing: unclosed() searched past the opener. CommonMark tests the whole line. - verified/…_v3.md called itself "this v2". Found while fixing `<?>`: the invisible-markup pattern ended PIs, CDATA and declarations at `?>` / `]]>` and comments only at `-->`, while a browser ends PIs/CDATA/declarations ("bogus comments") at the first `>`, comments also at `--!>`, and treats `<!-->` / `<!--->` as complete. Text after those points is visible (Chromium 141 textContent), so copies differing there compared equal. INVISIBLE_RE now follows the tokenizer; neg29 N6 cases. Reference first: eight new cases in harness/conformance/cases.json pin the parses (commonmark.js 0.31.2 agrees on all eight; 652/652 spec examples). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eda147862
ℹ️ 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".
…l5lib-checked) and raw HTML never counts as a value Codex review of d2be01d; reproduced first by harness/tests/neg30.sh (11 of 14 cases failed on 9eda147; 15 of 15 pass now): - Text inside hidden raw HTML (`<span hidden>confidence 80%</span>`, a `<template>`, a raw `<div hidden>` block) satisfied Why / Do / Source and the confidence label. Raw HTML can hide what it wraps (hidden, template, CSS), so text inside a raw HTML element — or a raw HTML block — no longer counts as a field value, a source link title or a confidence label. Text beside raw HTML still counts (P1++++). - Invisible markup around the title: a comment AFTER or INSIDE `**Title**` is now ignored (P2+, P2+++). A comment BEFORE it cannot be ignored: an item whose content starts with `<!--` opens an HTML block, so the title is raw text shown with its asterisks (commonmark.js 0.31.2 agrees; pinned as `comment-before-title`). P2 asserts that FAIL. - `<input value="<!-- A -->">` vs `"<!-- B -->"`, comment-like text in a textarea, and whitespace or tag-like text inside attribute values were removed or normalized by regexes that did not know HTML context. The checker now tokenizes rendered HTML the way a browser does (knowledge_md.html_segments: WHATWG tag / attribute / raw-text / comment / bogus-comment states) and only removes what that tokenizer marks as never displayed; tags, attributes and raw text stay byte-exact, and whitespace is collapsed only in text segments. Verification of the tokenizer: harness/conformance/html_oracle.py drives html5lib's WHATWG tokenizer and compares token streams on a 28-case corpus plus seeded fuzz — 100,000 fragments in CI, 500,000 locally, 0 divergences. Chromium 141 (DOMParser) agrees on 18,452 of 18,452 fuzzed fragments that removing what html_segments hides leaves the DOM unchanged apart from comments. CI's conformance job now installs html5lib 1.1 and runs the oracle. Round-29 cases N1/N1+/N1+++ now put plain text beside their raw HTML (under the new rule a textarea-only Do: is empty). Six new reference cases in cases.json (56 total, 652/652 spec examples). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 30 pushed in 2fb93c2: browser-faithful HTML tokenizer (html5lib-checked) replaces the invisible-markup regexes; raw HTML no longer counts as a field value. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fb93c2b70
ℹ️ 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".
The round-30 mutation run showed M13 surviving all 253 suite cases; only the html5lib conformance gate caught it (3 corpus cases, 734 of 100,000 fuzzed fragments). Two neg30 cases now pin it directly: a `>` inside a quoted attribute value must not end the tag, so whitespace after it is not collapsed (P3+++++) and a comment after it is not removed (P3++++++). Both FAIL against the M13 mutant and PASS on the real checker. The P3 cases also get plain text beside their raw HTML, so the copy comparison alone decides them (under round 30, a raw-HTML-only Do: is empty). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9afc610a73
ℹ️ 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".
…a147, 2fb93c2, 9afc610) Each reproduced first by harness/tests/neg31.sh (12 of 21 cases failed on 9afc610; 21 of 21 pass now); browser claims checked in Chromium 141. - A type-1 block `<script data-end="</script>">` counted as closed because CommonMark's end marker sits in the attribute; the browser leaves the script open and swallows what follows. unclosed() now also asks the tokenizer (knowledge_md.html_open_at_end: cut-off tag, comment, bogus comment, open raw-text element) and FAILs if either view is still open. - `[T](…)confidence 80%` passed: the link text's last visible character now takes part in the word boundary. - `<span hidden>x</bogus>confidence 80%` passed: a raw end tag now closes an element only when it matches the innermost open one (Chromium ignores the stray `</bogus>`); a stray end tag never ends a hidden region early. - `**<span hidden>Invisible</span>**` passed: the title is the visible text of the bold span, raw-HTML content excluded. - `x <span> y</span>` vs `x<span> y</span>` (same rendering) FAILed: text whitespace now follows the CSS model across inline boundaries, with one canonical place for the surviving space; block boundaries unchanged. - `Do:` followed only by an empty heading or a rule passed: a later block counts only if it shows Markdown text, code or an image (has_value()). - svg/math: CDATA is raw only while an svg/math element is open (nesting is tracked), so after `</svg>` it is a hidden bogus comment again. - Whitespace preservation is scoped to the element that asks for it (textarea, xmp, listing, plaintext, style attribute); only a <style> element, which can restyle anything, keeps a lesson exact everywhere. - neg11 case 6+ wrote its fixture directly; it now goes through mutate() and also asserts the sheet was counted (FAILs if verified/ is missing). - Not changed: the DOCTYPE finding. A `>` inside a quoted public identifier ENDS the DOCTYPE (WHATWG abrupt-doctype-public-identifier); html5lib and Chromium 141 both display `confidence 80%">`. neg31 Q9 pins it, and the oracle now fuzzes PUBLIC / SYSTEM doctypes (+2 corpus cases). Reference cases: 5 more in cases.json (61; 652/652 spec examples) and 5 in html_cases.json (33; 100,000 fuzzed fragments, 0 divergences). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 31 pushed in b30cdaf (11 findings answered on their threads; one disproved with html5lib + Chromium evidence). Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b30cdafe59
ℹ️ 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".
T's decision, 2026-09-27: every Codex round since 78d12a9 found new browser edge cases in raw HTML inside lessons (3 to 11 per review; 7 more on b30cdaf), while the real lesson file holds none. Rather than emulate a browser case by case, lessons are now plain Markdown: raw_html_in() FAILs a lesson holding any raw HTML other than `<!-- comments -->` — inline or as a block, tags, declarations, PIs and CDATA alike ("raw HTML in a lesson (only <!-- comments --> are allowed)"). HTML inside code, autolinks and comments stay allowed (neg32 R0+++++++ to R0+++++++++). This supersedes five of the seven b30cdaf findings (style attributes, intervening end tags, raw HTML at a link's end, an open <div> swallowing the fields, character references in attributes): each is now a FAIL (neg32 R2-R6). The other two are fixed: - regression from b30cdaf: a title that is inline code (`**`Title`**`) lost its text and was rejected; titles keep code text again (R1, R1+). - `</svg/>` is an end tag: the self-closing check now applies to start tags only, so the tokenizer leaves foreign content (R7). Both reproduce on b30cdaf and pass now. Expectation changes (marked CHANGED 2026-09-27 in the suites): 14 positive cases carried raw HTML in a lesson and now expect the ban — 4 from PR #1 (neg15 2b, neg23 G4+, neg26 J2, neg27 K4+) and 10 from rounds 29-31. 22 suites, 294 cases, 0 failing; conformance 652/652 spec, 61 edge cases; html5lib oracle 100,000 fragments, 0 divergences. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 32 in 37a18eb: raw HTML other than comments is now rejected in lessons (owner's decision), which supersedes the raw-HTML semantics findings. Please review the ban itself ( Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a18ebb0b
ℹ️ 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".
Codex review of 37a18eb: `Source: [Title][src]` with `[src]: https://…` elsewhere was accepted, although the documented shape is `[title](https://…)` and the v1 checker required `](`. parse_source() now re-reads the Source paragraph with an empty reference environment (MD.parseInline(content, {})), so a full, collapsed or shortcut reference link is plain text there and the field FAILs with `source line is not …`. Reference links elsewhere in a lesson are unaffected. neg32 R8 / R8+ reproduce it on 37a18eb (both passed there) and FAIL now; R8++ keeps a reference link in a Why field passing. cases.json pins the parse (`undefined-reference-is-text`; 62 cases, 652/652 spec examples). 22 suites, 297 cases, 0 failing. Mutation check on 37a18eb: 22 of 22 caught. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 33 in 9b395d4: inline-only Source links. Generated by Claude Code |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b395d41c7
ℹ️ 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".
… tags follow the spec on NBSP Codex review of 9b395d4; reproduced first by harness/tests/neg33.sh (3 of 7 cases failed on 9b395d4; 7 of 7 pass now): - `[T`K`](https://x.y)confidence 80%` passed: the link text's last character now includes inline-code text, so code glues to the next word just as text does (S1; S1+ with a space still passes). - `# <span hidden>Invisible</span>` counted as a verified sheet's H1 text: the H1 text now uses the same visibility rules as lesson titles (S3; code and text beside raw HTML still count, S3+ / S3++). - `- <!-- note -->` alone on the item's first line, then `**Title**`: the comment block ends before the title (commonmark.js agrees, pinned as `comment-block-then-title`), so invisible blocks are skipped when finding the title and the fields (S4; S4+ keeps a plain title failing). - harness/conformance/check.py allowlisted divergences by id only. Each exception now pins our exact output, which must match and must still differ from commonmark.js; a wrong pin FAILs the gate (checked with a tampered pin). Pinning exposed one more gap: markdown-it-py 4.0.0 let Python's `\s` (NBSP included) separate a raw INLINE tag's parts, so `<span\xa0a=b>` became raw HTML where spec 0.31.2 section 6.6 and cmark-gfm (GitHub) make it text. knowledge_md._patch_html_inline() rebuilds the inline tag pattern with spaces, tabs and a line ending only. Our three NBSP cases now render byte-for-byte like cmark-gfm 0.29.0.gfm.13 (new case `nbsp-in-inline-tag`). 23 suites, 304 cases, 0 failing; 652/652 spec examples, 64 edge cases; html5lib oracle 100,000 fragments, 0 divergences. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 33 in 4187200. Generated by Claude Code |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review Roast 🔥Verdict: No New Issues Found | Recommendation: Merge Oh wait, this incremental PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. Previous Issues — All Still Fixed
Incremental Changes Reviewed
🏆 Best part: The fix comment 💀 Worst part: Nothing. The diff is two lines across two files and both are unambiguous improvements. I'm genuinely disappointed. 📊 Overall: Like a Michelin-star restaurant that finally washed the lettuce — the architecture was already stellar, the test infrastructure envy-inducing, and now the salad course won't give you food poisoning. Files Reviewed (2 files)
Previous Review Summaries (4 snapshots, latest commit 6e8f79e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6e8f79e)Verdict: No New Issues Found | Recommendation: Merge Oh wait, this incremental PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. Previous Issues — All Still Fixed
Incremental Changes Reviewed
🏆 Best part: The 💀 Worst part: Two 📊 Overall: Like a Michelin-star restaurant that finally washed the lettuce — the architecture was already stellar, the test infrastructure envy-inducing, and now the salad course won't give you food poisoning. Files Reviewed (7 files)
Previous review (commit caeed36)Verdict: No New Issues Found | Recommendation: Merge Oh wait, this incremental PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. Previous Issues — All Still Fixed
Incremental Changes Reviewed
🏆 Best part: The fence closure fix wasn't just patched — it leverages the parser's own structural verdict. A closed fence spans opener + content + closer lines; an open one (over-indented closer, wrong container) only opener + content. The token-map arithmetic proves it. 💀 Worst part: Nothing. The previous review's 5 issues stay resolved, the 2 new behavioral changes are tested, and the conformance gate still passes 652/652 spec examples + 36 edge cases. 📊 Overall: Like a Michelin-star restaurant that finally washed the lettuce — the architecture was already stellar, the test infrastructure envy-inducing, and now the salad course won't give you food poisoning. Files Reviewed (6 files)
Previous review (commit 868313f)Verdict: No New Issues Found | Recommendation: Merge Oh wait, this incremental PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. Previous Issues — All Fixed
Incremental Changes Reviewed
🏆 Best part: The fix for the 💀 Worst part: Nothing. The previous review's 5 issues are all resolved with tests. The only thing left to roast is the comment claiming "cmark spacechar, commonmark.js \s" — they differ on vertical tab (cmark excludes it, JS includes it) — but since the conformance gate targets commonmark.js, the code behavior is correct for the reference. 📊 Overall: Like a Michelin-star restaurant that finally washed the lettuce — the architecture was already stellar, the test infrastructure envy-inducing, and now the salad course won't give you food poisoning. Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit 4187200)Verdict: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The conformance gate is genuinely impressive — 652/652 spec examples + 64 edge cases pinned against commonmark.js 0.31.2, plus 100,000 fuzzed fragments against html5lib. That's more rigor than most production parsers get. I'm almost annoyed it's this good. 💀 Worst part: The 📊 Overall: Like a Michelin-star restaurant that forgot to wash the lettuce — the architecture is stellar, the test infrastructure is envy-inducing, but one index bug in the salad course can still give you food poisoning. Files Reviewed (5 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 0 · Output: 0 · Cached: 0 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 418720018b
ℹ️ 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".
…I whitespace in tag patches Codex review of 4187200: - harness/conformance/check.py canonicalized the newline before `</pre>`, which is visible content, so a parser dropping it would still match. Newlines are now only canonicalized outside <pre> (the one needed case, before `<pre>`, stays). neg34 T4 checks `<pre>x\n</pre>` vs `<pre>x</pre>` differ; it fails against 4187200's gate and passes now. - `# <span>Visible</span>` in a verified sheet stays a FAIL, by design: the H1's text must be Markdown text or code, the same rule as lesson titles (raw HTML may hide what it wraps). The diagnostic now says so; neg34 T1. Kilo review of 4187200 (first run with credits): - "critical" lead_text/visible_text offset mismatch: does not reproduce. lead_text() stops at the first token visible_text() would not render as plain text, so it is always a prefix; neg34 T3 asserts it on 10 inputs, with and without code text. Documented in lead_text()'s docstring. - "warning" IndexError on an empty `# ` heading: does not reproduce; the heading always has an inline child. neg34 T2 / T2+ pin a clean FAIL. - suggestion on CR/FF in the inline tag pattern: CR never reaches inline parsing (normalized to LF), but checking FF against the references showed a real gap of mine: since 78d12a9 the block-start patch replaced `\s` with `[ \t]`, and 4187200's inline patch with `[ \t\n]`, while cmark (spacechar), cmark-gfm and commonmark.js all accept a form feed or vertical tab there. Both patches now use ASCII whitespace [ \t\n\r\f\v]; NBSP stays excluded (spec + cmark). 5 new conformance cases (ff/vt inline, after a block tag, type 7) diverged on 4187200 and are identical to commonmark.js now. - suggestion/nitpick comment wording (foreign end tags, the whitespace allowance in only_comments): reworded. 24 suites, 309 cases, 0 failing; 652/652 spec examples, 69 edge cases (3 pinned exceptions); html5lib oracle 100,000 fragments, 0 divergences. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 34 in 868313f (Codex and Kilo findings on 4187200 answered on their threads). Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868313f4e4
ℹ️ 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".
…any case Codex review of 868313f; both reproduced first by harness/tests/neg35.sh (2 of 3 cases failed on 868313f; 3 of 3 pass now): - unclosed() judged a fence reaching the end of the file closed if its last line looked like a closer with ANY indentation. A would-be closer indented 4+ columns past its container is content, so the fence never closes and swallows what follows. The check now reads the parser's own verdict: a closed fence spans opener + content + closer lines, an open one only opener + content. U2 (closer 6 columns deep) FAILs with `never closes`; U2+ (3 columns) still closes. - harness/conformance/check.py matched `<pre>` case-sensitively, so a raw `<PRE>` region was not protected from newline canonicalization. The pattern is case-insensitive now (U1). 25 suites, 312 cases, 0 failing; 652/652 spec examples, 69 edge cases; html5lib oracle 100,000 fragments, 0 divergences. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 35 in caeed36. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caeed3660c
ℹ️ 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".
…ide image descriptions Codex review of caeed36; reproduced first by harness/tests/neg36.sh (3 of 4 cases failed on caeed36; 4 of 4 pass now): - A field item starting with a comment-only block (`- <!-- note -->`, then `Why it matters here: x` on the next line) was ignored and the lesson reported the field missing. Comment-only blocks are skipped when reading a field, as they already were before the title (V1; V1+ keeps an empty field failing). commonmark.js agrees on the parse (`comment-block-then-field`). - ``: raw HTML nested in an image description escaped the raw-HTML ban because only top-level inline tokens were inspected. The ban now walks nested tokens (V2; V2+ keeps a comment there allowed). The new reference case `html-in-image-description` showed the three implementations render that alt text differently (commonmark.js raw, cmark-gfm escaped, markdown-it text only; the spec only recommends the plain string). It is a pinned, reviewed exception: the parse is identical, and the ban means no verdict depends on it. 26 suites, 316 cases, 0 failing; 652/652 spec examples, 71 edge cases (4 pinned exceptions); html5lib oracle 100,000 fragments, 0 divergences. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
@codex review Round 36 in 6e8f79e. Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ent (CI fix) 6e8f79e reworded the comment above ALLOWED in harness/conformance/check.py; neg34 T4 and neg35 U1 sliced that file's source up to the old comment text, so both failed with a ValueError and the "negative-test suites" CI job went red on 6e8f79e. The probes now slice up to `ALLOWED = {`, a code anchor. Reproduced locally (26 suites, 316 cases, 2 failing on 6e8f79e) and fixed (0 failing). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
|
CI note: the "negative-test suites" job failed on 6e8f79e because of my own change. I reworded a comment in Generated by Claude Code |
2 participants
What changed
The 27 review rounds on #1 were all Markdown-parsing edge cases in a hand-rolled parser. This PR stops hand-parsing.
check-knowledge.pynow reads Markdown with markdown-it-py (CommonMark 0.31.2 preset) and compares the two copies of each lesson by their rendered, visibility-normalised HTML. Comments, processing instructions, declarations and CDATA are removed, and whitespace is collapsed the way a browser does outside<pre>/<code>.check-knowledge.pyharness/knowledge_md.pyharness/tests/, run in CIParser patches (in
harness/knowledge_md.py, each pinned byharness/conformance/cases.json)<!plus any ASCII letter (CommonMark 0.31.2). HTML-block patterns take only space/tab as whitespace, where markdown-it-py's\salso matched a NBSP.Two reviewed exceptions in the gate: for
<div+NBSP and<span+NBSP, commonmark.js departs from the spec text (it uses JS\s). cmark and cmark-gfm (GitHub) follow the spec, and so do we.Test assertions corrected (the reference implementation proved the old reading wrong)
[T](https://x.y/<bad>)%3Cbad%3E)<-destination is text2. moreat column 0 after a lesson<ol start=\"2\">2.inside the field is lazy<!foo>before an H1<!foo> extraafter a fieldknowledge:strayGitHub renders with cmark-gfm (spec 0.29), where a lowercase
<!foo>is text. The README notes this; this harness targets 0.31.2.Evidence (local, same commands as CI)
Also in this PR
verified/…_v3.md: the proof repo was renamed tocloud-launch-pad, and v2 now carries a superseded banner.7946224). CONTRIBUTING: harness-change rules and the open→merged ledger convention.Risk
🤖 Generated with Claude Code
https://claude.ai/code/session_01T7N2VsxDrRZPwWM7zdFnuK
Generated by Claude Code