fix(ai-tutors): handle nested code fences - #1010
Conversation
Generated-by: Codex (GPT-5)
c8198dd to
7920d5e
Compare
potiuk
left a comment
There was a problem hiding this comment.
Approving, and I owe you an explanation for the delay and for the rebase I pushed.
Your implementation is the better one, and I merged the other one first. #1011 fixed the same bug, was opened after this PR, and I merged it without noticing yours — it had no CI at the time (its workflow runs were sitting unapproved), so it never surfaced in my review queue. That was my miss, not a judgement on this PR.
I have rebased this onto the merged state and kept your parser. The two implementations are not equivalent, and I checked the difference by running both rather than reading them:
| Input | the merged version | yours |
|---|---|---|
bare four-backtick fence ```` |
left untagged — MD040 still fires, which is the entire reason tag_bare_code_fences exists |
tagged ````text, at its own fence length |
a ```python line inside a three-backtick block |
treated as a close, so the real closing fence then looked like an opening one and got tagged — corrupting the output | correctly kept the block open |
Two things your version gets right that the other did not:
- Requiring the closing fence to carry no info string. That is the CommonMark rule, and it is what prevents the corruption in the second row above.
- Preserving the opening fence's length when tagging —
f"{indent}{fence}{default_lang}"rather than a hardcoded three backticks — which is what makes the first row work.
Using a single anchored regex with named groups for all of it reads better than counting backticks by hand, too.
What I changed while rebasing
- Kept your
tag_bare_code_fenceswholesale, discarding the merged implementation. - Merged the two test suites rather than replacing one with the other: the six tests from #1011 plus your end-to-end nested-fence case, adapted to the module binding the file already uses.
- Corrected one test that had pinned the wrong behaviour.
test_shorter_fence_inside_block_is_literalasserted that a bare four-backtick fence stays untagged. That encoded the merged version's limitation as if it were intended. The function's own docstring says it exists to give every language-less opening fence a language so MD040 passes, so a bare````must be tagged. It now asserts"````text\n```\n````". - Added two regression tests for the behaviours your version fixes and the other did not — the bare longer opening fence, and the info-string line that must not close a block. Both fail against the merged implementation.
Nine tests, all passing, and they now run in CI: ai-tutors/ became a uv workspace member as part of #1011, so there is a real pytest (ai-tutors) job.
One thing deliberately left out
inject-knowledge-base.py --check reports lesson-08-eval-driven-development.md as needing a refresh. I checked whether the parser change caused that, and it did not — both lesson-04 and lesson-08 were already stale on main before this PR, from source-page edits unrelated to code fences. Your lesson-04 regeneration stays, since it demonstrates this fix; lesson-08 wants its own regeneration commit and should not ride along here.
Worth noting for a maintainer: #928 regenerates lesson-04 only, so it now overlaps with this PR and probably wants closing or rebasing once this lands.
Thanks for this, and sorry for the ordering — the stricter parser is the one worth having.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md § Opening a pull request.
|
@potiuk thanks for the detailed review and for preserving the stronger implementation here. |
Summary
tag_bare_code_fencesto track the length of the currently open backtick fence.--checkpasses.Closes #1008.
Validation
uv run pytest ai-tutors/test_inject_knowledge_base.pypython3 ai-tutors/inject-knowledge-base.py --checkuv run ruff check ai-tutors/inject-knowledge-base.py ai-tutors/test_inject_knowledge_base.pyuv run ruff format --check ai-tutors/inject-knowledge-base.py ai-tutors/test_inject_knowledge_base.pyuv run prek run --all-filesGenerative AI Disclosure
This PR was prepared with AI assistance.
Generated-by: Codex (GPT-5)