Repository navigation
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.
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
| 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.20% 94.31% +0.10%
==========================================
Files 304 309 +5
Lines 38748 39511 +763
Branches 6561 6692 +131
==========================================
+ Hits 36504 37266 +762
- Misses 1430 1431 +1
Partials 814 814
🚀 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.
| skills = resolve_skills(arm, content_dir) | ||
| drops = arm.patch is None and (arm.base == "guidance:none" or arm.rev is not None) | ||
| 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.
A task's expected_skill naming the generated guidance skill is refused when the arm also carries a catalog. I agree with exai-bot here; what I add is the run. _check_expected_skills at arms.py:210 gets only {s.name for s in skills}, so the skill that guidance:as-skill installs itself (workspace.py, recorded in meta["skills"]) is never in the set.
- Repo
toy, task withexpected_skill: toy-guidance, armguidance:as-skill+skill-catalog:skillswith a catalog holding onlylint:check_arms(..., "toy")raisesArmError: ... task t's expected_skill 'toy-guidance' is not installed (installed: lint). - The same task with plain
guidance:as-skillpasses, andexpected_skill: lintwith the catalog arm passes. Repo idMy_Repobehaves the same. - Preparation would install the skill, so this is a false refusal of the very measurement the arm is for (does the agent load the packaged guidance).
Suggestion: when repo_id is given and the arm is guidance:as-skill, add slug(f'{repo_id}-guidance') to the names passed to _check_expected_skills. Add a test with a catalog arm, a task with that expected_skill, and check_arms(..., repo_id).
There was a problem hiding this comment.
Fixed in d89a240, same change as the exai-bot thread: a catalog arm with guidance:as-skill 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.
A thematic break (- - -, * * *) is read as nested list markers, so a # line inside the fence after it becomes a section. I agree with exai-bot here; what I add is the CommonMark comparison. _ITEM matches - - - as two markers, and _marker_columns (:60) gives the fence column 2, so the unindented # line ends the fence.
to_format('- - -\n ```\n# not a heading\n ```\n', 'xml')gives<section level="1" title="not a heading">. markdown-it-py parses an hr and a top-level fence and no heading.'* * *\n ```\n# not a heading\n ```\n'behaves the same.restore(...)equals the input, so the round-trip check passes and the cell measures a file whose code comment became a section.
Suggestion: in _marker_columns, return () when the line matches the rule alternative of _BLOCK (or reuse that pattern), and add both inputs to test_a_fence_opened_by_a_list_item_holds_body_text_only_inside_the_item with [] as the expected sections.
There was a problem hiding this comment.
Fixed in d89a240, same change as the exai-bot thread: - - - and * * * are thematic breaks, not list items. Pinned by new cases in tests/test_repo_guidance_format.py.
| return tuple(columns) | ||
|
|
||
|
|
||
| def _items_after(items: tuple[int, ...], line: str, lazy: bool = False) -> tuple[int, ...]: |
There was a problem hiding this comment.
An empty list marker (- alone on a line) opens no item, so a column-0 line that CommonMark reads as a new fence is read as the closer, and a later # line inside that fence becomes a section. I agree with exai-bot here; what I add is the harmful input. _ITEM requires (?=\S) after the marker, so _items_after never records the item.
to_format('-\n ```\n```\n# Real\n', 'xml')gives a section forReal. markdown-it-py: the item ends at the column-0 line, which opens a second fence, so# Realis code.to_format('-\n ```\n# H\n```\n# Real\n', 'xml')gives a section forRealand none forH. markdown-it-py gives the opposite:His a heading,Realis code.- The simple case in the bot's report (
-\n ```\n# Real\n) only leavesRealunconverted, which is the safe direction, but the inputs above are not.
Suggestion: let _ITEM match a marker followed by end of line (content column = marker width + 1), and add the three inputs to the list-fence test.
There was a problem hiding this comment.
Fixed in d89a240: an empty - opens an item, so the column-0 fence line closes its fence instead of being read as a section. Pinned by new cases in tests/test_repo_guidance_format.py.
| expanded = line.expandtabs(4) # a tab is indentation up to the next multiple of four | ||
| if not (line.strip() and len(expanded) - len(expanded.lstrip(" ")) < fence[1]): | ||
| return (None if _closes(fence, line) else fence), items, False | ||
| items = _items_after(items, line, para and bool(items)) |
There was a problem hiding this comment.
Any ordered marker is allowed to interrupt a paragraph, but only a list starting at 1 can. I agree with exai-bot here; what I add is the CommonMark comparison. _advance already knows para, but _items_after only uses it for the lazy test, so a numbered line after paragraph text always opens an item.
to_format('paragraph\n2. ```\n ```\n# Real\n', 'xml')gives a section forReal. markdown-it-py gives a paragraph and one top-level unclosed fence, so# Realis code.- The round-trip check passes, so nothing flags it.
Suggestion: when para is true and no item is open, treat an ordered marker whose number is not 1 (and any marker with empty content) as paragraph text in _marker_columns/_items_after. Add the input with [] as the expected sections.
There was a problem hiding this comment.
Fixed in d89a240, same change as the exai-bot thread: an ordered marker not numbered 1 no longer interrupts a paragraph. Pinned by new cases in tests/test_repo_guidance_format.py.
| def _check_generated_skill(arm: Any, skills: list[Skill], repo_id: str, tasks: list[Any], git_dir: Path | None) -> None: | ||
| """Refuse a ``guidance:as-skill`` arm whose generated skill directory is a skill of the arm, or is tracked by the | ||
| base of a task: preparation writes that directory first, so it would fail cell by cell, not here.""" | ||
| target = f"{SKILLS_DIR}/{slug(f'{repo_id}-guidance')}" |
There was a problem hiding this comment.
The new preflight is not pinned where it decides whether a batch starts. Mutation check on _check_generated_skill and its callers: each of these passes both new test files (144 passed):
- removing
ctx.manifest.idfrom the call atrepo_run.py:196or at:285(no test reaches either, so the preflight silently turns off and the collision fails per cell again; 313 repo-run tests also pass); - dropping
arm.base == "guidance:as-skill"atarms.py:202, which makesguidance:none+skill:skills/toy-guidancerefuse; - dropping
path == target or(a base tracking a plain file at the skill path stops being refused), or changingtarget + "/"totarget(a base file under.claude/skills/toy-guidance2/is refused); - removing the
slug(...)on this line (repo idmy repothen misses a skill namedmy_repo-guidance).
Suggestion: add tests for a non-as-skill arm with that skill name (accepted), a base tracking a plain file at the target, a sibling toy-guidance2 (accepted), a repo id that slugs differently, and one repo tasks check or repo run test whose arm collides, asserting the refusal.
There was a problem hiding this comment.
Fixed in d89a240. Your mutations now fail tests: a repo run refusal test (dropping the repo id at repo_run.py:285 fails it), plus cases for a plain file at the skill path, the -guidance2 sibling, guidance:none, and the slug of the repo id.
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'].
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