Compare commits

..
13 Commits
Author SHA1 Message Date
mathias d3faffc4c4 feat(tbd_ship): gate auto-merge on the risk tier; unclassified is not low (#63) (#73)
CD / Build & Import (push) Successful in 4s
CD / Lint / Test / Vet (push) Successful in 11s
CD / Deploy via GitOps (push) Skipped
Co-authored-by: Mathias <mthbqv@gmail.com>
2026-10-08 06:02:34 +00:00
mathiasandClaude Opus 5.5 dbf78a3b34 ci: move template expressions out of run: shells (signals#47)
CD / Lint / Test / Vet (push) Successful in 12s
CD / Build & Import (push) Successful in 6s
CD / Deploy via GitOps (push) Successful in 5s
"Derive image tags" expanded ${{ github.ref }} into the run: body. A
ref name may contain $(...) and the expansion happens before the shell
parses the script, so it now arrives via env: REF instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 09:33:44 +02:00
mathias c3f8d6c002 ci: run the check job on pull requests (#68) (#69)
CD / Deploy via GitOps (push) Successful in 5s
CD / Lint / Test / Vet (push) Successful in 13s
CD / Build & Import (push) Successful in 6s
Co-authored-by: Mathias <mthbqv@gmail.com>
2026-10-05 07:25:38 +00:00
mathias dc127a05b5 Merge pull request 'fix(tools): reject case-variant arg keys and ref-shaped branch names (#65)' (#66) from fix/65-strict-args-branch-names into main
CD / Lint / Test / Vet (push) Successful in 14s
CD / Build & Import (push) Successful in 6s
CD / Deploy via GitOps (push) Skipped
2026-10-05 05:10:51 +00:00
mathiasandClaude Opus 5.5 cd61239451 perf(tools): linear duplicate-key scan keyed on SimpleFold orbit
PR #66 review round 2: the pairwise EqualFold loop was quadratic, 7s of
CPU for ~90k keys under the 1 MiB body cap. foldKey maps each rune to
the smallest member of its unicode.SimpleFold orbit, equal exactly when
strings.EqualFold is, so the scan is a map lookup again. The scaling
test fails on the previous code (7.1s).

Refs #65

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 07:05:00 +02:00
mathiasandClaude Opus 5.5 31a3dd3e0e fix(tools): fail closed when the key scan cannot read the input
PR #66 review: json.Decoder.Token() fails on a number outside float64
range (1e999), which json.Unmarshal ignores in an unknown key, so a
decoy key ended the scan and the #65 repro wrote to main. The decoder
now uses UseNumber, and any scan error is a validation error. The
duplicate check folds with strings.EqualFold (as encoding/json does)
instead of ToLower, so long s and the Kelvin sign are caught in nested
objects too.

Refs #65

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 07:02:27 +02:00
mathiasandClaude Opus 5.5 ae50501ba3 fix(tools): reject case-variant arg keys and ref-shaped branch names
parseArgs now refuses, before any request: two keys equal under
strings.EqualFold in any object (exact duplicates included), and a
top-level key that differs from a schema field only in case. Without
this, encoding/json's case-insensitive last-wins match let
{"branch":"feat/x","Branch":"main"} write to main while a client-side
hook read feat/x.

file_write_branch (branch) and tbd_ship (branch and base) validate the
branch name: ^[A-Za-z0-9][A-Za-z0-9._/-]*$ plus no refs/ prefix, HEAD,
"..", "//", "/.", trailing "/" or ".", or ".lock". refs/heads/main
otherwise created refs/heads/refs/heads/main and wedged API writes to
main. tbd_ship's base is the protection-probe key, so a refs/-prefixed
base would also have read as unprotected.

Closes #65

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 06:58:31 +02:00
mathiasandClaude Opus 5.5 9396b4daaa chore(context): resync derived adapters from canonical AGENT.md
CD / Lint / Test / Vet (push) Successful in 28s
CD / Build & Import (push) Successful in 14s
CD / Deploy via GitOps (push) Successful in 6s
Clears ContextAdapterDrift; canonical changed in local-dev 12f694b/218cf0d.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 22:49:57 +02:00
mathiasandClaude Opus 5.5 0e2bd3d90e fix(mcp): audit record keeps legacy name/index alias targets
CD / Deploy via GitOps (push) Successful in 5s
CD / Lint / Test / Vet (push) Successful in 15s
CD / Build & Import (push) Successful in 7s
A caller using the #38 aliases (name for repo, index for number) left no
target in the tools/call audit record.

Refs #64

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-01 20:41:51 +02:00
mathiasandClaude Opus 5.5 b149dccdbe chore(context): re-sync adapters after root brain-scoping correction
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-01 20:41:47 +02:00
mathiasandClaude Opus 5.5 724a997002 feat(mcp): audit-log every tools/call (OWASP MCP08)
CD / Deploy via GitOps (push) Successful in 5s
CD / Lint / Test / Vet (push) Successful in 31s
CD / Build & Import (push) Successful in 14s
tools/call dispatched with no record, so repo_delete, pr_merge and
file_delete left no trail in gitea-mcp; only auth acceptance was logged.

One slog record per call: tool, caller, token fingerprint, target
(owner/repo/number/branch/path/tag), outcome, duration. Every other
argument is reduced to args_bytes so the audit line never copies file
content or issue bodies.

Closes #64

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-01 20:34:59 +02:00
mathiasandClaude Opus 5 59dabc9455 chore(context): adopt canonical context-sync.sh and re-sync adapters
CD / Lint / Test / Vet (push) Successful in 13s
CD / Build & Import (push) Successful in 17s
CD / Deploy via GitOps (push) Successful in 5s
infra's scripts/context-sync.sh is the estate canonical, and it moved in infra
262369b (a SIGPIPE fix on the placeholder-reporting path: `printf | grep -o |
head -1` became `head -1 < <(grep -o ...)`). Every other repo still carried the
old line, so ContextSyncScriptDrift and ContextAdapterDrift were both firing.

Also picks up the canonical AGENT.md change from local-dev a01ee80 — rule 5's
GitOps review lane and the new precedence rule — which every repo's adapters
need regenerated for regardless of the script.

Refs mathias/infra#470

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-30 10:36:32 +02:00
mathiasandClaude Opus 5 6b57a8ba8a chore(context): regenerate derived adapters from the canonical root context
CD / Lint / Test / Vet (push) Successful in 41s
CD / Build & Import (push) Successful in 22s
CD / Deploy via GitOps (push) Successful in 10s
ContextAdapterDrift was firing for 13 repos. Picks up behaviour rule 7
("Finish what's closable"), which landed in ~/dev/.context/AGENT.md and never
reached any repo's derived files.

Where mcp.json changes, it also picks up root MCP config the repo was stale on:
brain/gitea/infra move from public hostnames to tailnet Serve URLs, and gitea's
bearer changes from GITEA_MCP_TOKEN to DMABE_GITEA_API_TOKEN — gitea-mcp
authenticates by pass-through and 401s on the former, so a harness syncing from
the old file had a dead gitea server and no signal saying so.

No literal secrets: mcp.json carries ${VAR} placeholders only.

CLAUDE.md deliberately unchanged — context-sync writes it from PROJECT.md alone,
since Claude Code walks up to ~/dev/CLAUDE.md for root context.

Refs mathias/infra ContextAdapterDrift

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-25 07:34:24 +02:00
15 changed files with 944 additions and 33 deletions
+80 -4
View File
@@ -78,6 +78,27 @@ These rules apply to every task across every project, regardless of harness.
when a human reviewer outside the project is required. Document the reason in
PROJECT.md.
**Exception — a GitOps repo where a branch cannot be reconciled.** In a repo whose
deployment watches one branch (`mathias/infra`: Flux watches `main`), a branch is
invisible to the thing that applies it. A `risk/medium` change to `alerts.yaml` or
`litellm-config.yaml` can be linted on a branch but never *exercised* there, so a
PR would buy a review of a state that was never deployed.
There, `risk/medium`+ work stays on `main` and instead requires a **recorded
adversarial review pass before the push** — `skills:pr-gatekeeper` or equivalent,
reviewing the staged diff. **The reviewer's verdict goes in the issue, not a PR
thread.** No verdict recorded means the lane was not used, and saying "the gate was
green" is not the same claim.
Be honest about what this is: a recorded *independent* pass, not four-eyes in the
sense `assessor-loop` §4.1 means for a regulated organisation, because the reviewer
is an agent. It is more than the repo had and less than the model it mirrors; do not
describe it as the stronger property. (infra#470, decided 2026-09-29.)
This lane depends on the `risk/` label being right. A label that triggers a heavier
lane is load-bearing, not cosmetic — if work that belongs in it is unlabelled, the
lane silently does not apply.
6. **Close the loop — every substantive task ends with the same ritual.** Shipping
the code is not the end of the task; capturing it is. Run this unprompted:
- **Tag + bump SemVer** on the change (annotated tag; minor for a feature or
@@ -106,6 +127,43 @@ These rules apply to every task across every project, regardless of harness.
revisit Y". Those are exactly the items that need a locator. If it is worth
saying in a summary, it is worth writing where it can be found again without you.
7. **Finish what's closable — only involve me for a genuine call.** If a
tracker issue, PR, or checklist item is demonstrably resolved — the fix
landed, the state changed, you verified it live — close/merge/check it off
in the same turn. Don't leave it open "for the record" or park it waiting
on me to confirm something that already happened; that's bookkeeping, not
a decision. Reserve asking me (`AskUserQuestion`, a `DECISION NEEDED`
comment, or otherwise pausing) for when it's genuinely mine to call:
irreversible or prod/host-impacting actions, a real judgment call with no
clearly-better option, ambiguous intent, or anything secret-writes/
destructive-git already gate. Before claiming something is closable,
verify it live (re-check the issue, re-run the command) rather than
trusting your own earlier summary (brain: `filed-issue-not-same-as-fixed`).
8. **Control code needs an adversary, not a checklist.** Code whose job is to
refuse, deny, gate or contain is not adequately verified by a green gate.
Rule 5 makes CI the quality gate and that holds for ordinary work — but a
passing suite only asserts what its author already thought of, and for a
control the author is precisely the person whose imagination just failed.
Measured on `dispatch#75`: five review rounds, five real escapes verified by
execution, and **CI was green on every one of them** while `git rm
././.dispatch-allow` deleted the file the control existed to protect. So
route this work through a review with authority to block, instructed to
verify by execution rather than by reading the implementation. Expect to be
blocked more than once; that is the control working, not the reviewer being
difficult. (brain: `a-green-gate-is-not-evidence-for-control-code`; the
practices live in the `pr-gatekeeper` agent.)
9. **A test's name must not claim more than its assertions prove.** Name a test
after the mechanism it observes, not the goal it serves.
`DoesNotInheritEnv` is honest; `IsContained` is a claim the test cannot
support. This is not pedantry about naming — when name and assertion
diverge, the name wins in the reader's head and the gap becomes invisible,
which is what lets the defect survive review. Found three times in one PR,
twice in tests written to fix the previous instance of it; each passed while
the thing it was named for was exploitable. If you cannot name the mechanism,
you do not yet know what the test observes.
## CAD sprint patterns
Two CAD sprint archetypes (defs: brain `wiki/homelab/decisions/continuous-agentic-development-cad-concept-2026-06-16`):
@@ -246,10 +304,16 @@ entries that age well are about *why*, *how to avoid*, and *what to do when*.
|---------|-------|-------|
| **Claude Code, Claude Desktop** | `brain_query` (BM25), `brain_answer` (LLM-synth + sources) MCP tools | `brain_write` MCP tool |
| **Crush, Pi, Antigravity, other MCP-capable** | same MCP server: `ingestion-brain` (via the `mcp__*_brain__*` namespace once authenticated) | same |
| **Anything HTTP-only (curl, scripts)** | `POST https://brain-mcp.d-ma.be/query` with `{"query":"..."}` (auth via `BRAIN_MCP_TOKEN`) | `POST .../write` with `{"content":"...","filename":"..."}` |
| **Anything HTTP-only (curl, scripts)** | Tailnet only: `POST https://koala.tail30472e.ts.net:8443/query` with `{"query":"..."}`, sending `BRAIN_MCP_TOKEN` (not yet enforced on REST, hyperguild#149). The public `brain-mcp.d-ma.be` serves `/mcp` only (infra#517). | `POST .../write` with `{"content":"...","wing":"...","hall":"..."}` |
| **Browser / human inspection** | `https://git.d-ma.be/mathias/hyperguild` → `knowledge/` and `wiki/` markdown files |
- **Scoping**: defaults to `public` collection; client projects filter to `{client}` + `public`.
- **Scoping — the brain holds no confidential client content.** Nothing on the read path
filters by client: every token holder can read every wing, and `wing` on `brain_query`
is an optional argument, not an access control. Client confidentiality is enforced only
at write time (`capture`'s I1 gate refuses internal/confidential). So never `brain_write`
client material; generalise it first or keep it in the client's own repo. (hyperguild#144,
decided 2026-10-01. This line previously claimed client projects were filtered; they
were not.)
- **Routing**: brain_answer's LLM uses berget.ai as primary, iguana ollama as
fallback. Both are configurable in the `supervisor/ingestion-deployment.yaml`
on the koala k3s cluster; don't hardcode local-only model names into the
@@ -319,8 +383,20 @@ Workflow:
and missing-untracked adapters). A drift fails the check with a
message telling you to stage the regenerated files.
Behavior rules in this file and per-project rules in `PROJECT.md` apply
unconditionally on every host, every harness.
Behavior rules in this file and per-project rules in `PROJECT.md` apply on every
host and every harness.
**Where a project rule contradicts a rule here, the project rule governs — and it
must state its reason.** Both halves matter: the project file wins because it knows
something this file cannot (infra's Flux constraint is real and not visible from
here), and it must say why because a bare override is indistinguishable from drift.
This replaces the previous wording, which said rules here and in `PROJECT.md` both
apply *"unconditionally"*. That was not a tiebreak but a contradiction, and it went
unnoticed until rule 5 and infra's "never create feature branches" had to be obeyed
at the same time (infra#470). A standard can hold two individually sensible clauses
that are jointly unsatisfiable, and survive for as long as everyone reads it
charitably — the same failure the Inception Sprint Oath's G6 had.
## Engineering Skills
+10 -1
View File
@@ -8,6 +8,12 @@ name: CD
# (Only applies to branch pushes; tag pushes always trigger.)
paths-ignore:
- 'docs/**'
# gitea-mcp#68: without this a PR never got a CI run, so the risk/medium+
# PR lane merged on local `task check` alone. Only the check job runs for a
# PR -- build and deploy stay push/tag-only (no images, no infra writes,
# no secrets from a PR).
pull_request:
branches: [main]
env:
IMAGE: gitea-mcp
@@ -44,6 +50,7 @@ jobs:
build:
name: Build & Import
needs: check
if: github.event_name == 'push'
runs-on: self-hosted
outputs:
image-tag: ${{ steps.meta.outputs.sha-tag }}
@@ -52,10 +59,12 @@ jobs:
- name: Derive image tags
id: meta
env:
# signals#47: a ref name can carry $(...); pass it via env, not ${{ }}.
REF: ${{ github.ref }}
run: |
SHA=$(git rev-parse --short HEAD)
echo "sha-tag=${SHA}" >> "$GITHUB_OUTPUT"
REF="${{ github.ref }}"
if [[ "$REF" == refs/tags/v* ]]; then
echo "version-tag=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
echo "version=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
+80 -4
View File
@@ -73,6 +73,27 @@ These rules apply to every task across every project, regardless of harness.
when a human reviewer outside the project is required. Document the reason in
PROJECT.md.
**Exception — a GitOps repo where a branch cannot be reconciled.** In a repo whose
deployment watches one branch (`mathias/infra`: Flux watches `main`), a branch is
invisible to the thing that applies it. A `risk/medium` change to `alerts.yaml` or
`litellm-config.yaml` can be linted on a branch but never *exercised* there, so a
PR would buy a review of a state that was never deployed.
There, `risk/medium`+ work stays on `main` and instead requires a **recorded
adversarial review pass before the push** — `skills:pr-gatekeeper` or equivalent,
reviewing the staged diff. **The reviewer's verdict goes in the issue, not a PR
thread.** No verdict recorded means the lane was not used, and saying "the gate was
green" is not the same claim.
Be honest about what this is: a recorded *independent* pass, not four-eyes in the
sense `assessor-loop` §4.1 means for a regulated organisation, because the reviewer
is an agent. It is more than the repo had and less than the model it mirrors; do not
describe it as the stronger property. (infra#470, decided 2026-09-29.)
This lane depends on the `risk/` label being right. A label that triggers a heavier
lane is load-bearing, not cosmetic — if work that belongs in it is unlabelled, the
lane silently does not apply.
6. **Close the loop — every substantive task ends with the same ritual.** Shipping
the code is not the end of the task; capturing it is. Run this unprompted:
- **Tag + bump SemVer** on the change (annotated tag; minor for a feature or
@@ -101,6 +122,43 @@ These rules apply to every task across every project, regardless of harness.
revisit Y". Those are exactly the items that need a locator. If it is worth
saying in a summary, it is worth writing where it can be found again without you.
7. **Finish what's closable — only involve me for a genuine call.** If a
tracker issue, PR, or checklist item is demonstrably resolved — the fix
landed, the state changed, you verified it live — close/merge/check it off
in the same turn. Don't leave it open "for the record" or park it waiting
on me to confirm something that already happened; that's bookkeeping, not
a decision. Reserve asking me (`AskUserQuestion`, a `DECISION NEEDED`
comment, or otherwise pausing) for when it's genuinely mine to call:
irreversible or prod/host-impacting actions, a real judgment call with no
clearly-better option, ambiguous intent, or anything secret-writes/
destructive-git already gate. Before claiming something is closable,
verify it live (re-check the issue, re-run the command) rather than
trusting your own earlier summary (brain: `filed-issue-not-same-as-fixed`).
8. **Control code needs an adversary, not a checklist.** Code whose job is to
refuse, deny, gate or contain is not adequately verified by a green gate.
Rule 5 makes CI the quality gate and that holds for ordinary work — but a
passing suite only asserts what its author already thought of, and for a
control the author is precisely the person whose imagination just failed.
Measured on `dispatch#75`: five review rounds, five real escapes verified by
execution, and **CI was green on every one of them** while `git rm
././.dispatch-allow` deleted the file the control existed to protect. So
route this work through a review with authority to block, instructed to
verify by execution rather than by reading the implementation. Expect to be
blocked more than once; that is the control working, not the reviewer being
difficult. (brain: `a-green-gate-is-not-evidence-for-control-code`; the
practices live in the `pr-gatekeeper` agent.)
9. **A test's name must not claim more than its assertions prove.** Name a test
after the mechanism it observes, not the goal it serves.
`DoesNotInheritEnv` is honest; `IsContained` is a claim the test cannot
support. This is not pedantry about naming — when name and assertion
diverge, the name wins in the reader's head and the gap becomes invisible,
which is what lets the defect survive review. Found three times in one PR,
twice in tests written to fix the previous instance of it; each passed while
the thing it was named for was exploitable. If you cannot name the mechanism,
you do not yet know what the test observes.
## CAD sprint patterns
Two CAD sprint archetypes (defs: brain `wiki/homelab/decisions/continuous-agentic-development-cad-concept-2026-06-16`):
@@ -241,10 +299,16 @@ entries that age well are about *why*, *how to avoid*, and *what to do when*.
|---------|-------|-------|
| **Claude Code, Claude Desktop** | `brain_query` (BM25), `brain_answer` (LLM-synth + sources) MCP tools | `brain_write` MCP tool |
| **Crush, Pi, Antigravity, other MCP-capable** | same MCP server: `ingestion-brain` (via the `mcp__*_brain__*` namespace once authenticated) | same |
| **Anything HTTP-only (curl, scripts)** | `POST https://brain-mcp.d-ma.be/query` with `{"query":"..."}` (auth via `BRAIN_MCP_TOKEN`) | `POST .../write` with `{"content":"...","filename":"..."}` |
| **Anything HTTP-only (curl, scripts)** | Tailnet only: `POST https://koala.tail30472e.ts.net:8443/query` with `{"query":"..."}`, sending `BRAIN_MCP_TOKEN` (not yet enforced on REST, hyperguild#149). The public `brain-mcp.d-ma.be` serves `/mcp` only (infra#517). | `POST .../write` with `{"content":"...","wing":"...","hall":"..."}` |
| **Browser / human inspection** | `https://git.d-ma.be/mathias/hyperguild` → `knowledge/` and `wiki/` markdown files |
- **Scoping**: defaults to `public` collection; client projects filter to `{client}` + `public`.
- **Scoping — the brain holds no confidential client content.** Nothing on the read path
filters by client: every token holder can read every wing, and `wing` on `brain_query`
is an optional argument, not an access control. Client confidentiality is enforced only
at write time (`capture`'s I1 gate refuses internal/confidential). So never `brain_write`
client material; generalise it first or keep it in the client's own repo. (hyperguild#144,
decided 2026-10-01. This line previously claimed client projects were filtered; they
were not.)
- **Routing**: brain_answer's LLM uses berget.ai as primary, iguana ollama as
fallback. Both are configurable in the `supervisor/ingestion-deployment.yaml`
on the koala k3s cluster; don't hardcode local-only model names into the
@@ -314,8 +378,20 @@ Workflow:
and missing-untracked adapters). A drift fails the check with a
message telling you to stage the regenerated files.
Behavior rules in this file and per-project rules in `PROJECT.md` apply
unconditionally on every host, every harness.
Behavior rules in this file and per-project rules in `PROJECT.md` apply on every
host and every harness.
**Where a project rule contradicts a rule here, the project rule governs — and it
must state its reason.** Both halves matter: the project file wins because it knows
something this file cannot (infra's Flux constraint is real and not visible from
here), and it must say why because a bare override is indistinguishable from drift.
This replaces the previous wording, which said rules here and in `PROJECT.md` both
apply *"unconditionally"*. That was not a tiebreak but a contradiction, and it went
unnoticed until rule 5 and infra's "never create feature branches" had to be obeyed
at the same time (infra#470). A standard can hold two individually sensible clauses
that are jointly unsatisfiable, and survive for as long as everyone reads it
charitably — the same failure the Inception Sprint Oath's G6 had.
## Engineering Skills
+7
View File
@@ -125,6 +125,13 @@ by area:
| Search / Templates | code_search, create_project_from_template |
| Composite | tbd_ship (trunk-based ship helper) |
**`tbd_ship` merge gate (gitea-mcp#63).** A PR is squash-merged only when ALL of: the head commit's CI is fully green,
the base branch is not review-protected, and the issue named in `issue` carries **exactly one** `risk/` label, `risk/low`.
No `issue`, no `risk/` label, several, an unknown value (`risk/lowish`), `risk/medium`, `risk/high`, or an issue that
cannot be read all fail closed: the PR is opened, `merged:false`, with a `reason` and the `risk` tier in the result.
Unclassified is not low. Limit: the gate enforces the tier of the issue the caller names; it cannot prove that issue is
the one the change belongs to, so the link is the caller's claim (the convention in local-dev rule 5 still applies).
Shared tool-layer hygiene (`internal/tools/tool.go`):
- **Alias normalization** — `repo`↔`name` and `number`↔`index` are reconciled so
a tool works whichever spelling a caller sends (an explicit canonical wins).
+46
View File
@@ -0,0 +1,46 @@
package mcp
import (
"context"
"encoding/json"
"log/slog"
"time"
chassisauth "git.d-ma.be/mathias/mcp-chassis/auth"
"git.d-ma.be/mathias/gitea-mcp/internal/auth"
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
)
// auditTargetKeys are the only argument values copied into the audit record:
// they identify what a call acted on without copying what it wrote. Every
// other argument (file content, issue bodies) is reduced to args_bytes so the
// audit trail never becomes a second copy of repo content (gitea-mcp#64).
var auditTargetKeys = []string{"owner", "repo", "name", "number", "index", "branch", "path", "tag"}
// logToolCall emits one structured record per tools/call (OWASP MCP08).
func logToolCall(ctx context.Context, name string, args json.RawMessage, start time.Time, err error) {
attrs := []any{
"tool", name,
"caller", auth.Caller(ctx),
"args_bytes", len(args),
"duration_ms", time.Since(start).Milliseconds(),
}
if tok, ok := gitea.TokenFromContext(ctx); ok {
attrs = append(attrs, "token_fp", chassisauth.Fingerprint(tok))
}
var parsed map[string]any
if json.Unmarshal(args, &parsed) == nil {
for _, k := range auditTargetKeys {
if v, ok := parsed[k]; ok {
attrs = append(attrs, k, v)
}
}
}
outcome := "ok"
if err != nil {
outcome = err.Error()
}
attrs = append(attrs, "outcome", outcome)
slog.InfoContext(ctx, "mcp tool call", attrs...)
}
+135
View File
@@ -0,0 +1,135 @@
package mcp_test
import (
"bytes"
"context"
"encoding/json"
"errors"
"log/slog"
"net/http"
"net/http/httptest"
"strings"
"testing"
"git.d-ma.be/mathias/gitea-mcp/internal/auth"
"git.d-ma.be/mathias/gitea-mcp/internal/mcp"
"git.d-ma.be/mathias/gitea-mcp/internal/registry"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
type auditStubTool struct {
name string
err error
}
func (s auditStubTool) Descriptor() registry.ToolDescriptor {
return registry.ToolDescriptor{Name: s.name, InputSchema: json.RawMessage(`{"type":"object"}`)}
}
func (s auditStubTool) Call(context.Context, json.RawMessage) (json.RawMessage, error) {
if s.err != nil {
return nil, s.err
}
return json.RawMessage(`{"ok":true}`), nil
}
// captureAudit routes slog to a buffer and returns the decoded
// "mcp tool call" records emitted while fn runs.
func captureAudit(t *testing.T, fn func()) []map[string]any {
t.Helper()
var buf bytes.Buffer
prev := slog.Default()
slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, nil)))
t.Cleanup(func() { slog.SetDefault(prev) })
fn()
var out []map[string]any
for _, l := range strings.Split(strings.TrimSpace(buf.String()), "\n") {
if l == "" {
continue
}
var rec map[string]any
require.NoError(t, json.Unmarshal([]byte(l), &rec))
if rec["msg"] == "mcp tool call" {
out = append(out, rec)
}
}
return out
}
func auditServer(tools ...registry.Tool) http.Handler {
reg := registry.New()
for _, tl := range tools {
reg.Register(tl)
}
srv := mcp.NewServer(mcp.ServerOptions{Registry: reg})
return auth.CallerMiddleware(slog.Default(), srv)
}
func callTool(t *testing.T, h http.Handler, name string, args any) {
t.Helper()
body := map[string]any{
"jsonrpc": "2.0", "id": 1, "method": "tools/call",
"params": map[string]any{"name": name, "arguments": args},
}
b, err := json.Marshal(body)
require.NoError(t, err)
req := httptest.NewRequest(http.MethodPost, "/", bytes.NewBuffer(b))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("X-Auth-Request-User", "alice")
h.ServeHTTP(httptest.NewRecorder(), req)
}
// gitea-mcp#64: tools/call dispatched with no record at all.
func TestToolsCallLogsOneAuditRecordWithToolCallerAndTarget(t *testing.T) {
recs := captureAudit(t, func() {
callTool(t, auditServer(auditStubTool{name: "repo_delete"}), "repo_delete",
map[string]any{"owner": "mathias", "repo": "scratch", "confirm": "scratch"})
})
require.Len(t, recs, 1)
r := recs[0]
assert.Equal(t, "repo_delete", r["tool"])
assert.Equal(t, "alice", r["caller"])
assert.Equal(t, "mathias", r["owner"])
assert.Equal(t, "scratch", r["repo"])
assert.Equal(t, "ok", r["outcome"])
assert.Contains(t, r, "duration_ms")
}
func TestToolsCallAuditRecordsErrorOutcome(t *testing.T) {
recs := captureAudit(t, func() {
callTool(t, auditServer(auditStubTool{name: "pr_merge", err: errors.New("boom")}),
"pr_merge", map[string]any{"owner": "mathias", "repo": "x", "number": 3})
})
require.Len(t, recs, 1)
assert.Equal(t, "boom", recs[0]["outcome"])
assert.EqualValues(t, 3, recs[0]["number"])
}
// The audit line must not become a second copy of repo content.
func TestToolsCallAuditOmitsArgumentValuesOtherThanTarget(t *testing.T) {
const sentinel = "SENTINEL-FILE-CONTENT-7f3a"
recs := captureAudit(t, func() {
callTool(t, auditServer(auditStubTool{name: "file_write_branch"}), "file_write_branch",
map[string]any{"owner": "mathias", "repo": "x", "path": "a.txt", "content": sentinel})
})
require.Len(t, recs, 1)
raw, err := json.Marshal(recs[0])
require.NoError(t, err)
assert.NotContains(t, string(raw), sentinel)
assert.Contains(t, recs[0], "args_bytes")
}
// #38 keeps `name` (for repo) and `index` (for number) as legacy aliases; a
// caller using them must still leave a target in the audit record.
func TestToolsCallAuditRecordsLegacyAliasTargets(t *testing.T) {
recs := captureAudit(t, func() {
callTool(t, auditServer(auditStubTool{name: "issue_close"}), "issue_close",
map[string]any{"owner": "mathias", "name": "infra", "index": 7})
})
require.Len(t, recs, 1)
assert.Equal(t, "infra", recs[0]["name"])
assert.EqualValues(t, 7, recs[0]["index"])
}
+3
View File
@@ -4,6 +4,7 @@ import (
"encoding/json"
"errors"
"net/http"
"time"
"git.d-ma.be/mathias/gitea-mcp/internal/registry"
)
@@ -90,7 +91,9 @@ func (s *Server) handlePOST(w http.ResponseWriter, r *http.Request) {
NewErrorResponse(req.ID, -32602, "invalid params", nil))
return
}
start := time.Now()
out, err := s.opts.Registry.Dispatch(r.Context(), p.Name, p.Arguments)
logToolCall(r.Context(), p.Name, p.Arguments, start, err)
if err != nil {
code := -32000
if errors.Is(err, registry.ErrToolNotFound) {
+3
View File
@@ -63,6 +63,9 @@ func (t *FileWriteBranch) Call(ctx context.Context, raw json.RawMessage) (json.R
if args.Branch == "" {
return nil, fmt.Errorf("branch is required: %w", gitea.ErrValidation)
}
if err := ValidateBranchName(args.Branch); err != nil {
return nil, err
}
if args.Message == "" {
return nil, fmt.Errorf("message is required: %w", gitea.ErrValidation)
}
+42
View File
@@ -0,0 +1,42 @@
package tools
import (
"fmt"
"strings"
"testing"
"time"
)
// foldKey must agree with strings.EqualFold, which is how encoding/json matches.
func TestFoldKeyAgreesWithEqualFold(t *testing.T) {
pairs := [][2]string{{"branch", "Branch"}, {"sha", "ſha"}, {"k", "K"}, {"ss", "ß"}, {"a", "b"}, {"Σ", "ς"}}
for _, p := range pairs {
if got, want := foldKey(p[0]) == foldKey(p[1]), strings.EqualFold(p[0], p[1]); got != want {
t.Errorf("%q vs %q: foldKey equal=%v, EqualFold=%v", p[0], p[1], got, want)
}
}
}
// The duplicate scan is linear in the number of keys: 90k keys (about the
// 1 MiB body cap) must scan well under a second.
func TestRejectAmbiguousKeysScalesLinearly(t *testing.T) {
var b strings.Builder
b.WriteString("{")
for i := 0; i < 90_000; i++ {
if i > 0 {
b.WriteString(",")
}
fmt.Fprintf(&b, `"k%d":1`, i)
}
b.WriteString("}")
var dst struct {
Owner string `json:"owner"`
}
start := time.Now()
if err := rejectAmbiguousKeys([]byte(b.String()), &dst); err != nil {
t.Fatal(err)
}
if d := time.Since(start); d > 2*time.Second {
t.Errorf("scan of 90k keys took %v", d)
}
}
+116
View File
@@ -0,0 +1,116 @@
package tools_test
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"sync/atomic"
"testing"
"git.d-ma.be/mathias/gitea-mcp/internal/allowlist"
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
"git.d-ma.be/mathias/gitea-mcp/internal/tools"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// countingServer fails nothing by itself; it records how many requests reached
// it so a test can assert that a rejected call never touched Gitea.
func countingServer(t *testing.T) (*httptest.Server, *atomic.Int32) {
t.Helper()
var n atomic.Int32
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
n.Add(1)
w.WriteHeader(http.StatusTeapot)
}))
t.Cleanup(srv.Close)
return srv, &n
}
// #65 (1): encoding/json matches keys case-insensitively and the last one wins,
// so a client-side check reading `branch` could see a different value from the
// one the server acts on. Any two keys equal under strings.EqualFold, and any
// key that differs from a schema field only in case, are rejected before a
// request is made.
func TestParseArgsRejectsCaseVariantKeys(t *testing.T) {
cases := map[string]string{
"issue repro: branch + Branch": `{"owner":"probe","repo":"t","name":"t","path":"case8.txt","content":"x","message":"m","branch":"feat/y8","Branch":"main"}`,
"exact duplicate key": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","branch":"main"}`,
"case-only schema mismatch": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","BRANCH":"main"}`,
"nested object duplicate": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","extra":{"a":1,"A":2}}`,
// PR #66 review: a number the token scanner cannot parse as float64 must
// not make the scan give up and let the duplicate through.
"1e999 decoy before duplicate": `{"owner":"probe","repo":"t","name":"t","path":"case8.txt","content":"x","message":"m","z":1e999,"branch":"feat/y8","Branch":"main"}`,
"-1e309 decoy before exact dup": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","z":-1e309,"branch":"feat/y8","branch":"main"}`,
// encoding/json folds like strings.EqualFold: long s (U+017F) == s.
"nested long-s duplicate": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","extra":{"s":1,"\u017f":2}}`,
}
for name, raw := range cases {
t.Run(name, func(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewFileWriteBranch(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
_, err := tool.Call(context.Background(), json.RawMessage(raw))
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
})
}
}
// #65 (2): `refs/heads/main` as a branch makes Gitea create
// refs/heads/refs/heads/main, after which every API write to main 500s.
// Branch names that can create such refs are rejected before any request.
func TestBranchNameValidationRejectsBeforeRequest(t *testing.T) {
bad := []string{
"refs/heads/main", "refs/tags/v1", "a..b", "feat x", "feat\tx", "feat/",
"/feat", "-feat", ".feat", "feat//x", "feat/.x", "feat.lock", "feat.", "HEAD", "feat@{1}", "",
}
for _, b := range bad {
t.Run("file_write_branch "+b, func(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewFileWriteBranch(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
args, _ := json.Marshal(map[string]string{
"owner": "probe", "repo": "t", "path": "p", "content": "x", "message": "m", "branch": b,
})
_, err := tool.Call(context.Background(), args)
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
})
if b == "" {
continue // tbd_ship derives a branch when empty
}
t.Run("tbd_ship "+b, func(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewTBDShip(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
args, _ := json.Marshal(map[string]string{
"owner": "probe", "repo": "t", "path": "p", "content": "x", "message": "m", "branch": b,
})
_, err := tool.Call(context.Background(), args)
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
})
}
}
// tbd_ship's base is the branch-protection probe key: `refs/heads/main` would
// read as unprotected, so it is held to the same rule as the head branch.
func TestTBDShipRejectsRefsPrefixedBase(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewTBDShip(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
_, err := tool.Call(context.Background(), json.RawMessage(
`{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","base":"refs/heads/main"}`))
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
}
func TestBranchNameValidationAcceptsOrdinaryNames(t *testing.T) {
for _, b := range []string{"main", "feat/y8", "tbd/fix-abc-1234abcd", "hold/mail-chat-infra-21", "v1.2_x"} {
t.Run(b, func(t *testing.T) {
assert.NoError(t, tools.ValidateBranchName(b))
})
}
}
+101 -5
View File
@@ -37,7 +37,7 @@ const (
func (t *TBDShip) Descriptor() registry.ToolDescriptor {
return registry.ToolDescriptor{
Name: "tbd_ship",
Description: "Trunk-based ship of a single-file change: branch from base, write the file, open a PR, and auto-merge (squash) to base ONLY when the head commit's CI is fully green. Fails closed to PR-only (never merges) when CI is pending, red, or absent, when the base branch requires reviews, or when the merge isn't clean — returning the PR for manual handling with a reason. Set ci_timeout_seconds to poll CI to completion (default 0 = snapshot, which returns pending right after opening the PR). Returns {merged, pr_url, pr_number, ci_status, reason, branch}.",
Description: "Trunk-based ship of a single-file change: branch from base, write the file, open a PR, and auto-merge (squash) to base ONLY when the head commit's CI is fully green AND the named issue carries exactly one risk/ label, risk/low. Fails closed to PR-only (never merges) when CI is pending, red, or absent, when no issue is named or its risk/ label is missing, ambiguous, unknown, risk/medium or risk/high, when the issue cannot be read, when the base branch requires reviews, or when the merge isn't clean — returning the PR for manual handling with a reason. Set ci_timeout_seconds to poll CI to completion (default 0 = snapshot, which returns pending right after opening the PR). Returns {merged, pr_url, pr_number, ci_status, risk, reason, branch}.",
InputSchema: json.RawMessage(`{
"type":"object",
"properties":{
@@ -50,6 +50,7 @@ func (t *TBDShip) Descriptor() registry.ToolDescriptor {
"branch":{"type":"string","description":"Short-lived branch name. Default derived: tbd/<slug>-<hash>."},
"pr_title":{"type":"string","description":"PR title. Default: the commit message."},
"pr_body":{"type":"string"},
"issue":{"type":"integer","minimum":1,"description":"Issue number in the same repo whose risk/ label classifies this change. Auto-merge happens only for exactly one label, risk/low; no issue, none, several, an unknown value, risk/medium or risk/high means PR only, never merged (unclassified is not low)."},
"delete_branch":{"type":"boolean","description":"Delete the branch after a successful merge. Default true."},
"ci_timeout_seconds":{"type":"integer","minimum":0,"maximum":600,"description":"Poll CI up to this long for the head commit's runs to complete. Default 0 (single snapshot)."}
},
@@ -68,10 +69,76 @@ type tbdShipArgs struct {
Branch string `json:"branch"`
PRTitle string `json:"pr_title"`
PRBody string `json:"pr_body"`
Issue int `json:"issue"`
DeleteBranch *bool `json:"delete_branch"`
CITimeoutSec int `json:"ci_timeout_seconds"`
}
// riskTier is the change's classification under the lane model (local-dev .context/AGENT.md rule 5): only a change
// whose issue carries exactly one risk/ label, risk/low, may merge on green; everything else is reviewed by a human
// (gitea-mcp#63). The zero value is deliberately NOT riskLow: a forgotten field must never mean "low".
type riskTier string
const (
riskLow riskTier = "low"
riskMedium riskTier = "medium"
riskHigh riskTier = "high"
riskNone riskTier = "none" // no issue named, or the issue has no risk/ label: unclassified is not low
riskAmbiguous riskTier = "ambiguous" // more than one risk/ label
riskUnknown riskTier = "unknown" // risk/<something else>
riskUnreadable riskTier = "unreadable" // the issue lookup failed
)
// classifyRisk reads the tier from an issue's labels. Exact, case-sensitive prefix "risk/"; exactly one such label
// is required, so two labels (even identical ones) are ambiguous rather than "the lower one wins".
func classifyRisk(labels []gitea.Label) riskTier {
var found []string
for _, l := range labels {
if strings.HasPrefix(l.Name, "risk/") {
found = append(found, strings.TrimPrefix(l.Name, "risk/"))
}
}
switch len(found) {
case 0:
return riskNone
case 1:
default:
return riskAmbiguous
}
switch found[0] {
case "low":
return riskLow
case "medium":
return riskMedium
case "high":
return riskHigh
default:
return riskUnknown
}
}
const unclassifiedReason = "no risk/ label on the named issue (or no issue named) — unclassified is not low; PR opened, not merged"
// riskBlockReason explains why a tier other than riskLow blocks the merge.
func riskBlockReason(r riskTier) string {
switch r {
case riskMedium, riskHigh:
return fmt.Sprintf("the issue is risk/%s — above risk/low a human reviews the change; PR opened, not merged", r)
case riskAmbiguous:
return "the issue carries more than one risk/ label — ambiguous classification; PR opened, not merged"
case riskUnknown:
return "the issue carries an unknown risk/ value — PR opened, not merged"
case riskUnreadable:
return "could not read the issue's labels — fail-closed; PR opened, not merged"
case riskLow:
return "" // never asked: only a non-low tier needs explaining
case riskNone:
return unclassifiedReason
default: // the zero value and any tier added later: unclassified is not low
return unclassifiedReason
}
}
// shipGate is the decision produced by evaluateShipGate.
type shipGate struct {
merge bool
@@ -101,9 +168,10 @@ func classifyCI(runs []gitea.WorkflowRun) string {
}
// evaluateShipGate is the load-bearing safety of tbd_ship. It returns merge=true
// only when CI is fully green AND the base branch is not review-protected. Every
// other state fails closed to PR-only with an explanatory reason (#40).
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection) shipGate {
// only when CI is fully green AND the base branch is not review-protected AND the
// change is classified risk/low. Every other state fails closed to PR-only with an
// explanatory reason (#40, gitea-mcp#63).
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk riskTier) shipGate {
ci := classifyCI(runs)
// Review-protected base can't be auto-merged regardless of CI.
@@ -112,6 +180,11 @@ func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection) ship
"base branch requires %d approving review(s) — auto-merge disabled; PR opened for manual merge", bp.RequiredApprovals)}
}
// Only risk/low merges on green. Checked before CI so an unclassified change never reads as "waiting for CI".
if risk != riskLow {
return shipGate{false, ci, riskBlockReason(risk)}
}
switch ci {
case "success":
return shipGate{true, ci, ""}
@@ -160,6 +233,13 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
if branch == "" {
branch = deriveBranch(args.Message, args.Path, args.Content)
}
// Both names reach a ref: base is also the protection-probe key, so a
// `refs/heads/main` base would read as unprotected (#65).
for _, b := range []string{base, branch} {
if err := ValidateBranchName(b); err != nil {
return nil, err
}
}
deleteBranch := true
if args.DeleteBranch != nil {
deleteBranch = *args.DeleteBranch
@@ -223,7 +303,8 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
// CI gate on the PR head commit.
runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
gate := evaluateShipGate(runs, bp)
risk := t.riskTierOf(ctx, args.Owner, args.Repo, args.Issue)
gate := evaluateShipGate(runs, bp, risk)
result := map[string]any{
"merged": false,
@@ -231,6 +312,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
"pr_url": pr.HTMLURL,
"branch": branch,
"ci_status": gate.ciStatus,
"risk": string(risk),
}
if !gate.merge {
result["reason"] = gate.reason
@@ -254,6 +336,20 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
return textOK(result)
}
// riskTierOf classifies the change from the named issue. No issue, or an issue that cannot be read, never yields
// riskLow. The link between the PR and the issue is the caller's claim: the gate enforces the tier of the issue it
// is given, it cannot prove that issue is the one the change belongs to (stated limit, gitea-mcp#63).
func (t *TBDShip) riskTierOf(ctx context.Context, owner, repo string, issue int) riskTier {
if issue <= 0 {
return riskNone
}
iss, err := t.c.GetIssue(ctx, owner, repo, issue)
if err != nil || iss == nil {
return riskUnreadable
}
return classifyRisk(iss.Labels)
}
// pollRuns lists the head commit's workflow runs, polling up to timeoutSec for
// them to reach a terminal state. timeoutSec == 0 is a single snapshot (no
// sleep). A listing error yields no runs → the gate fails closed.
+57 -10
View File
@@ -20,22 +20,31 @@ func TestEvaluateShipGate(t *testing.T) {
name string
runs []gitea.WorkflowRun
bp *gitea.BranchProtection
risk riskTier
wantMerge bool
wantCI string
}{
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, true, "success"},
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, false, "pending"},
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, false, "pending"},
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, false, "failed"},
{"cancelled → no merge (fail-closed)", []gitea.WorkflowRun{run("completed", "cancelled")}, open, false, "failed"},
{"no runs → no merge (no CI gate)", nil, open, false, "none"},
{"mixed success+queued → no merge", []gitea.WorkflowRun{run("completed", "success"), run("queued", "")}, open, false, "pending"},
{"green but protected → no merge", []gitea.WorkflowRun{run("completed", "success")}, protected, false, "success"},
{"no runs + protected → no merge", nil, protected, false, "none"},
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskLow, true, "success"},
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, riskLow, false, "pending"},
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, riskLow, false, "pending"},
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, riskLow, false, "failed"},
{"cancelled → no merge (fail-closed)", []gitea.WorkflowRun{run("completed", "cancelled")}, open, riskLow, false, "failed"},
{"no runs → no merge (no CI gate)", nil, open, riskLow, false, "none"},
{"mixed success+queued → no merge", []gitea.WorkflowRun{run("completed", "success"), run("queued", "")}, open, riskLow, false, "pending"},
{"green but protected → no merge", []gitea.WorkflowRun{run("completed", "success")}, protected, riskLow, false, "success"},
{"no runs + protected → no merge", nil, protected, riskLow, false, "none"},
// gitea-mcp#63: the tier is an input. Green CI never overrides it; unclassified is not low.
{"green + risk/medium → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskMedium, false, "success"},
{"green + risk/high → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskHigh, false, "success"},
{"green + no risk label → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskNone, false, "success"},
{"green + ambiguous labels → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskAmbiguous, false, "success"},
{"green + unknown value → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskUnknown, false, "success"},
{"green + unreadable issue → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskUnreadable, false, "success"},
{"green + the zero tier → no merge (a forgotten field must not mean low)", []gitea.WorkflowRun{run("completed", "success")}, open, riskTier(""), false, "success"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
g := evaluateShipGate(tc.runs, tc.bp)
g := evaluateShipGate(tc.runs, tc.bp, tc.risk)
if g.merge != tc.wantMerge {
t.Errorf("merge = %v, want %v (reason: %q)", g.merge, tc.wantMerge, g.reason)
}
@@ -51,3 +60,41 @@ func TestEvaluateShipGate(t *testing.T) {
})
}
}
func TestClassifyRisk(t *testing.T) {
l := func(names ...string) []gitea.Label {
out := make([]gitea.Label, len(names))
for i, n := range names {
out[i] = gitea.Label{Name: n}
}
return out
}
tests := []struct {
name string
labels []gitea.Label
want riskTier
}{
{"low", l("risk/low"), riskLow},
{"medium", l("risk/medium"), riskMedium},
{"high", l("risk/high"), riskHigh},
{"low among others", l("type/fix", "risk/low", "size/s"), riskLow},
{"none", l("type/fix"), riskNone},
{"no labels", nil, riskNone},
{"two different", l("risk/low", "risk/high"), riskAmbiguous},
{"two identical", l("risk/low", "risk/low"), riskAmbiguous},
{"unknown value", l("risk/extreme"), riskUnknown},
{"empty value", l("risk/"), riskUnknown},
{"wrong case", l("Risk/Low"), riskNone},
{"value case matters: risk/Low is unknown, not low", l("risk/Low"), riskUnknown},
{"trailing space in the value is unknown, not low", l("risk/low "), riskUnknown},
{"leading space in the value is unknown, not low", l("risk/ low"), riskUnknown},
{"prefix only inside a name", l("not-risk/low"), riskNone},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if got := classifyRisk(tc.labels); got != tc.want {
t.Errorf("classifyRisk(%v) = %q, want %q", tc.labels, got, tc.want)
}
})
}
}
+115 -6
View File
@@ -26,11 +26,21 @@ func shipFake(t *testing.T, runsJSON string, merged, deleted *atomic.Bool) *http
// shipFakeOpts adds protection (requiredApprovals>0 → protected) and a merge
// status code, so tests can drive the review-protected and conflict paths.
func shipFakeOpts(t *testing.T, runsJSON string, requiredApprovals, mergeStatus int, merged, deleted *atomic.Bool) *httptest.Server {
t.Helper()
return shipFakeFull(t, runsJSON, requiredApprovals, mergeStatus, `[{"id":1,"name":"risk/low"}]`, http.StatusOK, merged, deleted)
}
// shipFakeFull also serves the issue whose risk/ label classifies the change (gitea-mcp#63): issueLabels is the
// labels JSON array, issueStatus the HTTP status of the issue lookup.
func shipFakeFull(t *testing.T, runsJSON string, requiredApprovals, mergeStatus int, issueLabels string, issueStatus int, merged, deleted *atomic.Bool) *httptest.Server {
t.Helper()
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
p := r.URL.Path
w.Header().Set("Content-Type", "application/json")
switch {
case r.Method == http.MethodGet && strings.Contains(p, "/issues/42"):
w.WriteHeader(issueStatus)
_, _ = w.Write([]byte(`{"number":42,"title":"t","state":"open","labels":` + issueLabels + `}`))
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
if requiredApprovals > 0 {
_, _ = w.Write([]byte(`{"required_approvals":` + itoa(requiredApprovals) + `}`))
@@ -83,7 +93,7 @@ func TestTBDShip_GreenCI_Merges(t *testing.T) {
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, true, res["merged"])
assert.Equal(t, "success", res["ci_status"])
@@ -98,7 +108,7 @@ func TestTBDShip_NoCI_FailsClosed(t *testing.T) {
srv := shipFake(t, `[]`, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.Equal(t, "none", res["ci_status"])
@@ -113,7 +123,7 @@ func TestTBDShip_RedCI_FailsClosed(t *testing.T) {
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.Equal(t, "failed", res["ci_status"])
@@ -128,7 +138,7 @@ func TestTBDShip_ReviewProtected_FailsClosed(t *testing.T) {
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 1, http.StatusOK, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.NotEmpty(t, res["reason"])
@@ -142,7 +152,7 @@ func TestTBDShip_MergeConflict_FailsClosed(t *testing.T) {
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 0, http.StatusConflict, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.NotEmpty(t, res["reason"])
@@ -160,6 +170,8 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
p := r.URL.Path
w.Header().Set("Content-Type", "application/json")
switch {
case r.Method == http.MethodGet && strings.Contains(p, "/issues/42"):
_, _ = w.Write([]byte(`{"number":42,"title":"t","state":"open","labels":[{"id":1,"name":"risk/low"}]}`))
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
w.WriteHeader(http.StatusNotFound)
_, _ = w.Write([]byte(`{"message":"not found"}`))
@@ -196,7 +208,7 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
}))
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","branch":"`+branch+`"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42,"branch":"`+branch+`"}`)
assert.Equal(t, true, res["merged"], "resume must merge when CI is green")
assert.Equal(t, float64(7), res["pr_number"], "must reuse the existing PR #7")
@@ -204,6 +216,103 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
assert.True(t, merged.Load())
}
// ---- gitea-mcp#63: the risk tier is an input to the gate. Only exactly one risk/low label merges. ----
const (
shipGreen = `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`
shipArgs = `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`
)
func TestTBDShip_RiskTier_OnlyLowMerges(t *testing.T) {
tests := []struct {
name string
labels string
wantMerged bool
wantInReason string
}{
{"risk/low merges", `[{"id":1,"name":"risk/low"}]`, true, ""},
{"risk/low among other labels merges", `[{"id":2,"name":"type/fix"},{"id":1,"name":"risk/low"},{"id":3,"name":"size/s"}]`, true, ""},
{"risk/medium does not merge", `[{"id":1,"name":"risk/medium"}]`, false, "risk/medium"},
{"risk/high does not merge", `[{"id":1,"name":"risk/high"}]`, false, "risk/high"},
{"no risk label is unclassified, not low", `[{"id":2,"name":"type/fix"}]`, false, "no risk/"},
{"no labels at all", `[]`, false, "no risk/"},
{"two risk labels are ambiguous", `[{"id":1,"name":"risk/low"},{"id":2,"name":"risk/high"}]`, false, "more than one"},
{"two identical risk/low labels are still ambiguous", `[{"id":1,"name":"risk/low"},{"id":2,"name":"risk/low"}]`, false, "more than one"},
{"an unknown risk value does not merge", `[{"id":1,"name":"risk/lowish"}]`, false, "unknown"},
{"an empty risk value does not merge", `[{"id":1,"name":"risk/"}]`, false, "unknown"},
{"labels are case-sensitive: RISK/LOW is not risk/low", `[{"id":1,"name":"RISK/LOW"}]`, false, "no risk/"},
{"a label that merely contains risk/low does not count", `[{"id":1,"name":"not-risk/low"}]`, false, "no risk/"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
var merged, deleted atomic.Bool
srv := shipFakeFull(t, shipGreen, 0, http.StatusOK, tc.labels, http.StatusOK, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, shipArgs)
assert.Equal(t, tc.wantMerged, res["merged"])
assert.Equal(t, tc.wantMerged, merged.Load(), "the merge endpoint is called iff the tier allows it")
assert.Equal(t, "success", res["ci_status"], "CI was green: the tier alone decided")
assert.Equal(t, float64(7), res["pr_number"], "the PR is opened either way")
if !tc.wantMerged {
assert.Contains(t, res["reason"], tc.wantInReason)
}
})
}
}
// No issue named → nothing classifies the change → no merge, even on green CI.
func TestTBDShip_NoIssue_Unclassified_NoMerge(t *testing.T) {
var merged, deleted atomic.Bool
srv := shipFake(t, shipGreen, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
assert.Equal(t, false, res["merged"])
assert.Contains(t, res["reason"], "issue")
assert.False(t, merged.Load(), "an unclassified change must not merge")
assert.Equal(t, float64(7), res["pr_number"])
}
// The issue lookup failing is not a reason to merge: fail closed, PR left open.
func TestTBDShip_IssueLookupFails_FailsClosed(t *testing.T) {
for _, status := range []int{http.StatusInternalServerError, http.StatusNotFound, http.StatusForbidden} {
var merged, deleted atomic.Bool
srv := shipFakeFull(t, shipGreen, 0, http.StatusOK, `[{"id":1,"name":"risk/low"}]`, status, &merged, &deleted)
res := callShip(t, srv.URL, shipArgs)
srv.Close()
assert.Equal(t, false, res["merged"], "status %d", status)
assert.NotEmpty(t, res["reason"], "status %d", status)
assert.False(t, merged.Load(), "status %d: an unreadable issue must not merge", status)
}
}
// risk/low is necessary, not sufficient: red CI still blocks it, and a review-protected base still blocks it.
func TestTBDShip_RiskLow_DoesNotOverrideTheOtherGates(t *testing.T) {
var merged, deleted atomic.Bool
red := shipFakeFull(t, `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`, 0, http.StatusOK, `[{"id":1,"name":"risk/low"}]`, http.StatusOK, &merged, &deleted)
defer red.Close()
assert.Equal(t, false, callShip(t, red.URL, shipArgs)["merged"])
assert.False(t, merged.Load())
prot := shipFakeFull(t, shipGreen, 1, http.StatusOK, `[{"id":1,"name":"risk/low"}]`, http.StatusOK, &merged, &deleted)
defer prot.Close()
assert.Equal(t, false, callShip(t, prot.URL, shipArgs)["merged"])
assert.False(t, merged.Load())
}
// The decision is visible to the caller.
func TestTBDShip_ReportsTheRiskTier(t *testing.T) {
var merged, deleted atomic.Bool
srv := shipFakeFull(t, shipGreen, 0, http.StatusOK, `[{"id":1,"name":"risk/medium"}]`, http.StatusOK, &merged, &deleted)
defer srv.Close()
assert.Equal(t, "medium", callShip(t, srv.URL, shipArgs)["risk"])
}
func TestTBDShip_AllowlistRejects(t *testing.T) {
tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))
+148 -2
View File
@@ -1,10 +1,14 @@
package tools
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"reflect"
"regexp"
"strings"
"unicode"
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
"git.d-ma.be/mathias/gitea-mcp/internal/registry"
@@ -22,12 +26,154 @@ func parseArgs(raw json.RawMessage, dst any) error {
if err := json.Unmarshal([]byte("{}"), dst); err != nil {
return err
}
} else if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
return err
} else {
// Reject ambiguity BEFORE alias normalisation: re-marshalling through a
// map would otherwise silently collapse exact duplicates (#65).
if err := rejectAmbiguousKeys(raw, dst); err != nil {
return err
}
if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
return err
}
}
return validateRequiredIdentifiers(dst)
}
// rejectAmbiguousKeys refuses input whose meaning depends on encoding/json's
// case-insensitive, last-key-wins matching (#65). A client-side check that reads
// `branch` must see the value the server acts on, so: no object at any depth may
// carry two keys equal under strings.EqualFold (exact duplicates included), and
// no top-level key may differ from a field of dst only in case.
func rejectAmbiguousKeys(raw json.RawMessage, dst any) error {
dec := json.NewDecoder(bytes.NewReader(raw))
// UseNumber: without it Token() fails on a number outside float64 range,
// which json.Unmarshal ignores in an unknown key — a decoy that would end
// the scan before the duplicate (PR #66 review).
dec.UseNumber()
if err := scanJSONValue(dec); err != nil {
if errors.Is(err, gitea.ErrValidation) {
return err
}
// Fail closed: input the scanner cannot read is input it cannot vouch for.
return fmt.Errorf("arguments are not valid JSON: %v: %w", err, gitea.ErrValidation)
}
var top map[string]json.RawMessage
if err := json.Unmarshal(raw, &top); err != nil {
return nil // not an object; nothing to compare against the schema
}
fields := jsonFieldNames(dst)
for k := range top {
for _, f := range fields {
if k != f && strings.EqualFold(k, f) {
return fmt.Errorf("argument %q does not match field %q exactly (keys are case-sensitive): %w", k, f, gitea.ErrValidation)
}
}
}
return nil
}
// scanJSONValue consumes one JSON value from dec, failing with ErrValidation if
// any object within it has two keys that are equal under strings.EqualFold.
func scanJSONValue(dec *json.Decoder) error {
tok, err := dec.Token()
if err != nil {
return err
}
d, ok := tok.(json.Delim)
if !ok {
return nil
}
switch d {
case '{':
seen := map[string]string{}
for dec.More() {
kt, err := dec.Token()
if err != nil {
return err
}
k, _ := kt.(string)
fk := foldKey(k)
if prev, dup := seen[fk]; dup {
return fmt.Errorf("duplicate argument keys %q and %q (keys must be unique ignoring case): %w", prev, k, gitea.ErrValidation)
}
seen[fk] = k
if err := scanJSONValue(dec); err != nil {
return err
}
}
case '[':
for dec.More() {
if err := scanJSONValue(dec); err != nil {
return err
}
}
}
_, err = dec.Token() // closing delimiter
return err
}
// foldKey maps every rune to the smallest member of its unicode.SimpleFold
// orbit, so foldKey(a) == foldKey(b) exactly when strings.EqualFold(a, b) —
// the folding encoding/json uses (long s == s, Kelvin sign == k). A map keyed
// on it keeps the duplicate scan linear (PR #66 review: a pairwise EqualFold
// loop cost 7s of CPU on ~90k keys).
func foldKey(s string) string {
var b strings.Builder
for _, r := range s {
min := r
for f := unicode.SimpleFold(r); f != r; f = unicode.SimpleFold(f) {
if f < min {
min = f
}
}
b.WriteRune(min)
}
return b.String()
}
// jsonFieldNames returns the json tag names of dst's top-level struct fields.
func jsonFieldNames(dst any) []string {
v := reflect.ValueOf(dst)
if v.Kind() != reflect.Pointer || v.IsNil() || v.Elem().Kind() != reflect.Struct {
return nil
}
t := v.Elem().Type()
names := make([]string, 0, t.NumField())
for i := 0; i < t.NumField(); i++ {
name, _, _ := strings.Cut(t.Field(i).Tag.Get("json"), ",")
if name == "" || name == "-" {
name = t.Field(i).Name
}
names = append(names, name)
}
return names
}
// branchNameRe is the shape the skills mcp-destructive-guard hook enforces.
var branchNameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`)
// ValidateBranchName rejects branch names that Gitea would turn into a ref
// other than refs/heads/<name> as the caller meant it. `refs/heads/main` in
// particular creates refs/heads/refs/heads/main, after which every API write to
// main fails with "matches more than one" until the ref is deleted by hand
// (#65). Beyond the shape regexp it applies git's own check-ref-format rules
// that the regexp admits.
func ValidateBranchName(b string) error {
bad := !branchNameRe.MatchString(b) ||
strings.HasPrefix(b, "refs/") ||
b == "HEAD" ||
strings.Contains(b, "..") ||
strings.Contains(b, "//") ||
strings.Contains(b, "/.") ||
strings.HasSuffix(b, "/") ||
strings.HasSuffix(b, ".") ||
strings.HasSuffix(b, ".lock")
if bad {
return fmt.Errorf("invalid branch name %q (want a short name like feat/x, no refs/ prefix): %w", b, gitea.ErrValidation)
}
return nil
}
// validateRequiredIdentifiers rejects an empty repo identifier at the tool layer
// before it reaches the client, where it would build a trailing-empty path
// segment (`/api/v1/repos/{owner}/`) and leak gitea's bare 404 (#37). `repo` and
+1 -1
View File
@@ -58,7 +58,7 @@ if [ "$FORCE" = false ]; then
while IFS= read -r _match; do
_lineno="${_match%%:*}"
_content="${_match#*:}"
_token=$(printf '%s' "$_content" | grep -o '{{[^}]*}}' | head -1)
_token=$(head -1 < <(grep -o '{{[^}]*}}' <<<"$_content"))
echo " $PROJECT_FILE:$_lineno: unfilled placeholder $_token" >&2
done <<< "$_placeholders"
echo "" >&2