my-pi/pi-permission-system/docs/plans/0304-consolidate-bash-command-analysis.md

issue, issue_title

issue	issue_title
304	Consolidate bash command analysis behind a single parsed representation and a candidate-combination helper

Consolidate bash command analysis

Problem Statement

Three concerns derive information from a bash command string, and each parses the command independently with tree-sitter:

• bash command-pattern matching — matches the whole command string (the bug tracked in #301).
• path rules — extractTokensForPathRules() walks the AST for path-candidate tokens.
• external_directory — extractExternalPathsFromBashCommand() walks the AST for paths resolving outside CWD (cd-aware).

There is no shared parsed representation, so adding a new bash-derived concern (such as the per-sub-command list #301 needs) means yet another standalone extractor with no guarantee the decompositions agree. Separately, the "evaluate several candidate values against a surface and combine the results" loop is copied across the two bash gates rather than abstracted: describeBashPathGate and describeBashExternalDirectoryGate each re-implement the same most-restrictive selection, and #301 would copy it a third time.

This is the "make the change easy, then make the easy change" prerequisite for #301. It is a behavior-preserving refactor: same decisions, same outputs, less duplication, and a representation that #301 can extend with one slice instead of a fourth parse and a fourth walker.

Goals

• Introduce a single parsed-bash value object (BashProgram) that parses once and exposes typed slices (pathTokens(), externalPaths(cwd)), so future bash-derived concerns add a method rather than a standalone extractor.
• Introduce a reusable most-restrictive selection helper (pickMostRestrictive) over PermissionCheckResults and migrate both bash gates' selection onto it.
• Keep behavior identical, verified by the existing suite staying green.
• Leave #301 as a roughly two-step change on top of this work.

Non-Goals

• No behavior change of any kind. The chain-evaluation fix lands in #301.
• Do not change PermissionManager.checkPermission(), PermissionsService, or the event-bus RPC.
• Do not parse-once-and-inject a shared BashProgram into the gates from the handler. That changes gate signatures and the gate pipeline — it belongs to the deferred gate-consolidation follow-up. This refactor keeps each gate's existing parse call; the win here is the representation and the selection helper, not parse-sharing across gates.
• Do not merge the rule-level combinators (evaluateFirst, evaluateMostRestrictive in rule.ts). Those operate on Rules one layer below; pickMostRestrictive operates on PermissionCheckResults at the gate layer.
• Do not touch bash-arity.ts, pattern-suggest.ts, the wildcard matcher, config schema, or docs/configuration.md (no config/behavior change).

Background

Relevant modules:

• src/handlers/gates/bash-path-extractor.ts — the tree-sitter-bash parser and AST walker. Private primitives getParser (lazy WASM init), resolveNodeText, collectPathCandidateTokens, extractLeadingCdTarget, computeEffectiveResolveBase; classification via classifyTokenAsPathCandidate (strict) and classifyTokenAsRuleCandidate (broad) from bash-token-classification.ts. Exports extractExternalPathsFromBashCommand(command, cwd) and extractTokensForPathRules(command). The two exports already share the walker; they differ only in classification and the external-path resolve/filter step.
• src/handlers/gates/bash-path.ts — describeBashPathGate: loops checkPermission("path", { path: token }) per token, with a #58 backward-compat filter (a token whose only match is the universal default is treated as unrestricted) and session-coverage detection, then keeps the most-restrictive uncovered result (deny short-circuit, then ask).
• src/handlers/gates/bash-external-directory.ts — describeBashExternalDirectoryGate: loops checkPermission("external_directory", { path }), filters to uncovered (state !== "allow"), then picks worstCheck = first deny ?? first uncovered.
• src/rule.ts — rule-level evaluate, evaluateFirst, evaluateMostRestrictive (out of scope; a layer below).
• test/bash-external-directory.test.ts — large (900+ line) suite that exercises extractExternalPathsFromBashCommand and extractTokensForPathRules directly. These exports must keep working unchanged (lift-and-shift: keep them as thin facades; do not rewrite this file).

Constraint from AGENTS.md / package skill that applies: behavior-preserving refactors must keep schema/example/docs aligned (no change needed here since behavior is unchanged), and extractions must have real consumers (no speculative exports — fallow will flag dead code).

Design Overview

BashProgram value object

One parse, two derived slices. The two existing extractors become thin facades over it (so the large extractor test suite stays green), and #301 later adds a topLevelCommands() method as a third slice.

// src/handlers/gates/bash-program.ts  (parse/walk primitives move here from bash-path-extractor.ts)

export class BashProgram {
  private constructor(
    private readonly rawTokens: string[],
    private readonly leadingCdTarget: string | undefined,
  ) {}

  /** Parse a bash command once into a reusable representation. */
  static async parse(command: string): Promise<BashProgram>;

  /** Broad path-candidate tokens for `path` rules (dot-files, relative paths). */
  pathTokens(): string[];

  /** Strict path candidates resolving outside `cwd` (cd-aware). */
  externalPaths(cwd: string): string[];

  // [#301] will add: topLevelCommands(): string[]
}

bash-path-extractor.ts keeps its public surface as facades:

export async function extractTokensForPathRules(command: string): Promise<string[]> {
  return (await BashProgram.parse(command)).pathTokens();
}
export async function extractExternalPathsFromBashCommand(
  command: string,
  cwd: string,
): Promise<string[]> {
  return (await BashProgram.parse(command)).externalPaths(cwd);
}

To avoid a circular import, the parse/walk primitives (getParser, resolveNodeText, collectPathCandidateTokens, extractLeadingCdTarget, computeEffectiveResolveBase, extractCommandName, findFirstCommand) move into bash-program.ts alongside BashProgram; bash-path-extractor.ts imports BashProgram and exposes the facades. The big test file's import path (#src/handlers/gates/bash-path-extractor) is unchanged.

pickMostRestrictive selection helper

The common core of both bash gates' selection — deny > ask > allow, first occurrence wins on ties — extracted as a pure function over results. The gates keep their surface-specific filters (session-coverage, #58 backward-compat, uncovered) and call the helper for the final pick.

// src/handlers/gates/candidate-check.ts

/** deny > ask > allow; first occurrence wins on ties. undefined for an empty list. */
export function pickMostRestrictive(
  results: readonly PermissionCheckResult[],
): PermissionCheckResult | undefined;

Fit check against each gate (confirms the seam is correct, not forced):

• External-directory gate: worstCheck = uncoveredEntries.find(deny)?.check ?? uncoveredEntries[0].check is exactly pickMostRestrictive(uncoveredEntries.map(e => e.check)). Clean drop-in.
• Path gate: keep the per-token loop that classifies each token (allow / session-covered / #58-unrestricted / uncovered) and the allSessionCovered bypass; replace the final worstCheck accumulation with pickMostRestrictive(uncoveredChecks). Behavior identical; the existing path-gate tests pin it. Note: the path gate currently short-circuits on the first deny; collecting uncovered results and then picking is output-identical (the picked deny is the same), at the cost of a few extra in-memory checkPermission calls — acceptable for a behavior-preserving refactor.

This gives pickMostRestrictive two consumers in this issue (both bash gates); #301 is the third.

Why both, honestly

pickMostRestrictive (#2) is the change that most directly removes duplication #301 would otherwise repeat. BashProgram (#1) centralizes the growing set of bash-derived slices into one cohesive object and is the seam #301 extends; its parse-sharing payoff is realized later by the deferred gate-consolidation work. The existing extractors already share the walker, so #1's near-term win is cohesion and extensibility rather than fewer parses.

Module-Level Changes

• src/handlers/gates/bash-program.ts — new module: BashProgram class plus the parse/walk primitives moved from bash-path-extractor.ts.
• src/handlers/gates/bash-path-extractor.ts — reduced to the two facade functions delegating to BashProgram; imports BashProgram. Public exports unchanged.
• src/handlers/gates/candidate-check.ts — new module: pickMostRestrictive.
• src/handlers/gates/bash-external-directory.ts — replace the worstCheck selection with pickMostRestrictive.
• src/handlers/gates/bash-path.ts — replace the worstCheck accumulation with pickMostRestrictive over the uncovered-token results; keep the #58 and session-coverage logic.
• docs/architecture/architecture.md — update the directory listing: add bash-program.ts (BashProgram) and candidate-check.ts (pickMostRestrictive), and revise the bash-path-extractor.ts entry to "facades over BashProgram". Review v3-architecture.md for the same.
• No changes to config schema, example config, docs/configuration.md, or README.md (behavior unchanged).

Test Impact Analysis

1. New unit tests enabled:
   • test/handlers/gates/bash-program.test.ts — BashProgram.parse().pathTokens() and .externalPaths(cwd) at the value-object level (a subset mirroring the existing extractor cases, plus that one parse yields both slices).
   • test/handlers/gates/candidate-check.test.ts — pickMostRestrictive: deny > ask > allow, first-wins on ties, empty → undefined.
2. Existing tests that stay as-is (the behavior-preservation guard):
   • test/bash-external-directory.test.ts — still exercises the facades; unchanged behavior.
   • test/handlers/gates/bash-path.test.ts, test/handlers/gates/bash-external-directory.test.ts — gate behavior unchanged; these are the migration's safety net.
3. No tests become redundant; new tests are additive at lower levels.

TDD / Refactor Order

Each step is behavior-preserving and leaves the full suite green.

1. refactor: extract pickMostRestrictive and use it in the bash external-directory gate
   • Add src/handlers/gates/candidate-check.ts with a new test/handlers/gates/candidate-check.test.ts.
   • Migrate describeBashExternalDirectoryGate's worstCheck onto it (its existing tests stay green).
   • Lands the helper with a real consumer immediately (no speculative export).
2. refactor: select most-restrictive bash path result via pickMostRestrictive
   • Refactor describeBashPathGate to collect uncovered-token results and call pickMostRestrictive, preserving the #58 backward-compat and session-coverage logic.
   • Run the path-gate suite and pnpm run check immediately (subtle logic).
3. refactor: introduce BashProgram and reduce extractors to facades
   • Add src/handlers/gates/bash-program.ts (BashProgram + moved primitives) with test/handlers/gates/bash-program.test.ts.
   • Reimplement extractTokensForPathRules / extractExternalPathsFromBashCommand as facades over BashProgram.
   • The large test/bash-external-directory.test.ts stays green unchanged.
   • Run pnpm run check (cross-module move).
4. docs: document BashProgram and the most-restrictive selection helper
   • Update docs/architecture/architecture.md (and v3-architecture.md if needed).
   • Docs-only commit.

After this issue ships, #301 becomes: add BashProgram.topLevelCommands(), add a bash command gate that evaluates each top-level command via checkPermission and selects with pickMostRestrictive, wire it into the tool-gate producer, and update docs/configuration.md.

Risks and Mitigations

• Moving the parse/walk primitives between modules is the largest single edit. Mitigation: it is a mechanical move with no logic change, gated by the unchanged extractor test suite and pnpm run check in step 3.
• The path-gate refactor touches subtle #58 and session-coverage logic. Mitigation: preserve the surrounding loop and filters; only the final selection moves to pickMostRestrictive; run the path-gate tests in the same step.
• Losing the path gate's deny short-circuit slightly changes work done (not output). Mitigation: acceptable for a behavior-preserving refactor; output is identical and inputs are small.

Open Questions

• Should bash-path-extractor.ts be renamed to reflect that it is now a thin facade layer (e.g. fold the facades into bash-program.ts and retire the file)? Deferred; renaming touches the large test file's import line and a few gate imports. Track and revisit if the facades lose value once #301 lands.
• Is parse-once-and-inject (a single BashProgram per tool_call shared by all bash gates) worth a follow-up alongside the gate-consolidation work? Deferred to that follow-up.