From 021db8349fb78481071a4daf20b52b42e6cedbec Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 22 Aug 2026 05:09:54 -0500 Subject: [PATCH 1/4] fix(cli): refuse an unknown --repo filter on add, commit, and push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unknown --repo name matched zero repositories and the command reported success having done nothing. With a modified file present in the worktree, `gr add . --repo missing` printed "No changes to stage." and exited 0. There was a change to stage; gr said there was not. That is a false claim about the working tree, which is what justifies a fix rather than a documentation note. validate_repo_filters_known already existed in src/core/repo.rs, already produced the right diagnostic, and already carried a basename-matching branch that names the intended repository. It was not called from add, commit, or push. This calls it from all three. Wired to CliOutcomeError::refusal. outcome.rs defines EXIT_REFUSED = 2 and carries a test named refusal_is_distinct_from_operational_failure, so the codebase already separates a refusal from an operational failure. pr/merge uses the same mapping. Three regression tests assert that an unknown --repo name exits 2 with the diagnostic, one per verb. A control asserts that a KNOWN name still reaches the work: it writes a file, runs the command, and asserts that file is present in the repository's index. It asserts the destination rather than the absence of a message. Mutation-verified in both directions. Removing add.rs's validation call turns exactly the add rejection test red while commit and push stay green. Making run_add return early after validation turns exactly the control red while the three rejection tests stay green. Other commands accept a repo filter without validating it. Identifying which requires reading each command's behavior rather than grepping an identifier, and is deliberately left to a follow-on so each batch is reviewed on its own. Ref #886 — closes at promotion. Premium boundary: grip is OSS — local workspace orchestration, no identity, org, or entitlement semantics. --- src/cli/commands/add.rs | 6 ++- src/cli/commands/commit.rs | 6 ++- src/cli/commands/push.rs | 8 ++- tests/cli_tests.rs | 101 +++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/src/cli/commands/add.rs b/src/cli/commands/add.rs index b05be190..d7649dfd 100644 --- a/src/cli/commands/add.rs +++ b/src/cli/commands/add.rs @@ -1,9 +1,10 @@ //! Add command implementation +use crate::cli::outcome::CliOutcomeError; use crate::cli::output::Output; use crate::core::manifest::Manifest; use crate::core::manifest_paths; -use crate::core::repo::{filter_repos, RepoInfo}; +use crate::core::repo::{filter_repos, validate_repo_filters_known, RepoInfo}; use crate::git::cache::invalidate_status_cache; use crate::git::{get_workdir, open_repo, path_exists}; use crate::util::log_cmd; @@ -19,6 +20,9 @@ pub fn run_add( repos_filter: Option<&[String]>, group_filter: Option<&[String]>, ) -> anyhow::Result<()> { + validate_repo_filters_known(manifest, repos_filter) + .map_err(|error| CliOutcomeError::refusal(error.to_string()))?; + Output::header("Checking repositories for changes to stage..."); println!(); diff --git a/src/cli/commands/commit.rs b/src/cli/commands/commit.rs index 20ad923a..88f86b7e 100644 --- a/src/cli/commands/commit.rs +++ b/src/cli/commands/commit.rs @@ -1,9 +1,10 @@ //! Commit command implementation +use crate::cli::outcome::CliOutcomeError; use crate::cli::output::Output; use crate::core::manifest::Manifest; use crate::core::manifest_paths; -use crate::core::repo::{filter_repos, RepoInfo}; +use crate::core::repo::{filter_repos, validate_repo_filters_known, RepoInfo}; use crate::git::cache::invalidate_status_cache; use crate::git::{get_workdir, open_repo, path_exists}; use crate::util::log_cmd; @@ -22,6 +23,9 @@ pub fn run_commit( repos_filter: Option<&[String]>, group_filter: Option<&[String]>, ) -> anyhow::Result<()> { + validate_repo_filters_known(manifest, repos_filter) + .map_err(|error| CliOutcomeError::refusal(error.to_string()))?; + if !json { Output::header("Committing changes..."); println!(); diff --git a/src/cli/commands/push.rs b/src/cli/commands/push.rs index adf0e45f..84ec0ce8 100644 --- a/src/cli/commands/push.rs +++ b/src/cli/commands/push.rs @@ -1,8 +1,11 @@ //! Push command implementation +use crate::cli::outcome::CliOutcomeError; use crate::cli::output::Output; use crate::core::manifest::Manifest; -use crate::core::repo::{filter_repos, get_manifest_repo_info, RepoInfo}; +use crate::core::repo::{ + filter_repos, get_manifest_repo_info, validate_repo_filters_known, RepoInfo, +}; use crate::git::remote::{force_push_branch, push_branch}; use crate::git::{get_current_branch, open_repo, path_exists}; use git2::Repository; @@ -27,6 +30,9 @@ pub fn run_push( repos_filter: Option<&[String]>, group_filter: Option<&[String]>, ) -> anyhow::Result<()> { + validate_repo_filters_known(manifest, repos_filter) + .map_err(|error| CliOutcomeError::refusal(error.to_string()))?; + if !json { if force { Output::header("Force pushing changes..."); diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 5be90a70..a08253d5 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -475,3 +475,104 @@ fn test_checkout_remove_rejects_extra_positional_args() { "unexpected extra arguments after checkout name", )); } + +// --- #196: repo-filter validation on the staging/commit/push verbs ------------- +// +// `validate_repo_filters_known` already existed and already produced the right +// message, including a basename suggestion for the common mistake that surfaced +// this: a manifest entry named `-` checked out at `./`, where +// the operator naturally types `--repo `. These three verbs did not call +// the validator, so an unknown `--repo` name matched zero repos and the command +// reported success. +// +// The failure direction is what makes it worth a test: another agent reads +// "pushed" or "staged" and acts on it. + +#[test] +fn test_add_unknown_repo_filter_is_refused_not_silently_empty() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("add") + .arg(".") + .arg("--repo") + .arg("missing") + .assert() + .code(2) + .stderr(predicate::str::contains( + "repo filter 'missing' not found in local manifest", + )); +} + +#[test] +fn test_commit_unknown_repo_filter_is_refused_not_silently_empty() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("commit") + .arg("-m") + .arg("msg") + .arg("--repo") + .arg("missing") + .assert() + .code(2) + .stderr(predicate::str::contains( + "repo filter 'missing' not found in local manifest", + )); +} + +#[test] +fn test_push_unknown_repo_filter_is_refused_not_silently_empty() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("push") + .arg("--repo") + .arg("missing") + .assert() + .code(2) + .stderr(predicate::str::contains( + "repo filter 'missing' not found in local manifest", + )); +} + +/// Control for the three tests above: a KNOWN repo name must still REACH THE +/// WORK, so the rejections cannot be passing merely because `gr add` fails for +/// some unrelated reason in this fixture. +/// +/// The control asserts the DESTINATION, not the absence of a message. An +/// earlier version of this test wrote no file and checked only that one +/// substring was missing from stderr — which passes identically whether `add` +/// stages the file or does nothing at all, and those are exactly the two +/// outcomes a control has to separate. The fixture commits its files before +/// cloning, so the worktree starts clean and the test must dirty it itself. +#[test] +fn test_add_known_repo_filter_reaches_the_work_and_stages() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + let repo = ws.repo_path("app"); + std::fs::write(repo.join("control.txt"), "dirty\n").unwrap(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("add") + .arg(".") + .arg("--repo") + .arg("app") + .assert() + .success() + .stderr(predicate::str::contains("not found in local manifest").not()); + + let staged = std::process::Command::new("git") + .args(["diff", "--cached", "--name-only"]) + .current_dir(&repo) + .output() + .unwrap(); + let staged = String::from_utf8_lossy(&staged.stdout); + assert!( + staged.contains("control.txt"), + "known --repo name must reach the work: expected control.txt in the index, got {staged:?}" + ); +} From 0b7029aad95c11e420c57e10e6ed3402b0a7b7a9 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 22 Aug 2026 10:18:41 -0500 Subject: [PATCH 2/4] fix(prune): protect the remote default branch, not just current and target `gr prune --execute` deleted the local `main` on any repo whose manifest target is not `main`. The guard skipped exactly two branches -- the one checked out and `repo.target_branch()` -- so once the integration target moved to `dev`, `main` was neither and fell out of protection. Nothing was edited to cause this: `main` had been protected only by coincidence, because target and default used to be the same value. Scope of the defect, measured rather than assumed: LOCAL ONLY. The `--remote` path is `git fetch --prune`, which prunes stale remote-tracking refs and does not delete remote branches, so the worst outcome was a local branch recreated from the remote. The fix protects a SET of three: current, target, and the remote's default branch read from `refs/remotes//HEAD`. The default is the only one of the three that asserts "permanent" rather than "currently interesting", which is the property a cleanup rule needs. Resolution is local -- a cleanup verb should not acquire a network failure mode. When the default cannot be resolved, the protected set GROWS rather than shrinks: `main` and `dev` are protected by name and the run says so. A resolution failure that silently dropped a branch would reproduce this exact defect inside its own fix, and would do it with nothing going red. Two witnesses, each killed by exactly one mutation: - target=dev with `main` present: `main` and `dev` survive while a genuinely merged branch is still deleted. That last assertion is a positive control; without it the test would pass against a guard that protected everything. - `origin/HEAD` deleted from a real clone, so resolution genuinely fails rather than being stubbed: both branches survive AND the output states the default could not be determined. A protected-more that is silent still reads as "the default resolved fine" to the next reader. The pre-existing `test_prune_skips_current_and_default` is left in place but does not cover any of this: its fixture has current == target == `main`, so `main` is protected twice over and the test passes with EITHER clause of the old guard removed. It kills no mutant while carrying the name of the guarantee it fails to check. Co-Authored-By: Claude --- src/cli/commands/prune.rs | 35 +++++++++-- src/git/mod.rs | 18 ++++++ tests/test_prune.rs | 123 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 4 deletions(-) diff --git a/src/cli/commands/prune.rs b/src/cli/commands/prune.rs index 773d0e5c..4d11834e 100644 --- a/src/cli/commands/prune.rs +++ b/src/cli/commands/prune.rs @@ -1,14 +1,15 @@ //! Prune command implementation //! -//! Deletes local branches that have been merged into the default branch. +//! Deletes local branches that have been merged into the manifest target. //! Optionally prunes remote tracking refs. use crate::cli::output::Output; use crate::core::manifest::Manifest; use crate::core::repo::{filter_repos, RepoInfo}; use crate::git::branch::{delete_local_branch, is_branch_merged, list_local_branches}; -use crate::git::{get_current_branch, open_repo, path_exists}; +use crate::git::{get_current_branch, get_default_branch, open_repo, path_exists}; use crate::util::log_cmd; +use std::collections::BTreeSet; use std::path::Path; use std::process::Command; @@ -63,11 +64,37 @@ pub fn run_prune( } }; + // Branches that must never be pruned. Three members, not two: the branch + // checked out, the manifest target, and the remote's DEFAULT branch. The + // default is the only one that asserts "permanent" rather than "currently + // interesting" -- and until the target moved off main it was protected + // only by coincidence, because target and default were the same value. + let mut protected: BTreeSet = BTreeSet::new(); + protected.insert(current_branch.clone()); + protected.insert(repo.target_branch().to_string()); + match get_default_branch(&git_repo, &repo.sync_remote) { + Some(default_branch) => { + protected.insert(default_branch); + } + None => { + // Unknown default: protect MORE, never less. A resolution failure + // that silently shrinks this set reproduces the exact defect the + // set exists to prevent, one level up, inside its own fix -- and + // it would fail with nothing going red. Say so out loud, because + // a protected-more that is silent still reads as "resolved fine". + protected.insert("main".to_string()); + protected.insert("dev".to_string()); + Output::warning(&format!( + "{}: could not determine the default branch (no {}/HEAD); protecting 'main' and 'dev' by name", + repo.name, repo.sync_remote + )); + } + } + let mut merged_branches: Vec = Vec::new(); for branch in &branches { - // Skip current branch and default branch - if branch == ¤t_branch || branch == repo.target_branch() { + if protected.contains(branch) { continue; } diff --git a/src/git/mod.rs b/src/git/mod.rs index cd7ebcca..4c49e849 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -198,6 +198,24 @@ pub fn get_current_branch(repo: &Repository) -> Result { } } +/// Resolve the remote's default branch from `refs/remotes//HEAD`. +/// +/// Local only: a cleanup verb must not acquire a network failure mode, so this +/// reads the ref that `clone` writes rather than asking the remote. Returns +/// `None` when the ref is absent, or present but not symbolic — both are normal +/// states for a clone that was never given one, and both mean "unknown" rather +/// than "no default exists". Callers must treat `None` as a reason to protect +/// more, never as a reason to protect less. +pub fn get_default_branch(repo: &Repository, remote: &str) -> Option { + let reference = repo + .find_reference(&format!("refs/remotes/{}/HEAD", remote)) + .ok()?; + let target = reference.symbolic_target()?; + target + .strip_prefix(&format!("refs/remotes/{}/", remote)) + .map(|name| name.to_string()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/test_prune.rs b/tests/test_prune.rs index 53001d09..a84733a9 100644 --- a/tests/test_prune.rs +++ b/tests/test_prune.rs @@ -2,9 +2,41 @@ mod common; +use assert_cmd::Command as AssertCommand; +use predicates::prelude::*; + use common::fixtures::WorkspaceBuilder; use common::git_helpers; +/// Put a fixture into the shape production actually runs in: manifest target is +/// `dev`, `dev` is checked out, and `main` exists as the release branch. Before +/// 2026-08-01 target and default were both `main`, which protected `main` by +/// coincidence; every prune test still encodes that retired arrangement. +fn dev_target_workspace(repo: &str) -> common::fixtures::WorkspaceFixture { + let ws = WorkspaceBuilder::new().add_repo(repo).build(); + let manifest_path = ws + .workspace_root + .join(".gitgrip") + .join("spaces") + .join("main") + .join("gripspace.yml"); + let yaml = std::fs::read_to_string(&manifest_path).unwrap(); + assert!( + yaml.contains("default_branch: main"), + "fixture no longer declares a target this helper knows how to move: {yaml}" + ); + std::fs::write( + &manifest_path, + yaml.replace("default_branch: main", "default_branch: dev"), + ) + .unwrap(); + + let repo_path = ws.repo_path(repo); + git_helpers::create_branch(&repo_path, "dev"); + assert!(git_helpers::branch_exists(&repo_path, "main")); + ws +} + #[test] fn test_prune_dry_run_lists_merged_branches() { let ws = WorkspaceBuilder::new().add_repo("alpha").build(); @@ -131,3 +163,94 @@ fn test_prune_no_merged_branches() { // Unmerged branch should still exist assert!(git_helpers::branch_exists(&repo_path, "feat/unmerged")); } + +#[test] +fn test_prune_protects_main_when_target_is_dev() { + // The production shape: target `dev`, standing on `dev`, `main` present. + // `main` is neither current nor target here, which is exactly the state the + // old two-slot guard left unprotected. + let ws = dev_target_workspace("alpha"); + let repo_path = ws.repo_path("alpha"); + + // A genuinely merged branch, so this test also proves prune still WORKS. + // Without it, a fix that protected everything would pass just as happily. + git_helpers::create_branch(&repo_path, "feat/spent"); + git_helpers::commit_file(&repo_path, "spent.txt", "x", "spent work"); + git_helpers::checkout(&repo_path, "dev"); + std::process::Command::new("git") + .args(["merge", "feat/spent", "--no-ff", "-m", "merge spent"]) + .current_dir(&repo_path) + .output() + .unwrap(); + + AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .args(["prune", "--execute", "--repo", "alpha"]) + .assert() + .success(); + + assert!( + git_helpers::branch_exists(&repo_path, "main"), + "the release branch must survive a prune run from dev" + ); + assert!(git_helpers::branch_exists(&repo_path, "dev")); + assert!( + !git_helpers::branch_exists(&repo_path, "feat/spent"), + "positive control: prune must still delete a merged branch, or this test \ + would pass against a guard that simply protects everything" + ); +} + +#[test] +fn test_prune_protects_more_and_says_so_when_default_is_unresolvable() { + // The default is made GENUINELY unresolvable -- origin/HEAD is deleted from a + // real clone -- rather than stubbed. A stub would assert the code path against + // a fixture instead of against the condition, and the real failure (a clone + // that was never given an origin/HEAD) would go unexercised. + let ws = dev_target_workspace("alpha"); + let repo_path = ws.repo_path("alpha"); + + let before = std::process::Command::new("git") + .args(["symbolic-ref", "refs/remotes/origin/HEAD"]) + .current_dir(&repo_path) + .output() + .unwrap(); + assert!( + before.status.success(), + "control: the clone must HAVE an origin/HEAD before we remove it, or this \ + test proves nothing about removing it" + ); + + std::process::Command::new("git") + .args(["symbolic-ref", "--delete", "refs/remotes/origin/HEAD"]) + .current_dir(&repo_path) + .output() + .unwrap(); + let after = std::process::Command::new("git") + .args(["symbolic-ref", "refs/remotes/origin/HEAD"]) + .current_dir(&repo_path) + .output() + .unwrap(); + assert!( + !after.status.success(), + "the removal must actually make resolution fail" + ); + + AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .args(["prune", "--execute", "--repo", "alpha"]) + .assert() + .success() + // Protecting more must not be silent. A silent protected-more reads as + // "the default resolved fine" to the next person who runs this. + // NOTE: Output::warning writes to stdout, not stderr -- asserted against + // the stream the binary actually uses, verified by running it. + .stdout(predicate::str::contains( + "could not determine the default branch", + )); + + assert!(git_helpers::branch_exists(&repo_path, "main")); + assert!(git_helpers::branch_exists(&repo_path, "dev")); +} From d84516601969abb00594e2c1504c9245a53052a1 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Tue, 25 Aug 2026 14:13:55 -0500 Subject: [PATCH 3/4] feat(spawn): pass Codex startup prompts as developer instructions --- Cargo.toml | 2 +- docs/gr-spawn.md | 12 +- src/cli/commands/spawn.rs | 281 +++++++++++++++++++++++++++++--------- 3 files changed, 229 insertions(+), 66 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ff97c9e2..7302b3b9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -99,11 +99,11 @@ urlencoding = "2" base64 = "0.22" futures = "0.3" which = "7" +tempfile = "3" clap_complete = "4.5.65" rusqlite = { version = "0.39.0", features = ["bundled"] } [dev-dependencies] -tempfile = "3" tokio-test = "0.4" wiremock = "0.6" assert_cmd = "2" diff --git a/docs/gr-spawn.md b/docs/gr-spawn.md index a2e2c7de..36a61672 100644 --- a/docs/gr-spawn.md +++ b/docs/gr-spawn.md @@ -66,7 +66,12 @@ Each agent can have a startup prompt file (Markdown) that defines its role, resp reviewer.md ``` -Prompts are injected via the `--prompt` flag when launching the agent's CLI tool. They should include: +Startup prompt delivery follows the target runtime's native instruction boundary. For +Codex, gitgrip passes the file as `developer_instructions`, keeping identity separate +from the first user turn. Other tools can consume the same file through their configured +launch arguments, such as Claude Code's `--append-system-prompt-file`. + +Prompts should include: - Role description - Responsibilities - Startup checklist (join channel, read journal, check unread, start loop) @@ -140,7 +145,7 @@ tmux select-window -t myproject:lead |-------|---------|-------------| | `session_name` | `"synapt"` | tmux session name | | `channel` | `"dev"` | Default channel all agents join | -| `auto_journal` | `true` | Agents read `recall_journal` on startup | +| `auto_journal` | `true` | Paste recall startup context into Codex as a compatibility fallback. Disable when a runtime SessionStart hook owns continuity. | | `mock_launch` | `false` | Use echo/sleep instead of real agent launch | ### [agents.*] section @@ -151,7 +156,7 @@ tmux select-window -t myproject:lead | `model` | no | `claude-sonnet-4-6` | Model ID | | `tool` | no | `claude` | CLI tool (`claude`, `codex`, `cursor`) | | `worktree` | no | `main` | Git worktree or `"new"` to auto-create | -| `startup_prompt` | no | — | Path to .md startup prompt file | +| `startup_prompt` | no | — | Path to an agent identity prompt. Codex receives it as developer instructions. | | `channel` | no | from `[spawn]` | Channel to auto-join | | `loop_interval` | no | `5m` | Channel read loop cadence | | `heartbeat_interval` | no | `60` | Seconds between heartbeat pings | @@ -174,4 +179,3 @@ tmux select-window -t myproject:lead | `SYNAPT_LOOP_INTERVAL` | `loop_interval` field | `2m` | Plus any custom vars from the `env` table in `agents.toml`. - diff --git a/src/cli/commands/spawn.rs b/src/cli/commands/spawn.rs index 79cc501d..bdbd28d2 100644 --- a/src/cli/commands/spawn.rs +++ b/src/cli/commands/spawn.rs @@ -272,7 +272,17 @@ fn write_launch_script( let safe_name = agent_name.replace(['/', '\\', ':'], "-"); let script_path = script_dir.join(format!("{}-launch.sh", safe_name)); let content = build_launch_script_content(env, worktree_path, launch_cmd); - std::fs::write(&script_path, content)?; + + let mut file = tempfile::NamedTempFile::new_in(&script_dir)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.as_file() + .set_permissions(std::fs::Permissions::from_mode(0o600))?; + } + file.write_all(content.as_bytes())?; + file.as_file().sync_all()?; + file.persist(&script_path).map_err(|e| e.error)?; Ok(script_path) } @@ -523,6 +533,13 @@ pub fn run_spawn_up( launch_env.extend(agent.env.clone()); let worktree_path = resolve_worktree_path(&workspace_root, &agent.worktree); + let codex_startup_prompt = if !mock_mode && agent.tool == "codex" { + read_agent_startup_prompt(&workspace_root, agent).map_err(|e| { + anyhow::anyhow!("failed to load Codex startup prompt for {}: {}", name, e) + })? + } else { + String::new() + }; // Build and send launch command let (launch_cmd, expected_process) = if mock_mode { @@ -608,6 +625,7 @@ pub fn run_spawn_up( parts.extend(resolved_defaults.iter().cloned()); parts.extend(model_inject.iter().cloned()); parts.extend(resolved_args.iter().cloned()); + parts.extend(codex_developer_instruction_args(&codex_startup_prompt)); (shell_join(&parts), Some(expected_process_name(binary))) }; @@ -662,16 +680,6 @@ pub fn run_spawn_up( } if !mock_mode && agent.tool == "codex" { - let startup_prompt = match read_agent_startup_prompt(&workspace_root, agent) { - Ok(prompt) => prompt, - Err(e) => { - Output::warning(&format!( - "Failed to read Codex startup prompt for {}: {}", - name, e - )); - String::new() - } - }; let recall_context = if config.spawn.auto_journal { generate_synapt_startup_context( &worktree_path, @@ -687,8 +695,7 @@ pub fn run_spawn_up( } else { String::new() }; - if let Some(prompt) = build_codex_initial_prompt(name, &startup_prompt, &recall_context) - { + if let Some(prompt) = build_codex_initial_prompt(name, &recall_context) { if let Err(e) = send_codex_initial_prompt(&target, &prompt) { Output::warning(&format!( "Failed to inject Codex startup context for {}: {}", @@ -765,6 +772,18 @@ fn read_agent_startup_prompt(workspace_root: &Path, agent: &AgentConfig) -> anyh .map_err(|e| anyhow::anyhow!("failed to read {}: {}", path.display(), e)) } +fn codex_developer_instruction_args(startup_prompt: &str) -> Vec { + if startup_prompt.trim().is_empty() { + return Vec::new(); + } + + let value = toml::Value::String(startup_prompt.to_string()); + vec![ + "--config".to_string(), + format!("developer_instructions={value}"), + ] +} + fn generate_synapt_startup_context( worktree_path: &Path, agent_name: &str, @@ -802,14 +821,9 @@ fn generate_synapt_startup_context( } } -fn build_codex_initial_prompt( - agent_name: &str, - startup_prompt: &str, - recall_context: &str, -) -> Option { - let startup_prompt = startup_prompt.trim(); +fn build_codex_initial_prompt(agent_name: &str, recall_context: &str) -> Option { let recall_context = recall_context.trim(); - if startup_prompt.is_empty() && recall_context.is_empty() { + if recall_context.is_empty() { return None; } @@ -817,19 +831,11 @@ fn build_codex_initial_prompt( "Load this startup context for agent `{}` before doing any work.", agent_name )]; - if !startup_prompt.is_empty() { - sections.push(format!( - "\n{}\n", - startup_prompt - )); - } - if !recall_context.is_empty() { - let recall_context = wrap_long_lines(recall_context, CODEX_STARTUP_MAX_LINE_CHARS); - sections.push(format!( - "\n{}\n", - recall_context - )); - } + let recall_context = wrap_long_lines(recall_context, CODEX_STARTUP_MAX_LINE_CHARS); + sections.push(format!( + "\n{}\n", + recall_context + )); sections.push( "Use this context to choose the next action. Do not summarize it unless asked.".to_string(), ); @@ -863,7 +869,7 @@ fn wrap_long_lines(text: &str, max_chars: usize) -> String { wrapped.join("\n") } -fn send_codex_initial_prompt(target: &str, prompt: &str) -> anyhow::Result<()> { +fn tmux_load_buffer(prompt: &str) -> anyhow::Result<()> { let mut child = Command::new("tmux") .args(["load-buffer", "-"]) .stdin(Stdio::piped()) @@ -881,31 +887,52 @@ fn send_codex_initial_prompt(target: &str, prompt: &str) -> anyhow::Result<()> { anyhow::bail!("tmux load-buffer exited with {}", status); } - let status = Command::new("tmux") - .args(["paste-buffer", "-d", "-t", target]) - .status()?; - if !status.success() { - anyhow::bail!("tmux paste-buffer exited with {}", status); - } + Ok(()) +} - let status = Command::new("tmux") - .args(["send-keys", "-t", target, "Enter"]) - .status()?; +fn tmux_run(args: &[&str]) -> anyhow::Result<()> { + let status = Command::new("tmux").args(args).status()?; if !status.success() { - anyhow::bail!("tmux send-keys Enter exited with {}", status); + anyhow::bail!("tmux {} exited with {}", args.join(" "), status); } + Ok(()) +} - std::thread::sleep(std::time::Duration::from_millis(300)); - let status = Command::new("tmux") - .args(["send-keys", "-t", target, "Enter"]) - .status()?; - if !status.success() { - anyhow::bail!("tmux send-keys confirm Enter exited with {}", status); - } +fn send_codex_initial_prompt_with( + target: &str, + prompt: &str, + mut load_buffer: L, + mut run_tmux: R, + mut sleep: S, +) -> anyhow::Result<()> +where + L: FnMut(&str) -> anyhow::Result<()>, + R: FnMut(&[&str]) -> anyhow::Result<()>, + S: FnMut(std::time::Duration), +{ + load_buffer(prompt)?; + run_tmux(&["paste-buffer", "-d", "-t", target])?; + run_tmux(&["send-keys", "-t", target, "Enter"])?; + + sleep(std::time::Duration::from_millis(300)); + run_tmux(&["send-keys", "-t", target, "Enter"])?; + + sleep(std::time::Duration::from_millis(300)); + run_tmux(&["send-keys", "-t", target, "Enter"])?; Ok(()) } +fn send_codex_initial_prompt(target: &str, prompt: &str) -> anyhow::Result<()> { + send_codex_initial_prompt_with( + target, + prompt, + tmux_load_buffer, + tmux_run, + std::thread::sleep, + ) +} + /// Resolve a worktree identifier to an absolute path. fn resolve_worktree_path(workspace_root: &Path, worktree: &str) -> PathBuf { if worktree == "main" { @@ -1806,18 +1833,87 @@ mod tests { assert!(script.contains("exec 'codex' 'resume'")); } + #[cfg(unix)] #[test] - fn test_codex_initial_prompt_combines_agent_prompt_and_recall_context() { - let prompt = build_codex_initial_prompt( - "opus", - "You are Opus.", - "Last session: shipped recall startup injection.", + fn test_launch_script_is_owner_only() { + use std::io::Read; + use std::os::unix::fs::PermissionsExt; + + let workspace = tempfile::tempdir().unwrap(); + let script_dir = workspace.path().join(".gitgrip/spawn"); + std::fs::create_dir_all(&script_dir).unwrap(); + let existing_script = script_dir.join("sentinel-launch.sh"); + std::fs::write(&existing_script, "old public content").unwrap(); + std::fs::set_permissions(&existing_script, std::fs::Permissions::from_mode(0o644)).unwrap(); + let mut old_reader = std::fs::File::open(&existing_script).unwrap(); + + let script_path = write_launch_script( + workspace.path(), + "sentinel", + &HashMap::new(), + Path::new("/tmp/worktree"), + "'codex'", ) .unwrap(); + let mode = std::fs::metadata(script_path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + let mut old_content = String::new(); + old_reader.read_to_string(&mut old_content).unwrap(); + assert_eq!(old_content, "old public content"); + assert!(!std::fs::read_to_string(existing_script) + .unwrap() + .contains("old public content")); + } + + #[test] + fn test_codex_developer_instructions_preserve_startup_prompt() { + let startup_prompt = concat!( + "You are Stromus.\n", + "Quotes: \"truth\" and 'care', plus \"\"\" and ''' fences.\n", + "Path: C:\\work\\synapt\n", + "Shell-like text stays text: $(touch nope) and `touch nope`." + ); + let args = codex_developer_instruction_args(startup_prompt); + + assert_eq!(args[0], "--config"); + let parsed = args[1].parse::().unwrap(); + assert_eq!( + parsed + .get("developer_instructions") + .and_then(|v| v.as_str()), + Some(startup_prompt) + ); + } + + #[test] + fn test_codex_developer_instructions_preserve_large_prompt() { + let startup_prompt = format!("# Stromus\n\n{}", "identity substrate\n".repeat(4_000)); + let args = codex_developer_instruction_args(&startup_prompt); + let parsed = args[1].parse::().unwrap(); + + assert_eq!( + parsed + .get("developer_instructions") + .and_then(|v| v.as_str()), + Some(startup_prompt.as_str()) + ); + } + + #[test] + fn test_codex_developer_instructions_skip_empty_prompt() { + assert!(codex_developer_instruction_args("").is_empty()); + assert!(codex_developer_instruction_args(" \n").is_empty()); + } + + #[test] + fn test_codex_initial_prompt_contains_recall_context_only() { + let prompt = + build_codex_initial_prompt("opus", "Last session: shipped recall startup injection.") + .unwrap(); + assert!(prompt.contains("agent `opus`")); - assert!(prompt.contains("")); - assert!(prompt.contains("You are Opus.")); + assert!(!prompt.contains("")); assert!(prompt.contains("")); assert!(prompt.contains("Last session: shipped recall startup injection.")); assert!(prompt.contains("Do not summarize it unless asked.")); @@ -1825,9 +1921,9 @@ mod tests { #[test] fn test_codex_initial_prompt_skips_empty_context() { - assert!(build_codex_initial_prompt("opus", "", "").is_none()); + assert!(build_codex_initial_prompt("opus", "").is_none()); - let prompt = build_codex_initial_prompt("opus", "", "Recall context").unwrap(); + let prompt = build_codex_initial_prompt("opus", "Recall context").unwrap(); assert!(!prompt.contains("")); assert!(prompt.contains("")); } @@ -1835,10 +1931,73 @@ mod tests { #[test] fn test_codex_initial_prompt_wraps_long_recall_lines() { let recall = "x".repeat(CODEX_STARTUP_MAX_LINE_CHARS + 1); - let prompt = build_codex_initial_prompt("opus", "", &recall).unwrap(); + let prompt = build_codex_initial_prompt("opus", &recall).unwrap(); assert!(prompt.contains(&format!("{}\nx", "x".repeat(CODEX_STARTUP_MAX_LINE_CHARS)))); } + #[test] + fn test_codex_prompt_transport_pastes_then_sends_exactly_three_enters() { + let mut loaded = Vec::new(); + let mut commands = Vec::new(); + let mut sleeps = Vec::new(); + + send_codex_initial_prompt_with( + "synapt:atlas", + "recall context", + |prompt| { + loaded.push(prompt.to_string()); + Ok(()) + }, + |args| { + commands.push(args.iter().map(|arg| arg.to_string()).collect::>()); + Ok(()) + }, + |duration| sleeps.push(duration), + ) + .unwrap(); + + assert_eq!(loaded, ["recall context"]); + assert_eq!( + commands, + [ + ["paste-buffer", "-d", "-t", "synapt:atlas"], + ["send-keys", "-t", "synapt:atlas", "Enter"], + ["send-keys", "-t", "synapt:atlas", "Enter"], + ["send-keys", "-t", "synapt:atlas", "Enter"], + ] + ); + assert_eq!( + sleeps, + [ + std::time::Duration::from_millis(300), + std::time::Duration::from_millis(300), + ] + ); + } + + #[test] + fn test_codex_prompt_transport_stops_after_tmux_failure() { + let mut commands = Vec::new(); + let error = send_codex_initial_prompt_with( + "synapt:sentinel", + "recall context", + |_| Ok(()), + |args| { + commands.push(args.iter().map(|arg| arg.to_string()).collect::>()); + if commands.len() == 3 { + anyhow::bail!("simulated tmux failure"); + } + Ok(()) + }, + |_| {}, + ) + .unwrap_err(); + + assert_eq!(error.to_string(), "simulated tmux failure"); + assert_eq!(commands.len(), 3); + assert_eq!(commands[2], ["send-keys", "-t", "synapt:sentinel", "Enter"]); + } + #[test] fn test_resolve_startup_prompt_path_uses_workspace_for_relative_paths() { let root = PathBuf::from("/tmp/gripspace"); From 93a7405bb08a914c449eb588119ea82eb8a5539e Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Tue, 25 Aug 2026 15:08:08 -0500 Subject: [PATCH 4/4] chore(release): prepare gitgrip 1.3.0 --- CHANGELOG.md | 19 +++++++++++++++++++ Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eca04aea..9f6aae0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,25 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.3.0] - 2026-08-25 + +**Scope.** This release promotes `v1.2.0..c095d5d`: 6 commits and 3 first-parent +units, measured with `git rev-list --count` and `git rev-list --first-parent --count` +over that exact range. All three units change the published Rust CLI. + +### Added +- **Codex startup prompts use the native developer boundary.** `gr spawn` loads each + configured `startup_prompt` and supplies it as `developer_instructions`, while recall + continuity remains a separate optional user payload. Missing configured prompts fail + closed. Generated launch scripts are constructed on mode-0600 inodes and atomically + replace prior files. + +### Fixed +- **Unknown repository filters are refused** by `gr add`, `gr commit`, and `gr push` + instead of matching zero repositories and reporting success. +- **Pruning protects the remote default branch** in addition to the current and target + branches. + ## [1.2.0] - 2026-08-21 **Scope, stated first because the range and the artifact are not the same thing.** diff --git a/Cargo.lock b/Cargo.lock index ad0c16a1..911eb4b1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -814,7 +814,7 @@ dependencies = [ [[package]] name = "gitgrip" -version = "1.2.0" +version = "1.3.0" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 7302b3b9..f49cf927 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "gitgrip" -version = "1.2.0" +version = "1.3.0" edition = "2021" rust-version = "1.80" description = "Multi-repo workflow tool - manage multiple git repositories as one"