From c0d476e8d3dfee8563d79f52e6e781b43ba58e22 Mon Sep 17 00:00:00 2001 From: howlcipher Date: Wed, 12 Aug 2026 16:54:53 -0400 Subject: [PATCH 1/6] Add RealTerraformTool: real fmt/init/validate/plan via subprocess Shells out to a real terraform binary for fmt/init/validate/plan only -- apply/destroy stay on SimulatedTerraformTool until Milestone 3. Adds tools/_subprocess_safety.py for explicit timeouts and secret redaction on every real subprocess call, since state/auth output can contain secrets. Plan parsing extracts only {address, action} from terraform show -json, never raw attribute values, feeding the existing risk classifier unchanged. Also renders LearningMoment.related_artifact, which had no real caller before this milestone. Co-Authored-By: Claude Sonnet 5 --- src/devops_learn/explanations/service.py | 2 + src/devops_learn/tools/_subprocess_safety.py | 86 ++++++ src/devops_learn/tools/terraform_tool.py | 281 ++++++++++++++++++- tests/explanations/test_service.py | 49 ++++ tests/fixtures/fake_terraform/terraform | 130 +++++++++ tests/tools/test_real_terraform_tool.py | 189 +++++++++++++ tests/tools/test_subprocess_safety.py | 52 ++++ 7 files changed, 785 insertions(+), 4 deletions(-) create mode 100644 src/devops_learn/tools/_subprocess_safety.py create mode 100644 tests/explanations/test_service.py create mode 100755 tests/fixtures/fake_terraform/terraform create mode 100644 tests/tools/test_real_terraform_tool.py create mode 100644 tests/tools/test_subprocess_safety.py diff --git a/src/devops_learn/explanations/service.py b/src/devops_learn/explanations/service.py index fb921e4..6654109 100644 --- a/src/devops_learn/explanations/service.py +++ b/src/devops_learn/explanations/service.py @@ -48,4 +48,6 @@ def render_learning_moment( lines.append(moment.why_it_matters) if depth >= ExplanationDepth.DEEP: lines.append(moment.deep_explanation) + if moment.related_artifact and depth >= ExplanationDepth.LEARNING: + lines.append(f"See: {moment.related_artifact}") return "\n".join(lines) diff --git a/src/devops_learn/tools/_subprocess_safety.py b/src/devops_learn/tools/_subprocess_safety.py new file mode 100644 index 0000000..f5d8e97 --- /dev/null +++ b/src/devops_learn/tools/_subprocess_safety.py @@ -0,0 +1,86 @@ +"""Shared subprocess-safety helpers for Real*Tool implementations. + +Extracted because RealTerraformTool needs both an explicit timeout and output +redaction, neither of which RealDockerTool/RealPythonTool have today: those +two tools never set a subprocess timeout and put stdout/stderr into +ToolResult.details verbatim. That gap matters more for Terraform, since +provider auth errors and init/plan output can contain secret-shaped values. +Deliberately generic (no Terraform-specific knowledge) so those two tools +could adopt it later; retrofitting them is out of scope for this milestone. +""" + +from __future__ import annotations + +import re +import subprocess +from dataclasses import dataclass + +DEFAULT_TIMEOUT_SECONDS = 120 +MAX_OUTPUT_CHARS = 20_000 + +_SECRET_PATTERN = re.compile( + r"(?i)\b([A-Z0-9_]*(?:SECRET|TOKEN|PASSWORD|API_KEY)[A-Z0-9_]*)\s*[=:]\s*(\S+)" +) + + +@dataclass(frozen=True) +class SafeRunResult: + returncode: int + stdout: str + stderr: str + timed_out: bool + + +def redact(text: str) -> str: + """Masks KEY=value-shaped secrets in free text. + + Defense in depth only. The primary defense against leaking sensitive + Terraform data is never extracting attribute values from structured + output in the first place -- see the module docstring in + tools/terraform_tool.py for why plan parsing never touches + resource_changes[].change.before/after. + """ + return _SECRET_PATTERN.sub(lambda m: f"{m.group(1)}=", text) + + +def _truncate(text: str) -> str: + if len(text) <= MAX_OUTPUT_CHARS: + return text + hidden = len(text) - MAX_OUTPUT_CHARS + return text[:MAX_OUTPUT_CHARS] + f"\n... [truncated, {hidden} more characters] ...\n" + + +def _sanitize(text: str) -> str: + return _truncate(redact(text)) + + +def run_safely( + command: list[str], *, cwd: str | None, timeout: int = DEFAULT_TIMEOUT_SECONDS +) -> SafeRunResult: + """Runs `command` as an argument array (never shell=True) with a timeout, + and always redacts + truncates stdout/stderr before returning, so a call + site can never forget to sanitize output before it reaches a ToolResult. + """ + try: + completed = subprocess.run( + command, + cwd=cwd, + capture_output=True, + text=True, + check=False, + timeout=timeout, + ) + except subprocess.TimeoutExpired as exc: + partial_stdout = exc.stdout if isinstance(exc.stdout, str) else "" + return SafeRunResult( + returncode=-1, + stdout=_sanitize(partial_stdout), + stderr=_sanitize(f"Command timed out after {timeout}s: {' '.join(command)}"), + timed_out=True, + ) + return SafeRunResult( + returncode=completed.returncode, + stdout=_sanitize(completed.stdout), + stderr=_sanitize(completed.stderr), + timed_out=False, + ) diff --git a/src/devops_learn/tools/terraform_tool.py b/src/devops_learn/tools/terraform_tool.py index 330b3dd..4d65bea 100644 --- a/src/devops_learn/tools/terraform_tool.py +++ b/src/devops_learn/tools/terraform_tool.py @@ -1,16 +1,32 @@ -"""Simulated TerraformTool. No real terraform binary invoked in V1; see docs/safety.md. +"""TerraformTool implementations: simulated (no binary invoked, used by +`analyze`/`review`) and real (used by `devops-learn terraform`). -plan() derives its resource count from templates/terraform/main.tf.reference -instead of a hardcoded string, so the learner's interpretation question stays -meaningful and the reference config can change without a second edit site. +SimulatedTerraformTool.plan() derives its resource count from +templates/terraform/main.tf.reference instead of a hardcoded string, so the +learner's interpretation question stays meaningful and the reference config +can change without a second edit site. + +RealTerraformTool shells out to a real `terraform` binary for fmt/init/ +validate/plan only -- see its class docstring for why apply/destroy are +deliberately not declared here. Its plan parsing never extracts raw resource +attribute values (before/after) from `terraform show -json`, only +{address, action} pairs: this is the primary defense against leaking +sensitive plan data, since Terraform's own "sensitive" markers can vary by +provider/version and are not a defense to rely on alone. Free-text +stdout/stderr from every real subprocess call is still redacted and +truncated via tools/_subprocess_safety.py as defense in depth. """ from __future__ import annotations +import json import re +import shutil +import tempfile from pathlib import Path from typing import Any, Mapping +from devops_learn.tools import _subprocess_safety from devops_learn.tools.approval import ApprovalRecord from devops_learn.tools.base import RiskLevel, Tool, ToolOperationSpec, ToolResult @@ -126,3 +142,260 @@ def execute( was_dry_run=dry_run, approval=approval, ) + + +_REAL_OPERATIONS = ( + ToolOperationSpec( + name="fmt", + risk_level=RiskLevel.SAFE, + supports_dry_run=False, + requires_approval=False, + is_destructive=False, + ), + ToolOperationSpec( + name="init", + risk_level=RiskLevel.SAFE, + supports_dry_run=False, + requires_approval=False, + is_destructive=False, + ), + ToolOperationSpec( + name="validate", + risk_level=RiskLevel.SAFE, + supports_dry_run=False, + requires_approval=False, + is_destructive=False, + ), + ToolOperationSpec( + name="plan", + risk_level=RiskLevel.SAFE, + supports_dry_run=False, + requires_approval=False, + is_destructive=False, + ), +) + + +def _terraform_command() -> str: + command = shutil.which("terraform") + if command is None: + raise FileNotFoundError( + "Terraform CLI not found. Install Terraform or use simulation mode." + ) + return command + + +def _details_from_plan_json(plan_json: Mapping[str, Any]) -> dict[str, Any]: + """Extracts only {address, action} pairs from a real `terraform show -json` + plan -- never the resource_changes[].change.before/after attribute maps, + which can contain sensitive values. See the module docstring.""" + create = change = replace = destroy = 0 + resources: list[dict[str, str]] = [] + for resource_change in plan_json.get("resource_changes", []): + actions = tuple(resource_change.get("change", {}).get("actions", ())) + if actions == ("create",): + create += 1 + action = "create" + elif actions == ("update",): + change += 1 + action = "update" + elif actions in (("delete", "create"), ("create", "delete")): + replace += 1 + action = "replace" + elif actions == ("delete",): + destroy += 1 + action = "destroy" + else: + continue # ("no-op",) / ("read",): not a change + address = str(resource_change.get("address", "unknown")) + resources.append({"address": address, "action": action}) + return { + "create": create, + "change": change, + "replace": replace, + "destroy": destroy, + "resources": resources, + } + + +class RealTerraformTool(Tool): + """Runs real terraform fmt/init/validate/plan via subprocess. + + Deliberately does not declare apply_approved_plan or + destroy_approved_environment: those stay simulated until Milestone 3, per + docs/roadmap.md. A caller invoking either operation on this tool gets a + KeyError from Tool.spec_for rather than a result that looks real but + silently isn't -- apply/destroy remain real capabilities of the + "terraform" tool name only via SimulatedTerraformTool, exactly how + cli/main.py already selects real-vs-simulated per command today. See + docs/adr/0004-controlled-tool-execution.md. + """ + + @property + def name(self) -> str: + return "terraform" + + @property + def operations(self) -> tuple[ToolOperationSpec, ...]: + return _REAL_OPERATIONS + + def execute( + self, + operation: str, + params: Mapping[str, Any], + *, + dry_run: bool, + approval: ApprovalRecord | None, + ) -> ToolResult: + spec = self.spec_for(operation) + assert not spec.requires_approval or dry_run or (approval is not None and approval.granted) + + if dry_run: + return ToolResult( + success=True, + summary=f"Would run terraform {operation} (dry run)", + details={"dry_run": True}, + risk_level=spec.risk_level, + was_dry_run=dry_run, + approval=approval, + ) + + working_dir = params.get("path") + timeout_override = params.get("timeout_seconds") + + def _timeout(default: int) -> int: + return int(timeout_override) if timeout_override is not None else default + + try: + terraform = _terraform_command() + if operation == "fmt": + result = _subprocess_safety.run_safely( + [terraform, "fmt", "-check", "-diff", "-no-color"], + cwd=working_dir, + timeout=_timeout(30), + ) + success = result.returncode == 0 + summary = "Configuration already formatted" if success else ( + "Configuration needs formatting" + ) + details: dict[str, Any] = { + "returncode": result.returncode, + "stdout": result.stdout, + "stderr": result.stderr, + } + elif operation == "init": + result = _subprocess_safety.run_safely( + [terraform, "init", "-input=false", "-no-color"], + cwd=working_dir, + timeout=_timeout(120), + ) + success = result.returncode == 0 + success_lines = [ + line + for line in result.stdout.strip().splitlines() + if "successfully initialized" in line.lower() + ] + if success_lines: + summary = success_lines[0].strip() + elif success: + summary = "Terraform has been initialized" + else: + summary = "terraform init failed" + details = { + "returncode": result.returncode, + "stdout": result.stdout, + "stderr": result.stderr, + } + elif operation == "validate": + result = _subprocess_safety.run_safely( + [terraform, "validate", "-json"], cwd=working_dir, timeout=_timeout(30) + ) + try: + parsed = json.loads(result.stdout) if result.stdout else {} + except json.JSONDecodeError: + parsed = {} + diagnostics = parsed.get("diagnostics", []) + success = result.returncode == 0 and bool(parsed.get("valid", False)) + summary = ( + "Success! The configuration is valid." + if success + else f"Validation failed: {len(diagnostics)} diagnostic(s)" + ) + details = { + "returncode": result.returncode, + "valid": parsed.get("valid"), + "diagnostic_count": len(diagnostics), + "stderr": result.stderr, + } + else: # plan + with tempfile.TemporaryDirectory() as tmp_dir: + plan_path = str(Path(tmp_dir) / "tfplan.out") + plan_result = _subprocess_safety.run_safely( + [terraform, "plan", "-input=false", "-no-color", f"-out={plan_path}"], + cwd=working_dir, + timeout=_timeout(180), + ) + if plan_result.returncode != 0: + return ToolResult( + success=False, + summary="(real, failed) terraform plan failed", + details={ + "returncode": plan_result.returncode, + "stdout": plan_result.stdout, + "stderr": plan_result.stderr, + }, + risk_level=spec.risk_level, + was_dry_run=dry_run, + approval=approval, + ) + show_result = _subprocess_safety.run_safely( + [terraform, "show", "-json", plan_path], cwd=working_dir, timeout=30 + ) + if show_result.returncode != 0: + return ToolResult( + success=False, + summary="(real, failed) terraform show -json failed", + details={ + "returncode": show_result.returncode, + "stderr": show_result.stderr, + }, + risk_level=spec.risk_level, + was_dry_run=dry_run, + approval=approval, + ) + try: + plan_json = json.loads(show_result.stdout) + except json.JSONDecodeError: + return ToolResult( + success=False, + summary="(real, failed) Could not parse terraform show -json output", + details={"error": "malformed JSON from terraform show -json"}, + risk_level=spec.risk_level, + was_dry_run=dry_run, + approval=approval, + ) + details = _details_from_plan_json(plan_json) + success = True + summary = ( + f"Plan: {details['create']} to add, {details['change']} to change, " + f"{details['replace']} to replace, {details['destroy']} to destroy." + ) + except (OSError, FileNotFoundError) as exc: + return ToolResult( + success=False, + summary=f"(real, failed) {exc}", + details={"error": str(exc)}, + risk_level=spec.risk_level, + was_dry_run=dry_run, + approval=approval, + ) + + prefix = "(real) " if success else "(real, failed) " + return ToolResult( + success=success, + summary=prefix + summary, + details=details, + risk_level=spec.risk_level, + was_dry_run=dry_run, + approval=approval, + ) diff --git a/tests/explanations/test_service.py b/tests/explanations/test_service.py new file mode 100644 index 0000000..e69d3ef --- /dev/null +++ b/tests/explanations/test_service.py @@ -0,0 +1,49 @@ +from devops_learn.domain.enums import ExecutionMode, ExplanationDepth +from devops_learn.domain.explanation_models import LearningMoment +from devops_learn.explanations.service import ExplanationService + + +def _moment(**overrides: object) -> LearningMoment: + defaults: dict[str, object] = dict( + concept="Terraform state", + trigger="before init", + summary="State tracks what Terraform believes exists.", + deep_explanation="Full detail about state internals.", + why_it_matters="Understanding state matters for trustworthy plans.", + related_artifact="docs/terraform-state.md", + ) + defaults.update(overrides) + return LearningMoment(**defaults) # type: ignore[arg-type] + + +def test_related_artifact_is_rendered_at_learning_depth() -> None: + rendered = ExplanationService().render_learning_moment( + _moment(), mode=ExecutionMode.COLLABORATIVE, depth=ExplanationDepth.LEARNING + ) + assert rendered is not None + assert "docs/terraform-state.md" in rendered + + +def test_related_artifact_is_omitted_below_learning_depth() -> None: + rendered = ExplanationService().render_learning_moment( + _moment(), mode=ExecutionMode.COLLABORATIVE, depth=ExplanationDepth.NORMAL + ) + assert rendered is not None + assert "docs/terraform-state.md" not in rendered + + +def test_related_artifact_absent_when_not_set() -> None: + rendered = ExplanationService().render_learning_moment( + _moment(related_artifact=None), + mode=ExecutionMode.COLLABORATIVE, + depth=ExplanationDepth.DEEP, + ) + assert rendered is not None + assert "See:" not in rendered + + +def test_learning_moment_suppressed_in_autonomous_mode_below_deep_depth() -> None: + rendered = ExplanationService().render_learning_moment( + _moment(), mode=ExecutionMode.AUTONOMOUS, depth=ExplanationDepth.LEARNING + ) + assert rendered is None diff --git a/tests/fixtures/fake_terraform/terraform b/tests/fixtures/fake_terraform/terraform new file mode 100755 index 0000000..329d163 --- /dev/null +++ b/tests/fixtures/fake_terraform/terraform @@ -0,0 +1,130 @@ +#!/usr/bin/env python3 +"""A fake `terraform` binary for tests/tools/test_real_terraform_tool.py. + +Behavior is selected by the FAKE_TERRAFORM_SCENARIO env var so a single +script covers every scenario instead of one file per case. Never touches +real infrastructure; only prints canned output and writes a placeholder +plan file when asked to `-out=`. +""" +from __future__ import annotations + +import json +import os +import sys +import time + +SCENARIO = os.environ.get("FAKE_TERRAFORM_SCENARIO", "success") + + +def _plan_out_path(args: list[str]) -> str | None: + for arg in args: + if arg.startswith("-out="): + return arg[len("-out="):] + return None + + +def main() -> int: + args = sys.argv[1:] + if not args: + return 1 + subcommand = args[0] + + if SCENARIO == "timeout": + time.sleep(5) + return 0 + + if subcommand == "fmt": + if SCENARIO == "fmt_needs_formatting": + print("main.tf\n+ location = \"eastus\"", file=sys.stdout) + return 3 + return 0 + + if subcommand == "init": + if SCENARIO == "init_failure": + print("Error: Failed to install provider", file=sys.stderr) + return 1 + print("Terraform has been successfully initialized!") + return 0 + + if subcommand == "validate": + if SCENARIO == "validate_failure": + print(json.dumps({ + "valid": False, + "diagnostics": [{"severity": "error", "summary": "Missing required argument"}], + })) + return 1 + print(json.dumps({"valid": True, "diagnostics": []})) + return 0 + + if subcommand == "plan": + out_path = _plan_out_path(args) + if SCENARIO == "plan_failure_with_secret": + # Mirrors real `terraform plan` output observed with no Azure credentials: + # a genuine "az login"/"Authorizer" auth failure, with a secret-shaped + # value mixed in to also exercise redaction. + print( + "Error: building AzureRM Client: obtain a token using " + "ARM_CLIENT_SECRET=super-secret-value-123 failed: could not configure " + "AzureCli Authorizer: obtaining tenant ID: running Azure CLI: exit status 1: " + "ERROR: Please run 'az login' to setup account.", + file=sys.stderr, + ) + return 1 + if out_path: + with open(out_path, "wb") as handle: + handle.write(b"fake-plan-binary-content") + return 0 + + if subcommand == "show": + if SCENARIO == "show_json_malformed": + print("{not valid json") + return 0 + if SCENARIO == "plan_with_sensitive_attribute": + print(json.dumps({ + "resource_changes": [ + { + "address": "azurerm_resource_group.main", + "change": {"actions": ["create"]}, + }, + { + "address": "azurerm_key_vault_secret.admin", + "change": { + "actions": ["create"], + "after": {"admin_password": "hunter2"}, + }, + }, + ] + })) + return 0 + if SCENARIO == "plan_mixed_actions": + print(json.dumps({ + "resource_changes": [ + {"address": "azurerm_resource_group.main", + "change": {"actions": ["create"]}}, + {"address": "azurerm_container_registry.main", + "change": {"actions": ["update"]}}, + {"address": "azurerm_log_analytics_workspace.main", + "change": {"actions": ["delete", "create"]}}, + {"address": "azurerm_storage_account.old", + "change": {"actions": ["delete"]}}, + {"address": "azurerm_resource_group.untouched", + "change": {"actions": ["no-op"]}}, + ] + })) + return 0 + # default "success" scenario: two creates + print(json.dumps({ + "resource_changes": [ + {"address": "azurerm_resource_group.main", + "change": {"actions": ["create"]}}, + {"address": "azurerm_container_registry.main", + "change": {"actions": ["create"]}}, + ] + })) + return 0 + + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/tools/test_real_terraform_tool.py b/tests/tools/test_real_terraform_tool.py new file mode 100644 index 0000000..24ec66a --- /dev/null +++ b/tests/tools/test_real_terraform_tool.py @@ -0,0 +1,189 @@ +import json +import os +from pathlib import Path + +import pytest + +from devops_learn.tools.base import RiskLevel +from devops_learn.tools.terraform_tool import RealTerraformTool +from devops_learn.validation import terraform_plan_analysis + +FIXTURES_DIR = Path(__file__).resolve().parents[1] / "fixtures" / "fake_terraform" + + +def _use_fake_terraform(monkeypatch, scenario: str = "success") -> None: + monkeypatch.setenv("PATH", f"{FIXTURES_DIR}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setenv("FAKE_TERRAFORM_SCENARIO", scenario) + + +def test_real_terraform_tool_has_safe_operations() -> None: + tool = RealTerraformTool() + assert tool.name == "terraform" + names = {spec.name for spec in tool.operations} + assert names == {"fmt", "init", "validate", "plan"} + for spec in tool.operations: + assert spec.risk_level is RiskLevel.SAFE + assert not spec.requires_approval + + +def test_real_terraform_tool_does_not_expose_apply_or_destroy() -> None: + tool = RealTerraformTool() + with pytest.raises(KeyError): + tool.spec_for("apply_approved_plan") + with pytest.raises(KeyError): + tool.spec_for("destroy_approved_environment") + + +def test_missing_binary_reports_failure(monkeypatch, tmp_path) -> None: + monkeypatch.setenv("PATH", str(tmp_path / "empty-path")) + (tmp_path / "empty-path").mkdir() + result = RealTerraformTool().execute( + "fmt", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert not result.success + assert "Terraform CLI not found" in result.summary + + +def test_fmt_success(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch) + result = RealTerraformTool().execute( + "fmt", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert result.success + assert "(real)" in result.summary + + +def test_fmt_needs_formatting(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "fmt_needs_formatting") + result = RealTerraformTool().execute( + "fmt", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert not result.success + assert "needs formatting" in result.summary.lower() + + +def test_init_success(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch) + result = RealTerraformTool().execute( + "init", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert result.success + assert "(real)" in result.summary + + +def test_init_failure(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "init_failure") + result = RealTerraformTool().execute( + "init", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert not result.success + assert "(real, failed)" in result.summary + + +def test_validate_success(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch) + result = RealTerraformTool().execute( + "validate", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert result.success + assert "valid" in result.summary.lower() + + +def test_validate_failure(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "validate_failure") + result = RealTerraformTool().execute( + "validate", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert not result.success + assert result.details["diagnostic_count"] == 1 + + +def test_plan_success_produces_classifier_shaped_details(monkeypatch, tmp_path) -> None: + """Proves the real plan output needs zero changes to + validation/terraform_plan_analysis.analyze().""" + _use_fake_terraform(monkeypatch) + result = RealTerraformTool().execute( + "plan", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert result.success + assert result.details.keys() >= {"create", "change", "replace", "destroy"} + assert result.details["create"] == 2 + + summary = terraform_plan_analysis.analyze(result.details) + assert summary.risk_level is RiskLevel.SAFE + + +def test_plan_mixed_actions_classified_correctly(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "plan_mixed_actions") + result = RealTerraformTool().execute( + "plan", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert result.success + assert result.details["create"] == 1 + assert result.details["change"] == 1 + assert result.details["replace"] == 1 + assert result.details["destroy"] == 1 + + summary = terraform_plan_analysis.analyze(result.details) + assert summary.risk_level is RiskLevel.DESTRUCTIVE + + +def test_plan_failure_is_reported_and_secret_is_redacted(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "plan_failure_with_secret") + result = RealTerraformTool().execute( + "plan", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert not result.success + dumped = json.dumps(result.details) + assert "super-secret-value-123" not in dumped + assert "ARM_CLIENT_SECRET=" in dumped + + +def test_plan_show_json_malformed_is_a_clean_failure(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "show_json_malformed") + result = RealTerraformTool().execute( + "plan", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert not result.success + assert "Could not parse" in result.summary + + +def test_plan_details_never_include_raw_attribute_values(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "plan_with_sensitive_attribute") + result = RealTerraformTool().execute( + "plan", {"path": str(tmp_path)}, dry_run=False, approval=None + ) + assert result.success + assert "hunter2" not in json.dumps(result.details) + + +def test_timeout_is_reported_not_hung(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch, "timeout") + result = RealTerraformTool().execute( + "fmt", + {"path": str(tmp_path), "timeout_seconds": 1}, + dry_run=False, + approval=None, + ) + assert not result.success + assert "timed out" in result.details.get("stderr", "").lower() + + +def test_invalid_working_directory_reports_failure(monkeypatch, tmp_path) -> None: + _use_fake_terraform(monkeypatch) + result = RealTerraformTool().execute( + "fmt", + {"path": str(tmp_path / "does-not-exist")}, + dry_run=False, + approval=None, + ) + assert not result.success + + +def test_dry_run_never_invokes_subprocess(monkeypatch, tmp_path) -> None: + monkeypatch.setenv("PATH", str(tmp_path)) # empty dir: would fail loudly if invoked + result = RealTerraformTool().execute( + "plan", {"path": str(tmp_path)}, dry_run=True, approval=None + ) + assert result.success + assert "dry run" in result.summary.lower() diff --git a/tests/tools/test_subprocess_safety.py b/tests/tools/test_subprocess_safety.py new file mode 100644 index 0000000..af9e108 --- /dev/null +++ b/tests/tools/test_subprocess_safety.py @@ -0,0 +1,52 @@ +from devops_learn.tools import _subprocess_safety + + +def test_redact_masks_secret_shaped_key_value_pairs() -> None: + text = "Error: ARM_CLIENT_SECRET=abc123XYZ failed to authenticate" + redacted = _subprocess_safety.redact(text) + assert "abc123XYZ" not in redacted + assert "ARM_CLIENT_SECRET=" in redacted + + +def test_redact_leaves_non_secret_text_untouched() -> None: + text = "Plan: 2 to add, 0 to change, 0 to destroy." + assert _subprocess_safety.redact(text) == text + + +def test_truncate_caps_long_output_with_a_visible_marker() -> None: + long_text = "x" * (_subprocess_safety.MAX_OUTPUT_CHARS + 500) + result = _subprocess_safety._truncate(long_text) + assert len(result) < len(long_text) + assert "truncated" in result + + +def test_run_safely_returns_sanitized_result_on_success() -> None: + result = _subprocess_safety.run_safely( + ["python3", "-c", "print('hello')"], cwd=None, timeout=10 + ) + assert result.returncode == 0 + assert "hello" in result.stdout + assert not result.timed_out + + +def test_run_safely_reports_timeout_without_hanging() -> None: + result = _subprocess_safety.run_safely( + ["python3", "-c", "import time; time.sleep(5)"], cwd=None, timeout=1 + ) + assert result.timed_out + assert result.returncode != 0 + assert "timed out" in result.stderr.lower() + + +def test_run_safely_redacts_secret_shaped_stderr() -> None: + result = _subprocess_safety.run_safely( + [ + "python3", + "-c", + "import sys; sys.stderr.write('API_KEY=super-secret-value')", + ], + cwd=None, + timeout=10, + ) + assert "super-secret-value" not in result.stderr + assert "API_KEY=" in result.stderr From 814fdb592d5e179bbd4ab0d053aa9809f97364fd Mon Sep 17 00:00:00 2001 From: howlcipher Date: Wed, 12 Aug 2026 16:55:04 -0400 Subject: [PATCH 2/6] Add devops-learn terraform command and its vertical-slice workflow workflows/terraform_flow.py runs fmt -> init -> validate -> plan -> risk analysis, teaching provider/state concepts just-in-time (gated by learner profile, matching local_flow's precedent) and diagnosing an auth failure on plan as an expected, honest outcome rather than a bug. cli/main.py wires RealTerraformTool in only for this command, keeping every other command's terraform tool simulated. Co-Authored-By: Claude Sonnet 5 --- src/devops_learn/cli/commands/terraform.py | 44 +++ src/devops_learn/cli/main.py | 31 +- src/devops_learn/workflows/terraform_flow.py | 360 +++++++++++++++++++ tests/workflows/test_terraform_flow.py | 165 +++++++++ 4 files changed, 595 insertions(+), 5 deletions(-) create mode 100644 src/devops_learn/cli/commands/terraform.py create mode 100644 src/devops_learn/workflows/terraform_flow.py create mode 100644 tests/workflows/test_terraform_flow.py diff --git a/src/devops_learn/cli/commands/terraform.py b/src/devops_learn/cli/commands/terraform.py new file mode 100644 index 0000000..f48c5e3 --- /dev/null +++ b/src/devops_learn/cli/commands/terraform.py @@ -0,0 +1,44 @@ +"""`devops-learn terraform [path]`: run the real Terraform vertical slice. + +Runs real fmt/init/validate/plan against /infra/terraform via +RealTerraformTool. This milestone ships real execution only against the +bundled projects/api_platform sample; generating configuration for arbitrary +projects is future work (see docs/roadmap.md). +""" + +from __future__ import annotations + +import argparse +from pathlib import Path + +from devops_learn.bootstrap import Platform +from devops_learn.cli.terminal_ui import TerminalUi +from devops_learn.workflows.terraform_flow import TerraformOptions, run_terraform_flow + + +def register(subparsers: "argparse._SubParsersAction[argparse.ArgumentParser]") -> None: + parser = subparsers.add_parser( + "terraform", + help="Run the real Terraform vertical slice: fmt, init, validate, plan, risk analysis", + ) + parser.add_argument( + "path", + nargs="?", + default="projects/api_platform", + help="Project containing infra/terraform/ (defaults to the bundled sample project)", + ) + parser.set_defaults(handler=run) + + +def run(args: argparse.Namespace, platform: Platform) -> None: + infra_root = f"{args.path}/infra/terraform" + if not Path(infra_root).is_dir(): + print(f"No Terraform configuration found at {infra_root}.") + print( + "This milestone ships real Terraform execution only against the bundled " + "projects/api_platform sample; generating configuration for arbitrary projects " + "is future work (see docs/roadmap.md)." + ) + return + options = TerraformOptions(project_root=args.path, infra_root=infra_root) + run_terraform_flow(platform, TerminalUi(), options) diff --git a/src/devops_learn/cli/main.py b/src/devops_learn/cli/main.py index 9c669ee..80b7289 100644 --- a/src/devops_learn/cli/main.py +++ b/src/devops_learn/cli/main.py @@ -7,21 +7,30 @@ from devops_learn.ai.anthropic_provider import AnthropicProvider from devops_learn.bootstrap import build_platform -from devops_learn.cli.commands import analyze, explain, history, init, local, profile, review +from devops_learn.cli.commands import ( + analyze, + explain, + history, + init, + local, + profile, + review, + terraform, +) from devops_learn.config.settings import load_settings from devops_learn.domain.enums import ExecutionMode from devops_learn.learning.persistence.connection import connect from devops_learn.tools.approval import ApprovalGate, CliApprovalGate, ThresholdApprovalGate from devops_learn.tools.base import RiskLevel, Tool -from devops_learn.tools.docker_tool import RealDockerTool +from devops_learn.tools.docker_tool import RealDockerTool, SimulatedDockerTool from devops_learn.tools.git_tool import SimulatedGitTool from devops_learn.tools.kubernetes_tool import SimulatedKubernetesTool -from devops_learn.tools.python_tool import RealPythonTool -from devops_learn.tools.terraform_tool import SimulatedTerraformTool +from devops_learn.tools.python_tool import RealPythonTool, SimulatedPythonTool +from devops_learn.tools.terraform_tool import RealTerraformTool, SimulatedTerraformTool from devops_learn.tools.validation_tool import SimulatedValidationTool from devops_learn.tools.cloud_tool import SimulatedCloudTool -_COMMAND_MODULES = (analyze, review, history, explain, profile, init, local) +_COMMAND_MODULES = (analyze, review, history, explain, profile, init, local, terraform) def build_parser() -> argparse.ArgumentParser: @@ -69,6 +78,18 @@ def _approval_gate_for_args(args: argparse.Namespace) -> ApprovalGate: def _tools_for_args(args: argparse.Namespace) -> dict[str, Tool] | None: + if args.command == "terraform": + # This command only ever invokes the "terraform" tool -- keep the rest + # simulated rather than provisioning real python/docker tools it never uses. + return { + "python": SimulatedPythonTool(), + "git": SimulatedGitTool(), + "docker": SimulatedDockerTool(), + "terraform": RealTerraformTool(), + "kubernetes": SimulatedKubernetesTool(), + "cloud": SimulatedCloudTool(), + "validation": SimulatedValidationTool(), + } if getattr(args, "real_tools", False) or args.command == "local": return { "python": RealPythonTool(), diff --git a/src/devops_learn/workflows/terraform_flow.py b/src/devops_learn/workflows/terraform_flow.py new file mode 100644 index 0000000..b6330f1 --- /dev/null +++ b/src/devops_learn/workflows/terraform_flow.py @@ -0,0 +1,360 @@ +"""Real Terraform vertical slice: fmt -> init -> validate -> plan -> risk analysis. + +Runs RealTerraformTool against a real, committed Terraform working directory +(projects/api_platform/infra/terraform/ by default). Never applies or +destroys -- see docs/roadmap.md (Milestone 3) and docs/safety.md. A plan +failure due to missing Azure credentials is treated as an expected, honest +outcome to teach from, not something to hide. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from datetime import datetime, timezone +from typing import Any + +from devops_learn.bootstrap import Platform +from devops_learn.domain.enums import ( + AuditEventType, + CloudProviderKind, + CostPriority, + EnvironmentKind, + ExecutionMode, + ExperienceState, + ExplanationDepth, +) +from devops_learn.domain.explanation_models import Explanation, LearningMoment +from devops_learn.domain.learner_profile_models import CompetencyArea +from devops_learn.domain.plan_models import TerraformPlanSummary +from devops_learn.domain.session_models import EngagementSession +from devops_learn.tools.approval import RiskLevel +from devops_learn.tools.base import ToolResult +from devops_learn.validation import terraform_plan_analysis +from devops_learn.workflows import presenters +from devops_learn.workflows.ui import Ui + + +@dataclass(frozen=True) +class TerraformOptions: + project_root: str + infra_root: str + + +def run_terraform_flow( + platform: Platform, ui: Ui, options: TerraformOptions +) -> EngagementSession: + session = platform.session_service.start( + project_root=options.project_root, + mode=ExecutionMode.COLLABORATIVE, + explanation_depth=ExplanationDepth.LEARNING, + cloud=CloudProviderKind.AZURE, + environment=EnvironmentKind.DEV, + cost_priority=CostPriority.BALANCED, + simulation_mode=False, + ) + assert session.id is not None + session_id = session.id + + _audit(platform, session_id, AuditEventType.SESSION_STARTED, "Terraform workflow started") + + fmt_result = _run_terraform_step( + platform, ui, session_id, "fmt", options.infra_root, "Terraform fmt" + ) + if not fmt_result.success: + ui.present( + f"Configuration is not formatted. Run `terraform fmt` in {options.infra_root} " + "to fix this before continuing." + ) + + _teach_provider_concept(platform, ui, session_id) + init_result = _run_terraform_step( + platform, ui, session_id, "init", options.infra_root, "Terraform init" + ) + if not init_result.success: + _diagnose_and_stop(platform, ui, session_id, "terraform init failed") + platform.session_service.complete(session) + return session + + validate_result = _run_terraform_step( + platform, ui, session_id, "validate", options.infra_root, "Terraform validate" + ) + if not validate_result.success: + _diagnose_and_stop(platform, ui, session_id, "terraform validate failed") + platform.session_service.complete(session) + return session + + _teach_state_concepts(platform, ui, session_id) + _audit(platform, session_id, AuditEventType.TERRAFORM_PLAN_STARTED, "Terraform plan started") + plan_result = _run_terraform_step( + platform, ui, session_id, "plan", options.infra_root, "Terraform plan" + ) + if not plan_result.success: + _diagnose_plan_failure(platform, ui, session_id, plan_result) + platform.session_service.complete(session) + return session + + summary = terraform_plan_analysis.analyze(plan_result.details) + ui.present(presenters.render_terraform_plan(summary)) + _audit(platform, session_id, AuditEventType.TERRAFORM_PLAN_COMPLETED, summary.reason) + _explain(platform, ui, "What to check", _what_to_check_explanation(summary)) + + platform.experience_tracker.record( + session_id, "Terraform", "Ran real fmt/init/validate/plan", ExperienceState.PRACTICED + ) + platform.experience_tracker.record( + session_id, "Azure", "Reviewed a real Azure resource plan", ExperienceState.INTRODUCED + ) + + _print_history_and_experience(platform, ui, session_id) + platform.session_service.complete(session) + return session + + +def _audit( + platform: Platform, + session_id: int, + event_type: AuditEventType, + summary: str, + payload: dict[str, Any] | None = None, +) -> None: + platform.audit_service.record( + session_id=session_id, + event_type=event_type, + occurred_at=_now(), + summary=summary, + payload=payload or {}, + ) + + +def _run_terraform_step( + platform: Platform, + ui: Ui, + session_id: int, + operation: str, + infra_root: str, + label: str, +) -> ToolResult: + _explain(platform, ui, label, _terraform_explanation(operation)) + result = platform.tool_service.invoke("terraform", operation, {"path": infra_root}) + ui.present(result.summary) + _audit( + platform, + session_id, + AuditEventType.TOOL_INVOKED, + f"terraform.{operation}", + {"success": result.success}, + ) + return result + + +def _diagnose_and_stop( + platform: Platform, ui: Ui, session_id: int, context: str +) -> None: + ui.present(f"{context}. Diagnosis starts from the output above.") + _audit(platform, session_id, AuditEventType.DEPLOYMENT_FAILED, context) + platform.experience_tracker.record( + session_id, "Troubleshooting", f"Investigated {context}", ExperienceState.INTRODUCED + ) + + +def _diagnose_plan_failure( + platform: Platform, ui: Ui, session_id: int, result: ToolResult +) -> None: + stderr = str(result.details.get("stderr", "")).lower() + is_auth_failure = "az login" in stderr or "authoriz" in stderr or "authent" in stderr + if is_auth_failure: + ui.present( + "DIAGNOSIS: terraform plan failed because Terraform could not authenticate to " + "Azure. The azurerm provider needs credentials to build API requests, even for a " + "read-only plan.\n\n" + "Fix by doing ONE of:\n" + " - `az login` (uses your interactive Azure CLI session), or\n" + " - setting ARM_CLIENT_ID, ARM_CLIENT_SECRET, ARM_TENANT_ID, and " + "ARM_SUBSCRIPTION_ID for a service principal.\n\n" + "This is an expected, honest failure on a machine with no Azure credentials -- " + "not a bug. See docs/safety.md." + ) + else: + ui.present("terraform plan failed. Diagnosis starts from the output above.") + _audit(platform, session_id, AuditEventType.DEPLOYMENT_FAILED, "terraform plan failed") + platform.experience_tracker.record( + session_id, + "Troubleshooting", + "Diagnosed a terraform plan authentication failure" + if is_auth_failure + else "Diagnosed a terraform plan failure", + ExperienceState.PRACTICED, + ) + + +def _should_teach(platform: Platform) -> bool: + profile = platform.learner_profile_service.load() + return not profile.is_strong(CompetencyArea.TERRAFORM) + + +def _teach_state_concepts(platform: Platform, ui: Ui, session_id: int) -> None: + if not _should_teach(platform): + return + moment = LearningMoment( + concept="Terraform state", + trigger="before running terraform plan", + summary=( + "Terraform tracks the infrastructure it manages in a state file. State is " + "Terraform's record of what it believes exists; the real Azure resources are the " + "ground truth, and the two can drift apart." + ), + deep_explanation=( + "Every managed resource gets an address (e.g. azurerm_resource_group.main) " + "recorded in state alongside the real Azure resource ID it maps to. Terraform " + "diffs your configuration against state, then confirms that against Azure during " + "plan/apply. State can contain resource attribute values, including sensitive " + "ones, which is why it is never committed to git (see .gitignore) -- and why " + "deleting a state file does NOT delete the real Azure resources; it only makes " + "Terraform forget about them." + ), + why_it_matters=( + "Understanding state is the difference between reading a `terraform plan` and " + "actually knowing what it will do." + ), + related_artifact="docs/terraform-state.md", + ) + _render_learning_moment(platform, ui, moment) + platform.experience_tracker.record( + session_id, + "Terraform state", + "Explained desired vs. actual state", + ExperienceState.INTRODUCED, + ) + + +def _teach_provider_concept(platform: Platform, ui: Ui, session_id: int) -> None: + if not _should_teach(platform): + return + moment = LearningMoment( + concept="Terraform provider", + trigger="before running terraform init", + summary=( + "A provider is the plugin Terraform uses to talk to a specific platform -- here, " + "the azurerm provider talks to Azure's Resource Manager API. `terraform init` " + "downloads it." + ), + deep_explanation=( + "A resource block's type prefix (azurerm_) tells Terraform which provider owns it " + "and which real API calls to make. A resource address like " + "azurerm_resource_group.main combines the resource type and a local name; " + "Terraform uses addresses to track dependencies between resources, such as the " + "container registry needing the resource group to exist first." + ), + why_it_matters=( + "init failing almost always means a provider or network problem, not a mistake in " + "your .tf files -- a different failure mode than validate or plan." + ), + related_artifact="docs/terraform-state.md", + ) + _render_learning_moment(platform, ui, moment) + platform.experience_tracker.record( + session_id, + "Terraform", + "Explained provider and resource address", + ExperienceState.INTRODUCED, + ) + + +def _render_learning_moment(platform: Platform, ui: Ui, moment: LearningMoment) -> None: + rendered = platform.explanation_service.render_learning_moment( + moment, mode=ExecutionMode.COLLABORATIVE, depth=ExplanationDepth.LEARNING + ) + if rendered: + ui.present(rendered) + + +def _explain(platform: Platform, ui: Ui, label: str, explanation: Explanation) -> None: + rendered = platform.explanation_service.render( + explanation, mode=ExecutionMode.COLLABORATIVE, depth=ExplanationDepth.LEARNING + ) + ui.present(rendered) + + +def _terraform_explanation(operation: str) -> Explanation: + if operation == "fmt": + return Explanation( + action="Checking Terraform formatting.", + why="Consistent formatting makes configuration easy to review and diff.", + ) + if operation == "init": + return Explanation( + action="Initializing the Terraform working directory.", + why=( + "Terraform must download the azurerm provider before it can validate, plan, " + "or apply anything." + ), + what_to_understand=( + "init also creates a local .terraform/ plugin cache, which is never committed " + "to git -- it is regenerated by running init again." + ), + ) + if operation == "validate": + return Explanation( + action="Validating the configuration.", + why="Validate catches syntax and type errors locally, before Terraform talks to Azure.", + ) + return Explanation( + action="Running terraform plan.", + why="Plan shows exactly what Terraform would change, without changing anything yet.", + what_to_understand=( + "Plan compares your configuration against Terraform's state file, then confirms " + "that against Azure. It requires Azure authentication even though it makes no " + "changes." + ), + ) + + +def _what_to_check_explanation(summary: TerraformPlanSummary) -> Explanation: + if summary.risk_level is RiskLevel.DESTRUCTIVE: + result = ( + "This plan would destroy real resources. Never approve an apply from a plan like " + "this without independently confirming that destruction is intended." + ) + elif summary.risk_level is RiskLevel.HIGH: + result = ( + "Terraform must destroy and recreate at least one resource. Confirm you " + "understand *why* before ever approving an apply: is the resource stateful, " + "would its address or endpoint change, and would that cause downtime?" + ) + elif summary.risk_level is RiskLevel.LOW: + result = "Confirm the in-place changes match what you intended to modify." + else: + result = ( + "Confirm the resource names in this plan won't collide with anything already in " + "the target subscription." + ) + return Explanation( + action="Reviewing what this plan means before any apply.", + what_to_understand=( + "Terraform state can drift from real infrastructure if something changes " + "resources outside Terraform. A plan is only trustworthy if state and Azure " + "haven't drifted apart." + ), + result=result, + ) + + +def _print_history_and_experience(platform: Platform, ui: Ui, session_id: int) -> None: + events = platform.audit_service.history(session_id) + lines = ["AUDIT HISTORY", ""] + for event in events: + lines.append(f"{event.sequence_no:>3}. {event.event_type.value}: {event.summary}") + ui.present("\n".join(lines)) + + grouped = platform.experience_tracker.summary(session_id) + lines = ["EXPERIENCE SUMMARY", ""] + for concept, entries in grouped.items(): + lines.append(concept) + for entry in entries: + lines.append(f" ✓ {entry.item}") + lines.append("") + ui.present("\n".join(lines).rstrip()) + + +def _now() -> datetime: + return datetime.now(timezone.utc) diff --git a/tests/workflows/test_terraform_flow.py b/tests/workflows/test_terraform_flow.py new file mode 100644 index 0000000..e8f7f9f --- /dev/null +++ b/tests/workflows/test_terraform_flow.py @@ -0,0 +1,165 @@ +import os +import sqlite3 +from pathlib import Path + +from devops_learn.bootstrap import build_platform +from devops_learn.domain.enums import AuditEventType +from devops_learn.domain.learner_profile_models import ( + CompetencyArea, + LearnerProfile, + ProficiencyLevel, +) +from devops_learn.domain.question_models import ClarifyingQuestion +from devops_learn.tools.approval import AutoApproveApprovalGate +from devops_learn.tools.terraform_tool import RealTerraformTool +from devops_learn.workflows.terraform_flow import TerraformOptions, run_terraform_flow +from devops_learn.workflows.ui import Ui + +FIXTURES_DIR = Path(__file__).resolve().parents[1] / "fixtures" / "fake_terraform" + + +class ScriptedUi(Ui): + def __init__(self) -> None: + self.presented: list[str] = [] + + def present(self, text: str) -> None: + self.presented.append(text) + + def ask_choice(self, question: ClarifyingQuestion) -> str: + return question.options[0] if question.options else "yes" + + def confirm(self, prompt: str, *, default: bool = False) -> bool: + return True + + +def _platform(conn: sqlite3.Connection): + return build_platform( + conn, tools={"terraform": RealTerraformTool()}, approval_gate=AutoApproveApprovalGate() + ) + + +def _use_fake_terraform(monkeypatch, scenario: str) -> None: + monkeypatch.setenv("PATH", f"{FIXTURES_DIR}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setenv("FAKE_TERRAFORM_SCENARIO", scenario) + + +def test_successful_plan_reaches_risk_analysis_and_records_experience( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "success") + platform = _platform(conn) + ui = ScriptedUi() + session = run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + assert session.id is not None + + joined = "\n".join(ui.presented) + assert "TERRAFORM PLAN" in joined + assert "Risk: SAFE" in joined + + events = {e.event_type for e in platform.audit_service.history(session.id)} + assert AuditEventType.TERRAFORM_PLAN_COMPLETED in events + assert AuditEventType.DEPLOYMENT_FAILED not in events + + experience = platform.experience_tracker.summary(session.id) + assert "Terraform" in experience + assert "Azure" in experience + + +def test_plan_auth_failure_produces_credential_diagnosis( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "plan_failure_with_secret") + platform = _platform(conn) + ui = ScriptedUi() + session = run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + assert session.id is not None + + joined = "\n".join(ui.presented) + assert "az login" in joined + assert "ARM_CLIENT_SECRET" in joined + assert "super-secret-value-123" not in joined # redacted before ever reaching the UI + + events = {e.event_type for e in platform.audit_service.history(session.id)} + assert AuditEventType.DEPLOYMENT_FAILED in events + assert AuditEventType.TERRAFORM_PLAN_COMPLETED not in events + + +def test_generic_plan_failure_does_not_claim_an_auth_problem( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "show_json_malformed") + platform = _platform(conn) + ui = ScriptedUi() + run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + + joined = "\n".join(ui.presented) + assert "az login" not in joined + assert "Diagnosis starts from the output above" in joined + + +def test_init_failure_stops_before_plan( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "init_failure") + platform = _platform(conn) + ui = ScriptedUi() + session = run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + assert session.id is not None + + events = {e.event_type for e in platform.audit_service.history(session.id)} + assert AuditEventType.DEPLOYMENT_FAILED in events + assert AuditEventType.TERRAFORM_PLAN_STARTED not in events + + +def test_beginner_learner_sees_teaching_moments( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "success") + platform = _platform(conn) + ui = ScriptedUi() + run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + joined = "\n".join(ui.presented) + assert "LEARNING MOMENT: Terraform provider" in joined + assert "LEARNING MOMENT: Terraform state" in joined + + +def test_learner_strong_in_terraform_sees_no_teaching_moments( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "success") + platform = _platform(conn) + platform.learner_profile_service.save( + LearnerProfile( + proficiencies={CompetencyArea.TERRAFORM: ProficiencyLevel.STRONG}, + learning_focus=(), + ) + ) + ui = ScriptedUi() + run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + joined = "\n".join(ui.presented) + assert "LEARNING MOMENT" not in joined + + +def test_mixed_actions_plan_is_classified_high_risk( + conn: sqlite3.Connection, monkeypatch, tmp_path +) -> None: + _use_fake_terraform(monkeypatch, "plan_mixed_actions") + platform = _platform(conn) + ui = ScriptedUi() + run_terraform_flow( + platform, ui, TerraformOptions(project_root=str(tmp_path), infra_root=str(tmp_path)) + ) + joined = "\n".join(ui.presented) + assert "Risk: DESTRUCTIVE" in joined From 47045970eafa3c60662dbff494afeaf50980ce56 Mon Sep 17 00:00:00 2001 From: howlcipher Date: Wed, 12 Aug 2026 16:55:16 -0400 Subject: [PATCH 3/6] Add real Terraform config for the bundled sample project projects/api_platform/infra/terraform/: resource group, container registry, Log Analytics workspace, tagged for easy identification and cleanup. Split into providers/variables/main/outputs.tf as a readability convention (Terraform still merges the whole directory). Fixes an ACR naming bug (hyphens aren't valid in that resource's name, unlike most others) caught by live terraform validate during review. .gitignore now excludes .terraform/ and *.tfstate* while keeping .terraform.lock.hcl committed. ProjectAnalyzer correctly now detects this real IaC. Co-Authored-By: Claude Sonnet 5 --- .gitignore | 17 ++++++++ .../infra/terraform/.terraform.lock.hcl | 22 ++++++++++ .../api_platform/infra/terraform/README.md | 34 +++++++++++++++ projects/api_platform/infra/terraform/main.tf | 42 +++++++++++++++++++ .../api_platform/infra/terraform/outputs.tf | 7 ++++ .../api_platform/infra/terraform/providers.tf | 18 ++++++++ .../api_platform/infra/terraform/variables.tf | 11 +++++ tests/analysis/test_project_analyzer.py | 4 +- 8 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 projects/api_platform/infra/terraform/.terraform.lock.hcl create mode 100644 projects/api_platform/infra/terraform/README.md create mode 100644 projects/api_platform/infra/terraform/main.tf create mode 100644 projects/api_platform/infra/terraform/outputs.tf create mode 100644 projects/api_platform/infra/terraform/providers.tf create mode 100644 projects/api_platform/infra/terraform/variables.tf diff --git a/.gitignore b/.gitignore index e6c3749..01305ff 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,20 @@ htmlcov/ *.sqlite3 .env .devops_learn/ + +# Terraform working-directory artifacts (see projects/api_platform/infra/terraform/). +# .terraform.lock.hcl is deliberately NOT listed here: it pins exact provider +# versions/hashes so `terraform init` is reproducible across machines and CI, +# which is normal, recommended Terraform practice -- unlike .terraform/ (a +# regenerable local plugin cache) and *.tfstate (can contain sensitive values, +# and since this milestone never applies, would only ever hold stale/empty +# state if committed). +**/.terraform/ +*.tfstate +*.tfstate.* +crash.log +crash.*.log +override.tf +override.tf.json +*_override.tf +*_override.tf.json diff --git a/projects/api_platform/infra/terraform/.terraform.lock.hcl b/projects/api_platform/infra/terraform/.terraform.lock.hcl new file mode 100644 index 0000000..eb0286e --- /dev/null +++ b/projects/api_platform/infra/terraform/.terraform.lock.hcl @@ -0,0 +1,22 @@ +# This file is maintained automatically by "terraform init". +# Manual edits may be lost in future updates. + +provider "registry.terraform.io/hashicorp/azurerm" { + version = "3.117.1" + constraints = "~> 3.0" + hashes = [ + "h1:3c9iOEtBMnHrpJLlhbQ0sCZPWhE/2dvEPcL8KkXAh7w=", + "zh:0c513676836e3c50d004ece7d2624a8aff6faac14b833b96feeac2e4bc2c1c12", + "zh:50ea01ada95bae2f187db9e926e463f45d860767a85ebc59160414e00e76c35d", + "zh:52c2a9edacc06b3f72153f5ef6daca0761c6292158815961fe37f60bc576a3d7", + "zh:618eed2a06b19b1a025b45b05891846d570a6a1cca4d23f4942f5a99e1f747ae", + "zh:61cde5d3165d7e5ec311d5d89486819cd605c1b2d54611b5c97bd4e97dba2762", + "zh:6a873358d5031fc222f5e05f029d1237f3dce8345c767665f393283dfa2627f6", + "zh:afdd80064b2a04da311856feb4ed45f77ff4df6c356e8c2b10afb51fe7e61c70", + "zh:b09113df7e0e8c8959539bd22bae6c39faeb269ba3c4cd948e742f5cf58c35fb", + "zh:d340db7973109761cfc27d52aa02560363337c908b2c99b3628adc5a70a99d5b", + "zh:d5a577226ebc8c65e8f19384878a86acc4b51ede4b4a82d37c3b331b0efcd4a7", + "zh:e2962b147f9e71732df8dbc74940c10d20906f3c003cbfaa1eb9fabbf601a9f0", + "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", + ] +} diff --git a/projects/api_platform/infra/terraform/README.md b/projects/api_platform/infra/terraform/README.md new file mode 100644 index 0000000..d2b808f --- /dev/null +++ b/projects/api_platform/infra/terraform/README.md @@ -0,0 +1,34 @@ +# infra/terraform + +This is a real, valid Terraform configuration for a minimal Azure footprint: +a resource group, a container registry, and a Log Analytics workspace. + +## Files in this directory + +Terraform merges every `*.tf` file in a directory into a single +configuration. The split here (`providers.tf` / `variables.tf` / `main.tf` / +`outputs.tf`) is a naming convention for readability, not a module boundary +-- Terraform does not treat these as separate modules, and there is no +requirement to name files this way. + +## What runs for real + +`devops-learn terraform` runs real `fmt`, `init`, `validate`, and `plan` +against this directory via `RealTerraformTool` +(`src/devops_learn/tools/terraform_tool.py`). `init` downloads the `azurerm` +provider plugin (network access, no Azure credentials needed). `plan` +requires Azure authentication (`az login`, or `ARM_CLIENT_ID` / +`ARM_CLIENT_SECRET` / `ARM_TENANT_ID` / `ARM_SUBSCRIPTION_ID` environment +variables) to build the provider client -- without it, `plan` fails cleanly +and the workflow explains why rather than faking a result. + +`apply`/`destroy` are not implemented against this directory yet; see +`docs/roadmap.md` (Milestone 3) and `docs/terraform-state.md`. + +## Local state + +Running `init`/`plan` here creates `.terraform/` and, if this configuration +is ever applied, `terraform.tfstate*` -- both are gitignored (see +`docs/terraform-state.md` for why state isn't committed). +`.terraform.lock.hcl` is committed on purpose: it pins the exact provider +version/hash so `init` is reproducible on every machine. diff --git a/projects/api_platform/infra/terraform/main.tf b/projects/api_platform/infra/terraform/main.tf new file mode 100644 index 0000000..1ec84b1 --- /dev/null +++ b/projects/api_platform/infra/terraform/main.tf @@ -0,0 +1,42 @@ +# The resources this configuration declares: everything the learner sees +# planned/applied against Azure by `devops-learn terraform`. +# +# Tagged consistently so every resource this project creates is obvious in +# the Azure portal and easy to find and delete. + +locals { + tags = { + project = "devops-learn-by-doing" + environment = "learning" + managed-by = "terraform" + purpose = "devops-learning" + } + + # Azure Container Registry names must be alphanumeric only (no hyphens), + # unlike most other Azure resource names -- strip them here rather than + # relaxing var.project_name itself. + registry_name = replace(var.project_name, "-", "") +} + +resource "azurerm_resource_group" "main" { + name = "${var.project_name}-rg" + location = var.location + tags = local.tags +} + +resource "azurerm_container_registry" "main" { + name = "${local.registry_name}acr" + resource_group_name = azurerm_resource_group.main.name + location = azurerm_resource_group.main.location + sku = "Basic" + tags = local.tags +} + +resource "azurerm_log_analytics_workspace" "main" { + name = "${var.project_name}-logs" + resource_group_name = azurerm_resource_group.main.name + location = azurerm_resource_group.main.location + sku = "PerGB2018" + retention_in_days = 30 + tags = local.tags +} diff --git a/projects/api_platform/infra/terraform/outputs.tf b/projects/api_platform/infra/terraform/outputs.tf new file mode 100644 index 0000000..ea34d4c --- /dev/null +++ b/projects/api_platform/infra/terraform/outputs.tf @@ -0,0 +1,7 @@ +output "resource_group_name" { + value = azurerm_resource_group.main.name +} + +output "container_registry_login_server" { + value = azurerm_container_registry.main.login_server +} diff --git a/projects/api_platform/infra/terraform/providers.tf b/projects/api_platform/infra/terraform/providers.tf new file mode 100644 index 0000000..10afbfc --- /dev/null +++ b/projects/api_platform/infra/terraform/providers.tf @@ -0,0 +1,18 @@ +# Which platform Terraform talks to, and which version of it. +# +# Terraform merges every *.tf file in this directory into a single +# configuration -- providers.tf / variables.tf / main.tf / outputs.tf is a +# naming convention for readability, not a module boundary. See README.md. + +terraform { + required_providers { + azurerm = { + source = "hashicorp/azurerm" + version = "~> 3.0" + } + } +} + +provider "azurerm" { + features {} +} diff --git a/projects/api_platform/infra/terraform/variables.tf b/projects/api_platform/infra/terraform/variables.tf new file mode 100644 index 0000000..93feebe --- /dev/null +++ b/projects/api_platform/infra/terraform/variables.tf @@ -0,0 +1,11 @@ +variable "project_name" { + description = "Short name used to prefix every resource this configuration creates." + type = string + default = "api-platform" +} + +variable "location" { + description = "Azure region to create resources in." + type = string + default = "eastus" +} diff --git a/tests/analysis/test_project_analyzer.py b/tests/analysis/test_project_analyzer.py index a8f95cc..059298f 100644 --- a/tests/analysis/test_project_analyzer.py +++ b/tests/analysis/test_project_analyzer.py @@ -14,7 +14,9 @@ def test_analyzes_the_bundled_example_project() -> None: assert assessment.containerization_status is MaturityStatus.GOOD assert assessment.healthcheck_status is MaturityStatus.GOOD assert assessment.ci_cd_status is MaturityStatus.MISSING - assert assessment.iac_status is MaturityStatus.MISSING + # projects/api_platform/infra/terraform/ is real Terraform config as of Milestone 2 + # (docs/roadmap.md) -- ProjectAnalyzer correctly detects it. + assert assessment.iac_status is MaturityStatus.GOOD def test_detects_missing_everything_for_an_empty_project(tmp_path: Path) -> None: From 4e42692b09f4bd67ccedf2a62b6b928643d3d155 Mon Sep 17 00:00:00 2001 From: howlcipher Date: Wed, 12 Aug 2026 16:55:22 -0400 Subject: [PATCH 4/6] Add real Terraform checks to CI fmt -check, init -backend=false, and validate against projects/api_platform/infra/terraform/ -- no Azure credentials needed, no plan/apply in CI. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd2b278..5c2a466 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,3 +23,14 @@ jobs: run: cd projects/api_platform && mypy app --strict - name: Test run: pytest + - name: Setup Terraform + uses: hashicorp/setup-terraform@v3 + with: + terraform_version: "1.15.8" + terraform_wrapper: false + - name: Terraform fmt check + run: terraform fmt -check -recursive projects/api_platform/infra/terraform + - name: Terraform init (no backend, no credentials) + run: terraform -chdir=projects/api_platform/infra/terraform init -backend=false -input=false + - name: Terraform validate + run: terraform -chdir=projects/api_platform/infra/terraform validate From c049c419e1c54ce9e12dd768f4cdbaaf5f922ad5 Mon Sep 17 00:00:00 2001 From: howlcipher Date: Wed, 12 Aug 2026 16:55:33 -0400 Subject: [PATCH 5/6] Document Milestone 2 and teach Terraform state as a first-class concept docs/terraform-state.md is the reference the in-flow teaching moments link back to: desired vs. actual vs. Terraform state, provider, resource address, dependencies, drift, local vs. remote state and locking, state sensitivity, why deleting state isn't deleting infrastructure, why hand-editing state is dangerous. Updates README/roadmap/safety/ development docs to describe exactly what's real this milestone (fmt/ init/validate/plan, no apply, no real Azure execution) and documents the RUN_AZURE_INTEGRATION_TESTS convention Milestone 3 will need. Co-Authored-By: Claude Sonnet 5 --- CONTRIBUTING.md | 11 +++-- README.md | 14 +++++- docs/development.md | 17 +++++++- docs/learning-model.md | 6 +++ docs/roadmap.md | 23 ++++++---- docs/safety.md | 28 ++++++++++++ docs/terraform-state.md | 94 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 178 insertions(+), 15 deletions(-) create mode 100644 docs/terraform-state.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8674c22..a15439b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -29,6 +29,11 @@ existing `CloudConcept`. See `docs/cloud-model.md`. ## Adding a tool -All tool operations are simulated in V1. Implement the `Tool` interface in -`src/devops_learn/tools/`, declare accurate `risk_level`/`is_destructive` metadata, and -never bypass `ToolService.invoke`. See `docs/safety.md`. +Implement the `Tool` interface in `src/devops_learn/tools/`, declare accurate +`risk_level`/`is_destructive` metadata, and never bypass `ToolService.invoke`. Provide a +`Simulated*Tool` for tests/offline use and a `Real*Tool` that shells out to the actual binary +via an argument list (never `shell=True`), an explicit timeout, and +`tools/_subprocess_safety.py`'s redaction. See `docs/safety.md` and `docs/development.md`. + +Any test needing real Azure credentials must be gated behind `RUN_AZURE_INTEGRATION_TESTS=1` so +normal CI never attempts to reach Azure. diff --git a/README.md b/README.md index 43882cc..ed3a6c7 100644 --- a/README.md +++ b/README.md @@ -24,8 +24,14 @@ Project -> Analysis -> Requirements -> Questions -> Recommendations -> Architect questions that cannot be inferred safely. - **Real local execution** (`devops-learn local`) runs actual `pytest`, `flake8`, `docker build`, `docker run`, and HTTP health checks against the project. -- **Simulated cloud/Terraform/Kubernetes** execution for safe learning without - credentials or cost (`devops-learn analyze` without `--real-tools`). +- **Real Terraform execution** (`devops-learn terraform`) runs actual + `terraform fmt`/`init`/`validate`/`plan` against a real, committed Azure + configuration, with structured `terraform show -json` parsing feeding a + deterministic risk classifier. No `apply`/`destroy` yet; `plan` requires + Azure authentication and fails cleanly, with an explanation, without it. +- **Simulated cloud/Kubernetes** execution for safe learning without + credentials or cost (`devops-learn analyze` without `--real-tools`; + Terraform in `analyze`/`review` also stays simulated). - **Controlled tool execution**: every capability is a `Tool` with declared risk level, dry-run support, and human approval; `ToolService` is the only caller of `Tool.execute`. @@ -53,6 +59,10 @@ devops-learn init projects/api_platform # Run a real local vertical slice: test -> lint -> docker build -> run -> verify devops-learn local projects/api_platform +# Run a real Terraform vertical slice: fmt -> init -> validate -> plan -> risk analysis +# (plan requires Azure credentials; fails cleanly with an explanation without them) +devops-learn terraform + # Simulate the full cloud workflow (no credentials, no cost) devops-learn analyze projects/api_platform --mode collaborative --depth learning diff --git a/docs/development.md b/docs/development.md index b150976..01c5247 100644 --- a/docs/development.md +++ b/docs/development.md @@ -35,6 +35,11 @@ devops-learn init projects/api_platform # Real local vertical slice: test -> lint -> docker build -> run -> verify devops-learn local projects/api_platform +# Real Terraform vertical slice: fmt -> init -> validate -> plan -> risk analysis +# (plan requires Azure credentials -- az login, or ARM_CLIENT_ID/ARM_CLIENT_SECRET/ +# ARM_TENANT_ID/ARM_SUBSCRIPTION_ID -- and fails cleanly, with an explanation, without them) +devops-learn terraform + # Simulate the full cloud workflow (no credentials, no cost) devops-learn analyze projects/api_platform --mode collaborative --depth learning @@ -84,7 +89,17 @@ declaration at construction time. Provide a `Simulated*Tool` for tests and environments where the real dependency is unavailable, and a `Real*Tool` that shells out to the actual binary. See `tools/python_tool.py` and -`tools/docker_tool.py` for the pattern. +`tools/docker_tool.py` for the base pattern, and `tools/terraform_tool.py` for a `Real*Tool` +that also needs an explicit subprocess timeout and output redaction — reuse +`tools/_subprocess_safety.py`'s `run_safely()`/`redact()` rather than calling `subprocess.run` +directly, so secret-shaped output can never accidentally reach a `ToolResult` unredacted. + +Any future test that requires real Azure credentials must be gated behind +`RUN_AZURE_INTEGRATION_TESTS=1` (e.g. +`@pytest.mark.skipif(os.environ.get("RUN_AZURE_INTEGRATION_TESTS") != "1", reason="requires az +login")`), so ordinary CI and `pytest` runs never attempt to reach Azure. Nothing in the +platform needs this yet — it is the convention Milestone 3 (`docs/roadmap.md`) will use as soon +as real Azure calls exist. ## Adding a workflow command diff --git a/docs/learning-model.md b/docs/learning-model.md index b2ca996..1484e5a 100644 --- a/docs/learning-model.md +++ b/docs/learning-model.md @@ -92,6 +92,12 @@ course. When the platform generates or runs Terraform, it explains: The depth of each of those points depends on the learner profile and the current explanation depth. +`devops-learn terraform` applies this to Terraform state specifically: +`workflows/terraform_flow.py` surfaces 2-3 state/provider concepts per run, +tied to the command about to execute, rather than the full picture at once. +See `docs/terraform-state.md` for the complete reference those moments link +back to. + ## AI-generated work must be reviewable Before significant execution the platform shows: diff --git a/docs/roadmap.md b/docs/roadmap.md index 9c479d1..3680643 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -26,15 +26,20 @@ allow-listed subprocess wrapper, gated by `ToolService.invoke`. offline/no-Docker environments. The `devops-learn local` command runs the full local vertical slice end to end and teaches Docker concepts in context. -## Milestone 2: real Terraform workflow - -Add `RealTerraformTool` shelling out to a real `terraform` binary for -`fmt`/`validate`/`plan` against generated configuration, with structured -parsing of `terraform show -json` feeding the existing -`validation/terraform_plan_analysis.analyze()` risk classifier unchanged. No -`apply` and no Azure credentials required yet; -`apply_approved_plan`/`destroy_approved_environment` stay simulated until -Milestone 3. +## Milestone 2: real Terraform workflow (done) + +`RealTerraformTool` (`tools/terraform_tool.py`) shells out to a real +`terraform` binary for `fmt`/`init`/`validate`/`plan` against a real, +committed configuration (`projects/api_platform/infra/terraform/`), with +structured parsing of `terraform show -json` feeding the existing +`validation/terraform_plan_analysis.analyze()` risk classifier unchanged. +`devops-learn terraform` runs the full vertical slice (fmt -> init -> +validate -> plan -> risk analysis) and teaches Terraform state, provider, +and resource-address concepts just-in-time (`docs/terraform-state.md`). No +`apply`; `apply_approved_plan`/`destroy_approved_environment` are not even +declared on `RealTerraformTool` and stay simulated until Milestone 3. `plan` +requires Azure authentication and fails cleanly, with an explanation, on a +machine without it — `init`/`validate`/`fmt` need no credentials. ## Milestone 3: first Azure vertical slice diff --git a/docs/safety.md b/docs/safety.md index a5547fa..eb15dea 100644 --- a/docs/safety.md +++ b/docs/safety.md @@ -10,6 +10,13 @@ - `devops-learn local ` runs real `pytest`, `flake8`, `docker build`, `docker run`, `docker logs`, and `docker stop` against the project you provide, and performs a real HTTP health check against the running container. +- `devops-learn terraform` runs real `terraform fmt`, `init`, `validate`, and `plan` (including + `terraform show -json`) against `projects/api_platform/infra/terraform/`. It never applies or + destroys anything. `init` downloads the `azurerm` provider plugin (network access, no Azure + credentials required); `plan` requires Azure authentication (`az login`, or + `ARM_CLIENT_ID`/`ARM_CLIENT_SECRET`/`ARM_TENANT_ID`/`ARM_SUBSCRIPTION_ID`) to build the + provider client and fails cleanly, with an explanation, without it — see "Real Terraform + execution" below. - The platform's own persistence (SQLite), audit log, decisions, experience tracking, and learner profile are real. - The reference Dockerfile, Terraform configuration, Kubernetes manifest, and GitHub Actions @@ -60,6 +67,27 @@ resources, applies Terraform, or modifies remote state. It: - stops the container at the end of the workflow, - clearly labels every real result with `(real)`. +## Real Terraform execution + +`devops-learn terraform` is the only command that uses `RealTerraformTool` +(`tools/terraform_tool.py`) — `devops-learn analyze --real-tools` and `devops-learn local` both +still use `SimulatedTerraformTool` for the `terraform` tool name; only `python`/`docker` become +real for those commands. `RealTerraformTool` declares only `fmt`/`init`/`validate`/`plan` — +`apply_approved_plan`/`destroy_approved_environment` are not implemented on it at all, so +invoking either raises `KeyError` rather than silently falling back to simulated behavior. Real +apply/destroy stay simulated until Milestone 3 (`docs/roadmap.md`). + +Every subprocess call goes through `tools/_subprocess_safety.py::run_safely`, which sets an +explicit timeout and always redacts secret-shaped `KEY=value` text and truncates long output +before it reaches a `ToolResult`. `plan()`'s parsing of `terraform show -json` never extracts +raw resource attribute values (`before`/`after`) — only `{address, action}` per resource change +— which is the primary defense against leaking sensitive plan data; free-text redaction is +defense in depth on top of that. + +A `terraform plan` failure due to missing Azure credentials is an expected, honest outcome on a +machine with no `az login`/`ARM_*` environment variables configured — `workflows/terraform_flow.py` +diagnoses it explicitly rather than treating it as a bug, and never fabricates a successful plan. + ## Terraform plan risk `validation/terraform_plan_analysis.analyze()` classifies a plan's `create`/`change`/`replace`/ diff --git a/docs/terraform-state.md b/docs/terraform-state.md new file mode 100644 index 0000000..d384451 --- /dev/null +++ b/docs/terraform-state.md @@ -0,0 +1,94 @@ +# Terraform state + +`devops-learn terraform` (`workflows/terraform_flow.py`) surfaces a subset of +these concepts just-in-time, right before the command that makes them +relevant -- not as a front-loaded course. This doc is the fuller reference +those in-flow moments link back to. + +## Desired state vs. actual infrastructure vs. Terraform state + +Three things, easy to conflate: + +- **Desired state**: what your `.tf` files declare should exist. +- **Actual infrastructure**: what really exists in Azure right now. +- **Terraform state**: Terraform's own record of what it believes exists, + stored in `terraform.tfstate`. This is neither of the other two -- it's + Terraform's memory, built from the last successful apply (or import). + +`terraform plan` diffs desired state against Terraform's state file, then +confirms that against the real provider API. All three can disagree with +each other; when state and actual infrastructure disagree, that's drift. + +## Provider + +A provider is the plugin Terraform uses to talk to a specific platform -- +`azurerm` for Azure. `terraform init` downloads it. Every resource type's +prefix (`azurerm_resource_group`, `azurerm_container_registry`) names the +provider that owns it. + +## Resource address + +A resource's address (e.g. `azurerm_resource_group.main`) combines its type +and a local name. Terraform uses addresses as keys into the state file and to +build the dependency graph between resources -- e.g. the container registry +in `main.tf` implicitly depends on the resource group because it references +`azurerm_resource_group.main.name`. + +## Dependencies + +Terraform builds a dependency graph from resource references (not file +order) and applies/destroys in graph order: dependencies before dependents on +create, dependents before dependencies on destroy. + +## Drift + +Drift is when the real infrastructure no longer matches what Terraform's +state file says it should be -- usually because someone changed a resource +outside Terraform (Azure Portal, `az` CLI, another tool). `terraform plan` +detects drift by refreshing state against the real provider before diffing, +so a plan can show changes even though nothing in your `.tf` files changed. + +## Local vs. remote state, and locking + +This milestone only uses **local state**: `terraform.tfstate` sits in the +working directory. That's fine solo, but two people (or two CI runs) applying +concurrently against local state can corrupt it. **Remote state** (e.g. an +Azure Storage blob) centralizes the file and typically adds **locking**, so +only one `apply` can run against a given state file at a time. Remote state +and locking are out of scope for this milestone; see `docs/roadmap.md`. + +## State sensitivity + +State can contain resource attribute values, including ones marked sensitive +by a provider (connection strings, generated passwords, keys). This is why +`terraform.tfstate*` is gitignored (see `.gitignore`) -- committing it would +commit those values in plaintext into version control history. + +## Why deleting state does not delete infrastructure + +`terraform.tfstate` is Terraform's memory, not a control plane. Deleting it +does not touch Azure at all -- it only makes Terraform forget every resource +it was tracking. Run `terraform plan` afterward and every real resource would +show up as something Terraform wants to *create*, because as far as +Terraform now knows, nothing exists yet. The real resources are still there, +now unmanaged and orphaned from Terraform's perspective. + +## Why hand-editing state is dangerous + +State's internal schema is version-specific and includes fields (resource +dependency ordering, provider-specific metadata) that aren't meant to be +edited by hand. A manual edit that doesn't match reality can cause Terraform +to plan a destructive "fix" -- e.g. destroying and recreating a resource +Terraform now believes doesn't match its record, even though the real +resource is fine. Use `terraform state` subcommands (`mv`, `rm`, `import`) +instead of editing the file directly; even those require care and are +DESTRUCTIVE-adjacent operations in this platform's risk model (see +`docs/safety.md`). + +## Real vs. simulated in this platform + +`devops-learn terraform` runs real `fmt`/`init`/`validate`/`plan` against +`projects/api_platform/infra/terraform/` and never applies or destroys. +`apply_approved_plan`/`destroy_approved_environment` -- and therefore any +real state file this platform would ever write -- stay simulated until +Milestone 3. See `docs/safety.md` and `docs/roadmap.md`. From da6a1a2394b58118720f698fff6ae0a1b1951c17 Mon Sep 17 00:00:00 2001 From: howlcipher Date: Wed, 12 Aug 2026 16:55:45 -0400 Subject: [PATCH 6/6] Add project CLAUDE.md, review subagents, and a destructive-command hook CLAUDE.md: durable rules on product direction, the Tool/ToolService gateway, real-vs-simulated labeling, and quality gates. .claude/agents/: terraform-reviewer, security-reviewer, learning-reviewer -- read-only review personas used during this milestone's own development. .claude/hooks/confirm_destructive_command.py: a PreToolUse Bash hook that forces explicit confirmation before terraform apply/destroy, az group/ resource delete, git push --force, and similar commands run directly via Bash, as a backstop for when a command bypasses ToolService entirely. Co-Authored-By: Claude Sonnet 5 --- .claude/agents/learning-reviewer.md | 19 +++++ .claude/agents/security-reviewer.md | 20 +++++ .claude/agents/terraform-reviewer.md | 20 +++++ .claude/hooks/confirm_destructive_command.py | 70 +++++++++++++++ .claude/settings.json | 15 ++++ CLAUDE.md | 90 ++++++++++++++++++++ 6 files changed, 234 insertions(+) create mode 100644 .claude/agents/learning-reviewer.md create mode 100644 .claude/agents/security-reviewer.md create mode 100644 .claude/agents/terraform-reviewer.md create mode 100755 .claude/hooks/confirm_destructive_command.py create mode 100644 .claude/settings.json create mode 100644 CLAUDE.md diff --git a/.claude/agents/learning-reviewer.md b/.claude/agents/learning-reviewer.md new file mode 100644 index 0000000..e3c9db6 --- /dev/null +++ b/.claude/agents/learning-reviewer.md @@ -0,0 +1,19 @@ +--- +name: learning-reviewer +description: Verifies the Terraform workflow actually teaches, not just automates — checks just-in-time moments, ExplanationDepth gating, and evidence recording. Use after any diff touching src/devops_learn/workflows/terraform_flow.py, docs/terraform-state.md, docs/learning-model.md, docs/roadmap.md, or docs/safety.md. +tools: Read, Grep, Glob +--- + +You evaluate whether this milestone genuinely closes the user's Terraform/Azure knowledge gaps, per `docs/learning-model.md`, or merely automates the work. You are read-only: report findings, do not edit anything. + +Check specifically: + +- `workflows/terraform_flow.py` calls `_teach_state_concepts` before `fmt`, `_teach_provider_concept` before `init`, and the post-plan "what to check" explanation after a successful plan — confirm these land at the point where the concept is actually relevant, not front-loaded before anything runs. +- Teaching moments use `LearningMoment`/`Explanation` via `ExplanationService.render`/`render_learning_moment`, gated by `ExplanationDepth`, not hardcoded to always or never show — confirm the depth-gating logic in `explanations/service.py` is genuinely exercised here (mode/depth aren't hardcoded to force max verbosity every time). +- A `terraform plan` failure due to missing Azure credentials produces a real diagnosis (`_diagnose_plan_failure`) explaining `az login`/`ARM_CLIENT_ID`/`ARM_CLIENT_SECRET`/`ARM_TENANT_ID`/`ARM_SUBSCRIPTION_ID` as the WHY, not just a bare failure message — this is the expected outcome on a credential-less machine and should be a genuine learning moment, not treated as an error to hide. +- `ExperienceTracker.record` calls in `terraform_flow.py` use only existing `CompetencyArea` (`terraform`, `azure`) and `ExperienceState` (`introduced`, `practiced`, `demonstrated`, etc.) vocabulary — no invented competency areas or states. +- `docs/terraform-state.md` actually covers: desired vs. actual state, the state file, provider, resource address, dependencies, drift, local vs. remote state, locking, state sensitivity, why deleting state ≠ deleting infrastructure, why hand-editing state is dangerous. +- `docs/terraform-state.md` is cross-linked from `docs/learning-model.md`'s "Just-in-time learning" section. +- `docs/roadmap.md`/`docs/safety.md`/README updates describe only what is genuinely implemented this milestone (real `fmt`/`init`/`validate`/`plan` against the bundled sample project; no real Azure execution, no `apply`/`destroy`) — flag any language that overstates real Azure deployment having happened. + +Report findings ranked BLOCKING/HIGH/MEDIUM/LOW with the specific file and line. diff --git a/.claude/agents/security-reviewer.md b/.claude/agents/security-reviewer.md new file mode 100644 index 0000000..0c6df89 --- /dev/null +++ b/.claude/agents/security-reviewer.md @@ -0,0 +1,20 @@ +--- +name: security-reviewer +description: Reviews for secret leakage, subprocess safety, and approval-gate integrity across RealTerraformTool, CI, and Claude Code hooks. Use after any diff touching src/devops_learn/tools/terraform_tool.py, src/devops_learn/tools/_subprocess_safety.py, .github/workflows/ci.yml, or .claude/settings.json. +tools: Read, Grep, Glob, Bash +--- + +You review this repo for credential handling, subprocess safety, and approval-gate integrity. You are read-only: report findings, do not fix them yourself. + +Check specifically: + +- `tools/_subprocess_safety.py`'s `redact()`/`run_safely()` are actually applied to every stdout/stderr that lands in a `ToolResult.details` from `RealTerraformTool` — grep for any `result.stdout`/`result.stderr` usage that bypasses `_subprocess_safety`. +- `RealTerraformTool.plan()`'s parsing of `terraform show -json` (`_details_from_plan_json`) never extracts `resource_changes[].change.before`/`.after` — only `{address, action}`. This is the primary defense against leaking sensitive plan data; confirm no future edit reintroduced raw attribute access. +- Every subprocess call in `terraform_tool.py` goes through `run_safely(..., timeout=...)` with an explicit timeout — none should call `subprocess.run` directly. +- `ToolOperationSpec` invariants hold: any operation with `risk_level=RiskLevel.DESTRUCTIVE` has `requires_approval=True` and `supports_dry_run=False` (enforced by `__post_init__`, but confirm no new operation tries to work around it, e.g. via a different risk level for something that is actually destructive). +- `.github/workflows/ci.yml`'s Terraform steps never reference `ARM_CLIENT_ID`/`ARM_CLIENT_SECRET`/`ARM_TENANT_ID`/`ARM_SUBSCRIPTION_ID` or any Azure credential, and never run `terraform plan`/`apply` (only `fmt -check`/`init -backend=false`/`validate`, none of which need credentials). +- `.gitignore` covers `.terraform/`, `*.tfstate*`, and crash logs; `.terraform.lock.hcl` is NOT ignored. +- `.claude/settings.json`'s `PreToolUse` hook (`.claude/hooks/confirm_destructive_command.py`) actually matches `terraform apply`, `terraform destroy`, `terraform state rm`, `az group delete`, `az resource delete`, `az role assignment (create|delete)`, `rm terraform.tfstate*`, `rm -rf .terraform`, and `git push --force` — test it yourself with sample stdin JSON rather than trusting the regex by inspection alone. +- No API keys, tokens, `.env` contents, or Terraform state ever appear in a commit, test fixture, or doc in this diff — grep for anything secret-shaped. + +Report findings ranked BLOCKING/HIGH/MEDIUM/LOW with the specific file and line. diff --git a/.claude/agents/terraform-reviewer.md b/.claude/agents/terraform-reviewer.md new file mode 100644 index 0000000..14a7e25 --- /dev/null +++ b/.claude/agents/terraform-reviewer.md @@ -0,0 +1,20 @@ +--- +name: terraform-reviewer +description: Reviews changes to *.tf files and RealTerraformTool for correctness and idiomatic Terraform. Use after any diff touching projects/api_platform/infra/terraform/ or src/devops_learn/tools/terraform_tool.py. +tools: Read, Grep, Glob, Bash +--- + +You review Terraform configuration and the RealTerraformTool implementation in this repo. You are read-only: report findings, do not rewrite the implementation. + +Check specifically: + +- Provider version is pinned (`~> X.Y`, never unpinned) in `providers.tf`. +- Resource naming and tags are consistent with `main.tf` (`project`, `environment`, `managed-by`, `purpose`). +- `.gitignore` correctly excludes `.terraform/` and `*.tfstate*` while `.terraform.lock.hcl` stays committed (verify it is not accidentally ignored). +- The `providers.tf`/`variables.tf`/`main.tf`/`outputs.tf` split in `projects/api_platform/infra/terraform/` stays a naming convention only — Terraform merges all `.tf` files in the directory, so nothing should assume module-style isolation between them. +- `RealTerraformTool` (`src/devops_learn/tools/terraform_tool.py`) only declares `fmt`/`init`/`validate`/`plan` operations, all `RiskLevel.SAFE` with `requires_approval=False` — flag immediately if `apply_approved_plan`/`destroy_approved_environment` are ever added to it (that's Milestone 3 territory and must go through `ApprovalGate`, not this tool). +- `terraform show -json` action-mapping (`_details_from_plan_json`) correctly classifies `("create",)`/`("update",)`/`("delete","create")` or `("create","delete")`/`("delete",)` into create/change/replace/destroy, and ignores `("no-op",)`/`("read",)`. +- No `shell=True` anywhere; every subprocess call is an argument list through `_subprocess_safety.run_safely`. +- You can run `terraform fmt -check -recursive` and `terraform validate` (after `terraform init -backend=false`) yourself against `projects/api_platform/infra/terraform/` to verify claims rather than reading code only. + +Report findings ranked BLOCKING/HIGH/MEDIUM/LOW with the specific file and line. diff --git a/.claude/hooks/confirm_destructive_command.py b/.claude/hooks/confirm_destructive_command.py new file mode 100755 index 0000000..b422c84 --- /dev/null +++ b/.claude/hooks/confirm_destructive_command.py @@ -0,0 +1,70 @@ +#!/usr/bin/env python3 +"""PreToolUse hook: forces explicit user confirmation before a Bash command +that could destroy infrastructure or discard history runs, instead of +letting it execute silently. + +This is a defense-in-depth backstop, not the platform's real approval +mechanism -- the platform's own Terraform/Azure/Docker execution always goes +through ToolService/ApprovalGate regardless of this hook (see +docs/adr/0004-controlled-tool-execution.md). This hook only matters when a +command bypasses that layer entirely, e.g. an agent running `terraform +apply` directly via the Bash tool instead of through RealTerraformTool. + +Reads the PreToolUse JSON payload from stdin, and if tool_input.command +matches a dangerous pattern, prints a permissionDecision "ask" response so +the normal permission prompt fires instead of silent execution. Any other +command is left untouched (no stdout, exit 0) -- this must not add friction +to routine commands like `terraform fmt`/`terraform plan`/`pytest`. +""" +from __future__ import annotations + +import json +import re +import sys + +_DANGEROUS_PATTERNS = ( + r"\bterraform\s+apply\b", + r"\bterraform\s+destroy\b", + r"\bterraform\s+state\s+rm\b", + r"\baz\s+group\s+delete\b", + r"\baz\s+resource\s+delete\b", + r"\baz\s+role\s+assignment\s+(create|delete)\b", + r"\brm\s+.*terraform\.tfstate", + r"\brm\s+-rf\s+.*\.terraform\b", + r"\bgit\s+push\b.*(--force\b|(? int: + try: + payload = json.load(sys.stdin) + except (json.JSONDecodeError, ValueError): + return 0 + + tool_input = payload.get("tool_input") if isinstance(payload, dict) else None + command = tool_input.get("command", "") if isinstance(tool_input, dict) else "" + command = str(command) + if not _COMPILED.search(command): + return 0 + + print( + json.dumps( + { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "ask", + "permissionDecisionReason": ( + "This command can destroy infrastructure or discard history " + "outside the platform's own ToolService approval gate. " + "Confirm you intend to run it before proceeding." + ), + } + } + ) + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..993f542 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "python3 \"${CLAUDE_PROJECT_DIR}/.claude/hooks/confirm_destructive_command.py\"" + } + ] + } + ] + } +} diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..285966d --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,90 @@ +# DevOps-Learn-by-Doing + +## Product + +This is an AI-assisted DevOps mastery platform, not an automation tool and +not an anti-AI exercise. The goal is real engineering work performed +together with AI in a way that deliberately closes the user's knowledge +gaps. AI automation and learner understanding are two separate dimensions: +running a command for the user is not the same as the user understanding +why it ran or what it did. See `docs/learning-model.md`. Never equate manual +typing with expertise, and never let AI execution stand in for competency +evidence — evidence lives in `ExperienceTracker`/`ExperienceState` +(`experience/tracker.py`, `domain/enums.py`). + +## Architecture + +Modular monolith (`docs/architecture.md`, `docs/adr/0001`). `bootstrap.py` +is the single composition root — all concrete implementations are wired +there via constructor injection, never a service locator. `domain/` depends +on nothing; `tools/`/`cloud/`/`ai/` depend only on `domain/`; +`workflows/`/`bootstrap.py` depend on everything; `cli/` depends only on +`bootstrap.py`/`workflows/`/`domain/`. Preserve this layering unless there +is a strong reason to change it. + +Every capability the platform can execute is a `Tool` (`tools/base.py`). +`ToolService.invoke` (`tools/service.py`) is the only caller of +`Tool.execute` anywhere in the codebase — this is what makes "destructive +operations require human approval" structural rather than a convention any +one caller has to remember. Never bypass `ToolService` for convenience, and +never call `Tool.execute` directly outside it. + +LLMs explain and reason; deterministic code decides. `LLMProvider` +(`ai/provider.py`) has no method that invokes a tool or makes a decision — +it only produces freeform explanation text. Recommendations, risk +classification, and plans are typed dataclasses produced by deterministic +services (`docs/adr/0008-structured-ai-output.md`), never freeform LLM +output parsed at runtime. + +## Safety + +- Never fabricate successful execution. Every real result is explicitly + labeled `(real)`/`(real, failed)`; every simulated result says + `(simulated)`. See `docs/safety.md` for the current real-vs-simulated + boundary — keep it accurate as implementation changes. +- `ToolOperationSpec` (`tools/base.py`) enforces at construction time that a + `RiskLevel.DESTRUCTIVE` operation must require approval and must never + support a dry-run bypass of that approval. Do not work around this. +- `ApprovalGate`/`CliApprovalGate` (`tools/approval.py`) is the only path to + approving a HIGH/DESTRUCTIVE operation; `AutoApproveApprovalGate`/ + `AutoDenyApprovalGate` exist only for tests, never for the real CLI. +- Never commit credentials, Terraform state, or `.env` files. Real + subprocess-backed tools must redact secret-shaped output before it lands + in a `ToolResult` (see `tools/_subprocess_safety.py`) and must set an + explicit timeout on every subprocess call. +- Never expose unrestricted arbitrary shell execution through a `Tool`. Real + tools shell out to a narrow, allow-listed set of operations via argument + arrays (never `shell=True`) — see `tools/docker_tool.py`, + `tools/python_tool.py`, `tools/terraform_tool.py`. + +## Product direction + +- Azure first; Terraform depth before AWS/GCP breadth (`docs/cloud-model.md`). +- Do not add Kubernetes/AKS until a real cloud deployment path is proven + reliable without it — not because Kubernetes is technically harder, but + because cluster-level complexity should sit on top of a working real + deployment path, not substitute for one (`docs/roadmap.md`). +- Prefer understandable learning infrastructure over enterprise complexity — + no module registries, landing zones, Terragrunt, or multi-subscription + setups unless a milestone genuinely requires them. +- See `docs/roadmap.md` for current milestone status before proposing new + scope. + +## Quality + +Before considering any change complete, all of the following must pass: + +``` +pytest +flake8 src tests projects/api_platform/app projects/api_platform/tests +mypy +cd projects/api_platform && mypy app --strict +``` + +Update `docs/roadmap.md`/`docs/safety.md` when implementation reality +changes — this repo's credibility depends on never claiming more than what +is genuinely implemented (e.g. do not describe real Azure execution as +working unless it has actually been verified against a real subscription). + +Do not put task-specific notes, implementation plans, or milestone journals +in this file — it is for durable project knowledge only.