Skip to content

growth-guards: gg_policy_content collapses git's exit 128 into 'not in the commit', so a failed probe applies looser policy than the index #1508

Description

@bmethod

gg_policy_content in scripts/lib/common.sh collapses git's operational failure (exit 128) into its "not in the commit" answer, so a failed probe silently loads looser policy than the index. The same family already refuses that collapse one file over, which is what makes this look like an oversight rather than a deliberate trade.

The inconsistency

scripts/lib/settings.sh's gg_settings_source documents the distinction explicitly — its comment notes that a missing path and an operational failure both come back nonzero, "with the same 128 an operational failure returns — so it is answered here".

gg_policy_content does not make that distinction:

gg_policy_content() { # FILE — content on stdout; 1 = the commit has no such file
  local file="$1"
  if git ls-files --error-unmatch -- "$file" >/dev/null 2>&1; then
    git show ":$file" || gg_collection_error "could not read the staged copy of $file"
    return 0
  fi
  if git cat-file -e "HEAD:$file" 2>/dev/null; then
    return 1
  fi
  if [ -f "$file" ]; then
    cat -- "$file" || gg_collection_error "could not read $file"
    return 0
  fi
  return 1
}

Both probes discard the exit status, so exit 1 ("no such path") and exit 128 (unreadable index, not a repository, corrupt object) are indistinguishable. Either falls through.

Why it matters

The function's own header states the invariant it is there to hold: policy files come from the INDEX, so staged edits govern staged scans, and a path staged for deletion governs as ABSENT. A failed probe breaks that in the fail-open direction:

  • a failed ls-files probe falls through to the worktree copy, so a commit is judged against unstaged excludes rather than the staged ones;
  • a failed cat-file probe falls through the same way, so a staged deletion of a policy file can be read as a never-tracked path and the worktree copy applied anyway;
  • either way the commit is judged against looser policy than the index actually carries, and nothing says so.

A gate that cannot read its own policy should not report a clean verdict — the family's own 0 clean / 1 violations / 2 could-not-complete contract already has the right answer available.

Second instance

scripts/suppression-ban repeats the pattern for the baseline read (around L189–197).

Pathspec note

Neither call uses :(literal), so a glob-shaped path (one containing *, ? or [) can miss the index entry it is meant to match and take the same fall-through.

Suggested fix

Distinguish the exit codes the way gg_settings_source already does — treat exit 1 as "absent" and anything else as an operational failure routed through gg_collection_error — and pass :(literal) on both probes.

Provenance

Found by Cursor Bugbot on drovr PR vanillagreencom/drovr#595, which tracks growth-guards as a committed skill (DRO-260); verified by reading against gg_settings_source. Not patched locally, since a vstack refresh would revert it.

Related reports from the same PR: #1500 (fixture repos inherit ambient git config), #1501 (shared-namespace cleanup count), #1502 (non-atomic settings-cache and baseline writes), #1503 (spurious error line on a passing run), #1506 (process-group signalling breaks concurrent runs), #1507 (skip-safe branch makes a suite vacuous).

Metadata

Metadata

Labels

skillsskills/, agents/, skill-templates/, and base agent instructions

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions