Skip to content

fix: read only the files a target emits as an ingredient's body (0.8.2) - #22

Merged
llima merged 26 commits into
mainfrom
fix/cli-0-8-2
Oct 4, 2026
Merged

llima merged 26 commits into
mainfrom
fix/cli-0-8-2

Conversation

@llima

@llima llima commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

Summary

This PR bumps the version to 0.8.2. Merging it publishes craftar@0.8.2 to npm once the release run is approved in the npm environment. 0.8.1 is already published, so the merge order is satisfied.

What changes. Only an ingredient's body files are read for sections and {{param}} citations. A body file is a file a target emits and every emitting target renders as text. A file no target emits, such as a notes.md beside rule.md, is now ignored, and that has three consequences:

  • A malformed marker in such a file no longer fails sync, status, diff, explain, ls or import, and no longer trips the schema: 1 gate.
  • A {{key}} in such a file no longer makes forge unify or import refuse.
  • take: param and take: section on such a file are now refused with … is not emitted by any target.

No emitted byte changes. The README has an Upgrading → to 0.8.2 note, which covers plans saved under 0.8.1.

How a declared file is matched. emittedFile and bodyFile take the ingredient directory, which is required (string | null). They match a declared file/files entry by path.join(dir, …), the same call the emitters use to read it. As a result, every file spelling that 0.8.1 synced still syncs with the same bytes: ./rule.md, /rule.md, ../r/rule.md, a/../rule.md, a script's ./run.sh, and so on. A table test pins this against what 0.8.1 does, as measured on Linux. null is passed only by the two importer sites, where the importer builds canonical names itself.

Errors. When a body file lies outside the ingredient directory, sits behind a symlinked directory, or is spelled with a different letter case on a case-insensitive file system, the error is now a user-facing message naming the file. Before, it was an internal: error.

Each dir call site is covered by a test that fails when its directory is replaced by null. The exception is ctx.text in sync.ts, where the mutation is equivalent: it receives the declared string as written.

Disclosures

  • An existing test was rewritten. In d5e41ad, the U1 test "refuses a variant-only file that would bring markers in…" in test/unify.test.ts was rewritten to use a dir-layout skill, because a rule's notes.md is no longer a body file. The reviewer judged the rewrite faithful to the original intent. A new test pins the relaxed rule-notes.md behaviour (ddcb02a).
  • Two commits are red in isolation. 8e2f9a5 and baf1faa each fail 37 tests in test/unify.test.ts. cdf49c8 restores green, and every later commit is green on its own.
  • Some commit texts are imprecise. History was not amended.
    • The body of 0daeb10 says the regression "broke 0.8.1 syncs". The regression came from d5e41ad; 0.8.1 itself was fine.
    • The subject of af1968d over-promises.
    • The body of 1e40309 claims parity with path.join. That parity only came with defdaf5.
    • cdf49c8 and 26c583f are docs: commits that also change source: extract.ts fallbacks, and the error message in sync.ts.
    • The subject of 02b6c98 says "every" site; the remaining sites are covered by 1b14bf1, 53ade25 and cb946d9.
  • Out of scope, to be recorded as tech debt. The script and hook emitters build output paths from the raw declared string, for example .claude/scripts/./run.sh. 0.8.1 does the same, and this PR does not change it.

Test plan

  • npm run typecheck: exit 0 on fc0b41c
  • npm run build: exit 0
  • vitest without test/ci.test.ts (Linux): 790 passed / 5 skipped
  • Oracle: skipped, no fixture available. The reviewer's spelling probe compared plan() output between 0228d69 and HEAD on 32 cases, and the paths, bytes, warnings and errors were identical.
  • test/golden/** is unchanged.
  • Windows: not run locally; CI ran windows-latest on node 22 and 24, and both passed. The skipIf(win32) symlink test ran only on Linux.
  • CI is green on this PR: ubuntu and windows × node 22 and 24, in runs 37221319156 and 37221341611.

llima added 26 commits October 3, 2026 21:34
Rulings by the user (2026-10-03): (1) a section marker or near miss in a file
no target emits is ignored silently; (2) param citation scans use the same
criterion as sections; (3) take: param or take: section on such a file is
refused with its own message.

Sites changed:
- src/core/extract.ts: added emittedFile() and bodyFile()
- src/core/sync.ts: sectionPass and ctx.text use bodyFile
- src/core/template-import.ts: readBase uses bodyFile
- src/importers/decide.ts: sourceKeys and listAdmitted use bodyFile
- src/core/param-writes.ts: cites() uses bodyFile
- src/importers/claude-code.ts: markerIn uses bodyFile
- src/core/unify.ts: planFrom, S2, S12, P3, declaredElsewhere, checkMarkers
… body

Updated the Forge layout section to clarify that sections are read only in files
a target emits as text. Added a note to the forge unify row about refusing
take: param/section on non-emitted files. Added to 0.8.2 upgrade section.
craftar-cli 0.8.2: files no target emits are not read as an ingredient's body.
A Forge with `file: ./rule.md` or `files: ["./run.sh"]` failed every
sync with an internal error because emittedFile compared paths as exact
strings. This normalizes both the file from listFiles and the meta.file
entry so that `./rule.md` equals `rule.md`.

The test confirms a regression that broke 0.8.1 syncs for such Forges.
…am change back

Pin ruling 2 (F9, 0.8.2): a placeholder in notes.md (a file no target
emits) no longer blocks parameter inference on the parent rule/deploy.

Proved by temporarily reverting listAdmitted to use substitutedFile —
the test then failed with "setting deploy.api would change rule/deploy-
notes"; restored by editing.
…variant

Pin the relaxed U1 for 0.8.2: a file no target emits (like notes.md
beside rule.md) is not read for section markers by checkMarkers, so
take: variant copies it as a raw diff, markers and all.

Proved by temporarily reverting the touched filter to substitutedFile —
the test then failed with U1 refusal "sections none would become n";
restored by editing.
- unify.test.ts: replace if (notesPlan) / if (rmNotesPlan) with
  expect().toBeDefined() and non-null assertions so a silently-empty
  plan fails loudly instead of passing on a no-op.
- param-writes.test.ts: replace .not.toContain() with .toBe("no error")
  for P14 so any unexpected rejection names itself.
- extract.ts: update the substitutedFile doc comment to say that
  whether a file is emitted at all is emittedFile's question and
  bodyFile combines both.
- template-import.ts: all comments that said "admitted" now say
  "body file", matching the function name bodyFile.
- Sections paragraph: define body files explicitly (the file of a rule,
  agent, command or steering; SKILL.md of a file-layout skill; every
  text file of a dir-layout skill; files listed in files of scripts/hooks).
- Marker paragraph: markers are read only in body files; in a file a
  target emits but does not render as text they are copied; in a file no
  target emits they are ignored.
- Params paragraph: only body files are read for {{param}} citations.
- to 0.8.2: split the bullet into two — one for the relaxation, one for
  the refusal of take: param / take: section on non-emitted files.
emittedFile used path.posix.normalize which keeps leading slashes, so
file: /rule.md stayed /rule.md and failed to match rule.md from listFiles.
Now strips leading slashes after normalizing, matching what path.join does.
Adds tests proving every 0.8.1-compatible spelling still syncs.
…h files are body files

0.8.2 bullet clarifies that 0.8.1 accepted both take: param and take: section
on non-emitted files and what to do if such a Forge exists. Sections paragraph
now lists the exact extensions for dir skills and clarifies files emitted but
not rendered as text.
Pins 0.8.2's bodyFile gate in planFrom: notes.md beside rule.md in a rule
is not a body file, so hunks there get no section name and names declared
there do not push rule.md's section to rows-2. Mutating either bodyFile
back to substitutedFile makes this test fail.
… comments

substitutedFile was imported into sync.ts but never used after the 0.8.2
bodyFile gate was added. Comments that said "admitted" now say "body file"
for consistency with the README and the code.
emittedFile and bodyFile now take an optional directory parameter. When
provided, the comparison uses path.join(dir, x) — the same call the
emitters use — so file: ../r/rule.md resolves to the actual file.
Pure unit tests for emittedFile verify that path.join is used correctly
on both POSIX and Windows: backslash is a separator only on Windows.
A file: declaration pointing outside the ingredient directory (../outside.md)
or behind a symlinked directory now fails with a message naming the file and
suggesting to keep it inside the ingredient dir — not as internal:.
…the status row

Clarify that only body files count as a citation for forge unify and import.
Change 'a file' to 'an emitted file' in the status row. Add guidance for
removing params left over from a 0.8.1 take: param on a non-emitted file.
…emittedFile

The dir parameter is now string | null: null only for metadata the
importer builds itself, whose names are canonical.
…n scans

Both tests fail when their respective functions use null instead of
the ingredient directory for path comparison.
…ile stays in its directory

README.md: specify that the upgrade warning is on sync and status only;
add a clause about body files staying inside the ingredient directory.
sync.ts: expand the error message to mention spelling differences.
extract.ts: restore fallbacks for meta.file to support test mocks that
bypass the schema.
…heck

Tests that the dir argument to bodyFile/emittedFile is correctly passed.
With file: ./rule.md in the meta and rule.md on disk, path.join(dir, x)
makes them equal; without dir, the string comparison fails.

Catches mutations at unify.ts :102, :375, :430.
… defaults stay

README: List the exact commands (sync, status, diff, explain, ls) instead
of 'every command that plans'. Add the letter-case caveat for the file
spelling check.

sync.ts: The error message now tells the user to spell `file` as it is
on disk.

extract.ts: Clarify that 'dir is not null' rather than 'dir is provided',
and note why the fallbacks ('rule.md', 'agent.md', etc.) are kept.
Pin four directory argument sites with mutation-verified tests:
- unify.ts:692 (checkMarkers) - U1 marker check
- unify.ts:87 (planFrom) - section name pre-fill uniqueness
- decide.ts:277 (listAdmitted) - F9 Forge-side scan
- sync.ts:118 (emittedFile) - marker warning for non-body files

Site unify.ts:445 skipped: no existing test refuses duplicate section
names through applyPlan. Site template-import.ts:53 skipped: the path
is reachable only for bases with non-canonical file, but such bases
are never reused due to metadata fingerprint mismatch.
…lares

The test verifies that bodyFile at :445 uses the dir argument when building
declaredElsewhere, ensuring ./b.sh is recognized as a body file alongside ./a.sh.
The test verifies that readBase at :53 uses the dir argument when parsing body
files, ensuring ./rule.md is recognized and its malformed markers are checked.
…ed-body-file error

The message now says "spell the declared path as it is on disk" instead of
"spell `file` as it is on disk"; the comment now lists all three causes.
@llima
llima merged commit d65afff into main Oct 4, 2026
8 checks passed
@llima
llima deleted the fix/cli-0-8-2 branch October 4, 2026 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant