Phase 16 — final review and cleanup#281
Open
leeovery wants to merge 28 commits into
Open
Conversation
Post-Phase-8 (imports) and Phase-15 (analysis caches), the rebuild discovery pipeline emits more than completed phase artifacts. Drop the "completed" qualifier so the abort message reflects reality. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…it-phase header Phase 8 deferred this for continue-feature. Mirror continue-epic's imports_count derivation and surface a callout signpost before the phase-revisit logic so a returning user sees the seed material count. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… target Pre-Phase-8 features had no imports/ folder; rm -rf was harmless. Post Phase 8 it silently discarded seed material that drove the feature's research. Add Imports Check + Move Imports sections (full letter renumber) so imports are carried forward to the target epic — files moved, manifest entries pushed with preserved imported_at, KB indexed under the new identity. The pre-existing remove --work-unit at cleanup drops the source chunks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Simulate the absorb-into-epic flow at the KB layer — index imports under the feature, move files + re-index under the target epic, drop source chunks. Asserts chunks land under the target identity and the source identity has none after cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ment menu Per design.md, all non-first inception sessions surface prior context via KB retrieval. The four downstream processing skills wire contextual-query.md; refinement did not. Add an A.5 section between Read State and Resume Check that builds a query from the existing map's topic names and loads contextual-query.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… branch A resumed first-session is a non-first inception entry — design says those surface prior context via KB retrieval. Wire contextual-query.md into the c/continue branch before routing back to SKILL.md Step 2. The r/restart branch is unchanged: a fresh restart returns to the launchpad state where imports auto-read applies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ads with established convention Drop the non-conventional "passing the constructed query and --boost:work-unit" trailing clause on both load directives. All four existing contextual-query.md call sites use a bare load — the reference itself constructs the query (its section A) and applies the work-unit boost (its section B). Pre-constructing at the call site duplicated that responsibility and risked Claude double-querying. Also rename refinement-session.md's inserted "A.5 Surface Prior Context" to "B. Surface Prior Context" and renumber the trailing sections B→C through K→L. The "A.5" name had no precedent in the codebase — CONVENTIONS.md only sanctions decimal sub-numbering for Step 0 sub-steps. All routing arrows and body-text section refs updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…md renumber SKILL.md context-refresh recovery instructions and discovery.cjs's conclusion-detection comment both pointed to "B. Resume Check" — that section is now "C. Resume Check" after the prior commit inserted "B. Surface Prior Context". Cosmetic only; no behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Rename block read and restored summary, routing, and source but never read description — silently dropping the field on every rename of an item that had one. Phase 14 added description as a first-class field on inception items; rename hadn't been updated to match. Mirror the existing exists-probe-then-get pattern from workflow-research-entry/references/invoke-skill.md: probe first so a bare get on a missing field doesn't error, conditionally write only when the source had a description so items that genuinely have none stay shape-consistent. Tests cover both branches: description preserved when present, field absent on target when source had none. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…driven style Initial fix used inline bash if/then/fi inside a fenced block — the only such usage anywhere in the skill references. Restructure to match the established prose-driven branching pattern (probe with exists, prose says "if the probe returns true, read", separate code blocks for required vs optional writes). Same runtime behaviour; matches the codebase convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous restructure split the rename across multiple fenced blocks
to remove inline if/then/fi — but that broke shell-var persistence
(each fenced block is a separate Bash invocation; \$saved_summary
set in block 1 isn't visible in block 4). Shell-var capture is also
a thin pattern (3 files codebase-wide, all pre-existing).
Switch to the conventional placeholder model: Claude reads each
field, holds the value in conversation memory, substitutes the
literal value into the next command via {summary value}-style
placeholders. Matches CONVENTIONS.md template-placeholder syntax and
removes both the broken-persistence trap and the non-conventional
shell-var pattern from this block.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…der pattern
Previous attempts deviated from the codebase convention:
- shell-var capture (\$saved_X) broke across separate Bash invocations
- two-word placeholders ({summary value}) violated single-token convention
- single quotes diverged from the file's own Add/Edit blocks
Match the canonical pattern from epic-display-and-menu.md (used for
previous_status read+restore): bare get commands, prose connector
"Use the returned values as {X} in the next commands", single-token
placeholders substituted directly into the next bash block. Quoting
matches the file's existing Add/Edit conventions — double quotes
around free-text fields (summary, description), unquoted enums
(routing, source).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…enumber The template's prose at line 41 referenced "D. Initialise Session Log" and "C. Self-Healing Check" — but Phase 16's insertion of "B. Surface Prior Context" into refinement-session.md bumped Self-Healing from C to D and Initialise Session Log from D to E. C in the current file is Resume Check, an entirely different concern. Fix: D → E, C → D. Final stale-letter ref in the inception skill; SKILL.md context-refresh ref was corrected in 533afe8. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… topic
The absorption flow created discussion/research/imports entries in
the target epic but never wrote phases.inception.items.{topic}. The
discovery map (refinement view, continue-epic display, convergence
counts) is built from phases.inception.items — absorbed topics were
invisible to all of it. Pre-existing bug exposed by the inception
phase initiative.
Add J. Register Inception Item between I. Move Imports and (new)
K. Cleanup. Writes init-phase + routing only, mirroring migration
038's pattern: summary, description, and source are left unset so
the next /continue-epic entry routes the topic through summary-
backfill where the user reviews derived values.
Routing reflects work already done on the feature: research if the
feature had a research phase, otherwise discussion.
Renumber: old J. Cleanup → K, old K. Post-Absorption → L. All
internal routing arrows updated.
Tests in test-absorb-into-epic.cjs exercise the CLI sequence the
new section prescribes, covering both routing branches, field-shape
back-compat (no summary/description/source written), discovery-map
visibility, and coexistence with pre-existing inception items.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
M3 fixed description preservation but kept the original assumption that summary is always present. Three write paths land inception items without a summary: - migration 038 (routing + source only) - ensure-inception-item.md D-step (caller-supplied, optional) - M5 absorption registration (just shipped, no summary) All three can land items at fresh lifecycle — exactly what rename requires. The pre-existing bare get summary would abort rename on those items. Apply the exists-probe pattern to summary (matching description). routing and source remain unconditional reads — verified across all write paths that both are always written. Test added for the migration-seeded shape (no summary, no description). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… text Phase 16 introduced these contextual-query loads with the full relative path used as the link text — convention is filename only, matching every other cross-skill Load directive in the codebase (e.g. compliance-check.md, knowledge-usage.md, convergence-analysis.md all use just the filename even when the target lives two directories away). Link target unchanged; only the visible label shrinks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The enum listed bare "split" and "elevation" — but the code writes
"research-split:{parent}" and "discussion-elevation:{parent}". Also
omitted the comma-accumulation pattern used when multiple sources
surface the same topic (e.g. "inception,research-analysis").
Update the enum to list the parametric forms and note the
accumulation grammar.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The discovery-map/ directory is throwaway design scaffolding that will be deleted once the initiative merges. CLAUDE.md is permanent runtime docs. Remove the pointer to discovery-map/design.md so the file doesn't ship with a dead reference. The lifecycle states list that the sentence introduced remains in place — it's self-contained. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the idea of letting OpenAIProvider point at any OpenAI-compatible embeddings endpoint (LM Studio, Ollama, vLLM, etc.) via a base_url config field — zero bundle cost, unlocks local embeddings without a new provider class.
The prose told callers to "quote with single quotes" while the code
block uses double quotes — and the codebase has 14 other sites that
also use double quotes for summary/description writes. Single
quotes would also break on apostrophes in English prose (a likely
input for these fields).
The code shows the quoting style. Strip the misleading prose; keep
the conditional guard ("if supplied and non-empty") since that's the
actual signal Claude needs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sted Step 5 The "Conclude inception?" prompt offered n/no with "Stay in the session for further refinement" — but by Step 7 the manifest writes from Step 5 have already landed and been committed. Returning to Step 3 (Session Loop) and looping back through Step 5 would crash on init-phase: the items already exist. Drop the menu entirely. By the time this reference loads, the map is seeded. Further changes route through standard refinement via /continue-epic, which is its own flow with idempotency built in. Result: section A is now Final Sweep (was B), section B is now Bridge (was C). Old section A (Final Confirmation menu) removed entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bsent menu Left a paragraph explaining why there's no n/no option after removing it. Skill files are runtime instructions, not change logs — Claude doesn't need to know about what isn't there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Direct unit tests for four exports that were only exercised transitively (computeTopicLifecycle, computeNextAction, computeMapSummary, TIER_RANK) — a refactor could previously change their semantics without any test failing. - computeTopicLifecycle: all 6 lifecycle branches + single-cancelled fall-through to fresh + discussion-status-wins precedence. - computeNextAction: every lifecycle × routing combination. - computeMapSummary: zero counts, full counting, unrecognised tier fallthrough. - TIER_RANK: rank ordering asserted directly. For the chunker, analysis.json was outside the validation loop and had no shape-specific tests. Add it to the loop, mirror the imports-config confidence assertion, and cover both expected shapes: single H2 section (Topics) preserved whole, and H3 fallback splitting when the H2 section exceeds max_lines. 27 new tests; 681 pass total. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The harness spec said "set -eo pipefail" but all 38 migration test files use "set -euo pipefail" (the -u flag adds undefined-variable checking on top). The doc was the outlier, not the tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 16 corrected "No completed artifacts found to index" → "No artifacts to index" but no test asserted the new wording. Add Test 63b: run rebuild on an empty project, confirm new wording present and stale wording absent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ders
Per CLAUDE.md's Output Format References rule, concrete format names
(tick / linear / local-markdown) should appear ONLY in
output-formats.md, the per-format directories, and README.md. Four
pre-existing leaks remained from before Phase 13's documentation
cleanup:
- workflow-manifest/SKILL.md:44, 182 — example values used
"local-markdown"; replaced with {format} placeholder.
- workflow-manifest/SKILL.md:359 — table row listed three format
names; replaced with a pointer to output-formats.md.
- continue-epic/references/epic-display-and-menu.md:152 — display
example used "linear"; replaced with {format} placeholder.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing fields M5's absorption flow (commit ea8e6d5) creates inception items with routing only, relying on the next /continue-epic entry to surface them via summary-backfill. But Step 6's filter was narrowed to source.startsWith('migration-seeded') — absorbed items default to source='inception' at render time and were silently excluded. The absorbed topic would sit on the discovery map forever with null summary/description until manually edited. Broaden the filter to source-agnostic: any item with missing summary or description surfaces for backfill. Stronger semantic: "if a topic exists with incomplete metadata, prompt the user to fill it." Catches migration-seeded items (existing behaviour) and absorption-registered items (new). Future write paths that land items with missing fields will also be caught without further filter changes. The Phase 14 expansion note (catching description-null items) is absorbed into the new general contract — no longer a special-case. Test contract updated: filter is now `!summary || !description`. New fixture row 'absorbed-no-fields' (routing only, no source) proves absorption items are caught. Other fixture rows unchanged; expected- recover set grows by one (the absorbed row). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Phase 16 — final implementation PR for the inception/discovery-map initiative. Catch-all for four independent items surfaced during prior phase reviews; none block but each closes a gap.
absorb-into-epic.md; carries files forward, pushes manifest entries with preservedimported_at, indexes under the target epic identity. Source chunks are dropped by the pre-existingremove --work-unitcleanup.continue-featuresurfacesimports_count. Mirrorscontinue-epic's derivation; renders a signpost callout inrevisit-phase.mdsection A before the early-return branch so it fires on every feature continuation with imports.src/knowledge/index.js; bundle rebuilt.contextual-query.mdintorefinement-session.md(new A.5 section) andfirst-session-resume.md(c/continue branch). Closes the design gap where all non-first sessions should retrieve via KB, but inception's two re-entry paths started blind.Stacks on
idea/inception-pr-15-kb-index-analysis. After this lands, the initiative is done.Test plan
node --test tests/scripts/test-*.cjs— 646 tests passbash tests/scripts/test-knowledge-cli.sh— 218 pass (includes new absorption KB-fidelity test)npm run buildproduces identical bundle to committed/continue-featureshows imports calloutknowledge queryfires before menu rendersknowledge queryfires; r/restart does not🤖 Generated with Claude Code