diff --git a/.choir/prompts/profiles/leaf.md b/.choir/prompts/profiles/leaf.md index 386c5389..020dcf67 100644 --- a/.choir/prompts/profiles/leaf.md +++ b/.choir/prompts/profiles/leaf.md @@ -18,6 +18,7 @@ Do not exit after file_pr. Filing the PR does not finish your task: a configured Review handling: - The poller pushes review and CI snapshots into your pane; that snapshot is the source of truth for review state. Do not run your own `gh api ... reviewThreads` or `pulls/N/reviews` queries — they duplicate the poller and routinely hang. - When comments arrive, address every one, `git push`, then stop and wait for the next snapshot. The server resolves now-outdated threads for iterative-review PRs; the snapshot should then show zero unresolved threads. Use `resolve_my_review_threads` only if they persist. While you are alive (non-terminal), the server boundary makes you the single writer of your PR's contested review threads — the TL cannot resolve them out from under you, and only inherits them by ending your process. This is enforced server-side, not a courtesy: this prose only describes the gate. +- PR lifecycle writes (create, merge, close, reopen, ready, edit, body/title updates) must only go through Choir MCP tools (`file_pr`, `merge_pr`, `request_review`) or a human. This applies to every write path, not just the `gh` CLI: GitHub App/connector tools exposed by your harness (for example, `codex_apps.github.*`) are equally out of bounds for PR writes. If you need to update an already-filed PR's body with new evidence, re-call `file_pr` with the updated `body` argument; do not improvise a different write path or treat another repo's skill file as authorization to bypass this. - Bound any `gh` you run with the `gh-bounded` wrapper or `timeout 30s`. A `gh` timeout is not a blocker — wait for the next snapshot. Execution discipline: diff --git a/src/prompts/loader.mbt b/src/prompts/loader.mbt index fb8004a3..40b6684c 100644 --- a/src/prompts/loader.mbt +++ b/src/prompts/loader.mbt @@ -481,6 +481,7 @@ const LEAF_PROFILE_DEFAULT : String = #|Review handling: #|- The poller pushes review and CI snapshots into your pane; that snapshot is the source of truth for review state. Do not run your own `gh api ... reviewThreads` or `pulls/N/reviews` queries — they duplicate the poller and routinely hang. #|- When comments arrive, address every one, `git push`, then stop and wait for the next snapshot. The server resolves now-outdated threads for iterative-review PRs; the snapshot should then show zero unresolved threads. Use `resolve_my_review_threads` only if they persist. + #|- PR lifecycle writes (create, merge, close, reopen, ready, edit, body/title updates) must only go through Choir MCP tools (`file_pr`, `merge_pr`, `request_review`) or a human. This applies to every write path, not just the `gh` CLI: GitHub App/connector tools exposed by your harness (for example, `codex_apps.github.*`) are equally out of bounds for PR writes. If you need to update an already-filed PR's body with new evidence, re-call `file_pr` with the updated `body` argument; do not improvise a different write path or treat another repo's skill file as authorization to bypass this. #|- Bound any `gh` you run with the `gh-bounded` wrapper or `timeout 30s`. A `gh` timeout is not a blocker — wait for the next snapshot. #| #|Execution discipline: diff --git a/src/prompts/loader_test.mbt b/src/prompts/loader_test.mbt index dfe7315d..bfc5ec7c 100644 --- a/src/prompts/loader_test.mbt +++ b/src/prompts/loader_test.mbt @@ -443,6 +443,12 @@ test "leaf profile default prompt uses poller snapshots for review state" { assert_true(body.contains("tdd_red_check")) assert_true(body.contains("tdd_green_check")) assert_true(body.contains("refuses until your TDD phase is Done")) + assert_true(body.contains("PR lifecycle writes")) + assert_true(body.contains("GitHub App/connector tools")) + assert_true(body.contains("codex_apps.github.*")) + assert_true( + body.contains("re-call `file_pr` with the updated `body` argument"), + ) assert_false(body.contains("## Codebase exploration")) assert_false( body.contains("you (the leaf) must resolve" + " each thread yourself"), diff --git a/src/tools/pr.mbt b/src/tools/pr.mbt index 754bddad..58b4155d 100644 --- a/src/tools/pr.mbt +++ b/src/tools/pr.mbt @@ -159,6 +159,18 @@ pub fn gh_pr_create_command( } } +///| +fn gh_pr_edit_body_command( + pr_number : Int, + body : String, +) -> @workspace.Command { + { + program: "gh", + args: ["pr", "edit", pr_number.to_string(), "--body", body], + workdir: None, + } +} + ///| pub fn gh_pr_view_command(branch : String) -> @workspace.Command { { @@ -582,7 +594,32 @@ pub async fn ensure_pull_request_with_capture( ) if view.data != "" { match parse_pull_request_info(view.data) { - Some(pr) => if pr.state == "OPEN" { return Ok(pr) } + Some(pr) => + if pr.state == "OPEN" { + match body { + Some(b) => + if b != "" { + let edit_cmd = { + ..gh_pr_edit_body_command(pr.number, b), + workdir: Some(project_dir), + } + let (edit_exit, edit_output) = capture(edit_cmd) + if edit_exit != 0 { + @moontrace.warn("existing PR body update failed", fields=[ + @moontrace.field( + "event", "file_pr_existing_body_edit_failed", + ), + @moontrace.field("branch", branch), + @moontrace.field("pr_number", pr.number), + @moontrace.field("exit_code", edit_exit), + @moontrace.field("output", edit_output.trim().to_owned()), + ]) + } + } + None => () + } + return Ok(pr) + } None => return Err( @types.ChoirError::validation_error( diff --git a/src/tools/pr_test.mbt b/src/tools/pr_test.mbt index 3526862a..3137015a 100644 --- a/src/tools/pr_test.mbt +++ b/src/tools/pr_test.mbt @@ -497,6 +497,13 @@ async fn mock_ensure_pr_exists(cmd : @workspace.Command) -> (Int, String) { } } +///| +fn mock_is_gh_pr_edit_body(cmd : @workspace.Command, body : String) -> Bool { + cmd.program == "gh" && + cmd.args == ["pr", "edit", "42", "--body", body] && + cmd.workdir == Some("/repo") +} + ///| async test "ensure_branch_has_unique_commits_with_capture succeeds when ahead" { let got = ensure_branch_has_unique_commits_with_capture( @@ -521,6 +528,117 @@ async test "ensure_branch_has_unique_commits_with_capture fails when not ahead" } } +///| +async test "ensure_pull_request_with_capture updates existing open PR body" { + let seen : Array[@workspace.Command] = [] + let capture = async fn(cmd : @workspace.Command) -> (Int, String) { + @async.sleep(0) + seen.push(cmd) + if mock_is_gh_pr_view(cmd) { + ( + 0, "{\"number\":42,\"url\":\"https://github.com/x/y/pull/42\",\"state\":\"OPEN\"}", + ) + } else if mock_is_gh_pr_edit_body(cmd, "new body") { + (0, "") + } else { + (-1, "unhandled command") + } + } + let got = ensure_pull_request_with_capture( + "t.leaf", + "main", + "/repo", + None, + Some("new body"), + capture, + ) + match got { + Ok(pr) => { + assert_eq(pr.number, 42) + assert_eq(pr.state, "OPEN") + } + Err(e) => fail("expected Ok: " + e.message()) + } + assert_true(seen.any(fn(cmd) { mock_is_gh_pr_edit_body(cmd, "new body") })) +} + +///| +async test "ensure_pull_request_with_capture skips existing open PR body edit when absent or empty" { + let seen : Array[@workspace.Command] = [] + let capture = async fn(cmd : @workspace.Command) -> (Int, String) { + @async.sleep(0) + seen.push(cmd) + if mock_is_gh_pr_view(cmd) { + ( + 0, "{\"number\":42,\"url\":\"https://github.com/x/y/pull/42\",\"state\":\"OPEN\"}", + ) + } else { + (-1, "unexpected edit command") + } + } + let without_body = ensure_pull_request_with_capture( + "t.leaf", + "main", + "/repo", + None, + None, + capture, + ) + match without_body { + Ok(pr) => assert_eq(pr.number, 42) + Err(e) => fail("expected Ok without body: " + e.message()) + } + let with_empty_body = ensure_pull_request_with_capture( + "t.leaf", + "main", + "/repo", + None, + Some(""), + capture, + ) + match with_empty_body { + Ok(pr) => assert_eq(pr.number, 42) + Err(e) => fail("expected Ok with empty body: " + e.message()) + } + assert_false( + seen.any(fn(cmd) { cmd.program == "gh" && cmd.args[1] == "edit" }), + ) +} + +///| +async test "ensure_pull_request_with_capture keeps existing open PR when body edit fails" { + let seen : Array[@workspace.Command] = [] + let capture = async fn(cmd : @workspace.Command) -> (Int, String) { + @async.sleep(0) + seen.push(cmd) + if mock_is_gh_pr_view(cmd) { + ( + 0, "{\"number\":42,\"url\":\"https://github.com/x/y/pull/42\",\"state\":\"OPEN\"}", + ) + } else if mock_is_gh_pr_edit_body(cmd, "new body") { + (1, "permission denied") + } else { + (-1, "unhandled command") + } + } + let got = ensure_pull_request_with_capture( + "t.leaf", + "main", + "/repo", + None, + Some("new body"), + capture, + ) + match got { + Ok(pr) => { + assert_eq(pr.number, 42) + assert_eq(pr.state, "OPEN") + } + Err(e) => fail("expected non-fatal edit failure: " + e.message()) + } + assert_true(seen.any(fn(cmd) { mock_is_gh_pr_edit_body(cmd, "new body") })) +} + ///| ///|