fix: read only the files a target emits as an ingredient's body (0.8.2) - #22
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR bumps the version to 0.8.2. Merging it publishes
craftar@0.8.2to npm once thereleaserun is approved in thenpmenvironment. 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 anotes.mdbesiderule.md, is now ignored, and that has three consequences:sync,status,diff,explain,lsorimport, and no longer trips theschema: 1gate.{{key}}in such a file no longer makesforge unifyorimportrefuse.take: paramandtake: sectionon 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.
emittedFileandbodyFiletake the ingredient directory, which is required (string | null). They match a declaredfile/filesentry bypath.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.nullis 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
dircall site is covered by a test that fails when its directory is replaced bynull. The exception isctx.textinsync.ts, where the mutation is equivalent: it receives the declared string as written.Disclosures
d5e41ad, the U1 test "refuses a variant-only file that would bring markers in…" intest/unify.test.tswas rewritten to use a dir-layout skill, because a rule'snotes.mdis no longer a body file. The reviewer judged the rewrite faithful to the original intent. A new test pins the relaxed rule-notes.mdbehaviour (ddcb02a).8e2f9a5andbaf1faaeach fail 37 tests intest/unify.test.ts.cdf49c8restores green, and every later commit is green on its own.0daeb10says the regression "broke 0.8.1 syncs". The regression came fromd5e41ad; 0.8.1 itself was fine.af1968dover-promises.1e40309claims parity withpath.join. That parity only came withdefdaf5.cdf49c8and26c583faredocs:commits that also change source:extract.tsfallbacks, and the error message insync.ts.02b6c98says "every" site; the remaining sites are covered by1b14bf1,53ade25andcb946d9..claude/scripts/./run.sh. 0.8.1 does the same, and this PR does not change it.Test plan
npm run typecheck: exit 0 onfc0b41cnpm run build: exit 0test/ci.test.ts(Linux): 790 passed / 5 skippedplan()output between0228d69and HEAD on 32 cases, and the paths, bytes, warnings and errors were identical.test/golden/**is unchanged.skipIf(win32)symlink test ran only on Linux.37221319156and37221341611.