fix(tools): trim command before matchCommand to close leading-whitespace bypass (#904)

[ausard]

Summary

Follow-up to #909, which layered exec-flag escapes on the read-only bash allowlist. That fix used the full command string as the regex target, but did not trim it first — so a stray leading whitespace character ( find . -exec ..., \tfind ...) would silently defeat the anchored ^find.*-exec.*$ pattern and re-open the escape.

This PR trims the command before both first-token extraction and full-command regex matching, and adds a regression test covering leading spaces, trailing spaces, and a leading tab. All prior tests remain green.

Diff scope

• src/permission/wildcard.ts: matchCommand now trims the incoming command and uses the trimmed value for both cmdName and the full-command regex test.
• tests/permission/wildcard.test.ts: adds a leading/trailing whitespace regression case.

Rebased on top of current master; the earlier duplicate of #909 is dropped.

Checks

• typecheck: pass
• lint: pass
• format: pass
• tests: 206 files / 2848 pass, 2 skipped
• build: (unchanged surface)

Closes #904

[ausard]

Security Review - PR #910

fix(tools): trim command before matchCommand to close leading-whitespace bypass (#904)

Scan performed by the Security vertical of the Alexi T-shape agent factory.
Trigger: pull_request. Base: origin/master. Head: auto/904-tool-layer-exec-flag-escapes-on-the-read-only-bash.

Summary

This PR narrows a real gap in the permission allowlist: matchCommand() in
src/permission/wildcard.ts was using the raw command string for both the
first-token fast path AND the full-command regex path, so a stray leading
space (or tab) in the incoming command silently caused a full-pattern like
find *-exec* to miss and fall through. The fix trims once and uses the
trimmed value for both branches. A regression test covers leading spaces,
trailing spaces, and a leading tab.

Verdict: safe to merge from a security standpoint. Change is a
hardening, not a weakening, of the permission gate. Wildcard tests pass
locally (23/23).

Findings below are context on the broader repo audit; none block this
PR. The relevant npm audit items are all pre-existing on master and
unrelated to the diff.

Critical (block)

None for this PR.

High (warn)

Pre-existing, not introduced by this PR. Logged here so they are not lost:

• xlsx@^0.18.5 (declared in package.json) has two open advisories
  (GHSA-4r6h-8v6p-xvw6 prototype pollution, GHSA-5pgg-2g8v-p4x9 ReDoS)
  with fixAvailable=false. xlsx does not appear to be imported anywhere
  under src/. Recommend a separate chore(deps) PR to either drop the
  dependency or pin the SheetJS CDN build per their advisory notes.
• npm audit --omit=dev currently reports 8 high / 3 moderate transitive
  advisories (@hono/node-server, @xmldom/xmldom, axios,
  express-rate-limit, fast-uri, form-data, path-to-regexp, xlsx).
  All except xlsx have fixAvailable=true via Dependabot; that queue
  should be worked in a dedicated PR, not folded into this one.

Info

• Permission-gate direction check. The change strengthens
  matchCommand() by ensuring exec-flag escape patterns (e.g.
  find *-exec*) cannot be bypassed with leading whitespace. It does NOT
  add any new --auto-style bypass, does NOT remove a confirmation
  prompt, and does NOT alter defaultPermission. Aligns with the
  AGENTS.md rule that --dangerously-skip-permissions stays opt-in.
• marked version. package.json pins marked@^15.0.12. This is
  intentional: marked@>=16 is a known incompatibility with our
  marked-terminal rendering path and is treated as block severity in
  this repo. Diff does not touch this; noted for future Dependabot
  triage.
• Secret scan. git diff origin/master...HEAD shows zero matches
  for AICORE_SERVICE_KEY, clientsecret, BEGIN PRIVATE KEY, xoxb-,
  ghp_, sk-*, base64 blobs >=200 chars, or password[:=].
• Workflow / CI hygiene. Diff does not touch .github/**,
  package.json, or package-lock.json. No workflow, action pin, or
  install-script surface change to review.
• Test coverage of the fix. Three regression cases added
  (tests/permission/wildcard.test.ts:104-111) covering leading space,
  trailing space, and leading tab. Meets the DoD requirement that
  permission-system changes ship with matching tests under
  tests/permission*.test.ts.

Recommendations

1. Merge this PR on green CI. It is a targeted permission-gate hardening.
2. Open a separate chore(deps) issue for the xlsx advisory: decide
   between dropping the unused dependency or replacing with a maintained
   fork. Label security.
3. Continue routine Dependabot triage for the seven other high-severity
   transitive advisories; none are exploitable through this diff.

---

Posted by role-security via agent-factory. Advisory only - the agent
does not push to this PR branch (commit_changes: false).

[github-actions]

Coverage Report

Metric	Coverage
Lines	62.83%
Statements	62.83%
Functions	58.37%
Branches	57.7%

Coverage Details

• Lines: 7761/12351
• Statements: 8073/12848
• Functions: 1206/2066
• Branches: 4389/7606

[ausard]

Architecture Review -- PR #910 (2026-07-03)

• Trigger: pull_request on refs/pull/910/merge
• Head: 90f1775c (fix(tools): trim command before matchCommand to close leading-whitespace bypass [alexi-bot])
• Base: master
• Files changed in this PR (excluding review artifacts): 2 (src/permission/wildcard.ts +5/-2, tests/permission/wildcard.test.ts +8/-0)
• Previous scheduled review: docs/adr/REVIEW-2026-06-29.md

Summary

Verdict on the PR itself: architecturally clean, merge unblocked. The
change is a two-line defensive .trim() inside matchCommand
(src/permission/wildcard.ts:154) plus three regression tests. It stays
inside its own module, does not cross any layer boundary, and does not
add a dependency or a new top-level directory. Reviewing solely on
architecture grounds: LGTM.

Verdict on the repository state the PR lands into: still degrading.
The 2026-06-29 review left seven block-severity or unresolved findings
(F1, F2, F5, F6, F10, F11, F12, F13). Between then and this PR, two more
upstream-sync PRs merged (#902 on 2026-07-02, #908 on 2026-07-03). #908
introduced a new instance of the F13 anti-pattern at ~4x the previous
scale -- fourteen empty comment-only stub files landed directly inside
src/tool/, plus five Kotlin files. These files shadow the real tool
implementations under src/tool/tools/ by filename, are not imported by
anything, and were merged without an ADR. This is F15 below and it
is the same recurring failure mode the 2026-06-29 review flagged in
Recommendation 2 (sync-allowlist guardrail). That recommendation is
still not implemented.

The PR under review actually depends on src/tool/tools/bash.ts being
the real bash tool, while src/tool/bash.ts (a 1-line comment file
added by the upstream sync) sits directly next to it. That is exactly
the "two-routers-same-name" trap ADR 001 was written to prevent, and
this time the collision is inside the module the fix operates on.

No new ADR in this run. F15 violates ADR 001's existing rule about
new top-level src/ directories (or, more precisely, its intent about
foreign-code intrusions). The right output is a PR review comment
flagging the state of src/tool/ and re-raising the guardrail
recommendation.

Findings

PR-scoped

PR-F1 -- info -- The two-line fix is layering-safe

src/permission/wildcard.ts:157 adds a .trim() and reuses trimmed
for both the fast-path cmdName split and the full-command regex
test. Both call sites stay within src/permission/. No new import,
no new export, no cross-package reach.

The behaviour change is limited to defence against a leading /
trailing whitespace bypass of full-command patterns (e.g.
find *-exec* was previously not matching " find . -exec ...").
This is a permission-hardening fix, not a semantic change to the
allowlist model.

PR-F2 -- info -- Test coverage matches the change

tests/permission/wildcard.test.ts:104-111 adds three regression
cases: leading space, trailing space, leading tab. Symmetric with the
.trim() call, no orphan expectations.

Repository-scoped (carried forward from REVIEW-2026-06-29)

Re-verified at HEAD 90f1775c. All still present, byte-identical
unless noted:

• F1 block: ADR deficit growing. docs/adr/ still contains only
  ADR 001 plus five REVIEW-*.md files; no ADR 002 backfill. Twenty
  top-level src/ directories still lack ADR justification.
• F2 block: src/cli/commands/models.ts:6 still imports
  DeploymentApi from @sap-ai-sdk/ai-api. Constitution III
  violation, unchanged.

  $ grep -rEn "from ['\"]@sap-ai-sdk" src/ --include="*.ts"
  src/providers/sapOrchestration.ts:35:} from '@sap-ai-sdk/orchestration';
  src/cli/commands/models.ts:6:import { DeploymentApi } from '@sap-ai-sdk/ai-api';

• F3 / F3b warn: unchanged. src/core/agenticChat.ts:19 still
  reads import type { CompletionResult, TokenUsage } from '../providers/sapOrchestration.js';. src/core/streamingOrchestrator.ts:16
  still imports buildSessionHeaders from
  '../providers/sessionHeaders.js'. The provider-surface re-exports
  landed but callers were not migrated.
• F4 warn: All six src/tool/tools/*.ts -> src/agent/
  inversions unchanged (edit, read, multiedit, write, grep, task).
• F5 warn: src/permission/PermissionView.kt and
  PermissionViewTest.kt still present. Fourth review in a row.
• F6 warn: src/core/migration/ SQL artefacts unchanged.
• F7 info: .github/prompts/role-architecture.md module map
  still ~10 modules; reality is 35 top-level dirs.
• F8 info: ESM .js import discipline remains clean outside
  test-string literals.
• F9 info: single cross-layer import in
  src/plugin/__tests__/ruleCommand.test.ts:47 unchanged.
• F10 block: src/core/billing.ts still present, 10 lines,
  zero callers.
• F11 block: src/core/catalog.ts still present, 152 lines,
  ModelCapability name collision with src/core/router.ts:12
  persists.
• F12 block: src/core/integration/schema.ts still present,
  single comment line, no code.
• F13 block: src/router/index.ts, src/session/data.ts,
  src/ui/timers.ts still present, still one-comment stubs, still
  zero consumers.
• F14 info: sync-allowlist guardrail still unfiled. F15 (below)
  is the direct consequence.

F15 -- block -- 2026-07-03 sync landed 14 stub .ts + 5 .kt files inside src/tool/ (NEW)

Commit 25fb3f6e (feat(sync): apply upstream changes (2026-07-03))
introduced a large set of foreign files directly at the top level of
src/tool/. Every one of the stub .ts files is a single-line
comment with no exports:

$ for f in src/tool/*.ts; do lines=$(wc -l <"$f"); bytes=$(wc -c <"$f"); if [ "$lines" -le 3 ] && [ "$bytes" -lt 200 ]; then echo "$lines lines / $bytes bytes  $f"; fi; done
1 lines / 96 bytes  src/tool/application-tools.ts
1 lines / 35 bytes  src/tool/apply-patch.ts
1 lines / 27 bytes  src/tool/bash.ts
1 lines / 90 bytes  src/tool/builtins.ts
1 lines / 27 bytes  src/tool/edit.ts
1 lines / 27 bytes  src/tool/glob.ts
1 lines / 27 bytes  src/tool/grep.ts
1 lines / 35 bytes  src/tool/native.ts
1 lines / 31 bytes  src/tool/question.ts
1 lines / 33 bytes  src/tool/read-filesystem.ts
1 lines / 27 bytes  src/tool/read.ts
1 lines / 28 bytes  src/tool/skill.ts
1 lines / 32 bytes  src/tool/todowrite.ts
1 lines / 35 bytes  src/tool/warpgrep.ts

None of them is imported from anywhere in src/ or tests/:

$ grep -rln "from ['\"].*/tool/\(bash\|apply-patch\|edit\|glob\|grep\|read\|read-filesystem\|skill\|todowrite\|question\|warpgrep\|native\|application-tools\|builtins\)['\"]" src/ tests/
# (no output)

Additionally, five Kotlin files landed under src/tool/
(KiloCliParserTest.kt, ReadToolView.kt, ShellToolView.kt,
ToolSupport.kt, ToolView.kt, totalling 66 lines). Their content
references package ai.kilocode.client.session.views.tool and
JetBrains-style class hierarchies. They cannot be compiled by
TypeScript, are not referenced by tsconfig.json or by any
.ts / .tsx file, and TypeScript's include glob does not pick
them up. They are dead weight at best and misleading at worst.

This is severity block for four compounding reasons:

1. Filename collision with the real implementations. For nine of
   the stubs (bash, apply-patch, edit, glob, grep, read,
   skill, todowrite, question), a real implementation with the
   same base name lives one directory deeper at
   src/tool/tools/<name>.ts. A developer skimming src/tool/
   will hit the empty stub first and be actively misled.
2. The current PR (fix(tools): trim command before matchCommand to close leading-whitespace bypass (#904) #910) is a security fix in this exact module
   tree. src/tool/bash.ts is now a 1-line comment file directly
   next to the real bash implementation at src/tool/tools/bash.ts.
   A future security-hardening PR that opens src/tool/bash.ts in an
   editor will land on the wrong file. That is not a theoretical
   risk -- it is the same trap ADR 001 called out.
3. Foreign-code smell, same shape as F10/F11/F12/F13.
   application-tools.ts (// application tool stub) and
   builtins.ts (// updated builtins) use the same "based on
   upstream" / "updated" phrasing pattern that has recurred through
   every prior review's block findings. The five Kotlin files
   further confirm this: they are IntelliJ-plugin code from a
   different repository, mass-imported without filtering.
4. ADR 001 intent violation. ADR 001 requires ADRs for new
   top-level src/ dirs and for changes to the provider surface.
   The intent -- that structural additions to src/ are reviewed --
   is violated by a bulk drop of 19 files into an existing directory
   without any accompanying decision record. src/tool/ grew from
   ~15 files to ~29 files (nearly doubling) in a single upstream
   sync PR with no architectural gate.

Resolution: extend the cleanup PR proposed in the 2026-06-29 review's
Recommendation 1 to also git rm src/tool/{application-tools,apply-patch,bash,builtins,edit,glob,grep,native,question,read,read-filesystem,skill,todowrite,warpgrep}.ts src/tool/*.kt.
Confirm nothing broke via npm run typecheck && npm run lint && npm run build.
Expected diff: purely deletions.

F16 -- info -- docs/ARCHITECTURE.md diagram has been stale since F13 landed (NEW-ish)

The mermaid diagram in docs/ARCHITECTURE.md:36-47 labels tool
files as Bash[bash.ts], Read[read.ts], etc. -- referring to the
top-level src/tool/*.ts filenames that (a) never actually existed
as the real implementation and (b) as of F15 now exist as empty
stubs. The real implementations live at src/tool/tools/<name>.ts.
The diagram was already a fiction before F15 and now it is a fiction
that happens to have matching file paths on disk pointing at wrong
files. This deserves a dedicated docs(core) follow-up PR after the
F15 cleanup lands.

Recommendations

Priorities re-ordered to reflect the growing sync-leak problem. These
are additions or promotions relative to REVIEW-2026-06-29 rather than
a full rewrite.

1. Merge PR fix(tools): trim command before matchCommand to close leading-whitespace bypass (#904) #910 as-is. The permission fix is small, correct,
   layer-safe, and tested. It should not be blocked on the wider
   src/tool/ cleanup.

2. Combined cleanup PR (chore(sync)) -- extended for F15. Add
   to the deletion set from the 2026-06-29 review:

   # Previously proposed
   git rm src/core/billing.ts                       # F10
   git rm src/core/catalog.ts                       # F11
   git rm -r src/core/integration                   # F12
   git rm -r src/router src/session src/ui          # F13
   
   # New in this review
   git rm src/tool/application-tools.ts \
          src/tool/apply-patch.ts \
          src/tool/bash.ts \
          src/tool/builtins.ts \
          src/tool/edit.ts \
          src/tool/glob.ts \
          src/tool/grep.ts \
          src/tool/native.ts \
          src/tool/question.ts \
          src/tool/read-filesystem.ts \
          src/tool/read.ts \
          src/tool/skill.ts \
          src/tool/todowrite.ts \
          src/tool/warpgrep.ts                       # F15 (.ts stubs)
   git rm src/tool/*.kt                              # F15 (Kotlin)

   Then npm run typecheck && npm run lint && npm run build. Expected
   diff: ~25 file deletions, zero source edits.

3. Sync-allowlist guardrail -- now urgent (role-infrastructure).
   Recommendation 2 of REVIEW-2026-06-29 is directly re-raised. Since
   that review, one more sync cycle has occurred and it produced F15
   at greater volume than F10-F13 combined. The guardrail should
   at minimum reject a sync PR when any of the following hold:

   • New top-level src/<name>/ dir without a matching
     docs/adr/NNN-*.md in the same diff.
   • Any file matching payment*, billing*, *credentials*,
     *stripe*.
   • Any file with <200 bytes containing only a comment outside
     docs/ and .github/.
   • Any .kt, .kts, .java, .swift, .gradle, .py, .rs
     file anywhere under src/ (this repo is single-language
     TypeScript ESM).

   Without this, F17/F18/... will keep arriving on a fortnightly
   cadence.

4. Finish the F3 / F3b migration. Unchanged from 2026-06-29
   Recommendation 3.

5. Delete the Kotlin permission files (F5). Unchanged; now bundled
   naturally with Recommendation 2's git rm src/tool/*.kt.

6. F6 -- migration directory decision. Unchanged.

7. F7 -- update role-architecture.md module map. Unchanged.

8. F16 -- refresh docs/ARCHITECTURE.md. After Recommendation 2
   lands, open a follow-up docs(core) PR that relabels the tool
   nodes in the mermaid diagram to point at src/tool/tools/*.ts
   (their real location) and updates the module list to reflect the
   post-cleanup top-level set.

ADR needed?

No new ADR in this run. F15, like F13 before it, is a violation of
existing ADR 001 rules rather than a new rule category. Writing an
ADR titled "Foreign files in existing src/ dirs" would just paraphrase
ADR 001's intent.

Standing ADR backlog is unchanged from REVIEW-2026-06-29:

• ADR 002 -- backfill baseline (after cleanup PR lands).
• ADR 003..005 as listed in ADR 001 follow-ups.

If the sync-allowlist guardrail is implemented (Recommendation 3), it
should ship with ADR 006 -- Sync intake policy, documenting which
paths and file types are accepted from upstream, the rejection
criteria, and the escalation flow when a sync legitimately needs a
new top-level module.

PR #910 disposition

• Architecture review verdict on this PR: approve on architecture
  grounds.
• The permission-hardening change is a two-line .trim(), correctly
  scoped, correctly tested, and does not interact with any layering
  concern. The wider src/tool/ state issue (F15) is a separate
  problem and should not be used to block this fix.

Filed by role-architecture running under agent-factory.yml.

[ausard]

Review - role-quality

• typecheck
• lint
• format
• tests (Lines: 62.92%, well above the 40% CI floor)
• build

Correctness

Fix addresses the exact Done when from #904: a leading whitespace character used to defeat the anchored ^find.*-exec.*$ full-command regex in src/permission/wildcard.ts:154. Trimming the command before both first-token extraction and regex matching closes the bypass. Regression tests cover leading spaces, trailing spaces, and a leading tab (tests/permission/wildcard.test.ts:105-111).

Security

Positive security fix — closes a permission-allowlist bypass on the read-only bash rail. No new risks introduced; trimming is a strict narrowing of what the regex sees. commandPatternToRegex still anchors on ^…$, so the trimmed value cannot accidentally admit trailing shell metacharacters.

Tests

• Mocks not required (pure permission function).
• Regression cases include leading space, trailing space, and tab — matches the \s+ split intent.
• No temp-dir setup needed; no snapshot noise.
• All 23 tests in wildcard.test.ts pass locally.

Performance

.trim() on the command string is O(n) and only runs on tool-permission check. No hot-path concern.

Style

• .js-suffixed imports respected (no local imports touched).
• No any, @ts-ignore, or eslint-disable.
• Comment explains why (regression), not just what — good.

Changes made: none (gate is green, no fix-ups required).

Verdict: Approved