feat(launchpad): STEP 4 -- goose config.yaml read-merge-write (#239 STEP 4)

.github/workflows/launchpad-agents-tests.yml

@@ -0,0 +1,81 @@
+name: launchpad — agents tests
+
+# Runs the unit tests for launchpad/agents/*.py.
+#
+# WHY THIS WORKFLOW EXISTS. Until it did, NO CI job ran anything under
+# launchpad/agents/ — review-code found the same High finding independently on
+# two separate pull requests (#260's 20 tests and #262's 25 tests, both green
+# locally and both never executed by CI). A test suite nothing runs is a claim,
+# not a check: it cannot fail, so it cannot protect anything.
+#
+# It also catches the failure mode that made the gap visible. goose_config.py
+# imports ruamel.yaml, a third-party package the repo recorded nowhere. On a
+# machine without it the module raises ImportError before a single test runs, and
+# nothing would have reported that. Installing from the requirements file here
+# means an unrecorded dependency now breaks CI rather than breaking a reader.
+#
+# `pull_request`, deliberately NOT `pull_request_target` — the suite under test
+# lives in the repository, so a pull request can modify the very code this job
+# runs. On `pull_request` that code executes with the fork's own permissions and
+# no access to repository secrets. Same reasoning as
+# launchpad-review-agent-controls.yml, which this mirrors.
+
+on:
+  pull_request:
+    paths:
+      - "launchpad/agents/**"
+      - ".github/workflows/launchpad-agents-tests.yml"
+  push:
+    branches: [launchpad]
+    paths:
+      - "launchpad/agents/**"
+
+# Read-only. These are pure unit tests against temp-directory fixtures: they
+# reach no network, no relay, and no GitHub API, so they need no token scope.
+permissions:
+  contents: read
+
+jobs:
+  tests:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.11"
+
+      - name: Install agent script dependencies
+        run: pip install -r launchpad/agents/requirements.txt
+
+      # Fails if the suite contains no test CASES, rather than reporting success
+      # for a run that executed nothing. `unittest discover` exits 0 on an empty
+      # suite, which is indistinguishable from a pass in the CI summary — the
+      # exact shape of the gap this workflow was added to close.
+      #
+      # COUNTS CASES, NOT FILES, and the first version of this guard counted
+      # files — which left the very hole it existed to close. Found by
+      # cross-vendor review: leave test_goose_config.py in place but rename every
+      # `test_*` method to `check_*`, and a file-counting guard reports one test
+      # file while `unittest discover` collects zero cases and exits 0. A guard
+      # that can be satisfied by a filename is not a guard.
+      #
+      # A module that fails to import counts as one case here (the loader
+      # substitutes a `_FailedTest`), so it passes this step and then fails the
+      # run below — which is the correct split: this step answers "is there
+      # anything to run", the run answers "does it pass".
+      - name: Confirm test cases were discovered
+        run: |
+          python3 - <<'PY'
+          import sys, unittest
+          suite = unittest.defaultTestLoader.discover("launchpad/agents", pattern="test_*.py")
+          n = suite.countTestCases()
+          print(f"discovered {n} test case(s)")
+          if n == 0:
+              print("::error::launchpad/agents has no discoverable test cases — "
+                    "this job would have passed vacuously.")
+              sys.exit(1)
+          PY
+
+      - name: Run agents unit tests
+        run: python3 -m unittest discover -s launchpad/agents -p "test_*.py" -v

launchpad/agents/goose_config.py

@@ -0,0 +1,253 @@
+#!/usr/bin/env python3
+"""Read-merge-write goose's config.yaml to enable the `developer` extension.
+
+STEP 4 of issue #239 (the Route 3 projector): goose's write/shell capability
+is a config-FILE toggle, not an env var --
+`desktop/src-tauri/src/managed_agents/config_bridge/goose.rs` only ever
+*reads* `extensions.developer` out of this file, and nothing in this
+repository writes it. This module builds read-merge-write from scratch: read
+the existing file if present (empty mapping otherwise), preserve every
+existing key untouched, set `extensions.developer = {type: builtin, enabled:
+true}`, and write atomically (temp file + rename in the same directory) so a
+crash mid-write cannot leave a half-written config an operator's next
+`goose` invocation trips over. Running it twice against the same file is a
+no-op, not an append-again.
+
+Uses ruamel.yaml's round-trip mode (not PyYAML) specifically so an operator's
+comments and quoting style survive a merge -- a plain load+dump strips both
+on every write, confirmed to lose a hand-written "# managed by ansible"
+comment and turn a quoted host string unquoted. That dependency is recorded
+in `launchpad/agents/requirements.txt` and installed in CI by
+`.github/workflows/launchpad-agents-tests.yml`; locally, either
+`pip install -r launchpad/agents/requirements.txt` or the
+`python3-ruamel.yaml` apt package works.
+
+Known limitation: no file locking across the read-modify-write window, so a
+goose process rewriting its own config.yaml at the same moment this script
+runs could lose one side's update. Each individual write stays atomic
+(temp file + rename), so this is a lost-update race, not corruption -- the
+plan's own atomicity requirement is about surviving a crash mid-write, not
+about concurrent writers, and this module does not attempt the latter.
+
+Does not wire into project-pack.py (STEP 5) -- this is the goose-config half
+only.
+
+Usage:
+    python3 launchpad/agents/goose_config.py --enable-developer
+"""
+
+from __future__ import annotations
+
+import argparse
+import os
+import stat
+import sys
+import tempfile
+from pathlib import Path
+
+from ruamel.yaml import YAML
+from ruamel.yaml.comments import CommentedMap
+
+
+class GooseConfigError(RuntimeError):
+    """goose's config.yaml could not be read or merged -- always fails
+    loudly, never silently discards or guesses at data the caller would
+    act on."""
+
+
+def _yaml() -> YAML:
+    y = YAML()
+    y.preserve_quotes = True
+    return y
+
+
+def goose_config_path(env: dict | None = None) -> Path:
+    """Mirrors goose.rs's `goose_config_path()` exactly, including its
+    edge case: `std::env::var("GOOSE_PATH_ROOT")` returns `Ok("")` for a
+    set-but-empty variable, so Rust does NOT treat empty as unset -- an
+    explicitly set (even empty) GOOSE_PATH_ROOT wins here too, rather than
+    silently falling back to a different path than the one goose itself
+    would resolve."""
+    env = env if env is not None else os.environ
+    if "GOOSE_PATH_ROOT" in env:
+        return Path(env["GOOSE_PATH_ROOT"]) / "config" / "config.yaml"
+    return Path.home() / ".config" / "goose" / "config.yaml"
+
+
+def read_config(path: Path) -> CommentedMap:
+    """The parsed mapping at `path`, or an empty mapping if the file does
+    not exist or is empty. Raises GooseConfigError on invalid YAML or a
+    non-mapping top-level value, rather than crashing with a raw
+    exception or silently discarding the file's contents."""
+    if not path.exists():
+        return CommentedMap()
+    try:
+        with path.open("r", encoding="utf-8") as f:
+            loaded = _yaml().load(f)
+    except Exception as exc:
+        raise GooseConfigError(f"{path} is not valid YAML: {exc}") from exc
+    if loaded is None:
+        return CommentedMap()
+    if not isinstance(loaded, dict):
+        raise GooseConfigError(
+            f"{path}'s top-level YAML value is a {type(loaded).__name__}, "
+            "not a mapping -- refusing to merge into it"
+        )
+    return loaded
+
+
+def merge_developer_extension(config: dict) -> dict:
+    """Returns a NEW mapping with `extensions.developer` enabled. Every
+    other top-level key, and every other extension, is preserved untouched
+    (comments and quoting included, when `config` came from `read_config`).
+    Idempotent: merging an already-merged mapping returns an equal
+    mapping -- `developer`'s existing position in `extensions` is kept
+    rather than moved to the end, so a second write matches the first
+    byte-for-byte.
+
+    Raises GooseConfigError if an existing `extensions` key is present but
+    is not itself a mapping."""
+    merged = config.copy() if isinstance(config, CommentedMap) else CommentedMap(config)
+    raw_extensions = merged.get("extensions")
+    if raw_extensions is None:
+        extensions = CommentedMap()
+    elif isinstance(raw_extensions, dict):
+        extensions = (
+            raw_extensions.copy()
+            if isinstance(raw_extensions, CommentedMap)
+            else CommentedMap(raw_extensions)
+        )
+    else:
+        raise GooseConfigError(
+            f"'extensions' is a {type(raw_extensions).__name__}, not a "
+            "mapping -- refusing to merge into it"
+        )
+    extensions["developer"] = {"type": "builtin", "enabled": True}
+    merged["extensions"] = extensions
+    return merged
+
+
+def write_config_atomic(path: Path, config: dict) -> None:
+    """Writes `config` to `path` via a temp file in the same directory,
+    then an atomic rename over the original -- a crash mid-write leaves
+    either the old file or the new one, never a half-written one.
+
+    If `path` is a symlink, writes through it (onto the resolved real
+    target) rather than replacing the symlink itself -- otherwise a
+    dotfile-managed config (Stow, chezmoi, a manual symlink into a synced
+    repo) silently loses its symlink on the first run.
+
+    If `path` already exists, the new file keeps its permission mode
+    (`tempfile.mkstemp` otherwise always creates at 0600, which would
+    silently narrow an existing 0644 file's permissions on every write)
+    and its LF-or-CRLF line-ending convention (a CRLF file written by a
+    Windows operator was silently rewritten to LF, which is the same class
+    of unasked-for edit as losing their comments -- and this repository does
+    support Windows, so it is not hypothetical). A file containing any CRLF
+    is treated as a CRLF file; mixed endings are normalised to CRLF rather
+    than preserved per-line. LF and CRLF are the only conventions handled:
+    a lone-CR (pre-OS X Mac) file normalises to LF, which is a limitation
+    of this function rather than a preserved convention."""
+    path.parent.mkdir(parents=True, exist_ok=True)
+    real_path = path.resolve() if path.is_symlink() else path
+    real_path.parent.mkdir(parents=True, exist_ok=True)
+
+    existing_mode = None
+    newline = "\n"
+    if real_path.exists():
+        existing_mode = real_path.stat().st_mode
+        # Read in full rather than sampling a prefix: a file can be LF for a
+        # hundred lines and CRLF after. These configs are a few KB at most.
+        if b"\r\n" in real_path.read_bytes():
+            newline = "\r\n"
+
+    fd, tmp_name = tempfile.mkstemp(
+        dir=str(real_path.parent), prefix=f".{real_path.name}.", suffix=".tmp"
+    )
+    try:
+        if existing_mode is not None:
+            os.chmod(fd, stat.S_IMODE(existing_mode))
+        # newline= is what preserves the convention: the YAML dumper always
+        # emits "\n", and Python translates it on the way out.
+        with os.fdopen(fd, "w", encoding="utf-8", newline=newline) as f:
+            _yaml().dump(config, f)
+        os.replace(tmp_name, real_path)
+    except Exception:
+        try:
+            os.unlink(tmp_name)
+        except OSError:
+            pass
+        raise
+
+
+def enable_developer_extension(
+    path: Path | None = None, env: dict | None = None
+) -> Path:
+    """Read-merge-write entry point: enables goose's `developer` extension
+    at `path` (default: `goose_config_path(env)`). Returns the path
+    written.
+
+    WHAT THIS GRANTS, STATED WHERE IT HAPPENS. goose's `developer` extension is
+    its shell-and-filesystem tool: enabling it gives the agent that loads this
+    config the ability to run commands and write files as the invoking user.
+    That is the entire point for #239 (The Professor could draft a page but had
+    no tool that could save it), and it is also a real expansion of blast
+    radius.
+
+    The issue-#239 plan's OPEN item 2 records that this decision is NOT settled
+    for live or unattended operation, and names who must settle it: "Who
+    arbitrates whether goose's `developer` extension is safe enough to enable
+    for a live/unattended run later -- not decided here [...] a live, unattended
+    agent with real shell access is a materially different blast radius than a
+    human-triggered local session under BYOK."
+
+    So this function is scoped to the human-triggered local proof the plan
+    sanctions (STEP 7). It must not be wired into an unattended or
+    cohort-facing runtime until that OPEN item has an answer, and this docstring
+    is deliberately the place a reader finds that out -- the plan is not in
+    scope for someone reading the module.
+    """
+    target = path if path is not None else goose_config_path(env)
+    current = read_config(target)
+    merged = merge_developer_extension(current)
+    write_config_atomic(target, merged)
+    return target
+
+
+def main(argv: list[str] | None = None) -> int:
+    # First line only, matching project-pack.py's own `__doc__.splitlines()[0]`.
+    # The full docstring is 40+ lines of rationale aimed at a reader of the
+    # source; dumping all of it into `--help` buries the two flags a caller
+    # actually needs to see.
+    parser = argparse.ArgumentParser(description=__doc__.splitlines()[0])
+    parser.add_argument(
+        "--enable-developer",
+        action="store_true",
+        help="enable goose's developer (shell/write) extension in config.yaml",
+    )
+    parser.add_argument(
+        "--path",
+        type=Path,
+        default=None,
+        help=(
+            "override goose's config.yaml path (default: GOOSE_PATH_ROOT or "
+            "~/.config/goose/config.yaml)"
+        ),
+    )
+    args = parser.parse_args(argv)
+
+    if not args.enable_developer:
+        parser.error("nothing to do -- pass --enable-developer")
+
+    try:
+        target = enable_developer_extension(path=args.path)
+    except GooseConfigError as exc:
+        print(f"error: {exc}", file=sys.stderr)
+        return 1
+
+    print(f"enabled developer extension in {target}", file=sys.stderr)
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())

launchpad/agents/requirements.txt

@@ -0,0 +1,37 @@
+# Third-party dependencies for launchpad/agents/*.py.
+#
+# WHY THIS FILE EXISTS. `goose_config.py` imports ruamel.yaml, and until this file
+# existed that dependency was recorded nowhere: not in a manifest, not in a CI
+# install step, only in prose inside the module's own docstring. A reader who
+# cloned the repo and ran the script got an ImportError, and CI could not have
+# caught it because no CI job ran these tests at all
+# (.github/workflows/launchpad-agents-tests.yml now does).
+#
+# ruamel.yaml rather than PyYAML, deliberately, and it is not interchangeable:
+# goose_config.py rewrites an operator's own ~/.config/goose/config.yaml, and
+# PyYAML's load+dump silently strips every comment and re-quotes every scalar on
+# the way through. Measured on a fixture carrying `# managed by ansible` and a
+# quoted host value: both were gone after one round trip. ruamel's round-trip
+# mode preserves them. Anything that edits a human-authored YAML file in place
+# needs the round-trip parser; PyYAML is fine for files only machines write.
+#
+# BOUNDED RANGE, both ends deliberate.
+#
+# Floor 0.17.21: that is what the python3-ruamel.yaml apt package ships on
+# Ubuntu 24.04, and pinning above it would make the apt route unusable for no
+# gain. (An earlier revision of this comment claimed an exact pin "would reject
+# the newer wheel pip installs" — that was simply wrong, pip installs whatever
+# version you name if it is still on the index. Corrected by cross-vendor
+# review; the real reason for a range is the apt/pip split, not pip behaviour.)
+#
+# Ceiling <0.20: a conservative "do not enter an untested release series"
+# boundary, NOT a major-version boundary. ruamel.yaml's own documentation puts
+# its eventual major transition at 1.0, so 0.20 is simply the next 0.x release
+# series -- an earlier revision of this comment called it "the next major
+# series", which was wrong (corrected by cross-vendor review). The reason for
+# any ceiling at all is that this module's whole purpose is round-tripping an
+# operator's hand-written config, so a serializer change in a release series
+# nobody here has tested could silently alter their file with no commit in this
+# repo to point at. Raise it deliberately, after checking round-tripping still
+# holds, rather than leaving it open.
+ruamel.yaml>=0.17.21,<0.20