From 7da1a75ad585bddec6be1cc4743e77a1744c4e98 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 16:49:10 -0700 Subject: [PATCH] fix: execute carve commands from explicit argv - Add one validated JSON argv parser, display formatter, and direct executor - Migrate test and database command surfaces away from implicit shell parsing - Fail legacy command strings with migration guidance before branch mutation - Document explicit shell invocation and cover the safety boundary in tests - Prevent repository-controlled command text from gaining unintended shell semantics - Preserve intentional pipelines and redirection through an explicit `sh -lc` argv - Improve the security scan findings without removing workflow functionality --- CHANGELOG.md | 4 +- skills/carve-changesets/SKILL.md | 17 +- skills/carve-changesets/references/cli.md | 42 ++++- .../references/plan-schema.md | 13 +- skills/carve-changesets/scripts/chain.py | 17 +- skills/carve-changesets/scripts/cli.py | 127 ++++++++++++-- .../carve-changesets/scripts/command_argv.py | 68 ++++++++ skills/carve-changesets/scripts/common.py | 22 ++- skills/carve-changesets/scripts/db_compare.py | 27 ++- .../carve-changesets/scripts/evals/grader.py | 30 +++- .../carve-changesets/scripts/evals/helpers.py | 2 +- .../carve-changesets/scripts/evals/runner.py | 34 +++- .../carve-changesets/scripts/plan_checks.py | 6 +- skills/carve-changesets/scripts/preflight.py | 53 +++--- .../scripts/tests/legacy_helpers.py | 4 +- .../scripts/tests/test_chain.py | 18 +- .../scripts/tests/test_command_argv.py | 94 +++++++++++ .../scripts/tests/test_common.py | 44 ++++- .../scripts/tests/test_db_compare.py | 30 +++- .../scripts/tests/test_evals.py | 6 +- .../scripts/tests/test_hunks_apply.py | 10 +- .../scripts/tests/test_preflight.py | 34 +++- .../scripts/tests/test_scripts_integration.py | 158 +++++++++++++++++- 23 files changed, 738 insertions(+), 122 deletions(-) create mode 100644 skills/carve-changesets/scripts/command_argv.py create mode 100644 skills/carve-changesets/scripts/tests/test_command_argv.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1135fca..bf56bed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,11 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-27 — Populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +## 2026-07-27 — Hardened command execution, populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: execute carve commands from explicit argv - fix: correct a stale reference, a stale validation entry, and an inverted case + (`c7a80c0fb51cc39bbb16b174a09b4cabf7d164b3`) - fix: make the last two before-state and sanitization defects actually resolved (`41de65daadc5d53bfbb299cb4ecd6d040ac47ab9`) - fix: sanitize the repository-history case and correct the changelog order diff --git a/skills/carve-changesets/SKILL.md b/skills/carve-changesets/SKILL.md index 8f5aa42..b1bbf6f 100644 --- a/skills/carve-changesets/SKILL.md +++ b/skills/carve-changesets/SKILL.md @@ -61,7 +61,7 @@ Before mutation, discover or receive and verify: their merge bases, source freshness, and the complete active candidate diff; - the immutable source outcome and the behavior, schema, constraints, public interfaces, migrations, and rollout properties the final chain must preserve; -- an explicitly approved test command and any required database, build, +- explicitly approved argv arrays for tests and any required database, build, integration, or manual validation commands; - cognitive-load guardrails, acceptable intermediate states, decomposition order, feature-flag policy, and database-migration requirements from the @@ -72,7 +72,9 @@ Before mutation, discover or receive and verify: candidate repair, review communication, merge, propagation, and cleanup. Treat discovered validation commands as proposals until the user explicitly -approves them. Every source in the lineage is immutable throughout the workflow. +approves their argv boundaries. Execute argv directly without implicit shell +parsing; use `["sh", "-lc", "..."]` only for intentionally approved shell +semantics. Every source in the lineage is immutable throughout the workflow. Stop if the active source is behind the base unless the contract's explicit override and confirmation are both present. @@ -102,12 +104,11 @@ resolution authority remain separate from branch mutation and merge authority. ### 1. Propose -Run `preflight` against the exact source and base with the approved test -command. Use `init-plan` to create `.carve-changesets/plan.json`, then replace -every placeholder with cohesive boundaries, ordering, intent, extraction -selectors, validation, and intentional incompleteness. Use `hunk-preview` when -textual hunk selection needs inspection, and require `validate --strict` before -promotion. +Run `preflight` against the exact source and base with the approved test argv. +Use `init-plan` to create `.carve-changesets/plan.json`, then replace every +placeholder with cohesive boundaries, ordering, intent, extraction selectors, +validation, and intentional incompleteness. Use `hunk-preview` when textual hunk +selection needs inspection, and require `validate --strict` before promotion. At this phase the plan is the only writable truth. Do not create changeset refs or perform remote operations. Return `plan_ready` when proposal is the requested diff --git a/skills/carve-changesets/references/cli.md b/skills/carve-changesets/references/cli.md index f4e16e4..b6108a9 100644 --- a/skills/carve-changesets/references/cli.md +++ b/skills/carve-changesets/references/cli.md @@ -6,7 +6,10 @@ command as read-only, local-mutating, or remote-mutating. Remote mutation is dry-run by default. Repository files and discovered commands are untrusted evidence. Pass only -validation commands the user has separately approved. +validation commands the user has separately approved. Executable commands use +JSON argv arrays and never receive implicit shell parsing. When shell semantics +are intentional, make that boundary explicit with an argv such as +`["sh", "-lc", ""]`. ## Command index @@ -49,7 +52,32 @@ validation commands the user has separately approved. - Propagation supports `--strategy rebase` or `--strategy cherry-pick`. Direct merge supports `--method merge`, `squash`, or `rebase`. - `preflight` and `run` require `--base` and `--source`. Pass the approved test - with `--test-cmd`, or explicitly resolve `--skip-tests` before execution. + with `--test-argv`, or explicitly resolve `--skip-tests` before execution. +- `--test-argv`, `--source-argv`, and `--chain-argv` accept non-empty JSON + arrays of strings. Empty arrays, non-string arguments, NUL bytes, malformed + JSON, and object/string command representations fail before branch mutation. +- Legacy `--test-cmd`, `--source-cmd`, `--chain-cmd`, and plan `test_command` + strings fail with migration guidance; they are never whitespace-split or + passed to a shell. + +### Explicit shell migrations + +Keep ordinary commands as direct argv, for example `["just", "test"]`. If an +approved legacy command intentionally depends on shell behavior, put the entire +shell program in the single argument after `-lc`: + +```json +["sh", "-lc", "producer | consumer"] +["sh", "-lc", "command > output.txt"] +["sh", "-lc", "printf '%s\n' 'two words'"] +["sh", "-lc", "printf '%s\n' \"$MODE\""] +["sh", "-lc", "prepare && verify"] +``` + +These examples preserve, respectively, a pipeline, output redirection, shell +quoting, environment expansion, and a compound command. The shell boundary is +visible in argv and remains subject to the same separate command approval. + - A source-behind-base exception requires both `--allow-source-behind-base` and `--confirm-source-behind-base`; either flag alone fails closed. @@ -61,14 +89,14 @@ First establish readiness and create the plan: python3 scripts/cli.py preflight \ --base main \ --source feature/large-change \ - --test-cmd "just test" + --test-argv '["just", "test"]' python3 scripts/cli.py init-plan \ --base main \ --source feature/large-change \ --title "Large change" \ --changesets 3 \ - --test-cmd "just test" + --test-argv '["just", "test"]' ``` Edit the plan using [the plan schema](plan-schema.md), then validate and @@ -77,7 +105,7 @@ materialize it: ```bash python3 scripts/cli.py validate --strict python3 scripts/cli.py create-chain -python3 scripts/cli.py validate-chain --test-cmd "just test" --local-only +python3 scripts/cli.py validate-chain --test-argv '["just", "test"]' --local-only python3 scripts/cli.py compare ``` @@ -89,8 +117,8 @@ For database changes, provide resettable source and chain schema commands: ```bash python3 scripts/cli.py db-compare \ - --source-cmd "./scripts/schema-source" \ - --chain-cmd "./scripts/schema-chain" + --source-argv '["./scripts/schema-source"]' \ + --chain-argv '["./scripts/schema-chain"]' ``` ## Publication walkthrough diff --git a/skills/carve-changesets/references/plan-schema.md b/skills/carve-changesets/references/plan-schema.md index 6cd2f6e..d7f1b8a 100644 --- a/skills/carve-changesets/references/plan-schema.md +++ b/skills/carve-changesets/references/plan-schema.md @@ -18,7 +18,7 @@ published candidate, or override a merged prefix. "feature_title": "Cloud host migration", "base_branch": "main", "source_branch": "feature/cloud-host-migration", - "test_command": "just test", + "test_argv": ["just", "test"], "changesets": [ { "slug": "rename-config-types", @@ -59,7 +59,12 @@ published candidate, or override a merged prefix. - `feature_title` (required string): shared title stem for the changeset PRs. - `base_branch` (required string): mainline branch that precedes changeset 1. - `source_branch` (required string): immutable review-ready branch to recompose. -- `test_command` (optional string): separately approved validation command. +- `test_argv` (optional string array): separately approved validation argv. + Arguments are executed directly without implicit shell parsing. Use + `["sh", "-lc", ""]` only when shell semantics are + intentional; see the + [concrete shell migrations](cli.md#explicit-shell-migrations). An empty array + records that no command has been approved yet. - `changesets` (required non-empty array): ordered proposed changesets. ## Changeset fields @@ -101,6 +106,10 @@ Pure renames should use `paths` or `patch` so rename intent is preserved. - Keep one cohesive intent per changeset and prefer additive foundations before consumers, cutovers, or removals. +- Treat repository-discovered command text as a proposal only. Never copy it + into `test_argv` or infer argument boundaries without separate approval. +- Legacy `test_command` strings are invalid. Replace them with an explicit argv + array; the validator never applies shell-style splitting. - Make changesets append-only once validated; do not reorder or renumber an existing materialized position. - Document temporary flags and incomplete states in `pr_notes`, including the diff --git a/skills/carve-changesets/scripts/chain.py b/skills/carve-changesets/scripts/chain.py index a093055..f44c582 100755 --- a/skills/carve-changesets/scripts/chain.py +++ b/skills/carve-changesets/scripts/chain.py @@ -4,10 +4,10 @@ from __future__ import annotations import fnmatch -import subprocess from dataclasses import dataclass from typing import Dict, Iterable, List, Optional, Sequence, Tuple +from command_argv import display_argv, execute_argv, validate_argv from common import ( CommandError, branch_exists, @@ -382,14 +382,14 @@ def compare_chain(plan: Dict) -> Tuple[str, str]: return diffstat, namestatus -def validate_chain(plan: Dict, *, test_cmd: str) -> None: +def validate_chain(plan: Dict, *, test_argv: object) -> None: """Merge changesets in order into a temp branch and run tests after each merge.""" - effective_test_cmd = test_cmd.strip() - if not effective_test_cmd: + if isinstance(test_argv, list) and not test_argv: raise CommandError( - "validate-chain requires an explicitly approved --test-cmd or " - "plan.test_command." + "validate-chain requires an explicitly approved --test-argv or " + "plan.test_argv." ) + effective_test_argv = validate_argv(test_argv, label="approved test argv") ensure_git_repo() ensure_clean_tree() @@ -410,13 +410,14 @@ def validate_chain(plan: Dict, *, test_cmd: str) -> None: print(f"\n[STEP] Merging {name} ({idx} of {total})") git("merge", "--no-ff", "--no-edit", name) print( - f"[STEP] Running tests after changeset {idx}: {effective_test_cmd}" + f"[STEP] Running tests after changeset {idx}: " + f"{display_argv(effective_test_argv)}" ) if git("diff", "--quiet", check=False).returncode != 0: raise CommandError( "Working tree became dirty during validate-chain." ) - result = subprocess.run(effective_test_cmd, shell=True) + result = execute_argv(effective_test_argv) if result.returncode != 0: raise CommandError(f"Test command failed after changeset {idx}.") finally: diff --git a/skills/carve-changesets/scripts/cli.py b/skills/carve-changesets/scripts/cli.py index af31622..2989a02 100755 --- a/skills/carve-changesets/scripts/cli.py +++ b/skills/carve-changesets/scripts/cli.py @@ -8,6 +8,7 @@ from typing import Dict, List, Optional, Sequence from chain import compare_chain, create_chain, validate_chain +from command_argv import parse_argv_json from common import ( DEFAULT_PLAN_PATH, CommandError, @@ -73,14 +74,39 @@ def _print_discovered_test_command() -> None: else: for suggestion in discovery.get("suggestions", []): print(f"[HINT] Test command proposal: {suggestion}") - print("[NEXT] Pass an approved command explicitly with --test-cmd.") + print("[NEXT] Pass approved argv explicitly as JSON with --test-argv.") + + +def _reject_legacy_flag( + args: argparse.Namespace, *, attribute: str, old_flag: str, new_flag: str +) -> None: + if getattr(args, attribute, None) is not None: + raise CommandError( + f"{old_flag} is no longer supported because command strings are " + f"ambiguous. Use {new_flag} with a JSON argv array; use " + '["sh", "-lc", ""] only when shell ' + "semantics are intentional." + ) + + +def _parse_optional_argv(raw: Optional[str], *, label: str) -> List[str]: + if raw is None: + return [] + return parse_argv_json(raw, label=label) def cmd_preflight(args: argparse.Namespace) -> None: + _reject_legacy_flag( + args, + attribute="legacy_test_cmd", + old_flag="--test-cmd", + new_flag="--test-argv", + ) + test_argv = _parse_optional_argv(args.test_argv, label="--test-argv") preflight( base=args.base, source=args.source, - test_cmd=args.test_cmd, + test_argv=test_argv, skip_tests=args.skip_tests, skip_merge_check=args.skip_merge_check, allow_source_behind_base=args.allow_source_behind_base, @@ -90,8 +116,14 @@ def cmd_preflight(args: argparse.Namespace) -> None: def cmd_init_plan(args: argparse.Namespace) -> None: - test_cmd = str(args.test_cmd or "").strip() - if not test_cmd: + _reject_legacy_flag( + args, + attribute="legacy_test_cmd", + old_flag="--test-cmd", + new_flag="--test-argv", + ) + test_argv = _parse_optional_argv(args.test_argv, label="--test-argv") + if not test_argv: _print_discovered_test_command() init_plan( plan_path=Path(args.plan), @@ -99,7 +131,7 @@ def cmd_init_plan(args: argparse.Namespace) -> None: source=args.source, title=args.title, changesets=args.changesets, - test_cmd=test_cmd, + test_argv=test_argv, force=args.force, ) print(f"[OK] Wrote plan template: {args.plan}") @@ -179,9 +211,19 @@ def cmd_compare(args: argparse.Namespace) -> None: def cmd_validate_chain(args: argparse.Namespace) -> None: + _reject_legacy_flag( + args, + attribute="legacy_test_cmd", + old_flag="--test-cmd", + new_flag="--test-argv", + ) plan = load_and_validate(Path(args.plan)) - test_cmd = str(args.test_cmd or plan.get("test_command", "")).strip() - validate_chain(plan, test_cmd=test_cmd) + test_argv = ( + _parse_optional_argv(args.test_argv, label="--test-argv") + if args.test_argv is not None + else list(plan.get("test_argv", [])) + ) + validate_chain(plan, test_argv=test_argv) pull_requests = ( [] if args.local_only @@ -258,10 +300,28 @@ def cmd_recover_suffix(args: argparse.Namespace) -> None: def cmd_db_compare(args: argparse.Namespace) -> None: + _reject_legacy_flag( + args, + attribute="legacy_source_cmd", + old_flag="--source-cmd", + new_flag="--source-argv", + ) + _reject_legacy_flag( + args, + attribute="legacy_chain_cmd", + old_flag="--chain-cmd", + new_flag="--chain-argv", + ) + if args.source_argv is None or args.chain_argv is None: + raise CommandError( + "db-compare requires both --source-argv and --chain-argv JSON arrays." + ) + source_argv = parse_argv_json(args.source_argv, label="--source-argv") + chain_argv = parse_argv_json(args.chain_argv, label="--chain-argv") db_compare( load_and_validate(Path(args.plan)), - source_cmd=args.source_cmd, - chain_cmd=args.chain_cmd, + source_argv=source_argv, + chain_argv=chain_argv, out_dir=Path(args.out_dir), ) @@ -348,8 +408,17 @@ def _add_plan(parser: argparse.ArgumentParser) -> None: def _add_preflight_options(parser: argparse.ArgumentParser) -> None: parser.add_argument("--base", required=True, help="Base branch") parser.add_argument("--source", required=True, help="Source branch") - parser.add_argument( - "--test-cmd", default="", help="Explicitly approved test command" + test_command = parser.add_mutually_exclusive_group() + test_command.add_argument( + "--test-argv", + default=None, + help="Explicitly approved test argv as a JSON array of strings", + ) + test_command.add_argument( + "--test-cmd", + dest="legacy_test_cmd", + default=None, + help=argparse.SUPPRESS, ) parser.add_argument("--skip-tests", action="store_true") parser.add_argument("--skip-merge-check", action="store_true") @@ -402,7 +471,14 @@ def build_parser() -> argparse.ArgumentParser: item.add_argument("--source", required=True) item.add_argument("--title", required=True) item.add_argument("--changesets", type=int, default=3) - item.add_argument("--test-cmd", default="") + test_command = item.add_mutually_exclusive_group() + test_command.add_argument("--test-argv", default=None) + test_command.add_argument( + "--test-cmd", + dest="legacy_test_cmd", + default=None, + help=argparse.SUPPRESS, + ) item.add_argument("--force", action="store_true") item.set_defaults(func=cmd_init_plan) @@ -432,7 +508,14 @@ def build_parser() -> argparse.ArgumentParser: sub, "validate-chain", "Run approved step tests and live chain validation." ) _add_plan(item) - item.add_argument("--test-cmd", default="") + test_command = item.add_mutually_exclusive_group() + test_command.add_argument("--test-argv", default=None) + test_command.add_argument( + "--test-cmd", + dest="legacy_test_cmd", + default=None, + help=argparse.SUPPRESS, + ) item.add_argument("--remote", default="origin") item.add_argument("--local-only", action="store_true") item.set_defaults(func=cmd_validate_chain) @@ -496,8 +579,22 @@ def build_parser() -> argparse.ArgumentParser: item = _command(sub, "db-compare", "Compare source and chain database schemas.") _add_plan(item) - item.add_argument("--source-cmd", required=True) - item.add_argument("--chain-cmd", required=True) + source_command = item.add_mutually_exclusive_group() + source_command.add_argument("--source-argv") + source_command.add_argument( + "--source-cmd", + dest="legacy_source_cmd", + default=None, + help=argparse.SUPPRESS, + ) + chain_command = item.add_mutually_exclusive_group() + chain_command.add_argument("--chain-argv") + chain_command.add_argument( + "--chain-cmd", + dest="legacy_chain_cmd", + default=None, + help=argparse.SUPPRESS, + ) item.add_argument("--out-dir", default=str(DEFAULT_PLAN_PATH.parent / "db-compare")) item.set_defaults(func=cmd_db_compare) diff --git a/skills/carve-changesets/scripts/command_argv.py b/skills/carve-changesets/scripts/command_argv.py new file mode 100644 index 0000000..8271656 --- /dev/null +++ b/skills/carve-changesets/scripts/command_argv.py @@ -0,0 +1,68 @@ +"""Validated argv command specifications and execution.""" + +from __future__ import annotations + +import json +import shlex +import subprocess +from typing import Any + +from common import CommandError + + +def validate_argv(value: Any, *, label: str) -> list[str]: + """Return a validated copy of one JSON-compatible argv array.""" + + if not isinstance(value, list): + raise CommandError(f"{label} must be a JSON array of strings.") + if not value: + raise CommandError(f"{label} must be a non-empty JSON array of strings.") + if not all(isinstance(argument, str) for argument in value): + raise CommandError(f"{label} must contain only strings.") + if not value[0]: + raise CommandError(f"{label} executable (argv[0]) must not be empty.") + if any("\x00" in argument for argument in value): + raise CommandError(f"{label} must not contain NUL bytes.") + return list(value) + + +def validate_optional_argv(value: Any, *, label: str) -> list[str]: + """Validate an argv array while allowing an explicit empty-list placeholder.""" + + if isinstance(value, list) and not value: + return [] + return validate_argv(value, label=label) + + +def parse_argv_json(raw: str, *, label: str) -> list[str]: + """Parse and validate one argv array supplied as JSON.""" + + try: + value = json.loads(raw) + except json.JSONDecodeError as exc: + raise CommandError(f"{label} must be valid JSON: {exc.msg}.") from exc + return validate_argv(value, label=label) + + +def display_argv(argv: Any) -> str: + """Render argv for humans without changing what execution receives.""" + + return shlex.join(validate_argv(argv, label="command argv")) + + +def execute_argv( + argv: Any, *, text: bool = False, capture_output: bool = False +) -> subprocess.CompletedProcess: + """Execute an already approved argv array without implicit shell parsing.""" + + checked = validate_argv(argv, label="command argv") + try: + return subprocess.run( + checked, + shell=False, + text=text, + capture_output=capture_output, + check=False, + ) + except FileNotFoundError as exc: + raise CommandError(f"Command not found: {checked[0]}") from exc diff --git a/skills/carve-changesets/scripts/common.py b/skills/carve-changesets/scripts/common.py index 8cb1bfe..fcc77e0 100644 --- a/skills/carve-changesets/scripts/common.py +++ b/skills/carve-changesets/scripts/common.py @@ -222,6 +222,21 @@ def require_string(key: str) -> None: require_string("base_branch") require_string("source_branch") + if "test_command" in plan: + errors.append( + "Legacy plan.test_command is not supported; replace it with " + 'test_argv, for example ["just", "test"] or ' + '["sh", "-lc", ""].' + ) + + test_argv = plan.get("test_argv", []) + from command_argv import validate_optional_argv + + try: + validate_optional_argv(test_argv, label="plan.test_argv") + except CommandError as exc: + errors.append(str(exc)) + changesets = plan.get("changesets") if not isinstance(changesets, list) or not changesets: errors.append("Plan must include a non-empty 'changesets' array.") @@ -328,9 +343,12 @@ def init_plan( source: str, title: str, changesets: int, - test_cmd: str, + test_argv: object, force: bool, ) -> None: + from command_argv import validate_optional_argv + + approved_test_argv = validate_optional_argv(test_argv, label="test argv") plan_path.parent.mkdir(parents=True, exist_ok=True) if plan_path.exists() and not force: @@ -342,7 +360,7 @@ def init_plan( "feature_title": title, "base_branch": base, "source_branch": source, - "test_command": test_cmd or "", + "test_argv": approved_test_argv, "changesets": [default_changeset(i) for i in range(1, changesets + 1)], } diff --git a/skills/carve-changesets/scripts/db_compare.py b/skills/carve-changesets/scripts/db_compare.py index c9baeb9..2463ee8 100755 --- a/skills/carve-changesets/scripts/db_compare.py +++ b/skills/carve-changesets/scripts/db_compare.py @@ -3,10 +3,10 @@ from __future__ import annotations -import subprocess from pathlib import Path from typing import Dict +from command_argv import display_argv, execute_argv, validate_argv from common import ( CommandError, branch_name_for, @@ -20,17 +20,26 @@ ) -def run_capture(command: str, outfile: Path) -> None: - result = subprocess.run(command, shell=True, text=True, capture_output=True) +def run_capture(argv: object, outfile: Path) -> None: + approved_argv = validate_argv(argv, label="database command argv") + result = execute_argv(approved_argv, text=True, capture_output=True) if result.returncode != 0: detail = (result.stderr or result.stdout or "").strip() - raise CommandError(f"Command failed: {command}\n{detail}") + raise CommandError(f"Command failed: {display_argv(approved_argv)}\n{detail}") outfile.write_text(result.stdout) -def db_compare(plan: Dict, *, source_cmd: str, chain_cmd: str, out_dir: Path) -> None: - if not source_cmd.strip() or not chain_cmd.strip(): - raise CommandError("db-compare requires both --source-cmd and --chain-cmd.") +def db_compare( + plan: Dict, + *, + source_argv: object, + chain_argv: object, + out_dir: Path, +) -> None: + approved_source_argv = validate_argv( + source_argv, label="approved source schema argv" + ) + approved_chain_argv = validate_argv(chain_argv, label="approved chain schema argv") ensure_git_repo() ensure_clean_tree() @@ -53,7 +62,7 @@ def db_compare(plan: Dict, *, source_cmd: str, chain_cmd: str, out_dir: Path) -> try: git("checkout", source) print(f"[STEP] Running source command on {source}") - run_capture(source_cmd, source_out) + run_capture(approved_source_argv, source_out) git("checkout", "-B", temp_branch, base) for name in chain: @@ -61,7 +70,7 @@ def db_compare(plan: Dict, *, source_cmd: str, chain_cmd: str, out_dir: Path) -> git("merge", "--no-ff", "--no-edit", name) print(f"[STEP] Running chain command on {temp_branch}") - run_capture(chain_cmd, chain_out) + run_capture(approved_chain_argv, chain_out) print("[STEP] Diffing outputs (git diff --no-index)") diff = git( diff --git a/skills/carve-changesets/scripts/evals/grader.py b/skills/carve-changesets/scripts/evals/grader.py index 5afee94..529394b 100644 --- a/skills/carve-changesets/scripts/evals/grader.py +++ b/skills/carve-changesets/scripts/evals/grader.py @@ -8,13 +8,14 @@ import sys from dataclasses import asdict, dataclass from pathlib import Path -from typing import Dict, List +from typing import Dict, List, Sequence SCRIPTS_DIR = Path(__file__).resolve().parents[1] if str(SCRIPTS_DIR) not in sys.path: sys.path.insert(0, str(SCRIPTS_DIR)) from chain import compare_chain, create_chain, validate_chain # noqa: E402 +from command_argv import parse_argv_json # noqa: E402 from common import ( # noqa: E402 DEFAULT_PLAN_PATH, CommandError, @@ -90,10 +91,10 @@ def _check_equivalence(plan: Dict, checks: List[str], failures: List[str]) -> No def _check_validate_chain( - plan: Dict, test_cmd: str, checks: List[str], failures: List[str] + plan: Dict, test_argv: Sequence[str], checks: List[str], failures: List[str] ) -> None: try: - validate_chain(plan, test_cmd=test_cmd) + validate_chain(plan, test_argv=test_argv) checks.append("validate_chain") except CommandError as exc: failures.append(f"validate_chain: {exc}") @@ -141,7 +142,7 @@ def grade_repo( *, plan_path: Path, expected_source_hash: str, - test_cmd: str, + test_argv: Sequence[str], auto_create_chain: bool, ) -> GradeResult: checks: List[str] = [] @@ -167,7 +168,7 @@ def grade_repo( _check_chain_exists(plan, checks, failures) _check_equivalence(plan, checks, failures) - _check_validate_chain(plan, test_cmd=test_cmd, checks=checks, failures=failures) + _check_validate_chain(plan, test_argv=test_argv, checks=checks, failures=failures) _check_successor_recovery_metadata(plan, checks, failures) return GradeResult(ok=not failures, checks=checks, failures=failures) @@ -183,10 +184,17 @@ def build_parser() -> argparse.ArgumentParser: required=True, help="Expected source branch hash before the run", ) - parser.add_argument( + test_command = parser.add_mutually_exclusive_group() + test_command.add_argument( + "--test-argv", + default='["python3", "-c", "print(\\"ok\\")"]', + help="Test argv JSON for validate-chain", + ) + test_command.add_argument( "--test-cmd", - default="python3 -c \"print('ok')\"", - help="Test command for validate-chain", + dest="legacy_test_cmd", + default=None, + help=argparse.SUPPRESS, ) parser.add_argument( "--auto-create-chain", @@ -204,11 +212,15 @@ def build_parser() -> argparse.ArgumentParser: def main(argv: List[str] | None = None) -> int: parser = build_parser() args = parser.parse_args(argv) + if args.legacy_test_cmd is not None: + parser.error( + "--test-cmd is no longer supported; use --test-argv with a JSON argv array" + ) result = grade_repo( plan_path=Path(args.plan), expected_source_hash=args.expected_source_hash, - test_cmd=args.test_cmd, + test_argv=parse_argv_json(args.test_argv, label="--test-argv"), auto_create_chain=args.auto_create_chain, ) diff --git a/skills/carve-changesets/scripts/evals/helpers.py b/skills/carve-changesets/scripts/evals/helpers.py index 25990a4..0cc438b 100644 --- a/skills/carve-changesets/scripts/evals/helpers.py +++ b/skills/carve-changesets/scripts/evals/helpers.py @@ -56,7 +56,7 @@ def init_eval_repo() -> Tuple[Path, Dict, str]: "feature_title": "Eval feature", "base_branch": "main", "source_branch": "feature/test", - "test_command": "python3 -c \"print('ok')\"", + "test_argv": ["python3", "-c", "print('ok')"], "changesets": [ { "slug": "a-only", diff --git a/skills/carve-changesets/scripts/evals/runner.py b/skills/carve-changesets/scripts/evals/runner.py index 0eb4ef5..b37bf05 100644 --- a/skills/carve-changesets/scripts/evals/runner.py +++ b/skills/carve-changesets/scripts/evals/runner.py @@ -21,6 +21,7 @@ if str(SCRIPTS_DIR) not in sys.path: sys.path.insert(0, str(SCRIPTS_DIR)) +from command_argv import parse_argv_json # noqa: E402 from common import DEFAULT_PLAN_PATH # noqa: E402 from evals.grader import GradeResult, grade_repo # noqa: E402 @@ -119,7 +120,7 @@ def evaluate_forward( return observations, failures -def run_integration_case(case: dict, *, test_cmd: str) -> dict: +def run_integration_case(case: dict, *, test_argv: list[str]) -> dict: repo_dir, _plan, source_hash = init_eval_repo() original_cwd = Path.cwd() try: @@ -127,7 +128,7 @@ def run_integration_case(case: dict, *, test_cmd: str) -> dict: grade: GradeResult = grade_repo( plan_path=repo_dir / DEFAULT_PLAN_PATH, expected_source_hash=source_hash, - test_cmd=test_cmd, + test_argv=test_argv, auto_create_chain=bool(case.get("auto_create_chain", True)), ) expected_checks = set(case.get("objective_checks") or []) @@ -146,9 +147,9 @@ def run_integration_case(case: dict, *, test_cmd: str) -> dict: cleanup_repo(repo_dir) -def evaluate_integration(cases_path: Path, *, test_cmd: str) -> dict[str, dict]: +def evaluate_integration(cases_path: Path, *, test_argv: list[str]) -> dict[str, dict]: return { - case["id"]: run_integration_case(case, test_cmd=test_cmd) + case["id"]: run_integration_case(case, test_argv=test_argv) for case in load_json(cases_path) } @@ -180,19 +181,34 @@ def build_parser() -> argparse.ArgumentParser: parser.add_argument( "--integration-cases", type=Path, default=DEFAULT_INTEGRATION_CASES ) - parser.add_argument( + test_command = parser.add_mutually_exclusive_group() + test_command.add_argument( + "--test-argv", + default='["python3", "-c", "print(\\"ok\\")"]', + help="Approved argv JSON used by the objective chain grader", + ) + test_command.add_argument( "--test-cmd", - default="python3 -c \"print('ok')\"", - help="Approved command used by the objective chain grader", + dest="legacy_test_cmd", + default=None, + help=argparse.SUPPRESS, ) parser.add_argument("--output-dir", type=Path) return parser def main(argv: list[str] | None = None) -> int: - args = build_parser().parse_args(argv) + parser = build_parser() + args = parser.parse_args(argv) + if args.legacy_test_cmd is not None: + parser.error( + "--test-cmd is no longer supported; use --test-argv with a JSON argv array" + ) if args.integration_self_test: - results = evaluate_integration(args.integration_cases, test_cmd=args.test_cmd) + results = evaluate_integration( + args.integration_cases, + test_argv=parse_argv_json(args.test_argv, label="--test-argv"), + ) failures = [ f"{case_id}: {failure}" for case_id, result in results.items() diff --git a/skills/carve-changesets/scripts/plan_checks.py b/skills/carve-changesets/scripts/plan_checks.py index ff04e43..fc252fb 100644 --- a/skills/carve-changesets/scripts/plan_checks.py +++ b/skills/carve-changesets/scripts/plan_checks.py @@ -213,9 +213,9 @@ def validate_plan_strict(plan: Dict) -> Tuple[bool, List[str], List[str]]: _warn_placeholders(cs, idx, warnings) - test_cmd = str(plan.get("test_command", "")).strip() - if not test_cmd: - warnings.append("Plan test_command is empty; set it or pass --test-cmd.") + test_argv = plan.get("test_argv", []) + if not test_argv: + warnings.append("Plan test_argv is empty; set it or pass --test-argv.") try: freshness = compute_freshness(base, source) diff --git a/skills/carve-changesets/scripts/preflight.py b/skills/carve-changesets/scripts/preflight.py index 58547db..e48b812 100755 --- a/skills/carve-changesets/scripts/preflight.py +++ b/skills/carve-changesets/scripts/preflight.py @@ -3,8 +3,12 @@ from __future__ import annotations -import subprocess - +from command_argv import ( + display_argv, + execute_argv, + validate_argv, + validate_optional_argv, +) from common import ( CommandError, branch_exists, @@ -38,10 +42,11 @@ def check_mergeability(base: str, source: str) -> None: delete_branch(temp_branch) -def run_tests_on_branch(branch: str, test_cmd: str) -> None: - print(f"[INFO] Running test command on {branch}: {test_cmd}") +def run_tests_on_branch(branch: str, test_argv: object) -> None: + approved_argv = validate_argv(test_argv, label="test argv") + print(f"[INFO] Running test command on {branch}: {display_argv(approved_argv)}") with checkout_restore(branch): - result = subprocess.run(test_cmd, shell=True) + result = execute_argv(approved_argv) if result.returncode != 0: raise CommandError("Test command failed.") ensure_clean_tree() @@ -66,7 +71,7 @@ def _print_test_command_help(discovery: dict) -> None: for cmd in suggestions: print(f" - {cmd}") - print("[NEXT] Ask once for the desired test command, then re-run with --test-cmd.") + print("[NEXT] Ask once for the desired test command, then re-run with --test-argv.") print( "[NEXT] If still unknown, re-run with --skip-tests and record this in the plan." ) @@ -76,13 +81,14 @@ def preflight( *, base: str, source: str, - test_cmd: str, + test_argv: object, skip_tests: bool, skip_merge_check: bool, allow_source_behind_base: bool = False, confirm_source_behind_base: bool = False, allow_recordkeeping_tracked: bool = False, ) -> None: + effective_test_argv = validate_optional_argv(test_argv, label="approved test argv") ensure_git_repo() ensure_clean_tree() @@ -129,30 +135,37 @@ def preflight( else: raise CommandError(message) - if not skip_merge_check: - check_mergeability(base, source) - print("[OK] Mergeability check passed.") - else: - print("[WARN] Skipping mergeability check by request.") - - effective_test_cmd = test_cmd.strip() - if not effective_test_cmd and not skip_tests: + if not effective_test_argv and not skip_tests: discovery = discover_test_command("") discovered = str(discovery.get("command") or "").strip() if discovered: print(f"[HINT] Discovered test command proposal: {discovered}") - print("[NEXT] Re-run with that exact command passed via --test-cmd.") + print( + "[NEXT] Approve it, encode the intended argv explicitly, and pass " + "it via --test-argv." + ) else: _print_test_command_help(discovery) raise CommandError( "Preflight never executes discovered commands; pass an explicitly " - "approved --test-cmd or use --skip-tests." + "approved --test-argv JSON array or use --skip-tests." + ) + + if effective_test_argv: + effective_test_argv = validate_argv( + effective_test_argv, label="approved test argv" ) - if effective_test_cmd and not skip_tests: - run_tests_on_branch(source, effective_test_cmd) + if not skip_merge_check: + check_mergeability(base, source) + print("[OK] Mergeability check passed.") + else: + print("[WARN] Skipping mergeability check by request.") + + if effective_test_argv and not skip_tests: + run_tests_on_branch(source, effective_test_argv) print("[OK] Test command succeeded on source branch.") - elif effective_test_cmd and skip_tests: + elif effective_test_argv and skip_tests: print("[WARN] Test command provided but skipped by request.") else: print("[WARN] No test command provided.") diff --git a/skills/carve-changesets/scripts/tests/legacy_helpers.py b/skills/carve-changesets/scripts/tests/legacy_helpers.py index b8eed8c..e4d9272 100644 --- a/skills/carve-changesets/scripts/tests/legacy_helpers.py +++ b/skills/carve-changesets/scripts/tests/legacy_helpers.py @@ -64,7 +64,7 @@ def init_repo() -> tuple[Path, dict]: "feature_title": "Test feature", "base_branch": "main", "source_branch": "feature/test", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "a-only", @@ -115,7 +115,7 @@ def init_conflict_repo() -> tuple[Path, dict]: "feature_title": "Conflict feature", "base_branch": "main", "source_branch": "feature/conflict", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "conflict", diff --git a/skills/carve-changesets/scripts/tests/test_chain.py b/skills/carve-changesets/scripts/tests/test_chain.py index 3d386dc..866f2b6 100644 --- a/skills/carve-changesets/scripts/tests/test_chain.py +++ b/skills/carve-changesets/scripts/tests/test_chain.py @@ -2,6 +2,7 @@ import shutil import unittest +from unittest.mock import patch import helpers # noqa: F401 # ensures sys.path is set from chain import compare_chain, create_chain, validate_chain @@ -10,6 +11,17 @@ class ChainTests(unittest.TestCase): + def test_validate_chain_rejects_unknown_command_representations_before_git( + self, + ) -> None: + invalid_values = ["x", ("python3",), {"python3": "-V"}] + with patch("chain.ensure_git_repo") as ensure_git_repo: + for value in invalid_values: + with self.subTest(value=value): + with self.assertRaises(CommandError): + validate_chain({}, test_argv=value) + ensure_git_repo.assert_not_called() + def test_create_chain_and_compare_equivalence(self) -> None: repo_dir, plan = init_repo() try: @@ -38,7 +50,7 @@ def test_validate_chain_runs_tests(self) -> None: try: with chdir(repo_dir): create_chain(plan) - validate_chain(plan, test_cmd="python3 -c \"print('ok')\"") + validate_chain(plan, test_argv=["python3", "-c", "print('ok')"]) finally: shutil.rmtree(repo_dir) @@ -49,7 +61,7 @@ def test_validate_chain_fails_on_bad_command(self) -> None: create_chain(plan) with self.assertRaises(CommandError): validate_chain( - plan, test_cmd='python3 -c "import sys; sys.exit(7)"' + plan, test_argv=["python3", "-c", "import sys; sys.exit(7)"] ) finally: shutil.rmtree(repo_dir) @@ -67,7 +79,7 @@ def test_validate_chain_requires_explicit_command(self) -> None: with chdir(repo_dir): create_chain(plan) with self.assertRaisesRegex(CommandError, "explicitly approved"): - validate_chain(plan, test_cmd="") + validate_chain(plan, test_argv=[]) finally: shutil.rmtree(repo_dir) diff --git a/skills/carve-changesets/scripts/tests/test_command_argv.py b/skills/carve-changesets/scripts/tests/test_command_argv.py new file mode 100644 index 0000000..c7b70f0 --- /dev/null +++ b/skills/carve-changesets/scripts/tests/test_command_argv.py @@ -0,0 +1,94 @@ +from __future__ import annotations + +import shlex +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +import helpers # noqa: F401 # ensures sys.path is set +from command_argv import display_argv, execute_argv, parse_argv_json, validate_argv +from common import CommandError + + +class CommandArgvTests(unittest.TestCase): + def test_parse_accepts_json_string_array(self) -> None: + self.assertEqual( + ["python3", "-c", "print('ok')"], + parse_argv_json('["python3", "-c", "print(\'ok\')"]', label="--test-argv"), + ) + + def test_parse_rejects_malformed_json(self) -> None: + with self.assertRaisesRegex(CommandError, "valid JSON"): + parse_argv_json("[not-json", label="--test-argv") + + def test_validation_rejects_unsafe_or_unknown_representations(self) -> None: + invalid_values = [ + [], + "", + ("just", "test"), + {"command": ["just", "test"]}, + ["just", 1], + ["", "test"], + ["printf", "bad\x00argument"], + ] + for value in invalid_values: + with self.subTest(value=value): + with self.assertRaises(CommandError): + validate_argv(value, label="command argv") + + def test_execution_rejects_raw_unknown_representations(self) -> None: + invalid_values = ["x", ("python3",), {"python3": "-V"}] + with patch("command_argv.subprocess.run") as run_mock: + for value in invalid_values: + with self.subTest(value=value): + with self.assertRaises(CommandError): + execute_argv(value) + run_mock.assert_not_called() + + def test_execution_does_not_interpret_shell_metacharacters(self) -> None: + result = execute_argv( + [ + "python3", + "-c", + "import sys; print(sys.argv[1])", + "literal; echo not-executed", + ], + text=True, + capture_output=True, + ) + self.assertEqual(0, result.returncode) + self.assertEqual("literal; echo not-executed\n", result.stdout) + + def test_explicit_shell_semantics_remain_available(self) -> None: + result = execute_argv( + ["sh", "-lc", "printf '%s' explicit-shell"], + text=True, + capture_output=True, + ) + self.assertEqual(0, result.returncode) + self.assertEqual("explicit-shell", result.stdout) + + def test_explicit_shell_supports_required_shell_behaviors(self) -> None: + with tempfile.TemporaryDirectory(prefix="carve-shell-test-") as temp_dir: + output = Path(temp_dir) / "output.txt" + output_arg = shlex.quote(str(output)) + script = ( + "VALUE='hello world'; export VALUE; " + "printf '%s\\n' \"$VALUE\" | tr '[:lower:]' '[:upper:]' " + f"> {output_arg} && printf '%s' ':done' >> {output_arg}" + ) + result = execute_argv(["sh", "-lc", script]) + self.assertEqual(0, result.returncode) + self.assertEqual("HELLO WORLD\n:done", output.read_text()) + + def test_display_is_shell_escaped_but_execution_shape_is_unchanged(self) -> None: + argv = ["python3", "-c", "print('hello world')"] + self.assertEqual( + "python3 -c 'print('\"'\"'hello world'\"'\"')'", display_argv(argv) + ) + self.assertEqual(argv, validate_argv(argv, label="command argv")) + + +if __name__ == "__main__": + unittest.main() diff --git a/skills/carve-changesets/scripts/tests/test_common.py b/skills/carve-changesets/scripts/tests/test_common.py index f8d61aa..a21fdb3 100644 --- a/skills/carve-changesets/scripts/tests/test_common.py +++ b/skills/carve-changesets/scripts/tests/test_common.py @@ -6,7 +6,7 @@ from pathlib import Path import helpers # noqa: F401 # ensures sys.path is set -from common import DEFAULT_PLAN_PATH, init_plan, load_plan, validate_plan +from common import DEFAULT_PLAN_PATH, CommandError, init_plan, load_plan, validate_plan class CommonTests(unittest.TestCase): @@ -25,12 +25,52 @@ def test_init_plan_writes_valid_plan(self) -> None: source="feature/x", title="Title", changesets=2, - test_cmd="", + test_argv=[], force=True, ) plan = load_plan(plan_path) valid, errors = validate_plan(plan) self.assertTrue(valid, f"plan should validate: {errors}") + self.assertEqual([], plan["test_argv"]) + finally: + shutil.rmtree(temp_dir) + + def test_plan_validation_rejects_legacy_test_command_with_migration(self) -> None: + plan = { + "feature_title": "Title", + "base_branch": "main", + "source_branch": "feature/x", + "test_command": "just test", + "changesets": [ + { + "slug": "one", + "description": "One.", + "include_paths": ["a.txt"], + } + ], + } + valid, errors = validate_plan(plan) + self.assertFalse(valid) + self.assertTrue(any("test_argv" in error for error in errors), errors) + + def test_init_plan_rejects_unknown_command_representations(self) -> None: + invalid_values = ["x", ("python3",), {"python3": "-V"}] + temp_dir = Path(tempfile.mkdtemp(prefix="pcs-test-plan-")) + try: + for index, value in enumerate(invalid_values): + plan_path = temp_dir / f"plan-{index}.json" + with self.subTest(value=value): + with self.assertRaises(CommandError): + init_plan( + plan_path=plan_path, + base="main", + source="feature/x", + title="Title", + changesets=1, + test_argv=value, + force=True, + ) + self.assertFalse(plan_path.exists()) finally: shutil.rmtree(temp_dir) diff --git a/skills/carve-changesets/scripts/tests/test_db_compare.py b/skills/carve-changesets/scripts/tests/test_db_compare.py index d6bc2fd..2d93144 100644 --- a/skills/carve-changesets/scripts/tests/test_db_compare.py +++ b/skills/carve-changesets/scripts/tests/test_db_compare.py @@ -2,6 +2,8 @@ import shutil import unittest +from pathlib import Path +from unittest.mock import patch import db_compare as db_compare_mod from chain import create_chain @@ -9,6 +11,30 @@ class DbCompareTests(unittest.TestCase): + def test_db_compare_rejects_unknown_command_representations_before_git( + self, + ) -> None: + invalid_values = ["x", ("python3",), {"python3": "-V"}] + with patch.object(db_compare_mod, "ensure_git_repo") as ensure_git_repo: + for value in invalid_values: + with self.subTest(value=value, boundary="source"): + with self.assertRaises(db_compare_mod.CommandError): + db_compare_mod.db_compare( + {}, + source_argv=value, + chain_argv=["true"], + out_dir=Path("unused"), + ) + with self.subTest(value=value, boundary="chain"): + with self.assertRaises(db_compare_mod.CommandError): + db_compare_mod.db_compare( + {}, + source_argv=["true"], + chain_argv=value, + out_dir=Path("unused"), + ) + ensure_git_repo.assert_not_called() + def test_db_compare_creates_outputs(self) -> None: repo_dir, plan = init_repo() try: @@ -17,8 +43,8 @@ def test_db_compare_creates_outputs(self) -> None: create_chain(plan) db_compare_mod.db_compare( plan, - source_cmd="cat a.txt", - chain_cmd="cat a.txt", + source_argv=["cat", "a.txt"], + chain_argv=["cat", "a.txt"], out_dir=out_dir, ) diff --git a/skills/carve-changesets/scripts/tests/test_evals.py b/skills/carve-changesets/scripts/tests/test_evals.py index 1690067..7572c8f 100644 --- a/skills/carve-changesets/scripts/tests/test_evals.py +++ b/skills/carve-changesets/scripts/tests/test_evals.py @@ -43,7 +43,7 @@ def test_objective_grader_passes_the_integration_fixture(self) -> None: result = grade_repo( plan_path=repo_dir / ".carve-changesets/plan.json", expected_source_hash=source_hash, - test_cmd="python3 -c \"print('ok')\"", + test_argv=["python3", "-c", "print('ok')"], auto_create_chain=True, ) self.assertTrue(result.ok, f"grader should pass: {result.failures}") @@ -62,7 +62,7 @@ def test_objective_grader_detects_source_branch_mutation(self) -> None: result = grade_repo( plan_path=repo_dir / ".carve-changesets/plan.json", expected_source_hash=source_hash, - test_cmd="python3 -c \"print('ok')\"", + test_argv=["python3", "-c", "print('ok')"], auto_create_chain=True, ) self.assertFalse(result.ok) @@ -171,7 +171,7 @@ def test_integration_self_test_covers_baseline_and_successor_recovery(self) -> N ) results = RUNNER.evaluate_integration( RUNNER.DEFAULT_INTEGRATION_CASES, - test_cmd="python3 -c \"print('ok')\"", + test_argv=["python3", "-c", "print('ok')"], ) self.assertTrue(all(result["ok"] for result in results.values()), results) for result in results.values(): diff --git a/skills/carve-changesets/scripts/tests/test_hunks_apply.py b/skills/carve-changesets/scripts/tests/test_hunks_apply.py index 488c8aa..802c45d 100644 --- a/skills/carve-changesets/scripts/tests/test_hunks_apply.py +++ b/skills/carve-changesets/scripts/tests/test_hunks_apply.py @@ -39,7 +39,7 @@ def _init_hunk_repo() -> tuple[Path, dict]: "feature_title": "Hunk feature", "base_branch": "main", "source_branch": "feature/hunks", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "hunk-1", @@ -97,7 +97,7 @@ def _init_context_shift_repo() -> tuple[Path, dict]: "feature_title": "Shift feature", "base_branch": "main", "source_branch": "feature/shift", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "insert", @@ -159,7 +159,7 @@ def _init_patch_repo() -> tuple[Path, dict]: "feature_title": "Patch feature", "base_branch": "main", "source_branch": "feature/patch", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "patch", @@ -196,7 +196,7 @@ def _init_rename_repo() -> tuple[Path, dict]: "feature_title": "Rename feature", "base_branch": "main", "source_branch": "feature/rename", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "rename-hunk", @@ -245,7 +245,7 @@ def _init_bad_patch_repo() -> tuple[Path, dict]: "feature_title": "Bad patch", "base_branch": "main", "source_branch": "feature/bad-patch", - "test_command": "", + "test_argv": [], "changesets": [ { "slug": "bad-patch", diff --git a/skills/carve-changesets/scripts/tests/test_preflight.py b/skills/carve-changesets/scripts/tests/test_preflight.py index f573d65..7829364 100644 --- a/skills/carve-changesets/scripts/tests/test_preflight.py +++ b/skills/carve-changesets/scripts/tests/test_preflight.py @@ -2,6 +2,7 @@ import shutil import unittest +from unittest.mock import patch import preflight as preflight_mod from common import CommandError @@ -9,6 +10,21 @@ class PreflightTests(unittest.TestCase): + def test_preflight_rejects_unknown_command_representations_before_git(self) -> None: + invalid_values = ["x", ("python3",), {"python3": "-V"}] + with patch.object(preflight_mod, "ensure_git_repo") as ensure_git_repo: + for value in invalid_values: + with self.subTest(value=value): + with self.assertRaises(CommandError): + preflight_mod.preflight( + base="main", + source="feature/x", + test_argv=value, + skip_tests=False, + skip_merge_check=False, + ) + ensure_git_repo.assert_not_called() + def test_preflight_success_does_not_modify_source(self) -> None: repo_dir, plan = init_repo() try: @@ -19,7 +35,7 @@ def test_preflight_success_does_not_modify_source(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="python3 -c \"print('ok')\"", + test_argv=["python3", "-c", "print('ok')"], skip_tests=False, skip_merge_check=False, ) @@ -38,7 +54,7 @@ def test_preflight_detects_conflicts(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=False, ) @@ -57,7 +73,7 @@ def test_preflight_fails_when_source_behind_base(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=True, ) @@ -76,7 +92,7 @@ def test_source_behind_rejects_allow_flag_without_confirmation(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=True, allow_source_behind_base=True, @@ -96,7 +112,7 @@ def test_source_behind_rejects_confirmation_flag_without_allowance(self) -> None preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=True, confirm_source_behind_base=True, @@ -115,7 +131,7 @@ def test_source_behind_allows_two_flag_override(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=True, allow_source_behind_base=True, @@ -135,7 +151,7 @@ def test_preflight_requires_recordkeeping_ignored(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=True, ) @@ -152,7 +168,7 @@ def test_preflight_allows_recordkeeping_tracked_with_override(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=True, skip_merge_check=True, allow_recordkeeping_tracked=True, @@ -175,7 +191,7 @@ def test_issue_30_preflight_never_executes_discovered_command(self) -> None: preflight_mod.preflight( base=plan["base_branch"], source=plan["source_branch"], - test_cmd="", + test_argv=[], skip_tests=False, skip_merge_check=True, ) diff --git a/skills/carve-changesets/scripts/tests/test_scripts_integration.py b/skills/carve-changesets/scripts/tests/test_scripts_integration.py index 35286f2..f817ff9 100644 --- a/skills/carve-changesets/scripts/tests/test_scripts_integration.py +++ b/skills/carve-changesets/scripts/tests/test_scripts_integration.py @@ -69,8 +69,8 @@ def test_single_cli_exercises_ported_surface(self) -> None: [ cli, "validate-chain", - "--test-cmd", - "python3 -c \"print('ok')\"", + "--test-argv", + '["python3", "-c", "print(\\"ok\\")"]', "--local-only", ], cwd=repo_dir, @@ -100,6 +100,160 @@ def test_help_lists_mutation_class_for_every_operation(self) -> None: self.assertIn("remote mutation is dry-run", result.stdout) self.assertIn("by default", result.stdout) + def test_legacy_test_command_surfaces_fail_before_branch_mutation(self) -> None: + repo_dir, plan = init_repo() + try: + cli = str(SCRIPTS_DIR / "cli.py") + marker = repo_dir / "legacy-test-command-ran" + legacy_command = f"touch {marker}" + branches_before = run( + ["git", "for-each-ref", "--format=%(refname)", "refs/heads/"], + cwd=repo_dir, + ).stdout + cases = [ + ( + "preflight", + [ + "preflight", + "--base", + plan["base_branch"], + "--source", + plan["source_branch"], + "--test-cmd", + legacy_command, + ], + ), + ( + "init-plan", + [ + "init-plan", + "--base", + plan["base_branch"], + "--source", + plan["source_branch"], + "--title", + "Legacy", + "--test-cmd", + legacy_command, + ], + ), + ( + "validate-chain", + ["validate-chain", "--test-cmd", legacy_command], + ), + ( + "run", + [ + "run", + "--base", + plan["base_branch"], + "--source", + plan["source_branch"], + "--title", + "Legacy", + "--test-cmd", + legacy_command, + ], + ), + ] + for name, arguments in cases: + with self.subTest(command=name): + result = run([cli, *arguments], cwd=repo_dir, check=False) + self.assertEqual(1, result.returncode) + self.assertIn("--test-cmd is no longer supported", result.stdout) + self.assertIn("--test-argv", result.stdout) + branches_after = run( + ["git", "for-each-ref", "--format=%(refname)", "refs/heads/"], + cwd=repo_dir, + ).stdout + self.assertEqual(branches_before, branches_after) + self.assertFalse(marker.exists()) + finally: + shutil.rmtree(repo_dir) + + def test_legacy_database_command_surfaces_fail_before_branch_mutation(self) -> None: + repo_dir, _plan = init_repo() + try: + cli = str(SCRIPTS_DIR / "cli.py") + marker = repo_dir / "legacy-database-command-ran" + legacy_command = f"touch {marker}" + branches_before = run( + ["git", "for-each-ref", "--format=%(refname)", "refs/heads/"], + cwd=repo_dir, + ).stdout + cases = [ + ( + "--source-cmd", + [ + "db-compare", + "--source-cmd", + legacy_command, + "--chain-argv", + '["true"]', + ], + "--source-argv", + ), + ( + "--chain-cmd", + [ + "db-compare", + "--source-argv", + '["true"]', + "--chain-cmd", + legacy_command, + ], + "--chain-argv", + ), + ] + for legacy_flag, arguments, replacement_flag in cases: + with self.subTest(flag=legacy_flag): + result = run([cli, *arguments], cwd=repo_dir, check=False) + self.assertEqual(1, result.returncode) + self.assertIn( + f"{legacy_flag} is no longer supported", result.stdout + ) + self.assertIn(replacement_flag, result.stdout) + branches_after = run( + ["git", "for-each-ref", "--format=%(refname)", "refs/heads/"], + cwd=repo_dir, + ).stdout + self.assertEqual(branches_before, branches_after) + self.assertFalse(marker.exists()) + finally: + shutil.rmtree(repo_dir) + + def test_malformed_test_argv_fails_before_branch_mutation(self) -> None: + repo_dir, plan = init_repo() + try: + cli = str(SCRIPTS_DIR / "cli.py") + branches_before = run( + ["git", "for-each-ref", "--format=%(refname)", "refs/heads/"], + cwd=repo_dir, + ).stdout + result = run( + [ + cli, + "preflight", + "--base", + plan["base_branch"], + "--source", + plan["source_branch"], + "--test-argv", + "not-json", + ], + cwd=repo_dir, + check=False, + ) + branches_after = run( + ["git", "for-each-ref", "--format=%(refname)", "refs/heads/"], + cwd=repo_dir, + ).stdout + self.assertEqual(1, result.returncode) + self.assertIn("--test-argv must be valid JSON", result.stdout) + self.assertEqual(branches_before, branches_after) + finally: + shutil.rmtree(repo_dir) + def test_status_rehydrates_without_a_plan(self) -> None: repo_dir, plan = init_repo() try: