Skip to content

ci(gate): grant the serena server and fail an enabled server with no grant - #198

Merged
wenzowski merged 1 commit into
mainfrom
claude/groom-cloud-35-zxgre4
Aug 9, 2026
Merged

wenzowski merged 1 commit into
mainfrom
claude/groom-cloud-35-zxgre4

Conversation

@wenzowski

Copy link
Copy Markdown
Contributor

.mcp.json declares the serena MCP server and .claude/settings.json turns it on
through enabledMcpjsonServers, but no permissions.allow rule named it — so every
Serena call prompted for approval, including the mem: reads AGENTS.md mandates at
their triggers and the memory-write path memory-guard deliberately redirects to the
Serena tools. The only grant in place was a git-ignored settings.local.json: it dies
with the container, and an allow there is a widening that house-style §8's raise-only
rule bars from an override layer.

mcp-allow-check had nothing to say about it. Its one predicate caught an allow rule
whose glob reaches the server segment — a grant the CLI skips, so it matches no tool —
and could not see the mirror case, a server with no grant at all. Both fail the same
way: silently, with an approval prompt as the only symptom.

What changed

  • .claude/settings.json grants mcp__serena__*.
  • mise-tasks/mcp-allow-check gains a second predicate, ungranted-enabled-server:
    every name in enabledMcpjsonServers must be matched by an allow rule whose server
    segment equals it. Still a pure function of the settings file — no network, no live
    MCP state — so the hook, CI and the bats suite answer identically.
  • tests/mcp-allow-check.bats covers it, including that an absent key and a boolean
    true are both no-ops, since a gate may only assert what it can enumerate.

For a reviewer

The grant is the whole server rather than the nine read-only tool names, so
write_memory, rename_memory, replace_symbol_body, replace_in_files and
safe_delete_symbol are pre-approved, and a tool serena adds upstream arrives
pre-approved rather than asking. That is a deliberate widening past house-style §5's
absence-means-ask; narrowing it to the read-only set is a one-line change and the gate
accepts either shape.

The suite's existing "this repo's own settings pass the gate today" case is what ties
the predicate to the real file — drop the grant and it goes red.

A connector re-registering as mcp__<uuid>__* (CLOUD-178) stays uncovered: a repo gate
may only assert the portable name, and the UUID is account-specific.

Refs: CLOUD-270

https://claude.ai/code/session_01DfV8M9dF49ZRgyic5FaRsA

@linear-code

linear-code Bot commented Aug 9, 2026 •

Copy link
Copy Markdown
CLOUD-35 Make `pr.rs` consume `ruleset.rs` instead of hardcoding checks and merge method

Why
Current PR behavior hardcodes required checks and merge method even though a ruleset module exists to derive them from host protection.

Acceptance

  • Hardcoded required checks and merge method are removed
  • PR logic reads [ci] config first, then falls back to host ruleset derivation

Refinement — Ready (incomplete — the derive-don't-hardcode constraint is right, but no computable gate exists until the [ci] surface does; see the open questions at the end)

Refinement gate: Definition of Ready & Done. This body carries only specializations.

  • Source of truth (§1). The one authority this change edits is the committed batten.toml's [[rule]] table — the derive-don't-hardcode gate itself. It is not [ci]: Config carries #[serde(deny_unknown_fields)] (crates/batten/src/config.rs:77) over a closed key set — the loader's own refusal enumerates it as version, min_batten_version, strictness, fail_on_warning, rule, scope, protected, unlanded, verb, marker (the last two landed in 30e1509) — so a batten.toml carrying [ci] is refused as a usage error (exit 1) rather than being an absent-but-addable table, and grep -rn 'merge_method' crates/batten/src/ returns zero. That surface belongs to CLOUD-54, whose Acceptance claims it in those words. The effective check-list/merge-method pair is therefore named as derived, supplied by CLOUD-54's reader, and re-typed nowhere; no file under crates/batten carries either value as a literal (rule 1). The pr.rs and ruleset.rs of the title are repo-doctor module names with no counterpart in this tree — crates/batten/src has neither, and ruleset appears in this repo only inside .github/workflows/. This is a port whose source repo the session cannot reach, so the "removal" framing above describes repo-doctor; the Batten-side artifact is the pr verb (pr create|ready|land|watch|dispatch, house-style §2), which is not yet in the declared surface (surface.rs declares check, enforce, config, spec, generate, hook, receipt). This issue therefore specifies the constraint that verb is built under, plus the rule that enforces it ahead of the code, rather than editing a module.

  • Computable predicate (§2) — none is available today, and that is the finding. Two rule shapes were specified across two refinement rounds and both were measured unshippable, so this clause is not satisfied. The issue is held out of the ready queue rather than carrying a gate that cannot land.

    • A module-scoped glob (the unbuilt pr verb's path) is skipped entirely — if matched.is_empty() { return Ok(()); } (crates/batten/src/rules.rs:338) — so it commits as a permanently-green no-op: the failure hk.pkl:125 names in its own words, a gate that cannot match its own label does not fail, it passes.
    • A repo-wide glob (crates/**/*.rs, 28 tracked files) with quoted-flag patterns is an immediate false positive. forbid is a line-level substring match (if line.contains(pattern), rules.rs:527), and "--squash" occurs in landed source at crates/batten/tests/primitives.rs:231 — repo.git(&["merge", "--squash", "feature"]), a fixture driving real git to build squash-merge history, not PR logic hardcoding a merge method. Measured: with that rule at severity = "deny", batten check reports crates/batten/tests/primitives.rs:1 and exits 2, so mise run batten-check (mise.toml:101; the hk step at hk.pkl:262) goes red the moment the rule lands.

    Tightening the pattern does not rescue it. The literal a fixture uses to exercise a merge method and the literal a PR verb would use to hardcode one are the same bytes, so no substring predicate can separate them. Separating them requires something to compare against — the [ci] table — at which point the gate stops being a banned shape and becomes "the effective check-list/merge-method pair equals the derived pair". That comparison is CLOUD-54's reader, not this issue's.

  • Scope of the predicate (§2, continued) — the forbid half only; the capability gap is CLOUD-54. The open-ended half — no inline required-check list — needs a command-kind rule (CLOUD-89), which the §5 effect split bars from check and routes to batten enforce (CLOUD-170). enforce is implemented (crates/batten/src/cli.rs:55) but nothing in this repo invokes it: the gate's only batten step runs cargo run --quiet -p batten -- check, so that half would have no call site and nothing to compare against until CLOUD-54's reader lands — calling it "wired into the gate" would be false. It ships with CLOUD-54 instead. The epic's "Option A" fixture suite under mise run test still covers the library primitive; after the 2026-08-07 §2 amendment it is no longer the gate.

  • Effect (§3). This change adds no command path, so it adds no row to the effect table and nothing to the derived read-only allowlist. The derivation is inspection-only, but the verb that will surface it — batten pr — is write in house-style §2 and declares its own row when it is built: no inheritance, and an absent row means Ask, never read (effect.rs).

  • Output & exit (§5). Pointer-only, byte-stable when surfaced; the PR/merge verdict maps to the standard exit contract at the consuming command, and the derive-don't-hardcode rule itself reports a path:line and exits 2 — a policy verdict, not 1 (house-style §6–§7).

  • Commit / bump (§6). ci → no bump — the change is Batten's own gate config (batten.toml) plus a test under crates/batten/tests/, with no edit to crates/batten/src, and a ci-typed commit releases nothing at any version. The changelog stays derived by release-plz, never authored.

  • Test obligation (§7) — contingent on the §2 resolution below. As written it also trips the rule it tests: the test's own source lives under crates/batten/tests/, inside the crates/**/*.rs glob, so a literal merge_method in it fires the committed rule against this tree. The repo already knows this trap and evades it deliberately — crates/batten/tests/cli.rs:1023 builds the conflict marker as "<".repeat(7) so no-conflict-markers cannot match the test asserting it. Any revived §7 must do the same. End-to-end over the compiled binary in the crates/batten/tests/cli.rs idiom (siblings: config_schema.rs, config_trust.rs, fail_on_warning.rs, surface.rs): a fixture repo carrying merge_method = "squash" in a matched .rs file makes batten check exit 2 with a pointer-only path:line, and the same tree without the literal exits 0. Second, the anti-vacuity assertion: over this repo's committed batten.toml the rule's glob must match at least one tracked file, so the gate can never silently decay into the skipped no-op rules.rs:341 permits. The engine's own forbid unit coverage substitutes for neither (DoR §7).

  • Blockers (§8). blockedBy CLOUD-12 — the rule/check engine that evaluates this gate, landed on main as crates/batten/src/rules.rs (PR feat(check): rule and check engine with a static forbid kind #54, merged; issue In Review, not yet Done) — and CLOUD-54, which owns both the [ci] surface §1 disclaims and the rulesets reader supplying the derived half, so neither can be specified here without silently pre-deciding it (DoR §8). relatedTo CLOUD-89 and CLOUD-170 — the command kind and the effect split that route the open-ended half to batten enforce; cross-references rather than dependencies, since that half is out of scope above. relatedTo CLOUD-36 — that sibling extracts the derive-don't-hardcode pattern as a core primitive (In Review; PR feat: extract the git and state core primitives (CLOUD-36) #150 merged 06:12Z); this issue applies it to the check-list / merge-method pair and ships the rule, so the two do not overlap. Refinable now; the rule half is implementable once CLOUD-12 closes, the host-derivation half once CLOUD-54's reader lands.


Open questions blocking Ready:

  1. Is this an independent unit of work, or a design constraint on CLOUD-54? Every clause above that survives contact with the tree describes something CLOUD-54 delivers: the [ci] surface, the rulesets reader, and now — per §2 — the only comparison that can make the gate exact. What remains uniquely here is the constraint "derive, never inline", which is a sentence in CLOUD-54's acceptance rather than a change of its own. The precedent for that disposition is CLOUD-47, canceled with its unique acceptance restated inside CLOUD-64.
  2. Which authoritative artifact does it change (§1)? None of the three candidates exists in this tree: pr.rs/ruleset.rs are predecessor module names (crates/batten/src has neither), batten pr is a house-style §2 verb absent from the declared surface, and [ci] is refused outright by #[serde(deny_unknown_fields)] — the loader's own message enumerates the ten keys it accepts, and ci is not among them. DoR §1 requires exactly one authoritative artifact; today there are zero.

Both are answerable the moment CLOUD-54 is refined, and neither is answerable before it. Per the gate document, refinement may proceed around a blocker but implementation may not — and here the blocker removes the artifact and the predicate, not just the schedule, so the honest state is Backlog with the dependency recorded rather than a Ready block asserting a gate that goes red on landing.


Canceled — the derive-don't-hardcode constraint belongs to CLOUD-54; this issue has no predicate of its own.

Both open questions above are answered by measurement, without CLOUD-54 being refined first.

§2 — no forbid predicate expresses "derive, don't hardcode." The prior round found one collision: a repo-wide glob false-positives on a landed fixture. Path-scoping to crates/batten/src/** clears that collision — measured, pattern = "--squash" over that glob exits 0 on the current tree — but the pair that remains is fatal, and its two errors run in opposite directions.

  • Pattern the flag literal → false negative on the target shape. pattern = "--squash", glob = "crates/batten/src/**", against a source file containing configured.unwrap_or("squash") — the merge method fixed in code, precisely what this issue forbids — batten check exits 0. The rule does not see it.
  • Pattern the field name → false positive on the correct implementation. pattern = "merge_method", glob = "crates/**", against a reader declaring allowed_merge_methods: Vec<String> — the GitHub rules API field any derived implementation must name — batten check reports <path>:4 and exits 2. forbid is a line-level substring match (forbid_in_file, crates/batten/src/rules.rs), and allowed_merge_methods contains merge_method.

A hardcoded merge method and a derived one are the same bytes. No substring predicate separates them in either direction, so neither tightening the pattern nor scoping the glob rescues the gate. The separating predicate is the comparison effective pair == host-derived pair, which is a reader rather than a banned shape, and that reader is CLOUD-54's.

§1 — zero authoritative artifacts, confirmed against the tree. crates/batten/src carries no pr.rs and no ruleset.rs. SURFACE declares check, enforce, config{,show,epoch,lint}, spec, doctor, generate{,completions,schema}, hook and receipt{,record,status} — no pr. [ci] is refused by Config's deny_unknown_fields. The first acceptance line is already vacuous here: merge_method, --squash, --rebase, --merge and --admin return zero hits across crates/batten/src, and --squash occurs once in the entire crate tree, in a test driving real git. That line described repo-doctor. The second is CLOUD-54's acceptance, in CLOUD-54's words.

The constraint is already authoritative as house-style §9, "reconstruct, don't hardcode," so an issue restating it holds a second copy of a rule that has one home (DoR §1). Its precedence — [ci] first, host derivation as fallback — is not carried across as written: it makes one fact answerable from two places, which DoR §1 bars and house-style §8's single committed authority contradicts. CLOUD-54's "rulesets primary, [ci] derived" is the direction that holds.

Unique acceptance folded into CLOUD-54, which owns the [ci] surface, the rulesets reader, and the only comparison that can gate the constraint. Parent epic CLOUD-9 closed naming no conjunct for this child. The refinement above stands as the record of why no gate could be authored; what it left open is closed here.

CLOUD-270 Grant the serena MCP server, and fail an enabled server that no allow rule names

Why

.mcp.json declares the serena server and .claude/settings.json turns it on through enabledMcpjsonServers, but no permissions.allow rule named it. Every Serena call therefore prompted for approval — including the mem: reads AGENTS.md mandates at their triggers, and the memory-write path memory-guard deliberately redirects to the Serena tools. The only grant in place was a git-ignored .claude/settings.local.json, which dies with the container; an allow there is also a widening, which house-style §8's raise-only override rule bars.

mcp-allow-check could not see it. Its single predicate caught an allow rule whose glob reaches the server segment — a grant the CLI skips, so it matches no tool — and had nothing to say about the mirror case: a server with no grant at all. Both fail the same way, silently, with an approval prompt as the only symptom.


Refinement — Ready

Refinement gate: Definition of Ready & Done. This body carries only specializations.

  • Source of truth (§1). One authority: .claude/settings.json's permissions.allow. mise-tasks/mcp-allow-check is that authority's mechanism rather than a second copy — it reads the file and asserts a property of it, holding no grant of its own. settings.local.json stops being a durable home for the grant, which is what removes the re-typing.
  • Computable predicate (§2). mise run mcp-allow-check (the hk gate step at hk.pkl, glob .claude/settings.json) gains a second predicate, ungranted-enabled-server: every name in enabledMcpjsonServers must be matched by an allow rule whose server segment equals it, exiting 1 and naming the server otherwise. A pure function of the settings file — no network, no live MCP state — so the same command answers the same way in the hook, on CI, and in the bats suite.
  • Grant scope. mcp__serena__*, one rule for the whole server, chosen over the nine read-only tool names. This pre-approves the write and destructive tools — write_memory, rename_memory, replace_symbol_body, replace_in_files, safe_delete_symbol — that house-style §5's absence-means-ask would otherwise leave prompting, and a tool serena adds upstream arrives pre-approved rather than asking. memory-guard still denies a direct Write/Edit to .serena/memories/, so the write path it redirects to now runs unprompted.
  • Effect (§3). No new command path: no row in SURFACE, nothing in the derived read-only allowlist.
  • Output & exit (§5). Pointer-only — a finding names the offending rule or server, never settings content (non-negotiable rule 4). The exit contract is unchanged: 0 clean, 1 a defect in the allowlist, 2 settings that are not readable JSON.
  • Commit / bump (§6). ci — Batten's own gate config and a task, with no edit under crates/batten/src. Patch until 0.1.0, since release-plz bumps the patch whatever the type says below that.
  • Test obligation (§7). tests/mcp-allow-check.bats: an enabled server no rule names exits 1 and names it; a tool-name glob, a bare server-level rule, and a tool-by-tool list each pass; two enabled servers with one grant fails on the ungranted one; an absent enabledMcpjsonServers and a boolean true are both no-ops, since a gate may only assert what it can enumerate. The suite's existing "this repo's own settings pass the gate today" case is the anti-vacuity link — drop the grant and it goes red.
  • Blockers (§8). None.

Out of scope. A connector re-registering under mcp__<uuid>__* (CLOUD-178) is not covered and cannot be: a repo gate may only assert the portable name, and the UUID is account-specific, so rule 1 keeps it out of committed config. CLOUD-191 is the durable answer there.

Review in Linear

…grant

`.mcp.json` declares serena and `enabledMcpjsonServers` turns it on, but no
allow rule named it, so every Serena call prompted for approval — including the
`mem:` reads AGENTS.md mandates at their triggers, and the memory-write path
`memory-guard` deliberately redirects to the Serena tools. The only grant was a
git-ignored `settings.local.json`, which dies with the container; an `allow`
there is also a widening that house-style §8 bars from an override layer.

`mcp-allow-check` could not see it. Its one predicate caught an allow rule whose
glob the CLI skips — a grant matching no tool — and had nothing to say about the
mirror case, a server with no grant at all. Both fail silently with an approval
prompt as the only symptom. `ungranted-enabled-server` closes the second: every
name in `enabledMcpjsonServers` must be matched by an allow rule whose server
segment equals it.

The grant is `mcp__serena__*` rather than the nine read-only tool names, so the
write and destructive tools are pre-approved too.

Refs: CLOUD-270
@wenzowski
wenzowski marked this pull request as ready for review August 9, 2026 18:00
@wenzowski
wenzowski force-pushed the claude/groom-cloud-35-zxgre4 branch from d5b185f to 50fa038 Compare August 9, 2026 18:00
@wenzowski

Copy link
Copy Markdown
Contributor Author

/fast-forward

@wenzowski
wenzowski merged commit 50fa038 into main Aug 9, 2026
5 checks passed
@wenzowski
wenzowski deleted the claude/groom-cloud-35-zxgre4 branch August 9, 2026 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants