next
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
bbdf7e8e84 |
chore(#4654): add local/no-unconfined-path-join and drain it to zero — Phase 4 of #4636 (#4674)
* chore(#4654): add local/no-unconfined-path-join and drain it to zero Phase 4 of epic #4636 — the ratchet, and the phase that makes the epic hold. THE MEASUREMENT THAT RESHAPED THE PHASE. An AST census (the repo's own parser, not grep) found what the epic never enumerated: ADR-4650 named seven containment implementations; `src/` alone held roughly 24 more hand-rolled gates across ~13 files, several guarding a write or an `fs.rmSync`. Two verified by reading rather than pattern-matching — `research-store.cts` comments its own as "ensure the resolved file path stays inside the store dir" immediately before a write, and `capability-lifecycle.cts` gates `fs.rmSync` with one. So the epic's Done-when "one containment predicate, used at every site" was FALSE when Phase 3 reported it satisfied. It is true now: the rule is clean across src/, scripts/, gsd-core/bin/ and hooks/ with an EMPTY allowlist. WHY NOT THE RULE THE ISSUE PROPOSED. #4654 proposed flagging `path.join` whose first argument is a managed root and whose later arguments derive from argv. That is a taint analysis over 2046 call sites, in ESLint, without type information; "derives from argv" is not locally decidable. Any approximation either floods or is trivially evaded, and a rule that fires on hundreds of correct sites earns an allowlist of hundreds — the opposite of a ratchet. What is actually duplicated is the COMPARISON, not the join, and that has one recognizable shape. Arm 1 X.startsWith(Y + sep) the hand-rolled containment idiom Arm 2 a containment predicate called as a bare statement, answer discarded Arm 2 is the issue's "asserts the result was narrowed, not merely that a helper was called". Its example `validatePath(x, root).resolved` is already structurally impossible — Phase 3 un-exported `validatePath` — so the remaining expressible failure is ignoring the answer, which is the defect that recurred five times in this epic. The census found exactly one live instance (`milestone.cts:1643`); it now returns the proven `ContainedPath` so consumers stop re-deriving the path the comment above it was extracted to stop them re-deriving. The rule deliberately does NOT try to catch validate-one-path-use-another where the answer is used but a different variable flows onward. That needs flow analysis; the branded `ContainedPath` from Phase 3 is the defense there, and the two are complementary. PER-SITE FAMILY CHOICE, NOT A DEFAULT. Phase 3's lesson binds: collapsing a lexical site onto the realpath family broke four tests and was caught only by the matrix. Every migrated site was triaged individually. The six installer-migrations tree-walks and the six capability-lifecycle gates take the LEXICAL family because their operands are already realpath-resolved and they deliberately treat the final component as a link; boundary sites take realpath. TWO SITES WITH AN INVERTED CONTRACT, which a mechanical swap would have broken. `installer-migrations.cts:127` and `runtime-artifact-install-plan.cts:144` REJECT `target === root` by contract, while the canonical comparison ACCEPTS it. Swapped naively, a migration could `rmdir` the user's config root and a third-party descriptor could write at configHome itself. Both keep `=== root` as an explicit additional arm alongside the predicate call — the predicate decides containment, the call site keeps its own extra condition (ADR-4650 decision 6). ONE DUPLICATE DELETED OUTRIGHT: `planning-inspect.cts`'s `isWithinRoot` was byte-identical to `isContainedIn` and said so in its own docstring. `isContainedIn` is now exported for callers that have already resolved both operands and need only the comparison, with a doc note that a caller which has NOT resolved them must use a full predicate instead. THE MARKER, AND WHY IT IS NOT THE ALLOWLIST. Nine sites are justified holdouts and carry `// allow-handrolled-containment: <reason>` with a mandatory, reviewable reason. Two justifications: (a) not a containment decision — an ancestor-walk loop condition, sub-repo grouping, worktree identity matching, declared-path coverage; (b) it IS containment but the canonical predicate is unreachable — `capability-validator.cjs` is a committed pre-build `.cjs` and the compiled `security.cjs` is untracked build output, so requiring it would break a fresh clone. `scripts/lib/drift-scan.cjs` runs under `lint:ci` with the same exposure. The marker was renamed from `allow-lexical-prefix-match` mid-phase because that name asserted only (a) and would have stated something false at the (b) sites. A marker suppresses BEFORE the violation counter increments, so a file whose every occurrence is marked still reports `staleAllowlistEntry` — otherwise a drained entry lingers and silently re-permits the site later. DEMONSTRATED RED, per #4654: a hand-rolled copy reintroduced into a real `src/` file made `npm run lint` fail with the rule's full guidance message; removing it returned the tree to clean. Both halves recorded — red alone proves nothing, since a rule red for an unrelated reason looks identical. DISCLOSED: `defaultRequireFromInstallRoot` (gsd-tools.cjs) previously carried two distinct rejection messages and two manual realpath calls; routing it through `tryWithinRoot` collapses them to one message, and a missing module now surfaces as MODULE_NOT_FOUND rather than ENOENT. No test asserts either message. The security property is preserved and slightly strengthened — the candidate is realpathed and containment re-checked, and the dangling-symlink oracle closure comes along with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4654): record the containment ratchet in CONTEXT.md and the security model Both entries previously described the seam without the thing that keeps it a seam. They now state what the rule bans, and — more usefully for whoever reads this next — what it deliberately does NOT attempt: deciding per path.join call whether an argument came from user input. That question is not locally decidable, and an approximation across ~2000 join sites would earn an exemption list of hundreds, which is the opposite of a ratchet. Also records the marker's two legitimate justifications and that its reason is mandatory, so the escape stays reviewable rather than becoming a mute button. Glossary gate 270 refs exit 0; install-tree goldens and CONTEXT-INDEX.json regenerated and confirmed byte-identical rather than assumed — which also confirms eslint-rules/ is not a shipped path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4654): close review findings and the two matrix failures MATRIX FAILURE 1 — a collapsed message broke a negative-proof test, and my evidence for collapsing it was wrong. I searched tests/ for the literal string "resolves outside its install root", found nothing, and reported that no test asserted it. The test matches a REGEX SUBSTRING, /outside its install root/, so the literal search missed it. What broke was "NEGATIVE PROOF: a symlinked module pointing OUTSIDE the install root is not loaded" — the test guarding the exact property I claimed was preserved. defaultRequireFromInstallRoot now does both checks again with both messages byte-identical, each routed through the canonical predicate, which is better than the original since that hand-rolled both comparisons. MATRIX FAILURE 2 — shipped migrations are checksum-locked, and a marker cannot serve there. migrationChecksum hashes plan.toString(), which INCLUDES comments, so a suppression marker inside a plan body drifts the baseline exactly as an edit does. Measured: with markers in place, two of the four still differed from their committed checksums. The four shipped bodies are now byte-identical to next, and the rule's config excludes those four paths BY NAME rather than by a directory wildcard, so a NEW migration is still covered. Six containment comparisons stay un-ratcheted there; that gap is recorded in the rule's Known gaps, in CONTEXT.md and in the security model rather than left implicit. Justification (c) is removed from the marker's documented reasons, because a marker was proven unable to express it. ADVERSARIAL REVIEW — the sharpest finding was that the rule banned the CORRECT shape while permitting the incorrect one: startsWith(root) with no separator is the genuinely unsafe form, since it accepts a sibling such as root-evil, and my own test blessed it as valid. Flagging every bare startsWith would swamp the rule, so that stays a STATED gap rather than a silent one. Closed for real: the template-literal spelling, which the census never saw because it only inspected plus-concatenation — that surfaced TWELVE more sites, now triaged and migrated. A separator reached through a const alias is now resolved via scope analysis. And isContainedIn, exported in Phase 3, was missing from the discarded-result set, so a bare no-op call went unflagged on the one function the epic funnels through. SECURITY REVIEW — the marker could over-suppress two ways: a block comment worked identically to a line comment, and one marker silently covered every violation sharing its line. It now requires a Line comment positioned after the flagged node ends, so it anchors to the node it trails. Four sites had dropped an unreachable-but-deliberate equality rejection against the root; each is restored as the call site's own arm. eslint.config.mjs still documented the OLD marker token, which my rename missed — it would have sent the next author in circles. A FALSE GREEN, recorded because it nearly stuck: lint:ci reported exit 0 from a stale eslint cache while twelve real violations existed. Every lint check here now clears the cache first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4654): anchor a suppression marker to the violation it actually trails The matrix caught this; my own test caught it, on its first execution. The case "two violations on one line: trailing marker suppresses only the one it trails" expected 1 error and got 0 — both were suppressed. ROOT CAUSE: the anchoring accepted any Line comment on the node's line whose range started at or after the node's end. A trailing marker at the END of a line sits after EVERY node on that line, so that condition held for all of them. "After the node" does not identify WHICH node the marker trails. The fix reads as correct and is not. FIX: deferred reporting. Violations accumulate during traversal instead of being reported immediately; at Program:exit each marker claims exactly ONE pending violation — the one on its line whose end is nearest before the marker begins — and every unclaimed violation is then counted and reported. One marker, one suppression. An earlier violation sharing the line is still reported, which is the property the security review asked for and the previous attempt only appeared to deliver. The counter now increments at flush time rather than during traversal, so a suppressed occurrence still does not keep an allowlist entry alive. AND A TOOL THAT SHOULD HAVE EXISTED BEFORE THE FIRST MATRIX RUN. `node --test` is hard-blocked here, so this rule's test file could only ever be executed on the remote matrix — which is why a broken anchoring shipped into a run. ESLint's programmatic Linter API is not a test runner, and exercising the rule through it verifies every case locally in seconds. All 24 now pass locally, including the two-on-one-line case that failed remotely. That loop should have been built before the rule was first sent to the matrix rather than after it failed twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4654): backfill PR 4674 into the changeset and complete 70-docs.json The phase gate requires enablementSequence and the Diataxis quadrants; 70-docs now carries both, with the how-to quadrant skipped for a stated reason rather than an empty field. The audience for this deliverable is a contributor who trips the rule, and the task-oriented guidance reaches them in the ESLint message itself — which names the correct predicate, says how to choose between the realpath and lexical families, cites the Phase 3 regression caused by choosing wrong, and gives the marker syntax. A docs/how-to page would be a second, driftable copy read by nobody at the moment of failure. enablementSequence is recorded as what it actually is: a VERIFICATION sequence, not an enablement one. The rule is never off, so there is no off-to-on transition to describe. scripts/lint-docs-required.cjs now passes (ok_docs_updated) — it could not evaluate against the mandated pr:0 placeholder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c03f97188f |
chore(#1328): remove orphaned root vitest.config.ts left by SDK retirement
vitest.config.ts configured Vitest (not a dependency) to run .ts test files (the repo has none) rooted at ./sdk, a directory deleted when the SDK package seam was retired in #191 (ADR-0174). No npm script, workflow, or dependency references it. Also drop the now-dead sdk/src/*.test.* branch in diff-touches-shipped-paths.cjs isCiGating(), which can never match since the sdk/ tree no longer exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
f729101eec |
refactor(scripts): replace process.exit() with ExitError + runMain handler (#739) (#740)
Part 1 of 2 of the n/no-process-exit cleanup (umbrella #738): convert every process.exit() call in standalone scripts/** CLIs to the rule-compliant pattern. - New shared helper scripts/lib/cli-exit.cjs: ExitError(code,message) + runMain() which translates a thrown ExitError / returned number into process.exitCode (never process.exit()), flushing output and still firing process.on('exit'). - main()-based entrypoints: throw new ExitError(code) for errors, return <code> for verdicts; invoked via runMain(main). Child exit codes preserved via return. - top-level-only scripts: imperative body extracted into main() so mid-flow aborts (throw ExitError) actually halt; pure consts/helpers stay at module scope. - diff-touches-shipped-paths.cjs: stdin event handling restructured to an async read so the whole flow runs under runMain; uncaughtException/unhandledRejection nets replaced by an in-band catch that preserves EXIT_ERROR=2. Exit codes verified unchanged for every converted script (success/error/help and the 0/1/2 semantic codes in diff-touches). Rule stays warn here; flipped to error in part 2 (#738) once gsd-core/bin/** is also clean. Refs #739 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
36a237573c | chore(#192): retire sdk release pipeline seam | ||
|
Tom Boucher
|
0efe4f0612 |
fix(3621): release-sdk hotfix cherry-picks test-fixture commits (#3623)
* fix(3621): cherry-pick test-fixture commits in hotfix runs
The release-sdk hotfix loop excluded test-fixture updates that align CI
with a cherry-picked production fix, leaving the hotfix branch with new
production behavior and stale test assertions. Broke v1.42.3 CI (run
25949422676) when fix(3562) was cherry-picked but its bundled test
correction in docs(3562) commit
|
||
|
Tom Boucher
|
fb92d1e596 |
fix(#2983): classifier exit-code discipline, base-tag staging, drop vestigial merge-back (#2984)
* fix(#2983): classifier exit-code discipline, base-tag staging, drop vestigial merge-back Three issues surfaced by CodeRabbit's post-merge review of #2981 plus a production failure on the v1.39.1 release run. (1) Overloaded classifier exit code scripts/diff-touches-shipped-paths.cjs reused exit 1 for both the legitimate "no shipped paths" result and Node's default exit on uncaught throw, so any classifier failure (corrupt package.json, EPERM, etc.) was indistinguishable from a normal skip — the workflow's `if ! ... ; then skip` idiom would silently drop the commit. Distinct exit codes now: 0 shipped — at least one path is in the npm `files` whitelist 1 not shipped — CI / test / docs / planning only 2 classifier error — workflow MUST fail-fast uncaughtException + unhandledRejection + try/catch around fs/JSON parsing all route to exit 2 with stderr context. (2) Classifier missing at the base tag (CRITICAL) `Prepare hotfix branch` runs `git checkout -b "$BRANCH" "$BASE_TAG"` BEFORE the cherry-pick loop, replacing the working tree with the base tag's contents. Base tags predating #2980 (notably v1.39.0, the most likely next hotfix base) don't have scripts/diff-touches-shipped-paths.cjs at all — `node <missing>` exits non-zero — `if !` skips every commit — empty hotfix branch published. Strictly worse than the original #2980 push-rejection, which at least failed loudly. Stage the classifier from the dispatched ref's working tree into $RUNNER_TEMP at the top of the run script (before any working-tree- mutating git command). The cherry-pick loop now references $CLASSIFIER (staged) instead of the in-tree path. Sanity guards: refuse to start if scripts/diff-touches-shipped-paths.cjs is missing in the dispatched ref, refuse to proceed if cp didn't materialize $CLASSIFIER. The cherry-pick loop captures node's exit via ${PIPESTATUS[1]} and dispatches via explicit case: 0 proceed with cherry-pick 1 skip into NON_SHIPPED_SKIPPED * emit ::error:: + exit "$CLASSIFIER_RC" (3) Drop the merge-back PR step Auto-cherry-pick only picks commits already on main (`git cherry HEAD origin/main` outputs the unmerged ones; we filter fix:/chore: from main). By construction every code commit on the hotfix branch is already on main. The only hotfix-branch-only commit is `chore: bump version to X.Y.Z for hotfix`, which either no-ops against main or rewinds main's in-progress version. The merge-back PR was vestigial. It also failed in production on run 25232968975 with `GitHub Actions is not permitted to create or approve pull requests (createPullRequest)` — org policy blocks PR creation from the workflow's GH_TOKEN. Even without that block, the PR would have nothing useful to merge. Step removed. The `pull-requests: write` permission granted solely for the merge-back step has been dropped from the release job (least-privilege). Regression coverage tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs adds 12 assertions across two describe blocks: - 5 classifier behavioral: exit 0/1 preserved, exit 2 on missing package.json, exit 2 on malformed JSON, exit-code constants exported. - 7 workflow contract: classifier staged before checkout, target is $RUNNER_TEMP, missing-source guard, missing-staged guard, PIPESTATUS-based dispatch, error branch fails workflow, loop uses staged path (not in-tree). tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs updated where it asserted the pre-#2983 `if ! ... ; then` shape: now accepts the post-#2983 case-dispatch form. The test still proves the classifier participates; bug-2983 enforces the specific shape. Run summary references for the curious reviewer: - Run 25232010071 — original #2980 trigger (workflow-file push rejection) - Run 25232968975 — failed merge-back step that prompted the "is this even useful?" question that drove the removal Closes #2983 * fix(#2983): address CodeRabbit findings on PR #2984 Two findings, both real, both fixed. (1) [Critical] PIPESTATUS capture clobbered by `|| true` Pre-fix shape: git diff-tree ... | node "$CLASSIFIER" || true CLASSIFIER_RC="${PIPESTATUS[1]}" When the classifier exits 1 ("not shipped" — common case) or 2 (error), `|| true` triggers the right-hand side. `true` is a one-command "pipeline" that overwrites PIPESTATUS to (0). ${PIPESTATUS[1]} on the next line is therefore unset (or stale under set -u). The case dispatch then matched the empty string — falling into `*)` and failing the workflow on every non-shipped commit, OR matching `0)` after some shells default-init unset to 0 and silently picking commits that don't ship. Local repro confirms the issue: $ bash -c 'set -euo pipefail; false | sh -c "exit 7" || true; \ echo "PIPESTATUS: ${PIPESTATUS[*]}"; \ echo "[1]: ${PIPESTATUS[1]:-<unset>}"' PIPESTATUS: 0 [1]: <unset> Fix: bracket the pipeline in `set +e`/`set -e`, snapshot PIPESTATUS into a local array on the very next line, then dispatch on the snapshot: set +e git diff-tree ... | node "$CLASSIFIER" PIPE_RC=("${PIPESTATUS[@]}") set -e DIFFTREE_RC="${PIPE_RC[0]}" CLASSIFIER_RC="${PIPE_RC[1]}" The snapshot must happen on the first line after the pipeline; any intervening simple command resets PIPESTATUS. The array form is invariant against that. Bonus from the new shape: $DIFFTREE_RC is now also captured. git diff-tree is unlikely to fail on a known-good $SHA, but if it does, we no longer feed partial/empty input to the classifier and call it "not shipped." A non-zero DIFFTREE_RC emits ::error::git diff-tree failed and exits. (2) [Minor] Stale "Merge-back PR opened against main" summary line The hotfix run summary still printed: echo "- Merge-back PR opened against main" But the merge-back step itself was removed in the previous commit on this branch. Operators reading the summary would expect a PR that doesn't exist. Replaced with explicit non-action text: echo "- No merge-back PR (auto-picked commits are already on main)" Test coverage bug-2983 test file gains 3 assertions: - PIPE_RC array-snapshot pattern is required (regex matches the exact `PIPE_RC=("${PIPESTATUS[@]}")` form). - The `pipeline || true; ${PIPESTATUS[1]}` antipattern is explicitly forbidden via assert.doesNotMatch. - DIFFTREE_RC is captured from PIPE_RC[0] and a non-zero value triggers ::error::git diff-tree failed. - Run summary forbids `Merge-back PR opened against main` and requires the new non-action sentence. bug-2964 test's loop-anchor window bumped 6 KB → 8 KB to accommodate the additional pre-pick scaffolding (the test's own comment had already anticipated this kind of growth, citing prior precedents from #2970 and #2980). Mark CodeRabbit comments resolved post-commit. Refs CR finding ids 3175253571, 3175253578 on PR #2984. |
||
|
Tom Boucher
|
7424271aa0 |
fix(#2980): hotfix cherry-pick only picks commits that change what ships (#2981)
* fix(#2980): pre-skip workflow-file cherry-picks in release-sdk hotfix loop The default GITHUB_TOKEN issued to the release-sdk run lacks the `workflow` scope, so the prepare job's `git push origin "$BRANCH"` is rejected by GitHub when any cherry-picked commit modifies a file under `.github/workflows/`: ! [remote rejected] hotfix/X.YY.Z -> hotfix/X.YY.Z (refusing to allow a GitHub App to create or update workflow ... without `workflows` permission) Pre-#2980 behavior: the auto_cherry_pick loop happily picked workflow-file commits, then the trailing push exploded with no clear signal which commit was the culprit. v1.39.1 hit this on PR #2977 (run 25232010071) — earlier release-sdk fixes (#2965, #2967, #2970) had been skipped on conflict so their workflow-file changes never reached the push step, masking the bug; #2977 was the first workflow-file commit to apply cleanly and the push immediately exploded. Fix: pre-pick guard in the cherry-pick loop. Inspect each candidate commit's file list via `git diff-tree --no-commit-id --name-only -r` BEFORE attempting the pick. If any path matches `^\.github/workflows/`, skip the commit, emit a `::warning::` annotation naming the dropped commit, and append to a new `WORKFLOW_SKIPPED` bucket. The run summary surfaces this bucket in its own section, distinct from `CONFLICT_SKIPPED` (real merge conflicts) and `POLICY_SKIPPED` (feat/refactor exclusions), so operators reviewing the run never confuse the remediation paths. The loud-warning piece is non-negotiable: silent drops were explicitly rejected as a failure mode during the option-1/2/3 tradeoff discussion. If a workflow-file fix genuinely needs to ship in a hotfix, the operator applies it manually on the hotfix branch using a token with `workflow` scope, or lands it on main and re-cuts the release. Regression covered by tests/bug-2980-skip-workflow-file-cherrypicks.test.cjs (5 assertions: pre-pick guard exists, uses `git diff-tree`, emits `::warning::`, lands in dedicated bucket, surfaces in summary). The bug-2964 test's 4 KB window after the cherry-pick-loop anchor was nudged to 6 KB to accommodate the new pre-pick scaffolding — the test's own comment had already anticipated this kind of growth (citing #2970's merge-commit pre-skip as prior precedent). Closes #2980 * refactor(#2980): replace workflow-file pre-skip with shipped-paths filter The previous commit on this branch caught only the .github/workflows/* subset of the bug, treating the symptom (push rejection on workflow-file changes) rather than the root cause (the fix:/chore: filter is too broad — it picks any commit with that conventional-commit type even when the diff cannot affect the published npm package). CI-only fixes (release-sdk.yml itself, hotfix tooling, test-only commits) shouldn't flow through hotfix runs at all — they cannot change what `npm install get-shit-done-cc@X.YY.Z` produces. The .github/workflows/* push rejection is just the loudest of these "shouldn't have been picked" cases; tests/, docs/, .planning/ commits get picked silently with the same lack of effect on consumers. Replace the workflow-file pre-skip with a shipped-paths filter: - New scripts/diff-touches-shipped-paths.cjs reads package.json `files`, plus package.json itself (always-shipped per `npm pack` semantics), and exits 0 iff any input path is in the shipped set. Lockfile is not shipped (npm pack excludes it unless explicitly in `files`). - Workflow loop now pipes `git diff-tree --no-commit-id --name-only -r` through the classifier; on exit 1 the commit is skipped and appended to a new NON_SHIPPED_SKIPPED bucket (replaces WORKFLOW_SKIPPED). - Run summary surfaces NON_SHIPPED_SKIPPED as informational — no ::warning:: annotation. A non-shipping commit cannot affect the package, so a yellow alert would imply remediation is possible and would mislead operators. The classifier in a separate .cjs file (rather than inline bash heredoc) is so its rules — directory-prefix vs exact-match, package.json-always-shipped, lockfile-not-shipped — are unit-testable in tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs (11 new assertions: 4 static workflow + 6 classifier behavioral + 1 mixed- diff edge case). Why this dissolves the original push-rejection bug: workflow files aren't in `files`, so workflow-only commits are skipped pre-pick. The push step never sees them. If a workflow-file fix genuinely needs to ship in a hotfix release (extremely rare — the hotfix workflow is read from main's ref, not the hotfix branch's), the operator applies it manually using a token with `workflow` scope. The pre-skip puts that requirement in the run summary explicitly. Closes #2980 |