mirror of
https://github.com/affaan-m/ECC.git
synced 2026-11-10 18:16:02 +00:00
fix(skill-comply): harden sandbox setup after review
- baseline commit ignores global gpgsign and hooks; git failures during sandbox setup raise instead of leaving a repo with no HEAD or origin/HEAD - local origin defaults to the branch that is pushed, so a setup command that switches branches no longer breaks origin/HEAD - gh stub lives in a per-scenario dir and fails on unsupported subcommands - files: validated (mapping of str to str); paths under .git are rejected - --fixtures keeps symlinks as links and skips .git - --max-turns and --timeout must be positive; a timeout logs a warning - SKILL.md documents the new flags and the sandbox
This commit is contained in:
1 parent
95b301d649
commit
b85445735e
5 files changed
+114
-32
No files matched your search
@@ -40,8 +40,24 @@ uv run python -m scripts.run --dry-run ~/.claude/skills/search-first/SKILL.md
|
||||
|
||||
# Custom models
|
||||
uv run python -m scripts.run --gen-model haiku --model sonnet <path>
|
||||
|
||||
# Long multi-step workflows: raise the per-scenario turn cap and timeout
|
||||
uv run python -m scripts.run --max-turns 120 --timeout 2400 <path>
|
||||
|
||||
# Copy a directory (plans, config, sources) into every scenario sandbox
|
||||
uv run python -m scripts.run --fixtures ./fixtures <path>
|
||||
```
|
||||
|
||||
| Flag | Default | Meaning |
|
||||
|---|---|---|
|
||||
| `--max-turns N` | 30 | Turn cap per scenario |
|
||||
| `--timeout S` | 900 | Seconds per scenario; on timeout the partial trace is still graded and a warning is logged |
|
||||
| `--fixtures DIR` | none | Copied into each sandbox (symlinks kept as links, `.git` skipped) |
|
||||
|
||||
### Sandbox
|
||||
|
||||
Each scenario runs in `/tmp/skill-comply-sandbox/<id>/`. It holds the files the scenario lists under `files:` plus any `--fixtures`, committed as a baseline. It has a local bare `origin` with `origin/HEAD` set, so push and merge steps work offline. A stub `gh` is first on `PATH`: it fakes `pr create`, `pr view`, `pr list` and `repo view` and fails on any other subcommand. Results for steps that open or merge PRs are therefore simulated.
|
||||
|
||||
## Key Concept: Prompt Independence
|
||||
|
||||
Measures whether a skill/rule is followed even when the prompt doesn't explicitly support it.
|
||||
|
||||
@@ -56,7 +56,9 @@ Rules:
|
||||
".claude/plans/example.plan.md": |
|
||||
# Plan
|
||||
- [ ] step 1
|
||||
The sandbox is a git repo with these files committed as the baseline.
|
||||
Paths must be relative, stay inside the sandbox, and not start with `.git/`.
|
||||
Every value must be a string. The sandbox is a git repo with these files
|
||||
committed as the baseline.
|
||||
- Prompts should be realistic — something a developer would actually ask
|
||||
|
||||
Skill content:
|
||||
|
||||
@@ -19,6 +19,13 @@ from scripts.spec_generator import generate_spec
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _positive_int(value: str) -> int:
|
||||
number = int(value)
|
||||
if number < 1:
|
||||
raise argparse.ArgumentTypeError(f"must be a positive integer, got {value}")
|
||||
return number
|
||||
|
||||
|
||||
def main() -> None:
|
||||
logging.basicConfig(level=logging.INFO, format="%(message)s")
|
||||
|
||||
@@ -54,13 +61,13 @@ def main() -> None:
|
||||
|
||||
parser.add_argument(
|
||||
"--max-turns",
|
||||
type=int,
|
||||
type=_positive_int,
|
||||
default=30,
|
||||
help="Turn cap per scenario; long multi-step workflows need more (default: 30)",
|
||||
)
|
||||
parser.add_argument(
|
||||
"--timeout",
|
||||
type=int,
|
||||
type=_positive_int,
|
||||
default=900,
|
||||
help="Seconds per scenario before the partial trace is graded (default: 900)",
|
||||
)
|
||||
@@ -124,6 +131,12 @@ def main() -> None:
|
||||
timeout=args.timeout,
|
||||
fixtures_dir=args.fixtures,
|
||||
)
|
||||
if run.timed_out:
|
||||
logger.warning(
|
||||
" %s hit the %ds timeout; its partial trace is graded, "
|
||||
"so compliance may be understated.",
|
||||
scenario.level_name, args.timeout,
|
||||
)
|
||||
result = grade(spec, list(run.observations))
|
||||
graded_results.append((scenario.level_name, result, list(run.observations)))
|
||||
logger.info(" %s: %.0f%%", scenario.level_name, result.compliance_rate * 100)
|
||||
|
||||
@@ -34,6 +34,7 @@ class ScenarioRun:
|
||||
scenario: Scenario
|
||||
observations: tuple[ObservationEvent, ...]
|
||||
sandbox_dir: Path
|
||||
timed_out: bool = False
|
||||
|
||||
|
||||
def run_scenario(
|
||||
@@ -49,7 +50,7 @@ def run_scenario(
|
||||
|
||||
sandbox_dir = _safe_sandbox_dir(scenario.id)
|
||||
_setup_sandbox(sandbox_dir, scenario, fixtures_dir=fixtures_dir)
|
||||
gh_bin = _install_gh_stub(SANDBOX_BASE)
|
||||
gh_bin = _install_gh_stub(sandbox_dir)
|
||||
env = {**os.environ, "PATH": f"{gh_bin}{os.pathsep}{os.environ.get('PATH', '')}"}
|
||||
|
||||
try:
|
||||
@@ -79,6 +80,7 @@ def run_scenario(
|
||||
scenario=scenario,
|
||||
observations=tuple(_parse_stream_json(partial)),
|
||||
sandbox_dir=sandbox_dir,
|
||||
timed_out=True,
|
||||
)
|
||||
|
||||
# claude -p returns rc=1 when --max-turns is reached, but the stream-json
|
||||
@@ -157,7 +159,15 @@ def _setup_sandbox(
|
||||
subprocess.run(["git", "init"], cwd=sandbox_dir, capture_output=True)
|
||||
|
||||
if fixtures_dir is not None:
|
||||
shutil.copytree(fixtures_dir, sandbox_dir, dirs_exist_ok=True)
|
||||
# Keep links as links (never copy what they point at) and leave any
|
||||
# .git in the fixtures out, so it cannot replace the sandbox repo.
|
||||
shutil.copytree(
|
||||
fixtures_dir,
|
||||
sandbox_dir,
|
||||
dirs_exist_ok=True,
|
||||
symlinks=True,
|
||||
ignore=shutil.ignore_patterns(".git"),
|
||||
)
|
||||
_write_scenario_files(sandbox_dir, getattr(scenario, "files", ()))
|
||||
|
||||
for cmd in scenario.setup_commands:
|
||||
@@ -184,17 +194,23 @@ GH_STUB = """#!/bin/sh
|
||||
case "$1 $2" in
|
||||
"pr create") echo "https://github.com/skill-comply/sandbox/pull/1" ;;
|
||||
"pr view"|"pr list"|"repo view") echo "{}" ;;
|
||||
*) echo "skill-comply gh stub: unsupported command: gh $*" >&2; exit 1 ;;
|
||||
esac
|
||||
exit 0
|
||||
"""
|
||||
|
||||
|
||||
def _install_gh_stub(base: Path) -> Path:
|
||||
"""Write a fake ``gh`` under ``base/_bin`` and return that directory."""
|
||||
bin_dir = base / "_bin"
|
||||
def _install_gh_stub(sandbox_dir: Path) -> Path:
|
||||
"""Write a fake ``gh`` in a per-scenario dir beside the sandbox.
|
||||
|
||||
One dir per scenario keeps parallel or repeated runs from rewriting a
|
||||
``gh`` another run is executing. It sits outside the sandbox repo, so the
|
||||
baseline commit never includes it.
|
||||
"""
|
||||
bin_dir = sandbox_dir.parent / f"{sandbox_dir.name}.bin"
|
||||
bin_dir.mkdir(parents=True, exist_ok=True)
|
||||
gh = bin_dir / "gh"
|
||||
gh.write_text(GH_STUB)
|
||||
gh.write_text(GH_STUB, encoding="utf-8")
|
||||
gh.chmod(0o755)
|
||||
return bin_dir
|
||||
|
||||
@@ -208,19 +224,27 @@ def _add_local_origin(sandbox_dir: Path) -> None:
|
||||
origin = sandbox_dir.parent / f"{sandbox_dir.name}.origin.git"
|
||||
if origin.exists():
|
||||
shutil.rmtree(origin)
|
||||
subprocess.run(["git", "init", "--bare", str(origin)], capture_output=True)
|
||||
subprocess.run(
|
||||
["git", "remote", "add", "origin", str(origin)],
|
||||
cwd=sandbox_dir, capture_output=True,
|
||||
)
|
||||
subprocess.run(
|
||||
["git", "push", "-u", "origin", "HEAD"], cwd=sandbox_dir, capture_output=True
|
||||
)
|
||||
# A setup command may have switched branches; the origin must default to
|
||||
# the branch that gets pushed, or origin/HEAD cannot be resolved.
|
||||
branch = (_git(sandbox_dir, "symbolic-ref", "--short", "HEAD").stdout or "").strip()
|
||||
_git(sandbox_dir, "init", "--bare", f"--initial-branch={branch}", str(origin))
|
||||
_git(sandbox_dir, "remote", "add", "origin", str(origin))
|
||||
_git(sandbox_dir, "push", "-u", "origin", branch)
|
||||
# Scripts that find the default branch read refs/remotes/origin/HEAD.
|
||||
subprocess.run(
|
||||
["git", "remote", "set-head", "origin", "-a"],
|
||||
cwd=sandbox_dir, capture_output=True,
|
||||
_git(sandbox_dir, "remote", "set-head", "origin", branch)
|
||||
|
||||
|
||||
def _git(cwd: Path, *args: str) -> subprocess.CompletedProcess:
|
||||
"""Run git in the sandbox; a failure raises instead of leaving a half-built repo."""
|
||||
result = subprocess.run(
|
||||
["git", *args], cwd=cwd, capture_output=True, text=True
|
||||
)
|
||||
if result.returncode != 0:
|
||||
raise RuntimeError(
|
||||
f"sandbox setup: git {' '.join(args)} failed "
|
||||
f"(rc={result.returncode}): {result.stderr.strip()[:300]}"
|
||||
)
|
||||
return result
|
||||
|
||||
|
||||
def _write_scenario_files(
|
||||
@@ -233,19 +257,25 @@ def _write_scenario_files(
|
||||
target.relative_to(root) # ValueError on traversal or absolute path
|
||||
if target == root:
|
||||
raise ValueError(f"Scenario file path must name a file: {rel!r}")
|
||||
if target.relative_to(root).parts[0] == ".git":
|
||||
# Files under .git (hooks, config) run on the next git command.
|
||||
raise ValueError(f"Scenario file path must not be inside .git: {rel!r}")
|
||||
target.parent.mkdir(parents=True, exist_ok=True)
|
||||
target.write_text(content)
|
||||
target.write_text(content, encoding="utf-8")
|
||||
|
||||
|
||||
def _commit_initial_state(sandbox_dir: Path) -> None:
|
||||
"""Commit the starting files so the agent sees a clean tree and a HEAD."""
|
||||
identity = ["-c", "user.name=skill-comply", "-c", "user.email=skill-comply@localhost"]
|
||||
subprocess.run(["git", "add", "-A"], cwd=sandbox_dir, capture_output=True)
|
||||
subprocess.run(
|
||||
["git", *identity, "commit", "--allow-empty", "-m", "chore: sandbox baseline"],
|
||||
cwd=sandbox_dir,
|
||||
capture_output=True,
|
||||
)
|
||||
# Global signing or hook config on the operator's machine must not break
|
||||
# the baseline commit.
|
||||
identity = [
|
||||
"-c", "user.name=skill-comply",
|
||||
"-c", "user.email=skill-comply@localhost",
|
||||
"-c", "commit.gpgsign=false",
|
||||
"-c", "core.hooksPath=/dev/null",
|
||||
]
|
||||
_git(sandbox_dir, "add", "-A")
|
||||
_git(sandbox_dir, *identity, "commit", "--allow-empty", "-m", "chore: sandbox baseline")
|
||||
|
||||
|
||||
def _redact_home_path(text: str) -> str:
|
||||
|
||||
@@ -66,10 +66,31 @@ def generate_scenarios(
|
||||
description=s["description"],
|
||||
prompt=s["prompt"].strip(),
|
||||
setup_commands=tuple(s.get("setup_commands", [])),
|
||||
files=tuple(
|
||||
(str(path), str(content))
|
||||
for path, content in (s.get("files") or {}).items()
|
||||
),
|
||||
files=_parse_files(s["id"], s.get("files")),
|
||||
))
|
||||
|
||||
return sorted(scenarios, key=lambda s: s.level)
|
||||
|
||||
|
||||
def _parse_files(scenario_id: str, raw: object) -> tuple[tuple[str, str], ...]:
|
||||
"""Validate a scenario's ``files`` mapping (relative path -> text content).
|
||||
|
||||
LLM output is untrusted: a list, a bare value, or a null body would
|
||||
otherwise become the text "None" or crash the run after generation.
|
||||
"""
|
||||
if raw is None:
|
||||
return ()
|
||||
if not isinstance(raw, dict):
|
||||
raise ValueError(
|
||||
f"Scenario {scenario_id!r}: files must be a mapping of path to content, "
|
||||
f"got {type(raw).__name__}"
|
||||
)
|
||||
for path, content in raw.items():
|
||||
if not isinstance(path, str) or not path.strip():
|
||||
raise ValueError(f"Scenario {scenario_id!r}: file path must be a non-empty string")
|
||||
if not isinstance(content, str):
|
||||
raise ValueError(
|
||||
f"Scenario {scenario_id!r}: content of {path!r} must be a string, "
|
||||
f"got {type(content).__name__}"
|
||||
)
|
||||
return tuple(raw.items())
|
||||
Reference in new issue
Block a user