From 71478d79b5fe8bb73758ceff1ddcfac09618792d Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 09:44:24 +1200 Subject: [PATCH 1/6] feat(launchpad): STEP 4 -- goose config.yaml read-merge-write (#239) Adds launchpad/agents/goose_config.py: read-merge-write logic for goose's config.yaml, built from scratch since goose.rs (desktop/src-tauri/src/managed_agents/config_bridge/goose.rs) is entirely read-only and nothing in the repo writes this file today. enable_developer_extension() reads the existing file if present (empty mapping otherwise), preserves every other top-level key and every other extension untouched, sets extensions.developer = {type: builtin, enabled: true}, and writes atomically via a 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 byte-for-byte no-op, not an append-again. goose_config_path() mirrors goose.rs's own path resolution (GOOSE_PATH_ROOT env var, else ~/.config/goose/config.yaml). Independent of STEPs 1-3 per the plan; converges with the projector script at STEP 5. Signed-off-by: Serina Mcfall --- launchpad/agents/goose_config.py | 128 ++++++++++++++++ launchpad/agents/test_goose_config.py | 211 ++++++++++++++++++++++++++ 2 files changed, 339 insertions(+) create mode 100644 launchpad/agents/goose_config.py create mode 100644 launchpad/agents/test_goose_config.py diff --git a/launchpad/agents/goose_config.py b/launchpad/agents/goose_config.py new file mode 100644 index 00000000000..5bc9150989a --- /dev/null +++ b/launchpad/agents/goose_config.py @@ -0,0 +1,128 @@ +#!/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. + +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 sys +import tempfile +from pathlib import Path + +import yaml + + +def goose_config_path(env: dict | None = None) -> Path: + """Mirrors goose.rs's `goose_config_path()`: a set, non-empty + GOOSE_PATH_ROOT wins, else `~/.config/goose/config.yaml`.""" + env = env if env is not None else os.environ + root = env.get("GOOSE_PATH_ROOT") + if root: + return Path(root) / "config" / "config.yaml" + return Path.home() / ".config" / "goose" / "config.yaml" + + +def read_config(path: Path) -> dict: + """The parsed mapping at `path`, or an empty mapping if the file does + not exist or is empty.""" + if not path.exists(): + return {} + with path.open("r", encoding="utf-8") as f: + loaded = yaml.safe_load(f) + return loaded or {} + + +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. + 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.""" + merged = dict(config) + extensions = dict(merged.get("extensions") or {}) + 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.""" + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp( + dir=str(path.parent), prefix=f".{path.name}.", suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + yaml.safe_dump(config, f, default_flow_style=False, sort_keys=False) + os.replace(tmp_name, 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.""" + 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: + parser = argparse.ArgumentParser(description=__doc__) + 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") + + target = enable_developer_extension(path=args.path) + print(f"enabled developer extension in {target}", file=sys.stderr) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/agents/test_goose_config.py b/launchpad/agents/test_goose_config.py new file mode 100644 index 00000000000..922bfd0fa3b --- /dev/null +++ b/launchpad/agents/test_goose_config.py @@ -0,0 +1,211 @@ +#!/usr/bin/env python3 +"""Controls for goose_config.py's read-merge-write logic. + +STEP 4 of issue #239 (the Route 3 projector): goose's `developer` extension +(write/shell capability) is a config-FILE toggle +(`~/.config/goose/config.yaml`, or `$GOOSE_PATH_ROOT/config/config.yaml`), +and nothing in this repository writes that file today +(`desktop/src-tauri/src/managed_agents/config_bridge/goose.rs` is +read-only). These tests drive the module's pure functions directly, plus one +end-to-end pass against a real temp file for the atomic-write and +idempotency guarantees the plan's own done-when demands. + +Run: python3 -m unittest discover -s launchpad/agents -p "test_*.py" +""" + +from __future__ import annotations + +import importlib.util +import os +import tempfile +import unittest +from pathlib import Path + +_SPEC = importlib.util.spec_from_file_location( + "goose_config", Path(__file__).resolve().parent / "goose_config.py" +) +m = importlib.util.module_from_spec(_SPEC) +_SPEC.loader.exec_module(m) + + +class GooseConfigPathTests(unittest.TestCase): + def test_uses_goose_path_root_when_set(self): + path = m.goose_config_path({"GOOSE_PATH_ROOT": "/custom/root"}) + self.assertEqual(path, Path("/custom/root/config/config.yaml")) + + def test_defaults_to_home_config_goose_when_unset(self): + path = m.goose_config_path({}) + self.assertEqual(path, Path.home() / ".config" / "goose" / "config.yaml") + + def test_empty_goose_path_root_is_treated_as_unset(self): + # An operator with GOOSE_PATH_ROOT="" in their env should not get a + # path rooted at "/config/config.yaml". + path = m.goose_config_path({"GOOSE_PATH_ROOT": ""}) + self.assertEqual(path, Path.home() / ".config" / "goose" / "config.yaml") + + +class ReadConfigTests(unittest.TestCase): + def test_missing_file_returns_empty_dict(self): + with tempfile.TemporaryDirectory() as d: + self.assertEqual(m.read_config(Path(d) / "does-not-exist.yaml"), {}) + + def test_reads_existing_mapping(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text("active_provider: anthropic\n", encoding="utf-8") + self.assertEqual(m.read_config(path), {"active_provider": "anthropic"}) + + def test_empty_file_returns_empty_dict(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text("", encoding="utf-8") + self.assertEqual(m.read_config(path), {}) + + +class MergeDeveloperExtensionTests(unittest.TestCase): + def test_adds_developer_extension_when_absent(self): + merged = m.merge_developer_extension({}) + self.assertEqual( + merged["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + + def test_preserves_unrelated_top_level_keys(self): + original = { + "active_provider": "databricks_v2", + "providers": {"databricks_v2": {"model": "goose-claude-4-6-opus"}}, + } + merged = m.merge_developer_extension(original) + self.assertEqual(merged["active_provider"], "databricks_v2") + self.assertEqual( + merged["providers"]["databricks_v2"]["model"], "goose-claude-4-6-opus" + ) + + def test_preserves_other_extensions(self): + original = { + "extensions": {"my-mcp": {"type": "stdio", "enabled": False}}, + } + merged = m.merge_developer_extension(original) + self.assertEqual( + merged["extensions"]["my-mcp"], {"type": "stdio", "enabled": False} + ) + self.assertEqual( + merged["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + + def test_does_not_mutate_the_input(self): + original = {"extensions": {"my-mcp": {"type": "stdio", "enabled": False}}} + m.merge_developer_extension(original) + self.assertNotIn("developer", original["extensions"]) + + def test_is_idempotent_when_developer_already_enabled(self): + once = m.merge_developer_extension({}) + twice = m.merge_developer_extension(once) + self.assertEqual(once, twice) + + def test_overwrites_a_disabled_developer_entry_to_enabled(self): + original = {"extensions": {"developer": {"type": "builtin", "enabled": False}}} + merged = m.merge_developer_extension(original) + self.assertEqual( + merged["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + + +class WriteConfigAtomicTests(unittest.TestCase): + def test_creates_parent_directory_and_file(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "nested" / "config.yaml" + m.write_config_atomic(path, {"a": 1}) + self.assertTrue(path.is_file()) + self.assertEqual(m.read_config(path), {"a": 1}) + + def test_leaves_no_temp_files_behind_on_success(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + m.write_config_atomic(path, {"a": 1}) + self.assertEqual(os.listdir(d), ["config.yaml"]) + + def test_overwrites_existing_file(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text("stale: true\n", encoding="utf-8") + m.write_config_atomic(path, {"fresh": True}) + self.assertEqual(m.read_config(path), {"fresh": True}) + + +class EnableDeveloperExtensionEndToEndTests(unittest.TestCase): + """The plan's own done-when for STEP 4: run twice against a fixture + carrying an unrelated provider block and one other extension -- both + survive byte-for-byte except the added/updated developer entry, and the + second run is a no-op (idempotent, not append-again).""" + + def _fixture_path(self, d: str) -> Path: + path = Path(d) / "config.yaml" + fixture = { + "active_provider": "databricks_v2", + "providers": { + "databricks_v2": { + "model": "goose-claude-4-6-opus", + "host": "https://dbc.example", + } + }, + "extensions": {"my-mcp": {"type": "stdio", "enabled": False}}, + } + m.write_config_atomic(path, fixture) + return path + + def test_creates_file_when_none_exists(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + self.assertFalse(path.exists()) + written = m.enable_developer_extension(path=path) + self.assertEqual(written, path) + cfg = m.read_config(path) + self.assertEqual( + cfg["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + + def test_preserves_unrelated_provider_block_and_other_extension(self): + with tempfile.TemporaryDirectory() as d: + path = self._fixture_path(d) + m.enable_developer_extension(path=path) + cfg = m.read_config(path) + self.assertEqual(cfg["active_provider"], "databricks_v2") + self.assertEqual( + cfg["providers"]["databricks_v2"]["model"], "goose-claude-4-6-opus" + ) + self.assertEqual( + cfg["extensions"]["my-mcp"], {"type": "stdio", "enabled": False} + ) + self.assertEqual( + cfg["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + + def test_second_run_is_a_byte_for_byte_no_op(self): + with tempfile.TemporaryDirectory() as d: + path = self._fixture_path(d) + m.enable_developer_extension(path=path) + after_first = path.read_bytes() + m.enable_developer_extension(path=path) + after_second = path.read_bytes() + self.assertEqual( + after_first, + after_second, + "a second run must be a no-op, not append-again", + ) + + def test_defaults_to_goose_config_path_when_no_path_given(self): + with tempfile.TemporaryDirectory() as d: + fixture_path = Path(d) / "config" / "config.yaml" + fixture_path.parent.mkdir(parents=True) + m.write_config_atomic(fixture_path, {"active_provider": "anthropic"}) + written = m.enable_developer_extension(env={"GOOSE_PATH_ROOT": d}) + self.assertEqual(written, fixture_path) + cfg = m.read_config(fixture_path) + self.assertEqual(cfg["active_provider"], "anthropic") + self.assertEqual( + cfg["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + + +if __name__ == "__main__": + unittest.main() From b921efc96e0bed584154fcb719afa975ffebc51e Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 10:13:24 +1200 Subject: [PATCH 2/6] fix(launchpad): address review-code findings on STEP 4 goose config (#239) review-code on PR #262 found six real defects in the first pass: - Blocker: plain PyYAML load+dump strips comments and re-quotes scalars on every write. Switched to ruamel.yaml's round-trip mode (preserve_quotes), confirmed a hand-written comment and inline comment both survive a merge. - High: GOOSE_PATH_ROOT="" was treated as unset, diverging from goose.rs's actual std::env::var() behavior (Ok("") for a set-but-empty var). Now mirrors Rust exactly, even though that edge case is arguably a footgun -- diverging would mean this script patches a different file than the one goose itself reads. - Medium: writing through a symlinked config.yaml replaced the symlink itself instead of writing through it. write_config_atomic now resolves a symlink target first and renames onto the real file. - Medium: tempfile.mkstemp always created the replacement at mode 0600, silently narrowing an existing file's permissions. Now preserves the original file's mode when overwriting. - Medium: malformed YAML or a non-mapping top-level/extensions value crashed with a raw traceback. Both now raise GooseConfigError with a clear message, matching project-pack.py's fail-loudly convention. - Medium (documented, not fixed): no file locking across the read-modify-write window. Noted as a known limitation in the module docstring -- the plan's own atomicity requirement is about surviving a crash mid-write, not concurrent writers, and this module does not attempt the latter. Adds 6 new tests covering each fix. Requires ruamel.yaml (python3-ruamel.yaml apt package, or pip install ruamel.yaml) -- noted in the module docstring. Signed-off-by: Serina Mcfall --- launchpad/agents/goose_config.py | 124 +++++++++++++++++++++----- launchpad/agents/test_goose_config.py | 65 +++++++++++++- 2 files changed, 164 insertions(+), 25 deletions(-) diff --git a/launchpad/agents/goose_config.py b/launchpad/agents/goose_config.py index 5bc9150989a..49a2fa41b35 100644 --- a/launchpad/agents/goose_config.py +++ b/launchpad/agents/goose_config.py @@ -13,6 +13,21 @@ `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. Requires the +`python3-ruamel.yaml` apt package (or `pip install ruamel.yaml`) -- +PERSONA_PACK_SPEC.md's tooling notes should mention this once STEP 5 wires +this module into the projector CLI. + +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. @@ -24,42 +39,88 @@ import argparse import os +import stat import sys import tempfile from pathlib import Path -import yaml +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()`: a set, non-empty - GOOSE_PATH_ROOT wins, else `~/.config/goose/config.yaml`.""" + """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 - root = env.get("GOOSE_PATH_ROOT") - if root: - return Path(root) / "config" / "config.yaml" + 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) -> dict: +def read_config(path: Path) -> CommentedMap: """The parsed mapping at `path`, or an empty mapping if the file does - not exist or is empty.""" + 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 {} - with path.open("r", encoding="utf-8") as f: - loaded = yaml.safe_load(f) - return loaded or {} + 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. + 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.""" - merged = dict(config) - extensions = dict(merged.get("extensions") or {}) + 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 @@ -68,15 +129,31 @@ def merge_developer_extension(config: dict) -> dict: 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.""" + 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).""" 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 = real_path.stat().st_mode if real_path.exists() else None + fd, tmp_name = tempfile.mkstemp( - dir=str(path.parent), prefix=f".{path.name}.", suffix=".tmp" + 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)) with os.fdopen(fd, "w", encoding="utf-8") as f: - yaml.safe_dump(config, f, default_flow_style=False, sort_keys=False) - os.replace(tmp_name, path) + _yaml().dump(config, f) + os.replace(tmp_name, real_path) except Exception: try: os.unlink(tmp_name) @@ -119,7 +196,12 @@ def main(argv: list[str] | None = None) -> int: if not args.enable_developer: parser.error("nothing to do -- pass --enable-developer") - target = enable_developer_extension(path=args.path) + 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 diff --git a/launchpad/agents/test_goose_config.py b/launchpad/agents/test_goose_config.py index 922bfd0fa3b..38b05a678fa 100644 --- a/launchpad/agents/test_goose_config.py +++ b/launchpad/agents/test_goose_config.py @@ -17,6 +17,7 @@ import importlib.util import os +import stat import tempfile import unittest from pathlib import Path @@ -37,11 +38,14 @@ def test_defaults_to_home_config_goose_when_unset(self): path = m.goose_config_path({}) self.assertEqual(path, Path.home() / ".config" / "goose" / "config.yaml") - def test_empty_goose_path_root_is_treated_as_unset(self): - # An operator with GOOSE_PATH_ROOT="" in their env should not get a - # path rooted at "/config/config.yaml". + def test_empty_goose_path_root_mirrors_rust_treating_it_as_set(self): + # Rust's std::env::var("GOOSE_PATH_ROOT") returns Ok("") for a + # set-but-empty variable, so goose.rs's own resolution does NOT + # treat empty as unset. This module mirrors that exactly, even + # though it is arguably a footgun -- diverging would mean this + # script patches a different file than the one goose itself reads. path = m.goose_config_path({"GOOSE_PATH_ROOT": ""}) - self.assertEqual(path, Path.home() / ".config" / "goose" / "config.yaml") + self.assertEqual(path, Path("config/config.yaml")) class ReadConfigTests(unittest.TestCase): @@ -61,6 +65,34 @@ def test_empty_file_returns_empty_dict(self): path.write_text("", encoding="utf-8") self.assertEqual(m.read_config(path), {}) + def test_invalid_yaml_raises_goose_config_error(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text("{{{{not valid", encoding="utf-8") + with self.assertRaises(m.GooseConfigError): + m.read_config(path) + + def test_non_mapping_top_level_raises_goose_config_error(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text("- just\n- a\n- list\n", encoding="utf-8") + with self.assertRaises(m.GooseConfigError): + m.read_config(path) + + def test_preserves_comments_on_round_trip(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text( + "# managed by ansible -- do not edit by hand\n" + 'active_provider: databricks_v2 # production creds\n', + encoding="utf-8", + ) + cfg = m.read_config(path) + m.write_config_atomic(path, cfg) + written = path.read_text(encoding="utf-8") + self.assertIn("# managed by ansible -- do not edit by hand", written) + self.assertIn("# production creds", written) + class MergeDeveloperExtensionTests(unittest.TestCase): def test_adds_developer_extension_when_absent(self): @@ -109,6 +141,10 @@ def test_overwrites_a_disabled_developer_entry_to_enabled(self): merged["extensions"]["developer"], {"type": "builtin", "enabled": True} ) + def test_non_mapping_extensions_key_raises_goose_config_error(self): + with self.assertRaises(m.GooseConfigError): + m.merge_developer_extension({"extensions": ["not", "a", "mapping"]}) + class WriteConfigAtomicTests(unittest.TestCase): def test_creates_parent_directory_and_file(self): @@ -131,6 +167,27 @@ def test_overwrites_existing_file(self): m.write_config_atomic(path, {"fresh": True}) self.assertEqual(m.read_config(path), {"fresh": True}) + def test_preserves_existing_file_permissions(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_text("a: 1\n", encoding="utf-8") + os.chmod(path, 0o644) + m.write_config_atomic(path, {"a": 2}) + self.assertEqual(stat.S_IMODE(path.stat().st_mode), 0o644) + + def test_writes_through_a_symlink_rather_than_replacing_it(self): + with tempfile.TemporaryDirectory() as d: + real_path = Path(d) / "real-config.yaml" + m.write_config_atomic(real_path, {"stale": True}) + link_path = Path(d) / "config.yaml" + link_path.symlink_to(real_path) + + m.write_config_atomic(link_path, {"fresh": True}) + + self.assertTrue(link_path.is_symlink(), "the symlink must survive the write") + self.assertEqual(link_path.resolve(), real_path) + self.assertEqual(m.read_config(real_path), {"fresh": True}) + class EnableDeveloperExtensionEndToEndTests(unittest.TestCase): """The plan's own done-when for STEP 4: run twice against a fixture From a3f6b7879f2f523fd289ad32ea5aa0577555fc92 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 15:21:09 +1200 Subject: [PATCH 3/6] fix(launchpad): address second review-code pass on STEP 4 (#239) A second independent review-code pass on PR #262 found five more findings (1 High, 3 Medium, 1 Low), all confirmed. Fixes: - High: ruamel.yaml was recorded nowhere and installed by nothing, so the module raised ImportError on any machine without it and no CI job could have caught that -- because NO CI job ran launchpad/agents tests at all. The same High was found independently on PR #260 (its 20 tests also never executed). Adds launchpad/agents/requirements.txt (the dependency, with why ruamel and not PyYAML) and .github/workflows/launchpad-agents-tests.yml, which installs it and runs the suite. The workflow fails if it discovers zero test files, since `unittest discover` exits 0 on an empty suite and a vacuous pass is exactly the gap being closed. - Medium: comment preservation -- the guarantee this module exists for -- was only asserted across read_config -> write_config_atomic, which skips merge_developer_extension entirely. Since the merge copies the mapping, a copy that dropped ruamel's comment attachments would have lost every comment on the real path while the test still passed. Verified the real path is in fact correct (comments do survive), so this was a coverage gap rather than a live bug -- but it was proving the wrong thing. - Medium: every fixture was built by calling this module's own writer, so "before" and "after" had both been through the same serializer -- the one shape that cannot detect a serializer mangling human-authored YAML. Adds OPERATOR_AUTHORED_CONFIG as raw text (top-of-file comment, inline comment, comment nested two levels deep, deliberately quoted scalar, inline comment inside `extensions`) and four controls through the real entry point, including one asserting the developer block is the ONLY line added. - Medium: nothing in the code said that enabling goose's `developer` extension grants shell and filesystem access, or that the plan's OPEN item 2 leaves the live/unattended decision explicitly unsettled. Now stated in enable_developer_extension's own docstring, quoting the plan. - Low: --help dumped the whole 40-line docstring. Now first line only, matching project-pack.py's own `__doc__.splitlines()[0]`. Mutation-checked the new coverage: reverting the dumper to PyYAML makes the suite fail (1 failure, 8 errors) rather than pass. 29 tests, all green. Signed-off-by: Serina Mcfall --- .github/workflows/launchpad-agents-tests.yml | 65 +++++++++++ launchpad/agents/goose_config.py | 38 +++++-- launchpad/agents/requirements.txt | 21 ++++ launchpad/agents/test_goose_config.py | 108 +++++++++++++++++++ 4 files changed, 226 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/launchpad-agents-tests.yml create mode 100644 launchpad/agents/requirements.txt diff --git a/.github/workflows/launchpad-agents-tests.yml b/.github/workflows/launchpad-agents-tests.yml new file mode 100644 index 00000000000..42609264c6a --- /dev/null +++ b/.github/workflows/launchpad-agents-tests.yml @@ -0,0 +1,65 @@ +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 pattern matches nothing, rather than reporting success for a + # run that executed zero tests. `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. + - name: Confirm tests were discovered + run: | + count=$(ls launchpad/agents/test_*.py 2>/dev/null | wc -l) + echo "discovered $count test file(s)" + test "$count" -gt 0 || { + echo "::error::no launchpad/agents/test_*.py files found — this job would have passed vacuously." + exit 1 + } + + - name: Run agents unit tests + run: python3 -m unittest discover -s launchpad/agents -p "test_*.py" -v diff --git a/launchpad/agents/goose_config.py b/launchpad/agents/goose_config.py index 49a2fa41b35..869ebb4accf 100644 --- a/launchpad/agents/goose_config.py +++ b/launchpad/agents/goose_config.py @@ -16,10 +16,11 @@ 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. Requires the -`python3-ruamel.yaml` apt package (or `pip install ruamel.yaml`) -- -PERSONA_PACK_SPEC.md's tooling notes should mention this once STEP 5 wires -this module into the projector CLI. +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 @@ -167,7 +168,28 @@ def enable_developer_extension( ) -> Path: """Read-merge-write entry point: enables goose's `developer` extension at `path` (default: `goose_config_path(env)`). Returns the path - written.""" + 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) @@ -176,7 +198,11 @@ def enable_developer_extension( def main(argv: list[str] | None = None) -> int: - parser = argparse.ArgumentParser(description=__doc__) + # 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", diff --git a/launchpad/agents/requirements.txt b/launchpad/agents/requirements.txt new file mode 100644 index 00000000000..b8dce192bc2 --- /dev/null +++ b/launchpad/agents/requirements.txt @@ -0,0 +1,21 @@ +# 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. +# +# Floor, not a pin: 0.17.21 is what the python3-ruamel.yaml apt package ships on +# Ubuntu 24.04, so pinning above it would make the apt route unusable while +# pinning to it exactly would reject the newer wheel pip installs. +ruamel.yaml>=0.17.21 diff --git a/launchpad/agents/test_goose_config.py b/launchpad/agents/test_goose_config.py index 38b05a678fa..5b50fdc1de1 100644 --- a/launchpad/agents/test_goose_config.py +++ b/launchpad/agents/test_goose_config.py @@ -94,6 +94,114 @@ def test_preserves_comments_on_round_trip(self): self.assertIn("# production creds", written) +# The fixture below is RAW TEXT on purpose, not built by calling this module's +# own writer. An earlier version of these controls built every fixture with +# write_config_atomic(), so both the "before" and the "after" had already been +# through the same serializer -- which is exactly the shape that cannot detect a +# serializer that mangles human-authored YAML. review-code found this twice +# independently. It carries a top-of-file comment, an inline comment on a +# top-level key, a comment nested two levels deep, a deliberately quoted scalar, +# and an inline comment inside `extensions` -- the block this module writes into. +OPERATOR_AUTHORED_CONFIG = """\ +# top-of-file: managed by ansible, do not edit by hand +active_provider: databricks_v2 # inline on a top-level key +providers: + databricks_v2: + # a comment nested two levels deep + model: goose-claude-4-6-opus + host: "https://dbc.example" # quoted on purpose +extensions: + my-mcp: + type: stdio # inline inside the block this module writes into + enabled: false +""" + + +class OperatorAuthoredConfigTests(unittest.TestCase): + """The guarantee this module exists for, exercised through the REAL entry + point (`enable_developer_extension`, merge included) against a file a human + wrote by hand. + + The distinction matters and was a genuine coverage gap: comment + preservation was previously only asserted across read_config -> + write_config_atomic, which skips merge_developer_extension entirely. Since + the merge copies the mapping (`config.copy()` / `CommentedMap(...)`), a copy + that dropped ruamel's comment attachments would have lost every comment on + the real path while the old test still passed.""" + + def _write_fixture(self, d: str) -> Path: + path = Path(d) / "config.yaml" + path.write_text(OPERATOR_AUTHORED_CONFIG, encoding="utf-8") + return path + + def test_merge_preserves_every_comment_and_quoting_style(self): + with tempfile.TemporaryDirectory() as d: + path = self._write_fixture(d) + m.enable_developer_extension(path=path) + written = path.read_text(encoding="utf-8") + + for probe in ( + "# top-of-file: managed by ansible, do not edit by hand", + "# inline on a top-level key", + "# a comment nested two levels deep", + "# quoted on purpose", + "# inline inside the block this module writes into", + ): + self.assertIn(probe, written, f"comment lost through merge: {probe}") + + # The quoting style itself, not just the value: PyYAML's dumper + # re-emits this unquoted, which is how the original Blocker was found. + self.assertIn('host: "https://dbc.example"', written) + + def test_merge_adds_developer_without_disturbing_existing_extension(self): + with tempfile.TemporaryDirectory() as d: + path = self._write_fixture(d) + m.enable_developer_extension(path=path) + cfg = m.read_config(path) + self.assertEqual( + cfg["extensions"]["my-mcp"], {"type": "stdio", "enabled": False} + ) + self.assertEqual( + cfg["extensions"]["developer"], {"type": "builtin", "enabled": True} + ) + self.assertEqual(cfg["active_provider"], "databricks_v2") + + def test_second_run_on_operator_authored_file_is_byte_for_byte_no_op(self): + with tempfile.TemporaryDirectory() as d: + path = self._write_fixture(d) + m.enable_developer_extension(path=path) + after_first = path.read_bytes() + m.enable_developer_extension(path=path) + self.assertEqual( + after_first, + path.read_bytes(), + "a second run against a human-authored file must be a no-op", + ) + + def test_only_addition_is_the_developer_block(self): + """Everything the operator wrote survives verbatim: the output is the + input plus the developer entry, with no other line changed.""" + with tempfile.TemporaryDirectory() as d: + path = self._write_fixture(d) + m.enable_developer_extension(path=path) + written = path.read_text(encoding="utf-8") + + original_lines = OPERATOR_AUTHORED_CONFIG.splitlines() + surviving = [ln for ln in written.splitlines() if ln in original_lines] + self.assertEqual( + surviving, + original_lines, + "every original line must survive, in its original order", + ) + + added = [ln for ln in written.splitlines() if ln not in original_lines] + self.assertEqual( + added, + [" developer:", " type: builtin", " enabled: true"], + "the developer block must be the ONLY addition", + ) + + class MergeDeveloperExtensionTests(unittest.TestCase): def test_adds_developer_extension_when_absent(self): merged = m.merge_developer_extension({}) From a96750f2cc03022be2e8d20d9aa51073ffe3843e Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 15:27:02 +1200 Subject: [PATCH 4/6] fix(launchpad): address cross-vendor review of the STEP 4 fix (#239) Codex reviewed the previous commit as the independent cross-vendor pass and returned REQUEST CHANGES on one Medium plus two Lows. All three were right. - Medium: the "Confirm tests were discovered" guard counted FILES, not test cases -- so it still permitted the exact vacuous pass it was added to prevent. Codex's repro: leave test_goose_config.py in place but rename every `test_*` method to `check_*`; the guard reports one test file while `unittest discover` collects zero cases and exits 0. Reproduced it verbatim (old guard: files=1 -> PASS; new guard: cases=0 -> FAIL) and replaced the check with unittest's own loader plus countTestCases(). - Low: test_only_addition_is_the_developer_block claimed the operator's file "survives verbatim" but compared line MEMBERSHIP, which is weaker than the claim in three ways Codex named -- splitlines() hides a missing trailing newline, hides a CRLF/LF conversion, and would tolerate the three added lines being interleaved anywhere among the originals. Now compares the whole file against the fixture plus the appended block, which is genuinely byte-exact and needs no separate ordering argument. - Low: requirements.txt's rationale was factually wrong -- it claimed an exact pin "would reject the newer wheel pip installs", which is not how pip works. Corrected to state the real reason (the apt/pip floor split), and added the <0.20 ceiling Codex asked for: an open upper bound let a future major series in, and a serializer change there could silently alter an operator's hand-written config with no commit here to point at. Codex confirmed the five findings from the previous pass are substantively fixed, and found no Critical or High. 29 tests, all green. Signed-off-by: Serina Mcfall --- .github/workflows/launchpad-agents-tests.yml | 34 ++++++++++++++------ launchpad/agents/requirements.txt | 20 +++++++++--- launchpad/agents/test_goose_config.py | 30 +++++++++-------- 3 files changed, 57 insertions(+), 27 deletions(-) diff --git a/.github/workflows/launchpad-agents-tests.yml b/.github/workflows/launchpad-agents-tests.yml index 42609264c6a..0a97c40b917 100644 --- a/.github/workflows/launchpad-agents-tests.yml +++ b/.github/workflows/launchpad-agents-tests.yml @@ -48,18 +48,34 @@ jobs: - name: Install agent script dependencies run: pip install -r launchpad/agents/requirements.txt - # Fails if the pattern matches nothing, rather than reporting success for a - # run that executed zero tests. `unittest discover` exits 0 on an empty + # 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. - - name: Confirm tests were discovered + # + # 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: | - count=$(ls launchpad/agents/test_*.py 2>/dev/null | wc -l) - echo "discovered $count test file(s)" - test "$count" -gt 0 || { - echo "::error::no launchpad/agents/test_*.py files found — this job would have passed vacuously." - exit 1 - } + 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 diff --git a/launchpad/agents/requirements.txt b/launchpad/agents/requirements.txt index b8dce192bc2..a30a1c44b22 100644 --- a/launchpad/agents/requirements.txt +++ b/launchpad/agents/requirements.txt @@ -15,7 +15,19 @@ # 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. # -# Floor, not a pin: 0.17.21 is what the python3-ruamel.yaml apt package ships on -# Ubuntu 24.04, so pinning above it would make the apt route unusable while -# pinning to it exactly would reject the newer wheel pip installs. -ruamel.yaml>=0.17.21 +# 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: an open upper bound let every future release in, including a +# major-series one. This module's whole purpose is round-tripping an operator's +# hand-written config, so a serializer change in a future ruamel could silently +# alter their file with no commit in this repo to point at. 0.19.x is current +# and is what pip resolves today, so the range covers both routes while keeping +# the next major series out until someone tests it. +ruamel.yaml>=0.17.21,<0.20 diff --git a/launchpad/agents/test_goose_config.py b/launchpad/agents/test_goose_config.py index 5b50fdc1de1..e250df6ae63 100644 --- a/launchpad/agents/test_goose_config.py +++ b/launchpad/agents/test_goose_config.py @@ -179,26 +179,28 @@ def test_second_run_on_operator_authored_file_is_byte_for_byte_no_op(self): ) def test_only_addition_is_the_developer_block(self): - """Everything the operator wrote survives verbatim: the output is the - input plus the developer entry, with no other line changed.""" + """BYTE-EXACT: the output is the input with the developer block appended + and nothing else changed at all. + + An earlier version of this control compared line *membership* + (`[ln for ln in written.splitlines() if ln in original_lines]`), which + cross-vendor review correctly called weaker than its own docstring + claimed: `splitlines()` discards a missing trailing newline, hides a + CRLF/LF conversion, and would tolerate the three new lines being + interleaved anywhere among the originals. Comparing the whole string + closes all three at once and needs no separate ordering argument.""" with tempfile.TemporaryDirectory() as d: path = self._write_fixture(d) m.enable_developer_extension(path=path) - written = path.read_text(encoding="utf-8") - original_lines = OPERATOR_AUTHORED_CONFIG.splitlines() - surviving = [ln for ln in written.splitlines() if ln in original_lines] - self.assertEqual( - surviving, - original_lines, - "every original line must survive, in its original order", + expected = OPERATOR_AUTHORED_CONFIG + ( + " developer:\n" " type: builtin\n" " enabled: true\n" ) - - added = [ln for ln in written.splitlines() if ln not in original_lines] self.assertEqual( - added, - [" developer:", " type: builtin", " enabled: true"], - "the developer block must be the ONLY addition", + path.read_text(encoding="utf-8"), + expected, + "output must be the operator's file byte-for-byte, plus only " + "the appended developer block", ) From beb1b75d4209b1a1520b00af911cdcd12136260d Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 15:34:24 +1200 Subject: [PATCH 5/6] fix(launchpad): preserve CRLF line endings in goose config.yaml (#239) Second cross-vendor pass from Codex found a real behavioural bug, not just a coverage gap: write_config_atomic rewrote a CRLF file to LF. Reproduced independently before fixing -- a 5-CRLF input came back with 0, and the operator's original bytes were not a prefix of the output. This repository supports Windows (there is a Windows Rust CI job), so a Windows operator's hand-maintained config.yaml being silently converted is not hypothetical, and it is the same class of unasked-for edit as dropping their comments, which is the whole reason this module uses a round-trip parser. write_config_atomic now detects the target file's own convention (any CRLF present => CRLF) and passes it to the writer via `newline=`, since the YAML dumper always emits "\n" and Python translates on the way out. Mixed endings normalise to CRLF rather than being preserved per-line; stated in the docstring rather than left to be discovered. Two test changes, both from the same review: - test_only_addition_is_the_developer_block now compares read_bytes() against an encoded expected value. It used read_text(), which normalises newlines -- so it passed while the implementation was actively rewriting CRLF to LF. That is precisely why the bug survived the previous pass. - Adds test_preserves_crlf_line_endings (asserts no bare LF survives, the original bytes are an exact prefix, and the full expected byte string) and test_lf_file_stays_lf, so preserving CRLF cannot regress into emitting CRLF into a file that never had it. Also corrects requirements.txt's ceiling rationale: <0.20 is a conservative "untested release series" boundary, not a major-version boundary -- ruamel's own docs put its major transition at 1.0. The specifier is unchanged; the reasoning attached to it was wrong. 31 tests, all green. Signed-off-by: Serina Mcfall --- launchpad/agents/goose_config.py | 21 ++++++++++-- launchpad/agents/requirements.txt | 16 +++++---- launchpad/agents/test_goose_config.py | 49 +++++++++++++++++++++++++-- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/launchpad/agents/goose_config.py b/launchpad/agents/goose_config.py index 869ebb4accf..27d6b69d6b1 100644 --- a/launchpad/agents/goose_config.py +++ b/launchpad/agents/goose_config.py @@ -139,12 +139,25 @@ def write_config_atomic(path: Path, config: dict) -> None: 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).""" + silently narrow an existing 0644 file's permissions on every write) + and its 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.""" 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 = real_path.stat().st_mode if real_path.exists() else None + 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" @@ -152,7 +165,9 @@ def write_config_atomic(path: Path, config: dict) -> None: try: if existing_mode is not None: os.chmod(fd, stat.S_IMODE(existing_mode)) - with os.fdopen(fd, "w", encoding="utf-8") as f: + # 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: diff --git a/launchpad/agents/requirements.txt b/launchpad/agents/requirements.txt index a30a1c44b22..14d412cacae 100644 --- a/launchpad/agents/requirements.txt +++ b/launchpad/agents/requirements.txt @@ -24,10 +24,14 @@ # 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: an open upper bound let every future release in, including a -# major-series one. This module's whole purpose is round-tripping an operator's -# hand-written config, so a serializer change in a future ruamel could silently -# alter their file with no commit in this repo to point at. 0.19.x is current -# and is what pip resolves today, so the range covers both routes while keeping -# the next major series out until someone tests it. +# 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 diff --git a/launchpad/agents/test_goose_config.py b/launchpad/agents/test_goose_config.py index e250df6ae63..278ee04f9af 100644 --- a/launchpad/agents/test_goose_config.py +++ b/launchpad/agents/test_goose_config.py @@ -196,13 +196,58 @@ def test_only_addition_is_the_developer_block(self): expected = OPERATOR_AUTHORED_CONFIG + ( " developer:\n" " type: builtin\n" " enabled: true\n" ) + # read_BYTES, not read_text: `read_text` normalises newlines, so a + # CRLF/LF rewrite is invisible to it. Cross-vendor review caught the + # text-mode version of this assertion passing while the + # implementation was in fact rewriting CRLF to LF. self.assertEqual( - path.read_text(encoding="utf-8"), - expected, + path.read_bytes(), + expected.encode("utf-8"), "output must be the operator's file byte-for-byte, plus only " "the appended developer block", ) + def test_preserves_crlf_line_endings(self): + """A Windows operator's CRLF file stays CRLF. + + Measured before this was fixed: a 5-CRLF input came back with 0, and + the original bytes were not a prefix of the output — the module was + silently converting the whole file to LF, which is the same class of + unasked-for edit as dropping their comments.""" + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + original = ( + b"# managed by hand\r\n" + b"active_provider: anthropic\r\n" + b"extensions:\r\n" + b" my-mcp:\r\n" + b" type: stdio\r\n" + ) + path.write_bytes(original) + + m.enable_developer_extension(path=path) + after = path.read_bytes() + + self.assertNotIn(b"\n", after.replace(b"\r\n", b""), + "no bare LF may survive in a CRLF file") + self.assertTrue( + after.startswith(original), + "the operator's original bytes must be an exact prefix of the result", + ) + self.assertEqual( + after, + original + b" developer:\r\n type: builtin\r\n enabled: true\r\n", + ) + + def test_lf_file_stays_lf(self): + """The mirror of the CRLF case: preserving CRLF must not mean emitting + CRLF into a file that never had it.""" + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "config.yaml" + path.write_bytes(b"active_provider: anthropic\n") + m.enable_developer_extension(path=path) + self.assertNotIn(b"\r", path.read_bytes()) + class MergeDeveloperExtensionTests(unittest.TestCase): def test_adds_developer_extension_when_absent(self): From f9676581131e78255cbc4e6311583538bf1a1aeb Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 15:37:03 +1200 Subject: [PATCH 6/6] docs(launchpad): narrow the line-ending claim to LF-or-CRLF (#239) Codex's approving pass noted, non-blocking, that "keeps its line-ending convention" overclaims: a lone-CR (pre-OS X Mac) file normalises to LF rather than being preserved. Taken because an overclaiming docstring is exactly the defect class this PR's whole review chain has been about -- the claim is now "LF-or-CRLF", with the lone-CR case named as a limitation of the function rather than left for a reader to discover. No behaviour change; docstring only. 31 tests, all green. Signed-off-by: Serina Mcfall --- launchpad/agents/goose_config.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/launchpad/agents/goose_config.py b/launchpad/agents/goose_config.py index 27d6b69d6b1..a24f440ad95 100644 --- a/launchpad/agents/goose_config.py +++ b/launchpad/agents/goose_config.py @@ -140,12 +140,14 @@ def write_config_atomic(path: Path, config: dict) -> None: 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 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 + 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.""" + 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)