Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d3faffc4c4 | ||
|
|
dbf78a3b34 | ||
|
|
c3f8d6c002 | ||
|
|
dc127a05b5 | ||
|
|
cd61239451 | ||
|
|
31a3dd3e0e | ||
|
|
ae50501ba3 | ||
|
|
9396b4daaa | ||
|
|
0e2bd3d90e | ||
|
|
b149dccdbe | ||
|
|
724a997002 | ||
|
|
59dabc9455 | ||
|
|
6b57a8ba8a | ||
|
|
e1e9a2e30e |
@@ -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
|
when a human reviewer outside the project is required. Document the reason in
|
||||||
PROJECT.md.
|
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
|
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:
|
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
|
- **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
|
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.
|
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
|
## CAD sprint patterns
|
||||||
|
|
||||||
Two CAD sprint archetypes (defs: brain `wiki/homelab/decisions/continuous-agentic-development-cad-concept-2026-06-16`):
|
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 |
|
| **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 |
|
| **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 |
|
| **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
|
- **Routing**: brain_answer's LLM uses berget.ai as primary, iguana ollama as
|
||||||
fallback. Both are configurable in the `supervisor/ingestion-deployment.yaml`
|
fallback. Both are configurable in the `supervisor/ingestion-deployment.yaml`
|
||||||
on the koala k3s cluster; don't hardcode local-only model names into the
|
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
|
and missing-untracked adapters). A drift fails the check with a
|
||||||
message telling you to stage the regenerated files.
|
message telling you to stage the regenerated files.
|
||||||
|
|
||||||
Behavior rules in this file and per-project rules in `PROJECT.md` apply
|
Behavior rules in this file and per-project rules in `PROJECT.md` apply on every
|
||||||
unconditionally on every host, every harness.
|
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
|
## Engineering Skills
|
||||||
|
|
||||||
|
|||||||
+10
-1
@@ -8,6 +8,12 @@ name: CD
|
|||||||
# (Only applies to branch pushes; tag pushes always trigger.)
|
# (Only applies to branch pushes; tag pushes always trigger.)
|
||||||
paths-ignore:
|
paths-ignore:
|
||||||
- 'docs/**'
|
- '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:
|
env:
|
||||||
IMAGE: gitea-mcp
|
IMAGE: gitea-mcp
|
||||||
@@ -44,6 +50,7 @@ jobs:
|
|||||||
build:
|
build:
|
||||||
name: Build & Import
|
name: Build & Import
|
||||||
needs: check
|
needs: check
|
||||||
|
if: github.event_name == 'push'
|
||||||
runs-on: self-hosted
|
runs-on: self-hosted
|
||||||
outputs:
|
outputs:
|
||||||
image-tag: ${{ steps.meta.outputs.sha-tag }}
|
image-tag: ${{ steps.meta.outputs.sha-tag }}
|
||||||
@@ -52,10 +59,12 @@ jobs:
|
|||||||
|
|
||||||
- name: Derive image tags
|
- name: Derive image tags
|
||||||
id: meta
|
id: meta
|
||||||
|
env:
|
||||||
|
# signals#47: a ref name can carry $(...); pass it via env, not ${{ }}.
|
||||||
|
REF: ${{ github.ref }}
|
||||||
run: |
|
run: |
|
||||||
SHA=$(git rev-parse --short HEAD)
|
SHA=$(git rev-parse --short HEAD)
|
||||||
echo "sha-tag=${SHA}" >> "$GITHUB_OUTPUT"
|
echo "sha-tag=${SHA}" >> "$GITHUB_OUTPUT"
|
||||||
REF="${{ github.ref }}"
|
|
||||||
if [[ "$REF" == refs/tags/v* ]]; then
|
if [[ "$REF" == refs/tags/v* ]]; then
|
||||||
echo "version-tag=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
|
echo "version-tag=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
|
||||||
echo "version=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
|
echo "version=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
|
||||||
|
|||||||
@@ -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
|
when a human reviewer outside the project is required. Document the reason in
|
||||||
PROJECT.md.
|
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
|
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:
|
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
|
- **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
|
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.
|
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
|
## CAD sprint patterns
|
||||||
|
|
||||||
Two CAD sprint archetypes (defs: brain `wiki/homelab/decisions/continuous-agentic-development-cad-concept-2026-06-16`):
|
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 |
|
| **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 |
|
| **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 |
|
| **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
|
- **Routing**: brain_answer's LLM uses berget.ai as primary, iguana ollama as
|
||||||
fallback. Both are configurable in the `supervisor/ingestion-deployment.yaml`
|
fallback. Both are configurable in the `supervisor/ingestion-deployment.yaml`
|
||||||
on the koala k3s cluster; don't hardcode local-only model names into the
|
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
|
and missing-untracked adapters). A drift fails the check with a
|
||||||
message telling you to stage the regenerated files.
|
message telling you to stage the regenerated files.
|
||||||
|
|
||||||
Behavior rules in this file and per-project rules in `PROJECT.md` apply
|
Behavior rules in this file and per-project rules in `PROJECT.md` apply on every
|
||||||
unconditionally on every host, every harness.
|
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
|
## Engineering Skills
|
||||||
|
|
||||||
|
|||||||
@@ -125,6 +125,13 @@ by area:
|
|||||||
| Search / Templates | code_search, create_project_from_template |
|
| Search / Templates | code_search, create_project_from_template |
|
||||||
| Composite | tbd_ship (trunk-based ship helper) |
|
| 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`):
|
Shared tool-layer hygiene (`internal/tools/tool.go`):
|
||||||
- **Alias normalization** — `repo`↔`name` and `number`↔`index` are reconciled so
|
- **Alias normalization** — `repo`↔`name` and `number`↔`index` are reconciled so
|
||||||
a tool works whichever spelling a caller sends (an explicit canonical wins).
|
a tool works whichever spelling a caller sends (an explicit canonical wins).
|
||||||
|
|||||||
@@ -3,7 +3,7 @@ module git.d-ma.be/mathias/gitea-mcp
|
|||||||
go 1.26.2
|
go 1.26.2
|
||||||
|
|
||||||
require (
|
require (
|
||||||
git.d-ma.be/mathias/mcp-chassis v0.6.0
|
git.d-ma.be/mathias/mcp-chassis v0.7.1
|
||||||
github.com/hashicorp/golang-lru/v2 v2.0.7
|
github.com/hashicorp/golang-lru/v2 v2.0.7
|
||||||
github.com/stretchr/testify v1.11.1
|
github.com/stretchr/testify v1.11.1
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -2,6 +2,10 @@ git.d-ma.be/mathias/mcp-chassis v0.5.0 h1:0w3dt4t4r8OtZBHIOoZkR6p6L3L6YDiqhMh9bT
|
|||||||
git.d-ma.be/mathias/mcp-chassis v0.5.0/go.mod h1:Ks7EK2UnGAN0H3rJjKUxUagX8/ZBdtLrOlcUbv0RwH8=
|
git.d-ma.be/mathias/mcp-chassis v0.5.0/go.mod h1:Ks7EK2UnGAN0H3rJjKUxUagX8/ZBdtLrOlcUbv0RwH8=
|
||||||
git.d-ma.be/mathias/mcp-chassis v0.6.0 h1:/MuxT9IyzZbN26+COlI2FK0moTyXMLG/qXc/cm0i0Vg=
|
git.d-ma.be/mathias/mcp-chassis v0.6.0 h1:/MuxT9IyzZbN26+COlI2FK0moTyXMLG/qXc/cm0i0Vg=
|
||||||
git.d-ma.be/mathias/mcp-chassis v0.6.0/go.mod h1:Ks7EK2UnGAN0H3rJjKUxUagX8/ZBdtLrOlcUbv0RwH8=
|
git.d-ma.be/mathias/mcp-chassis v0.6.0/go.mod h1:Ks7EK2UnGAN0H3rJjKUxUagX8/ZBdtLrOlcUbv0RwH8=
|
||||||
|
git.d-ma.be/mathias/mcp-chassis v0.7.0 h1:ct31v1F1PMhMCva/V0ErEbgMzwnT+27lYKbug2A/quE=
|
||||||
|
git.d-ma.be/mathias/mcp-chassis v0.7.0/go.mod h1:Ks7EK2UnGAN0H3rJjKUxUagX8/ZBdtLrOlcUbv0RwH8=
|
||||||
|
git.d-ma.be/mathias/mcp-chassis v0.7.1 h1:4h8TOBHTA0L/no4IPcr4tLcQtmKUlhE2/cXyPdNFGmM=
|
||||||
|
git.d-ma.be/mathias/mcp-chassis v0.7.1/go.mod h1:Ks7EK2UnGAN0H3rJjKUxUagX8/ZBdtLrOlcUbv0RwH8=
|
||||||
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
|
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
|
||||||
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
|
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
|
||||||
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
|
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
|
||||||
|
|||||||
@@ -2,9 +2,12 @@ package auth
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
|
chassisauth "git.d-ma.be/mathias/mcp-chassis/auth"
|
||||||
|
|
||||||
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -25,11 +28,34 @@ func PassthroughMiddleware(validator TokenValidator, onValid, fallback http.Hand
|
|||||||
token, hasBearer := strings.CutPrefix(authz, "Bearer ")
|
token, hasBearer := strings.CutPrefix(authz, "Bearer ")
|
||||||
if hasBearer && token != "" {
|
if hasBearer && token != "" {
|
||||||
if login, ok := validator.ValidateToken(r.Context(), token); ok {
|
if login, ok := validator.ValidateToken(r.Context(), token); ok {
|
||||||
|
// gitea-mcp#60. This middleware logged nothing at all, so there
|
||||||
|
// was no way to tell a PAT pass-through request from one that
|
||||||
|
// fell through to the shared JWT/static path — only inference
|
||||||
|
// from which failures were absent.
|
||||||
|
//
|
||||||
|
// It matters more here than on the other two paths: this is
|
||||||
|
// where a request acts as a SPECIFIC Gitea user. An audit trail
|
||||||
|
// that cannot say which identity performed a write is not an
|
||||||
|
// audit trail.
|
||||||
|
//
|
||||||
|
// chassisauth.Fingerprint, not a local equivalent: the chassis
|
||||||
|
// emits the same field on its own accepted/rejected lines, and
|
||||||
|
// two different fingerprints for one token cannot be correlated.
|
||||||
|
slog.Info("mcp auth accepted",
|
||||||
|
"auth_path", "passthrough",
|
||||||
|
"caller", login,
|
||||||
|
"token_fp", chassisauth.Fingerprint(token),
|
||||||
|
"path", r.URL.Path,
|
||||||
|
)
|
||||||
ctx := withCaller(r.Context(), login)
|
ctx := withCaller(r.Context(), login)
|
||||||
ctx = gitea.WithToken(ctx, token)
|
ctx = gitea.WithToken(ctx, token)
|
||||||
onValid.ServeHTTP(w, r.WithContext(ctx))
|
onValid.ServeHTTP(w, r.WithContext(ctx))
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
// A token that is not a valid PAT falls through SILENTLY. The
|
||||||
|
// chassis owns the verdict; logging here too would double-count
|
||||||
|
// every static-token request, since all of them reach this
|
||||||
|
// middleware first.
|
||||||
}
|
}
|
||||||
fallback.ServeHTTP(w, r)
|
fallback.ServeHTTP(w, r)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -0,0 +1,83 @@
|
|||||||
|
package auth
|
||||||
|
|
||||||
|
import (
|
||||||
|
"bytes"
|
||||||
|
"context"
|
||||||
|
"encoding/json"
|
||||||
|
"log/slog"
|
||||||
|
"net/http"
|
||||||
|
"net/http/httptest"
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
type stubValidator struct{ login string }
|
||||||
|
|
||||||
|
func (s stubValidator) ValidateToken(context.Context, string) (string, bool) {
|
||||||
|
return s.login, s.login != ""
|
||||||
|
}
|
||||||
|
|
||||||
|
// gitea-mcp#60: PassthroughMiddleware logged nothing at all, success or failure.
|
||||||
|
//
|
||||||
|
// While verifying #59 end to end there was no way to confirm from the server
|
||||||
|
// logs whether calls were using the new PAT pass-through (caller's own Gitea
|
||||||
|
// PAT, own identity) or falling through to the shared JWT/static path. The only
|
||||||
|
// way to reason about it was inference. The chassis names its two paths as of
|
||||||
|
// v0.7.0; this is the third.
|
||||||
|
//
|
||||||
|
// This matters more than the other two: pass-through is the path where the
|
||||||
|
// request acts as a SPECIFIC Gitea user. An audit trail that cannot say which
|
||||||
|
// identity performed a write is not an audit trail.
|
||||||
|
func TestPassthroughMiddleware_LogsTheAcceptedPath(t *testing.T) {
|
||||||
|
var buf bytes.Buffer
|
||||||
|
prev := slog.Default()
|
||||||
|
slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo})))
|
||||||
|
t.Cleanup(func() { slog.SetDefault(prev) })
|
||||||
|
|
||||||
|
reached := false
|
||||||
|
h := PassthroughMiddleware(stubValidator{login: "alice"},
|
||||||
|
http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true }),
|
||||||
|
http.HandlerFunc(func(http.ResponseWriter, *http.Request) { t.Fatal("must not fall back") }))
|
||||||
|
|
||||||
|
req := httptest.NewRequest(http.MethodGet, "/mcp", nil)
|
||||||
|
req.Header.Set("Authorization", "Bearer a-real-gitea-pat")
|
||||||
|
h.ServeHTTP(httptest.NewRecorder(), req)
|
||||||
|
require.True(t, reached)
|
||||||
|
|
||||||
|
var line map[string]any
|
||||||
|
require.NoError(t, json.Unmarshal([]byte(strings.TrimSpace(buf.String())), &line))
|
||||||
|
require.Equal(t, "mcp auth accepted", line["msg"])
|
||||||
|
require.Equal(t, "passthrough", line["auth_path"])
|
||||||
|
require.Equal(t, "alice", line["caller"])
|
||||||
|
require.NotEmpty(t, line["token_fp"], "correlatable without being a credential")
|
||||||
|
|
||||||
|
for k, v := range line {
|
||||||
|
require.NotContains(t, []any{v}, "a-real-gitea-pat",
|
||||||
|
"the PAT must never appear in any field, including %q", k)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// A token that is not a valid PAT must fall through silently to the chassis,
|
||||||
|
// which owns the rejection log. Logging here as well would double-count every
|
||||||
|
// static-token request — every one of them reaches this middleware first.
|
||||||
|
func TestPassthroughMiddleware_FallsThroughWithoutLogging(t *testing.T) {
|
||||||
|
var buf bytes.Buffer
|
||||||
|
prev := slog.Default()
|
||||||
|
slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo})))
|
||||||
|
t.Cleanup(func() { slog.SetDefault(prev) })
|
||||||
|
|
||||||
|
fellBack := false
|
||||||
|
h := PassthroughMiddleware(stubValidator{},
|
||||||
|
http.HandlerFunc(func(http.ResponseWriter, *http.Request) { t.Fatal("must not accept") }),
|
||||||
|
http.HandlerFunc(func(http.ResponseWriter, *http.Request) { fellBack = true }))
|
||||||
|
|
||||||
|
req := httptest.NewRequest(http.MethodGet, "/mcp", nil)
|
||||||
|
req.Header.Set("Authorization", "Bearer not-a-pat")
|
||||||
|
h.ServeHTTP(httptest.NewRecorder(), req)
|
||||||
|
|
||||||
|
require.True(t, fellBack)
|
||||||
|
require.Empty(t, strings.TrimSpace(buf.String()),
|
||||||
|
"the chassis owns the verdict; logging here too would double-count every static-token call")
|
||||||
|
}
|
||||||
@@ -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...)
|
||||||
|
}
|
||||||
@@ -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"])
|
||||||
|
}
|
||||||
@@ -4,6 +4,7 @@ import (
|
|||||||
"encoding/json"
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"time"
|
||||||
|
|
||||||
"git.d-ma.be/mathias/gitea-mcp/internal/registry"
|
"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))
|
NewErrorResponse(req.ID, -32602, "invalid params", nil))
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
start := time.Now()
|
||||||
out, err := s.opts.Registry.Dispatch(r.Context(), p.Name, p.Arguments)
|
out, err := s.opts.Registry.Dispatch(r.Context(), p.Name, p.Arguments)
|
||||||
|
logToolCall(r.Context(), p.Name, p.Arguments, start, err)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
code := -32000
|
code := -32000
|
||||||
if errors.Is(err, registry.ErrToolNotFound) {
|
if errors.Is(err, registry.ErrToolNotFound) {
|
||||||
|
|||||||
@@ -63,6 +63,9 @@ func (t *FileWriteBranch) Call(ctx context.Context, raw json.RawMessage) (json.R
|
|||||||
if args.Branch == "" {
|
if args.Branch == "" {
|
||||||
return nil, fmt.Errorf("branch is required: %w", gitea.ErrValidation)
|
return nil, fmt.Errorf("branch is required: %w", gitea.ErrValidation)
|
||||||
}
|
}
|
||||||
|
if err := ValidateBranchName(args.Branch); err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
if args.Message == "" {
|
if args.Message == "" {
|
||||||
return nil, fmt.Errorf("message is required: %w", gitea.ErrValidation)
|
return nil, fmt.Errorf("message is required: %w", gitea.ErrValidation)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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
@@ -37,7 +37,7 @@ const (
|
|||||||
func (t *TBDShip) Descriptor() registry.ToolDescriptor {
|
func (t *TBDShip) Descriptor() registry.ToolDescriptor {
|
||||||
return registry.ToolDescriptor{
|
return registry.ToolDescriptor{
|
||||||
Name: "tbd_ship",
|
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(`{
|
InputSchema: json.RawMessage(`{
|
||||||
"type":"object",
|
"type":"object",
|
||||||
"properties":{
|
"properties":{
|
||||||
@@ -50,6 +50,7 @@ func (t *TBDShip) Descriptor() registry.ToolDescriptor {
|
|||||||
"branch":{"type":"string","description":"Short-lived branch name. Default derived: tbd/<slug>-<hash>."},
|
"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_title":{"type":"string","description":"PR title. Default: the commit message."},
|
||||||
"pr_body":{"type":"string"},
|
"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."},
|
"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)."}
|
"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"`
|
Branch string `json:"branch"`
|
||||||
PRTitle string `json:"pr_title"`
|
PRTitle string `json:"pr_title"`
|
||||||
PRBody string `json:"pr_body"`
|
PRBody string `json:"pr_body"`
|
||||||
|
Issue int `json:"issue"`
|
||||||
DeleteBranch *bool `json:"delete_branch"`
|
DeleteBranch *bool `json:"delete_branch"`
|
||||||
CITimeoutSec int `json:"ci_timeout_seconds"`
|
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.
|
// shipGate is the decision produced by evaluateShipGate.
|
||||||
type shipGate struct {
|
type shipGate struct {
|
||||||
merge bool
|
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
|
// 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
|
// only when CI is fully green AND the base branch is not review-protected AND the
|
||||||
// other state fails closed to PR-only with an explanatory reason (#40).
|
// change is classified risk/low. Every other state fails closed to PR-only with an
|
||||||
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection) shipGate {
|
// explanatory reason (#40, gitea-mcp#63).
|
||||||
|
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk riskTier) shipGate {
|
||||||
ci := classifyCI(runs)
|
ci := classifyCI(runs)
|
||||||
|
|
||||||
// Review-protected base can't be auto-merged regardless of CI.
|
// 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)}
|
"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 {
|
switch ci {
|
||||||
case "success":
|
case "success":
|
||||||
return shipGate{true, ci, ""}
|
return shipGate{true, ci, ""}
|
||||||
@@ -160,6 +233,13 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
if branch == "" {
|
if branch == "" {
|
||||||
branch = deriveBranch(args.Message, args.Path, args.Content)
|
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
|
deleteBranch := true
|
||||||
if args.DeleteBranch != nil {
|
if args.DeleteBranch != nil {
|
||||||
deleteBranch = *args.DeleteBranch
|
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.
|
// CI gate on the PR head commit.
|
||||||
runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
|
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{
|
result := map[string]any{
|
||||||
"merged": false,
|
"merged": false,
|
||||||
@@ -231,6 +312,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
"pr_url": pr.HTMLURL,
|
"pr_url": pr.HTMLURL,
|
||||||
"branch": branch,
|
"branch": branch,
|
||||||
"ci_status": gate.ciStatus,
|
"ci_status": gate.ciStatus,
|
||||||
|
"risk": string(risk),
|
||||||
}
|
}
|
||||||
if !gate.merge {
|
if !gate.merge {
|
||||||
result["reason"] = gate.reason
|
result["reason"] = gate.reason
|
||||||
@@ -254,6 +336,20 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
return textOK(result)
|
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
|
// 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
|
// them to reach a terminal state. timeoutSec == 0 is a single snapshot (no
|
||||||
// sleep). A listing error yields no runs → the gate fails closed.
|
// sleep). A listing error yields no runs → the gate fails closed.
|
||||||
|
|||||||
@@ -20,22 +20,31 @@ func TestEvaluateShipGate(t *testing.T) {
|
|||||||
name string
|
name string
|
||||||
runs []gitea.WorkflowRun
|
runs []gitea.WorkflowRun
|
||||||
bp *gitea.BranchProtection
|
bp *gitea.BranchProtection
|
||||||
|
risk riskTier
|
||||||
wantMerge bool
|
wantMerge bool
|
||||||
wantCI string
|
wantCI string
|
||||||
}{
|
}{
|
||||||
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, true, "success"},
|
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskLow, true, "success"},
|
||||||
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, false, "pending"},
|
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, riskLow, false, "pending"},
|
||||||
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, false, "pending"},
|
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, riskLow, false, "pending"},
|
||||||
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, false, "failed"},
|
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, riskLow, false, "failed"},
|
||||||
{"cancelled → no merge (fail-closed)", []gitea.WorkflowRun{run("completed", "cancelled")}, open, 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, false, "none"},
|
{"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, false, "pending"},
|
{"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, false, "success"},
|
{"green but protected → no merge", []gitea.WorkflowRun{run("completed", "success")}, protected, riskLow, false, "success"},
|
||||||
{"no runs + protected → no merge", nil, protected, false, "none"},
|
{"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 {
|
for _, tc := range tests {
|
||||||
t.Run(tc.name, func(t *testing.T) {
|
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 {
|
if g.merge != tc.wantMerge {
|
||||||
t.Errorf("merge = %v, want %v (reason: %q)", g.merge, tc.wantMerge, g.reason)
|
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)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -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
|
// shipFakeOpts adds protection (requiredApprovals>0 → protected) and a merge
|
||||||
// status code, so tests can drive the review-protected and conflict paths.
|
// 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 {
|
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()
|
t.Helper()
|
||||||
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
p := r.URL.Path
|
p := r.URL.Path
|
||||||
w.Header().Set("Content-Type", "application/json")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
switch {
|
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/"):
|
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
|
||||||
if requiredApprovals > 0 {
|
if requiredApprovals > 0 {
|
||||||
_, _ = w.Write([]byte(`{"required_approvals":` + itoa(requiredApprovals) + `}`))
|
_, _ = 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)
|
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, true, res["merged"])
|
||||||
assert.Equal(t, "success", res["ci_status"])
|
assert.Equal(t, "success", res["ci_status"])
|
||||||
@@ -98,7 +108,7 @@ func TestTBDShip_NoCI_FailsClosed(t *testing.T) {
|
|||||||
srv := shipFake(t, `[]`, &merged, &deleted)
|
srv := shipFake(t, `[]`, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.Equal(t, "none", res["ci_status"])
|
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)
|
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.Equal(t, "failed", res["ci_status"])
|
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)
|
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 1, http.StatusOK, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.NotEmpty(t, res["reason"])
|
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)
|
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 0, http.StatusConflict, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.NotEmpty(t, res["reason"])
|
assert.NotEmpty(t, res["reason"])
|
||||||
@@ -160,6 +170,8 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
|
|||||||
p := r.URL.Path
|
p := r.URL.Path
|
||||||
w.Header().Set("Content-Type", "application/json")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
switch {
|
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/"):
|
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
|
||||||
w.WriteHeader(http.StatusNotFound)
|
w.WriteHeader(http.StatusNotFound)
|
||||||
_, _ = w.Write([]byte(`{"message":"not found"}`))
|
_, _ = w.Write([]byte(`{"message":"not found"}`))
|
||||||
@@ -196,7 +208,7 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
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, true, res["merged"], "resume must merge when CI is green")
|
||||||
assert.Equal(t, float64(7), res["pr_number"], "must reuse the existing PR #7")
|
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())
|
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) {
|
func TestTBDShip_AllowlistRejects(t *testing.T) {
|
||||||
tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
|
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"}`))
|
_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))
|
||||||
|
|||||||
+148
-2
@@ -1,10 +1,14 @@
|
|||||||
package tools
|
package tools
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"bytes"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"reflect"
|
"reflect"
|
||||||
|
"regexp"
|
||||||
"strings"
|
"strings"
|
||||||
|
"unicode"
|
||||||
|
|
||||||
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
||||||
"git.d-ma.be/mathias/gitea-mcp/internal/registry"
|
"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 {
|
if err := json.Unmarshal([]byte("{}"), dst); err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
} else if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
|
} else {
|
||||||
return err
|
// 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)
|
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
|
// validateRequiredIdentifiers rejects an empty repo identifier at the tool layer
|
||||||
// before it reaches the client, where it would build a trailing-empty path
|
// 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
|
// segment (`/api/v1/repos/{owner}/`) and leak gitea's bare 404 (#37). `repo` and
|
||||||
|
|||||||
@@ -58,7 +58,7 @@ if [ "$FORCE" = false ]; then
|
|||||||
while IFS= read -r _match; do
|
while IFS= read -r _match; do
|
||||||
_lineno="${_match%%:*}"
|
_lineno="${_match%%:*}"
|
||||||
_content="${_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
|
echo " $PROJECT_FILE:$_lineno: unfilled placeholder $_token" >&2
|
||||||
done <<< "$_placeholders"
|
done <<< "$_placeholders"
|
||||||
echo "" >&2
|
echo "" >&2
|
||||||
|
|||||||
Reference in New Issue
Block a user