Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions klasp/src/adopt/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@ use crate::adopt::plan::{AdoptionPlan, ChainSupport, DetectedGate, GateType, Pro
/// The string always ends with a newline. It is suitable for direct `print!`
/// (not `println!`) to stdout.
pub fn render_plan(plan: &AdoptionPlan) -> String {
render_plan_inner(plan, true)
}

/// Like [`render_plan`] but omits the "Next:" footer.
///
/// Use this from callers (e.g. `klasp setup`) that immediately run the
/// next-step commands themselves, making the footer stale/misleading.
pub fn render_plan_no_next(plan: &AdoptionPlan) -> String {
render_plan_inner(plan, false)
}

fn render_plan_inner(plan: &AdoptionPlan, show_next: bool) -> String {
if plan.findings.is_empty() {
return "No existing gates detected. Run `klasp init` for a fresh klasp.toml.\n"
.to_string();
Expand All @@ -20,10 +32,12 @@ pub fn render_plan(plan: &AdoptionPlan) -> String {
out.push('\n');
out.push_str(&render_gate(gate));
}
out.push_str("\nNext:\n");
out.push_str(" klasp init --adopt --mode mirror\n");
out.push_str(" klasp install --agent all\n");
out.push_str(" klasp doctor\n");
if show_next {
out.push_str("\nNext:\n");
out.push_str(" klasp init --adopt --mode mirror\n");
out.push_str(" klasp install --agent all\n");
out.push_str(" klasp doctor\n");
}
out
}

Expand Down
44 changes: 39 additions & 5 deletions klasp/src/cmd/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use std::path::Path;
use std::process::ExitCode;

use klasp_agents_codex::git_hooks::{MANAGED_END, MANAGED_START};
use klasp_core::{
AgentSurface, CheckSourceConfig, ConfigV1, InstallContext, KlaspError, GATE_SCHEMA_VERSION,
};
Expand Down Expand Up @@ -190,10 +191,12 @@ pub(crate) fn check_surfaces(repo_root: &Path, config: Option<&ConfigV1>, c: &mu
}
}

/// Check 2 — byte-equality of the on-disk hook against a fresh re-render at
/// the binary's `GATE_SCHEMA_VERSION`. A mismatch means the binary was
/// upgraded since the last `klasp install` (the gate runtime would
/// fail-open in this state).
/// Check 2 — verify the on-disk hook contains a fresh managed block at the
/// binary's `GATE_SCHEMA_VERSION`. For surfaces that splice a managed block
/// into an existing hook (e.g. Codex), the file may have user content above
/// or below the block; we compare only the block itself rather than the
/// full file. A mismatch means the binary was upgraded since the last
/// `klasp install` (the gate runtime would fail-open in this state).
fn check_hook(repo_root: &Path, surface: &dyn AgentSurface, c: &mut Counters) {
let agent_id = surface.agent_id();
let hook_path = surface.hook_path(repo_root);
Expand All @@ -217,7 +220,20 @@ fn check_hook(repo_root: &Path, surface: &dyn AgentSurface, c: &mut Counters) {
};
let expected = surface.render_hook_script(&ctx);

if actual == expected {
let ok = actual == expected || {
// Surfaces with managed blocks (e.g. Codex) may have user content
// outside the block. Compare only the block portion — that's what
// encodes the schema version.
match (
extract_managed_block(&actual),
extract_managed_block(&expected),
) {
(Some(a), Some(e)) => a == e,
_ => false,
}
};

if ok {
c.ok(&format!(
"hook[{agent_id}]: current (schema v{GATE_SCHEMA_VERSION})"
));
Expand All @@ -228,6 +244,24 @@ fn check_hook(repo_root: &Path, surface: &dyn AgentSurface, c: &mut Counters) {
}
}

/// Extract the klasp managed block (inclusive of both markers and the
/// trailing newline) from a hook file body. Returns `None` when no block
/// markers are present or the markers appear out of order.
fn extract_managed_block(s: &str) -> Option<&str> {
let start = s.find(MANAGED_START)?;
let end_marker = s.find(MANAGED_END)?;
if end_marker < start {
return None;
}
let end_of_marker = end_marker + MANAGED_END.len();
let end = if s.as_bytes().get(end_of_marker) == Some(&b'\n') {
end_of_marker + 1
} else {
end_of_marker
};
Some(&s[start..end])
}

/// Check 3 — settings JSON exists, parses, and contains klasp's
/// `PreToolUse[Bash]` entry.
///
Expand Down
6 changes: 3 additions & 3 deletions klasp/src/cmd/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ fn try_run(args: &SetupArgs) -> Result<ExitCode> {
// Step 1: detect existing gates.
let plan = crate::adopt::detect::detect_all(&repo_root).context("detecting existing gates")?;

println!("klasp setup — detected {} gate(s)", plan.findings.len());
if args.dry_run {
println!("(--dry-run: printing plan only, writing nothing)");
println!("(--dry-run mode — no files will be written)");
}
println!("klasp setup — detected {} gate(s)", plan.findings.len());

// Step 2: detect installed agents on this machine.
let home = crate::fs_util::home_dir();
Expand All @@ -61,7 +61,7 @@ fn try_run(args: &SetupArgs) -> Result<ExitCode> {
);

// Step 3: print the gate detection plan.
print!("{}", crate::adopt::render::render_plan(&plan));
print!("{}", crate::adopt::render::render_plan_no_next(&plan));

// Interactive: gate selection prompt.
let gates_to_use = if args.interactive {
Expand Down
39 changes: 39 additions & 0 deletions klasp/tests/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,45 @@ policy = "any_fail"
);
}

/// Regression guard for #102: when a `.git/hooks/pre-commit` already has
/// user content above the klasp managed block, `doctor` must still exit 0.
/// The old byte-equality check always failed in this shape; the new
/// `contains(expected_trimmed)` secondary check must pass it.
#[test]
fn doctor_codex_hook_with_user_content_above_block_exits_0() {
let repo = fresh_repo_with_claude();
let toml_with_codex = r#"version = 1

[gate]
agents = ["claude_code", "codex"]
policy = "any_fail"
"#;
write_toml(repo.path(), toml_with_codex);
install_seeded(repo.path());

// Seed a pre-commit hook with user content before invoking codex install.
// install_codex_seeded will append the managed block after the user content.
let hooks_dir = repo.path().join(".git/hooks");
fs::create_dir_all(&hooks_dir).unwrap();
fs::write(hooks_dir.join("pre-commit"), "#!/bin/sh\nmake lint\n").unwrap();

install_codex_seeded(repo.path());

let out = run_doctor(repo.path());
assert!(
out.status.success(),
"codex hook with user content above managed block must pass doctor\nstdout:\n{}\nstderr:\n{}",
stdout(&out),
stderr(&out)
);
let so = stdout(&out);
assert!(so.contains("OK hook[codex]:"), "stdout:\n{so}");
assert!(
!so.contains("FAIL"),
"no FAIL lines expected\nstdout:\n{so}"
);
}

/// When `[gate].agents` includes `"codex"` and the codex hook IS installed,
/// `klasp doctor` must exit 0.
#[test]
Expand Down
26 changes: 26 additions & 0 deletions klasp/tests/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,32 @@ fn dry_run_prints_plan_writes_nothing() {
so.to_lowercase().contains("dry-run") || so.contains("writing nothing"),
"stdout should indicate dry-run mode:\n{so}"
);
let first_nonempty = so.lines().find(|l| !l.trim().is_empty()).unwrap_or("");
assert!(
first_nonempty.starts_with("(--dry-run"),
"first non-empty stdout line must start with \"(--dry-run\", got: {first_nonempty:?}"
);
}

// ─── AC: no stale "Next:" footer from setup ──────────────────────────────────

/// `klasp setup` must NOT print a "Next:" block — those commands are run
/// automatically by setup itself, so the footer would be misleading.
#[test]
fn setup_output_does_not_contain_next_footer_when_gates_detected() {
let Some(repo) = fixture_repo() else { return };
let home = fake_home(&[FakeAgent::Claude]);

// Seed a gate so the plan is non-empty (empty plans never emit "Next:" anyway).
fs::write(repo.path().join(".pre-commit-config.yaml"), "repos: []\n").unwrap();

let out = run_setup(repo.path(), home.path(), &[]);

let so = stdout(&out);
assert!(
!so.contains("Next:"),
"`klasp setup` stdout must not contain \"Next:\" footer\nstdout: {so}"
);
}

// ─── AC: claude-only narrowing ───────────────────────────────────────────────
Expand Down
Loading