From 0d62ac501dd6e9cc791cf4ec69603f71eac08ee2 Mon Sep 17 00:00:00 2001 From: tegwick Date: Mon, 27 Jul 2026 00:56:34 +0200 Subject: [PATCH] Review/optimize checklist, executive-summary format, build executor (T02-T04) docs/review-optimize-checklist.md: six checks (naming, TTL/scoping, redundancy, compaction, ease of use, posture), applied for real to the rein-openweights plan's section 4 -- including a genuinely useful finding (credentials.py already expects this exact path/delivery shape, zero code changes needed to consume it). docs/executive-summary-format.md: six fixed fields, no bao syntax, no restating earlier sections, explicit approve/reject/revise decision. Rendered for real into the plan's section 5 -- ready for an actual decision. src/ops_mason/{plan,executor,audit}.py: the phase-4 build executor for credential_type openbao-approle-kv. Refuses to run against anything but an approved plan -- verified the refusal never even calls subprocess.run. role_id/secret_id (the AppRole's own access credential, not the downstream secret) land as 0600 files, never logged; the HCL policy goes over stdin, never argv; the audit trail is metadata-only. 12 tests, all mocked at the bao boundary (no live OpenBao access from this session). Co-Authored-By: Claude Sonnet 5 --- .gitignore | 3 + docs/executive-summary-format.md | 55 ++++++++ docs/review-optimize-checklist.md | 63 +++++++++ plans/rein-openweights-openrouter-approle.md | 65 +++++++-- pyproject.toml | 22 +++ src/ops_mason/__init__.py | 0 src/ops_mason/audit.py | 48 +++++++ src/ops_mason/executor.py | 134 +++++++++++++++++++ src/ops_mason/plan.py | 56 ++++++++ tests/test_executor.py | 123 +++++++++++++++++ tests/test_plan.py | 51 +++++++ workplans/MASON-WP-0001-foundation.md | 37 ++++- 12 files changed, 645 insertions(+), 12 deletions(-) create mode 100644 docs/executive-summary-format.md create mode 100644 docs/review-optimize-checklist.md create mode 100644 pyproject.toml create mode 100644 src/ops_mason/__init__.py create mode 100644 src/ops_mason/audit.py create mode 100644 src/ops_mason/executor.py create mode 100644 src/ops_mason/plan.py create mode 100644 tests/test_executor.py create mode 100644 tests/test_plan.py diff --git a/.gitignore b/.gitignore index 5361415..345c765 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,6 @@ __pycache__/ .claude/* !.claude/rules/ !.claude/rules/*.md + +# Local runtime audit trail -- not repo content +audit/ diff --git a/docs/executive-summary-format.md b/docs/executive-summary-format.md new file mode 100644 index 0000000..8410728 --- /dev/null +++ b/docs/executive-summary-format.md @@ -0,0 +1,55 @@ +# Executive summary format (phase 3) + +The one mandatory human checkpoint (`INTENT.md` design principle 4). Lives +as section 5 of the plan file itself (`docs/construction-plan-format.md`) +— generated from sections 1-4, not a separate document that can drift +from the plan it's summarizing. + +## Why this shape + +Written so the founder never has to read a `bao` command, a policy JSON +blob, or an AppRole config to make the call. Six fixed fields, always in +this order, always answerable in one or two sentences each: + +1. **One-line ask.** What is being requested, in a sentence a non-expert + would understand. +2. **Who/what gets access.** The exact new principal (AppRole, role, + service identity) — and an explicit statement that nothing *else* + gains anything, if that's true (it almost always should be, per + `INTENT.md`'s reuse-before-creation principle). +3. **To what.** The exact path/policy/scope — narrow enough that a reader + can tell at a glance whether it's proportionate to the ask. +4. **For how long.** Token TTL and, separately, whether the underlying + AppRole credential itself expires or rotates — these are two different + questions and both need an answer, not just the token TTL. +5. **Blast radius if the credential leaks.** What could actually be done + with it — bounded to what section 3 grants, plus a note on whether the + downstream secret itself (e.g. a provider API key) carries its own + independent risk that this plan doesn't change either way. +6. **Cost to reverse.** How many commands, touching how many other + consumers, to undo this if it turns out wrong. If the answer is + "everything is isolated, reversing this touches nothing else," say so + explicitly — that's exactly the property `INTENT.md`'s + reuse-vs-new reasoning is supposed to produce. + +Ends with an explicit three-way decision: **approve**, **reject**, or +**send back to phase 1** with what to change. No plan should read as a +foregone conclusion — "send back" needs to be as easy to pick as +"approve." + +## What this format deliberately excludes + +- No `bao` command syntax, no policy HCL, no JSON. +- No restating sections 1-4 in full — the executive summary is a + compression of what's already there, findable by scrolling up in the + same file if more detail is wanted. +- No default recommendation baked into the rendering itself beyond what + the plan's own reuse-vs-new reasoning already produced — the summary + reports the plan's conclusion, it doesn't editorialize a second opinion + on top of it. + +## Worked example + +`plans/rein-openweights-openrouter-approle.md` section 5 — the real +executive summary for the first real plan, ready for an actual approve / +reject / revise decision. diff --git a/docs/review-optimize-checklist.md b/docs/review-optimize-checklist.md new file mode 100644 index 0000000..5e95a4c --- /dev/null +++ b/docs/review-optimize-checklist.md @@ -0,0 +1,63 @@ +# Review / optimize checklist (phase 2) + +Runs against a draft plan (`plans/.md`, section 2-3 filled in) before +it's shown to anyone. Fill in section 4 ("Review notes") of the plan file +directly — this checklist is not a separate artifact, it's what section 4 +*is*. A plan with an empty section 4 is not ready for phase 3. + +This can be run by a human or an agent going through the plan by hand — +it does not need to be automated tooling before it's useful. Automating +individual checks (e.g. a script that diffs proposed TTLs against +existing lanes) is a reasonable future step once there's more than one +worked example to generalize a checker from — not before (same +generalize-from-a-second-example discipline the `glas-harness` reins use +for their own deferred decisions). + +## The checks + +1. **Naming convention.** Does every new object's name follow the + `-` shape already visible in existing + lanes (`agent-harness-binky-mail`, `workload-kv-read-`)? A + plan proposing an unrelated naming scheme should be flagged, not + waved through for "this one's different." + +2. **TTL/scoping match.** Do proposed `token_ttl`/`token_max_ttl`/ + `token_num_uses`/`secret_id_ttl` values match an existing comparable + lane, or is there a stated reason to diverge? Silent divergence (a + shorter or longer TTL with no explanation) is a review finding, not a + detail to skip past. + +3. **Redundancy check.** Does a policy, AppRole, or KV path already exist + that overlaps what's being proposed? This should already have been + asked in section 2 (existing-structure survey) — phase 2 is the + second pass confirming that answer still holds after the proposed + changes are fully drafted, not skipping it because section 2 already + looked. + +4. **Compaction opportunity.** Does satisfying this demand make any + *existing* lane redundant or mergeable? Not every plan will find one — + most won't — but the check should be asked every time, not only when + it's obviously true. + +5. **Ease of use for the consumer.** Is the credential-acquisition path + this plan builds toward straightforward for the actual consuming code + to use (e.g. does it match an env-var/file-path convention the + consumer's code already expects, per its own `credentials.py` or + equivalent)? A technically-correct plan that's awkward for the + consumer to actually integrate is a real review finding. + +6. **Posture check.** Do the choices here (TTL length, rotation policy, + `secret_id_ttl` expiry-or-not) match current organizational posture + (`ops-warden/wiki/WorkloadSecurityPosture.md` — build phase today) — + not assuming production-tier rigor that isn't the current posture, and + not assuming dev-tier looseness forever. State the posture assumption + explicitly rather than leaving it implicit. + +## Worked example + +`plans/rein-openweights-openrouter-approle.md` section 4 applies all six +checks against a real plan — including one genuine finding (the TTL +values were carried over from `agent-harness-binky-mail` by direct +analogy rather than re-derived from scratch, which is exactly check 2 +working as intended: matching an existing lane rather than inventing a +new number). diff --git a/plans/rein-openweights-openrouter-approle.md b/plans/rein-openweights-openrouter-approle.md index 5497086..7fb74aa 100644 --- a/plans/rein-openweights-openrouter-approle.md +++ b/plans/rein-openweights-openrouter-approle.md @@ -3,7 +3,7 @@ id: rein-openweights-openrouter-approle demand_source: glas-harness/workplans/GLAS-WP-0002-T02 consumer_repo: rein-openweights credential_type: openbao-approle-kv -status: draft +status: reviewed approved_by: null approved_at: null created: "2026-07-27" @@ -97,12 +97,59 @@ as-is for its current interactive-caller consumers. - **Compaction opportunity:** none identified — nothing existing becomes redundant once this lands; the shared `activity-core` lane keeps its own consumers. -- **Open item carried into phase 3/4:** confirm current build-phase - posture (`ops-warden/wiki/WorkloadSecurityPosture.md`) still supports - `secret_id_ttl=0` (no expiry, matching `agent-harness-binky-mail`'s - choice) rather than a rotation schedule — note this explicitly in the - executive summary rather than assuming it silently. +- **Ease of use for the consumer:** matches directly — `credentials.py` + already reads `REIN_OPENWEIGHTS_APPROLE_DIR` (defaults to a + `role_id`/`secret_id` file pair) and a KV path override via + `REIN_OPENWEIGHTS_OPENROUTER_KV_PATH` (defaulting to exactly + `reins/rein-openweights/openrouter`, field `api_key`) — this plan's + path #1 and delivery #4 need **zero code changes** in + `rein-openweights` to consume; the code was already written expecting + this exact shape. +- **Posture check:** build phase (one founder-operator, pre-revenue) per + `ops-warden/wiki/WorkloadSecurityPosture.md` — `agent-harness-binky-mail` + used `secret_id_ttl=0` (no expiry) at this same posture; carrying that + forward here rather than adding a rotation schedule neither + `agent-harness-binky-mail` nor current posture requires. **Explicit + assumption, not a silent default** — flagged again in the executive + summary below for the approval decision to confirm or override. - +## 5. Executive summary (phase 3) + +**One-line ask:** create a narrowly-scoped, non-interactive credential +lane so `rein-openweights` can fetch its own OpenRouter API key at run +time without an operator present. + +**Who/what gets access:** a new OpenBao AppRole named `rein-openweights`. +Nothing else — no existing role, human, or service gains anything new. + +**To what:** read-only access to exactly one KV path, +`reins/rein-openweights/openrouter` (field `api_key`) — a path that does +not exist today and holds nothing else. The AppRole cannot read the +existing shared `activity-core`/`llm-connect` secret or any other path. + +**For how long:** each login is a 15-minute token, renewable up to a +30-minute cap, bounded number of uses per token — matches the existing +`agent-harness-binky-mail` lane exactly. The AppRole credential itself +(`role_id`/`secret_id`) does not expire on a schedule at current +(build-phase) posture, same choice already made for +`agent-harness-binky-mail`. + +**Blast radius if the credential leaks:** whoever holds a valid +`role_id`/`secret_id` pair can mint a short-lived token that reads one +OpenRouter API key — nothing else in OpenBao. Actual damage from the +OpenRouter key itself is real but bounded (API spend on that key, no +data-store or infra access) and independent of this plan (same exposure +`openrouter-llm-connect`'s existing consumers already carry). + +**Cost to reverse:** delete one AppRole, one policy, one KV path — three +`bao` commands, no other lane or consumer affected. Fully isolated +blast/reversal radius by construction (that was the point of declining +reuse in §2). + +**Decision needed:** approve as proposed, reject, or send back to phase 1 +with feedback (e.g. different TTL, different path name, or "actually +reuse the shared lane instead"). + +## 6. Build result (phase 4) + + diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..c1d805a --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,22 @@ +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[project] +name = "ops-mason" +version = "0.1.0" +description = "Builder of NetKingdom security infrastructure (AppRoles, policies, KV paths) for ops-warden to route to" +requires-python = ">=3.11" +dependencies = [ + "PyYAML>=6.0", +] + +[project.optional-dependencies] +dev = ["pytest>=8"] + +[tool.hatch.build.targets.wheel] +packages = ["src/ops_mason"] + +[tool.pytest.ini_options] +testpaths = ["tests"] +pythonpath = ["src"] diff --git a/src/ops_mason/__init__.py b/src/ops_mason/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/src/ops_mason/audit.py b/src/ops_mason/audit.py new file mode 100644 index 0000000..c69b06a --- /dev/null +++ b/src/ops_mason/audit.py @@ -0,0 +1,48 @@ +"""Metadata-only audit trail for what ops-mason built. + +Same invariant as ops-warden's own audit trail (`warden activity`): +object names and plan references only, never secret material. +""" + +from __future__ import annotations + +import json +from dataclasses import asdict, dataclass +from datetime import UTC, datetime +from pathlib import Path + +_DEFAULT_LOG = Path(__file__).resolve().parents[2] / "audit" / "build-log.jsonl" + + +@dataclass +class BuildRecord: + plan_id: str + plan_path: str + approved_by: str + approved_at: str + objects: dict[str, str] + built_at: str + + +def record_build( + *, + plan_id: str, + plan_path: str, + approved_by: str, + approved_at: str, + objects: dict[str, str], + log_path: Path | None = None, +) -> BuildRecord: + record = BuildRecord( + plan_id=plan_id, + plan_path=plan_path, + approved_by=approved_by, + approved_at=approved_at, + objects=objects, + built_at=datetime.now(UTC).isoformat(), + ) + path = log_path or _DEFAULT_LOG + path.parent.mkdir(parents=True, exist_ok=True) + with path.open("a") as fh: + fh.write(json.dumps(asdict(record)) + "\n") + return record diff --git a/src/ops_mason/executor.py b/src/ops_mason/executor.py new file mode 100644 index 0000000..f1c59be --- /dev/null +++ b/src/ops_mason/executor.py @@ -0,0 +1,134 @@ +"""Phase 4: execute an approved construction plan against OpenBao. + +Narrow on purpose (MASON-WP-0001-T04) — implements only the operations +the first real plan (rein-openweights-openrouter-approle, +credential_type: openbao-approle-kv) needs: a read-only policy scoped to +one KV path, an AppRole bound to it, and role_id/secret_id delivery. + +Never writes to the KV path itself and never touches the downstream +secret value (the OpenRouter API key). The KV path a policy references +comes into existence later, when the founder does a paste-once-provision +through ops-warden's desk (`bao kv put`) — not through this executor. +role_id/secret_id are the AppRole's own access credential, not the +downstream secret; generating and delivering them is squarely +ops-mason's job (INTENT.md: "create ... credentials, tokens"), but they +are still handled write-only — never logged, never returned in a +printable form beyond confirmation that delivery happened. +""" + +from __future__ import annotations + +import os +import subprocess +from dataclasses import dataclass, field +from pathlib import Path + +from ops_mason.audit import record_build +from ops_mason.plan import ConstructionPlan + + +class BuildRefused(RuntimeError): + """The plan is not approved. This is the one place a bug is a security incident.""" + + +class BuildError(RuntimeError): + """The plan was approved but a bao operation failed.""" + + +@dataclass +class AppRoleKVSpec: + policy_name: str + kv_path: str + approle_name: str + kv_capabilities: tuple[str, ...] = ("read",) + token_ttl: str = "15m" + token_max_ttl: str = "30m" + token_num_uses: int = 0 + secret_id_ttl: str = "0" + delivery_dir: Path | None = None + bao_bin: str = "bao" + audit_log_path: Path | None = None + + +def _run(bao_bin: str, args: list[str], input_text: str | None = None) -> str: + proc = subprocess.run( + [bao_bin, *args], + input=input_text, + capture_output=True, + text=True, + timeout=30, + ) + if proc.returncode != 0: + raise BuildError(f"`{bao_bin} {' '.join(args)}` failed: {proc.stderr.strip()[:300]}") + return proc.stdout + + +def _policy_hcl(kv_path: str, capabilities: tuple[str, ...]) -> str: + caps = ", ".join(f'"{c}"' for c in capabilities) + return f'path "{kv_path}" {{\n capabilities = [{caps}]\n}}\n' + + +def build_approle_kv_lane(plan: ConstructionPlan, spec: AppRoleKVSpec) -> dict[str, str]: + """Create the policy + AppRole for an `openbao-approle-kv` plan, deliver role_id/secret_id. + + Refuses unless plan.is_approved(). Returns object names only (no + secret material) and appends a metadata-only audit record. + """ + if not plan.is_approved(): + raise BuildRefused( + f"plan {plan.id!r} is not approved " + f"(status={plan.status!r}, approved_by={plan.approved_by!r}, " + f"approved_at={plan.approved_at!r}) — refusing to build" + ) + + policy_hcl = _policy_hcl(spec.kv_path, spec.kv_capabilities) + _run(spec.bao_bin, ["policy", "write", spec.policy_name, "-"], input_text=policy_hcl) + + _run( + spec.bao_bin, + [ + "write", + f"auth/approle/role/{spec.approle_name}", + f"token_policies={spec.policy_name}", + f"token_ttl={spec.token_ttl}", + f"token_max_ttl={spec.token_max_ttl}", + f"token_num_uses={spec.token_num_uses}", + f"secret_id_ttl={spec.secret_id_ttl}", + ], + ) + + role_id = _run( + spec.bao_bin, ["read", "-field=role_id", f"auth/approle/role/{spec.approle_name}/role-id"] + ).strip() + secret_id = _run( + spec.bao_bin, + ["write", "-field=secret_id", "-f", f"auth/approle/role/{spec.approle_name}/secret-id"], + ).strip() + + delivered_to = "" + if spec.delivery_dir is not None: + spec.delivery_dir.mkdir(parents=True, exist_ok=True) + os.chmod(spec.delivery_dir, 0o700) + role_id_path = spec.delivery_dir / "role_id" + secret_id_path = spec.delivery_dir / "secret_id" + role_id_path.write_text(role_id + "\n") + secret_id_path.write_text(secret_id + "\n") + os.chmod(role_id_path, 0o600) + os.chmod(secret_id_path, 0o600) + delivered_to = str(spec.delivery_dir) + + objects = { + "policy_name": spec.policy_name, + "approle_name": spec.approle_name, + "kv_path": spec.kv_path, + "delivered_to": delivered_to, + } + record_build( + plan_id=plan.id, + plan_path=str(plan.path), + approved_by=plan.approved_by or "", + approved_at=plan.approved_at or "", + objects=objects, + log_path=spec.audit_log_path, + ) + return objects diff --git a/src/ops_mason/plan.py b/src/ops_mason/plan.py new file mode 100644 index 0000000..83ad6b9 --- /dev/null +++ b/src/ops_mason/plan.py @@ -0,0 +1,56 @@ +"""Parse a construction plan file's YAML frontmatter. + +See docs/construction-plan-format.md. The frontmatter's status field is +the phase-4 hard gate: the build executor (executor.py) refuses to run +against anything but status: approved with both approved_by/approved_at +set. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +import yaml + + +class PlanError(RuntimeError): + pass + + +@dataclass +class ConstructionPlan: + id: str + status: str + approved_by: str | None + approved_at: str | None + demand_source: str + consumer_repo: str + credential_type: str + path: Path + + @classmethod + def load(cls, path: str | Path) -> ConstructionPlan: + path = Path(path) + text = path.read_text() + parts = text.split("---", 2) + if len(parts) < 3 or not text.startswith("---"): + raise PlanError(f"{path}: missing YAML frontmatter") + data: dict[str, Any] = yaml.safe_load(parts[1]) or {} + missing = {"id", "status"} - set(data) + if missing: + raise PlanError(f"{path}: frontmatter missing field(s): {', '.join(sorted(missing))}") + return cls( + id=data["id"], + status=data["status"], + approved_by=data.get("approved_by"), + approved_at=data.get("approved_at"), + demand_source=data.get("demand_source", ""), + consumer_repo=data.get("consumer_repo", ""), + credential_type=data.get("credential_type", ""), + path=path, + ) + + def is_approved(self) -> bool: + return self.status == "approved" and bool(self.approved_by) and bool(self.approved_at) diff --git a/tests/test_executor.py b/tests/test_executor.py new file mode 100644 index 0000000..f2e2d2e --- /dev/null +++ b/tests/test_executor.py @@ -0,0 +1,123 @@ +import json +from unittest.mock import MagicMock, patch + +import pytest + +from ops_mason.executor import AppRoleKVSpec, BuildError, BuildRefused, build_approle_kv_lane +from ops_mason.plan import ConstructionPlan + + +def _plan(tmp_path, *, status="approved", approved_by="bernd", approved_at="2026-07-27"): + text = ( + "---\n" + "id: test-lane\n" + f"status: {status}\n" + + (f'approved_by: "{approved_by}"\n' if approved_by else "approved_by: null\n") + + (f'approved_at: "{approved_at}"\n' if approved_at else "approved_at: null\n") + + "---\n# Plan\n" + ) + p = tmp_path / "plan.md" + p.write_text(text) + return ConstructionPlan.load(str(p)) + + +def _spec(tmp_path) -> AppRoleKVSpec: + return AppRoleKVSpec( + policy_name="workload-kv-read-test-lane", + kv_path="reins/test/openrouter", + approle_name="test-lane", + delivery_dir=tmp_path / "delivery", + audit_log_path=tmp_path / "audit.jsonl", + ) + + +def test_refuses_when_not_approved(tmp_path) -> None: + plan = _plan(tmp_path, status="draft", approved_by=None, approved_at=None) + with pytest.raises(BuildRefused, match="not approved"): + build_approle_kv_lane(plan, _spec(tmp_path)) + + +def test_refuses_when_approved_status_but_missing_approver(tmp_path) -> None: + plan = _plan(tmp_path, status="approved", approved_by=None, approved_at="2026-07-27") + with pytest.raises(BuildRefused, match="not approved"): + build_approle_kv_lane(plan, _spec(tmp_path)) + + +def test_refusal_never_calls_bao(tmp_path) -> None: + plan = _plan(tmp_path, status="reviewed", approved_by=None, approved_at=None) + with patch("ops_mason.executor.subprocess.run") as run: + with pytest.raises(BuildRefused): + build_approle_kv_lane(plan, _spec(tmp_path)) + run.assert_not_called() + + +def test_build_writes_policy_approle_and_delivers_credentials(tmp_path) -> None: + plan = _plan(tmp_path) + spec = _spec(tmp_path) + + def fake_run(cmd, input=None, capture_output=True, text=True, timeout=30): + result = MagicMock(returncode=0, stderr="") + if cmd[1:3] == ["read", "-field=role_id"]: + result.stdout = "role-id-value\n" + elif "-field=secret_id" in cmd: + result.stdout = "secret-id-value\n" + else: + result.stdout = "" + return result + + with patch("ops_mason.executor.subprocess.run", side_effect=fake_run) as run: + objects = build_approle_kv_lane(plan, spec) + + assert objects["policy_name"] == "workload-kv-read-test-lane" + assert objects["approle_name"] == "test-lane" + assert objects["kv_path"] == "reins/test/openrouter" + + role_id_file = spec.delivery_dir / "role_id" + secret_id_file = spec.delivery_dir / "secret_id" + assert role_id_file.read_text() == "role-id-value\n" + assert secret_id_file.read_text() == "secret-id-value\n" + assert oct(role_id_file.stat().st_mode)[-3:] == "600" + assert oct(secret_id_file.stat().st_mode)[-3:] == "600" + + # policy write call carried the HCL on stdin, not argv -- never in a log line + policy_call = next(c for c in run.call_args_list if c.args[0][:2] == ["bao", "policy"]) + assert "reins/test/openrouter" in policy_call.kwargs["input"] + + # audit record landed at the explicit path, metadata only, no role_id/secret_id values + audit_text = spec.audit_log_path.read_text() + assert "policy_name" in audit_text + assert "role-id-value" not in audit_text + assert "secret-id-value" not in audit_text + + +def test_build_appends_audit_record(tmp_path) -> None: + plan = _plan(tmp_path) + spec = _spec(tmp_path) + + def fake_run(cmd, input=None, capture_output=True, text=True, timeout=30): + result = MagicMock(returncode=0, stderr="") + result.stdout = "value\n" + return result + + with ( + patch("ops_mason.executor.subprocess.run", side_effect=fake_run), + patch("ops_mason.executor.record_build") as record_build_mock, + ): + build_approle_kv_lane(plan, spec) + + record_build_mock.assert_called_once() + kwargs = record_build_mock.call_args.kwargs + assert kwargs["plan_id"] == "test-lane" + assert kwargs["approved_by"] == "bernd" + assert "policy_name" in kwargs["objects"] + assert kwargs["log_path"] == spec.audit_log_path + + +def test_bao_failure_raises_build_error(tmp_path) -> None: + plan = _plan(tmp_path) + spec = _spec(tmp_path) + + fake_result = MagicMock(returncode=1, stderr="permission denied", stdout="") + with patch("ops_mason.executor.subprocess.run", return_value=fake_result): + with pytest.raises(BuildError, match="permission denied"): + build_approle_kv_lane(plan, spec) diff --git a/tests/test_plan.py b/tests/test_plan.py new file mode 100644 index 0000000..35a9e4b --- /dev/null +++ b/tests/test_plan.py @@ -0,0 +1,51 @@ +import pytest + +from ops_mason.plan import ConstructionPlan, PlanError + + +def _write(tmp_path, frontmatter: str, body: str = "\n# Plan\n") -> str: + p = tmp_path / "plan.md" + p.write_text(f"---\n{frontmatter}\n---\n{body}") + return str(p) + + +def test_load_parses_frontmatter(tmp_path) -> None: + path = _write( + tmp_path, + "id: test-plan\nstatus: draft\ndemand_source: x\nconsumer_repo: y\ncredential_type: openbao-approle-kv", + ) + plan = ConstructionPlan.load(path) + assert plan.id == "test-plan" + assert plan.status == "draft" + assert plan.consumer_repo == "y" + + +def test_is_approved_false_when_draft(tmp_path) -> None: + path = _write(tmp_path, "id: p\nstatus: draft") + assert ConstructionPlan.load(path).is_approved() is False + + +def test_is_approved_false_when_approved_but_missing_approver(tmp_path) -> None: + path = _write(tmp_path, "id: p\nstatus: approved") + assert ConstructionPlan.load(path).is_approved() is False + + +def test_is_approved_true_when_fully_approved(tmp_path) -> None: + path = _write( + tmp_path, + 'id: p\nstatus: approved\napproved_by: "bernd"\napproved_at: "2026-07-27"', + ) + assert ConstructionPlan.load(path).is_approved() is True + + +def test_missing_frontmatter_raises(tmp_path) -> None: + p = tmp_path / "plan.md" + p.write_text("# no frontmatter here\n") + with pytest.raises(PlanError, match="missing YAML frontmatter"): + ConstructionPlan.load(str(p)) + + +def test_missing_required_field_raises(tmp_path) -> None: + path = _write(tmp_path, "id: p\n# status omitted") + with pytest.raises(PlanError, match="missing field"): + ConstructionPlan.load(path) diff --git a/workplans/MASON-WP-0001-foundation.md b/workplans/MASON-WP-0001-foundation.md index c99e8b2..669b598 100644 --- a/workplans/MASON-WP-0001-foundation.md +++ b/workplans/MASON-WP-0001-foundation.md @@ -57,9 +57,20 @@ note any compaction opportunity (two existing lanes that could merge). This can start as a checklist a human/agent runs manually against a draft plan — doesn't need to be automated tooling on day one. +**Done (2026-07-27).** `docs/review-optimize-checklist.md`: six checks +(naming, TTL/scoping, redundancy, compaction, ease of use for the +consumer, posture) — manual for now, automating a checker deferred until +there's a second worked example to generalize from. Applied for real to +`plans/rein-openweights-openrouter-approle.md` §4 — including a +genuinely useful finding (`credentials.py` already expects exactly this +plan's path/delivery shape, zero code changes needed to consume it) and +one explicit posture assumption flagged rather than left silent +(`secret_id_ttl=0`, carried from `agent-harness-binky-mail`, surfaced +again in the executive summary for the approval decision to confirm). + ```task id: MASON-WP-0001-T02 -status: todo +status: done priority: high state_hub_task_id: "58a446fb-78d0-42b4-b10a-1119266b1016" ``` @@ -74,9 +85,17 @@ be readable without knowing `bao` syntax. Document in `docs/executive-summary-format.md`, with the rein-openweights AppRole plan (task T05) as the worked example. +**Done (2026-07-27).** `docs/executive-summary-format.md`: six fixed +fields (one-line ask, who/what gets access, to what, for how long, +blast radius, cost to reverse) plus an explicit approve/reject/revise +decision — no `bao` syntax, no restating sections 1-4, no editorializing +beyond the plan's own reuse-vs-new conclusion. Rendered for real into +`plans/rein-openweights-openrouter-approle.md` §5 — ready for an actual +decision, not a mockup. + ```task id: MASON-WP-0001-T03 -status: todo +status: done priority: medium state_hub_task_id: "e37debfd-8a00-4780-b036-9376c6a10557" ``` @@ -98,9 +117,21 @@ the one place a bug is a real security incident, not a bad UX. Start narrow: implement only the operations the first real plan (T05) needs, not a general OpenBao automation framework. +**Done (2026-07-27).** `src/ops_mason/plan.py` (frontmatter parser + +`is_approved()`), `src/ops_mason/executor.py` +(`build_approle_kv_lane` — policy write, AppRole create, +role_id/secret_id delivery, never the KV path's value), `src/ops_mason/audit.py` +(metadata-only JSONL build log). 12 tests, all mocked at the `bao` +subprocess boundary (no live OpenBao access from this session — see +`GLAS-WP-0002-T02`'s original blocker). Explicitly verified: the refusal +gate never even calls `subprocess.run` when the plan isn't fully +approved (`test_refusal_never_calls_bao`); role_id/secret_id land as +`0600` files and never appear in a log line or exception message; the +policy HCL goes over stdin, never argv. + ```task id: MASON-WP-0001-T04 -status: todo +status: done priority: high state_hub_task_id: "e949f4b7-5ef4-4e42-8a07-61b6c8040298" ```