feat(launchpad): STEP 4 -- goose config.yaml read-merge-write (#239 STEP 4) - #262
Conversation
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 <serina.mcfall@gmail.com>
…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 <serina.mcfall@gmail.com>
|
review-code ran on the first push and found 6 real defects (1 Blocker, 1 High, 4 Medium). All addressed in b921efc:
6 new tests added (25 total, all passing) covering each fix, including a manual CLI sanity check confirming comments survive a real Still draft — has not gone through |
Review pipeline — PR #262Stages run: Not applicable, declared rather than faked:
I could not run this PR's tests. Findings1. High —
|
|
Filed the CI gap from my review above as #270 ( Scoped to #270 also carries the |
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 <serina.mcfall@gmail.com>
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 <serina.mcfall@gmail.com>
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 <serina.mcfall@gmail.com>
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 <serina.mcfall@gmail.com>
|
Second The five review findings
Root cause of the High, worth stating plainly: no CI job ran anything under What cross-vendor review then caught that I had missedPass 1 → REQUEST CHANGES. My "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 Pass 2 → REQUEST CHANGES, and this one was a real bug, not a coverage gap. Pass 3 → APPROVE. Codex executed the suite itself (31 tests, OK) and probed mixed line endings, lone-CR, symlink + permission interaction, empty files, and CRLF idempotency across two runs. No blockers, no regressions. Also corrected two rationales that were simply wrong: Verification: 31 tests, all green. Mutation-checked — reverting the dumper to PyYAML makes the suite fail (1 failure, 8 errors) rather than pass. Still draft: no |
ciaran-slow
left a comment
ciaran-slow
left a comment
There was a problem hiding this comment.
Comment review recording the pipeline result. Not an approval and not a change-request — the merge decision is unchanged by this.
Reviewed via the full pipeline — detail in my comment on this PR.
The careful parts are genuinely careful. goose.rs really is read-only for config.yaml and nothing else in the repo writes it — both premises check out. The GOOSE_PATH_ROOT mirroring is the detail most people get wrong: std::env::var returns Ok("") for a set-but-empty variable, so Rust does not treat empty as unset, and "GOOSE_PATH_ROOT" in env mirrors that exactly rather than doing the intuitive-but-wrong truthiness check. The atomic write, symlink handling and permission-mode preservation each solve a real failure rather than a decorative one.
One High finding — must be fixed before this merges.
goose_config.py:47 — ruamel.yaml is a new dependency that nothing records and nothing installs. No manifest under launchpad/ declares it; no workflow installs it; the docstring names it in prose and defers recording it to STEP 5. python3 -c "import ruamel.yaml" → ModuleNotFoundError on a clean checkout, so all 24 tests die at import before any assertion — I could not run this PR's suite at all. And STEP 5's spec is "one deterministic run… no other human step" against a freshly-cloned repo, which an unrecorded pip install defeats. The fix pattern is already on disk: launchpad-review-agent-controls.yml:41-42 does pip install pyyaml with a comment giving exactly this reason. Please add launchpad/agents/requirements.txt and, ideally, guard the import so a missing dependency raises GooseConfigError naming the pip command rather than a traceback. (#270 covers the CI side.)
Three Medium items, non-blocking:
test_goose_config.py:82— the module's headline claim is that comments survive a merge, buttest_preserves_comments_on_round_tripnever callsmerge_developer_extension. WhetherCommentedMap.copy()carries comment metadata is the thing that decides the guarantee, and no test asks. I could not settle it either, because of the High above. One three-line test pins it.test_goose_config.py:198—_fixture_pathbuilds its fixture with the module's ownwrite_config_atomic, so byte-for-byte idempotency is only proven against already-normalised input. An operator's hand-written config (enabled: no, four-space indent, quoted hosts) would be reformatted by the first run and the test would not notice.goose_config.py:124— this line grants goose's shell/write capability, and the 35-line docstring never says so. The plan's OPEN item 2 reserves who arbitrates that for unattended use. STEP 5 wraps this in "one run, no other human step", so a sentence here is what survives into STEP 5 when the plan file does not.
ciaran-slow
left a comment
ciaran-slow
left a comment
There was a problem hiding this comment.
Change-request review. The finding(s) below are the author's to resolve before this merges — full detail, probes and line citations are in my pipeline comment on this PR.
Reviewed via the full pipeline — detail in my comment on this PR.
The careful parts are genuinely careful. goose.rs really is read-only for config.yaml and nothing else in the repo writes it — both premises check out. The GOOSE_PATH_ROOT mirroring is the detail most people get wrong: std::env::var returns Ok("") for a set-but-empty variable, so Rust does not treat empty as unset, and "GOOSE_PATH_ROOT" in env mirrors that exactly rather than doing the intuitive-but-wrong truthiness check. The atomic write, symlink handling and permission-mode preservation each solve a real failure rather than a decorative one.
One High finding — must be fixed before this merges.
goose_config.py:47 — ruamel.yaml is a new dependency that nothing records and nothing installs. No manifest under launchpad/ declares it; no workflow installs it; the docstring names it in prose and defers recording it to STEP 5. python3 -c "import ruamel.yaml" → ModuleNotFoundError on a clean checkout, so all 24 tests die at import before any assertion — I could not run this PR's suite at all. And STEP 5's spec is "one deterministic run… no other human step" against a freshly-cloned repo, which an unrecorded pip install defeats. The fix pattern is already on disk: launchpad-review-agent-controls.yml:41-42 does pip install pyyaml with a comment giving exactly this reason. Please add launchpad/agents/requirements.txt and, ideally, guard the import so a missing dependency raises GooseConfigError naming the pip command rather than a traceback. (#270 covers the CI side.)
Three Medium items, non-blocking:
test_goose_config.py:82— the module's headline claim is that comments survive a merge, buttest_preserves_comments_on_round_tripnever callsmerge_developer_extension. WhetherCommentedMap.copy()carries comment metadata is the thing that decides the guarantee, and no test asks. I could not settle it either, because of the High above. One three-line test pins it.test_goose_config.py:198—_fixture_pathbuilds its fixture with the module's ownwrite_config_atomic, so byte-for-byte idempotency is only proven against already-normalised input. An operator's hand-written config (enabled: no, four-space indent, quoted hosts) would be reformatted by the first run and the test would not notice.goose_config.py:124— this line grants goose's shell/write capability, and the 35-line docstring never says so. The plan's OPEN item 2 reserves who arbitrates that for unattended use. STEP 5 wraps this in "one run, no other human step", so a sentence here is what survives into STEP 5 when the plan file does not.
Independent adjudication — all four findings are already fixedAdjudicated in a fresh context, separate from the pipeline that produced the findings. The change-request review is stale, not wrong. All four findings were real and accurately cited at the head they were written against; all four were fixed 36 minutes before the review restating them was submitted.
Each citation was verified at Findings1. Real at review time: line 47 was All three legs now closed: Suite: Severity, had it survived: Blocker, up one rung from the reviewer's High. A 24-test suite that dies at import is not a partial check, it is zero checks wearing the costume of 24 — wrong now, and against STEP 5's stated "one deterministic run, no other human step". The review body said "must be fixed before this merges" while the ladder said High; the ladder should have carried that. Honest gap: the original 2. Real at review time. Now closed by a new The question the review said it could not settle is now settled. Probed directly: 3.
Residual, found by feeding the reviewer's own adversarial input — a genuinely new observation, Low. Two of the three traits are handled and one is not: the comment survives, the single quotes survive, Deliberately not promoted into finding 3 — the cited gap is closed and the requested assertion exists. Same class as the CRLF issue the cross-vendor pass caught, so it likely deserves its own issue rather than being smuggled in under a resolved finding's number. 4. Real at review time, though slightly overstated: the docstring did already mention write/shell capability in passing, so "says nothing about" was not strictly accurate — but the pending-arbitration half was genuinely absent, and that was the substance. The whole docstring was read before ruling. The reviewer's fifth item (Low — Verdict
Handed 4. 0 confirmed, 0 refuted, 4 resolved. Nothing survives to block the merge. Total non-survival usually means a broken adjudicator rather than a flawless PR, so to be explicit about why this case differs: the suite was executed (31 and 51 tests, green), the new CI workflow was confirmed green on the exact head SHA, the one runtime question the review declared unsettled was answered, and one residual gap was found by probing the reviewer's own adversarial input. This is resolution verified, not accepted. Requesting re-review against current head — @ciaran-slow, the finding set was correct when written at 01:38 and re-posted unchanged at 03:52 and 03:57 without re-reading the branch. Cross-stack note: this PR is on the #239 stack and shares no root cause with the #118 chain (#261/#263/#264/#266/#267), as expected. The one genuine cross-cutting item — no CI job running 🤖 Adjudicated by Claude Code ( |
benmitchell11
left a comment
benmitchell11
left a comment
There was a problem hiding this comment.
Independent pass, done after reading @ciaran-slow's existing COMMENTED/CHANGES_REQUESTED review and checking it against current HEAD (eaeec02b1) rather than trusting either the review or the PR body at face value.
All previously-flagged issues appear already resolved at HEAD. Concretely verified, not just read:
- Missing dependency manifest (ciaran-slow's High).
launchpad/agents/requirements.txtnow pinsruamel.yaml>=0.17.21,<0.20with a well-argued floor/ceiling rationale, and.github/workflows/launchpad-agents-tests.ymlinstalls it before running tests, gated onpull_request/pushpaths touchinglaunchpad/agents/**. I installed fresh from that requirements file (ruamel.yamlwas not present in my environment beforehand) and confirmed the suite actually imports and runs — this is no longer a "trust the docstring" claim. - Comment-preservation-through-merge gap (Medium #1).
test_goose_config.py's newOperatorAuthoredConfigTests.test_merge_preserves_every_comment_and_quoting_stylenow drives the real end-to-end path (enable_developer_extension, merge included) against a hand-authored fixture with a top-of-file comment, an inline comment, a nested comment, and a quoted scalar, and asserts all five survive. This is the gap the earliertest_preserves_comments_on_round_triptest left open (it never calledmerge_developer_extension). - Idempotency fixture built by the module's own writer (Medium #2).
OPERATOR_AUTHORED_CONFIGis now a raw string literal (not built viawrite_config_atomic), andtest_second_run_on_operator_authored_file_is_byte_for_byte_no_op/test_only_addition_is_the_developer_blockexercise it.test_only_addition_is_the_developer_blockin particular does a byte-exact comparison against "input + exactly the appended developer block," which is a strong guarantee against silent reformatting of an operator's file. - Undisclosed capability grant (Medium #3).
enable_developer_extension's docstring now has an explicit "WHAT THIS GRANTS, STATED WHERE IT HAPPENS" section naming the shell/write capability and citing the plan's OPEN item 2.
Ran the suite myself (pip install -r launchpad/agents/requirements.txt, then python3 -m unittest discover -s launchpad/agents -p "test_goose_config.py" -v, on Windows): 28/31 pass. The 3 failures are all platform artifacts, not logic bugs, and match exactly what the PR's own "Not verified" section discloses ("behavior on Windows path semantics... this sandbox is Linux-only"):
test_writes_through_a_symlink_rather_than_replacing_it—os.symlinkneeds an elevated privilege Windows doesn't grant by default (WinError 1314).test_preserves_existing_file_permissions— Windows has no POSIX permission bits;chmod(0o644)doesn't round-trip.test_only_addition_is_the_developer_block— the test's ownpath.write_text(OPERATOR_AUTHORED_CONFIG)fixture setup silently gets LF→CRLF-translated by Python's default text-mode write on Windows, before the module under test ever runs. Since CI runs onubuntu-latest, none of these should surface there — worth a one-line note in the test module if Windows contributors are expected, but not a blocker.
Checked the failure-mode classes this category of change typically has, per this repo's own review conventions:
- Preserves structure it doesn't own: yes, verified above through the real merge path, not just read→write.
- Partial write on failure:
write_config_atomicwrites to atempfile.mkstempsibling,os.replaces it in, and theexceptclause unlinks the temp file and re-raises — no window where a crash leaves a half-written file at the real path. - "Merge" that's actually clobber: no — confirmed via
test_preserves_unrelated_provider_block_and_other_extension,test_merge_adds_developer_without_disturbing_existing_extension, and the byte-exact test above.
One minor nit, not blocking: the PR body's "Verification" section pastes a run showing "Ran 25 tests," but HEAD's suite now has 31 (OperatorAuthoredConfigTests and the CRLF tests were added in later fixup commits after that body text was apparently written). Worth refreshing the pasted output so it matches what's actually on the branch.
No new blocking findings from me. Given the prior CHANGES_REQUESTED review's concerns look genuinely addressed at this commit, this looks close to mergeable pending the reviewer re-confirming their own findings against current HEAD (I'm not the one who can clear that review).
Requested changes are done — clear for approval@ciaran-slow's change-request is satisfied, and I am independent of this one: the fixes were written by a different session, and the verification was done by a separate adjudication pass, not by whoever wrote them. All four findings were real and accurately cited at the head they were written against ( Verified at head
Suite: 31 tests in One residual found while probing rather than reading, filed separately and not blocking: four-space indentation is silently renormalised to two on first write, invisible to the committed fixture because it uses two-space indent. Ready for a human approval. I am an agent and do not approve — the command is below for whoever picks it up, and it deliberately is not run here. # ---- NOT RUN BY ME. For a human to run, when they choose: ----
gh pr review 262 --approve |
4 participants
Summary
STEP 4 of issue #239's plan: read-merge-write logic for goose's
config.yaml, built from scratch since the existing Rust code only reads it.review-coderan per the plan's own gate for this step and found 6 real defects (1 Blocker); all fixed in a follow-up commit before this body was written.Related issue
Refs #239
Issue type
Task
Agent provenance
Objective
Add
launchpad/agents/goose_config.py, giving the projector a way to enable goose'sdeveloper(shell/write) extension without hand-editing an operator'sconfig.yaml.Impacted components
launchpad/agents/goose_config.py
launchpad/agents/test_goose_config.py
Approach and rejected alternatives
Used
ruamel.yaml's round-trip mode instead of plain PyYAML (the convention used elsewhere in this repo, e.g.launchpad/the-professor/tools/server.py) specifically becausereview-codedemonstrated plain load+dump silently strips an operator's comments and re-quotes scalars on every write — confirmed by writing a fixture with a# managed by ansiblecomment and a quoted host value, both of which vanished after one round trip throughyaml.safe_load/yaml.safe_dump. Rejected keeping plain PyYAML and documenting the loss as a known limitation instead of fixing it — Serina's call, since it adds a new dependency (python3-ruamel.yamlapt package, orpip install ruamel.yaml) not used anywhere else in the repo yet; she chose to add it.Also rejected implementing file locking across the read-modify-write window (a
review-codeMedium finding: a goose process rewriting its own config at the same moment this script runs could lose one side's update). Documented as a known limitation in the module docstring instead — the plan's own atomicity requirement is about surviving a crash mid-write (solved: temp file + rename), not about concurrent writers, and this module does not attempt the latter.Verification
Command run:
Raw output:
Also manually ran the real CLI entry point (
python3 launchpad/agents/goose_config.py --enable-developeragainst a hand-written fixture with a# hand-edited, do not clobbercomment and an inline# my favouritecomment) and confirmed both comments and the existingmy-mcpextension survive, withdevelopercleanly appended.Not verified
Not run against a real, running
gooseprocess — only against static fixture files. The plan's own STEP 7 (live end-to-end proof) is what actually exercises this against a live goose+buzz-acp session; this PR only proves the file-merge logic in isolation. Also not verified: behavior on Windows path semantics (this sandbox is Linux-only), and the documented file-locking gap (concurrent writers) was reasoned about, not exercised under real concurrency.Security implications
This is the first code in the repo that writes to a file outside the repo/build tree (an operator's
~/.config/goose/config.yamlor$GOOSE_PATH_ROOTequivalent).review-codespecifically flagged this class of change (citing #9's PR #238, where review-code found a real security bug in far less risky read-only code) and found: a symlinked config would have been silently replaced instead of written through (fixed — now resolves and writes through the real target); file permissions would have been silently reset to0600on every write (fixed — now preserves the original mode); malformed YAML would have crashed with a raw traceback instead of a clear error (fixed — raisesGooseConfigError). Remaining, documented rather than fixed: no locking across the read-modify-write window (see Approach above).Escalations
Two decisions raised to Serina rather than made unilaterally: (1) whether to add
ruamel.yamlas a new dependency to fix the comment-stripping Blocker, vs. documenting it as a known limitation — she chose to add it, and ran thesudo apt-get install python3-ruamel.yamlherself since it needs a real terminal for the password prompt. (2) implicitly, whether the no-locking gap needed fixing now — treated as out of the plan's stated scope (crash-mid-write atomicity, not concurrent-writer safety) and documented rather than escalated as a blocking question, since the plan itself doesn't ask for it.