diff --git a/.github/scripts/openspec-tracking.cjs b/.github/scripts/openspec-tracking.cjs index 519c947c..53178ccf 100644 --- a/.github/scripts/openspec-tracking.cjs +++ b/.github/scripts/openspec-tracking.cjs @@ -46,6 +46,17 @@ * * Usage: * node .github/scripts/openspec-tracking.cjs < input.json + * node .github/scripts/openspec-tracking.cjs --list [changesDirectory] + * + * The second form is the one three workflows call to list unarchived change + * directories: it prints `listUnarchivedChanges()` as a sorted JSON array of + * names on stdout, and nothing else. That routes the listing through + * `readdirSync`, which behaves the same on every platform `node` runs on, + * rather than through a shell `find`, where `-printf` is a GNU extension that + * a checkout under BSD find (macOS) does not recognise. A missing changes + * directory prints `[]` and exits zero; an unreadable one throws and exits + * non-zero, and the calling workflow turns that into a `::warning::` + * annotation rather than either an empty list or a failed run. * * Reads one JSON object on stdin and prints the plan as JSON on stdout: * @@ -308,6 +319,15 @@ function planActions(input) { // An open issue for a change that is no longer under `openspec/changes/` has // been archived. This is the success path and the only way an issue closes // without a human. + // + // PRECONDITION: `unarchived` is a listing that actually succeeded. Absence + // from it is read as "archived", so an EMPTY array means "every change is + // archived", never "the listing could not be read". A caller that cannot + // enumerate the directory must skip the run rather than pass `[]`: doing the + // latter closes every open tracking issue with "Archived. reached + // openspec/changes/archive/", which is false. Each workflow gates its apply + // step on the listing having succeeded, and openspec-sweep.yml did not, which + // is what this note exists to stop happening again. for (const issue of issues) { if (issue.state === "open" && !seen.has(issue.change)) { const action = { @@ -328,6 +348,25 @@ function main(raw) { return planActions(input); } +const DEFAULT_CHANGES_DIRECTORY = "openspec/changes"; + +/** + * `--list` mode. Prints `listUnarchivedChanges(changesDirectory)` as a JSON + * array on stdout, and nothing else, so a caller can pipe stdout straight + * into `jq` without stripping any other output. + * + * A missing directory is not a fault (see `listUnarchivedChanges`) and prints + * `[]`. An unreadable one throws out of `readdirSync`, which this + * deliberately does not catch: letting it propagate is what turns it into a + * non-zero exit for `require.main` to report, which is the signal the calling + * workflow needs to tell "nothing to report" apart from "could not tell". + */ +function runList(changesDirectory) { + process.stdout.write( + `${JSON.stringify(listUnarchivedChanges(changesDirectory))}\n` + ); +} + module.exports = { ARCHIVE_DIRECTORY, DEFAULT_STALE_DAYS, @@ -340,11 +379,17 @@ module.exports = { issueBody, planActions, main, + runList, }; if (require.main === module) { try { - process.stdout.write(`${JSON.stringify(main(), null, 2)}\n`); + const args = process.argv.slice(2); + if (args[0] === "--list") { + runList(args[1] ?? DEFAULT_CHANGES_DIRECTORY); + } else { + process.stdout.write(`${JSON.stringify(main(), null, 2)}\n`); + } } catch (error) { console.error(`openspec-tracking failed: ${error.message}`); process.exitCode = 1; diff --git a/.github/scripts/openspec-tracking.test.cjs b/.github/scripts/openspec-tracking.test.cjs index cb99e2e5..6d7ea404 100644 --- a/.github/scripts/openspec-tracking.test.cjs +++ b/.github/scripts/openspec-tracking.test.cjs @@ -28,8 +28,25 @@ const { issueBody, planActions, main, + runList, } = require("./openspec-tracking.cjs"); +/** Captures what `runList` writes to stdout, without a real process spawn. */ +function captureList(changesDirectory) { + const original = process.stdout.write.bind(process.stdout); + let written = ""; + process.stdout.write = (chunk) => { + written += chunk; + return true; + }; + try { + runList(changesDirectory); + } finally { + process.stdout.write = original; + } + return JSON.parse(written); +} + /** The decision-bearing fields of an action, without the rendered prose. */ function shape(action) { const { title, body, comment, ...rest } = action; @@ -76,6 +93,38 @@ test("a missing changes directory is not a fault", () => { assert.deepEqual(listUnarchivedChanges("/nonexistent/openspec/changes"), []); }); +test("--list prints the scan as a sorted JSON array, archive excluded", () => { + const root = mkdtempSync(join(tmpdir(), "openspec-tracking-list-")); + try { + const changes = join(root, "changes"); + mkdirSync(join(changes, "archive", "2026-01-01-old"), { recursive: true }); + mkdirSync(join(changes, "beta"), { recursive: true }); + mkdirSync(join(changes, "alpha"), { recursive: true }); + assert.deepEqual(captureList(changes), ["alpha", "beta"]); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("--list against a missing directory prints [], not a fault", () => { + assert.deepEqual(captureList("/nonexistent/openspec/changes"), []); +}); + +test("--list round-trips a change name containing a space", () => { + const root = mkdtempSync(join(tmpdir(), "openspec-tracking-list-space-")); + try { + const changes = join(root, "changes"); + mkdirSync(join(changes, "probe with spaces"), { recursive: true }); + mkdirSync(join(changes, "probe-normal"), { recursive: true }); + assert.deepEqual(captureList(changes), [ + "probe with spaces", + "probe-normal", + ]); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + test("a pull request touching a change directory claims it", () => { const claims = claimsFromPullRequests([ { number: 265, files: ["openspec/changes/thing/tasks.md", "src/a.ts"] }, @@ -337,6 +386,29 @@ test("the sweep escalates at most once per window", () => { ); }); +test("an empty listing closes every tracked issue, so a caller must never guess it", () => { + // Pinning the hazard, not the feature. Absence from `unarchived` is read as + // "archived", so `[]` claims every change is archived. A workflow that could + // not read the directory and passed `[]` anyway would close every open + // tracking issue with a false comment. openspec-sweep.yml did exactly that + // until its apply step was gated on the listing having succeeded. + const plan = planActions({ + mode: "sweep", + sha: "abc123", + staleDays: 7, + unarchived: [], + issues: [ + { number: 11, state: "open", body: marker("real-change") }, + { number: 12, state: "open", body: marker("other-change") }, + ], + }); + assert.deepEqual(shapes(plan), [ + { type: "close", reason: "archived", change: "real-change", issue: 11 }, + { type: "close", reason: "archived", change: "other-change", issue: 12 }, + ]); + assert.match(plan.actions[0].comment, /reached `openspec\/changes\/archive\/`/); +}); + test("an unknown mode is a caller defect, not a silent pass", () => { assert.throws(() => planActions({ mode: "daily" }), /unknown mode/); }); diff --git a/.github/workflows/openspec-label.yml b/.github/workflows/openspec-label.yml index 41e0ba3c..b8afeb63 100644 --- a/.github/workflows/openspec-label.yml +++ b/.github/workflows/openspec-label.yml @@ -63,10 +63,41 @@ jobs: run: | set -euo pipefail + # The listing is delegated to openspec-tracking.cjs --list rather + # than to a shell `find`. A `find` form can always be made portable + # for today's shapes (this file once carried `-printf '%f\n'`, a GNU + # extension BSD find rejects, fixed to `-exec basename {} \;` + # instead), but that is a property someone has to get right again + # every time the listing changes. Routing it through `readdirSync`, + # already covered by openspec-tracking.test.cjs, removes the + # question of which `find` flags are portable rather than answering + # it correctly this once. + # + # The script can still fail (an unreadable `openspec/changes`), and + # that must read as "could not tell" rather than as "resolved": + # reporting an unreadable directory as zero unarchived changes would + # tell a stack its OpenSpec change is done when nobody checked. So + # the exit status is captured outside the assignment position `set + # -e` watches, and an unreadable directory gets its own summary and + # an explicit annotation instead of either silence or an abort. This + # workflow's whole point is to never fail a check for a reason + # unrelated to the pull request. changes="" - if [ -d openspec/changes ]; then - changes=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -printf '%f\n' | sort) + listing_failed=0 + if ! raw=$(node .github/scripts/openspec-tracking.cjs --list 2>&1); then + echo "::warning::Could not list openspec/changes/ ($raw). Leaving the '$LABEL' label untouched rather than guessing." + listing_failed=1 + else + changes=$(jq -r '.[]' <<<"$raw") + fi + + if [ "$listing_failed" -eq 1 ]; then + { + echo "### OpenSpec: unknown" + echo "" + echo "Could not list \`openspec/changes/\` on this branch, so whether an unarchived change remains is unknown. The \`$LABEL\` label was left as-is." + } >> "$GITHUB_STEP_SUMMARY" + exit 0 fi if [ -z "$changes" ]; then diff --git a/.github/workflows/openspec-sweep.yml b/.github/workflows/openspec-sweep.yml index 6de1ce5b..5401d91d 100644 --- a/.github/workflows/openspec-sweep.yml +++ b/.github/workflows/openspec-sweep.yml @@ -50,6 +50,7 @@ jobs: fetch-depth: 0 # full history, so `git log` can date each change directory - name: Plan escalations + id: plan env: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} @@ -59,8 +60,33 @@ jobs: set -euo pipefail unarchived='[]' - if [ -d openspec/changes ]; then - now=$(date +%s) + now=$(date +%s) + # The listing is delegated to openspec-tracking.cjs --list rather + # than to a shell `find`. This file once carried `find ... -printf + # '%f\n'` inside a process substitution: `done < <(...)` hands the + # loop the substitution's own exit status, not find's, so a failed + # `-printf` on BSD find (macOS) ran the loop zero times and this + # step silently reported "0 unarchived changes" and exited 0. That + # is the exact silent case this workflow exists to avoid, and a + # different `find` form only fixes this one occurrence of it. + # Routing the listing through `readdirSync`, already covered by + # openspec-tracking.test.cjs, removes the question of which `find` + # flags are portable rather than answering it correctly once. The + # script's own exit status is captured by a plain assignment, not a + # process substitution, so its failure cannot go unseen the same way. + if ! changes_json=$(node .github/scripts/openspec-tracking.cjs --list 2>&1); then + # SKIP THE RUN, do not fall through. An empty `unarchived` is not + # "we could not tell", it is "every change is archived": the + # planner's close-as-archived pass closes every open tracking issue + # whose change is absent from the listing. Falling through here + # closed all of them with "Archived. reached + # openspec/changes/archive/", which is false, on nothing worse than + # a transient read error. This mirrors openspec-tracking.yml, which + # gates its apply step the same way. + echo "::warning::Could not list openspec/changes/ ($changes_json). Reporting zero unarchived changes would close every open tracking issue as archived, so this run is skipped instead." + echo "skipped=true" >> "$GITHUB_OUTPUT" + exit 0 + else # Names and ages go into JSON as jq ARGUMENTS, never joined into a # line and split back apart. A directory name containing a space # desynced the split, `tonumber` threw on the wrong field, and under @@ -80,8 +106,7 @@ jobs: fi unarchived=$(jq -c --arg name "$name" --argjson age "$age" \ '. + [{name: $name, ageDays: $age}]' <<<"$unarchived") - done < <(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -printf '%f\n' | sort) + done < <(jq -r '.[]' <<<"$changes_json") fi # Bodies are handed over raw. The marker is parsed in one place, in @@ -114,8 +139,10 @@ jobs: > /tmp/openspec-sweep-plan.json cat /tmp/openspec-sweep-plan.json + echo "skipped=false" >> "$GITHUB_OUTPUT" - name: Apply the plan + if: steps.plan.outputs.skipped == 'false' env: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} diff --git a/.github/workflows/openspec-tracking.yml b/.github/workflows/openspec-tracking.yml index 4daf80c7..8109f3f5 100644 --- a/.github/workflows/openspec-tracking.yml +++ b/.github/workflows/openspec-tracking.yml @@ -92,12 +92,28 @@ jobs: --json number,state,body \ | jq -c '[.[] | {number, state: (.state | ascii_downcase), body: (.body // "")}]') + # The listing is delegated to openspec-tracking.cjs --list rather + # than to a shell `find`. This file once carried `-printf '%f\n'`, a + # GNU extension a checkout under BSD find (macOS) does not + # recognise, and its failure reached this pipe under `pipefail`, + # aborting the whole run. A different `find` form fixes that + # occurrence; routing the listing through `readdirSync`, already + # covered by openspec-tracking.test.cjs, removes the question of + # which `find` flags are portable rather than answering it once. + # + # A listing failure here (an unreadable `openspec/changes`) must not + # read as "nothing unarchived": that would close or skip a tracking + # issue for a change that is still there, nobody just could not see + # it. So this is the same skip this step already takes when + # `gh pr list` fails above: warn, skip this run, let the next push + # re-evaluate, and never fail the check. unarchived='[]' - if [ -d openspec/changes ]; then - unarchived=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -printf '%f\n' | sort \ - | jq -R -s -c 'split("\n") | map(select(length > 0) | {name: .})') + if ! names_json=$(node .github/scripts/openspec-tracking.cjs --list 2>&1); then + echo "::warning::Could not list openspec/changes/ ($names_json). Skipping this run rather than reporting a change as archived when nobody could tell." + echo "skipped=true" >> "$GITHUB_OUTPUT" + exit 0 fi + unarchived=$(jq -c '[.[] | {name: .}]' <<<"$names_json") jq -n -c \ --arg sha "$SHA" \