[serbrech]

Problem

scripts/security-review.mjs check #4 (unsafe-git) iterates over every changed file with no file-type filter — unlike the other checks, which scope to workflowFiles or jstsFiles. Any markdown file containing the literal string git add . on an added line is reported as an error.

This fires on prose that documents the prohibition. Concretely, it blocked #1558 by flagging Squad's own decision log:

{
  "category": "unsafe-git",
  "severity": "error",
  "message": "Unsafe git operation: `git add .` ...",
  "file": ".squad/decisions.md",
  "line": 257
}

…where line ~262 reads:

What: Added mandatory Git Safety section to copilot-instructions.md: prohibits git add ., requires feature branches and PRs …

The scanner flagged the very rule it was documenting.

Why it surfaced now: .squad/decisions/inbox/ is gitignored, so inbox entries never appeared in PR diffs. Once Scribe merged them into .squad/decisions.md, those lines became + lines for the first time.

Fix

Adds UNSAFE_GIT_EXCLUDED_PATHS and skips the unsafe-git check for append-only memory/log files — records of what happened, never executed:

• .squad/decisions.md
• .squad/agents/*/history.md
• .squad/log/**, .squad/orchestration-log/** (gitignored; listed defensively)

Deliberately still in scope — instruction surfaces:

• .github/copilot-instructions.md
• .squad/agents/*/charter.md, .squad/team.md, .squad/routing.md, squad.agent.md
• .squad/templates/**

A malicious PR could swap a prohibition for an instruction in those files, so they must stay scannable. The line is drawn at files that are structurally append-only versus files agents read as active instructions.

Verification

Detection still works — a probe file with git add . and git push --force was correctly caught:

"file": "scripts/__scan-probe.sh", "line": 3  → git add .
"file": "scripts/__scan-probe.sh", "line": 4  → git push --force
🔒 Security review: 2 error(s).

Probe was deleted; no trace remains. Running the patched scanner against this PR's own diff returns ✅ No security concerns found.

Why this is a separate PR

squad-repo-health.yml uses pull_request_target and checks out the base branch for trusted scripts (correct — it prevents fork PRs from executing arbitrary code with a write token). That means CI always runs the dev copy of security-review.mjs, so this fix cannot take effect on the PR that contains it. It has to land on dev first.

Merge order: this PR → then #1558 goes green.

Known limitation (not fixed here)

The pattern /git\s+add\s+\./ also matches scoped paths like git add .squad/, which is the safe, recommended form. That is a pattern-precision bug distinct from this scoping bug — filed as follow-up rather than bundled in.

Refs #1556

[github-actions]

🟢 Impact Analysis — PR #1559

Risk tier: 🟢 LOW

📊 Summary

Metric	Count
Files changed	1
Files added	0
Files modified	1
Files deleted	0
Modules touched	1

🎯 Risk Factors

• 1 files changed (≤5 → LOW)
• 1 module(s) touched (≤1 → LOW)

📦 Modules Affected

scripts (1 file)

• scripts/security-review.mjs

---

This report is generated automatically for every PR. See #733 for details.

[github-actions]

🛫 PR Readiness Check

ℹ️ This comment updates on each push. Last checked: commit 8f45747

PR Scope: 🔧 Infrastructure

⚠️ 2 item(s) to address before review

Status	Check	Details
✅	Single commit	1 commit — clean history
✅	Not in draft	Ready for review
✅	Branch up to date	Up to date with dev
❌	Copilot review	No Copilot review yet — it may still be processing
✅	Changeset present	No source files changed — changeset not required
✅	Scope clean	No .squad/ or docs/proposals/ files
✅	No merge conflicts	No merge conflicts
✅	Copilot threads resolved	0 active Copilot thread(s) resolved (1 outdated skipped)
❌	CI passing	4 check(s) still running

Files Changed (1 file, +16 −0)

File	+/−
scripts/security-review.mjs	+16 −0

Total: +16 −0

---

This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.

[Copilot]

🟡 Not ready to approve

The new exclusion regex for agent histories does not match nested history paths (e.g. _alumni/.../history.md), so false positives can still occur in repo-real append-only history files.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR updates the unsafe-git portion of the security diff scanner to avoid flagging literal unsafe-git strings that appear in append-only Squad log/memory documents (e.g., decision logs) while keeping instruction-bearing files in scope.

Changes:

• Add UNSAFE_GIT_EXCLUDED_PATHS for known append-only log/memory paths.
• Skip the unsafe-git pattern scan for files matching those excluded paths.

File summaries

File	Description
scripts/security-review.mjs	Adds an exclusion list and short-circuits the unsafe-git scan for those paths.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[tamirdresher]

Reviewed the exclusion logic in scripts/security-review.mjs. The unsafe-git false-positive fix is correct and the nested-path regex issue flagged by the automated review is already resolved in this commit. One trade-off worth noting for the record: .squad/decisions.md and agent history.md files are loaded into every agent's context on spawn, so excluding them from the unsafe-git scanner does create a theoretical blind spot if a maliciously-crafted 'decision' entry contained an actual unsafe git instruction disguised as prose. Accepting this trade-off for now since these files are structurally append-only and reviewed via normal PR process before landing, and the false-positive cost of NOT excluding them was actively blocking legitimate PRs. Approving with this caveat on record; worth revisiting if the decision-inbox pipeline becomes more automated in the future.