From a07d45997191bb22b7504aaf1df738057553377f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sun, 6 Sep 2026 10:38:42 -0700 Subject: [PATCH 1/3] fix(cli): list change directories portably, and stop swallowing find failures The three OpenSpec reporting workflows listed openspec/changes with `find -printf '%f\n'`, a GNU extension BSD find does not recognise. On label.yml and tracking.yml that aborted the run under set -e/pipefail; on sweep.yml the listing sat inside a process substitution, so its failure never reached the loop and the step silently reported zero unarchived changes instead. Replace -printf with `-exec basename {} \;`, which both find implementations support, and capture the listing's own exit status explicitly rather than through a pipe or process substitution. An unreadable openspec/changes now produces an explicit ::warning:: annotation and skips reporting, distinguishable in the log from "nothing to report", while the run itself still never fails a check. --- .github/workflows/openspec-label.yml | 35 +++++++++++++++- .github/workflows/openspec-sweep.yml | 56 +++++++++++++++---------- .github/workflows/openspec-tracking.yml | 20 ++++++++- 3 files changed, 86 insertions(+), 25 deletions(-) diff --git a/.github/workflows/openspec-label.yml b/.github/workflows/openspec-label.yml index 41e0ba3c..6a322a8f 100644 --- a/.github/workflows/openspec-label.yml +++ b/.github/workflows/openspec-label.yml @@ -63,10 +63,41 @@ jobs: run: | set -euo pipefail + # `-exec basename {} \;` lists directory names identically on GNU + # and BSD find. `-printf '%f\n'` does not: it is a GNU extension, so + # a checkout built or tested on macOS ran BSD find, which does not + # recognise `-printf`, and the failed find status once fed straight + # into this assignment under `set -e`, aborting the whole run. + # + # `find` itself 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="" + listing_failed=0 if [ -d openspec/changes ]; then - changes=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -printf '%f\n' | sort) + if ! changes=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ + -not -name archive -exec basename {} \; 2>&1); then + echo "::warning::Could not list openspec/changes/ (find reported: $changes). Leaving the '$LABEL' label untouched rather than guessing." + listing_failed=1 + changes="" + else + changes=$(printf '%s\n' "$changes" | sort) + fi + 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..c915a9d2 100644 --- a/.github/workflows/openspec-sweep.yml +++ b/.github/workflows/openspec-sweep.yml @@ -61,27 +61,41 @@ jobs: unarchived='[]' if [ -d openspec/changes ]; then now=$(date +%s) - # 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 - # `set -euo pipefail` that failed the whole run: the "signal expected - # to be red" failure this workflow exists to avoid. Nothing enforces - # kebab-case directory names, so the encoding must not assume it. - while IFS= read -r name; do - [ -z "$name" ] && continue - # The last commit that touched this change directory. A directory - # with no commits at all is treated as brand new rather than - # infinitely old, so a checkout quirk cannot manufacture a report. - last=$(git log -1 --format=%ct -- "openspec/changes/$name" || true) - if [ -z "$last" ]; then - age=0 - else - age=$(( (now - last) / 86400 )) - 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) + # `-exec basename {} \;` lists directory names identically on GNU + # and BSD find. `-printf '%f\n'` is a GNU extension that a checkout + # tested under BSD find does not recognise, and the failed find + # exit status used to feed a process substitution: `done < <(...)` + # hands the loop the substitution's own status, not find's, so the + # loop ran zero iterations and this step reported "0 unarchived + # changes" and exited 0. That is the silent case this workflow + # must not repeat, so the listing is captured into a variable + # first, where its exit status is the ordinary status of that + # assignment, checked explicitly rather than lost behind a pipe. + if ! names=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ + -not -name archive -exec basename {} \; 2>&1); then + echo "::warning::Could not list openspec/changes/ (find reported: $names). Reporting zero unarchived changes would be wrong; skipping the sweep for this run instead." + 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 + # `set -euo pipefail` that failed the whole run: the "signal expected + # to be red" failure this workflow exists to avoid. Nothing enforces + # kebab-case directory names, so the encoding must not assume it. + while IFS= read -r name; do + [ -z "$name" ] && continue + # The last commit that touched this change directory. A directory + # with no commits at all is treated as brand new rather than + # infinitely old, so a checkout quirk cannot manufacture a report. + last=$(git log -1 --format=%ct -- "openspec/changes/$name" || true) + if [ -z "$last" ]; then + age=0 + else + age=$(( (now - last) / 86400 )) + fi + unarchived=$(jq -c --arg name "$name" --argjson age "$age" \ + '. + [{name: $name, ageDays: $age}]' <<<"$unarchived") + done <<<"$(printf '%s\n' "$names" | sort)" + fi fi # Bodies are handed over raw. The marker is parsed in one place, in diff --git a/.github/workflows/openspec-tracking.yml b/.github/workflows/openspec-tracking.yml index 4daf80c7..550adb31 100644 --- a/.github/workflows/openspec-tracking.yml +++ b/.github/workflows/openspec-tracking.yml @@ -92,10 +92,26 @@ jobs: --json number,state,body \ | jq -c '[.[] | {number, state: (.state | ascii_downcase), body: (.body // "")}]') + # `-exec basename {} \;` lists directory names identically on GNU + # and BSD find; `-printf '%f\n'` is a GNU extension that a checkout + # tested under BSD find does not recognise, and the failed status + # used to reach this pipe under `pipefail`, aborting the whole run. + # + # A `find` 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 \ + if ! names=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ + -not -name archive -exec basename {} \; 2>&1); then + echo "::warning::Could not list openspec/changes/ (find reported: $names). Skipping this run rather than reporting a change as archived when nobody could tell." + echo "skipped=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + unarchived=$(printf '%s\n' "$names" | sort \ | jq -R -s -c 'split("\n") | map(select(length > 0) | {name: .})') fi From 43e8f0fcfb5f6ba3f8ec58f2a0c7e22047d8888e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sun, 6 Sep 2026 10:43:22 -0700 Subject: [PATCH 2/3] fix(cli): route change-directory listing through readdirSync, not find a07d459 fixed the immediate bug: -exec basename {} \; instead of -printf '%f\n', portable across GNU and BSD find. That commit is superseded here, not wrong; it is still a correct fix for the exact form it changed. The problem with stopping there is that "portable find" is a property of the specific flags chosen today, and nothing stops a future edit to any of these three workflows from reaching for -printf, -regex, or another GNU extension again. The question "which find flags are portable" can be asked and answered correctly and still be asked again next time by someone who does not know it was ever asked. openspec-tracking.cjs already has listUnarchivedChanges(), built on readdirSync and covered by tests. readdirSync behaves the same on every platform node runs on, so routing the listing through it removes the portability question rather than re-answering it. This adds a --list mode to the script (prints the scan as a sorted JSON array on stdout) and points all three workflows at it, shaping the JSON with jq into whatever shape each site needs downstream. The loudness property carries over unchanged: a missing openspec/changes prints [] and is not a fault; an unreadable one throws, which reaches the workflow as this process's non-zero exit, and each workflow turns that into an explicit ::warning:: annotation and a skip rather than either an empty list or a failed run. sweep.yml's iteration still reads names from a here-string/process-substitution built on the captured JSON rather than an unquoted word-split, so a name containing a space keeps surviving the trip (the failure mode #269 already fixed once). Adds three tests for --list: a tree with archive/ plus two changes, a missing directory, and a name containing a space round-tripping through the JSON array. --- .github/scripts/openspec-tracking.cjs | 38 ++++++++++- .github/scripts/openspec-tracking.test.cjs | 49 ++++++++++++++ .github/workflows/openspec-label.yml | 32 +++++----- .github/workflows/openspec-sweep.yml | 74 +++++++++++----------- .github/workflows/openspec-tracking.yml | 28 ++++---- 5 files changed, 153 insertions(+), 68 deletions(-) diff --git a/.github/scripts/openspec-tracking.cjs b/.github/scripts/openspec-tracking.cjs index 519c947c..14e78669 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: * @@ -328,6 +339,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 +370,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..f536f5fe 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"] }, diff --git a/.github/workflows/openspec-label.yml b/.github/workflows/openspec-label.yml index 6a322a8f..b8afeb63 100644 --- a/.github/workflows/openspec-label.yml +++ b/.github/workflows/openspec-label.yml @@ -63,14 +63,18 @@ jobs: run: | set -euo pipefail - # `-exec basename {} \;` lists directory names identically on GNU - # and BSD find. `-printf '%f\n'` does not: it is a GNU extension, so - # a checkout built or tested on macOS ran BSD find, which does not - # recognise `-printf`, and the failed find status once fed straight - # into this assignment under `set -e`, aborting the whole run. + # 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. # - # `find` itself can still fail (an unreadable `openspec/changes`), - # and that must read as "could not tell" rather than as "resolved": + # 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 @@ -80,15 +84,11 @@ jobs: # unrelated to the pull request. changes="" listing_failed=0 - if [ -d openspec/changes ]; then - if ! changes=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -exec basename {} \; 2>&1); then - echo "::warning::Could not list openspec/changes/ (find reported: $changes). Leaving the '$LABEL' label untouched rather than guessing." - listing_failed=1 - changes="" - else - changes=$(printf '%s\n' "$changes" | sort) - fi + 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 diff --git a/.github/workflows/openspec-sweep.yml b/.github/workflows/openspec-sweep.yml index c915a9d2..57f39b58 100644 --- a/.github/workflows/openspec-sweep.yml +++ b/.github/workflows/openspec-sweep.yml @@ -59,43 +59,43 @@ jobs: set -euo pipefail unarchived='[]' - if [ -d openspec/changes ]; then - now=$(date +%s) - # `-exec basename {} \;` lists directory names identically on GNU - # and BSD find. `-printf '%f\n'` is a GNU extension that a checkout - # tested under BSD find does not recognise, and the failed find - # exit status used to feed a process substitution: `done < <(...)` - # hands the loop the substitution's own status, not find's, so the - # loop ran zero iterations and this step reported "0 unarchived - # changes" and exited 0. That is the silent case this workflow - # must not repeat, so the listing is captured into a variable - # first, where its exit status is the ordinary status of that - # assignment, checked explicitly rather than lost behind a pipe. - if ! names=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -exec basename {} \; 2>&1); then - echo "::warning::Could not list openspec/changes/ (find reported: $names). Reporting zero unarchived changes would be wrong; skipping the sweep for this run instead." - 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 - # `set -euo pipefail` that failed the whole run: the "signal expected - # to be red" failure this workflow exists to avoid. Nothing enforces - # kebab-case directory names, so the encoding must not assume it. - while IFS= read -r name; do - [ -z "$name" ] && continue - # The last commit that touched this change directory. A directory - # with no commits at all is treated as brand new rather than - # infinitely old, so a checkout quirk cannot manufacture a report. - last=$(git log -1 --format=%ct -- "openspec/changes/$name" || true) - if [ -z "$last" ]; then - age=0 - else - age=$(( (now - last) / 86400 )) - fi - unarchived=$(jq -c --arg name "$name" --argjson age "$age" \ - '. + [{name: $name, ageDays: $age}]' <<<"$unarchived") - done <<<"$(printf '%s\n' "$names" | sort)" - fi + 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 + echo "::warning::Could not list openspec/changes/ ($changes_json). Reporting zero unarchived changes would be wrong; skipping the sweep for this run instead." + 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 + # `set -euo pipefail` that failed the whole run: the "signal expected + # to be red" failure this workflow exists to avoid. Nothing enforces + # kebab-case directory names, so the encoding must not assume it. + while IFS= read -r name; do + [ -z "$name" ] && continue + # The last commit that touched this change directory. A directory + # with no commits at all is treated as brand new rather than + # infinitely old, so a checkout quirk cannot manufacture a report. + last=$(git log -1 --format=%ct -- "openspec/changes/$name" || true) + if [ -z "$last" ]; then + age=0 + else + age=$(( (now - last) / 86400 )) + fi + unarchived=$(jq -c --arg name "$name" --argjson age "$age" \ + '. + [{name: $name, ageDays: $age}]' <<<"$unarchived") + done < <(jq -r '.[]' <<<"$changes_json") fi # Bodies are handed over raw. The marker is parsed in one place, in diff --git a/.github/workflows/openspec-tracking.yml b/.github/workflows/openspec-tracking.yml index 550adb31..8109f3f5 100644 --- a/.github/workflows/openspec-tracking.yml +++ b/.github/workflows/openspec-tracking.yml @@ -92,28 +92,28 @@ jobs: --json number,state,body \ | jq -c '[.[] | {number, state: (.state | ascii_downcase), body: (.body // "")}]') - # `-exec basename {} \;` lists directory names identically on GNU - # and BSD find; `-printf '%f\n'` is a GNU extension that a checkout - # tested under BSD find does not recognise, and the failed status - # used to reach this pipe under `pipefail`, aborting the whole run. + # 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 `find` failure here (an unreadable `openspec/changes`) must not + # 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 - if ! names=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d \ - -not -name archive -exec basename {} \; 2>&1); then - echo "::warning::Could not list openspec/changes/ (find reported: $names). Skipping this run rather than reporting a change as archived when nobody could tell." - echo "skipped=true" >> "$GITHUB_OUTPUT" - exit 0 - fi - unarchived=$(printf '%s\n' "$names" | 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" \ From a5ba03d143171bcb4d54d725594942a03071dad6 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sun, 6 Sep 2026 10:58:08 -0700 Subject: [PATCH 3/3] fix(cli): skip the sweep when the listing fails, instead of closing everything The review caught a real one, and the consequence is worse than the missing skip it looks like. `openspec-sweep.yml` warned on a failed listing and then fell through. `unarchived` stayed `[]`, and `[]` is not "we could not tell" to the planner, it is "every change is archived": absence from the listing is exactly what archiving looks like. So a transient read error would have closed every open tracking issue with "Archived. reached openspec/changes/archive/", which is false, and the run would have reported success. Reproduced against the committed planner before fixing: planActions({mode:"sweep", unarchived:[], issues:[#11 open, #12 open]}) -> [{close, archived, real-change, 11}, {close, archived, other-change, 12}] The failure branch now sets `skipped=true` and exits, and "Apply the plan" is gated on `steps.plan.outputs.skipped == 'false'`. That is what openspec-tracking.yml already did; the sweep was the one of the three that did not, which is why the earlier verification missed it. The loud path only checked that the listing step warned and exited 0, never what the planner then did with an empty array against pre-existing issues. Also records the precondition where it can be read: absence from `unarchived` means archived, so a caller that cannot enumerate the directory must skip rather than pass `[]`. A test pins the hazard rather than the feature, so the behaviour cannot be quietly relied on as safe. 231 script tests pass. `pnpm lint` clean apart from the five pre-existing warnings in files this branch does not touch. --- .github/scripts/openspec-tracking.cjs | 9 +++++++++ .github/scripts/openspec-tracking.test.cjs | 23 ++++++++++++++++++++++ .github/workflows/openspec-sweep.yml | 15 +++++++++++++- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/.github/scripts/openspec-tracking.cjs b/.github/scripts/openspec-tracking.cjs index 14e78669..53178ccf 100644 --- a/.github/scripts/openspec-tracking.cjs +++ b/.github/scripts/openspec-tracking.cjs @@ -319,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 = { diff --git a/.github/scripts/openspec-tracking.test.cjs b/.github/scripts/openspec-tracking.test.cjs index f536f5fe..6d7ea404 100644 --- a/.github/scripts/openspec-tracking.test.cjs +++ b/.github/scripts/openspec-tracking.test.cjs @@ -386,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-sweep.yml b/.github/workflows/openspec-sweep.yml index 57f39b58..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 }} @@ -74,7 +75,17 @@ jobs: # 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 - echo "::warning::Could not list openspec/changes/ ($changes_json). Reporting zero unarchived changes would be wrong; skipping the sweep for this run instead." + # 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 @@ -128,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 }}