diff --git a/.harness/config.yaml b/.harness/config.yaml index 584a4ae..881ac5c 100644 --- a/.harness/config.yaml +++ b/.harness/config.yaml @@ -22,7 +22,12 @@ test_command: .venv/bin/python -m pytest tests/ -q # the check falls back to test_command's own interpreter. clean_clone_python: .venv310/bin/python # Bash commands stage agents may run without prompting. Everything else -# is denied in headless mode, so grant exactly what the stages need. +# is denied in headless mode. Read-only search and inspection is granted +# broadly: a denial costs a turn and buys nothing, because the harness's +# own rules are enforced by the coordinator after a stage has run, not by +# this list. Mutation is denied by hooks/bash_guard.py, which ships with +# the harness and is passed to every stage invocation, rather than by +# these entries happening to omit it. allowed_tools: - "Bash(.venv/bin/python:*)" - "Bash(python3:*)" @@ -32,3 +37,15 @@ allowed_tools: - "Bash(git status:*)" - "Bash(git diff:*)" - "Bash(git log:*)" + - "Bash(grep:*)" + - "Bash(rg:*)" + - "Bash(find:*)" + - "Bash(head:*)" + - "Bash(tail:*)" + - "Bash(wc:*)" + - "Bash(sort:*)" + - "Bash(uniq:*)" + - "Bash(diff:*)" + - "Bash(git show:*)" + - "Bash(git branch:*)" + - "Bash(git ls-files:*)" diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index b50988c..ca9e31b 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -60,7 +60,7 @@ Since story-032 the `create` value is **a path at or beneath** one of that stage One reusable template per agent role: `planner.md`, `implementer.md`, `tester.md`, `verifier.md`, `documenter.md`, `assist.md`. Each follows the five-layer structure: harness layer (durable rules shared by every agent), role layer (responsibilities and do-not boundaries), workflow layer (workflow priorities), stage layer (current objective), and runtime state layer (`{{placeholder}}` fields the coordinator fills at runtime). Optional placeholders render as `None` when nothing applies. -The shared harness-layer block (stay in scope, produce required artifacts, avoid blocked paths) lives once in `prompts/harness-layer.md` and is injected into the workflow-stage templates — `implementer.md`, `tester.md`, and `documenter.md` — through a single `{{harness_layer}}` placeholder, so a shared-rule change is a one-file edit. The verifier's harness layer is a distinct evidence-discipline block, not a duplicate, and is intentionally left inline. `planner.md` and `assist.md` are not workflow stages and have no harness layer. +The shared harness-layer block (stay in scope, produce required artifacts, avoid blocked paths) lives once in `prompts/harness-layer.md` and is injected into the workflow-stage templates — `implementer.md`, `tester.md`, and `documenter.md` — through a single `{{harness_layer}}` placeholder, so a shared-rule change is a one-file edit. The verifier's harness layer is a distinct evidence-discipline block, not a duplicate, and is intentionally left inline. `planner.md` and `assist.md` are not workflow stages and have no harness layer. Since story-035 the partial also carries `{{allowed_tools}}` — the granted Bash commands, injected from the target's own configuration so the prose cannot drift from what is permitted — and one sentence, explicitly labeled guidance rather than the enforcement, saying each Bash call must be a single command. It names no specific command; the list is the injected value. Nothing in the harness depends on an agent obeying that sentence, and "Tool allowlist" below says why it is there anyway. A prompt states a boundary; it does not hold it. `implementer.md`'s do-not list now states the create/modify distinction (existing tests may be modified, new test files may not be created unless the story grants an exception) and says outright that the coordinator enforces it — the prompt describes a rule that exists elsewhere rather than being the rule. Its runtime state layer injects `{{stage_exceptions}}` so a stage that *has* been granted an exception can see it, and `{{clean_clone_result}}` so a retry caused by the clean-clone check arrives with the evidence behind it. That placeholder exists because a clean-clone retry has no verifier finding to carry: the verifier passed, and the coordinator must not fabricate an agent's judgement by writing `retry-guidance.json` itself. @@ -77,8 +77,8 @@ The drift source that paragraph used to name is closed: `planner.md` no longer s - `story_coordinator.py` — the Story Coordinator. Loads the workflow definition, story artifact, and rules; creates the story branch and run directory *or resumes an existing one*; loops: determine stage → assemble context → render prompt → invoke agent → save artifacts → update state → route (advance, retry, or escalate). `run_story` takes an optional keyword-only `start_stage` overriding where execution enters — the recorded stage on a resume, `stage_names[0]` on a fresh run. It is named `start_stage` rather than `stage` because `stage` is the loop's name for the stage being executed. A `start_stage` the loaded workflow does not define is refused above everything else, in the same shape as the other pre-flight refusals: exit 1, one message naming the stages the workflow does define, nothing created and no agent invoked. See "Resuming a run" below for the resume branch, the escalation commit and the guard. Post-stage checks run in a fixed order: required artifacts present → required artifacts written by *this* attempt → declared artifacts match their schemas → changed-files record clear of blocked paths → stage output ownership → the revert check. The freshness check sits immediately after the presence check and never before it, so an artifact that is genuinely absent keeps its own missing-artifacts reason and the two stay distinguishable; see "An artifact's presence is not evidence of its authorship" below. Schema validation sits in the middle deliberately, so a malformed `changed-files.json` escalates with a validation error naming the field rather than raising out of the blocked-paths check that reads the same file. Ownership runs last, on the same record, after that record is known to be well-formed and clear of blocked paths. `_ownership_violation` returns a frozen `OwnershipViolation(path, prefix)`, and the escalation reason names stage, path, and prefix in both `events.log` and `escalation-summary.md`. `granted_paths(story, stage_name)` reads the story's grants for that stage and `grant_covers(granted, path)` decides whether any of them covers a path; the enforced list is passed whole and the grants are handed alongside it as an exemption, so `_ownership_violation(run_dir, record_name, prefixes, granted)` skips a covered path rather than the prefix being removed. Each grant is appended to `events.log` as `stage exception applied: may create `, one per grant whatever its granularity, so routing stays reconstructable from the log alone. The revert check sits immediately after, in the same block, reusing that same `enforced` list *and* that same exemption rather than recomputing either: `governed_edits(run_dir, record_name, prefixes, granted)` returns a frozen `GovernedEdits(paths, prefixes)` holding the sorted `modified` and `deleted` entries under any of the prefixes and covered by no grant, plus the prefixes that matched, and it names no stage, no prefix and no artifact. When `paths` is empty nothing at all happens — no clone, no suite, no artifact — because a check that can say nothing should not run. Otherwise `revert_check(run_dir, target_root, config, artifact, paths, baseline)` is shaped exactly like `clean_clone_check`: `tempfile.mkdtemp` scratch, the shared `run_clean_clone` with `revert` set to the governed paths, `shutil.rmtree` in a `finally` whatever the result, and `RevertCheckResult.as_record()` written under the declared artifact name. `permitted` is `result.exit_code != 0`. Two conditions stop it from running and neither permits: a stage that declares the check with no baseline captured (decided *before* any clone is attempted, so the reason names the missing directory rather than surfacing as a generic clone failure) and a clone that cannot be built. A check that did not run escalates naming the reason; `permitted` false escalates naming the stage, the prefixes and the paths; `permitted` true appends `_revert_check_permitted` and falls through to the existing advance. Both escalations go through `_escalate`, which does not touch `retry_count`. Since story-020 `_escalate` takes `target_root` and `harness_root` as required keywords — it commits and it records the harness revision — so every escalation site forwards both; a new escalation that forgets them is a `TypeError` rather than a run that quietly leaves its work uncommitted. `_build_clone` and `run_clean_clone` gained a `revert` parameter for this and, in story-019, the `baseline` the reverted content is restored *from*; both default to reverting nothing, so the clean-clone check is untouched. When `revert` is non-empty the restore runs inside the clone *after* the working-tree diff is applied and the untracked files are copied and *before* `git add -A`, so the clone commits those paths as the stage found them while every other change is present: the baseline's copy is copied over the clone's for each governed path it holds, and each governed path it does **not** hold is deleted in the clone, because a path absent from the baseline did not exist when the stage started. Deleting rather than skipping is the point — skipping decides nothing and would report a permission the check never established, which is the assertion-that-cannot-fail failure mode `tests/test_baseline_honesty.py` exists to prevent. Naming paths with no baseline raises `RuntimeError` naming them. No code path in the check reverts to HEAD any longer. Pre-flight story reading is one function, `read_story(story_text)`: load `story.schema.json`, parse with it, validate against it, and return a frozen `StoryReading` carrying both the `parsed` story (`None` when parsing failed) and the `problems` list. It runs above the run-directory creation and the branch checkout, so a rejection is an exit-1 refusal leaving no run directory, no `state.json`, no log, and no new branch — and no agent invoked. It is called exactly once per run, and the parse it returns is the run's only reading of the artifact: `reading.parsed` is threaded into every `build_context` call and into `_complete`, which takes the completion-report title and commit-message subject from `story["story"]["title"]` rather than scanning lines. A missing title is a loud `KeyError`; the schema marks it required and the run cannot reach `_complete` without having validated. `read_story` stays schema conformance only. Whether a story's `stage_exceptions` mean anything against the workflow *this run loaded* is a separate question the schema cannot answer, so it is a separate function — `stage_exception_problems(story, stages)`, called from `run_story` beside `read_story` and above the run-directory creation. It refuses an exception naming a stage the workflow does not define, and one granting a value beneath no prefix that stage was restricted on: an exception that grants nothing is a planning error, not a harmless one. Matching was exact equality against the declared prefixes until story-032 widened it to *containment* — a grant may name the whole prefix, a directory beneath it, or a single file beneath it — while both refusal messages read as they always did, so `create: tests` against a declared `tests/` still refuses rather than silently granting part of it. The stage → restricted-prefix mapping it needs is derived by `stage_restrictions(stages) -> list[tuple[str, str]]`, sitting immediately above it, which returns the workflow's (stage, prefix) create-restriction pairs in declared order; `stage_exception_problems` builds its per-stage mapping from that helper (taking the stage-name set from `stages` directly, so a stage declaring no `may_not_create` is still known to exist and the "names a stage the workflow does not define" branch is unaffected). The derivation exists once because story-025's plan-time strictness check needs the same pairs, and a second copy would be a second answer to "what does the workflow restrict". A third pre-flight joins them since story-021, the clean-tree check: `dirty_paths(target_root)` runs one `git status --porcelain` and returns the sorted, deduplicated paths, stripping the status codes, reducing a rename's `old -> new` to the new path, and unquoting a quoted path. It only *reads* the target — no commit, no branch, no stash, no index change — and a root that is not a git repository, or a git that fails for any other reason, reports nothing dirty, the same one-directional bias `unchanged_since_escalation` takes. Untracked files count, because a file no stage produced is exactly what `git add -A` would absorb; ignored files do not, which is why a gitignored `.harness/runs/` is not what the check is about. See "The tree a run starts from" below for which runs it applies to and why. A fourth joins them since story-027, the finished-branch check: `story_branch(config, story_id)` derives the branch name from the configured prefix in one place, and `completion_commits(target_root, branch, story_id)` returns one `" "` line per commit reachable from that branch that a finished run of this story made. It sits **above** the clean-tree check, so a developer whose tree is also dirty meets the refusal that makes the run pointless before the one that makes it unaccountable, rather than in two round trips. See "The branch a run starts from" below for the evidence it uses and why. A fifth joins them since story-030, the base check: `resolve_base(target_root, config, base)` settles what a story branch is cut from, and `base_problems(target_root, base, declared)` returns what refuses a run that would cut it from somewhere else. It sits between the finished-branch check and the clean-tree check, gated on the story branch not already existing, and `branch_behind(target_root, branch, base)` feeds the note an *existing* branch gets instead of a refusal. Both entry points read those two functions — see "The base a story branches from" below. A sixth joins them since story-028, and it is the only one that asks about the *definition* rather than about the target repository: `retry_routing_problems(stages)` sits beside `stage_exception_problems` and refuses a declared route whose destination the workflow does not define, and one whose destination does not sit strictly before the stage declaring it — see "Where a retry goes" below. All six refusals print through one extracted `refuse(header, problems, guidance)`, so the refusal shape (exit 1, one message per problem, nothing created) is a single code path rather than a copy per reason; what differs between them is the sentence above the list and the sentence below it, which is what the five thin callers — `refuse_bad_story`, `_refuse_dirty_tree`, `_refuse_finished_branch`, `_refuse_base` and `_refuse_bad_routing` — supply. `refuse` and `refuse_bad_story` are **public** since story-025, because plan time and pre-flight must print a given defect identically: `l5-plan` reports a failing artifact through `refuse_bad_story`, so the same defect in the same artifact produces the same text whether it is caught when the artifact is written or when it is run, from one function rather than from two that agree today. `_refuse_dirty_tree` stays private — it is a run-time refusal with no plan-time counterpart — and a helper is promoted when a second caller outside the coordinator actually needs it, not on principle. `_refuse_base` is the one place that rule is currently bent and it is recorded rather than glossed: it *does* have a caller outside the coordinator — `l5-plan` prints its base refusal through it, which is what makes the two entry points unable to word the same defect differently — and it kept its underscore. The decision functions the script reads, `resolve_base` and `base_problems`, are public; promote `_refuse_base` to match them in the story that has another reason to touch it. `load_state(run_dir)` moved above the run-directory creation for this, since the clean-tree check decides from the state the run is starting from and `load_state` reads a file rather than requiring a directory; nothing else about the resume decision moved with it. The retry branch archives before it increments: `archive_attempt(run_dir, archivable_artifacts(stages), state.retry_count + 1)` copies the superseded attempt's artifacts under `attempts/attempt-N/` — see the archive decisions below. `append_event(run_dir, message, *, kind, stage, artifacts, duration_seconds, verifier_outcome, retry_decision, retry_reason, retry_category, retry_stage)` is the run's single event write path: the prose message stays positional and is what the `events.log` line is built from, and the *same call* appends one structured entry to `execution-history.json`. `load_history(run_dir)` is the read side, called only by `append_event` for the next sequence number. `run_story` captures `stage_started_at = time.monotonic()` at the stage-started event and reads it through a local `elapsed()` at every event that ends a stage, so a completed stage's entry carries a duration the log only made derivable; `_escalate` forwards whatever structured fields an escalation has and tags its entry `escalated`. The clean-clone check is the last thing the verifier branch does on a passing verdict: `clean_clone_check(run_dir, target_root, config, artifact)` builds a scratch clone with `tempfile.mkdtemp`, runs `run_clean_clone`, removes the scratch directory in a `finally` whatever the result, and writes the returned `CleanCloneResult.as_record()` to the run directory under the declared artifact name. `_build_clone` does `git clone --no-local` from the target's filesystem path — over git's normal transport, see "The clone is built over the normal transport" below — applies the target's tracked edits as `git diff --binary HEAD` piped to `git apply`, copies the untracked-but-not-ignored files from `git ls-files --others --exclude-standard`, then commits inside the clone; the target repository is only read. `_link_interpreter_roots` links the top-level directory of each configured interpreter path into the clone and appends those names to the clone's `.git/info/exclude`, because a virtualenv is gitignored and therefore absent from a fresh clone, and a `.gitignore` entry for a directory does not cover a symlink standing in its place. Zero exit appends `_clean_clone_passed` and falls through to the existing advance; non-zero takes the retry path the verification-failed branch already takes — `archive_attempt` above the increment, then increment, save, `_clean_clone_failed`, and `index = stage_names.index(destination)` — or the existing escalation path at the ceiling, with `_clean_clone_failures` collapsing the output's `FAILED` lines into the one-line reason. Since story-028 the destination on that path is `clean_clone["retry_stage"]`, read off the widened declaration, rather than borrowed from the verifier's own table. Both events are module-level helpers rather than inline calls, for a reason worth keeping: `tests/test_story_011_validation.py` proves its own non-vacuity by deleting the first `retry_decision="retry",` line at the verification-failed branch's indentation, and an inline clean-clone branch nests deeper and sits earlier in the file, so its line *contains* that indented text and the mutation lands there instead of where it was aimed. - `story_parser.py` — lexer plus schema-directed interpreter for the story artifact. **The story dialect is not YAML**; see the module docstring before reaching for `yaml.safe_load`, which reads committed artifacts differently and wrongly. The lexer produces line/indent/content records, drops blank lines and full-line comments, consumes a `key: |` block scalar body whole (so blank and `#`-shaped lines *inside* it survive), and rejects tab indentation. The interpreter dispatches on the schema node's `type`, consulting structure only where the schema is silent. Under `items.type == "string"` a `- ` item is the verbatim remainder of its line, colons included; under `items.type == "object"` the same syntax parses into key/value pairs. Scalars are never coerced — every value is a `str`. A single `StoryParseError` carries line, expectation, and finding, rendering as `line 12: expected …, found …`. - `schema_validator.py` — `schemas_dir`, `load_schema`, `shipped_schemas`, `unsupported_keywords`, and `validate(instance, schema) -> list[str]`. `shipped_schemas(harness_root=None) -> tuple[str, ...]` reads `schemas/manifest.json` through `schemas_dir`, so the override behaves identically to `load_schema`'s, and raises `ValueError` on anything short of a well-formed non-empty list of strings. A deliberately small JSON Schema subset — `type`, `required`, `properties`, `items`, `enum` — because the harness is standard library only. `validate` walks the whole schema first and raises `ValueError` if any keyword outside that subset appears anywhere in it, so a schema can never claim a constraint the validator silently drops. Errors carry a tracked JSON path, the expectation, and the found value: `$.blocking_issues[0].severity: expected one of ["high", "medium", "low"], found string ("critical")`. -- `context_assembler.py` — builds each stage's runtime context from the story artifact, prior stage artifacts, retry state, and architecture documents, and renders it into the prompt template. `build_context` takes the raw `story_text`, the required keyword-only `story` (the parsed artifact from `read_story`), and — since story-028 — the required keyword-only `workflow`, whose `workflow_context` it merges into the assembled context; it never reads either artifact itself. The workflow was the one thing every stage's context was built without, which is why the verifier could not be told what the coordinator routes on. Making it required rather than optional was deliberate: a default would let a call site silently render a verifier prompt with no categories in it. `{{story}}` is `story_text` verbatim, and `{{acceptance_criteria}}` comes from the parsed list via `_dashed_lines`, which renders one `- `-prefixed criterion per line and returns `None` for an absent or empty list. `{{stage_exceptions}}` follows the same convention through `_exception_lines`: one dash-prefixed line per grant naming the stage, the granted path, and the reason, `None` when the story declares none. `render()` is single-pass: `re.sub` does not re-scan substituted text, so a placeholder injected by one substitution is not itself resolved. `build_context` therefore resolves the shared `prompts/harness-layer.md` partial as a **two-pass render** — it renders that partial (including the partial's own `{{blocked_paths}}` placeholder) against the assembled context first, then stores the already-resolved text as the `harness_layer` context value for injection into stage templates. When the partial is absent, `harness_layer` is left unset and renders as `None`. The schema placeholders come from `schema_context(harness_root) -> dict[str, str]`, a public function of the same module: it globs `harness_root/schemas/*.schema.json` and exposes each file's text under the stem with hyphens replaced by underscores plus `_schema` (`verification-result.schema.json` → `{{verification_result_schema}}`). `build_context` merges it with `update` at the point the inline loop used to run, before the two-pass render, so the values are available to any template. A new schema file becomes an injectable placeholder with no code change. The glob appears exactly once in the module because it has two callers: `build_context` for workflow stages, and `l5-plan` for the planner template, which no coordinator renders. `workflow_context(workflow, rules) -> dict[str, str | None]` sits beside `schema_context` for the same reason: it maps the loaded workflow's stage names to `{{workflow_stages}}`, each stage's `may_not_create` declarations to `{{stage_create_restrictions}}` (`" may not create files under "`, one line per pair), the rules' `blocked_paths` to `{{blocked_paths}}`, and the workflow's declared retry routes to `{{retry_routes}}` (`" -> : "`, one line per declared category), all through the shared `_dashed_lines` helper — `build_context`'s own `blocked_paths` rendering goes through the same helper, so the harness-layer partial and the planner render the list identically. `_dashed_lines` returns `None` for an empty list, and `render()` maps `None` to the literal `None`, so the empty-list edge changes no rendered prompt. `retry_routes(stages) -> list[RetryRoute]` sits above it as the single derivation of the workflow's `(declared_by, category, stage, when)` triples, in the same spirit as `stage_restrictions`. It lives in this module rather than in the coordinator because the coordinator imports this module and not the reverse, and its two readers — the coordinator's pre-flight check on the table and the rendering just above — therefore read one answer to "what does this workflow route". The rendering exists once: there is no second function turning workflow routes into prompt text. -- `agent_runner.py` — invokes `claude -p` headlessly (`--permission-mode acceptEdits --output-format stream-json --verbose`, prompt on stdin), streams raw output to the run's log, and returns the agent's final result text. +- `context_assembler.py` — builds each stage's runtime context from the story artifact, prior stage artifacts, retry state, and architecture documents, and renders it into the prompt template. `build_context` takes the raw `story_text`, the required keyword-only `story` (the parsed artifact from `read_story`), and — since story-028 — the required keyword-only `workflow`, whose `workflow_context` it merges into the assembled context; it never reads either artifact itself. The workflow was the one thing every stage's context was built without, which is why the verifier could not be told what the coordinator routes on. Making it required rather than optional was deliberate: a default would let a call site silently render a verifier prompt with no categories in it. `{{story}}` is `story_text` verbatim, and `{{acceptance_criteria}}` comes from the parsed list via `_dashed_lines`, which renders one `- `-prefixed criterion per line and returns `None` for an absent or empty list. `{{stage_exceptions}}` follows the same convention through `_exception_lines`: one dash-prefixed line per grant naming the stage, the granted path, and the reason, `None` when the story declares none. `render()` is single-pass: `re.sub` does not re-scan substituted text, so a placeholder injected by one substitution is not itself resolved. `build_context` therefore resolves the shared `prompts/harness-layer.md` partial as a **two-pass render** — it renders that partial (including the partial's own `{{blocked_paths}}` placeholder) against the assembled context first, then stores the already-resolved text as the `harness_layer` context value for injection into stage templates. When the partial is absent, `harness_layer` is left unset and renders as `None`. The schema placeholders come from `schema_context(harness_root) -> dict[str, str]`, a public function of the same module: it globs `harness_root/schemas/*.schema.json` and exposes each file's text under the stem with hyphens replaced by underscores plus `_schema` (`verification-result.schema.json` → `{{verification_result_schema}}`). `build_context` merges it with `update` at the point the inline loop used to run, before the two-pass render, so the values are available to any template. A new schema file becomes an injectable placeholder with no code change. The glob appears exactly once in the module because it has two callers: `build_context` for workflow stages, and `l5-plan` for the planner template, which no coordinator renders. `workflow_context(workflow, rules) -> dict[str, str | None]` sits beside `schema_context` for the same reason: it maps the loaded workflow's stage names to `{{workflow_stages}}`, each stage's `may_not_create` declarations to `{{stage_create_restrictions}}` (`" may not create files under "`, one line per pair), the rules' `blocked_paths` to `{{blocked_paths}}`, and the workflow's declared retry routes to `{{retry_routes}}` (`" -> : "`, one line per declared category), all through the shared `_dashed_lines` helper — `build_context`'s own `blocked_paths` rendering goes through the same helper, so the harness-layer partial and the planner render the list identically. `_dashed_lines` returns `None` for an empty list, and `render()` maps `None` to the literal `None`, so the empty-list edge changes no rendered prompt. `retry_routes(stages) -> list[RetryRoute]` sits above it as the single derivation of the workflow's `(declared_by, category, stage, when)` triples, in the same spirit as `stage_restrictions`. It lives in this module rather than in the coordinator because the coordinator imports this module and not the reverse, and its two readers — the coordinator's pre-flight check on the table and the rendering just above — therefore read one answer to "what does this workflow route". The rendering exists once: there is no second function turning workflow routes into prompt text. `config_context(config) -> dict[str, str | None]` joins the pair since story-035, mapping the target config's `allowed_tools` to `{{allowed_tools}}` through the same `_dashed_lines` helper, and `build_context` merges it beside `workflow_context` from a new **optional** keyword-only `allowed_tools` argument — optional where `workflow` is required, deliberately, because a call that omits it must render exactly what it rendered before the argument existed. The coordinator passes `config.get("allowed_tools")` at its one call site. See "Tool allowlist" above for why the grants are injected rather than restated in prose. +- `agent_runner.py` — invokes `claude -p` headlessly (`--permission-mode acceptEdits --output-format stream-json --verbose`, prompt on stdin), streams raw output to the run's log, and returns the agent's final result text. Since story-035 it also passes `--settings` on every stage invocation, registering the shipped deny-only Bash guard: `hooks_dir(harness_root=None)` resolves `hooks/` relative to this module the way `schema_validator` resolves schemas, and `guard_settings` reads `hooks/settings.json` and substitutes the guard's absolute path for the declaration's `{guard_path}`. An absent or unreadable declaration, or a missing guard file, returns `None` and the stage runs without the hook — the guard is the net and the allowlist is the gate, so failing to register it must not stop a run. The settings are resolved *here* rather than passed in, so `run_agent`'s signature is unchanged and the fake runners the suite injects need not know the hook exists. See "Tool allowlist" above for what the guard decides. - `harness_config.py` — loads `.harness/config.yaml` (a deliberately small YAML subset parsed directly, keeping the harness dependency-free), workflow definitions, and execution rules. Also owns `find_target_root(start) -> Path`: the walk-up from a starting directory to the nearest ancestor containing `.harness/config.yaml`, exiting 1 with `No .harness/config.yaml found here or above. Run l5-init first.` when none exists. That loop appears exactly once in the repository — `l5-run`, `l5-plan`, and `l5-status` all call it (story-009 extracted it from `l5-run`, which `l5-status` had copied byte-for-byte). `l5-init`'s config check is a different thing: a non-walking existence probe on a directory it was explicitly given. - `plan_commit.py` — the decision behind `l5-plan`'s post-session commit, kept out of the script so it is testable without spawning `claude`. Every function returns what happened rather than printing it. `snapshot(stories_dir)` is every file under the configured stories directory right now (a directory that does not exist yet snapshots as empty, so a repository's first story appears like any other); `new_artifacts(stories_dir, before)` is what appeared since. **Appearance is the whole test** — nothing here reads the session's exit status, and a session that only edited an existing artifact yields nothing. `commit_artifacts` runs `git add -- ` then `git commit -m -- `: the pathspec on the *commit* as well as the add is what keeps unrelated dirty work — and whatever the developer had already staged — out of it. There is no `git add -A` anywhere in this module. `commit_subject` is `Plan story-NNN: ` for one readable artifact, `Plan story-NNN` on any parse failure, and `Plan story-NNN, story-MMM` when one session added more than one, committed together because splitting them would invent an order the session did not have. The title is read through `story_coordinator.read_story` — the run's one reading of a story artifact, per that rule below — not through a second `story_parser.parse` call, so the commit message describes the artifact the way the run that executes it will. `current_branch`, `resolve_remote` and `push_commit` resolve the remote from `branch.<name>.remote`, fall back to `origin` when the branch tracks nothing, and report rather than attempt when neither exists; the push is `git push <remote> HEAD`, so a branch with no upstream is pushed under its own name without writing tracking configuration into the developer's repository as a side effect of planning. Nothing in the module rolls back, amends or resets: a failed push leaves the commit exactly where it is. It chooses, creates and switches no branch — the commit lands on whatever branch the developer was on, as the planner's own commit did. - `plan_validation.py` — the decision behind `l5-plan`'s plan-time validation, added by story-025 and kept out of the script for the same reason `plan_commit` is. It **returns** the problems it found; it never prints them, never repairs an artifact and never deletes one. `artifact_problems(artifacts, stages) -> dict[Path, list[str]]` is the composition function: one `story_coordinator.read_story` call per artifact, its parse handed to `story_coordinator.stage_exception_problems` and to `strictness_problems`, keyed by path and holding only the artifacts with problems, so an empty mapping is the whole of "these may be committed". An artifact `read_story` has something to say about yields that and nothing further — `run_story`'s own pre-flight order rather than a second one, because a story that failed to parse has no parse for the later checks and one that failed its *schema* has one whose shape they may not assume (`stage_exception_problems` indexes `exception["stage"]`; `strictness_problems` splits each entry as a string). @@ -94,7 +94,17 @@ The drift source that paragraph used to name is closed: `planner.md` no longer s ### Tool allowlist -Headless agents cannot answer permission prompts, so `.harness/config.yaml` carries an `allowed_tools` list of Bash command patterns (for example `Bash(.venv/bin/python:*)`) that the runner passes to every stage invocation via `--allowedTools`. Grant exactly what the stages need: the test command, `chmod`, and read-only git inspection. A command outside the allowlist is denied, and a stage that cannot gather its evidence will fail verification honestly rather than invent it. (story-001's first execution escalated for exactly this reason before the allowlist existed.) +Headless agents cannot answer permission prompts, so `.harness/config.yaml` carries an `allowed_tools` list of Bash command patterns (for example `Bash(.venv/bin/python:*)`) that the runner passes to every stage invocation via `--allowedTools`. A command outside the allowlist is denied, and a stage that cannot gather its evidence will fail verification honestly rather than invent it. (story-001's first execution escalated for exactly this reason before the allowlist existed.) + +**Read-only breadth is granted, and it weakens no boundary.** The list carried eight prefixes and omitted every search and inspection command a stage actually reaches for; story-028's implementer was denied six calls and re-attempted the same work through narrower ones, at a turn apiece. Since story-035 it also grants `grep`, `rg`, `find`, `head`, `tail`, `wc`, `sort`, `uniq`, `diff`, `git show`, `git branch` and `git ls-files`. **The harness does not rely on this list to enforce its rules**: blocked paths, stage output ownership, the `may_not_create` prefixes and the revert check are all decided by the coordinator *after* the stage has run, from the tree and from the stage's own declarations, and `rules/execution-rules.json` lists `blocked_paths` for the coordinator to enforce rather than for the CLI to. Granting `grep` grants the ability to read faster and nothing else. Write access is a separate question and is not granted: nothing that mutates the working tree, the index or the repository is on the list, and stages have `Edit` and `Write` under `acceptEdits` and need no shell to change files. `.harness/config.yaml` is target-repository configuration, so this is *this repository's* answer; `templates/config.yaml` carries the same read-only set so an initialized repository starts where this one is, deliberately rather than by drift. + +**The guard is the net behind the allowlist.** Because the widened list admits commands with mutating forms — `find -delete` is the clearest — `hooks/bash_guard.py` ships with the harness as a deny-only PreToolUse hook matching Bash, registered by `hooks/settings.json` and passed to every stage invocation through the CLI's `--settings` argument. `agent_runner` resolves the hooks directory relative to its own module, exactly as `schema_validator` resolves schemas, substitutes the guard's absolute path into the shipped declaration, and passes the result; `run_agent`'s signature is unchanged, so every fake runner the suite injects is untouched. The guard decomposes a command across pipes, semicolons, logical operators and newlines, and into the interiors of `$(...)` and backtick substitutions, then denies when a component's leading word is a mutator, when `find` is invoked with `-exec`, `-execdir`, `-delete` or `-ok`, or when a redirect names a file. It has **no allow path at all**, so a command the allowlist would refuse is never admitted by the guard reporting no problem with it, and the allowlist remains the thing that permits. + +**Its bias is fail-open, deliberately.** A command it cannot parse — an unbalanced quote, an unterminated substitution, a heredoc — a malformed payload and unreadable stdin each produce *no decision* rather than a deny, and the call falls through to `allowed_tools`. A fail-closed parser mistake would stop runs that should have proceeded, which is the more expensive error, and this is the same one-directional bias `dirty_paths`, `unchanged_since_escalation` and `completion_commits` already take. + +**Mode 2 is not fixed, and the prompt sentence is a cost measure rather than a boundary.** Two failure modes were visible in story-028's record. Mode 1 is an ungranted command — `grep`, `head`, `wc` — and widening the list fixes it. Mode 2 is a granted command in an ungranted *shape*: `.venv/bin/python -m pytest ... | grep -E ... > file; head -10 file` runs a granted binary, but the permission check matches the whole call string against a prefix pattern and cannot see that the leading command is permitted, so the entire call is denied. The heredoc form is the same shape. **Nothing here makes a composed call succeed.** What was added against it is a sentence in `prompts/harness-layer.md` saying each Bash call must be one command, and beside it the granted list itself, injected through `context_assembler.config_context` as `{{allowed_tools}}` from the target's own configuration so it cannot drift from what is permitted. Agents routinely ignore instructions of this kind; the story assumes only that some will follow it, and a partial hit rate on a cheap sentence is worth having. Read that text as guidance and the coordinator's post-stage checks as the enforcement — a reader who believes otherwise will narrow the guard on a false premise. + +`config_context` sits beside `schema_context` and `workflow_context` and renders through the same `_dashed_lines` helper, so an absent or empty `allowed_tools` renders as `None` like every other optional placeholder. `build_context`'s new argument is **optional and keyword-only**, where `workflow` is required, and the asymmetry is deliberate rather than an inconsistency: a verifier rendered with no retry categories in it would be a defect, while a stage rendered with no granted list is exactly what every call site rendered before this existed, so a call that omits it must render what it rendered before. The coordinator passes the loaded target config's `allowed_tools` at its one `build_context` call site. ### Rules (`rules/`) diff --git a/hooks/bash_guard.py b/hooks/bash_guard.py new file mode 100755 index 0000000..0c460b9 --- /dev/null +++ b/hooks/bash_guard.py @@ -0,0 +1,457 @@ +#!/usr/bin/env python3 +"""Deny a Bash call that mutates the working tree, the index or the repository. + +This is a PreToolUse hook, registered against Bash by hooks/settings.json and +passed to every stage invocation by orchestration/agent_runner.py. It reads the +hook payload from stdin and writes a deny decision to stdout when it finds a +mutator; otherwise it writes nothing at all. + +Two properties are load-bearing and neither is an accident of the code: + +**It denies only.** There is no allow path anywhere in this file — no input +produces a decision other than "deny". The allowlist in the target's +.harness/config.yaml is the thing that permits; this is the net behind it, so a +command the allowlist would refuse is never admitted by the guard reporting no +problem with it. + +**Its bias is fail-open.** Anything this cannot establish yields *no* decision +rather than a deny, and the call falls through to the allowlist. Unreadable +stdin, a malformed payload, an unbalanced quote, an unterminated substitution +and a heredoc all take that path. A fail-closed parser mistake would stop runs +that should have proceeded, which is the more expensive error, and it is the +same one-directional bias the coordinator's own checks already take. + +What it does not cover, stated here rather than implied: it reads the command as +written, so a mutator spelled to avoid recognition (quoted, assembled from +variables, base64-decoded, run through an interpreter) is not seen. That is the +allowlist's job, not this one's. +""" +from __future__ import annotations + +import json +import re +import sys + +HOOK_EVENT = "PreToolUse" + +# Commands that write to the filesystem. chmod is deliberately absent: it is +# granted on purpose, and it changes a mode rather than content. +MUTATORS = frozenset( + { + "rm", + "rmdir", + "mv", + "cp", + "dd", + "tee", + "truncate", + "ln", + "mkdir", + "touch", + "chown", + "chgrp", + "shred", + "unlink", + "install", + "patch", + "rsync", + "mkfifo", + } +) + +# git subcommands that move the working tree, the index or the repository. +# status, diff, log, show, branch and ls-files are granted read-only +# inspection and are deliberately not here. +GIT_MUTATORS = frozenset( + { + "add", + "am", + "apply", + "bisect", + "checkout", + "cherry-pick", + "clean", + "clone", + "commit", + "config", + "fetch", + "filter-branch", + "gc", + "init", + "merge", + "mv", + "notes", + "prune", + "pull", + "push", + "rebase", + "reflog", + "remote", + "reset", + "restore", + "revert", + "rm", + "stash", + "submodule", + "switch", + "tag", + "update-index", + "update-ref", + "worktree", + } +) + +# find actions that run a command or write something. +FIND_ACTIONS = frozenset( + {"-exec", "-execdir", "-ok", "-okdir", "-delete", "-fprint", "-fprintf", "-fls"} +) + +# Commands whose arguments name another command to run. The command being +# wrapped is checked in its own right, so `xargs rm` is not a way past this. +WRAPPERS = frozenset( + {"xargs", "env", "sudo", "doas", "nohup", "nice", "time", "timeout", "command"} +) + +SEPARATORS = ";\n&|" +ASSIGNMENT = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") +REDIRECT = re.compile(r"^(?:\d*|&)>>?") + + +class Unparseable(Exception): + """The command could not be decomposed, so the guard says nothing.""" + + +def _unquote(word: str) -> str: + if len(word) >= 2 and word[0] == word[-1] and word[0] in "'\"": + return word[1:-1] + return word + + +def _substitution(command: str, start: int, closer: str) -> tuple[str, int]: + """The interior of a command substitution, and the index past its closer.""" + depth = 1 + quote: str | None = None + index = start + while index < len(command): + char = command[index] + if quote == "'": + if char == "'": + quote = None + index += 1 + continue + if char == "\\": + index += 2 + continue + if quote == '"': + if char == '"': + quote = None + index += 1 + continue + if char in "'\"": + quote = char + index += 1 + continue + if closer == ")" and char == "(": + depth += 1 + elif char == closer: + depth -= 1 + if depth == 0: + return command[start:index], index + 1 + index += 1 + raise Unparseable("unterminated command substitution") + + +def decompose(command: str) -> list[str]: + """Every command string inside `command`. + + Its top-level components — split on pipes, semicolons, logical operators + and newlines — plus the interior of every command substitution, written as + $(...) or with backticks, decomposed the same way. A substitution inside + single quotes is left alone, because the shell does not run it either. + """ + components: list[str] = [] + substitutions: list[str] = [] + current: list[str] = [] + quote: str | None = None + index = 0 + length = len(command) + + while index < length: + char = command[index] + + if quote == "'": + if char == "'": + quote = None + current.append(char) + index += 1 + continue + + if char == "\\": + if command[index + 1 : index + 2] == "\n": + index += 2 # line continuation: the next line is this command + continue + current.append(char) + current.append(command[index + 1 : index + 2]) + index += 2 + continue + + if char == "$" and command[index + 1 : index + 2] == "(": + interior, index = _substitution(command, index + 2, ")") + substitutions.append(interior) + current.append(" ") + continue + + if char == "`": + interior, index = _substitution(command, index + 1, "`") + substitutions.append(interior) + current.append(" ") + continue + + if quote == '"': + if char == '"': + quote = None + current.append(char) + index += 1 + continue + + if char in "'\"": + quote = char + current.append(char) + index += 1 + continue + + # A heredoc's body is text rather than commands, and reading it as + # commands would deny for words that are only ever data. The guard has + # nothing to say about it. + if char == "<" and command[index + 1 : index + 2] == "<": + raise Unparseable("heredoc") + + # Redirection operators are consumed whole so that the & of 2>&1 and + # of &> is not mistaken for a separator. + if char == ">" or (char == "&" and command[index + 1 : index + 2] == ">"): + # A redirect starts a word of its own unless what precedes it is a + # bare file descriptor: `f>out` is an argument and a redirect, + # while `2>&1` is one operator. + trailing = "".join(current).rsplit(" ", 1)[-1].rsplit("\t", 1)[-1] + if trailing and not trailing.isdigit(): + current.append(" ") + if char == "&": + current.append("&") + index += 1 + current.append(">") + index += 1 + if command[index : index + 1] == ">": + current.append(">") + index += 1 + if command[index : index + 1] == "&": + current.append("&") + index += 1 + continue + + if char in SEPARATORS: + components.append("".join(current)) + current = [] + while index < length and command[index] in SEPARATORS: + index += 1 + continue + + if char in "()": # a subshell begins a new command + components.append("".join(current)) + current = [] + index += 1 + continue + + current.append(char) + index += 1 + + if quote is not None: + raise Unparseable("unbalanced quote") + + components.append("".join(current)) + for interior in substitutions: + components.extend(decompose(interior)) + return [component for component in components if component.strip()] + + +def tokens(component: str) -> list[str]: + """The component's words, with their quoting left on. + + Quotes are kept so that a quoted operator stays an argument: `grep ">" f` + searches for a character, and reporting it as a redirect would be a denial + the story exists to stop paying for. + """ + words: list[str] = [] + current: list[str] = [] + quote: str | None = None + for char in component: + if quote: + current.append(char) + if char == quote: + quote = None + continue + if char in "'\"": + quote = char + current.append(char) + continue + if char.isspace(): + if current: + words.append("".join(current)) + current = [] + continue + current.append(char) + if current: + words.append("".join(current)) + return words + + +def _command_words(words: list[str]) -> list[str]: + """The words from the command name onward, with env assignments dropped.""" + for position, word in enumerate(words): + if ASSIGNMENT.match(word): + continue + return words[position:] + return [] + + +def _name(word: str) -> str: + return _unquote(word).rsplit("/", 1)[-1] + + +def _writes_a_file(words: list[str]) -> str | None: + """The redirect in these words that names a file, if any. + + A descriptor duplication (2>&1) and a redirect to /dev/null write no file + and are left alone. + """ + for position, word in enumerate(words): + match = REDIRECT.match(word) + if not match: + continue + rest = word[match.end() :] + if rest.startswith("&"): + continue + target = rest or (words[position + 1] if position + 1 < len(words) else "") + target = _unquote(target) + if not target or target == "/dev/null": + continue + return word if rest else f"{word} {target}" + return None + + +def _mutator(words: list[str], depth: int = 0) -> str | None: + """What in these words mutates, named, or None.""" + words = _command_words(words) + if not words: + return None + name = _name(words[0]) + arguments = words[1:] + + if name in MUTATORS: + return name + if name == "git": + for argument in arguments: + if argument.startswith("-"): + continue + subcommand = _unquote(argument) + return f"git {subcommand}" if subcommand in GIT_MUTATORS else None + return None + if name == "find": + for argument in arguments: + if _unquote(argument) in FIND_ACTIONS: + return f"find {_unquote(argument)}" + return None + if name == "sed": + for argument in arguments: + stripped = _unquote(argument) + if stripped == "--in-place" or stripped.startswith("--in-place="): + return "sed --in-place" + if stripped.startswith("-i"): + return "sed -i" + return None + if name == "perl": + for argument in arguments: + stripped = _unquote(argument) + if stripped.startswith("-") and not stripped.startswith("--"): + if "i" in stripped[1:].split("e", 1)[0]: + return "perl -i" + return None + if name in WRAPPERS and depth < 3: + # Skip the wrapper's own flags and any numeric argument (a timeout's + # duration), then judge whatever command it was given. + remainder = list(arguments) + while remainder: + candidate = _unquote(remainder[0]) + if candidate.startswith("-") or candidate.replace(".", "").isdigit(): + remainder = remainder[1:] + continue + break + return _mutator(remainder, depth + 1) + return None + + +def offence(command: str) -> str | None: + """Why this command is denied, or None when the guard has nothing to say. + + Raises Unparseable when the command cannot be decomposed, which the caller + turns into no decision rather than into a denial. + """ + for component in decompose(command): + words = tokens(component) + found = _mutator(words) + if found: + return f"{found} in `{component.strip()}`" + redirect = _writes_a_file(words) + if redirect: + return f"redirect `{redirect}` in `{component.strip()}`" + return None + + +def deny(reason: str) -> dict: + return { + "hookSpecificOutput": { + "hookEventName": HOOK_EVENT, + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } + } + + +def main() -> int: + # Every failure below is silent by design: no decision returns the call to + # the allowlist, which is the gate. See the module docstring. + try: + payload = json.loads(sys.stdin.read()) + except (OSError, ValueError): + return 0 + if not isinstance(payload, dict): + return 0 + tool_input = payload.get("tool_input") + if not isinstance(tool_input, dict): + return 0 + command = tool_input.get("command") + if not isinstance(command, str) or not command.strip(): + return 0 + + try: + reason = offence(command) + except Unparseable: + return 0 + except Exception: # a parser defect must not stop a run + return 0 + if reason is None: + return 0 + + json.dump( + deny( + f"This harness denies Bash commands that write to the working tree, " + f"the index or the repository: {reason}. Use Edit or Write to change " + f"files." + ), + sys.stdout, + ) + sys.stdout.write("\n") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/hooks/settings.json b/hooks/settings.json new file mode 100644 index 0000000..8c3a36f --- /dev/null +++ b/hooks/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "{guard_path}" + } + ] + } + ] + } +} diff --git a/orchestration/agent_runner.py b/orchestration/agent_runner.py index 9790348..8bf17a8 100644 --- a/orchestration/agent_runner.py +++ b/orchestration/agent_runner.py @@ -12,6 +12,14 @@ from dataclasses import dataclass from pathlib import Path +# The hooks ship with the harness code, like the schemas, so they are resolved +# relative to this module rather than to a caller-supplied root. +HARNESS_ROOT = Path(__file__).resolve().parents[1] + +SETTINGS_NAME = "settings.json" +GUARD_NAME = "bash_guard.py" +GUARD_PLACEHOLDER = "{guard_path}" + @dataclass class AgentResult: @@ -19,6 +27,30 @@ class AgentResult: result_text: str +def hooks_dir(harness_root: Path | None = None) -> Path: + return (harness_root or HARNESS_ROOT) / "hooks" + + +def guard_settings(harness_root: Path | None = None) -> str | None: + """The shipped hook declaration, with the guard's own path resolved. + + The declaration's shape lives in hooks/settings.json, a data file, because + the harness root varies by installation and only the absolute path can be + computed here. An absent or unreadable declaration returns None and the + stage runs without the hook: the guard is the net behind the allowlist, + which is the gate, so failing to register it must not stop a run. + """ + directory = hooks_dir(harness_root) + guard = directory / GUARD_NAME + try: + declaration = (directory / SETTINGS_NAME).read_text(encoding="utf-8") + except OSError: + return None + if not guard.is_file(): + return None + return declaration.replace(GUARD_PLACEHOLDER, str(guard)) + + def run_agent( prompt: str, *, @@ -48,6 +80,13 @@ def run_agent( cmd += ["--model", model] if allowed_tools: cmd += ["--allowedTools", *allowed_tools] + # Every stage invocation carries the deny-only Bash guard. It is resolved + # here rather than passed in, so run_agent's signature is unchanged and no + # caller — including the fake runners the suite injects — has to know the + # hook exists. + settings = guard_settings() + if settings: + cmd += ["--settings", settings] log_path.parent.mkdir(parents=True, exist_ok=True) result_text = "" diff --git a/orchestration/context_assembler.py b/orchestration/context_assembler.py index 525ef1f..c07ec79 100644 --- a/orchestration/context_assembler.py +++ b/orchestration/context_assembler.py @@ -112,6 +112,17 @@ def workflow_context(workflow: dict, rules: dict) -> dict[str, str | None]: } +def config_context(config: dict) -> dict[str, str | None]: + """Map the target config's grants to their injectable placeholder name. + + The granted Bash commands are rendered from the target's own configuration + rather than restated in prose, so what a stage is told it may run cannot + drift from what is actually permitted. A config declaring no allowed_tools + renders as None, the optional-placeholder convention. + """ + return {"allowed_tools": _dashed_lines(config.get("allowed_tools"))} + + def _read(path: Path) -> str | None: return path.read_text(encoding="utf-8") if path.is_file() else None @@ -170,6 +181,7 @@ def build_context( retry_count: int, retry_category: str | None = None, retry_stage: str | None = None, + allowed_tools: list[str] | None = None, ) -> dict[str, str | None]: standards_dir = target_root / config.get("standards_dir", ".harness/standards") standards = _read_files( @@ -229,6 +241,12 @@ def build_context( # in any template. blocked_paths is rendered identically by both, through # the same helper, so the merge changes nothing about it. context.update(workflow_context(workflow, rules)) + # The target config's grants, injected the same way. This argument is + # optional where `workflow` is required, and the asymmetry is deliberate: + # a stage rendered with no categories in it would be a defect, while a + # stage rendered with no granted list is exactly what every call site + # rendered before this existed, so omitting it must change nothing. + context.update(config_context({"allowed_tools": allowed_tools})) # Two-pass render: resolve the shared harness-layer partial (including its # own {{blocked_paths}} placeholder) against the assembled context before diff --git a/orchestration/story_coordinator.py b/orchestration/story_coordinator.py index 124ad21..2e59ac0 100644 --- a/orchestration/story_coordinator.py +++ b/orchestration/story_coordinator.py @@ -2471,6 +2471,7 @@ def elapsed() -> float | None: retry_count=state.retry_count, retry_category=routed_category, retry_stage=routed_stage, + allowed_tools=config.get("allowed_tools"), ) template = context_assembler.load_template(harness_root, stage["prompt"]) prompt = context_assembler.render(template, context) diff --git a/prompts/harness-layer.md b/prompts/harness-layer.md index 70521d6..a9b1881 100644 --- a/prompts/harness-layer.md +++ b/prompts/harness-layer.md @@ -6,4 +6,14 @@ All work must: - avoid modifying blocked paths under any circumstances. Blocked paths for every stage: -{{blocked_paths}} \ No newline at end of file +{{blocked_paths}} + +Bash commands granted to you without prompting: +{{allowed_tools}} + +Guidance, not the enforcement: make each Bash call a single command. The +permission check matches the whole call string against a prefix pattern, so a +call that composes commands — with a pipe, a semicolon, a logical operator, a +redirect or a heredoc — is denied even when every command inside it is granted. +Run the parts as separate calls instead. Nothing in the harness depends on your +following this; it is here to save you the turns a denial costs. \ No newline at end of file diff --git a/templates/config.yaml b/templates/config.yaml index e693333..5f6223e 100644 --- a/templates/config.yaml +++ b/templates/config.yaml @@ -11,10 +11,27 @@ architecture_docs: - .harness/docs/ARCHITECTURE.md test_command: {test_command} # Bash commands stage agents may run without prompting. Headless agents -# cannot answer permission prompts, so grant exactly what the stages need. +# cannot answer permission prompts. Read-only search and inspection is +# granted broadly: a denial costs a turn and buys nothing, because the +# harness's own rules are enforced by the coordinator after a stage has +# run, not by this list. Mutation is denied by hooks/bash_guard.py, which +# ships with the harness and is passed to every stage invocation, rather +# than by these entries happening to omit it. allowed_tools: - "Bash(git status:*)" - "Bash(git diff:*)" - "Bash(git log:*)" - "Bash(ls:*)" - "Bash(cat:*)" + - "Bash(grep:*)" + - "Bash(rg:*)" + - "Bash(find:*)" + - "Bash(head:*)" + - "Bash(tail:*)" + - "Bash(wc:*)" + - "Bash(sort:*)" + - "Bash(uniq:*)" + - "Bash(diff:*)" + - "Bash(git show:*)" + - "Bash(git branch:*)" + - "Bash(git ls-files:*)" diff --git a/tests/test_harness_layer_extraction.py b/tests/test_harness_layer_extraction.py index d1579c0..4cda6ee 100644 --- a/tests/test_harness_layer_extraction.py +++ b/tests/test_harness_layer_extraction.py @@ -67,7 +67,13 @@ def test_shared_partial_file_holds_the_block_once(harness_root): """AC1: prompts/harness-layer.md exists and holds the shared block including the {{blocked_paths}} placeholder line.""" partial = (harness_root / "prompts" / "harness-layer.md").read_text() - assert partial == SHARED_BLOCK + # The block opens the file and is held to its text exactly. It was the + # whole file until story-035 appended the granted-list placeholder and the + # single-command sentence to it; equality is repointed to a prefix rather + # than relaxed, and the block still has to appear once and only once. + assert partial.startswith(SHARED_BLOCK) + assert partial.count("[Harness Layer]") == 1 + assert partial.count("Blocked paths for every stage:") == 1 assert "{{blocked_paths}}" in partial diff --git a/tests/test_story_035_validation.py b/tests/test_story_035_validation.py new file mode 100644 index 0000000..679e6e9 --- /dev/null +++ b/tests/test_story_035_validation.py @@ -0,0 +1,994 @@ +"""Independent validation for story-035: grant stages the read-only tools they +need, and deny mutation at the door. + +Three subjects, and each is exercised as the thing it actually is rather than +read as prose: + + * `hooks/bash_guard.py` is driven **as a program**, in a subprocess, with real + PreToolUse payloads on stdin. Nothing here imports the guard to call + `offence` directly, because what ships is a command line: a guard that + denied correctly in-process and crashed on a payload would pass the first + reading and fail the run. + * `orchestration/agent_runner.py` is driven with its own `subprocess.Popen` + replaced, so the argument list `run_agent` builds is inspected as built + rather than described. + * `orchestration/context_assembler.py` and `prompts/harness-layer.md` are + rendered, and the "omitting the new argument changes nothing" claim is + settled against a mutant of today's module with the new merge removed — + which is what the pre-story code was at that line. + +Every absence asserted here carries a demonstration that the same check can +report the violation it exists to catch: + + * "the guard says nothing about this command" is asserted only beside a + command that differs by the one feature under test and *is* denied — + `find -name` beside `find -delete`, `2>&1` beside `> out`, `git show` beside + `git commit`; + * "the guard has no allow path" is a scan for a non-docstring `"allow"` + literal whose control is a mutant guard carrying one, which the same scan + reports and which, run on the same read-only command, really does emit an + allow decision the shipped guard does not; + * "a malformed payload produces no decision" sits beside a well-formed + mutating payload through the identical driver, which does produce one, so + silence is shown to be about the input rather than about the driver; + * "the guard writes nothing" is a before/after snapshot of a directory and of + the payload file in it, whose control is a stub program that appends to + that same file and is reported by the same snapshot; + * "no granted entry writes to the tree" is checked against the guard's own + mutator tables, with a control list carrying `Bash(rm:*)` and + `Bash(git commit:*)` that the same check flags; + * "no existing allowlist entry was removed or altered" and "run_agent's + signature is unchanged" are both bounded at this story's own range through + `conftest`, never against a bare HEAD, and each carries a mutant control. + +Nothing here invokes a model. +""" +import ast +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import (BASELINE, HARNESS_ROOT, load_mutant, + repository_file_at) + +import agent_runner +import context_assembler +import harness_config +import schema_validator +import story_parser + +VALIDATION_FILE = Path(__file__) + +GUARD_REL = "hooks/bash_guard.py" +SETTINGS_REL = "hooks/settings.json" +GUARD_PATH = HARNESS_ROOT / GUARD_REL +CONFIG_REL = ".harness/config.yaml" +TEMPLATE_CONFIG_REL = "templates/config.yaml" +HARNESS_LAYER_REL = "prompts/harness-layer.md" + +WORKFLOW = harness_config.load_workflow(HARNESS_ROOT, "story-workflow") + + +# --------------------------------------------------------------------------- +# Driving the guard as a program +# --------------------------------------------------------------------------- + + +def payload_for(command: str) -> str: + """A real PreToolUse hook payload for a Bash call.""" + return json.dumps( + { + "session_id": "story-035-validation", + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_input": {"command": command}, + } + ) + + +def run_guard(stdin: str | None, *, guard: Path = GUARD_PATH, + cwd: Path | None = None) -> subprocess.CompletedProcess: + """The guard, run as a program with `stdin` on its standard input. + + `stdin=None` gives it nothing to read, which is the unreadable-stdin case. + """ + return subprocess.run( + [sys.executable, str(guard)], + input=stdin if stdin is not None else "", + capture_output=True, + text=True, + cwd=str(cwd) if cwd else None, + ) + + +def decision(command: str, *, guard: Path = GUARD_PATH) -> str | None: + """The guard's permission decision for `command`, or None when it is silent. + + Silence is the guard's fail-open answer, and it is a different outcome from + a decision — never conflated here with "not denied by name". + """ + result = run_guard(payload_for(command), guard=guard) + assert result.returncode == 0, (command, result.returncode, result.stderr) + if not result.stdout.strip(): + return None + emitted = json.loads(result.stdout) + return emitted["hookSpecificOutput"]["permissionDecision"] + + +def reason(command: str) -> str: + result = run_guard(payload_for(command)) + return json.loads(result.stdout)["hookSpecificOutput"]["permissionDecisionReason"] + + +# --------------------------------------------------------------------------- +# The corpora. Every command the guard is shown in this module is reachable +# from these lists, so the never-allow sweep at the bottom covers the module. +# --------------------------------------------------------------------------- + +#: AC3, the named mutators. +MUTATORS = [ + "rm -rf build", + "mv src/a.py src/b.py", + "cp src/a.py src/b.py", + "dd if=/dev/zero of=out.bin", + "tee captured.txt", + "truncate -s 0 notes.txt", + "ln -s a.py b.py", + "sed -i s/old/new/ notes.txt", + "perl -i -pe s/old/new/ notes.txt", +] + +#: AC3, the mutating git subcommands, each named in the criterion. +GIT_MUTATORS = [ + "git add -A", + "git commit -m message", + "git checkout main", + "git reset --hard", + "git rebase main", + "git merge main", + "git push origin main", + "git stash", + "git clean -fd", + "git rm notes.txt", + "git mv a.py b.py", + "git apply patch.diff", + "git restore notes.txt", + "git switch main", +] + +#: AC4, one mutator reached through each composition form. +COMPOSITIONS = { + "pipe": "ls src | rm -rf build", + "semicolon": "ls src; rm -rf build", + "double ampersand": "ls src && rm -rf build", + "double pipe": "ls src || rm -rf build", + "continuation line": "ls src \\\n&& rm -rf build", + "continuation line before a semicolon": "ls src; \\\n rm -rf build", + "newline": "ls src\nrm -rf build", + "$() substitution": "echo $(rm -rf build)", + "backtick substitution": "echo `rm -rf build`", + "$() inside double quotes": 'echo "$(rm -rf build)"', +} + +#: AC5. +FIND_DENIED = [ + "find . -name '*.pyc' -exec rm {} \\;", + "find . -name '*.pyc' -execdir rm {} \\;", + "find . -name '*.pyc' -delete", + "find . -name '*.pyc' -ok rm {} \\;", +] +FIND_ALLOWED = [ + "find . -name '*.py'", + "find orchestration -type f", + "find tests -newer conftest.py", +] + +#: AC6. +REDIRECT_DENIED = [ + "ls src > listing.txt", + "ls src >> listing.txt", + "grep -n token orchestration/agent_runner.py > hits.txt", +] +REDIRECT_ALLOWED = [ + "ls src 2>&1", + "grep -rn token orchestration 2>/dev/null", + "ls src > /dev/null", + "grep -rn token orchestration >> /dev/null", +] + +#: AC7, the read-only set the story grants, plus chmod and the test command. +READ_ONLY = [ + "grep -rn allowed_tools orchestration", + "rg --files-with-matches allowed_tools", + "head -20 orchestration/agent_runner.py", + "tail -5 prompts/harness-layer.md", + "wc -l tests/conftest.py", + "sort tests/conftest.py", + "uniq tests/conftest.py", + "diff templates/config.yaml .harness/config.yaml", + "cat .harness/config.yaml", + "ls -la hooks", + "chmod +x hooks/bash_guard.py", + "git status --short", + "git diff --stat", + "git log --oneline -5", + "git show HEAD:.harness/config.yaml", + "git branch --show-current", + "git ls-files hooks", + ".venv/bin/python -m pytest tests/ -q", + "grep -n '>' prompts/harness-layer.md", + "grep -rn 'rm -rf' tests", +] + +#: AC9, inputs the guard cannot establish anything about. +UNPARSEABLE = [ + 'grep -rn "unterminated orchestration', + "cat <<'EOF'\nrm -rf build\nEOF", + "echo $(rm -rf build", +] + +MALFORMED_PAYLOADS = { + "empty": "", + "not json": "this is not json", + "a json array": "[]", + "a non-dict tool_input": json.dumps({"tool_input": "rm -rf build"}), + "an empty object": "{}", + "no command key": json.dumps({"tool_input": {"description": "rm -rf build"}}), +} + +EVERY_COMMAND = ( + MUTATORS + GIT_MUTATORS + list(COMPOSITIONS.values()) + FIND_DENIED + + FIND_ALLOWED + REDIRECT_DENIED + REDIRECT_ALLOWED + READ_ONLY + UNPARSEABLE +) + + +# --------------------------------------------------------------------------- +# AC3, AC4: the guard denies what mutates, however it is reached +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("command", MUTATORS) +def test_the_guard_denies_each_named_mutator(command): + """AC3: run as a program with a real payload, each named mutator is denied.""" + assert decision(command) == "deny", command + + +@pytest.mark.parametrize("command", GIT_MUTATORS) +def test_the_guard_denies_each_mutating_git_subcommand(command): + """AC3: the fourteen git subcommands the criterion names.""" + assert decision(command) == "deny", command + + +@pytest.mark.parametrize("form,command", sorted(COMPOSITIONS.items())) +def test_the_guard_denies_a_mutator_reached_through_a_composition(form, command): + """AC4: the mutator is found wherever in a composed command it sits. + + Its control is the same composition with the mutator replaced by a + read-only command below: the denial is about `rm`, not about the shape. + """ + assert decision(command) == "deny", form + assert "rm" in reason(command), form + + +@pytest.mark.parametrize("form,command", sorted(COMPOSITIONS.items())) +def test_the_same_compositions_without_a_mutator_are_not_denied(form, command): + """The control for the composition table: shape alone denies nothing. + + Without this, every composition assertion above would still pass if the + guard simply denied anything containing a pipe or a newline — which is the + denial this story exists to stop paying for. + """ + harmless = command.replace("rm -rf build", "wc -l tests/conftest.py") + assert decision(harmless) is None, (form, harmless) + + +@pytest.mark.parametrize("command", FIND_DENIED) +def test_the_guard_denies_find_that_executes_or_writes(command): + """AC5: -exec, -execdir, -delete and -ok.""" + assert decision(command) == "deny", command + + +@pytest.mark.parametrize("command", FIND_ALLOWED) +def test_the_guard_says_nothing_about_find_without_those_actions(command): + """AC5, the absence half. Controlled by FIND_DENIED above: the same + `find . -name '*.pyc'` prefix is denied the moment `-delete` is appended, + so silence here is about the missing action rather than about `find`.""" + assert decision(command) is None, command + assert decision(command + " -delete") == "deny", command + + +@pytest.mark.parametrize("command", REDIRECT_DENIED) +def test_the_guard_denies_a_redirect_that_writes_a_file(command): + """AC6: > and >>.""" + assert decision(command) == "deny", command + assert "redirect" in reason(command), command + + +@pytest.mark.parametrize("command", REDIRECT_ALLOWED) +def test_the_guard_leaves_duplication_and_dev_null_alone(command): + """AC6, the absence half, controlled by pointing the same redirect at a + real file — which is denied — so silence is about the target rather than + about the operator.""" + assert decision(command) is None, command + assert decision(command.replace("/dev/null", "captured.txt") + if "/dev/null" in command + else command + " > captured.txt") == "deny", command + + +@pytest.mark.parametrize("command", READ_ONLY) +def test_the_guard_says_nothing_about_the_read_only_set(command): + """AC7: chmod, the twelve granted read-only commands, the read-only git + subcommands, quoted operators and the test command itself. + + The control is every denial above: the same guard, the same driver, the + same payload shape, denying the mutating counterpart of each of these.""" + assert decision(command) is None, command + + +def test_the_read_only_git_subcommands_are_distinguished_from_the_mutating_ones(): + """AC7's discrimination, stated as a pair rather than as two lists. + + `git show` is silent and `git commit` is denied through one code path, so + the pairing is the evidence that the subcommand is what is being read.""" + for readable, mutating in (("status --short", "add -A"), + ("diff --stat", "commit -m x"), + ("log --oneline", "reset --hard"), + ("show HEAD", "checkout main"), + ("branch --show-current", "switch main"), + ("ls-files", "rm notes.txt")): + assert decision(f"git {readable}") is None, readable + assert decision(f"git {mutating}") == "deny", mutating + + +def test_a_wrapped_mutator_is_judged_as_the_command_it_wraps(): + """xargs and env are not a way past the guard, and their read-only + counterparts are the control that the wrapper itself is not what denies.""" + assert decision("find . -name '*.pyc' | xargs rm") == "deny" + assert decision("find . -name '*.py' | xargs grep -l token") is None + assert decision("env FOO=1 rm -rf build") == "deny" + assert decision("env FOO=1 grep -rn token orchestration") is None + + +# --------------------------------------------------------------------------- +# AC8: the guard never allows +# --------------------------------------------------------------------------- + + +def _non_docstring_string_constants(source: str) -> list[str]: + """Every string literal in `source` that is not a docstring. + + Docstrings are excluded because the guard's own prose is *about* there + being no allow path, and a scan that could not tell the two apart would + report the documentation as the defect. + """ + tree = ast.parse(source) + docstrings = set() + for node in ast.walk(tree): + if isinstance(node, (ast.Module, ast.ClassDef, ast.FunctionDef, + ast.AsyncFunctionDef)): + body = getattr(node, "body", []) + if body and isinstance(body[0], ast.Expr) \ + and isinstance(body[0].value, ast.Constant) \ + and isinstance(body[0].value.value, str): + docstrings.add(id(body[0].value)) + return [ + node.value for node in ast.walk(tree) + if isinstance(node, ast.Constant) and isinstance(node.value, str) + and id(node) not in docstrings + ] + + +def test_the_guard_has_no_allow_decision_in_its_source(): + """AC8: no code in the guard can emit an allow decision. + + A behavioural sweep alone would only show that the corpus below does not + reach one; this shows there is nothing to reach.""" + literals = _non_docstring_string_constants( + GUARD_PATH.read_text(encoding="utf-8")) + assert "allow" not in [literal.strip().lower() for literal in literals] + assert not [literal for literal in literals if "allow" in literal.lower()] + + +def test_the_allow_scan_reports_a_guard_that_carries_one(tmp_path): + """The control for the scan above, and for the sweep below. + + A mutant guard with an allow path is reported by the same scan and, run + through the same driver on a command the shipped guard is silent about, + really does emit an allow decision. So both the scan and the sweep are + shown capable of the finding they report the absence of.""" + mutant = load_mutant( + GUARD_PATH, + [(' "permissionDecision": "deny",', + ' "permissionDecision": "deny",\n' + ' "_allowKey": "allow",')], + name="bash_guard_with_an_allow_literal", tmp_path=tmp_path) + literals = _non_docstring_string_constants( + Path(mutant.__file__).read_text(encoding="utf-8")) + assert "allow" in [literal.strip().lower() for literal in literals] + + allower = load_mutant( + GUARD_PATH, + [(" if reason is None:\n return 0", + ' if reason is None:\n' + ' json.dump({"hookSpecificOutput": {\n' + ' "hookEventName": HOOK_EVENT,\n' + ' "permissionDecision": "allow",\n' + ' "permissionDecisionReason": "control"}}, sys.stdout)\n' + ' return 0')], + name="bash_guard_that_allows", tmp_path=tmp_path) + control_guard = Path(allower.__file__) + assert decision("ls -la hooks", guard=control_guard) == "allow" + assert decision("ls -la hooks") is None + + +@pytest.mark.parametrize("command", EVERY_COMMAND) +def test_no_command_in_this_module_draws_an_allow(command): + """AC8, behaviourally, over every command this module shows the guard.""" + assert decision(command) in (None, "deny"), command + + +# --------------------------------------------------------------------------- +# AC9: the fail-open bias, and that the guard writes nothing +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("command", UNPARSEABLE) +def test_an_unparseable_command_produces_no_decision(command): + """AC9: silence rather than a denial, so the call falls to the allowlist. + + Each of these carries a mutator that *would* be denied if the guard could + read the command — which is the control that the silence is the fail-open + path rather than the guard finding nothing to say.""" + result = run_guard(payload_for(command)) + assert result.returncode == 0, command + assert result.stdout.strip() == "", command + + +def test_the_unparseable_cases_carry_a_mutator_the_guard_otherwise_denies(): + """The control for the three inputs above: each becomes a denial as soon as + the feature that made it unparseable is removed.""" + assert decision('grep -rn "unterminated" orchestration') is None + assert decision("rm -rf build") == "deny" + assert decision("echo $(rm -rf build)") == "deny" + + +@pytest.mark.parametrize("description,raw", sorted(MALFORMED_PAYLOADS.items())) +def test_a_malformed_payload_produces_no_decision(description, raw): + """AC9: a payload the guard cannot read yields nothing, not a deny.""" + result = run_guard(raw) + assert result.returncode == 0, description + assert result.stdout.strip() == "", description + + +def test_unreadable_stdin_produces_no_decision(): + """AC9: nothing on stdin at all.""" + result = run_guard(None) + assert result.returncode == 0 + assert result.stdout.strip() == "" + + +def test_the_same_driver_does_produce_a_decision_for_a_well_formed_payload(): + """The control for every silence above: the driver, the subprocess and the + payload shape are the same ones that carry a denial out, so silence is a + property of the input rather than of how the guard is being run.""" + result = run_guard(payload_for("rm -rf build")) + assert result.returncode == 0 + assert json.loads(result.stdout)["hookSpecificOutput"][ + "permissionDecision"] == "deny" + + +def _snapshot(directory: Path) -> dict[str, bytes]: + return { + str(path.relative_to(directory)): path.read_bytes() + for path in sorted(directory.rglob("*")) if path.is_file() + } + + +def test_the_guard_writes_nothing_to_the_payload_it_was_given(tmp_path): + """AC9: no file the guard can see changes, including the payload itself.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + payload_file = workspace / "payload.json" + payload_file.write_text(payload_for("rm -rf build"), encoding="utf-8") + before = _snapshot(workspace) + + for command in ("rm -rf build", "ls -la hooks", "git commit -m x", + 'grep -rn "unterminated orchestration'): + result = run_guard(payload_file.read_text(encoding="utf-8"), + cwd=workspace) + assert result.returncode == 0, command + + assert _snapshot(workspace) == before + + +def test_the_snapshot_reports_a_program_that_does_write(tmp_path): + """The control for the snapshot above: a stub that appends to the payload + file in its own working directory is caught by the identical check.""" + workspace = tmp_path / "writing-workspace" + workspace.mkdir() + payload_file = workspace / "payload.json" + payload_file.write_text(payload_for("rm -rf build"), encoding="utf-8") + before = _snapshot(workspace) + + stub = tmp_path / "writing_stub.py" + stub.write_text( + "import pathlib, sys\n" + "sys.stdin.read()\n" + "pathlib.Path('payload.json').open('a').write('touched')\n", + encoding="utf-8") + subprocess.run([sys.executable, str(stub)], input="{}", capture_output=True, + text=True, cwd=str(workspace), check=True) + + assert _snapshot(workspace) != before + + +# --------------------------------------------------------------------------- +# AC10, AC11: what agent_runner passes, and what its signature still is +# --------------------------------------------------------------------------- + + +class FakePopen: + """Enough of Popen for run_agent, recording the argument list it was built + with. The real CLI is never invoked.""" + + calls: list[list[str]] = [] + + def __init__(self, cmd, **kwargs): + FakePopen.calls.append(list(cmd)) + self.stdin = open(kwargs.get("_devnull", "/dev/null"), "w") + self.stdout = iter([json.dumps({"type": "result", "result": "done"}) + "\n"]) + + def wait(self): + self.stdin.close() + return 0 + + +def _built_command(monkeypatch, tmp_path) -> list[str]: + FakePopen.calls = [] + monkeypatch.setattr(agent_runner.subprocess, "Popen", FakePopen) + agent_runner.run_agent( + "prompt", + stage="implementer", + cwd=tmp_path, + log_path=tmp_path / "agent.log", + permission_mode="acceptEdits", + model=None, + allowed_tools=["Bash(grep:*)"], + ) + assert len(FakePopen.calls) == 1 + return FakePopen.calls[0] + + +def test_run_agent_passes_settings_registering_the_guard(monkeypatch, tmp_path): + """AC10: every stage invocation carries the settings, and the path they + name is a file that exists on disk.""" + cmd = _built_command(monkeypatch, tmp_path) + assert "--settings" in cmd + settings = json.loads(cmd[cmd.index("--settings") + 1]) + hooks = settings["hooks"]["PreToolUse"] + assert [entry["matcher"] for entry in hooks] == ["Bash"] + commands = [hook["command"] for entry in hooks for hook in entry["hooks"]] + assert len(commands) == 1 + named = Path(commands[0]) + assert named.is_absolute() + assert named.is_file(), named + assert named.resolve() == GUARD_PATH.resolve() + assert agent_runner.GUARD_PLACEHOLDER not in cmd[cmd.index("--settings") + 1] + + +def test_the_settings_check_reports_a_declaration_naming_nothing(tmp_path): + """The control for AC10: the same resolution against a hooks directory + whose guard is absent yields no settings at all, so "settings are passed" + is not something that holds for any harness root.""" + empty = tmp_path / "harness-without-a-guard" + (empty / "hooks").mkdir(parents=True) + (empty / "hooks" / "settings.json").write_text( + (HARNESS_ROOT / SETTINGS_REL).read_text(encoding="utf-8"), encoding="utf-8") + assert agent_runner.guard_settings(empty) is None + assert agent_runner.guard_settings(tmp_path / "nothing-at-all") is None + assert agent_runner.guard_settings() is not None + + +def test_the_shipped_declaration_holds_the_guard_path_as_a_placeholder(): + """The declaration is a data file whose absolute path is computed, so the + shape does not have to be rebuilt in code for a different installation.""" + declaration = (HARNESS_ROOT / SETTINGS_REL).read_text(encoding="utf-8") + assert agent_runner.GUARD_PLACEHOLDER in declaration + assert str(HARNESS_ROOT) not in declaration + + +def _signature_names(source: str) -> list[str]: + node = next(item for item in ast.parse(source).body + if isinstance(item, ast.FunctionDef) and item.name == "run_agent") + args = node.args + return [arg.arg for arg in (*args.posonlyargs, *args.args, *args.kwonlyargs)] + + +def test_run_agent_signature_is_unchanged_by_this_story(tmp_path): + """AC11, bounded at this story's own range through conftest rather than + against a bare HEAD, so the answer survives the run's own commit.""" + before = repository_file_at("orchestration/agent_runner.py", + validation_file=VALIDATION_FILE, bound=BASELINE) + today = (HARNESS_ROOT / "orchestration" / "agent_runner.py").read_text( + encoding="utf-8") + assert _signature_names(today) == _signature_names(before) + + # The control: a signature that did gain a parameter is reported by the + # same comparison, so the equality above is not vacuous. + widened = today.replace( + " allowed_tools: list[str] | None = None,\n", + " allowed_tools: list[str] | None = None,\n" + " settings: str | None = None,\n", 1) + assert _signature_names(widened) != _signature_names(before) + + +#: The keyword arguments the coordinator calls its injected runner with. +CALL_SITE_KWARGS = ("stage", "cwd", "log_path", "permission_mode", "model", + "allowed_tools") + + +def _unsatisfied_runners(directory: Path) -> list[tuple[str, list[str]]]: + """Runner-shaped callables under `directory` that the call site would break. + + A definition is runner-shaped when it takes `stage` and `log_path` as + keyword-only arguments; it satisfies the call site when it names every + keyword the coordinator passes, or absorbs the rest with **kwargs. + """ + unsatisfied = [] + for module in sorted(directory.glob("*.py")): + tree = ast.parse(module.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + names = [arg.arg for arg in node.args.kwonlyargs] + if "stage" not in names or "log_path" not in names: + continue + if node.args.kwarg is not None: + continue + missing = [kwarg for kwarg in CALL_SITE_KWARGS if kwarg not in names] + if missing: + unsatisfied.append((f"{module.name}::{node.name}", missing)) + return unsatisfied + + +def _runner_shaped_count(directory: Path) -> int: + return sum( + 1 + for module in directory.glob("*.py") + for node in ast.walk(ast.parse(module.read_text(encoding="utf-8"))) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and {"stage", "log_path"} <= {arg.arg for arg in node.args.kwonlyargs} + ) + + +def test_every_fake_runner_in_the_suite_still_satisfies_the_call_site(): + """AC11's consequence: no fake runner needed editing for this story. + + Asserted as "each accepts what the coordinator passes" rather than as + "the suite passes", because a fake that had quietly grown a parameter + would still let the suite pass while breaking the next caller.""" + tests_dir = HARNESS_ROOT / "tests" + assert _runner_shaped_count(tests_dir) > 0, \ + "no fake runner was discovered, so this asserts nothing" + assert _unsatisfied_runners(tests_dir) == [] + + +def test_the_fake_runner_scan_reports_one_that_does_not_satisfy_it(tmp_path): + """The control for the scan above: a runner-shaped fake missing the call + site's newest keyword is reported by the identical scan, so the empty + result means "none present" rather than "none looked for".""" + planted = tmp_path / "planted" + planted.mkdir() + (planted / "test_planted.py").write_text( + "def fake_runner(prompt, *, stage, cwd, log_path, permission_mode,\n" + " model):\n" + " return None\n", + encoding="utf-8") + assert _runner_shaped_count(planted) == 1 + assert _unsatisfied_runners(planted) == [ + ("test_planted.py::fake_runner", ["allowed_tools"])] + + +# --------------------------------------------------------------------------- +# AC12, AC13: config_context, and that omitting the argument changes nothing +# --------------------------------------------------------------------------- + + +GRANTS = ["Bash(grep:*)", "Bash(git show:*)"] + + +def test_config_context_maps_allowed_tools_through_the_shared_helper(): + """AC12: dash-prefixed lines, and rendered by _dashed_lines rather than by + a second copy of it — shown by replacing the helper and seeing the result + change, which no independent formatting could do.""" + assert context_assembler.config_context({"allowed_tools": GRANTS}) == { + "allowed_tools": "- Bash(grep:*)\n- Bash(git show:*)"} + assert context_assembler.config_context({"allowed_tools": GRANTS}) == { + "allowed_tools": context_assembler._dashed_lines(GRANTS)} + + +def test_config_context_renders_none_when_the_config_declares_no_grants(): + """AC12's absence half, controlled by the populated case above: the same + call with grants renders them, so None is about the config.""" + assert context_assembler.config_context({}) == {"allowed_tools": None} + assert context_assembler.config_context({"allowed_tools": []}) == { + "allowed_tools": None} + assert context_assembler.config_context( + {"allowed_tools": GRANTS})["allowed_tools"] is not None + + +def test_config_context_uses_the_shared_dashed_lines_helper(monkeypatch): + """AC12 names the helper, so the helper is what is checked.""" + monkeypatch.setattr(context_assembler, "_dashed_lines", + lambda items: "SENTINEL") + assert context_assembler.config_context( + {"allowed_tools": GRANTS}) == {"allowed_tools": "SENTINEL"} + + +def _context(target_root: Path, **extra) -> dict: + story_text = (target_root / ".harness" / "stories" / "story-001.yaml").read_text( + encoding="utf-8") + run_dir = target_root / ".harness" / "runs" / "story-001" + run_dir.mkdir(parents=True, exist_ok=True) + return dict( + story_text=story_text, + story=story_parser.parse(story_text, schema_validator.load_schema("story")), + run_dir=run_dir, + target_root=target_root, + harness_root=HARNESS_ROOT, + config=harness_config.load_config(target_root), + rules=harness_config.load_rules(HARNESS_ROOT), + workflow=WORKFLOW, + retry_count=0, + **extra, + ) + + +def test_the_new_build_context_argument_is_optional_and_keyword_only(): + """AC13, read off the signature.""" + parameters = ast.parse( + (HARNESS_ROOT / "orchestration" / "context_assembler.py").read_text( + encoding="utf-8")) + node = next(item for item in parameters.body + if isinstance(item, ast.FunctionDef) and item.name == "build_context") + assert "allowed_tools" not in [arg.arg for arg in node.args.args] + keyword_only = [arg.arg for arg in node.args.kwonlyargs] + assert "allowed_tools" in keyword_only + index = keyword_only.index("allowed_tools") + assert node.args.kw_defaults[index] is not None + + +def test_omitting_the_argument_renders_what_it_rendered_before_this_story( + target_root, tmp_path): + """AC13: a call that omits the argument renders exactly what a + context_assembler without the merge renders — which is the pre-story code + at that line, reconstructed from today's source rather than recovered from + history, so it stays honest when the story commits.""" + without_the_merge = load_mutant( + HARNESS_ROOT / "orchestration" / "context_assembler.py", + [(" context.update(config_context({\"allowed_tools\": allowed_tools}))", + " pass # the merge this story added, removed")], + name="context_assembler_before_story_035", tmp_path=tmp_path) + + today = context_assembler.build_context(**_context(target_root)) + before = without_the_merge.build_context(**_context(target_root)) + + assert today["allowed_tools"] is None + assert {key: value for key, value in today.items() + if key != "allowed_tools"} == before + + # The control: supplying the argument *does* change the render, so the + # equality above is a statement about omission rather than about the merge + # being inert. + supplied = context_assembler.build_context( + **_context(target_root, allowed_tools=GRANTS)) + assert supplied["allowed_tools"] == "- Bash(grep:*)\n- Bash(git show:*)" + assert supplied["harness_layer"] != today["harness_layer"] + + +# --------------------------------------------------------------------------- +# AC14: the harness layer +# --------------------------------------------------------------------------- + + +def test_the_harness_layer_carries_the_placeholder_and_the_sentence(): + """AC14: the granted list is injected, and the sentence is marked as + guidance rather than as the enforcement.""" + partial = (HARNESS_ROOT / HARNESS_LAYER_REL).read_text(encoding="utf-8") + assert "{{allowed_tools}}" in partial + assert "single command" in partial + assert "denied even when every command inside it is granted" in partial + assert "Guidance, not the enforcement" in partial + # It names no specific command: the list is the injected value. + assert "grep" not in partial + + +def test_the_rendered_harness_layer_shows_the_configured_grants(target_root): + """AC14: with grants supplied the layer shows them; without, it renders + None. The pair is each other's control.""" + supplied = context_assembler.build_context( + **_context(target_root, allowed_tools=GRANTS))["harness_layer"] + omitted = context_assembler.build_context( + **_context(target_root))["harness_layer"] + + assert "- Bash(grep:*)" in supplied + assert "- Bash(git show:*)" in supplied + assert "{{" not in supplied + assert "- Bash(grep:*)" not in omitted + assert "Bash commands granted to you without prompting:\nNone" in omitted + + +def test_the_stage_prompts_render_the_grants_through_the_shared_layer(target_root): + """The grants reach a stage, rather than only the partial. + + Which templates that is comes off the templates themselves — the ones + injecting {{harness_layer}} — rather than from a list written here, so a + template that stops injecting the shared block is not silently excused. + The verifier carries its own [Harness Layer] block instead, and it is + asserted below to be exactly the set that does not inject the partial.""" + context = context_assembler.build_context( + **_context(target_root, allowed_tools=GRANTS)) + injecting = [name for name in ("implementer.md", "tester.md", "verifier.md", + "documenter.md") + if "{{harness_layer}}" in context_assembler.load_template( + HARNESS_ROOT, name)] + assert injecting, "no stage template injects the shared harness layer" + for prompt_file in injecting: + rendered = context_assembler.render( + context_assembler.load_template(HARNESS_ROOT, prompt_file), context) + assert "{{" not in rendered, prompt_file + assert "- Bash(grep:*)" in rendered, prompt_file + + # The control for the selection: the templates left out are left out + # because they carry their own block, not because they render nothing. + for prompt_file in ("implementer.md", "tester.md", "verifier.md", + "documenter.md"): + rendered = context_assembler.render( + context_assembler.load_template(HARNESS_ROOT, prompt_file), context) + assert "[Harness Layer]" in rendered, prompt_file + + +def test_the_coordinator_passes_the_configs_grants_to_build_context(): + """The wiring the render depends on: without it the placeholder would be + injectable and never injected.""" + source = (HARNESS_ROOT / "orchestration" / "story_coordinator.py").read_text( + encoding="utf-8") + assert 'allowed_tools=config.get("allowed_tools")' in source + + +# --------------------------------------------------------------------------- +# AC1, AC2, AC15: the two allowlists +# --------------------------------------------------------------------------- + + +#: The twelve read-only prefixes AC1 names. +ADDED_READ_ONLY = ("grep", "rg", "find", "head", "tail", "wc", "sort", "uniq", + "diff", "git show", "git branch", "git ls-files") + +#: Entries specific to this repository, which the l5-init template does not +#: carry and AC15 explicitly allows for. +REPOSITORY_SPECIFIC = {"Bash(.venv/bin/python:*)", "Bash(python3:*)", + "Bash(chmod:*)"} + + +def allowed_tools_in(text: str) -> list[str]: + """The allowed_tools entries of a config's text, in order. + + A reader of text rather than of a directory, because one of the two configs + is a template and one of the readings is at a git bound; cross-checked + against harness_config.load_config below so it cannot drift from the parse + the harness itself uses. + """ + entries: list[str] = [] + collecting = False + for raw in text.splitlines(): + line = raw.split("#", 1)[0].rstrip() + if not line.strip(): + continue + if line.strip() == "allowed_tools:": + collecting = True + continue + if collecting and line.lstrip().startswith("- "): + entries.append(harness_config._unquote(line.strip()[2:].strip())) + continue + if collecting: + break + return entries + + +def test_this_modules_config_reader_agrees_with_the_harness_parse(): + """So every assertion below is about the config rather than about a second + parser written here.""" + assert allowed_tools_in((HARNESS_ROOT / CONFIG_REL).read_text( + encoding="utf-8")) == harness_config.load_config( + HARNESS_ROOT)["allowed_tools"] + + +@pytest.mark.parametrize("command", ADDED_READ_ONLY) +def test_the_config_grants_each_added_read_only_prefix(command): + """AC1.""" + assert f"Bash({command}:*)" in allowed_tools_in( + (HARNESS_ROOT / CONFIG_REL).read_text(encoding="utf-8")) + + +def test_no_existing_entry_was_removed_or_altered(): + """AC2, bounded at this story's own range: every entry the config carried + before is still there, unaltered and in its original order.""" + before = allowed_tools_in(repository_file_at( + CONFIG_REL, validation_file=VALIDATION_FILE, bound=BASELINE)) + today = allowed_tools_in((HARNESS_ROOT / CONFIG_REL).read_text( + encoding="utf-8")) + assert before, "the baseline config declares no grants, so this asserts nothing" + assert today[:len(before)] == before + + # The control: an entry dropped from the middle is reported by the same + # comparison, so "unchanged" is not something that holds for any list. + dropped = [entry for entry in today if entry != before[1]] + assert dropped[:len(before)] != before + + +def test_permission_mode_is_still_accept_edits(): + """AC2, at both ends of this story's range.""" + def mode(text: str) -> str: + return next(line.split(":", 1)[1].strip() + for line in text.splitlines() + if line.startswith("permission_mode:")) + + before = repository_file_at(CONFIG_REL, validation_file=VALIDATION_FILE, + bound=BASELINE) + today = (HARNESS_ROOT / CONFIG_REL).read_text(encoding="utf-8") + assert mode(today) == "acceptEdits" + assert mode(today) == mode(before) + + +def _writing_entries(entries: list[str], mutators, git_mutators) -> list[str]: + """The entries that name a command able to write, judged by the guard's own + tables rather than by a second list written here.""" + flagged = [] + for entry in entries: + inner = entry[len("Bash("):-len(":*)")] if entry.startswith("Bash(") else entry + words = inner.split() + if not words: + continue + name = words[0].rsplit("/", 1)[-1] + if name in mutators: + flagged.append(entry) + elif name == "git" and len(words) > 1 and words[1] in git_mutators: + flagged.append(entry) + return flagged + + +def test_no_granted_entry_names_a_command_that_writes(tmp_path): + """AC1's second half, for both configs, read entry by entry rather than + sampled — and judged against the guard's own mutator tables, so the two + halves of this story cannot disagree about what mutates.""" + guard = load_mutant(GUARD_PATH, [], name="bash_guard_tables", + tmp_path=tmp_path) + for relative in (CONFIG_REL, TEMPLATE_CONFIG_REL): + entries = allowed_tools_in( + (HARNESS_ROOT / relative).read_text(encoding="utf-8")) + assert entries, relative + assert _writing_entries(entries, guard.MUTATORS, + guard.GIT_MUTATORS) == [], relative + + # The control: a list carrying a writing grant is reported by the same + # check, so an empty result means "none present" rather than "none looked + # for". + assert _writing_entries( + ["Bash(grep:*)", "Bash(rm:*)", "Bash(git commit:*)"], + guard.MUTATORS, guard.GIT_MUTATORS) == ["Bash(rm:*)", "Bash(git commit:*)"] + + +def test_the_template_grants_the_same_read_only_set(): + """AC15: the two lists agree, allowing for this repository's own entries.""" + here = set(allowed_tools_in( + (HARNESS_ROOT / CONFIG_REL).read_text(encoding="utf-8"))) + template = set(allowed_tools_in( + (HARNESS_ROOT / TEMPLATE_CONFIG_REL).read_text(encoding="utf-8"))) + assert here - template == REPOSITORY_SPECIFIC + assert template - here == set() + for command in ADDED_READ_ONLY: + assert f"Bash({command}:*)" in template