Skip to content

Commit da5cb49

Browse files
fix(review): apply 8-angle code-review findings from #13 (#14)
## Summary #13 merged while its adversarial multi-angle review (8 independent angles, each finding separately verified) was still running. This PR applies **every confirmed finding** — three of them genuine fail-closed violations — as the follow-up sweep. ## Changes **Correctness / fail-closed integrity** | Finding (angle) | Fix | |---|---| | A 200 whose body carries an estate error shape was recorded as an "ok" verdict — the boj router unwraps cartridge failure envelopes into 200 bodies, so a hard miss (e.g. echidna without `ANTHROPIC_API_KEY`) looked like success *(cross-file)* | `invoke()` now inspects 200 bodies for `error` / `success:false` and returns them as failures | | Absent `repo_root` defaulted to the daemon's CWD, silently grafting whatever tree the server was launched from onto a fight for an unrelated slug *(3 angles independently)* | New `plan_ungrounded()` — no filesystem probed at all; every plan whose ground-truth is absent now carries an explicit `ground-truth absent` blocker | | A relative `repo_root` (CLI default `.`) was sent to experts that run in their own working directories; panic-attack requires an absolute path *(line-by-line)* | `summon()` canonicalizes first; on failure the call is refused and recorded fail-closed | | Routing let the obligation reroute away from the planner's chosen specialist (HypatiaFleet+Scan → panic-attack) *(line-by-line)* | Routing is now group-primary, with a test pinning it | | `SQUABBLE_PORT` garbage silently fell back to 7741; `0` would bind an ephemeral port the cartridge can't target *(line-by-line + altitude)* | Malformed or zero port refuses to boot (exit 69); startup prints the actually-bound address | | The golden path (`just test`) never compiled the `boj` feature — a break in `--summon` would ship silently *(cross-file)* | `just test` now also runs the boj-feature tests | **Evidence fidelity** *(altitude)*: `Report` gains typed `expert_verdicts: Vec<ExpertVerdict>` (`#[serde(default)]`, back-compat) — every expert call recorded verbatim (check, cartridge, tool, `ok` = transport **and** body, honest meaning, full untruncated verdict). Previously verdicts were spliced into evidence prose and cut at 400 chars — silent evidence loss. **API robustness** *(altitude)*: `/api/v1/diagnose` accepts both the bare `Gate` and a `{gate: …}` wrapper (no more opaque 422 for callers generalising from `/api/v1/fight`). **Docs / honesty (doctrine #10)** *(removed-behavior + cross-file + reuse)*: fixed the stale "lives in squabble-cli" claim in `context.rs`; the CLI doc now names `fight`/`--summon` and its network surface; the App doc no longer claims a non-loopback "refusal" guard that never existed; README no longer calls squabble-app a stub; `boj.rs` documents the deliberate, feature-contained reversal of `fetch.rs`'s shell-out stance. **Simplifications**: `structural_scan` is a pure producer (clippy allow dropped); the summon cfg-pair collapsed to inline cfg; fight's `USAGE` shared with the top-level help; `FightRequest.repo_root: Option<PathBuf>`. **Refuted in verification (not applied):** the cartridge `domain:"ci"` claim (laminar-mcp uses the identical value) and all efficiency candidates (honest scale analysis at n≈5 escalations, loopback, freshness-by-design). ## RSR Quality Checklist ### Required - [x] Tests pass (43 default + 11 boj-feature tests) - [x] Code is formatted (`cargo fmt --check` clean) - [x] Linter is clean (clippy 0 findings, both feature configs) - [x] No banned language patterns - [x] No `unsafe` blocks without `// SAFETY:` comments (none) - [x] No banned functions - [x] SPDX license headers present on all new/modified source files - [x] No secrets, credentials, or `.env` files included ### As Applicable - [x] Documentation updated for user-facing changes (README status + crate table) - [x] No new dependencies ## Testing Beyond the suites: live re-check of every fixed behavior — ungrounded fight emits the honesty blocker instead of probing the CWD; both diagnose shapes accepted; `SQUABBLE_PORT=0` refuses with exit 69; new unit tests pin group-primary routing, body-error detection, back-compat parsing, and the never-reads-CWD guarantee. **Note:** commits unsigned (no signing key in this environment) — owner/CI to sign. ## Screenshots n/a --- _Generated by [Claude Code](https://claude.ai/code/session_01SUURnQUnzTXYftzWFs5pfZ)_
2 parents 4bd7aaa + 227a93f commit da5cb49

9 files changed

Lines changed: 382 additions & 173 deletions

File tree

Justfile

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,12 @@ clean-all: clean
130130
# TEST & QUALITY
131131
# ═══════════════════════════════════════════════════════════════════════════════
132132

133-
# Run all tests
133+
# Run all tests — including the feature-gated boj expert client, which the
134+
# plain --workspace run never compiles (the golden path must cover it or a
135+
# break in `--summon` ships silently).
134136
test *args:
135137
cargo test --workspace {{args}}
138+
cargo test -p squabble-cli --features boj {{args}}
136139

137140
# Run tests with verbose output
138141
test-verbose:

README.adoc

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,14 @@ A standalone, per-repo CI/CD fighter that drives a stuck pipeline to a
1313
*legitimate* green — or improves red→yellow, or fails and hands the human a
1414
structured report. It *wins* the gate; it never cheats past it.
1515

16-
NOTE: Status — *v0.1 in active development (updated 2026-07-01).* The engine and
16+
NOTE: Status — *v0.1 in active development (updated 2026-07-03).* The engine and
1717
its formal invariant are real and verified (see below); live gate fetch via `gh`
18-
landed in #6. Still open: the `squabble apply` plumbing (moves are proposed, not
19-
yet executed) and the GitHub App webhook server (honest stub pending the owner's
20-
hosting decision). See `docs/CHARTER.adoc` for the design and open owner decisions.
18+
landed in #6; `squabble fight` (classify each red as self-win | escalate |
19+
assign-owner), the `squabble-app` loopback HTTP backend, and the boj-server
20+
expert summoning (`--summon`, feature `boj`) landed in #13. Still open: the
21+
`squabble apply` plumbing (moves are proposed, not yet executed) and the GitHub
22+
App *webhook* leg (pending the owner's hosting decision). See
23+
`docs/CHARTER.adoc` for the design and open owner decisions.
2124

2225
== Core principle: squabble ≠ bypass
2326

@@ -36,8 +39,9 @@ check away, or any override are provably *not* paths to green.
3639
[cols="1,3"]
3740
|===
3841
| `crates/squabble-core` | The fight engine. Pure, detachable, no hypatia/estate dependency — `diagnose → propose move → apply → verify-satisfied`. The valuable reusable part.
39-
| `crates/squabble-cli` | `squabble` — runs locally or as a CI step. `squabble diagnose <gate.json>` today; git/`gh` apply plumbing next.
40-
| `crates/squabble-app` | GitHub App (axum webhook + octocrab). *Stub in v0.1.* Delivers the trigger button; calls the same `squabble-core`.
42+
| `crates/squabble-fight`| The shared host brain: a2ml ecosystem-context reader + workflow ground-truth + the fight planner. One planner for the CLI and the App, so they cannot drift.
43+
| `crates/squabble-cli` | `squabble` — runs locally or as a CI step. `fetch`, `diagnose`, and `fight` (with `--summon` under the `boj` feature to put expert verdicts on the report); apply plumbing next.
44+
| `crates/squabble-app` | The loopback HTTP backend (axum, `127.0.0.1:7741`): serves the shared planner to the boj-server `cicd-squabbler-mcp` cartridge. The GitHub App *webhook* leg (trigger button + octocrab) is still to come — no webhook route is registered.
4145
| `spark/` | The formal heart. `Gate_Machine` + the proved theorem `green ⇔ (non-empty ∧ all passed)`.
4246
|===
4347

crates/squabble-app/src/main.rs

Lines changed: 61 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,22 @@
1111
//! Surface (all JSON):
1212
//!
1313
//! * `GET /health` — liveness + version.
14-
//! * `POST /api/v1/diagnose` — body: a `Gate`; returns the pure engine's
15-
//! `Diagnosis` (state + one conservative move per unsatisfied check).
14+
//! * `POST /api/v1/diagnose` — body: a `Gate` (`{checks:[...]}` — a
15+
//! `{gate:{checks:[...]}}` wrapper is also accepted for symmetry with
16+
//! `/api/v1/fight`); returns the pure engine's `Diagnosis`.
1617
//! * `POST /api/v1/fight` — body: `{slug, gate, repo_root?}`; returns the
1718
//! full `Outcome`/`Report` evidence manifest from the shared planner.
19+
//! When `repo_root` is absent the fight runs **ungrounded**: no filesystem
20+
//! is probed (deliberately not the daemon's own working directory, which
21+
//! would mis-attribute evidence) and the report carries an explicit
22+
//! "ground-truth absent" blocker.
1823
//!
19-
//! Binding is **loopback-only by design** (`127.0.0.1`, port from
20-
//! `SQUABBLE_PORT`, default 7741): this backend has no auth layer, and the
21-
//! boj gateway is co-located. It deliberately refuses to bind non-loopback.
24+
//! The bind address is hardcoded to `127.0.0.1` (only the port is
25+
//! configurable via `SQUABBLE_PORT`) because this backend has no auth layer
26+
//! and the boj gateway is co-located; no interface knob is offered, and none
27+
//! should be added without adding authentication first. A malformed or zero
28+
//! `SQUABBLE_PORT` refuses to boot (exit 69) rather than silently binding
29+
//! somewhere the cartridge is not pointing.
2230
//!
2331
//! The GitHub App *webhook* leg (`check_run.requested_action` — the
2432
//! "Squabble!" button — authenticated via octocrab) is still the next step
@@ -43,9 +51,18 @@ struct FightRequest {
4351
/// The gate: required checks + realised runs (from `squabble fetch` or a host).
4452
gate: Gate,
4553
/// Optional path to a local checkout; enables the a2ml context reader and
46-
/// workflow ground-truthing. Absent/missing paths are fail-safe (empty
47-
/// context, conservative classification).
48-
repo_root: Option<String>,
54+
/// workflow ground-truthing. Absent → the fight runs ungrounded (no
55+
/// filesystem probes) and says so in the report.
56+
repo_root: Option<PathBuf>,
57+
}
58+
59+
/// Accepts both the bare-`Gate` body and a `{gate: ...}` wrapper, so callers
60+
/// who generalise from `/api/v1/fight`'s shape by analogy don't get a 422.
61+
#[derive(Debug, Deserialize)]
62+
#[serde(untagged)]
63+
enum DiagnoseRequest {
64+
Bare(Gate),
65+
Wrapped { gate: Gate },
4966
}
5067

5168
async fn health() -> Json<serde_json::Value> {
@@ -57,44 +74,68 @@ async fn health() -> Json<serde_json::Value> {
5774
}))
5875
}
5976

60-
async fn diagnose_handler(Json(gate): Json<Gate>) -> Json<Diagnosis> {
77+
async fn diagnose_handler(Json(req): Json<DiagnoseRequest>) -> Json<Diagnosis> {
78+
let gate = match req {
79+
DiagnoseRequest::Bare(g) | DiagnoseRequest::Wrapped { gate: g } => g,
80+
};
6181
Json(diagnose(&gate))
6282
}
6383

6484
async fn fight_handler(Json(req): Json<FightRequest>) -> Json<Outcome> {
65-
let root = PathBuf::from(req.repo_root.unwrap_or_else(|| ".".to_string()));
66-
let (_context, outcome) = squabble_fight::plan_at_root(&req.gate, &req.slug, &root);
85+
let outcome = match req.repo_root {
86+
Some(root) => squabble_fight::plan_at_root(&req.gate, &req.slug, &root).1,
87+
// No checkout supplied: plan ungrounded rather than silently probing
88+
// whatever directory this daemon happens to have been launched from.
89+
None => squabble_fight::plan_ungrounded(&req.gate, &req.slug),
90+
};
6791
Json(outcome)
6892
}
6993

70-
fn port_from_env() -> u16 {
94+
/// Parse `SQUABBLE_PORT`. Fail-closed: a value that is set but malformed or
95+
/// zero (zero would bind an OS-assigned ephemeral port the cartridge cannot
96+
/// know about) refuses to boot instead of silently using the default.
97+
fn port_from_env() -> Result<u16, String> {
7198
match std::env::var("SQUABBLE_PORT") {
72-
Ok(v) => v.parse().unwrap_or_else(|_| {
73-
eprintln!("squabble-app: SQUABBLE_PORT=`{v}` is not a port; using {DEFAULT_PORT}");
74-
DEFAULT_PORT
75-
}),
76-
Err(_) => DEFAULT_PORT,
99+
Ok(v) => match v.parse::<u16>() {
100+
Ok(0) => Err(
101+
"SQUABBLE_PORT=0 would bind an ephemeral port the cartridge cannot target"
102+
.to_string(),
103+
),
104+
Ok(p) => Ok(p),
105+
Err(e) => Err(format!("SQUABBLE_PORT=`{v}` is not a port: {e}")),
106+
},
107+
Err(_) => Ok(DEFAULT_PORT),
77108
}
78109
}
79110

80111
#[tokio::main]
81112
async fn main() {
113+
let port = match port_from_env() {
114+
Ok(p) => p,
115+
Err(e) => {
116+
eprintln!("squabble-app: {e} — refusing to boot (fail-closed)");
117+
std::process::exit(69); // EX_UNAVAILABLE
118+
}
119+
};
120+
82121
let app = Router::new()
83122
.route("/health", get(health))
84123
.route("/api/v1/diagnose", post(diagnose_handler))
85124
.route("/api/v1/fight", post(fight_handler));
86125

87-
// Loopback-only: no auth layer exists, so a wider bind is not offered.
88-
let addr = SocketAddr::from((Ipv4Addr::LOCALHOST, port_from_env()));
126+
// Loopback-only: the interface is hardcoded, not configurable (no auth
127+
// layer exists, so a wider bind is not offered).
128+
let addr = SocketAddr::from((Ipv4Addr::LOCALHOST, port));
89129
let listener = match tokio::net::TcpListener::bind(addr).await {
90130
Ok(l) => l,
91131
Err(e) => {
92132
eprintln!("squabble-app: cannot bind {addr}: {e}");
93133
std::process::exit(69); // EX_UNAVAILABLE — fail loudly, no fake OK.
94134
}
95135
};
136+
let bound = listener.local_addr().unwrap_or(addr);
96137
println!(
97-
"squabble-app {} listening on http://{addr} (loopback-only)",
138+
"squabble-app {} listening on http://{bound} (loopback-only)",
98139
env!("CARGO_PKG_VERSION")
99140
);
100141

0 commit comments

Comments
 (0)