docs(finalize): make nw-finalize consistent on preserving workspace - #85
docs(finalize): make nw-finalize consistent on preserving workspace#85tburny wants to merge 6 commits into
Conversation
nw-finalize stated both that docs/feature/{feature-id}/ is preserved and
that it is removed. Phase C step 3 of the skill was hardened to preserve
the directory, but the change never reached the Overview, Phase D step 5,
the Success Criteria, Example 1, the Deliverables line, or the Expected
Outputs block -- nor either copy of the command asset, which still carried
a literal `rm -rf docs/feature/{feature-id}/`.
ASSUMPTION (maintainer may reject cheaply): PRESERVE is canonical. Phase C
step 3 is the only passage that names a system-level consequence -- the
wave matrix derives feature status from this directory, so deleting it
makes finalized features disappear from the matrix. Deletion silently
destroys data another part of nWave reads; preservation is merely untidy.
All passages are now aligned on preserve, and Phase C/D delete session
markers and resume state only.
Aligned assets:
- nWave/skills/nw-finalize/SKILL.md
- nWave/tasks/nw/finalize.md
- plugins/nw/commands/finalize.md (byte-identical duplicate of the task)
A regression guard asserts that no finalize asset claims workspace removal
and that each states why the workspace is retained.
Issue: nWave-ai#83
|
The following files are on the sensitive-file denylist and cannot be
These files are typically managed by the release pipeline or contain This PR remains open for discussion. No mirror branch has been created. |
The first pass aligned the body passages but left the two highest-visibility lines untouched in all three assets: the frontmatter `description` (which drives skill dispatch) and the one-line Overview both still promised to "clean up the temporary workspace". A reader who never opens the body sees only those two lines, so the contradiction survived where it was most likely to be read. Wording follows nWave-ai/nWave-experimental, which already resolved this the same way: "preserves the feature workspace, and cleans session artifacts". The migration table's "Source (temporary workspace)" header is likewise no longer temporary. Issue: nWave-ai#83
|
Pushed a follow-up commit ( A skill-quality evaluation of the changed The wording now follows A repo-wide search for |
|
The following files are on the sensitive-file denylist and cannot be
These files are typically managed by the release pipeline or contain This PR remains open for discussion. No mirror branch has been created. |
|
Question on the This PR edits the marketplace payload under I kept the payload edits rather than stripping them, deliberately, because the asymmetry favours it: if the payload IS regenerated, these hunks are harmless noise that your pipeline overwrites; if it is NOT, stripping them would ship a marketplace copy contradicting the source — which is exactly the divergence issue #83 describes. If the payload is generated, say so and I will drop those hunks from this PR. Nothing else in the diff depends on them. |
plugins/** is on the repository's sensitive-file denylist and cannot be changed through the external PR flow -- the mirror bot refuses to create a nwave-dev branch while those paths are touched. The payload is reverted to its committed state and the regression guard now covers only the two source assets. This leaves plugins/nw/commands/finalize.md asserting workspace removal while its source counterpart asserts preservation. If the payload is regenerated from nWave/tasks/nw/ at release time that resolves itself; if it is not, it needs a maintainer-side edit that an external contributor cannot make. Issue: nWave-ai#83
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
Review found the first pass fixed only half the contradiction. The workspace was
declared preserved while the 'What NOT to Migrate (Discard)' table still listed
deliver/execution-log.json, roadmap.json and the wave-decisions files under a
'Why discard' column introduced by 'disposable after'. An agent following the
table would gut the preserved directory: des-verify-integrity and /nw-continue
both read execution-log.json, and every Success Criteria box would still tick
green because the directory itself existed.
The table is now titled 'What NOT to Migrate (stays in the workspace)' and says
plainly that not-migrated means not-copied, never deleted.
Four further defects the same review surfaced:
- Approval gate. Phase C listed every remaining file, showed that list for
approval, then deleted a narrower set including .nwave/des/deliver-session.json
-- a path outside docs/feature/ that could never appear in the listing. The
step now lists exactly the deletion candidates by path, so the approved set and
the deleted set are the same set.
- Idempotency. Preserving the workspace makes finalize re-runnable, and the
'project directory not found' error was the de facto guard against a second
run. This change removed its effect, so a second run would write a conflicting
evolution doc and overwrite permanent copies edited since the first. Phase A
now refuses when an evolution doc for the feature already exists.
- Corrupt-input handling. The completeness gate parses execution-log.json but
the error table had no row for it being missing, unparseable or empty -- an
empty log satisfies 'every step is DONE' vacuously. Three BLOCK rows added.
- Unbounded deletion. 'and any temp files' is now 'delete exactly the approved
paths', with an explicit instruction to leave anything that looks temporary
but was not approved.
The guard's patterns were verified too narrow: the review showed none of them
matched "On approval: delete docs/feature/{id}/", "Deleted: docs/feature/x" or
a quoted rm -rf. Patterns are broadened to destructive-verb-near-path, plus the
'Why discard'/'disposable after' phrasings that made this ambiguity survivable.
Because a line may legitimately say 'removes session markers only -- docs/feature
is retained', lines carrying an explicit retention marker are exempt; that limit
is documented in the test.
Guard now fires on 11 lines of the base file, up from 7, and is clean at HEAD.
Issue: nWave-ai#83
3f0d8f3 to
69d066e
Compare
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
1 similar comment
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
69d066e to
89d08cc
Compare
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
The nw-finalize frontmatter description was changed on this branch to say
the feature workspace is preserved. Copies of the old wording ("cleans up
the temporary workspace") survived downstream and contradicted it:
- nWave/framework-catalog.yaml (hand-maintained registry)
- docs/reference/skills/nw-finalize.md
- docs/reference/skills/index.md
- docs/reference/commands/index.md
The docs/reference/ pages are GENERATED by scripts/docgen.py and were
hand-edited here rather than regenerated. A full `poe docgen` run inside a
git worktree rewrites the "Source" GitHub URL on all 190 pages using the
checkout directory name, which would corrupt every page with the worktree
path. The three edited lines were verified byte-identical to docgen output
rendered to a scratch directory.
plugins/nw/commands/finalize.md also carries the old wording. It is
release-pipeline-generated and on the mirror denylist, so it is left alone.
Also collapse the duplicated wave-status-matrix rationale in SKILL.md — it
was stated in both the Overview and Phase C step 3. The Overview now states
retention only and points at Phase C, where the action happens.
Also make both regression guards in the issue-83 test consistent: the
preservation test asserted path.is_file() before reading, its removal-scan
sibling did not, so a renamed asset gave a bare FileNotFoundError in one
and an actionable message in the other.
Issue: nWave-ai#83
|
Pushed Downstream copies aligned. The corrected frontmatter description had stale copies in A caution about
Also collapsed the wave-matrix rationale that this PR had duplicated across the Overview and Phase C, and made the two guard tests consistent about checking One known limitation: the guard matches the rationale by exact phrase, so a future rewording of Phase C step 3 will trip it. Documented in the test rather than loosened, since loosening it would weaken the thing it guards. |
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
The earlier passes introduced "feature workspace", which does not occur anywhere
in v3.21.0 -- 0 occurrences across skills, tasks, agents, guides and code. It was
inherited by copying nWave-experimental's already-resolved description, where the
term is well established (10 occurrences in skills, 2 in tasks). That is the
4.0.0 atdd-pure track, a different line with its own vocabulary decisions, so
importing the term backwards makes this file the only place in v3 that uses it.
Only the QUALIFIER was ever wrong. The noun is native here: nw-finalize already
says "Scan workspace", "Preserve workspace", "Commit workspace" and bare
"workspace" at base. What contradicted preservation was "temporary" (4
occurrences in the skill, 4 in the task asset) -- a directory that is retained is
not temporary.
So the qualifier is dropped rather than replaced. The file now says "the
workspace", which is what the surrounding v3 prose already said.
The description propagates into nWave/framework-catalog.yaml and three generated
docs/reference pages, so those move with it; leaving them behind would reinstate
exactly the description drift this PR exists to remove. docgen was not run --
it derives page Source URLs from the checkout directory name and corrupts all
190 pages when run from a worktree -- so the four lines were hand-edited.
For the record, v3's vocabulary for docs/feature/{feature-id}/ is genuinely
fragmented: "feature dir" (4 skills / 1 task / 1 agent / 6 guides / 9 code),
"project directory" (3/3/0/4/8), "feature directory" (1/0/0/4/7), "feature
folder" (4/0/0/0/0). "project directory" is not available as a synonym: it is
the --project-dir CLI flag, and it denotes the deliver subdirectory
(docs/feature/<id>/deliver), not the feature root. Unifying those is out of
scope here.
Issue: nWave-ai#83
|
Pushed Earlier passes used "feature workspace", which has 0 occurrences in v3.21.0. I inherited it by copying Only the qualifier was wrong. "workspace" is native here ("Scan workspace", "Preserve workspace" at base); "temporary" is what contradicted preservation. So the qualifier is dropped, not swapped. The description propagates into Guard green (4 passed). |
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
2 participants
Why
nw-finalizetold the agent both to preserve and to removedocs/feature/{feature-id}/, so two runs of the same command on the same feature could legitimately disagree about whether the directory survived. Phase C step 3 of the skill was hardened to preserve it — and states the reason: the wave-status matrix derives feature status from that directory, so removing it makes finalized features disappear from the matrix. That change never reached the six other passages, nor either copy of the command asset, both of which still carried a literalrm -rf docs/feature/{feature-id}/. Whichever passage an agent happened to weight decided whether it destroyed data another part of nWave reads, at the exact moment the feature was declared complete.This aligns every passage on preserve.
What changed
nWave/skills/nw-finalize/SKILL.md— Overview, Phase D step 5, Success Criteria, Example 1, Deliverables and Expected Outputs now state preservation; Phase C/D cleanup is scoped to session markers and resume state only.nWave/tasks/nw/finalize.md— removed the literalrm -rf docs/feature/{feature-id}/; aligned the same six passages.plugins/nw/commands/finalize.md— byte-identical duplicate of the task asset, aligned identically. (Worth noting this file is a full copy ofnWave/tasks/nw/finalize.md; it drifted in lockstep here, but nothing enforces that.)tests/bugs/test_issue_83_finalize_workspace_preserved.py— new regression guard, parametrized over all three assets.Which way the contradiction was resolved, and why
Preserve is treated as canonical. Phase C step 3 is the only passage that names a system-level consequence; the removal passages assert the behaviour without justifying it. Deletion silently destroys data another part of nWave reads, whereas preservation is merely untidy — the asymmetry decides it.
This is not only inference.
nWave-ai/nWave-experimentalhas already resolved the same contradiction the same way:nWave/skills/nw-finalize/SKILL.mdthere states "docs/feature/{feature-id}/is the active feature workspace and remains the feature's living history after finalization", consistently across its description, Overview, and Success Criteria. This PR brings the stable track in line with that.If the intended behaviour is in fact removal, this PR is the wrong shape and should be rejected rather than amended — the fix would then be to delete Phase C step 3 and its stated rationale instead.
Test plan
./.venv/bin/python -m pytest tests/bugs/test_issue_83_finalize_workspace_preserved.py -q— 3 passed (one per asset).1d0f13ccarries 7 removal assertions across the three files, including two literalrm -rf docs/feature/{feature-id}/. At HEAD the same search returns nothing../.venv/bin/python -m pytest tests/des/unit -q— unchanged from the pre-existing baseline (1311 passed, 2 failed, 1 xfail). The two failures are present on unmodified1d0f13cand are unrelated to this change:test_hook_completed_emitted_on_block_pathandtest_pre_tool_use_blocked_event_carries_hook_id.Note for anyone reproducing this:
uv synccurrently fails on a fresh clone of this repo (pyproject.tomlforce-includeslib/python/des, which is untracked here), so the suite was run in a hand-built virtualenv with the repo root onsys.path.Focus areas
plugins/nw/commands/finalize.mdduplicatingnWave/tasks/nw/finalize.mdbyte-for-byte is the underlying reason this drift could happen twice. Deduplicating them is out of scope here, but the duplication is the bug behind the bug.Closes #83