b901d1e06f
* test(#1953): failing-first suite for the complexity-triggered refactor hook 60 behavioral cases against src/complexity-trigger.cts, which does not exist yet: decision-point counting, the comment/literal stripping leak surface, threshold and jump-delta boundaries at limit-1/limit/limit+1, stable-anchor baseline semantics, and fs fault injection via mock.method. Two fast-check properties assert that stripping never manufactures a decision point and that comments and string literals are score-neutral. Also registers the refactor-trigger capability manifest (inert until refactor.trigger_enabled) and regenerates the capability registry and matrix. Verified RED on the remote runner before any implementation exists. * feat(#1953): complexity-triggered refactor extension point Adds the opt-in refactor-trigger capability. After a phase executes, an execute:post step measures per-function complexity for the files the phase touched and writes a scoped refactor proposal when a function crosses the configured threshold or drifts past its recorded anchor. Design notes worth carrying: - The signal is computed in-core (decision-point counting over comment- and literal-stripped source, Node builtins only) rather than via Memtrace or a shelled-out analyzer. The hook fires as a deterministic CLI, not an agent with MCP tools, and core takes no external dependencies — this is the only option a behavioral test can bind to. The metric sits behind a seam. - The baseline is a stable anchor, not a rolling value: set on first observation, moved only on disposition. A rolling baseline makes the delta the single-phase change, so a function creeping +2 per phase never trips a delta of 5 and the jump check adds nothing over the absolute threshold. - Strict mode records an open deviation window in the broken-windows ledger rather than declaring its own ship:pre gate. ship.md has no generic ship:pre gate dispatch — only two hardcoded branches — so a third gate of any kind would be declared and never evaluated. - The gate clears on the proposal being dispositioned, never on the score improving. A blocking complexity number is one an executor can satisfy by splitting a coherent function in two. execute-phase.md gains a generic execute:post step-dispatch contract; it previously matched only ref.skill == "code-review", so any other step registered there was declared and never run. The code-review branch is unchanged. Full rationale in ADR-1953. Closes #1953 * fix(#1953): close git option injection and symlink escape in the refactor hook Three findings from the isolated security review, all fixed inline. HIGH — changedFilesSince interpolated the --since value into a revision token placed before the -- separator. A -- only stops PATHSPEC parsing of arguments after it; git still option-parses what comes before. So --since '--output=/tmp/x' became --output=/tmp/x..HEAD, which git accepts as --output=<file> and uses to redirect diff output — an arbitrary write. Fixed with --end-of-options before the revision range plus a conservative ref validator. The validator deliberately permits ~ ^ @ { } because those are legitimate git REVISION syntax (HEAD~1, main@{yesterday}) as distinct from ref-NAME syntax; --end-of-options is the actual barrier. The doc comment asserting the trailing -- was sufficient was wrong and is corrected. MEDIUM — resolveConfinedPath confined by string prefix only, so a symlink committed inside the repo passed the check (its own path is under cwd) and readFileSync then followed it outside the root. Now lstat-checks for a regular file and skips anything else with REFACTOR_FILE_UNREADABLE, so one bad path skips one file and the run continues. LOW — the new execute:post dispatch contract showed the gsd_run example before the rule requiring ref.command be validated first. That prose is executed by an agent, so textual order is execution order. Reordered. Refs #1953 * fix(#1953): make the analyzer able to see TypeScript at all Found by running the shipped analyzer over its own source: it reported functions=1 for a 940-line module with 24 function forms. A return-type annotation or a generic parameter list made a function invisible — `function f(a): number {}` and `function f<T>(a: T): T {}` both detected as zero. Since gsd-core is written in .cts and the capability declares .ts/.cts/.mts analyzable, the feature silently found nothing in this repo's own primary language while reporting success. A safety net that reports "all clear" because it cannot see is worse than no safety net. All 98 tests passed over this, because every fixture was plain JS — the exact failure the test matrix's own "assert against the shape production uses" warning describes. Adds a TypeScript-shapes suite covering return types (including unions, generics, object literals and type predicates), generic parameter lists (constrained and defaulted), export/async/generator combinations, annotated arrows, class-method modifiers, and optional/ default/rest params — plus the two traps: an overload signature has no body and must not count, and `a < b && c > d` is a comparison, not a generic. Detection now reports 24/37/21 functions for the three source files, which matches a hand count exactly. Also from review: - The strict-mode ledger dedup identified entries by parsing a prose description string. That is banned by CONTRIBUTING's raw-text-matching rule and was a real bug: the "exactly one window per untriaged proposal" guarantee rested on prose matching, so rewording a description or editing WINDOWS.md by hand silently produced duplicates. Now matches structurally on kind + phase + file + line. - A property test asserted on the stripper's output text. Reframed to assert the same invariant through analyzeSource's score. - nextBaseline's `candidates` parameter has been dead since the anchor change; removed from the signature and all call sites. - Extracted the duplicated require-or-degrade and capability-check boilerplate. - ADR-1953's Implementation bullet still named a `refactor.ship-gate` in check-command-router.cts — a leftover from the design cut D6 rejects. That file is untouched and no such gate exists. Removed. Refs #1953 * fix(#1953): keep execute-phase.md under its byte ceiling; un-vacuum the large-file test Five of the seven remote-runner failures were one cause: the execute:post dispatch contract, written out inline, grew execute-phase.md 1876 bytes (93,400 -> 95,276) against a frozen PRE_PHASE6 ceiling of 93,600. A drift-ack does not clear that — tests/phase6-capstone-conformance.test.cjs and tests/fix-2285-claude-orchestration-wiring.test.cjs assert the file is literally under the cap. The contract now lives in gsd-core/references/loop-hook-dispatch.md, which already claimed to be the point-agnostic dispatch reference and already documented ref.skill and ref.agent. It gains the ref.command shape, its in-context validation rule, the advisory-by-construction statement, and a note that a point whose workflow hand-rolls one kind is not implementing this contract. execute-phase.md now defers to it in one line: 145 bytes of growth, 55 B of headroom under the cap. Better placement than the first cut — the reference was overstating its coverage, and this makes the claim true rather than duplicating prose next to it. Acknowledged by appending to tests/emitted-drift-acks/2930-*.json rather than a new 1953-*.json: two ack sources may never name the same path, and that fragment is already the accumulating ack for this file. Sixth and seventh failures: analyzesLargeFileWithinBounds tripped its own vacuity guard — the fixture generated ~480 KB against a `> 500000` assert, so the guard fired and the three assertions after it never ran. The test has been vacuous since it was written. The matrix row specifies ~1 MB, so N goes 8000 -> 20000 (1.17 MB, 17% margin) and the guard to > 1_000_000. Verified by reproducing the exact body against the compiled module: 1168888 bytes, 118 ms, all four assertions hold. Refs #1953 * fix(#1953): fold the execute:post step deferral into the existing resolve line The remaining two failures were one test: execute-phase.md carries a SECOND, tighter assertion than the 93,600 ceiling — `<=93400`, which is exactly its current size. The file cannot grow by a single byte. My previous fix got it under 93,600 but not under 93,400, so it still failed. ("H." in the report is just the parent describe of that same test, not a separate defect.) Rather than add a paragraph, the deferral now REPLACES the existing hook resolution line. It read: Resolve active step hooks from `EXECUTE_POST_HOOKS_JSON` where `kind == "step"` and `ref.skill == "code-review"`. which is the bug itself written down — only code-review was ever dispatched. It now reads: Dispatch each `kind == "step"` hook per @gsd-core/references/loop-hook-dispatch.md. For `code-review`: The following prose already begins "If no active code-review step hook exists", so it reads correctly and the code-review handling is untouched. Net effect on the file is -11 bytes: 93,400 -> 93,389, under the margin assertion rather than merely under the ceiling. That also removes the need for a drift-ack: the file shrank, so there is no growth to acknowledge, and the append to the shared 2930-*.json fragment is reverted. Leaving it would have shipped a claim of "145 bytes of growth" that is no longer true, on a file six other issues share. The test's own comment states the principle this ended up honoring: "the host loop must stay small — optional-feature detail belongs in the capability fragment, not the host workflow." Putting the dispatch contract in the reference rather than inline is that rule, applied. Refs #1953 * fix(#1953): keep the code-review hook literal the workflow test requires tests/code-review.test.cjs extracts the <step name="code_review_gate"> block and asserts it contains `ref.skill == "code-review"` verbatim. The previous commit replaced the line carrying that literal, so the token vanished and the test went red — a fair assertion: code-review IS the bespoke branch there and the workflow should still name it. Restored inside the same one-line deferral, which now reads: Dispatch `kind == "step"` hooks per @gsd-core/references/loop-hook-dispatch.md. `ref.skill == "code-review"`: 93,396 bytes — still under the `<=93400` margin assertion and 4 bytes below the base, so the file continues to shrink rather than grow. Because three consecutive runs were each reddened by a different assertion on this one file, this change was verified by sweeping ALL of them at once rather than one run at a time: every test under tests/ that reads execute-phase.md or references/loop-hook-dispatch.md was located by resolving its path constants, and each content/size assertion was evaluated directly against the working tree — 22 assertions, plus two real executions (gen-section-manifest --check, and emitted-attribution's full real-tree differential). All pass. That sweep also confirms the earlier judgement call: the net change to execute-phase.md is a SHRINK, and the size ratchet only gates growth, so reverting the append to the shared 2930-*.json ack fragment was correct — an ack would have been both unnecessary and factually wrong. Refs #1953 * chore(#1953): backfill changeset pr number to 3261 * docs(#1953): add the missing how-to for acting on a refactor proposal Reference and explanation shipped (COMMANDS.md, CONFIGURATION.md, FEATURES.md 159, ADR-1953) but the Diataxis how-to quadrant did not, and that is the one a user reaches for. CONTRIBUTING's required-docs table is 'new command -> COMMANDS.md + FEATURES.md', so CI was green on a gap. Enabling this feature is genuinely multi-step and no single page walked it: turn it on, tune the threshold, understand advisory vs strict, discover that strict needs a SECOND toggle on a DIFFERENT capability, and know what to do when a proposal appears. The two-toggle subtlety in particular was a footnote in a config table; here it is a section with both commands. Follows the shape of its closest siblings, resolve-edge-coverage-findings and resolve-prohibition-findings — both 'the loop surfaced a finding, here is what to do with it'. Includes a reason-code table for the silent cases, since the analyzer is deliberately quiet in six situations and a user who expected a proposal needs to tell 'nothing to report' from 'could not look'. Indexed from docs/README.md beside the other loop how-tos. Docs-only: exempt from the push gate, no re-verification, pass marker on 2af188b4 untouched. Refs #1953 * feat(#1953): warn when strict mode is on but nothing will actually block Closes acceptance criterion 5, which I had wrongly marked satisfied. refactor.trigger_strict records an untriaged proposal as an open deviation window, but a ship only STOPS if workflow.windows_enforce is also on — a toggle owned by the broken-windows capability that this feature neither sets nor requires. So a user could enable strict, believe ship was gated, and find out otherwise at ship time. The split itself stays: requires:["broken-windows"] would force-install the ledger on advisory users who never enable strict, and a ship:pre gate of our own would never fire because ship.md has no generic ship:pre gate dispatch. What was missing was discoverability, so that is what this fixes. `refactor evaluate` now emits a typed REFACTOR_STRICT_NOT_ENFORCING warning, naming the exact remediation command, whenever strict is on and either workflow.windows_enforce is off or broken-windows is unavailable. It fires only on a run that produced a candidate — with nothing to block on there is nothing to warn about, and warning every run would be noise. Reads workflow.windows_enforce through the same resolveConfigKey walk the router already uses for its own keys rather than a second config reader. Four tests cover the matrix: strict+enforce-off warns, strict+enforce-on does not, strict+ledger-absent warns, strict-off never warns. Also corrects a user-facing message in this same file that told the user to run `gsd-tools config-set` — the wrong form. docs/CONFIGURATION.md and the broken-windows capability both use `gsd config-set`, and gsd-tools is invoked as `node gsd-tools.cjs`, so the bare form may not resolve. The two adjacent messages in this file now agree. Refs #1953 --------- Co-authored-by: sim <sim@local>
7.4 KiB
How to catch complexity before it compounds
Goal: Turn on the complexity-triggered refactor hook, understand the proposals it surfaces after a phase, and dispose of each one — so a function that grew a branch per phase gets refactored while it is still one function, instead of becoming a rewrite nobody noticed accumulating.
Prerequisites: A GSD project on v1.10.0 or later. Nothing else — the hook has no external dependency and needs no indexer. It is off by default; a project that never enables it is completely unaffected.
For what the metric measures, why the anchor moves only on disposition, and the metric's known biases, see Complexity-Triggered Refactor and ADR-1953. This guide covers only how to use it.
Turn it on
gsd config-set refactor.trigger_enabled true
That is the whole setup. From the next /gsd-execute-phase, an execute:post step measures the files that phase touched and writes a proposal only if something crosses a line. Nothing else about the loop changes: the hook never edits code, never picks a refactor, and never blocks.
To check it without running a phase:
node gsd-tools.cjs refactor evaluate --phase 3 --raw
Read a proposal
A triggered proposal lands at .planning/phases/<N>/<NN>-REFACTOR.md. It names one target — the hotspot — and lists every other candidate it found.
Two numbers matter, and they answer different questions:
score— the function's absolute complexity right now. It triggered because it exceedsrefactor.complexity_threshold(default 15). Answers "is this function too complex?"delta— growth over the function's anchor, the score recorded the last time you consciously decided about it. It triggered because it exceedsrefactor.complexity_jump_delta(default 5). Answers "has this been quietly creeping?"
A proposal can carry both reasons. The delta is the one worth reading closely: because the anchor does not move on its own, a function gaining two points per phase accumulates against it and trips the delta a phase or more before the absolute threshold would have caught it. That gap is the entire point of the second number.
Both thresholds are strictly greater — a score of exactly 15 against a threshold of 15 does not trigger. This matches ESLint's complexity: {max: N}.
Act on it
A proposal stays untriaged until you disposition it. There are exactly two ways, and both clear it:
node gsd-tools.cjs refactor accept --phase 3
node gsd-tools.cjs refactor decline --phase 3 --reason "flat dispatch table — branchy by construction, not a hotspot"
Accept when you intend to refactor. The anchor re-anchors to the function's current score, so after you do the work the next evaluation measures growth from the new, lower baseline.
Decline when the complexity is justified. The reason is required and recorded. The anchor re-anchors to the current score — you have consciously accepted this much complexity, so the delta clock restarts from here rather than nagging you every phase about growth you already signed off.
The score improving does not clear a proposal — only a disposition does. This is deliberate. If clearing required the number to go down, the cheapest way to satisfy it would be splitting one coherent function into two incoherent ones: identical total complexity, worse cohesion. The gate asks whether you decided, not whether the metric moved.
To see what is outstanding across the project:
node gsd-tools.cjs refactor status
Tune the thresholds
If proposals feel like noise, raise the threshold rather than turning the hook off:
gsd config-set refactor.complexity_threshold 20 # ESLint's own default
gsd config-set refactor.complexity_jump_delta 8
Defaults are 15 (SonarSource's default) and 5. For reference, radon's rank C — "moderate, slightly complex" — begins at 11, and ESLint's complexity rule defaults to 20. There is no universally correct number; start at the default and raise it once you have seen a few proposals you disagreed with.
Make it block before ship
Advisory mode surfaces proposals and tracks nothing. To make an untriaged proposal a task that must be resolved before shipping, you need two settings, not one:
gsd config-set refactor.trigger_strict true # 1. record proposals in the ledger
gsd config-set workflow.windows_enforce true # 2. make the ledger block /gsd-ship
Why two: refactor.trigger_strict records an untriaged proposal as an open deviation entry in the broken-windows ledger. The blocking is that capability's existing ship:pre gate, which is separately opt-in. Setting only the first gives you tracking without enforcement — which is a reasonable place to stop, but it will not stop a ship.
If you enable only refactor.trigger_strict, every refactor evaluate that triggers reports a typed warning saying so, so you never learn about the enforcement gap by hitting it at ship time:
"warnings": [
{
"reason": "refactor_strict_not_enforcing",
"message": "refactor.trigger_strict is on, but workflow.windows_enforce is off, so ship will not actually be blocked. Run: gsd config-set workflow.windows_enforce true"
}
]
If the broken-windows capability is not installed, strict mode still records the proposal locally and says so in its output (ledger_recorded: false with a note) — and the same refactor_strict_not_enforcing warning fires, with a message telling you to install the broken-windows capability first. It cannot block on its own.
Dispositioning resolves the ledger entry automatically: accept marks it fixed, decline marks it waived with your reason attached.
When it stays quiet
The hook is deliberately silent in several situations. If you expected a proposal and got none, check these before assuming it is broken — --raw reports the reason in every case:
| You see | What happened |
|---|---|
refactor_no_touched_files |
The phase changed nothing the analyzer looks at. |
refactor_analyzer_unsupported |
The file's language or path is out of scope. Only .js .cjs .mjs .ts .cts .mts are analyzed; tests/ and generated gsd-core/bin/lib/ paths are excluded by design. |
refactor_analyzer_unparseable |
An unterminated string, template, or block comment. The analyzer refuses to emit a score it cannot defend rather than guessing — a silently wrong number is worse than none. |
refactor_git_unavailable |
Not a git repository, git missing, the call timed out, or the phase has no committed PLAN.md to anchor against. Degrades quietly, exit 0. |
refactor_file_unreadable |
One file could not be read, or resolved outside the project root. That file is skipped; the rest of the run continues. |
Nothing at all, below_threshold |
Everything the phase touched is under both lines. This is the normal, healthy case. |
Two limits worth knowing up front. A renamed function loses its anchor — it reads as a delete plus an add, so it is evaluated against the absolute threshold only until it is dispositioned again. And the metric is biased against a flat switch: twelve readable cases score 13, which is why proposals are advisory and why decline takes a reason instead of demanding a code change.