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 4 issue(s) in this PR. By severity: 4 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.28% +0.08%
==========================================
Files 304 309 +5
Lines 38748 39492 +744
Branches 6561 6689 +128
==========================================
+ Hits 36504 37237 +733
- Misses 1430 1435 +5
- Partials 814 820 +6
🚀 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.
| raise ArmError(f"arms {cells[cell]!r} and {arm.spec!r} would share one result directory ({cell})") | ||
| skills = resolve_skills(arm, content_dir) | ||
| drops = arm.patch is None and (arm.base == "guidance:none" or arm.rev is not None) | ||
| drops = arm.patch is None and (arm.base in ("guidance:none", "guidance:as-skill") or arm.rev is not None) |
There was a problem hiding this comment.
A skill: or skill-catalog: skill named <repo id>-guidance passes the arm preflight, and then every cell of a guidance:as-skill arm fails in preparation. check_arms only sees skills the user names. _apply_as_skill creates .claude/skills/<repo id>-guidance itself (workspace.py:540), and _install_skills then finds the destination.
- Repo
toy, armguidance:as-skill+skill:skills/toy-guidance:check_armspasses.prepare_workspaceraisesPrepError: task fix_add: the workspace already has .claude/skills/toy-guidance. guidance:as-skill+skill-catalog:skillswith a catalog skilltoy-guidance: same result.- The error is explicit, but it arrives per cell after the batch has started. A base that ships
.claude/skills/toy-guidance/SKILL.mdsurfaces the same way, since the newdropsentry hides it from the check whileworkspace.py:540still refuses it.
I agree with exai-bot on this point. Suggestion: in check_arms, for guidance:as-skill, refuse a resolved skill whose target is .claude/skills/<slug(repo id)>-guidance (pass the manifest id in), and add a test that calls check_arms and prepare_workspace for both arm forms. Also update the docstring above (arms.py:186), which still names only guidance:none and pinned revisions as the arms that drop base files.
There was a problem hiding this comment.
Fixed in 2c9b57c. The as-skill collision is refused in check_arms (generated name vs the arm's skills, and any file the task's base tracks under it, whatever the drop globs), and the docstring now names guidance:as-skill.
Tests call check_arms for both arm forms and for a base that ships toy-guidance/SKILL.md. The preparation check in _apply_as_skill is kept as the second guard.
… 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.
| _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.
| 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.
| 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.
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