Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
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
|
||||
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`):
|
||||
@@ -319,8 +377,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
|
||||
|
||||
|
||||
@@ -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`):
|
||||
@@ -314,8 +372,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
|
||||
|
||||
|
||||
@@ -3,7 +3,7 @@ module git.d-ma.be/mathias/gitea-mcp
|
||||
go 1.26.2
|
||||
|
||||
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/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.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.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.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
|
||||
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
|
||||
|
||||
@@ -2,9 +2,12 @@ package auth
|
||||
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"strings"
|
||||
|
||||
chassisauth "git.d-ma.be/mathias/mcp-chassis/auth"
|
||||
|
||||
"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 ")
|
||||
if hasBearer && token != "" {
|
||||
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 = gitea.WithToken(ctx, token)
|
||||
onValid.ServeHTTP(w, r.WithContext(ctx))
|
||||
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)
|
||||
})
|
||||
|
||||
@@ -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", "number", "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,123 @@
|
||||
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")
|
||||
}
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user