18 KiB
issue, issue_title
| issue | issue_title |
|---|---|
| 580 | pi-permission-system: shell-tool alias config model (shellTools) |
Shell-tool alias config model (shellTools)
Release Recommendation
Release: mid-batch — defer (batch "shell-tool-aliases"); confirm at ship time
This is Phase 11 Step 2 of the pi-permission-system improvement roadmap, tagged Release: batch "shell-tool-aliases".
The batch tail is Step 3 (#574) — the enforcement gate that consumes this config.
Step 2 delivers only the validated, merged, documented config surface with no runtime behavior change, so it ships together with Step 3, not on its own.
A feat: commit that lands here waits on main and auto-batches into the release cut when Step 3 lands.
Problem Statement
classifyToolKind decides "what does this invocation access?"
from a closed set of hardcoded built-in tool names.
A tool that carries bash semantics under a different name — e.g. @howaboua/pi-codex-conversion replaces the native bash tool with exec_command (cmd + optional workdir) — is classified as a generic extension tool, so it never receives command decomposition, wrapper flooring, bash path/external-directory token gates, or bash: config rules.
The same shell operation is then evaluated differently depending on which toolset is active.
The access-intent boundary has no way to record that a foreign tool name is really a shell. Config is the right home for that recording: config files are the source of truth for policy, and the project prefers config patterns over new runtime mechanisms.
This issue delivers the config surface only. Consuming it at gate time — routing an aliased invocation through the bash enforcement stack — is Phase 11 Step 3 (#574).
Goals
- Add an optional
shellToolsfield tounifiedConfigSchemamapping a tool name to{ commandArgument, workdirArgument? }, with.metadescriptions and strict fail-closed validation. - Regenerate
schemas/permissions.schema.jsonfrom the zod source viapnpm run gen:schema(never hand-edited); keep the parity test green. - Carry the field through
PermissionSystemExtensionConfig,normalizePermissionSystemConfig, andmergeUnifiedConfigs()so it is not silently dropped before runtime (the #332/#347 class; post-#356 the compiler flags the gap). - Merge
shellToolsshallowly by tool name across scopes: project entries add/override per tool name on top of global, never dropping a global entry wholesale. - Document the field in
config/config.example.json,docs/configuration.md, andREADME.md. - Not breaking:
shellToolsis a new optional field; existing configs are unaffected on upgrade.
Non-Goals
- No runtime behavior change.
Nothing reads
shellToolsyet —grep -c shellTools src/config-schema.tsgoes 0 → ≥ 1, but no gate consults it. Wiring the recording intoclassifyToolKind/ the tool-call gate pipeline is Step 3 (#574) and is deliberately deferred. - No tool-removal or toolset lever.
shellToolsonly ever tightens enforcement (routes a tool through the bash stack) and is inert when the tool is not registered. Opting a project out ofpi-codex-conversionis a package-disable / active-tools concern that Pi owns, not a permission-config field. - No per-agent frontmatter surface.
Per-agent frontmatter stays tolerant and carries only its
permissionblock;shellToolsis a file-config field, matching the other runtime knobs.
Background
Relevant existing modules and conventions (from the package-pi-permission-system skill and the code):
src/config-schema.ts— the single source of truth. Composable zod schemas drive both runtime validation and the generated JSON Schema (buildPermissionsJsonSchema).id-tagged sub-schemas (permissionState,permissionMap,denyWithReason) become$defs; everything else inlines.UnifiedPermissionConfigisz.infer<typeof unifiedConfigSchema>.schemas/permissions.schema.json— generated viapnpm run gen:schema(scripts/generate-permissions-schema.ts+biome format); never edited by hand. A parity test intest/config-schema.test.tsfails on drift. That test also asserts$defsis exactly["denyWithReason", "permissionMap", "permissionState"].src/extension-config.ts—PermissionSystemExtensionConfig(the runtime type) andnormalizePermissionSystemConfig(raw: UnifiedPermissionConfig), which reads fields directly off the typed parameter (so an omitted field is a compile error post-#356).DEFAULT_EXTENSION_CONFIGmust not carry an explicitundefinedoptional field — tests usedeepEqual.src/config-loader.ts—mergeUnifiedConfigs(base, override): boolean/number scalars replace, array fields replace,permissiondeep-shallow merges viamergeFlatPermissions.config/config.example.json,docs/configuration.md,README.md— kept aligned with the schema whenever the config shape changes.
Constraints from AGENTS.md / the package skill that apply:
- Config files are validated strictly against
unifiedConfigSchemaand rejected fail-closed on any invalid field.strictObjectat the alias level makes an unknown alias key an error. - A field on the runtime type but not the merge intermediate is silently dropped — carry it through all three sites.
- Keep
config-schema.ts, example config,docs/configuration.md, andREADME.mdaligned — the schema and config types both derive fromconfig-schema.ts, the one edit point. - Mark the completed roadmap step (
✅on Step 2's heading and its Mermaid node) in the implementation doc-update commit, not a deferred ship commit.
Design Overview
Config shape
// A single aliased shell tool's field mapping.
const shellToolAliasSchema = z.strictObject({
commandArgument: z.string().min(1).meta({
description:
"The input field holding the shell command string for this tool (e.g. 'cmd').",
}),
workdirArgument: z.string().min(1).optional().meta({
description:
"Optional input field holding the working directory for this tool (e.g. 'workdir').",
}),
});
// tool name -> alias mapping
const shellToolsSchema = z
.record(
z.string().min(1).meta({
description: "A non-bash tool name that carries shell semantics.",
}),
shellToolAliasSchema,
)
.meta({
description:
"Maps non-bash tool names that carry shell semantics to the input fields holding their command and working directory.",
markdownDescription:
"Records which non-`bash` tools carry shell semantics, mapping each tool name to the input field holding its command (and optionally its working directory).\n\nUse this when an extension replaces the native `bash` tool under a different name — e.g. `@howaboua/pi-codex-conversion` registers `exec_command` with a `cmd` field and an optional `workdir`. Recording the alias lets the permission system gate that tool through the same bash enforcement stack as native `bash` (command decomposition, wrapper flooring, path/external-directory token gates, and `bash:` rules).\n\nExample:\n\n```json\n\"shellTools\": {\n \"exec_command\": { \"commandArgument\": \"cmd\", \"workdirArgument\": \"workdir\" }\n}\n```\n\n**Merge order:** shallow-merge by tool name across global → project. A project entry overrides a specific tool's mapping on key collision but never drops a global entry.",
});
export type ShellToolAlias = z.infer<typeof shellToolAliasSchema>;
export type ShellToolsConfig = z.infer<typeof shellToolsSchema>;
Then add shellTools: shellToolsSchema.optional() to unifiedConfigSchema's strictObject.
Design notes:
- The alias sub-schema is not
id-tagged, so it inlines underproperties.shellTools.additionalPropertiesin the generated JSON Schema rather than becoming a fourth$def. This keeps the parity test's$defs === ["denyWithReason", "permissionMap", "permissionState"]assertion unchanged. strictObjectat the alias level fails closed on an unknown field (e.g. a typo'dcommandFeild), matching the rest of the config's strict validation.commandArgumentis required (a shell alias with no command field is meaningless);workdirArgumentis optional (a tool may not project a working directory).- Both field names are
.min(1)non-empty strings.
Merge semantics — shallow by tool name
shellTools is security-relevant: in Step 3 an entry is what routes a tool through the bash enforcement stack, so a dropped entry is a silent enforcement regression (the "silent bypass" class this package guards against).
Merge must therefore be additive: a project can override a specific tool's mapping but can never silently drop a global entry.
// In mergeUnifiedConfigs, alongside the permission deep-shallow merge:
const baseShell = base.shellTools;
const overrideShell = override.shellTools;
if (baseShell && overrideShell) {
merged.shellTools = { ...baseShell, ...overrideShell };
} else if (baseShell) {
merged.shellTools = baseShell;
} else if (overrideShell) {
merged.shellTools = overrideShell;
}
The spread replaces a colliding tool's alias object wholesale (no deep-merge of commandArgument/workdirArgument) — a project overriding exec_command supplies the full mapping, so it can never end up with a commandArgument and a stale global workdirArgument.
This mirrors the permission block's structure but one level shallower (a flat tool→alias record, not a nested pattern map).
Decision rationale (confirmed with the operator during planning):
- A project that wants a different field mapping for a tool sets that tool's key — shallow-merge replaces just that object.
- A project that wants no
pi-codex-conversiondisables the package; theexec_commandtool is then unregistered and anyshellToolsentry is inert. - A project that wants a tool gated loosely uses
bash:/path:rules, not un-recording the shell semantics. - The only capability "replace wholesale" adds over shallow-merge — "define one entry and silently drop all global entries" — has no legitimate use and is a footgun, so it is rejected.
Carry-through
normalizePermissionSystemConfig copies the optional field only when present (matching piInfrastructureReadPaths):
if (raw.shellTools !== undefined) {
result.shellTools = raw.shellTools;
}
PermissionSystemExtensionConfig gains shellTools?: ShellToolsConfig;.
DEFAULT_EXTENSION_CONFIG is untouched — the field stays absent (no explicit undefined), preserving deepEqual equality in tests.
Module-Level Changes
src/config-schema.ts— addshellToolAliasSchema+shellToolsSchema(with.meta), addshellTools: shellToolsSchema.optional()tounifiedConfigSchema, exportShellToolAliasandShellToolsConfigtypes.schemas/permissions.schema.json— regenerated viapnpm run gen:schema(do not hand-edit).src/extension-config.ts— addshellTools?: ShellToolsConfigtoPermissionSystemExtensionConfig; copy it innormalizePermissionSystemConfigwhen defined. ImportShellToolsConfigfromconfig-schema(or re-exported viaconfig-loader, matching the existingUnifiedPermissionConfigimport path).src/config-loader.ts— add the shallow-by-tool-name merge block inmergeUnifiedConfigs.test/config-schema.test.ts— new accept/reject cases (see TDD Order); the existing parity +$defsassertions stay green.test/config-loader.test.ts(or the merge test file) — new shallow-merge cases forshellTools.test/extension-config.test.ts(or wherevernormalizePermissionSystemConfigis tested) — carry-through case.config/config.example.json— add ashellToolsblock showingexec_command.docs/configuration.md— add ashellToolssubsection under Runtime Knobs and include it in the Full Example.README.md— add a one-line mention ofshellToolsin the Configuration section (pointer to the docs reference).docs/architecture/architecture.md— mark Phase 11 Step 2 complete (✅on the Step 2 heading and its Mermaid nodeS2); norule.ts-type listing is touched (this change adds a config field, not aRule/Rulesetfield).
Grep confirmation performed during planning: shellTools / ShellTool appears nowhere in src/ today, so no existing symbol collides.
The health-metric row (shellTools schema sites 0 → ≥ 1, line 875 of architecture.md) is a Phase 11 target, satisfied by this step; leave the target table as written (it tracks the phase, not per-step baselines).
Test Impact Analysis
This is an additive config-surface change, not an extraction, so the extraction-specific questions are largely N/A:
- New tests enabled — schema accept/reject for the
shellToolsshape,mergeUnifiedConfigsshallow-merge behavior, andnormalizePermissionSystemConfigcarry-through. All are new unit tests over existing seams; nothing was previously untestable. - Redundant tests — none.
No existing test covers
shellTools(the field is new). - Tests that must stay — the parity test (
committed schemas/permissions.schema.json is in sync) and the$defsassertion genuinely guard schema drift and must stay; the design deliberately keeps$defsat three entries so the latter stays green without edit.
Invariants at risk
This change touches config-schema.ts, extension-config.ts, and config-loader.ts — surfaces the #356 carry-through hardening and the #547 strict-validation / schema-parity work already refactored.
- #356 carry-through invariant — a runtime-type field must be readable from the typed
UnifiedPermissionConfig, so an omitted merge/normalize site is a compile error. Pinned by the type-level testinferred types match the hand-written domain typesand bytsc; addingshellToolsexercises exactly this path. - #547 schema-parity invariant — the committed JSON Schema equals
buildPermissionsJsonSchema(). Pinned bycommitted schemas/permissions.schema.json is in sync; regenerating the schema in the same commit keeps it green. - #547
$defsshape invariant — exactly three shared sub-schemas. Pinned byextracts the shared sub-schemas into $defs; the design keeps the alias sub-schema un-id-tagged so this stays green.
No earlier phase step's documented Outcome: invariant is regressed — this step only adds an optional field.
TDD Order
-
Schema surface (
test: add shellTools schema cases→feat(pi-permission-system): add shellTools config schema).- Red: in
test/config-schema.test.ts, add cases — accepts a config withshellTools: { exec_command: { commandArgument: "cmd", workdirArgument: "workdir" } }; accepts an alias with onlycommandArgument; rejects an alias missingcommandArgument; rejects an unknown field inside an alias (strictObject); rejects a non-stringcommandArgument. - Green: add
shellToolAliasSchema+shellToolsSchema+ the optional field + exported types toconfig-schema.ts; runpnpm run gen:schemato regenerate the committed JSON (the parity test then passes). - Verify:
pnpm run check, the new + existing config-schema tests, and$defsstill equals the three entries. - Commit the schema source, regenerated
schemas/permissions.schema.json, and the test together (feat:).
- Red: in
-
Runtime carry-through + merge (
feat(pi-permission-system): carry shellTools through config merge).- Red: add a
normalizePermissionSystemConfigcarry-through test (field copied when present, absent fromDEFAULT_EXTENSION_CONFIG) andmergeUnifiedConfigsshallow-merge tests — global-only survives, project-only survives, project overrides a colliding tool key, project adds a new tool without dropping the global entry. - Green: add
shellTools?: ShellToolsConfigtoPermissionSystemExtensionConfig, theif (raw.shellTools !== undefined)copy innormalizePermissionSystemConfig, and the shallow-merge block inmergeUnifiedConfigs. - Verify:
pnpm run check,pnpm -r run testfor the package. - Note: because
normalizePermissionSystemConfigreads the typed field, the compiler enforces the carry-through — a missed site failstsc.
- Red: add a
-
Docs + example + roadmap (
docs(pi-permission-system): document shellTools config).- Update
config/config.example.json(add theexec_commandshellToolsblock),docs/configuration.md(ashellToolssubsection + Full Example entry), andREADME.md(one-line mention). - Mark Phase 11 Step 2 complete in
docs/architecture/architecture.md(✅on the Step 2 heading and Mermaid nodeS2) in this same commit. - Verify:
pnpm exec rumdl checkon the edited markdown; confirmconfig.example.jsonstill parses and validates against the schema. docs:type is ahidden: truechangelog entry that does not cut a release on its own — correct for this deferred batch member.
- Update
Risks and Mitigations
- Schema
$defsdrift breaks the parity/$defstest — mitigated by leaving the alias sub-schema un-id-tagged (inlines) and regenerating the JSON in step 1; the parity test is the guard. - Silent field drop before runtime (#332/#347 class) — mitigated by the compile-time carry-through (#356) plus explicit merge/normalize tests in step 2.
- Merge choice locks in Step 3 runtime behavior — the shallow-by-tool-name decision is deliberate and operator-confirmed; documented here and in the
markdownDescriptionso Step 3 consumes a known, additive contract. - Example config that fails validation — mitigated by the step-3 verify that
config.example.jsonparses and validates against the regenerated schema.
Open Questions
None outstanding.
The one design ambiguity (merge semantics) was resolved to shallow-merge-by-tool-name during planning.
Step 3 (#574) owns all consumption-time questions (which dispatch point consults the alias, workdir as effective base, review-log shape) — deferred by design.