fix(graphify): run /gsd-graphify build inline (with regression fence) (#3169)
* fix(graphify): run /gsd-graphify build inline instead of spawning a sub-agent Closes #3166 graphify v0.7+ split the build into a fast AST-extraction phase (cached) followed by a separate clustering + report-write phase. The cached extraction phase survived sub-agent isolation, but the post-extraction phase was SIGTERM'd when the agent exited, leaving the cache populated and no graph.json / graph.html / GRAPH_REPORT.md artifacts written to .planning/graphs/. The skill now runs `graphify update .`, the three artifact copies, the snapshot, and the status report as a single foreground Bash call so the entire pipeline survives to completion. The CLI's `graphify build` pre-flight still returns `action: "spawn_agent"` so external callers and existing tests in tests/graphify.test.cjs keep working. Regression test (tests/bug-3166-graphify-inline-build.test.cjs) parses the skill's YAML frontmatter and body structurally to fence against re-introducing Task to allowed-tools or `Task(` invocation syntax — a future edit cannot regress the fix without tripping the fence. Verified against safishamsi/graphify v0.7.0–v0.7.8 release notes: `graphify update .` invocation and output filenames are unchanged in v0.7+; no GSD-side interface migration is required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): drop yaml dep from bug-3166 fence — replace with inline parser CI failed with MODULE_NOT_FOUND on `require('yaml')` — the package resolved locally as a transitive dep but isn't declared in package.json. The project pattern (see tests/helpers.cjs `parseFrontmatter`) deliberately avoids pulling in yaml/js-yaml. Replace with a narrow inline parser that handles the scalar + block-list subset used in this skill's frontmatter. Verified the fence still trips when Task is reintroduced to allowed-tools. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): parse fenced blocks structurally for #3166 fence Address CodeRabbit nitpicks on PR #3169: the body assertions used raw markdown text regex (\bTask\s*\(/, /graphify\s+update\s+\./) which violates the project's "parse, never grep" testing convention and risks false-positives on prose. Replace with extractFencedBlocks(body) which returns [{lang, content}, ...] tuples per markdown code fence. Body assertions now run against parsed blocks: - "no fenced code block contains Task(" → deepEqual offending blocks to [] (vs. regex on raw body) - "a bash block invokes graphify update . / build snapshot" → filter to lang === 'bash', then substring-check inside parsed content Substring checks within already-parsed fenced content are structural — prose mentioning the word "Task" can no longer false-positive, and a future prose reference to graphify cannot satisfy the positive assertions either. The frontmatter side already used a parser; both sides now match. Verified: re-introducing Task( inside a code fence still trips the assertion. Full suite 7499/7499 passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): rename readFileSync-bound var to satisfy lint-no-source-grep The structural-parse refactor introduced `b.content.includes(...)` calls on parsed fenced-block records, but `loadSkill()` had also bound `const content = fs.readFileSync(...)` for the markdown text. The lint-no-source-grep regex scanner cannot distinguish scopes — it sees "variable `content` is bound from readFileSync" and "`content.includes` is called" and flags it as a source-grep test, even though the two `content`s are different lexical entities. Rename the readFileSync-bound local to `markdown`. Now `b.content` is unambiguously a property access on a parsed-block record. Lint passes (0 violations across 401 test files); behavior unchanged (4/4 tests still pass, including the negative regression case). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): tighten snapshot assertion to gsd-tools.cjs prefix CodeRabbit nitpick on bug-3166 fence: the snapshot bash assertion accepted any 'graphify build snapshot' substring. Tighten to require it follows 'gsd-tools.cjs', matching the actual fenced invocation in commands/gsd/graphify.md (which uses node "$HOME/.../gsd-tools.cjs" graphify build snapshot — note the closing quote, so a literal 'gsd-tools graphify build snapshot' substring would not match). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -105,6 +105,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
|
||||
|
||||
### Fix
|
||||
|
||||
- **`/gsd-graphify build` now runs inline instead of spawning a sub-agent** — graphify v0.7+ split the build into a fast AST-extraction phase (cached) followed by a separate clustering + report-write phase. The cached extraction phase survived sub-agent isolation, but the post-extraction phase was SIGTERM'd when the agent exited, leaving the cache populated and no `graph.json` / `graph.html` / `GRAPH_REPORT.md` artifacts written to `.planning/graphs/`. The skill now runs `graphify update .`, the three artifact copies, the snapshot, and the status report as a single foreground Bash call so the entire pipeline survives to completion. The CLI's `graphify build` pre-flight still returns `action: "spawn_agent"` so external callers and existing tests keep working. Regression covered by `tests/bug-3166-graphify-inline-build.test.cjs` (4 structural assertions that parse `commands/gsd/graphify.md` YAML frontmatter and body to fence against re-introducing `Task` to `allowed-tools` or `Task(` invocation syntax). (#3166)
|
||||
- **`gsd-pristine/` is now populated by the installer when local patches are detected** — `saveLocalPatches` declared a `pristineDir` variable and JSDoc'd "saves pristine copies (from manifest) to gsd-pristine/ to enable three-way merge during reapply-patches", but no code ever wrote to that directory. Effect: the `/gsd-reapply-patches` Step 5 verifier (#2972) silently degraded to its over-broad fallback heuristic ("every significant backup line"), exactly the silent-success-on-lost-content failure mode #2969 was designed to prevent. Fix: new `populatePristineDir({ packageSrc, pristineDir, modified, runtime, pathPrefix, isGlobal })` helper runs the install transform pipeline (`copyWithPathReplacement`) into a tmp staging dir, then copies out only the modified-file paths into `gsd-pristine/`. `saveLocalPatches` now accepts a `pristineCtx` and calls the helper when local patches are detected; the install entry point passes the package source root, runtime, pathPrefix, and isGlobal so transforms produce byte-identical output to what `copyWithPathReplacement` would have written under normal install. Soft-fails on transform errors (logs a warning, continues with empty pristine — no worse than pre-fix behavior). Pristine reflects the about-to-install version's content, which is what the verifier needs as the "what would survive without the user's modifications" baseline. Regression covered by `tests/bug-2998-pristine-dir-populated.test.cjs` (6 tests across two suites): asserts the helper is exported, returns 0 for empty modified list, writes one pristine file per source-existing path, skips ghost paths without corrupting pristine, and produces deterministic output (two runs with same inputs yield byte-identical pristine — the property `pristine_hashes` in `backup-meta.json` depends on). (#2998)
|
||||
- **`release-sdk` hotfix re-run no longer fails at `Dry-run publish validation` when the version is already on npm** — the `Detect prior publish (reconciliation mode)` step sets `skip_publish=true` when the package version is already on the registry, and the actual publish step honors that gate. The `Dry-run publish validation` step was missing the same guard, so any operator re-run of an already-published hotfix (the typical recovery path when later steps fail mid-flight) hit `npm publish --dry-run` first and got `npm error You cannot publish over the previously published versions: X.Y.Z` — `npm publish --dry-run` contacts the registry and rejects existing-version targets even though it doesn't actually publish. The dry-run validation step is now gated on the same `steps.prior_publish.outputs.skip_publish != 'true'` condition as the publish step. The rehearsal still runs on first publishes (where it has value); it skips only in the specific reconciliation case where the publish itself would be skipped. Trigger run: [25233855236](https://github.com/gsd-build/get-shit-done/actions/runs/25233855236/job/73995605643). Regression covered by `tests/bug-2987-dry-run-validation-skip-on-reconciliation.test.cjs`. (#2987)
|
||||
- **`release-sdk` hotfix flow hardened against silent classifier failures, missing-classifier-at-base-tag, and a vestigial merge-back PR step** — three issues surfaced by CodeRabbit's post-merge review of #2981 plus a production failure on the v1.39.1 release run. **(1)** `scripts/diff-touches-shipped-paths.cjs` reused exit code `1` for both the legitimate "no shipped paths" classifier result and Node's default uncaught-throw exit, so any tooling failure was indistinguishable from a normal skip. The script now uses `0` (shipped), `1` (not shipped), `2` (classifier error) with `try`/`catch` + `uncaughtException`/`unhandledRejection` handlers routing all failure paths to exit `2`. **(2)** The workflow's `git checkout -b "$BRANCH" "$BASE_TAG"` overwrote the working tree with the base tag's contents *before* the cherry-pick loop ran the classifier — but base tags predating the classifier's introduction (notably v1.39.0) don't have the file in their tree, so `node scripts/diff-touches-shipped-paths.cjs` would exit non-zero and silently drop every commit, producing an empty hotfix release. The classifier is now staged into `$RUNNER_TEMP` at the top of `Prepare hotfix branch` (before any working-tree-mutating git command), and the loop references that staged copy. The cherry-pick loop snapshots `$PIPESTATUS` into a local array (`PIPE_RC=("${PIPESTATUS[@]}")`) immediately after the classifier pipeline — under bracketed `set +e`/`set -e` — and dispatches via explicit `case`: `0` proceeds, `1` skips into `NON_SHIPPED_SKIPPED`, anything else emits `::error::shipped-paths classifier failed for $SHA (exit N)` and fails the workflow. CodeRabbit on PR #2984 caught a subtler bug in the first iteration: `pipeline \|\| true; RC=${PIPESTATUS[1]}` is broken because `\|\| true` runs `true` as its own one-command pipeline on the failure paths, overwriting `PIPESTATUS` to `(0)` and leaving `${PIPESTATUS[1]}` unset. The array-snapshot form is invariant against this. The same hardening also surfaces `git diff-tree`'s exit code (via `PIPE_RC[0]`); a non-zero diff-tree result now also fails the workflow rather than feeding partial input to the classifier. **(3)** Removed the `Open merge-back PR (hotfix only)` step. The auto-cherry-pick hotfix flow only picks commits already on main (`git cherry HEAD origin/main` outputs the unmerged ones), so by construction every code commit on the hotfix branch is already on main. The only hotfix-branch-only commit is the version-bump chore, which would either no-op against main or rewind main's in-progress version. The step also failed in production with `GitHub Actions is not permitted to create or approve pull requests (createPullRequest)` (org policy) on run [25232968975](https://github.com/gsd-build/get-shit-done/actions/runs/25232968975). The `pull-requests: write` permission previously granted to the release job has been dropped in line with least-privilege. The run-summary line that previously echoed `Merge-back PR opened against main` has been replaced with `No merge-back PR (auto-picked commits are already on main)` so operators reading the summary see an accurate non-action statement (CodeRabbit on PR #2984). Regression covered by `tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs` (15 assertions across exit-code semantics, classifier staging, error dispatch, PIPESTATUS-snapshot hardening, diff-tree fail-fast, merge-back removal, and run-summary accuracy). (#2983)
|
||||
|
||||
Reference in New Issue
Block a user