Repository navigation
feat(repo): guidance:format=xml and guidance:as-skill arms - #333
Conversation
Two guidance parts for `repo run --arm`, to settle format and packaging debates with data: the same guidance content in another shape, compared per model. - guidance:format=xml renders each guidance file as a <guidance> element with one <section level title> per Markdown heading. The rendering must restore to the original file exactly, or the cell's preparation fails. - guidance:as-skill removes the guidance files, as guidance:none does, and packages their text into .claude/skills/<repo>-guidance/SKILL.md; the skill is recorded in `skills`, so usage is measured like any skill. guidance:format=md is refused (it is guidance:raw). POML is not offered: its documented components have no heading tag. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…-skill Self-review of 78457e2 found three faults in the new arms: - The packaged SKILL.md had a description with ": " in a plain YAML scalar, so the frontmatter did not parse and the skill could not load. - A guidance file that is not UTF-8 raised UnicodeDecodeError instead of a PrepError for the cell (guidance:format and guidance:as-skill share the reader now). - A symlinked guidance file was dropped without a trace; a link to another guidance file is now packaged once under its target, and a link to any other file fails the cell. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved defects misparse fenced code as guidance sections, corrupt source newlines on Windows, and incorrectly reject combined guidance-as-skill and skill installation arms.
3 open issues
What changed in this PR
3 new findings.
This PR adds two repo run arms for comparing how guidance packaging affects agent behavior while keeping the model and guidance content fixed.
Changes:
- Adds XML-style heading conversion with a round-trip check.
- Adds packaging of repository guidance into one discoverable skill, recorded for usage measurement.
- Adds regression tests and documents the new arms and their limitations.
| File | Description |
|---|---|
dokimi/tests/test_repo_guidance_format.py |
Tests conversion, packaging, parsing, and error handling. |
dokimi/dokimi/repos/workspace.py |
Integrates the new arms into workspace preparation and skill metadata. |
dokimi/dokimi/repos/guidance_format.py |
Implements reversible heading-to-section conversion. |
dokimi/docs/repo-evaluation-contract.md |
Adds the arms to the workspace preparation contract. |
dokimi/docs/benchmarking-and-comparison.md |
Explains comparisons, behavior, and known limitations. |
dokimi/changelog.d/20261031_120000_napetrov_guidance-format-arms.md |
Announces the new guidance arms. |
Review effort: Balanced
| if _FENCE.match(line): | ||
| fenced = not fenced |
There was a problem hiding this comment.
| if arm.base == "guidance:as-skill": | ||
| return _apply_as_skill(ws, manifest, matched) |
| rendered = to_format(_guidance_text(ws, rel, f"guidance:format={fmt}"), fmt) | ||
| except FormatError as exc: | ||
| raise PrepError(f"guidance:format={fmt}: {rel}: {exc}") from exc | ||
| (ws / rel).write_text(rendered, encoding="utf-8") |
There was a problem hiding this comment.
| if _FENCE.match(line): | ||
| fenced = not fenced |
There was a problem hiding this comment.
Must-fix: fence state changes on any backtick or tilde prefix, regardless of the opening delimiter and length. For input "\n```\n# example\n", the three-backtick line incorrectly closes the four-backtick fence and # example becomes a section even though it is code. Mixed delimiters and lines such as ```text inside a fence cause the same problem. Round-trip equality still passes, so it cannot detect this semantic change. Track the opening character and run length, and close only on a matching delimiter of sufficient length with whitespace-only trailing content. Add regression tests for these cases.
| if arm.base == "guidance:as-skill": | ||
| return _apply_as_skill(ws, manifest, matched) |
There was a problem hiding this comment.
Must-fix: update the arm preflight to recognize that this arm removes matched files. repo run calls arms.check_arms() before preparation, but its drops condition recognizes only guidance:none and revision arms. For a base containing .claude/skills/lint/SKILL.md, guidance globs including .claude/skills/**, and guidance:as-skill+skill:skills/lint, preflight incorrectly rejects the arm as already shipping lint. Preparation would package and remove that file, then _install_skills() would replace the emptied directory successfully. Include guidance:as-skill in the preflight removal condition and add a regression covering check_arms() plus preparation (the existing removal test in test_repo_skill_arms.py provides a fixture).
| if (ws / rel).is_symlink(): | ||
| links.append(rel) | ||
| continue |
There was a problem hiding this comment.
Must-fix: this silently leaves guidance in Markdown when its symlink target is not matched by the guidance globs. For example, with AGENTS.md -> docs/guide.md, docs/guide.md containing # Guide\n, and globs matching only AGENTS.md, _apply_format returns success with no files rendered. The XML arm then measures exactly the raw guidance despite being reported as a format treatment; recording the link in metadata does not prevent that misleading comparison. Validate that each link ultimately reaches a regular guidance file being rendered, and raise PrepError otherwise (or explicitly render its safely resolved target). Add a regression test for this unmatched-target case.
| rendered = to_format(_guidance_text(ws, rel, f"guidance:format={fmt}"), fmt) | ||
| except FormatError as exc: | ||
| raise PrepError(f"guidance:format={fmt}: {rel}: {exc}") from exc | ||
| (ws / rel).write_text(rendered, encoding="utf-8") |
There was a problem hiding this comment.
Must-fix: write_text uses platform-default newline translation. On Windows, LF guidance becomes CRLF, and preserved CRLF becomes CRCRLF because _guidance_text decodes raw bytes without newline normalization. The restore check runs before this write, so the actual file violates the exact-restoration guarantee. The SKILL.md write in _apply_as_skill has the same defect and violates verbatim packaging. Use write_bytes(text.encode("utf-8")) or write_text(..., newline="") at both sites, and verify on-disk bytes rather than newline-normalizing read_text() results.
| target = posixpath.normpath(posixpath.join(posixpath.dirname(rel), os.readlink(ws / rel))) | ||
| if target not in matched: | ||
| raise PrepError(f"guidance:as-skill: {rel} links to {target}, which is not a guidance file") | ||
| continue |
There was a problem hiding this comment.
Must-fix: membership of the immediate target does not establish that a link ultimately reaches a packaged regular file. With AGENTS.md → CLAUDE.md and CLAUDE.md → AGENTS.md, both targets are in matched, both files are skipped and deleted, and preparation succeeds with a frontmatter-only skill. The base checkout does not reject these cycles; _guidance_at validates links only for revision arms. Resolve each chain with cycle detection and require it to terminate at a regular matched guidance file before deleting anything. Add a cyclic-link regression test.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
+ Coverage 94.40% 94.48% +0.07%
==========================================
Files 313 317 +4
Lines 40240 40728 +488
Branches 6828 6920 +92
==========================================
+ Hits 37990 38481 +491
+ Misses 1435 1432 -3
Partials 815 815
🚀 New features to boost your workflow:
|
- a fence closes only on a run at least as long as its opener, of the same character, and never inside a longer run of backticks or tildes - guidance:as-skill and guidance:format follow symlink chains: a chain that cycles or ends outside the matched guidance files is a prep error - guidance:as-skill removes the base skill of the same name the arm packages, so check_arms does not refuse it (drops rule also covers as-skill) - guidance files and the packaged SKILL.md are written as bytes, so no platform newline translation runs after the round-trip check Thread: PRRT_kwDOVBg7g86yWVOg Thread: PRRT_kwDOVBg7g86yWVZ2 Thread: PRRT_kwDOVBg7g86yWVOl Thread: PRRT_kwDOVBg7g86yWVZ4 Thread: PRRT_kwDOVBg7g86yWVOr Thread: PRRT_kwDOVBg7g86yWVZ8 Thread: PRRT_kwDOVBg7g86yWVZ6 Thread: PRRT_kwDOVBg7g86yWVZ_ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if fence is None: | ||
| return match.group(1) if match else None |
There was a problem hiding this comment.
Must-fix: validate Markdown fence openers before entering fence state. For input "```foobar\n# Heading\n", the backtick in the information string makes the first line invalid as a Markdown fence, but this branch opens one and leaves the real heading unconverted. Similarly, _FENCE` accepts a leading tab via `\s{0,3}`, although that is four-column indentation (indented code), not a fence opener. Both cases pass the round-trip check while producing the wrong section structure. Restrict indentation to up to three literal spaces and reject backtick openers whose information string contains a backtick; add tests asserting that subsequent headings become sections.
There was a problem hiding this comment.
Fixed in 6bd48b4. An opener is a fence only with up to three literal spaces of indent, so a tab is body text. A backtick opener whose info string holds a backtick is not a fence (CommonMark).
Tests assert that the heading after each such line is a section.
Left unresolved for your check.
| chain cycles or ends outside the matched guidance files: that file would be dropped or left unrendered.""" | ||
| chain = [rel] | ||
| while (ws / chain[-1]).is_symlink(): | ||
| target = posixpath.normpath(posixpath.join(posixpath.dirname(chain[-1]), os.readlink(ws / chain[-1]))) |
There was a problem hiding this comment.
Must-fix: lexical normalization is not equivalent to filesystem symlink resolution when .. follows a symlinked directory. For example, let sub -> other/sub, CLAUDE.md -> sub/../AGENTS.md, and have both root AGENTS.md and other/AGENTS.md, with only root AGENTS.md and CLAUDE.md matched. This code normalizes the link to root AGENTS.md and accepts it, but the filesystem resolves it to other/AGENTS.md. The format arm therefore leaves CLAUDE.md pointing to unrendered guidance, and the as-skill arm discards its distinct content. Resolve the actual target strictly, verify it is a regular file within the workspace and among the matched files, and translate dangling/cyclic-link errors into PrepError. Add this case for both callers.
There was a problem hiding this comment.
Fixed in 6bd48b4. _link_file resolves the link with Path.resolve(strict=True), as the filesystem does, so a .. after a linked directory reaches the file the filesystem reaches. That file must be a regular file inside the workspace and one of the matched guidance files.
A dangling link or a cycle is a PrepError. Tests cover guidance:format and guidance:as-skill: the linked-directory case, a link in a subdirectory, and a dangling link.
Left unresolved for your check.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at cf6ced2. The arm wiring, the drops change and the byte-level writes hold up, and the earlier fence, cycle and CRLF points are fixed. Three things are worth fixing before merge, and there is one nit.
What I ran, on a Linux review host at cf6ced2:
make ci-local: 28 passed, 2 failed, 7 skipped. 12,013 tests passed, changed-line coverage passes. The two FAILs areInstall gitleaks(a download that returned exit 22) andScan commits, which depends on it. Neither reads the PR's tree.- Reproductions for the code findings, with no model or network involved.
- A mutation run of 57 hand-made mutants against the new tests: 13 pass the full suite. The ones that decide something are in the inline comments.
Nit (outside this diff): the arm lists were not updated everywhere. dokimi/dokimi/commands/repo_run.py:463 (--arm help) still lists guidance:none | guidance:raw | guidance:bridge | guidance:rev:REV | patch:FILE | ..., so repo run --help omits the two new arms while the unknown-arm error, built from ARM_FORMS, lists them. dokimi/docs/preparing-a-repository.md:9 and :251 name only none, raw and bridge. Suggestion: add guidance:as-skill and guidance:format=xml to the help string and the line at :9.
All commits carry a Signed-off-by.
Bot points I checked that no longer apply at this head (8)
- Copilot's point on a four-backtick block holding a three-backtick line does not apply: a fence now closes only on a run of the same character at least as long as its opener (
guidance_format.py:63). - Copilot's point on
guidance:as-skill+skill:...being refused by preflight does not apply:arms.py:200now includesguidance:as-skillindrops, and the new test fails if it is removed. - Copilot's point on newline translation in
write_textdoes not apply: both writers usewrite_byteson UTF-8 bytes. - exai-bot's point on mixed fence delimiters and lengths does not apply: the close check compares character and length.
- exai-bot's point on the preflight not knowing the removal arm does not apply: same
dropschange atarms.py:200. - exai-bot's point on a symlink whose target is outside the matched guidance files does not apply:
_link_fileraises aPrepErrorfor it. - exai-bot's point on
write_textnewline translation in both writers does not apply: both write bytes. - exai-bot's point on a link cycle between two matched files does not apply:
_link_filefollows the chain and refuses a cycle.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 1db497b, which fix the nit from my review:
1db497b:dokimi/dokimi/commands/repo_run.py:463: the--armhelp now listsguidance:as-skillandguidance:format=xml;dokimi/docs/preparing-a-repository.md:9names both arms too.
make ci-local at 1db497b: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the three risks from my review.
…guard tests A fence opener follows CommonMark: up to three spaces (not a tab), and a backtick fence's info string holds no backtick. A link of a guidance file is resolved as the filesystem resolves it, so a `..` after a linked directory reaches the file it names, and a dangling link or a cycle is a PrepError. The guard against writes through a linked .claude/skills and a dangling skill directory are pinned by tests. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if fence is None: | ||
| return _opening_run(line) |
There was a problem hiding this comment.
Must-fix: fence tracking misses list-contained openers but recognizes their closing delimiters as new openers. For example, to_format("- ```\n echo hi\n ```\n# Real\n", "xml") leaves # Real as Markdown: - ``` does not match _FENCE, while the closing ``` starts a fence that never closes. The round-trip check still succeeds, so the cell silently runs an incomplete heading transformation. Track Markdown container context when recognizing fences (including when the container ends), and add a regression asserting that the heading after this list-contained block becomes a section.
There was a problem hiding this comment.
Fixed in d94e063, with the tab and marker details in d1d0672. A fence opened by a list item (- ```` ) now records its item's content column: its closer may be indented up to three columns past it, and a line indented less than that column ends the item and its fence. Your example now makes # Reala section. Regression:test_a_fence_opened_by_a_list_item_holds_body_text_only_inside_the_item` (the bullet case fails without the fix; the list cases are pinned).
Local: the guidance-format and format-touching test modules pass (129 passed in the targeted set).
…sion Pins the case the review asked for: a base that ships .claude/skills/lint keeps that skill, and guidance:as-skill installs its own skill next to it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…d1031 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A fence opened in a list item (- ```) was not seen, while its closing delimiter, indented to the item's column, opened a fence that never closed, so the headings after the block stayed Markdown. A fence now records the column of its item: its closer may be indented up to three columns past that column, and a line indented less than it ends the item and the fence. Red before the fix: the bullet and the ordered-item cases fail; the list-item tests pass now. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A list marker takes one to four spaces before its content, so five spaces make indented code, not a fence; several markers on one line (- - ```) open an item each. A tab counts to the next multiple of four in a closer as it does in an item's indentation. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| match = _OPENER.match(line, column) | ||
| if match is None or (match.group(1)[0] == "`" and "`" in match.group(2)): | ||
| return None | ||
| return match.group(1), column |
There was a problem hiding this comment.
Must-fix: fences opened on a list continuation line lose their list indentation. For to_format('- Example:\n ```\n code\n# Real\n', 'xml'), _opened(' ```') returns a fence with column 0 because that line has no list marker. Consequently _advance does not end the list's fence at # Real, and the real top-level heading remains Markdown instead of becoming a section. The round-trip check still passes, so this silently measures a partially converted file. Track the active list's content column across continuation lines and add this case to the list-fence regression tests.
There was a problem hiding this comment.
Fixed in 45f2f4f. The open list items are now tracked across lines (outer and nested, not only the marker line), so a fence on a continuation line takes its item's column. # Real after the list is a section.
Regression: test_a_fence_opened_by_a_list_item_holds_body_text_only_inside_the_item. Your repro and the ordered variant were red on d1d0672, green now.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The earlier threads are fixed, and the list-item fence work is the right direction. One risk remains in the same fence tracking, so I'd fix it before merge.
What I ran, on a Linux review host at d1d0672:
make ci-local: 30 passed, 0 failed, 7 skipped. Changed-line coverage passes.- Reproductions for the code finding, with no model or network involved.
- Mutation runs on the new fence, list and tab logic, and on the link and
.claude/skillsguards. The link and guard mutants all fail a test. Several list-marker and tab mutants pass the full suite (see below). - A differential fuzz of about 60k fence/list/tab line combinations against a CommonMark parser. After the case in the inline comment, no heading mismatch remained.
Checked, no issues
- Links: a link through a linked directory, a link in a subdirectory, a dangling link, a cycle and a link outside the workspace are refused or resolved as the filesystem does, for both arms. Replacing the resolution with
normpathor a barereadlinkfails the tests. - Guard: reducing the loop to
.claude, or droppingor skill_dir.is_symlink(), fails a test each. - Commits: every non-merge commit carries a
Signed-off-by.
Not blocking, for later: the tab and list-marker details () and + markers, the one-to-four-space bound, a closer indented one to three spaces at top level) are not pinned. Mutants for each pass the whole test file. Also, a # line inside a multi-line HTML comment or <details> block becomes a section, since only code fences are tracked.
Bot points I checked that no longer apply at this head (11)
- Copilot's point on a four-backtick block holding a three-backtick line does not apply: a fence closes only on a run of the same character at least as long as its opener (
guidance_format.py:72). - Copilot's point on
guidance:as-skill+skill:...being refused by preflight does not apply:arms.pyincludesguidance:as-skillindrops, and a test pins it. - Copilot's point on newline translation in
write_textdoes not apply: both writers usewrite_byteson UTF-8 bytes. - exai-bot's point on mixed fence delimiters and lengths does not apply: the close check compares character and length.
- exai-bot's point on the preflight not knowing the removal arm does not apply: same
dropschange inarms.py. - exai-bot's point on a symlink whose target is outside the matched guidance files does not apply:
_link_fileraises aPrepErrorfor it. - exai-bot's point on
write_textnewline translation in both writers does not apply: both write bytes. - exai-bot's point on a link cycle between two matched files does not apply:
_link_fileresolves strictly and refuses a cycle. - exai-bot's point on a backtick in a fence info string and a tab-indented opener does not apply:
_OPENERtakes up to three spaces and_openedrejects a backtick in a backtick fence's info string. - exai-bot's point on lexical
..resolution does not apply:_link_fileusesPath.resolve(strict=True)and checks the result is a matched file inside the workspace. - exai-bot's point on a fence opened by a list item does not apply at this head:
- ```now opens a fence, and the heading after a closed list-item block becomes a section.
A fence opened on a line without a list marker, indented to the open item's content column, took column 0. Its dedent check then never ended it, so a real heading after the list stayed body text, and the round trip still passed. Track the open items (outermost first) across lines: a non-blank line ends the items it is indented less than, a marker opens one per marker, and a fence takes the innermost column. Lines are read expanded for the opener, as for the closer. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| _ROOT, _ROOT_CLOSE, _CLOSE = "<guidance>", "</guidance>", "</section>" | ||
| _HEADING = re.compile(r"^(#{1,6}) (\S[^\r]*)$") # canonical only: one space, no CR; anything else is body text | ||
| _FENCE = re.compile(r"^( *)(`{3,}|~{3,})(.*)$") # the indentation, a run of ``` or ~~~, and what follows it | ||
| _ITEM = re.compile(r" *(?:[-*+]|\d{1,9}[.)]) {1,4}(?=\S)") # a list marker and the one to four spaces after it |
There was a problem hiding this comment.
Must-fix: tab-separated list markers are not recognized, so their closing fence is mistaken for a new top-level opener. For to_format("-\t```\n echo hi\n ```\n# Real\n", "xml"), the first line opens a valid Markdown list-contained fence, but _ITEM requires literal spaces and misses it. The closing ``` then opens a fence with column 0, leaving # Real unconverted; the round-trip check still passes. Ordered markers followed by a tab have the same problem. Parse markers and their padding using expanded columns consistently with _opened, and add regression cases for both marker types.
There was a problem hiding this comment.
Partly fixed in 2c9b57c. The marker is now matched on the expanded line, as the fence opener is, and pinned.
Your example gives no section, and CommonMark (markdown-it-py 3.0.0) agrees: the item's content column after -<tab> is 4, so the 2-space body ends it and # Real is in the fence the closer opens. With a 4-space body the section appears (new cases, -<tab> and 1.<tab>). Before this change the sections were the same on the generated inputs I compared (0 differences), so this is a consistency fix, not a visible one.
| name = slug(f"{manifest.id}-guidance") | ||
| skill_dir = ws / SKILLS_DIR / name |
There was a problem hiding this comment.
Must-fix: include the generated guidance skill in preflight collision validation. For repository id toy, guidance:as-skill+skill:skills/toy-guidance passes check_arms() when the base has no skill at that path, but this code creates .claude/skills/toy-guidance before _install_skills(), which then raises PrepError because the destination exists. A catalog containing toy-guidance has the same problem. Reject these duplicate destinations before the batch starts (passing the repository id into validation), and add a regression test covering both preflight and preparation.
There was a problem hiding this comment.
Fixed in 2c9b57c. check_arms takes the repo id and refuses a guidance:as-skill arm whose generated .claude/skills/<repo>-guidance is one of its skills (skill: or catalog), before any cell.
Tests: the arm-skill and catalog-skill collisions; a base that tracks the directory's SKILL.md. Both via check_arms; repo run and repo tasks check pass the id.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a333c8b. The list-continuation fix for my open thread holds, and the fence tracking now agrees with a CommonMark parser on the cases I tried. One thing is worth fixing before merge: the guidance skill's name is not checked in the arm preflight.
What I ran, on a Linux review host at a333c8b:
make ci-local: 28 passed, 2 failed, 7 skipped. Tests pass and changed-line coverage passes. The two FAILs areInstall gitleaks(a download that returned exit 22) andScan commits, which depends on it. Neither reads the PR's tree.- Reproductions for the code finding and for the tab-marker point, with no model or network involved.
- A differential run of about 100k fence/list/tab line combinations against markdown-it-py, and a mutation run of 52 hand-made mutants against the two test files.
The preflight does not know the skill that guidance:as-skill creates. See the inline comment at arms.py:200.
Checked, no issues
- Fences: the new continuation-line tracking never converts a heading that markdown-it-py treats as fence body. The divergences left (a lazy unindented line inside an item, an empty
-item) only leave a heading unconverted, which is the safe direction. - Docs and rules: the arm names, the
.claude/skills/<repo id>-guidance/SKILL.mdpath and the## File:layout in the docs match the code. The changelog fragment is in place, no new page needs anav:entry, and every non-merge commit carries aSigned-off-by.
Bot points I checked that no longer apply at this head (13)
- Copilot's point on a four-backtick block holding a three-backtick line does not apply: a fence closes only on a run of the same character at least as long as its opener.
- Copilot's point on
guidance:as-skill+skill:...being refused by the base-skill check does not apply:arms.py:200includesguidance:as-skillindrops. - Copilot's point on newline translation in
write_textdoes not apply: both writers usewrite_byteson UTF-8 bytes. - exai-bot's point on mixed fence delimiters and lengths does not apply: the close check compares character and length.
- exai-bot's point on the preflight not knowing the removal arm does not apply: same
dropschange atarms.py:200. - exai-bot's point on a symlink whose target is outside the matched guidance files does not apply:
_link_fileraises aPrepErrorfor it. - exai-bot's point on
write_textnewline translation in both writers does not apply: both write bytes. - exai-bot's point on a link cycle between two matched files does not apply:
_link_fileresolves strictly and refuses a cycle. - exai-bot's point on a backtick in a fence info string and a tab-indented opener does not apply:
_OPENERtakes up to three spaces and_openedrejects a backtick in a backtick fence's info string. - exai-bot's point on lexical
..resolution does not apply:_link_fileusesPath.resolve(strict=True). - exai-bot's point on a fence opened by a list item does not apply at this head:
- ```opens a fence and the heading after the closed block becomes a section. - exai-bot's point on a fence opened on a list continuation line does not apply at this head:
- Example:\n ```\n code\n# Real\nnow gives a section for# Real. - exai-bot's point on tab-separated list markers does not apply: for
-\t```\n echo hi\n ```\n# Real\nand the1.\tform, a CommonMark parser (markdown-it-py) also finds no heading, because the tab puts the item content at column 4 and the two-space line opens a top-level fence. The arm gives the same result.
… marker check_arms refuses a guidance:as-skill arm whose generated skill .claude/skills/<repo>-guidance is one of its skills or is tracked by a task's base, before any cell runs, instead of failing cell by cell in preparation. A list marker followed by a tab is read at its expanded columns, as the fence opener is, so the item's fence is recognised; pinned against CommonMark. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| return items | ||
| expanded = line.expandtabs(4) | ||
| indent = len(expanded) - len(expanded.lstrip(" ")) | ||
| return tuple(column for column in items if column <= indent) + _marker_columns(line) |
There was a problem hiding this comment.
Must-fix: an unindented lazy paragraph continuation does not end a Markdown list item, but this filter drops its content column. For - Example:\ncontinued text\n ```\n code\n# Real\n, the list remains open through continued text, so the fence belongs to the item at column 2 and # Real ends that item and is a heading. Here the fence is instead tracked at column 0, leaving # Real as body text. The round-trip check still succeeds, so the XML arm silently measures an incorrect heading transformation. Preserve list state across lazy paragraph continuations (while distinguishing actual block boundaries), and add this input to the section-recognition tests.
There was a problem hiding this comment.
Real, fixed in 9b5e4f2. A non-blank line that continues a paragraph (not a heading, quote, rule, list marker or fence opener) no longer ends the open list items, so the fence after continued text sits at the item's column and # Real is a section. Red-then-green: your input and two block-start negatives (column-0 fence, heading after a lazy line) are in the section-recognition test.
… fence tracking Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if repo_id is not None and arm.patch is None and arm.base == "guidance:as-skill": | ||
| _check_generated_skill(arm, skills, repo_id, tasks, git_dir) |
There was a problem hiding this comment.
Must-fix: the generated skill is checked for collisions here but never included in the names passed to _check_expected_skills below. With repo id toy, a task declaring expected_skill: toy-guidance, and guidance:as-skill+skill-catalog:skills containing an unrelated lint skill, preflight raises "expected_skill 'toy-guidance' is not installed", although preparation installs it and records it in meta['skills']. Include the generated name in expected-skill validation as well as collision validation, and add a test for this composition.
There was a problem hiding this comment.
Fixed in d89a240: a catalog arm with guidance:as-skill now counts the generated <repo>-guidance skill as installed, so an expected_skill naming it passes. Pinned by a new test in tests/test_repo_skill_arms.py.
| _ROOT, _ROOT_CLOSE, _CLOSE = "<guidance>", "</guidance>", "</section>" | ||
| _HEADING = re.compile(r"^(#{1,6}) (\S[^\r]*)$") # canonical only: one space, no CR; anything else is body text | ||
| _FENCE = re.compile(r"^( *)(`{3,}|~{3,})(.*)$") # the indentation, a run of ``` or ~~~, and what follows it | ||
| _ITEM = re.compile(r" *(?:[-*+]|\d{1,9}[.)]) {1,4}(?=\S)") # a list marker and the one to four spaces after it |
There was a problem hiding this comment.
Must-fix: empty list markers are valid Markdown, but this pattern requires content after the marker. For to_format('-\n ```\n# Real\n', 'xml'), the fence belongs to a list item at content column 2, and the unindented heading ends that item and its fence. Here the - is not recognized, so the fence is treated as top-level and # Real is incorrectly left as body text. Recognize marker-only items (including ordered markers), and add a regression asserting that Real becomes a section; the round-trip check cannot detect this structural error.
There was a problem hiding this comment.
Fixed in d89a240: an empty - or 1. is a list item, so its fence sits at the content column. Pinned by new cases in tests/test_repo_guidance_format.py.
| indent = len(expanded) - len(expanded.lstrip(" ")) | ||
| if lazy and not _marker_columns(line) and not _BLOCK.match(expanded) and not _OPENER.match(expanded): | ||
| return items | ||
| return tuple(column for column in items if column <= indent) + _marker_columns(line) |
There was a problem hiding this comment.
Must-fix: thematic breaks take precedence over list markers in Markdown, but _marker_columns('- - -') returns (2, 4) here. For to_format('- - -\n ```\n# not a heading\n ```\n', 'xml'), the fence is consequently assigned column 2 and ends at the unindented hash line, which is incorrectly rendered as a section. In Markdown this is a thematic break followed by a top-level fence, so the hash line must remain code. The round-trip check still passes and does not detect this structural change. Exclude thematic breaks from list-marker detection (also covering * * *) and add a regression asserting that this input produces no sections.
There was a problem hiding this comment.
Fixed in d89a240: - - - and * * * are thematic breaks, not list items. Pinned by new cases in tests/test_repo_guidance_format.py.
| items = _items_after(items, line, para and bool(items)) | ||
| opened = _opened(line, items) |
There was a problem hiding this comment.
Must-fix: ordered lists can interrupt a CommonMark paragraph only when their starting number is 1, but this path treats any numbered marker as a new item. For to_format('paragraph\n2. ```\n ```\n# Real\n', 'xml'), 2. ``` is paragraph text, and the following three-space-indented backticks open a top-level fence, so # Real must remain code. Here the numbered line opens a list-contained fence, the next line closes it, and # Real becomes a section. The round-trip check still passes, so it cannot detect this misclassification. Preserve paragraph context when deciding whether a numbered marker actually opens a list, and add this input as a regression test.
There was a problem hiding this comment.
Fixed in d89a240: an ordered marker not numbered 1 no longer interrupts a paragraph. Pinned by new cases in tests/test_repo_guidance_format.py.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 9b5e4f2. The preflight for the generated skill name is in, and the lazy-paragraph fix works for the case it targets. The fence tracking and the preflight each still have wrong results that the round-trip check cannot see. Four bugs, one risk and one nit are inline; fix the bugs before merge.
What I ran, on a Linux review host at 9b5e4f2:
make ci-local: 30 passed, 0 failed, 7 skipped. Changed-line coverage passes.- Reproductions for the code findings, with no model or network involved.
- A differential run of about 120k random fence/list/blank/lazy line combinations against markdown-it-py, and a mutation run of 23 mutants against the two test files: 19 of them pass the full files (the ones that decide something are in the risk and the nit).
Still open from my earlier review: none. My earlier thread on the generated skill name is fixed.
Checked, no issues
- Arm callers: both
check_armscall sites (repo_run.py:196and:285) passctx.manifest.id, and the name is slugged the same way at preflight and inworkspace.py. - Docs and rules: arm names and the
.claude/skills/<repo id>-guidance/SKILL.mdpath in the three doc pages match the code. The changelog fragment is in place, no new page needs anav:entry, and every non-merge commit carries aSigned-off-by. - Lazy paragraph fix:
- Example:\ncontinued text\n ```\n code\n# Real\ngives a section for# Real.
Bot points I checked that no longer apply at this head (15)
- Copilot's point on a four-backtick block holding a three-backtick line does not apply: a fence closes only on a run of the same character at least as long as its opener.
- Copilot's point on
guidance:as-skill+skill:...being refused by the base-skill check does not apply:arms.pyincludesguidance:as-skillindrops. - Copilot's point on newline translation in
write_textdoes not apply: both writers usewrite_byteson UTF-8 bytes. - exai-bot's point on mixed fence delimiters and lengths does not apply: the close check compares character and length.
- exai-bot's point on the preflight not knowing the removal arm does not apply: same
dropschange inarms.py. - exai-bot's point on a symlink whose target is outside the matched guidance files does not apply:
_link_fileraises aPrepErrorfor it. - exai-bot's point on
write_textnewline translation in both writers does not apply: both write bytes. - exai-bot's point on a link cycle between two matched files does not apply:
_link_fileresolves strictly and refuses a cycle. - exai-bot's point on a backtick in a fence info string and a tab-indented opener does not apply:
_OPENERtakes up to three spaces and_openedrejects a backtick in a backtick fence's info string. - exai-bot's point on lexical
..resolution does not apply:_link_fileusesPath.resolve(strict=True). - exai-bot's point on a fence opened by a list item does not apply at this head:
- ```opens a fence and the heading after the closed block becomes a section. - exai-bot's point on a fence opened on a list continuation line does not apply at this head:
- Example:\n ```\n code\n# Real\ngives a section for# Real. - exai-bot's point on tab-separated list markers does not apply: for
-\t```\n echo hi\n ```\n# Real\na CommonMark parser also finds no heading, and the arm gives the same result. - exai-bot's point on the generated skill missing from the collision check does not apply:
_check_generated_skillrefuses it for askill:or catalog skill and for a base that tracks the directory. - exai-bot's point on an unindented lazy paragraph line ending the item does not apply:
- Example:\ncontinued text\n ```\n code\n# Real\ngives a section for# Real.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| text = line.expandtabs(4).strip() | ||
| return opened, items, opened is None and bool(text) and not _BLOCK.match(text) |
There was a problem hiding this comment.
Must-fix: indented code sets para=True, enabling an invalid lazy list continuation. For to_format('- a\n\n code\noutside\n ```\n# not a heading\n', 'xml'), the six-space-indented code is a code block within the list, not a paragraph. Therefore outside ends the list and the following fence opens at top level, keeping the hash line as code. Here outside retains the item's column 2; the fence is incorrectly attached to that item and ends at the hash line, which becomes a <section>. The round-trip check still passes despite this structural change. Account for indentation relative to the active list content column when setting paragraph state, and add a regression asserting that this input produces no section.
There was a problem hiding this comment.
Fixed in d89a240: indented code is no longer paragraph text, so a following outside line cannot continue it lazily. Pinned by a new case in tests/test_repo_guidance_format.py.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 78fb39b, which fix the nit from my review:
78fb39b:dokimi/tests/test_repo_guidance_format.py:75: added the four requested inputs (quote, rule, heading, blank-then-lazy) to the section-recognition parametrize list.
make ci-local at 78fb39b: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the four bugs and the risk from my review.
The fence tracker of guidance:format=xml now follows CommonMark where the review threads of intel#333 showed it did not: an empty list item opens its content column one past the marker, a thematic break is not a list item, an ordered marker only interrupts a paragraph when numbered 1, indented code is not paragraph text, and a blank line ends an empty item. check_arms also counts the generated guidance skill of a guidance:as-skill arm as installed, so an expected_skill naming it is accepted. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The refusal of a guidance:as-skill arm whose generated skill the arm also ships was tested through repo tasks check only; repo run passes the repo id on its own call, and dropping it there passed every test. Add a repo run test that asserts the refusal before any cell. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| return (opened, items, opened is None and bool(text) and not _BLOCK.match(text) and not code, | ||
| bool(markers) and not text) |
There was a problem hiding this comment.
Must-fix: stripping the list marker before checking _BLOCK makes a blockquote paragraph set para=False, although CommonMark allows its text to continue lazily through both the quote and list containers. For to_format('- > quoted\nlazy\n ```\n# Real\n', 'xml'), lazy consequently drops the list, the fence opens at column 0, and # Real incorrectly remains code instead of becoming a section. Preserve paragraph-continuation state for quoted paragraphs (distinguishing them from non-paragraph quote contents), and add this input to the fence-boundary regression cases with expected sections ['Real'].
There was a problem hiding this comment.
Fixed in 1531c45: a block quote with paragraph text keeps the paragraph state, so - > quoted + lazy keeps the list.
The input is a regression case with sections ['Real'] (plus two pins for a top-level quote and a quoted heading).
…ist item A quote whose content is paragraph text sets the paragraph state, so a less indented line that follows it lazily stays in the list item. Before, the quote marker counted as a block start and dropped the list, so a fence at column 0 opened over the heading after it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| return (quoted and bool(inner.strip()) and not _BLOCK.match(inner) and not _OPENER.match(inner) | ||
| and _ITEM.match(inner) is None) |
There was a problem hiding this comment.
Must-fix: quoted indented code is classified as paragraph text. For '- > code\nlazy\n ```\n# Real\n', stripping the quote marker leaves four spaces before code, which starts an indented code block, not a paragraph eligible for lazy continuation. This helper returns True, so lazy incorrectly keeps the list item open and the renderer produces a Real section. The item should end at lazy, making the subsequent fence top-level and # Real its body text (no sections). Check the remaining indentation after removing the quote markers, and add this case alongside the quoted-heading regression.
There was a problem hiding this comment.
Fixed in cbf5633: after the > markers, four or more columns of indent is code, not paragraph text, so lazy ends the item. Pinned by the - > code case in tests/test_repo_guidance_format.py.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 1531c45. The arm wiring, the link checks and the as-skill preflight hold up, and every earlier thread I checked is fixed. One thing should be fixed before merge: guidance:format=xml breaks files that start with YAML frontmatter, so the arm can measure changed loading rather than markup. The guidance skill also skews the skill metrics of guidance:as-skill arms. Details are in the line comments.
Agreed with @exai-bot on quoted indented code inside a list item: it is real with five spaces after >, for example `- > code
lazy
# Real
`, where this code makes `Real` a section and markdown-it-py finds none. With the thread's own input (`- > code`, one space) both agree.
**What I ran**, at d89a240:
- `make ci-local --offline`: 12,346 tests passed, 0 failed; 18 checks passed, 1 failed. The FAIL is `schema_compat`: it reports schema files that `main` added after this PR's base. The PR does not touch schemas.
- The docker smoke (docker-oracle and docker-probe on the pinned catalog task), at 9b5e4f2: both pass, the isolation probe is closed.
- `to_format` on the inputs from all the fence and list threads, plus frontmatter and an HTML comment, compared with markdown-it-py (CommonMark). The thread inputs now match. The quoted-code case and the HTML comment do not. Setext headings are out of scope by design.
- On Windows (Server 2025, Python 3.13), at 9b5e4f2: `pytest tests/test_repo_guidance_format.py tests/test_repo_skill_arms.py`: 116 passed, 28 failed. 21 of the failures are in `test_repo_skill_arms.py` and fail the same way on the base (a shell `fake-agent`, `os.mkfifo`). The 7 new ones come from POSIX symlinks Windows cannot resolve and from `write_text` fixtures, which write CRLF there. Repository mode runs in Linux containers, so this is not a product bug.
**Checked, no issues**
- **Links:** `_link_file` resolves strictly. A dangling link, a cycle, a link out of the workspace and a link to an unmatched file are refused, for both arms.
- **Preflight:** `drops` covers `guidance:as-skill`. A named or catalog skill, or a base file, at the generated skill path is refused before any cell. An `expected_skill` naming the generated skill is accepted. Both `repo run` and `repo tasks check` pass the repo id, and a test pins it.
- **Docs and changelog:** the fragment is under `Added` and says what a user gets. The arm names in `--arm` help, `ARM_FORMS` and the docs match. The new files carry the SPDX header.
| links.append(rel) | ||
| continue | ||
| try: | ||
| rendered = to_format(_guidance_text(ws, rel, f"guidance:format={fmt}"), fmt) |
There was a problem hiding this comment.
Blocking: guidance:format=xml puts <guidance> above a file's YAML frontmatter, so the harness no longer reads it.
Files the globs can match start with ---: .github/instructions/*.md (applyTo:, in the contract's example globs), .claude/rules/*.md (paths:), .cursor/rules/*.mdc, a SKILL.md. After rendering they start with <guidance>, so a scoped rule loses its scope and a skill its name. A # comment in the YAML also becomes a section:
to_format('---\npaths:\n - src/**\n# a yaml comment\n---\n# Rules\n', 'xml')
# '<guidance>\n---\npaths:\n - src/**\n<section level="1" title="a yaml comment">\n---...'The round-trip check passes, so the arm measures changed loading, not markup.
Suggestion: keep a leading --- ... --- block above <guidance> verbatim (and in restore), or refuse such files; test with a paths: rule.
There was a problem hiding this comment.
Fixed in cbf5633: a leading --- ... --- block stays verbatim above <guidance>, and restore keeps it. Pinned by test_a_yaml_front_matter_block_stays_verbatim_above_the_root.
| if "guidance_skill" in arm_meta: # packaged by guidance:as-skill: usage is measured on it like any skill | ||
| installed.append(arm_meta.pop("guidance_skill")) |
There was a problem hiding this comment.
The packaged guidance skill is counted with the arm's skills, so skill_invoked and skill_selected_correct change meaning for guidance:as-skill arms.
guidance:as-skill+skill:lint, noexpected_skill: loading only the guidance skill givesskill_invoked: True.- Same arm,
expected_skill: lint: loading the guidance first (its description asks for that) givesskill_selected_correct: False. - Plain
guidance:as-skillon tasks with anexpected_skill: every cell reports it not invoked, though the arm never installed it.
Suggestion: record the guidance skill apart (e.g. guidance_skill_loaded in the cell's skills) and leave it out of the names passed to skill_usage.
There was a problem hiding this comment.
Fixed in cbf5633: the generated skill is no longer in the catalog's skills; its usage is recorded as guidance_skill_loaded in the cell (contract updated). Pinned by test_the_generated_guidance_skill_is_measured_apart_from_the_catalog.
| if git_dir is None: | ||
| return | ||
| for task in tasks: | ||
| for path in _base_skill_files(git_dir, task.base)[0]: |
There was a problem hiding this comment.
A base that tracks .claude or .claude/skills as a symlink passes the guidance:as-skill preflight, then every cell fails. _check_generated_skill reads only _base_skill_files(...)[0], and plain guidance:as-skill never reaches _check_shipped, so check_arms passes and _apply_as_skill refuses each cell (.claude is a symlink). Same kind as the case fixed in this thread.
Suggestion: raise for [1] here too, using the shipped cache (one git ls-tree per base, not per task).
There was a problem hiding this comment.
Fixed in cbf5633: check_arms refuses a base that links .claude or .claude/skills for plain guidance:as-skill too, via the shipped cache. Pinned by test_a_base_that_links_claude_refuses_a_plain_as_skill_arm_before_any_cell.
| ``.claude/skills/<repo>-guidance/SKILL.md``: each file under a ``## File: <path>`` line, its text verbatim.""" | ||
| if not matched: | ||
| raise PrepError("guidance:as-skill: the base has no guidance file to package") | ||
| name = slug(f"{manifest.id}-guidance") |
There was a problem hiding this comment.
The generated skill name can break the Agent Skills name rule. slug() keeps . and _, and manifest ids allow both with no length limit, so numpy.dev gives numpy.dev-guidance, which skills/formats.py:50 (_SPEC_NAME, max 64) reports as invalid.
Suggestion: map . and _ to -, collapse repeats, cut to 64 chars, here and in _check_generated_skill; test with a dotted id.
There was a problem hiding this comment.
Fixed in cbf5633: the generated name follows the Agent Skills rule (numpy.dev gives numpy-dev-guidance, capped at 64 chars), in preparation and in the preflight. Pinned by test_the_generated_guidance_skill_name_follows_the_agent_skills_name_rule.
| open_levels: list[int] = [] | ||
| for line in text.split("\n"): | ||
| fence, items, para, empty = _advance(fence, items, line, para, empty) | ||
| heading = _HEADING.match(line) if fence is None else None # a line of a fence is body text |
There was a problem hiding this comment.
A heading line inside a multi-line HTML comment becomes a <section>.
to_format('<!--\n# commented out\n-->\n# Real\n', 'xml') # sections: commented out, RealCommonMark (markdown-it-py) finds only Real, and the section crosses the comment's end.
Suggestion: treat lines from <!-- to the line holding --> as body text, like a fence, and add this input to the section tests.
There was a problem hiding this comment.
Fixed in cbf5633: lines from <!-- to the line holding --> are body text, not sections. Pinned by test_a_heading_inside_an_html_comment_block_is_body_text.
| raise PrepError(f"guidance:format={fmt}: {rel}: {exc}") from exc | ||
| (ws / rel).write_bytes(rendered.encode("utf-8")) # no newline translation: the restore check holds on disk | ||
| written.append(rel) | ||
| return written, ({"guidance_links_kept": links} if links else {}) |
There was a problem hiding this comment.
Nit: guidance_links_kept is a new meta.json key, but the meta.json paragraph of docs/repo-evaluation-contract.md §3 lists only arm_files, arm_rev and arm_patch. Please add a line for it.
There was a problem hiding this comment.
Fixed in cbf5633: docs/repo-evaluation-contract.md §3 now lists guidance_links_kept and guidance_skill.
- a quoted indented code line does not continue a paragraph lazily - a YAML front matter block stays verbatim above the guidance root - a heading inside a multi-line HTML comment is body text - guidance:as-skill: its skill is measured as guidance_skill_loaded, not in skills - the generated skill name follows the Agent Skills name rule (dots, underscores, length) - a base linking .claude or .claude/skills is refused by check_arms - guidance_links_kept and guidance_skill documented in the contract Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| they applied to; the skill records its sha256 and appears in `skills` like any | ||
| installed skill, so `repo report` counts whether the agent loaded it. |
There was a problem hiding this comment.
Must-fix: the generated guidance skill is not recorded in skills, and repo report does not count its usage. _apply_as_skill writes meta["guidance_skill"], _add_arm_use writes the separate result.json field guidance_skill_loaded, and report aggregation only reads skills. Thus a standalone guidance:as-skill run has no reported skill-load count. Replace this claim with the separate metadata/result fields and note that usage must currently be read from result.json, consistent with the updated evaluation contract.
Suggestion:
| they applied to; the skill records its sha256 and appears in `skills` like any | |
| installed skill, so `repo report` counts whether the agent loaded it. | |
| they applied to; the skill's name, path and sha256 are recorded in | |
| `meta.json` under `guidance_skill`. Its usage is recorded separately in | |
| `result.json` as `guidance_skill_loaded` (`null` if the trace is unreadable), | |
| and is not currently aggregated by `repo report`. |
There was a problem hiding this comment.
Fixed in 42e4e5d: the doc now says the skill's usage is in result.json as guidance_skill_loaded and that repo report does not aggregate it yet.
| names = {s.name for s in skills} | ||
| if repo_id is not None and arm.patch is None and arm.base == "guidance:as-skill": | ||
| _check_generated_skill(arm, skills, repo_id, tasks, git_dir, shipped) | ||
| names.add(guidance_skill_name(repo_id)) # the skill preparation installs beside the catalog's |
There was a problem hiding this comment.
Must-fix: preflight accepts guidance:as-skill+skill-catalog:skills with expected_skill="toy-guidance", but the runner passes only meta["skills"] (e.g. ['lint']) to skill_usage. A trace containing Skill('toy-guidance') therefore produces skill_invoked=False and skill_selected_correct=False, even though the expected skill was loaded. Keep generated-guidance usage separate, but either reject this expected-skill configuration or explicitly handle it in selection accounting. Extend the new acceptance test through result generation.
There was a problem hiding this comment.
Open decision: an expected_skill naming the generated guidance skill, on a catalog arm, is accepted (d89a240) but scored from the catalog usage only (runner.py:281), so it reads not invoked. Keep it accepted and count it in selection, or reject it in preflight? I recommend reject.
There was a problem hiding this comment.
Maintainer decision: reject in preflight. Done in d1b1723: a catalog arm with guidance:as-skill refuses an expected_skill naming the generated skill, before any cell.
| if lines and lines[0] == _FRONT: | ||
| for index in range(1, len(lines)): | ||
| if lines[index] == _FRONT: |
There was a problem hiding this comment.
Must-fix: the earlier frontmatter fix does not cover CRLF files. For to_format('---\r\npaths:\r\n - src/**\r\n---\r\n# Rules\r\n', 'xml'), splitting on \n leaves delimiter lines as '---\r', so this check misses the frontmatter and puts <guidance> before it. The round-trip check still passes, but the harness no longer sees the leading path-scoping metadata. Recognize CRLF delimiters without changing their bytes, and add a CRLF-frontmatter regression test.
There was a problem hiding this comment.
Fixed in 42e4e5d: CRLF front matter delimiters match, their bytes unchanged. Pinned by test_a_crlf_front_matter_block_stays_verbatim_above_the_root.
| if comment: # inside an HTML comment block: body text up to the line that closes it | ||
| comment = "-->" not in line | ||
| out.append(line) | ||
| continue |
There was a problem hiding this comment.
Must-fix: comment blocks inside list items never end when their containing item ends. For to_format('- item\n <!--\n# Real\n-->\n', 'xml'), # Real is a top-level Markdown heading (its indentation ends the list and its HTML block), but this branch retains comment=True and emits it as body text. Unlike fences, comment handling bypasses _advance, so the round-trip check cannot detect the incorrect classification. Track the comment's container indentation and reprocess a line outside that container; add a regression asserting that Real becomes a section.
There was a problem hiding this comment.
Fixed in 42e4e5d: a comment block ends when a line leaves its list item; a lazy comment line stays outside it. Pinned in the html-comment cases.
…ds with its list item A front matter block with CRLF delimiters was not recognised, so the root element went above it. A comment block opened inside a list item now ends when a line leaves that item, as a fence does. The guidance-as-skill usage doc now says where the generated skill's usage is recorded. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ed guidance skill A guidance:as-skill arm on a skill-catalog accepted an expected_skill that named the generated <repo>-guidance skill. Its usage is recorded apart from the catalog's, so the catalog usage read the expected skill as never invoked. check_arms now refuses it before any cell; a catalog skill still passes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
LGTM
I checked d1b1723, plus one fix commit of ours on top. All six points from my last review are fixed:
- YAML front matter (LF and CRLF) stays above
<guidance>. - The guidance skill is measured apart, as
guidance_skill_loaded. - The as-skill preflight refuses a linked
.claude. - The skill name follows the Agent Skills rule.
- HTML comment blocks are body text.
- The contract lists the new
meta.jsonkeys.
Merge: safe now. It merges cleanly into current main, 13 commits behind. The PR's tests and test_docs_code_refs.py pass on the merge: 244 passed. Once it is in, #338 (4 files), #332 and #359 (one file each) conflict with it and need a rebase.
What I ran, at d678ed4:
make ci-local --offline: 12,372 tests passed, 0 failed; 18 checks passed, 1 failed. The FAIL isschema_compat: it reports schema files thatmainadded after this PR's base. The PR does not touch schemas.- Three mutants (front matter detection off, the
.claudelink check removed,guidance_skill_loadedalwaysFalse): each one fails a new test. to_formatagainst markdown-it-py (CommonMark) on every thread input, plus 80k random mixes of list, fence, quote, comment and heading lines. Every thread input matches. The random cases differ only in the safe direction: a heading left unconverted, mostly the indented and CRLF headings that are body text by design. None of them turns body text into a section.
Checked, no issues
expected_skillon a catalog arm: naming the generated skill is refused before any cell, as the maintainer chose in the thread.- Preflight cache:
_check_generated_skilland_check_shippedshare onegit ls-treeper commit. - Docs and changelog: the fragment names the Agent Skills name and the
expected_skillrefusal. The docs say thatrepo reportdoes not aggregateguidance_skill_loadedyet.
Fixed in this branch, up to d678ed4:
- A column-0
<!--after a list item was read as a lazy line of the item, so a later comment in the item could end early and turn a##line inside it into a section. d678ed4 makes such a line start a block that ends the item, as in CommonMark. The new case intest_a_heading_inside_an_html_comment_block_is_body_textfails without the fix.
Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at d678ed4. The delta since my last review closes my five threads, and the full suite passes. Three things are worth fixing before merge, all in the new HTML-comment and empty-item tracking in guidance_format.py: two inputs that turn body text into a <section>, and a test gap on multi-line comment blocks. Two doc nits are inline.
What I ran, on a Linux review host at d678ed4:
make ci-local: 30 passed, 0 failed, 7 skipped. 12,425 tests passed, changed-line coverage passes.- Reproductions for the code findings, compared against markdown-it-py (CommonMark), with no model or network involved.
- A differential run of 150k random list, fence, quote, comment and heading line mixes against markdown-it-py, and mutation runs on the new comment, empty-item and paragraph logic.
Still open from my earlier review: none. The generated-skill name in the expected_skill check, the thematic break, empty marker and ordered-marker fence cases, and the preflight pins are fixed.
Checked, no issues
- Arm wiring: both
check_armscallers inrepo_run.pypass the repo id; dropping it at either fails a test.numpy.devgivesnumpy-dev-guidance, an 80-character id is capped at 64. - Generated skill:
guidance_skill_loadedisnullfor an unreadable trace and is kept out of the names passed toskill_usage; theexpected_skillrefusal on a catalog arm is pinned. - Front matter: LF and CRLF front matter stay above
<guidance>, andrestoreputs them back; the mutants fail tests. - Commits and rules: all 20 non-merge commits carry a
Signed-off-by; the changelog fragment is underAdded; no new page needs anav:entry; the new tests write only totmp_path.
Bot points I checked that no longer apply at this head (26)
- Copilot's point on a four-backtick block holding a three-backtick line does not apply: a fence closes only on a run of the same character at least as long as its opener.
- Copilot's point on
guidance:as-skill+skill:...being refused by the preflight does not apply:guidance:as-skillis in thedropsrule inarms.py, and tests pin it. - Copilot's point on newline translation in
write_textdoes not apply: both writers usewrite_byteson UTF-8 bytes. - exai-bot's point on mixed fence delimiters and lengths does not apply: the close check compares character and length.
- exai-bot's point on the preflight not knowing the removal arm does not apply: same
dropschange inarms.py. - exai-bot's point on a symlink whose target is outside the matched guidance files does not apply:
_link_fileraises aPrepErrorfor it. - exai-bot's point on
write_textnewline translation in both writers does not apply: both write bytes. - exai-bot's point on a link cycle between two matched files does not apply:
_link_fileresolves strictly and refuses a cycle. - exai-bot's point on a backtick in a fence info string and a tab-indented opener does not apply: only up to three spaces open a fence, and a backtick in a backtick opener's info string does not.
- exai-bot's point on lexical
..resolution does not apply:_link_fileusesPath.resolve(strict=True). - exai-bot's point on a fence opened by a list item does not apply at this head:
- ```opens a fence and the heading after the closed block becomes a section. - exai-bot's point on a fence opened on a list continuation line does not apply at this head:
- Example:\n ```\n code\n# Real\ngives a section for# Real. - exai-bot's point on tab-separated list markers does not apply: a CommonMark parser also finds no heading for its example, and the arm gives the same result.
- exai-bot's point on the generated skill missing from the collision check does not apply:
check_armstakes the repo id and refuses it for askill:or catalog skill and for a base that tracks the directory. - exai-bot's point on an unindented lazy paragraph line ending the list item does not apply at this head: its example gives a section for
# Real. - exai-bot's point on
expected_skillnaming the generated skill being refused with a catalog arm does not apply as stated: the maintainer chose to refuse it, and the refusal is now the pinned behaviour. - exai-bot's point on empty list markers does not apply at this head: an empty
-or1.opens an item. - exai-bot's point on thematic breaks read as list markers does not apply at this head:
- - -and* * *give no list item. - exai-bot's point on an ordered marker not numbered 1 interrupting a paragraph does not apply at this head.
- exai-bot's point on indented code enabling a lazy list continuation does not apply at this head.
- exai-bot's point on a block-quote paragraph inside a list item losing its paragraph state does not apply at this head.
- exai-bot's point on quoted indented code treated as paragraph text does not apply at this head.
- exai-bot's point on the docs claiming the generated skill is in
skillsdoes not apply: the docs now say usage is inresult.jsonasguidance_skill_loadedand not aggregated byrepo report. - exai-bot's point on
expected_skillnaming the generated skill scoring as not invoked does not apply: a catalog arm withguidance:as-skillnow refuses thatexpected_skillbefore any cell. - exai-bot's point on CRLF front matter does not apply at this head: CRLF delimiters stay verbatim above the root element.
- exai-bot's point on a comment block outliving its list item does not apply at this head: the block ends when a line leaves its item.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 1fe311f, which fix the two nits from my review:
a2717df:dokimi/docs/benchmarking-and-comparison.md:325: "its text verbatim" now reads "(a missing final newline is added)".1fe311f:dokimi/docs/repo-evaluation-contract.md:489: "noskillsobject" is now "noskillsentry" for the generated skill.
make ci-local at 1fe311f: 30 passed, 0 failed, 7 skipped.
GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.
Not approving yet. Still open: the three risks from my review.
# Conflicts: # dokimi/docs/repo-evaluation-contract.md # dokimi/dokimi/repos/workspace.py
…tinues a paragraph - A comment block that opens after a list marker (`- <!-- c`) is a block of its item, so the heading after it is body text once the item ends. The comment is read against the items open before the line. - A heading, quote or rule indented four columns or more past its container continues paragraph text, so a following `-` does not start a list and the fence after it stays a fence. Tests: the `- <!-- c` case and the indented-heading case fail before and pass after; the other new comment cases pass both ways. Checked against markdown-it-py 3.0.0 and a differential run over random documents (the new scanner never differs from markdown-it more than the old one). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
* feat(repo): guidance:format=xml and guidance:as-skill arms Two guidance parts for `repo run --arm`, to settle format and packaging debates with data: the same guidance content in another shape, compared per model. - guidance:format=xml renders each guidance file as a <guidance> element with one <section level title> per Markdown heading. The rendering must restore to the original file exactly, or the cell's preparation fails. - guidance:as-skill removes the guidance files, as guidance:none does, and packages their text into .claude/skills/<repo>-guidance/SKILL.md; the skill is recorded in `skills`, so usage is measured like any skill. guidance:format=md is refused (it is guidance:raw). POML is not offered: its documented components have no heading tag. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): valid skill frontmatter and clean failures for guidance:as-skill Self-review of 78457e2 found three faults in the new arms: - The packaged SKILL.md had a description with ": " in a plain YAML scalar, so the frontmatter did not parse and the skill could not load. - A guidance file that is not UTF-8 raised UnicodeDecodeError instead of a PrepError for the cell (guidance:format and guidance:as-skill share the reader now). - A symlinked guidance file was dropped without a trace; a link to another guidance file is now packaged once under its target, and a link to any other file fails the cell. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): guidance:format fences, symlink checks and LF-safe writes - a fence closes only on a run at least as long as its opener, of the same character, and never inside a longer run of backticks or tildes - guidance:as-skill and guidance:format follow symlink chains: a chain that cycles or ends outside the matched guidance files is a prep error - guidance:as-skill removes the base skill of the same name the arm packages, so check_arms does not refuse it (drops rule also covers as-skill) - guidance files and the packaged SKILL.md are written as bytes, so no platform newline translation runs after the round-trip check Thread: PRRT_kwDOVBg7g86yWVOg Thread: PRRT_kwDOVBg7g86yWVZ2 Thread: PRRT_kwDOVBg7g86yWVOl Thread: PRRT_kwDOVBg7g86yWVZ4 Thread: PRRT_kwDOVBg7g86yWVOr Thread: PRRT_kwDOVBg7g86yWVZ8 Thread: PRRT_kwDOVBg7g86yWVZ6 Thread: PRRT_kwDOVBg7g86yWVZ_ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * List the new guidance arms in repo run help and the docs status Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(repo): guidance fence openers, links through linked directories, guard tests A fence opener follows CommonMark: up to three spaces (not a tab), and a backtick fence's info string holds no backtick. A link of a guidance file is resolved as the filesystem resolves it, so a `..` after a linked directory reaches the file it names, and a dangling link or a cycle is a PrepError. The guard against writes through a linked .claude/skills and a dangling skill directory are pinned by tests. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * test(repo): as-skill packages beside a skill the base ships, no collision Pins the case the review asked for: a base that ships .claude/skills/lint keeps that skill, and guidance:as-skill installs its own skill next to it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(format): a fence opened by a list item ends with the item A fence opened in a list item (- ```) was not seen, while its closing delimiter, indented to the item's column, opened a fence that never closed, so the headings after the block stayed Markdown. A fence now records the column of its item: its closer may be indented up to three columns past that column, and a line indented less than it ends the item and the fence. Red before the fix: the bullet and the ordered-item cases fail; the list-item tests pass now. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(format): list markers and tabs in fence openers and closers A list marker takes one to four spaces before its content, so five spaces make indented code, not a fence; several markers on one line (- - ```) open an item each. A tab counts to the next multiple of four in a closer as it does in an item's indentation. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(format): a fence on a list continuation line stays in its item A fence opened on a line without a list marker, indented to the open item's content column, took column 0. Its dedent check then never ended it, so a real heading after the list stayed body text, and the round trip still passed. Track the open items (outermost first) across lines: a non-blank line ends the items it is indented less than, a marker opens one per marker, and a fence takes the innermost column. Lines are read expanded for the opener, as for the closer. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(format): annotate the render state so mypy keeps the item tuple type Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): as-skill collisions refused in preflight; tab after a list marker check_arms refuses a guidance:as-skill arm whose generated skill .claude/skills/<repo>-guidance is one of its skills or is tracked by a task's base, before any cell runs, instead of failing cell by cell in preparation. A list marker followed by a tab is read at its expanded columns, as the fence opener is, so the item's fence is recognised; pinned against CommonMark. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): a lazy paragraph continuation keeps the list item open for fence tracking Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * test(repo): pin the lazy-line exclusions in fence tracking Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(repo): track empty list items and rules in guidance fences The fence tracker of guidance:format=xml now follows CommonMark where the review threads of intel#333 showed it did not: an empty list item opens its content column one past the marker, a thematic break is not a list item, an ordered marker only interrupts a paragraph when numbered 1, indented code is not paragraph text, and a blank line ends an empty item. check_arms also counts the generated guidance skill of a guidance:as-skill arm as installed, so an expected_skill naming it is accepted. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * test(repo): pin the repo run call site of the generated-skill check The refusal of a guidance:as-skill arm whose generated skill the arm also ships was tested through repo tasks check only; repo run passes the repo id on its own call, and dropping it there passed every test. Add a repo run test that asserts the refusal before any cell. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): a lazy line continues a block quote's paragraph inside a list item A quote whose content is paragraph text sets the paragraph state, so a less indented line that follows it lazily stays in the list item. Before, the quote marker counted as a block start and dropped the list, so a fence at column 0 opened over the heading after it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): review fixes for the guidance format and as-skill arms - a quoted indented code line does not continue a paragraph lazily - a YAML front matter block stays verbatim above the guidance root - a heading inside a multi-line HTML comment is body text - guidance:as-skill: its skill is measured as guidance_skill_loaded, not in skills - the generated skill name follows the Agent Skills name rule (dots, underscores, length) - a base linking .claude or .claude/skills is refused by check_arms - guidance_links_kept and guidance_skill documented in the contract Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): CRLF front matter stays above the root; an HTML comment ends with its list item A front matter block with CRLF delimiters was not recognised, so the root element went above it. A comment block opened inside a list item now ends when a line leaves that item, as a fence does. The guidance-as-skill usage doc now says where the generated skill's usage is recorded. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): a catalog arm refuses an expected_skill naming the generated guidance skill A guidance:as-skill arm on a skill-catalog accepted an expected_skill that named the generated <repo>-guidance skill. Its usage is recorded apart from the catalog's, so the catalog usage read the expected skill as never invoked. check_arms now refuses it before any cell; a catalog skill still passes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(repo): a column-0 HTML comment after a list item ends the item Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com> * Note that as-skill adds a missing final newline Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * Say as-skill cells have no skills entry for the generated skill Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(guidance): HTML comment after a list marker; indented heading continues a paragraph - A comment block that opens after a list marker (`- <!-- c`) is a block of its item, so the heading after it is body text once the item ends. The comment is read against the items open before the line. - A heading, quote or rule indented four columns or more past its container continues paragraph text, so a following `-` does not start a list and the fence after it stays a fence. Tests: the `- <!-- c` case and the indented-heading case fail before and pass after; the other new comment cases pass both ways. Checked against markdown-it-py 3.0.0 and a differential run over random documents (the new scanner never differs from markdown-it more than the old one). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> --------- Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com> Co-authored-by: Anton Volkov <antonwolfy@gmail.com> Co-authored-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
Summary
Two new
repo runarms that hold the guidance content fixed and change only its shape, so a per-model comparison can say which packaging an agent follows.Not in this PR:
guidance:format=mdis refused (it isguidance:raw).role,task,example,document,table,img,stylesheet,let) have no heading element, so a headings-to-tags mapping needs a decision first.Evidence
guidance:format=xmlandguidance:as-skillare unknown arms;parse_armrejects them.After:
tests/test_repo_guidance_format.py(54 collected, offline): round-trip over 11 inputs (CRLF,#no-space, seven hashes, tilde fences, a title with</section>and quotes); fenced# lines stay body text, with CommonMark fence rules (up to three spaces of indent, no backtick in a backtick opener's info string, a close needs the same character, at least the opener's length, and no info string); a body line that looks like a tag is refused; a non-UTF-8 or non-round-tripping file is aPrepError; a link is resolved as the filesystem resolves it (through linked directories), and a dangling link, a cycle, or a target that is not a matched guidance file is aPrepError; writes are byte-exact (no newline translation); the packaged skill's frontmatter parses withyaml.safe_loadto exactly{name, description}; a base that ships its own skill keeps it beside the packaged one..claude/skillsas a link, or a dangling link at the skill directory, is refused with the host left empty (tests in the same file).~{3,}fence, dropping the info-string close condition, droppingor skill_dir.is_symlink(), reducing the guard loop to.claude; and the converter and frontmatter checks from round 1.make ci-localon e734e16 (round 1): 0 FAIL, 30 pass, 7 skip, exit 0. Two timing-sensitive tests (test_reference_checksregex limit,test_repo_environmentunbalanced quote) exceeded their budgets once under box load; both passed alone and on a clean rerun.:(invalid YAML), non-UTF-8 guidance in as-skill raisingUnicodeDecodeError(now aPrepError), and a silently dropped symlinked guidance file (now packaged once, or aPrepError). Round 2 and round 3 (fresh sonnet pass over each round diff): 0 blocking.Merge Danger
Door: two-way. Two new arm names and an additive grammar; no persisted format, no existing arm changed. Existing
guidance:raw|none|bridgepaths are untouched. Only the new arms write different files.Known limits, stated so the results are read correctly:
guidance:as-skilluses one fixed sentence as the skill description (GUIDANCE_SKILL_DESCRIPTION). Whether the agent loads the skill depends on that sentence, so as-skill results measure "this description" as well as the packaging. The benchmarking guide says so.sub/AGENTS.mdis read as a## File:section, so the agent is not told it applies only undersub/).Blast Radius: low. Only runs that select the new arms are affected.
guidance:formatandguidance:as-skillrefuse a guidance symlink they cannot follow to a matched file (before, it was silently left as is or dropped). Docs:repo-evaluation-contract.md§3 step 5 and a new "Guidance formats and packaging" section inbenchmarking-and-comparison.md. One changelog fragment (### Added).🤖 Generated with Claude Code