take: section in forge unify (0.8.0) - #20
Merged
Merged
Conversation
Spec 12: a hunk can become a section, and a plan can say so. The schema change lets the plan carry take: section and the section object with its name and optional lines. The engine decides; a placeholder refusal in applyPlan throws until the engine code lands.
Spec 12 §6.1–§6.4: the default and value are read off the diff, never the plan, so a plan cannot claim a text the variant does not hold.
Spec 12 §6.4: anchors must be equal lines, or the value would be read from the wrong variant lines.
Spec 12 §6.5: workspaces on the base and on the variant's profile must render their old text; the proof is the gate before any write.
Updated two U1 advice assertions in the test suite: - test/unify.test.ts line 789 - test/cli.test.ts line 1871 The old advice text "editing sections through unify is not supported yet" is now "take base for the marker lines, or take: section to fill the section".
Ruling 3: the human changes keep to section; a pre-filled name never decides anything. A block hunk gets a slug from the nearest heading above or section-<n>; a hunk touching an existing section gets that section's exact name.
One definition of a section's span and of "touches", used by derivation and pre-fill. Removes the dead closer variable and thinking comments.
Unify will need the same edit; one round-trip-gated copy.
Spec 12 §6.6: S13–S16 checks for section extractions, profile edit with both params and sections in one editYamlText call, manifest bump when adding the first section marker.
A new section already in place in the profile still adds markers to the body, so the Forge needs schema: 2. The condition was checking sectionAssign (what gets written to the profile) instead of sections (what adds markers).
Spec 12 §6.7: each write prefix leaves every workspace rendering its old text. The manifest is written first when it moves to schema: 2. Spec 12 §4.4/§4.5: text report shows manifest line, section lines with line counts, and W3 warnings for new sections. --json gains sections array and manifestEdited key. Updated existing --json test to include the new keys (sections: [], manifestEdited: false).
Covers spec 12 §10.5 AC 3: after a take: section extraction from a block hunk, and a second extraction into an existing section, no workspace byte moves, AGENTS.md included.
Spec 11 Ruling 9 may re-point a profile from base--<p> to base; nothing else may move.
Spec 12 §9: status line, forge unify row, sections paragraph and Upgrading.
…ailure The two tests were empty and passed vacuously (spec 12 §10.4). Now they exercise the already-in-place code path and late-failure recovery.
…tions included Spec 12 §6.2 and Ruling 1 say a pure-insertion hunk contributes N+1 for from and N for to. The previous logic only included hunks with base lines in the span calculation, causing refusal on insertion-first/last runs. Also fixes --save-plan option text (D6) and S5 error message wording when no explicit lines were specified in the plan.
A reuse-only plan (only existing sections, no new sections, no variant takes, no param takes) was normalizing mixed-EOL bases through mergeFile. Now we skip the write entirely when the body does not change per spec 12 §6.7 step 3.
Add kiro to targets in the golden test profiles and workspaces so the kiro steering assertion runs. The expected Forge diff shows only the targets change (kiro added to all three profiles).
Add table-driven tests for S5 (lines out of range), S12 (variant holds marker), S13 (another profile sets the section), and S16 (profile sections is an alias). Each scenario exits 1, emits its fragment, and the Forge snapshot equals the before state.
…holder branch
Remove dead `|| takes.includes("section")` in the non-section branch of
unify.ts (line 562): if any hunk has take: section, the code follows the
section branch earlier (line 423), so the array cannot include "section"
in the non-section path.
Remove unused ForgeManifestSchema import from src/importers/claude-code.ts
(the manifest extraction was moved to manifest-edit.ts).
Document that the placeholder branch (step 5 of proveSections) is
unreachable with the current architecture: D, V, and K all derive from
the same extractions array, so any placeholder D/V substitutes is also
in K and filtered out by others(). The check is a defensive measure for
future architecture changes.
SF-A: The step 5 placeholder guard of proveSections IS reachable when a
param extraction wraps a placeholder in extra braces ({{k}} token k →
key who produces {{{{who}}}}). The test proves this path via applyPlan
and replaces the incorrect 'unreachable' comment in section-extract.test.ts.
SF-B: An empty span adjacent to another new span now fails at S7 in both plan orders, not at S17 with order-dependent results. The overlap check treats a shared boundary as overlap when either span is empty, and runs are sorted deterministically (by from, to, name). The marker comparator is now a proper total order (at, isOpener, name tiebreaker).
…ding at command level Extract s5CoverageError helper to replace the duplicated if/else choosing between the two S5 wordings. The no-lines wording is tested at the unit level; a command-level test would require overlapping hunks which the diff algorithm doesn't produce, so we document that limitation instead.
Update 'when it adds a Forge's first marker it sets schema: 2' to 'when it adds a section to a Forge still at schema: 1 it sets schema: 2'. Add refusal: 'a span that covers a hunk outside the run (including take: base on a hunk inside a section the plan fills)'.
The non-empty prev vs empty curr check only detected touching boundaries (prev.to === curr.to), not containment. An empty span fully inside a non-empty span slipped through to S17. Using >= catches both cases.
The both-empty overlap check used 'prev.to === curr.to || prev.from === curr.to', which wrongly flagged adjacent positions as overlapping. Two empty spans only overlap when they insert after the SAME line (prev.to === curr.to), not when they are one base line apart (prev.from === curr.to).
Added unit test (section-extract.test.ts) and CLI test (cli.test.ts) for the S5 message when an existing section's span covers a take: base hunk. Removed the false comment claiming this scenario couldn't be tested at the command level.
The comparator comment now explains that the order is total because S7 refuses every empty span sharing a position with another span's marker. After S7 passes, at any position there's either one section's pair (opener before closer) or different sections' markers (closer before opener, then by name).
The previous test had no assertion (review round 4). The hunks are built by hand because the diff never splits one insertion. The comment above the S7 loop now names the containment case.
craftar-cli 0.8.0: take: section in forge unify (spec 12).
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
1 participant
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
craftar@0.8.0to npm after approval in thenpmenvironment.forge unifycan now turn a client block into a section (spec 12 in the workspace). A hunk set totake: section, withsection: { name, lines? }, works like this:sections;lineswidens it over equal base lines;take: paramandtake: sectioncan share a plan.schema: 1,unifysetsschema: 2incraftar.forge.yamlin place, as the first write.manifestWithSectionsis shared withimport; import's bytes and messages are unchanged.take base for the marker lines, or take: section to fill the section. Two existing assertions changed for that text only:test/unify.test.ts(malformed markers) andtest/cli.test.ts(flavors would become none).--save-planpre-fillssection: { name }:section-<n>;take: baseinside a section the plan fills;sync,resolveor importer behaviour changed. No generated byte moves for workspaces already in sync. Every pre-existing golden is byte-identical tomain.ff96788says it pins the no-linesS5 wording at command level. It only added a comment; the pinning is in88442b3.f25aacbalso changes the--save-planhelp text and the S5 wording; its body says so, its subject does not.Test plan
npm run typecheck: exit 0npm run build: exit 0npx vitest run --exclude test/ci.test.tson Linux: 29 files passed / 1 skipped, 759 passed / 5 skipped / 0 failedtest/golden-take-section.test.ts) extracts a section, fills it from a second client, and re-imports. Every workspace staysunchanged(AGENTS.md and.kiro/included), and a negative control showsupdate.take: section(invalid_enum_value, exit 1, Forge untouched); measured by handtest/ci.test.tsand Windows were not run locally)