Skip to content

gstack-diff-scope reports every scope false on uncommitted work, and can't set two categories for one file #2299

Description

@ShahriarLak

gstack-diff-scope reports every scope false on uncommitted work, and can't set two categories for one file

Version: 1.60.1.0
File: bin/gstack-diff-scope

Two independent bugs. Both fail silently — SCOPE_* comes back false rather than erroring, so the consuming skill just skips a reviewer and prints nothing. I hit the first one during a /ship and only noticed because I knew the diff was frontend-only.


Bug 1 — uncommitted work is invisible, so /ship skips reviewers

git diff BASE...HEAD --name-only exits 0 with empty output when the branch has no commits yet. The || fallback is therefore never reached:

# bin/gstack-diff-scope:10
FILES=$(git diff "${BASE}...HEAD" --name-only 2>/dev/null || git diff "${BASE}" --name-only 2>/dev/null || echo "")

FILES is empty, the early-exit block at :12-23 prints all-false, and every scope-gated reviewer is skipped.

This is not an edge case for /ship. /ship detects scope in Step 9 (Pre-Landing Review), which runs before it commits in Step 15. So the common flow — start work, run /ship with changes still in the working tree — hits this every time.

Repro

git checkout -b feat/x            # no commits yet
echo "// x" >> src/Button.jsx     # uncommitted frontend change
gstack-diff-scope main
# SCOPE_FRONTEND=false   ← expected true
# ...all false
git add -A && git commit -m x
gstack-diff-scope main
# SCOPE_FRONTEND=true    ← same diff, different answer

Impact

In ship/sections/review-army.md, specialist dispatch is gated on these flags:

  • SCOPE_FRONTEND → design specialist
  • SCOPE_AUTH / SCOPE_BACKEND → security specialist
  • SCOPE_MIGRATIONS → data-migration specialist (tagged [NEVER_GATE], i.e. meant to run even when silent)

An uncommitted migration or auth change silently skips the specialist that exists specifically to catch it. Step 1 also uses SCOPE_FRONTEND to decide whether to recommend /design-review, so that recommendation never fires either.

Suggested fix

Union the committed diff with the working tree and untracked files:

COMMITTED=$(git diff "${BASE}...HEAD" --name-only 2>/dev/null || true)
WORKING=$(git diff HEAD --name-only 2>/dev/null || true)
UNTRACKED=$(git ls-files --others --exclude-standard 2>/dev/null || true)
FILES=$(printf '%s\n%s\n%s\n' "$COMMITTED" "$WORKING" "$UNTRACKED" | sed '/^$/d' | sort -u)

Untracked matters too: a brand-new .jsx component or test file is exactly the kind of thing a reviewer should see, and /ship commits it in Step 15 regardless.


Bug 2 — one case for nine independent flags makes categories mutually exclusive

Lines :35-83 classify every file through a single case. case is first-match-wins, so each file can only ever set one category, even though the nine SCOPE_* values are independent booleans.

*.jsx (:39) precedes *.test.* (:53), so:

gstack-diff-scope main   # diff contains only src/Button.test.jsx
# SCOPE_FRONTEND=true
# SCOPE_TESTS=false      ← it is unambiguously a test file

The asymmetry is visible within the same run: util.test.ts sets TESTS (because *.test.* at :53 precedes *.ts at :81) while Button.test.jsx sets FRONTEND and not TESTS. Same intent, opposite result, purely from arm ordering.

Suggested fix

One case per category so the flags are genuinely independent.

BACKEND is the one that must stay exclusive — the # Backend: everything else that's code (excluding views/components already matched) comment at :76 shows it currently relies on fall-through to avoid claiming .tsx/.jsx. Make that explicit with a per-file frontend flag:

f_frontend=false
case "$f" in
  *.tsx|*.jsx|*.vue|*.svelte|*.astro) f_frontend=true ;;
  # ...remaining frontend patterns
esac
[ "$f_frontend" = true ] && FRONTEND=true

# ...independent case blocks for PROMPTS / TESTS / DOCS / CONFIG /
#    MIGRATIONS / API / AUTH

if [ "$f_frontend" = false ]; then
  case "$f" in
    *.rb|*.py|*.go|*.rs|*.java|*.php|*.ex|*.exs) BACKEND=true ;;
    *.ts|*.js|*.mjs|*.cjs|*.mts|*.cts) BACKEND=true ;;
  esac
fi

Known behaviour change

With independent flags, util.test.ts sets BACKEND and TESTS where it previously set only TESTS. Backend-test-only PRs can then trip the security specialist via the SCOPE_BACKEND=true AND DIFF_LINES>100 gate. That errs toward more review rather than less, but it's a real change and worth a deliberate call.


Verification

I patched both locally and checked 15 cases:

Input Before After
uncommitted .jsx, no commits (all false) FRONTEND
untracked new .jsx (all false) FRONTEND
Button.test.jsx FRONTEND FRONTEND TESTS
Button.spec.tsx FRONTEND FRONTEND TESTS
util.test.ts TESTS BACKEND TESTS
A.jsx FRONTEND FRONTEND
server.ts BACKEND BACKEND
app.mjs BACKEND BACKEND
db/migrate/001_x.rb BACKEND MIGRATIONS BACKEND MIGRATIONS
src/api/users.ts BACKEND API BACKEND API
src/lib/auth.ts BACKEND AUTH BACKEND AUTH
README.md DOCS DOCS
package.json CONFIG CONFIG
clean tree, no changes (all false) (all false)

Happy to open a PR if useful.

Minor

*.module.css|*.module.scss at :38 are unreachable — they sit in the same arm as *.css|*.scss, which already match them.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions