fix: audit remediation + v3.3.0.0 — course memory, pipeline orchestration, re-run detection - #57
Merged
Conversation
- smoke/integration check() now captures and prints failure diagnostics instead of discarding stderr - migration block runs under a trap with per-check guards so a migrate failure records a FAIL instead of killing the suite and leaking a tempdir - version assertions read VERSION instead of hardcoding v3.2.0.0; new VERSION/plugin.json/CHANGELOG agreement checks - new v1.1 migration test exercising the failed_items int->list conversion (fixture existed but was never loaded) - integration-test: stale-file test runs in a sandbox copy, never mutating the tracked tree; trap quoting fixed for paths with spaces - version classifier extracted to bin/lib/version-classify.sh, sourced by setup, idstack-doctor, and the unit test - the test now exercises the shipped code instead of a local mirror - new .github/workflows/test.yml (ubuntu/macos, python 3.9/3.12) running all four suites on push and PR - release.yml now verifies tag == VERSION == plugin.json == CHANGELOG and runs the smoke test before publishing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…or, setup
- idstack-status: PROJECT_NAME now travels via the environment instead of
being interpolated into Python source; an apostrophe in a course name no
longer blanks the dashboard with a SyntaxError. Readiness block failure
prints a message instead of vanishing behind '|| true'. course-import
added to the pipeline progression (it joins before learning-objectives).
- idstack-gen-skills: a template missing {{PREAMBLE}} is now an ERROR
counted by --dry-run and generation (was a silent SKIP that let the
drift gate pass green over an absent output); templates/report-format.md
added to the fail-loud preflight; stale marketplace.json comment removed.
- idstack-doctor: plugin version parsed with python3 json (grep fallback
without python3); enabled-detection scoped to idstack's own list entry
instead of a fixed -A4 window; vestigial-symlink scan is scope-aware.
- setup: --keep-legacy honored in all three legacy-removal paths (was 1 of
3); vestigial-symlink cleanup is scope-aware so --local never touches
$HOME; 'claude plugin marketplace add'/'plugin install' failures now
error loudly with a manual-recovery hint instead of aborting silently
under set -e; dead duplicate rm branch collapsed; Codex link count
reported from actual links created.
- integration-test: new regression tests for the apostrophe project name,
the course-import suggestion, and the missing-{{PREAMBLE}} error path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preamble (affects every generated skill):
- context-recovery f-string with nested same-type quotes replaced with
concatenation; it was a SyntaxError on every Python < 3.12 (macOS system
python3 is 3.9) that died silently behind 2>/dev/null, killing welcome-back
messages, quality trends, and next-skill suggestions since it shipped
- SUGGESTED_NEXT now handles import-first projects (course-import joins the
chain before learning-objectives)
- every preamble bash block re-derives $_IDSTACK — blocks run in separate
shells, so a value derived in an earlier block was never actually available
Canonical $_IDSTACK resolution:
- new {{IDSTACK_RESOLVE}} generator placeholder spliced from
templates/snippets/idstack-resolve.sh into all 47 bash blocks that call
$_IDSTACK/bin tools; learn's and course-export's hand-rolled path lists
(which omitted the marketplace cache — the way most users install — and
included a banned legacy path) are replaced by it. Drift is now
structurally impossible and smoke-test bans raw _IDSTACK= in templates.
Pipeline orchestrator:
- child skills invoked as skill: "idstack:<name>" — the bare name never
resolved in Claude Code, breaking the orchestrator's execution loop
- status table, announcements, and resume hint use /idstack: forms
(including the '/idstack pipeline' space typo); pipeline now logs its own
completion to the timeline
Contract fixes across skills:
- five skills checked non-canonical manifest section names in their re-run
guard (assessment_design, course_builder, course_export, course_import,
course_quality_review) — re-run detection never fired for them; corrected
to the canonical names, plus prose sites and red-team's nonexistent
course_builder.output_path (now course_content.content_dir)
- 8 body-duplicated 'Preamble: Project Manifest' sections slimmed to their
skill-specific deltas
- course-quality-review and course-export (single-section owners) now write
through bin/idstack-manifest-merge; needs-analysis and learning-objectives
document their Write-tool fallback the way course-import models it
- user-facing /skill refs namespaced to /idstack:<skill> (~45 sites) so the
Codex translation rule has a prefix to strip
- learning-objectives: missing 'Top recommendations' report section added;
[Alignment-1] tier corrected from T2 to T5 (Biggs 1996) at both cite sites
- manifest-schema.md report_path claim corrected (idstack-status globs the
export folder); index.html.tmpl carries a concrete course-import row shape
Tests:
- new test/test-preamble-python.sh runs the context-recovery block on the
host interpreter with positive output assertions (the old block produced
empty output on 3.9 and exited 0 — verified); wired into smoke + CI
- smoke-test: 70 new assertions pinning canonical section names, /idstack:
namespacing, the resolve-snippet lockstep, merge-tool usage, and Top
recommendations; integration-test tree check compares before/after state
All 22 generated files regenerated in this commit.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- README: drop the hardcoded 'v2.5.0' status/new-in block (two releases stale); point at CHANGELOG.md so the README can't drift again - TODOS: mark the v2.4 .idstack/reports/ description as historical — the current contract is HTML reports under .idstack/exports/<course-slug>/ - report-format.md: idstack-status discovers reports by globbing the export folder, not by reading report_path — say so Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
VERSION, plugin.json (the line marketplace installs actually serve), CHANGELOG entry, and landing page (hero badge, what's-new card, JSON-LD softwareVersion/dateModified). smoke-test needed no edit — its version assertions read VERSION now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The enabled-detection logic replaced in the previous commit was correct but
untestable while inlined in idstack-doctor. Extracted behind plugin_is_enabled
following the version-classify.sh precedent, with 9 pinned cases.
Demonstrated the bug the scoping fixes: given
idstack@idstack
Status: disabled
superpowers@marketplace
Status: enabled
the old 'grep -A4' window reads the neighbour's status line and reports a
disabled idstack as 'installed and enabled' — the doctor gives a clean bill
of health for an install that cannot work. The scoped window ends the entry
at a blank line or the next name@marketplace id and returns disabled.
Also pins: doctor sources the shared parser, and no 'grep -A4' remains.
smoke-test 365 -> 370 assertions; new suite wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…3.0.0 The doctor's fixed-window enabled-check was a user-facing bug (a broken install diagnosed as healthy) and was missing from the entry. Also corrects the suite count (six) and assertion count (370) after test-plugin-status.sh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reintroduces all 10 defects fixed in this branch into a throwaway copy and asserts the guarding test fails: preamble f-string, course-import suggestion, non-canonical section names, bare /skill refs, pipeline namespace, hand-rolled _IDSTACK, plugin-status fixed window, version disagreement, stale generated file, missing Top recommendations. All 10 are caught. This is the check that was missing before: the version-classifier suite passed green while testing a copy of the classifier rather than the shipped code, and gen-skills counted a placeholder-less template as neither generated nor failed. A guard nobody has ever seen fail is not a guard. Kept out of smoke-test (copies the repo per mutation); runs as its own CI job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
16 behavioral tests against a repo copy with a fake $HOME and a stub `claude` on PATH, so neither the real install nor the working tree is touched: flag parsing (unknown flag exits 2, --help documents --keep-legacy), scope selection (user vs project), all three legacy-cleanup shapes with and without --keep-legacy, the unrecognized-directory guard, --local not touching $HOME, loud failure when `claude` exits nonzero, and the post-setup regeneration invariant. This was the audit's coverage inversion: 14 smoke assertions on landing-page CSS, zero on the script every new user runs first — which is how four defects lived in it. Three matching mutations added (per-skill loop ignoring --keep-legacy, --local leaking into $HOME, silent `claude` failure); all 13 mutations now caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seven suites in CI (setup + plugin-status added since the entry was written), 371 smoke assertions, and the mutation suite documented as the check that proves the guards actually guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Eight confirmed findings from the review pass, plus the CI failure the new mutation job caught. Correctness: - bin/idstack-migrate --init creates a canonical manifest (every section at its default) when none exists. Three skills instructed the user to run idstack-migrate to create one in standalone mode; it was a no-op on a missing file, so the merge that followed exited 4 and standalone results were silently never persisted. The skeleton is produced by running the existing migration chain over a minimal seed, so 'canonical' has one definition. accessibility-review, assessment-design, and red-team now call it; accessibility-review and assessment-design also regained the standalone persistence note that the section-slimming dropped. - $_IDSTACK marketplace-cache resolution sorted lexically while its comment claimed 'highest version' — it ranks 3.9.0.0 above 3.10.0.0 and would pick a stale install once the minor hits double digits. Now sorts basenames by numeric version field. Preamble's three copies re-synced. - templates/manifest-schema.md is spliced verbatim into 20 skills (getline bypasses placeholder expansion), so its merge example called $_IDSTACK with nothing resolving it. The resolution is now written out inline, with a note explaining why the placeholder is not used there. - bin/idstack-status printed bare /skill commands (checkboxes, suggested next, readiness verdict) — the exact form this branch bans in skill text because it is not a valid command. All namespaced; integration-test asserts no bare form can return. Test integrity — three ways the suite was weaker than it looked: - The mutation job failed in CI and was right to: pinned to Python 3.12, where PEP 701 makes the nested-quote f-string legal, so reintroducing that bug is a no-op and reported NOT-GUARDED. Job moved to 3.9 (where the bug exists) and the case now skips explicitly with a reason on 3.12+. - Five mutations edited a .tmpl without regenerating, so they were caught by the staleness gate rather than the assertion under test — GUARDED proved nothing about that assertion. All template mutations now regenerate first; still 13/13 caught, now for the right reason. - test-setup's 'leaves generated files fresh' check asserted the fixture was fresh, not that setup regenerated: it passed with setup's regeneration call deleted. It now dirties a generated file first and asserts staleness before the run. Also: manifest-merge suite covers --init (creation, schema version, every mergeable section present, merge-after-init, non-clobbering, and that plain migrate still no-ops); CHANGELOG corrected — learning-objectives moved to the merge path and course-import, not learning-objectives, is the second Write-tool-fallback skill. Co-Authored-By: Claude Fable 5 <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.
A full audit of the toolchain (generation pipeline, install/test infrastructure, skill content contracts) found the architecture sound but surfaced a set of bugs that broke user-visible behavior — several since the features shipped — plus the test-infrastructure gaps that let them ship.
The bugs
templates/preamble.mdf-string with nested same-type quotes2>/dev/null, so welcome-back messages, quality trends, and next-skill suggestions never worked for most users.pipelineinvoked children asskill: "needs-analysis"idstack:needs-analysis. The orchestrator's execution loop could not resolve a single child skill in Claude Code.learn+course-export$_IDSTACKpath list omitted the marketplace cachebin/calls pointed at a nonexistent directory.bin/idstack-statusinterpolated$PROJECT_NAMEinto Python sourcebin/idstack-doctorused a fixedgrep -A4windowenabledwas read as idstack's — a disabled install diagnosed as healthy.setup--keep-legacyhonored in 1 of 3 deletion paths;--localdeleted a symlink under$HOME; a nonzeroclaudeexit aborted setup silently before the Codex step.Why they shipped, and what changed
The suite had never run automatically — no CI at all — and parts of it were not testing what they appeared to test.
test-version-classifier.shtested a local copy of the classifier, so it stayed green while the two real call sites could drift.gen-skillscounted a placeholder-less template as neither generated nor failed, so--dry-runpassed over absent output.test.yml): all seven suites on push and PR, ubuntu + macos, Python 3.9 and 3.12. The 3.9 leg is what catches the context-recovery class of bug.release.ymlnow refuses to publish unless tag ==VERSION==plugin.json==CHANGELOGand the smoke test passes../setupis tested — 16 behavioral tests against a repo copy with a fake$HOMEand a stubclaude. It had zero coverage while the smoke test spent 14 assertions on landing-page CSS.test/mutation-test.sh) reintroduces each of the 13 defects into a throwaway copy and asserts the guarding test fails. All 13 are caught. A guard nobody has seen fail is not a guard.bin/lib/—version-classify.shandplugin-status.sh— sourced by their callers, so the unit tests exercise shipped code.{{IDSTACK_RESOLVE}}generator placeholder splices one canonical$_IDSTACKresolution into every bash block that needs it (blocks run in separate shells, so each must be self-contained), and smoke-test bans hand-rolled derivations.smoke-test: 272 → 371 assertions, and it now prints diagnostics instead of a bare
FAIL.Verification
bash -nclean on every script;gen-skills --dry-runreports 0 stale across all 22 generated files.--payloadforms.allowed-toolsblock, per skill.SKILLS_COMPLETEDandSUGGESTED_NEXT.Note on the release
plugin.jsonis bumped to 3.3.0.0 — marketplace installs cache a versioned snapshot, so without that bump none of these fixes reach users. Existing users update withgit pull && ./setup.🤖 Generated with Claude Code