diff --git a/.taskmaster/tasks/tasks.json b/.taskmaster/tasks/tasks.json index d525bf0..eab3dd9 100644 --- a/.taskmaster/tasks/tasks.json +++ b/.taskmaster/tasks/tasks.json @@ -3485,6 +3485,17 @@ "status": "pending", "subtasks": [], "updatedAt": "2026-08-15T22:54:36.185Z" + }, + { + "id": 180, + "title": "CLI for the Epic write path (status, criteria, tradeoffs)", + "description": "Marking a done-criterion met, setting Epic status and recording tradeoffs are POST /api/v1/epics/:id/{criteria,status,tradeoffs}. describe epic READS all of it — completeness bar, unmet criteria, reconciliation — but the CLI cannot WRITE any of it, so completeness is permanently 0% for anyone not driving the API by hand. That makes the Epic layer read-only in practice and the reconciliation feature unusable, since it exists to contrast claimed vs delivered and nothing can ever claim.", + "details": "Add an `epics` command group mirroring `tasks`:\n mcpctl epics criterion --met|--unmet [--note]\n mcpctl epics status [--note]\n mcpctl epics tradeoff --criterion --why \nRoutes already exist and are RBAC-mapped to edit:epics (url-permission.ts). Closing an Epic with unmet criteria must keep going through the tradeoffs path — the CLI must not offer a way to mark it delivered while criteria are unmet.\nPer the project rule, these are project-scoped commands: keep them out of the root --help (see task 174).\nReferenced from docs/epics.md as \"task 180\" — update that note when this lands.", + "testStrategy": "Unit: criterion --met flips only the named id and leaves others untouched; status delivered with unmet criteria is refused and names them; tradeoff records the reason. Verify describe epic completeness moves off 0% after a criterion is marked, and that reconciliation then has something to contrast.", + "status": "pending", + "priority": "medium", + "dependencies": [], + "subtasks": [] } ], "metadata": { @@ -3497,4 +3508,4 @@ ] } } -} +} \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index c7fd973..aaea9f3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,9 +35,18 @@ Key routing rules: - `project` — workspace grouping servers, prompts, agents - `llm` — server-managed LLM provider (api key + endpoint) - `agent` — LLM persona pinned to one Llm; project attach surfaces project Prompts as system context, project MCP servers as tools, and exposes the agent itself as an MCP virtual server (`agent-/chat`). See `docs/agents.md`, `docs/chat.md`. + Tool access is bounded by **whoever invoked the run**, not by the agent's + memberships — knowledge travels, action is gated. See `docs/agent-authority.md`. - `prompt` / `promptrequest` — curated content / legacy pending proposal (use `proposal` for new work). - `skill` — Claude Code skill bundle (SKILL.md + files + typed metadata). Materialised onto disk by `mcpctl skills sync`. See `docs/skills.md`. - `proposal` — generic pending proposal queue, replaces `promptrequest`. Covers both prompts and skills. See `docs/proposals.md`. Triage via `mcpctl review`. - `revision` — append-only audit + diff log shared by prompts and skills. Auto-bumps semver on save. See `docs/revisions.md`. +- `epic` / `epictask` / `task` — the work tracker: vision → roadmap node → work item. + Tasks carry a risk level that decides which review gates they must pass, and + accepting one **merges its PR**. See `docs/epics.md`. + **`tasks` = work items**; the inference queue is `inference-tasks` / `itasks`. +- `gitprovider` / `repo` — git forge connection + attached repositories. Discovery + and import via `mcpctl repos discover|import`; a forge connection is what makes + merge-on-accept possible. See `docs/gitproviders.md`. - `rbac` — access control bindings - `mcptoken` — bearer credentials for HTTP-mode mcplocal diff --git a/README.md b/README.md index e12537e..1ccf887 100644 --- a/README.md +++ b/README.md @@ -500,6 +500,17 @@ new FileCache('ns', { maxSize: '10%' }) // 10% of partition | **promptrequest** | Pending prompt proposal | LLM-submitted, needs approval | | **rbac** | Access control bindings | Who can do what | | **serverattachment** | Server-to-project link | Virtual resource for `apply` | +| **epic** | The vision work is measured against | Done-criteria, completeness, tradeoffs | +| **epictask** | A roadmap node under an Epic | Owns a subset of the Epic's criteria | +| **task** | A work item with a review lifecycle | `todo → … → awaiting_review → merged` | +| **gitprovider** | A connection to a git forge | Gitea instance + API token | +| **repo** | A repository attached to a project | Imported from a forge, or added by URL | + +> **`tasks` changed meaning.** It used to list the durable inference queue, +> which now answers only to `inference-tasks` / `itasks`. Bare `tasks` lists +> work items. Existing RBAC bindings are rewritten once at startup, so an old +> `view:tasks` grant keeps authorizing the queue and does not silently become +> authority over work items. See [docs/inference-tasks.md](docs/inference-tasks.md). ## Agents diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index 131a4b6..4aaae54 100644 --- a/completions/mcpctl.bash +++ b/completions/mcpctl.bash @@ -224,7 +224,7 @@ _mcpctl() { else case "$create_sub" in gitprovider) - COMPREPLY=($(compgen -W "--url --type --description --secret --key --skip-probe -h --help" -- "$cur")) + COMPREPLY=($(compgen -W "--url --type --description --secret --key --webhook-secret-key --skip-probe -h --help" -- "$cur")) ;; epic) COMPREPLY=($(compgen -W "--project --content --content-file --description --criterion -h --help" -- "$cur")) diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index 30a055e..9876f3d 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -420,6 +420,7 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create gitprovider" -l type -d 'Dr complete -c mcpctl -n "__mcpctl_subcmd_active create gitprovider" -l description -d 'Description' -x complete -c mcpctl -n "__mcpctl_subcmd_active create gitprovider" -l secret -d 'Secret holding the API token' -x complete -c mcpctl -n "__mcpctl_subcmd_active create gitprovider" -l key -d 'Key within that secret' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create gitprovider" -l webhook-secret-key -d 'Key in that secret holding the webhook signing secret; without it incoming webhooks are refused' -x complete -c mcpctl -n "__mcpctl_subcmd_active create gitprovider" -l skip-probe -d 'Skip the credential check at registration time' # create epic options diff --git a/docs/agent-authority.md b/docs/agent-authority.md new file mode 100644 index 0000000..64b42c3 --- /dev/null +++ b/docs/agent-authority.md @@ -0,0 +1,118 @@ +# Agent run authority + +An agent carries two different things from the projects it joins: + +- **knowledge** — prompts, skills, project context +- **capability** — the projects' MCP tools + +Treating those as one thing is what makes agent membership dangerous. `sre-bot` +belongs to `payments` so it understands payments; that should not mean everyone +who can talk to `sre-bot` can call payments' tools through it. + +The rule: + +> **Knowledge travels with the agent. Action is bounded by the authority of the +> run.** + +Prompts load regardless of who is asking — that is what makes an agent a QA +reviewer rather than a generic bot. Every MCP tool call is gated. + +## Authority is per-run, not per-agent + +The same agent runs under different authority depending on how it was invoked. +One `sre-bot`, three modes: + +| Mode | Who decides what it may do | Effective tool scope | Status | +|---|---|---|---| +| **caller** | the human who invoked it | agent's memberships **∩** caller's `run:projects` | **implemented** | +| **agent** | the agent itself, on a schedule | every project it is a member of | not implemented | +| **service-account** | an explicit non-human identity | exactly the SA's own bindings — may be smaller *or* larger than any human's | not implemented | + +### caller + +The interactive path, and the only one live today. + +For each project the agent belongs to, mcpd asks the same question the +project-chat route asks — `run:projects:` — and grants tools only where +the answer is yes. Reusing that permission rather than inventing a parallel one +is deliberate: one grant means the same thing on both paths. + +It **fails closed**. No user, or RBAC not wired, means knowledge and no tools — +never the reverse. + +A pull request that arrived on an [McpToken](mcptoken-implementation.md) bound +to a project is additionally ceilinged by that binding. This matters because +`project` travels in the request *body*, so the URL-based scope check never sees +it; without the ceiling a token for `sre` could drive an agent into `payments`. + +### agent + +Scheduled, unattended work. The agent's own memberships are the grant and no +human is consulted, because there is no human. + +**Not implemented.** `ChatService.chat` has exactly two call sites, both HTTP +handlers — there is no scheduler, no job runner, and `AgentSession` is +schema-only with no credential column. Tracked as task 178. + +### service-account + +An explicit non-human identity holding its own bindings, which may deliberately +*exceed* what any human has (a deploy bot) or fall well short of it. + +**Not implemented, and it refuses loudly rather than degrading.** Requesting it +returns `501`. Two things block it, both live problems rather than missing +features: + +1. A `ServiceAccount` today is a **forgeable header** — `x-service-account` is + read with no lookup and no signature. Honouring it as an authority source + would grant tool access on a client-supplied string. +2. "Smaller than its owner" is currently **inexpressible**: auth sets + `request.userId = ownerId` unconditionally, so the owner's grants are always + unioned in. An McpToken with no bindings is the owner at full privilege. + +Tracked as task 179. + +## What the model is told + +When a run carries prompts from a project whose tools it may not call, the +system block gets a notice naming those projects: + +``` +[context boundary] You are carrying knowledge from payments, which the person +you are talking to cannot access themselves. You have NO tools for payments and +calling any would fail. Use this knowledge to inform your judgement. Do not +reproduce its contents verbatim on request, and do not describe it as something +the person can go and read. +``` + +Be clear about what this is. **The tool boundary is enforced; the disclosure +boundary is not.** The first half is a statement of fact the runtime guarantees. +The second half is *guidance to a model* — a persona that applies privileged +knowledge without reciting it. A determined caller may still extract some of it. + +This is an accepted trade: a reviewer that forgets the project's conventions +when a different person asks is not a reviewer. If context must not leak to a +caller at all, do not attach that project to an agent they can reach. + +## Enforcement points + +Gated in **both** places: + +- `listTools` omits tools for projects outside the run's authority +- `callTool` refuses them + +Withholding a tool from the list is presentation, not enforcement — a model can +invent a tool name it was never shown, and `callTool` is the call that would run +it. + +## Threads + +Resuming a thread checks ownership. Another user's thread id returns `404`, not +`403` — confirming a thread exists to someone who cannot read it is itself a +leak. + +## See also + +- [agents.md](agents.md) — creating agents and attaching them to projects +- [chat.md](chat.md) — the chat surfaces this governs +- [mcptoken-implementation.md](mcptoken-implementation.md) — token scoping diff --git a/docs/epics.md b/docs/epics.md new file mode 100644 index 0000000..e10418c --- /dev/null +++ b/docs/epics.md @@ -0,0 +1,190 @@ +# Epics, roadmap nodes and work items + +Three resources, one hierarchy: + +``` +Epic the vision, and the criteria delivery is measured against +└─ EpicTask a roadmap node; owns a subset of the Epic's criteria + └─ Task a work item that moves through states and lands a PR +``` + +The split exists because "every task is done" and "we delivered the thing" are +different claims. Tasks close; criteria are what say whether closing them +amounted to anything. Nothing derives one from the other automatically — see +[Reconciliation](#reconciliation). + +## Epic + +The versioned statement of intent. `content` is the vision prose; `doneCriteria` +is the list the work is graded against. + +A criterion is `id=text`, and prefixing the id with `q:` marks it a **quality** +criterion rather than an outcome one. Both count toward completeness — grading +only the mechanical half is how you end up with every task done and no car. + +```sh +mcpctl create epic checkout-rewrite \ + --project payments \ + --content-file vision.md \ + --criterion 'cart=A cart survives a session drop' \ + --criterion 'q:latency=p99 checkout under 400ms' + +mcpctl get epics +mcpctl describe epic checkout-rewrite # vision, criteria, completeness, reconciliation, roadmap +``` + +`status` is `open`, `delivered`, `delivered_with_tradeoffs` or `cancelled`. +Closing an Epic with unmet criteria requires the tradeoffs path — you may ship +something short of the vision, but not while calling it whole. + +> **API only, no CLI yet.** Marking a criterion `met`, setting status and +> recording tradeoffs are `POST /api/v1/epics/:id/{criteria,status,tradeoffs}`. +> The CLI reads all of it (`describe epic`) but cannot write it. Tracked as +> task 180. + +## EpicTask + +A roadmap node. Its `--criterion` ids must match ids on the parent Epic: the +node is claiming *"these parts of the vision are mine"*. + +```sh +mcpctl create epictask cart-persistence \ + --epic checkout-rewrite \ + --criterion 'cart=A cart survives a session drop' \ + --depends-on session-store + +mcpctl get epictasks +mcpctl describe epictask cart-persistence +``` + +`--depends-on` builds the dependency graph `describe epic` renders as a roadmap. + +## Task + +The unit an agent actually works on. It carries a `risk` level, which decides +how much review it must survive, and optionally a repo and a pull request. + +```sh +mcpctl create task cart-cookie-ttl \ + --epictask cart-persistence \ + --title 'Extend cart cookie TTL to 30 days' \ + --risk normal \ + --repo + +mcpctl get tasks +mcpctl describe task cart-cookie-ttl # state, legal moves, verdicts, git, history +``` + +> **`tasks` means work items.** It used to mean the durable inference queue, +> which now answers only to `inference-tasks` / `itasks`. See +> [inference-tasks.md](inference-tasks.md). + +### States + +``` +todo → in_progress → building → self_testing → qa_review + ↓ ↓ + done ← security_review → sre_review + ↓ + awaiting_review ⇄ (reject → in_progress) + ↓ accept + merged → reopened → in_progress +``` + +Every state can go back to `in_progress` and out to `cancelled`; a failed build +goes *back* to work rather than forward. `merged` has exactly one exit, +`reopened`, and only CI drives it (see [gitproviders.md](gitproviders.md#post-merge-failure)). + +```sh +mcpctl tasks advance cart-cookie-ttl building --note 'kicked off CI' +mcpctl tasks history cart-cookie-ttl +``` + +Transitions are compare-and-swap: two agents racing the same task, one wins and +the other gets a 409 naming the state it lost to. + +### Risk decides the gates + +| Risk | Must pass before `done` | +|---|---| +| `trivial` | nothing — docs, formatting | +| `normal` | `qa_review` | +| `high` | `qa_review`, `security_review`, `sre_review` | + +Verification effort scales with risk instead of running one fixed pipeline over +everything. A verdict is pinned to the commit it judged, so a later push +invalidates it rather than silently carrying forward: + +```sh +mcpctl tasks verdict cart-cookie-ttl \ + --stage qa_review --commit 4f2a91c --pass \ + --summary 'covers the session-drop path; added a regression test' +``` + +Reaching `done` without the required passing verdicts is refused, and the error +names which stage is missing. + +### The human gate + +`done` means it left the developer's loop, not that it shipped. `awaiting_review` +is where a human decides: + +```sh +mcpctl tasks accept cart-cookie-ttl --note 'ships with the friday release' +mcpctl tasks reject cart-cookie-ttl --note 'cookie is set before consent — move it after' +``` + +**Accept lands the code.** If the task has a PR, accept merges it through the +repo's gitprovider and records the merge commit. If the merge fails — conflict, +or someone pushed after the review — the task stays un-merged and says why. A +stale head is refused rather than merged, because accepting a commit nobody +reviewed is the one thing the gate exists to prevent. + +Reject requires its comment. That comment is what the agent works from, so an +empty rejection is a bug report with the contents removed. + +## Reconciliation + +`describe epic` prints claimed completeness and, directly under it, what the +delivered work actually says. Those two disagree by design: + +- **Completeness** counts `met` flags. Nothing flips them automatically, so an + Epic whose every Task is merged still reads 0% until someone asserts the + vision was met. +- **Reconciliation** walks the roadmap and reports which criteria have merged + work behind them, which are claimed but undelivered, and which no roadmap node + owns at all (`unowned`). + +Attribution is by criterion id, which is a *convention* — nothing enforces that +an EpicTask's criteria ids exist on the parent. Reconciliation reports where the +convention doesn't hold rather than guessing an owner. It never writes `met`: +deriving it from task state would assert that finishing the tasks delivered the +vision, which is exactly the assumption under audit. + +## RBAC + +| Resource | Verbs | +|---|---| +| `epics` | `view`, `create`, `edit`, `delete` — `edit` also covers status/criteria/tradeoffs | +| `epictasks` | `view`, `create`, `edit`, `delete` — `edit` also covers dependency edges | +| `tasks` | `view`, `create`, `edit`, `delete`, **`run`** | + +`run:tasks` is the gated one: advancing state, recording verdicts, accepting and +rejecting are `run`, not `create`. A reviewer who may drive tasks through the +board should not need permission to invent them, and — more to the point — +anyone who can *file* a task should not thereby be able to *accept* one. + +```sh +mcpctl create rbac task-reviewer --role run --resource tasks +mcpctl create rbac epic-reader --role view --resource epics +``` + +Bindings written before the `tasks` / `inference-tasks` split are rewritten once +at startup, so an old `view:tasks` grant keeps authorizing the inference queue +and does not silently become authority over work items. + +## See also + +- [gitproviders.md](gitproviders.md) — forges, repo import, merge and post-merge reopen +- [agent-authority.md](agent-authority.md) — whose authority an agent acts under +- [inference-tasks.md](inference-tasks.md) — the durable inference queue that used to own this name diff --git a/docs/gitproviders.md b/docs/gitproviders.md new file mode 100644 index 0000000..dc8a6a5 --- /dev/null +++ b/docs/gitproviders.md @@ -0,0 +1,149 @@ +# Git providers and repositories + +Two resources: + +- **`gitprovider`** — a connection to a git forge (Gitea today). One per + instance/account. Owns the credential. +- **`repo`** (CodeRepo) — a repository attached to a project. Either imported + from a provider or added by URL. + +Registering a provider is what makes discovery, import and **merge-on-accept** +possible. A repo added by bare URL can be worked in, but nothing can land its +pull requests, because there is no API to land them through. + +## Registering a forge + +The token lives in a Secret; the provider references it by name and key. + +```sh +mcpctl create secret gitea-token --data TOKEN= + +mcpctl create gitprovider mysources \ + --url https://mysources.co.uk \ + --secret gitea-token --key TOKEN +``` + +Registration **probes the credential** — a wrong URL or an under-scoped token +fails here with a clear message, rather than later at discovery where it looks +like "the forge has no repos". Use `--skip-probe` to wire config before the +infrastructure exists. + +The token needs repository scope. Discovery deliberately calls `/repos/search` +rather than `/user/repos`, because the latter requires `read:user`, which the +repo-scoped tokens people actually create for automation do not carry. + +## Discover and import + +```sh +mcpctl repos discover mysources # everything visible +mcpctl repos discover mysources -q mcp --limit 20 # filter +mcpctl repos discover mysources --project mcpctl # mark what's already imported + +mcpctl repos import mysources michal/mcpctl michal/lab --project mcpctl +mcpctl repos import mysources michal/mcpctl --project mcpctl --ssh +``` + +Discovery stores nothing — it is a live read. Import creates CodeRepo rows. + +Import is partial by design: it takes what it can and reports what it skipped +with reasons, rather than failing the batch because one repo is empty or +archived. An empty repo is flagged unavailable — there is nothing to clone. + +Imported repos carry **denormalized provenance** (`sourceProvider`, +`sourceFullName`) and store no credential of their own — they inherit the +forge's. Deleting a provider leaves its repos in place with their provenance +intact, so where a repo came from outlives the connection. + +To attach a repo without a provider: + +```sh +mcpctl create repo mcpctl --project mcpctl \ + --url https://mysources.co.uk/michal/mcpctl.git --branch main +``` + +```sh +mcpctl get repos +mcpctl get gitproviders +mcpctl describe repo mcpctl +``` + +## Merge on accept + +When a task carrying a PR is accepted, mcpctl merges it through the repo's +provider — see [epics.md](epics.md#the-human-gate). Two refusals matter: + +- **Stale head.** The accept records which commit was reviewed. If someone + pushed after that, the merge is refused rather than landing code nobody + approved. Re-review the current head and accept again. +- **Conflict.** Reported as-is; the task stays un-merged. + +A failure always leaves the task un-merged and says why. Nothing reports success +the board cannot back up. + +Repos with no `gitProviderId` cannot be merged through — the accept says so and +tells you to import from a provider or merge in the forge. + +## Post-merge failure + +CI can fail *after* a merge lands. Without a way to hear about it, the board +keeps claiming `merged` for work that broke the build. The webhook closes that +loop by moving the task to `reopened`. + +Add the signing secret to the provider's Secret and point the provider at it: + +```sh +mcpctl create secret gitea-token \ + --data TOKEN= --data WEBHOOK= + +mcpctl create gitprovider mysources \ + --url https://mysources.co.uk \ + --secret gitea-token --key TOKEN \ + --webhook-secret-key WEBHOOK +``` + +Then in the forge, add a webhook on **commit status** events: + +``` +URL: https:///api/v1/webhooks/git/mysources +Content: application/json +Secret: +``` + +> **It fails closed.** The endpoint is unauthenticated by nature — a forge calls +> it — so it accepts nothing unless the provider has a signing key configured +> *and* the signature verifies. **A provider without `--webhook-secret-key` +> refuses every delivery**, which means post-merge failures silently never +> reopen anything. If you want this loop, you must set the key. + +Accepted signature headers are `X-Gitea-Signature` and `X-Hub-Signature-256` +(HMAC-SHA256 over the raw body, `sha256=` prefix optional), compared in constant +time. + +Matching is precise, not approximate: a failing status carries a commit sha, and +the accept path recorded exactly which commit each merge produced. A failure on +a commit mcpctl did not merge is ignored — it is somebody else's business, and +guessing at "whatever merged most recently" would reopen innocent work. + +Only `failure` and `error` states do anything. Success and pending are +acknowledged and ignored. + +## RBAC + +| Resource | Verbs | +|---|---| +| `gitproviders` | `view`, `create`, `edit`, `delete`, **`run`** | +| `repos` | `view`, `create`, `edit`, `delete` | + +`run:gitproviders` covers discovery, import and probe. Those reach out to a +third-party system with a stored credential, so they are `run`, not `view` — +holding `view:gitproviders` should not let you drive the forge's API. + +```sh +mcpctl create rbac forge-user --role run --resource gitproviders +``` + +## See also + +- [epics.md](epics.md) — the work items whose PRs get merged here +- [agent-authority.md](agent-authority.md) — whose authority an agent acts under +- [secret-backends.md](secret-backends.md) — where the forge credential lives diff --git a/docs/inference-tasks.md b/docs/inference-tasks.md index 6304d91..c288211 100644 --- a/docs/inference-tasks.md +++ b/docs/inference-tasks.md @@ -136,10 +136,21 @@ result is just discarded server-side. ## CLI +> **Renamed.** This queue used to answer to `mcpctl get tasks`. Agentic-teams +> **work items** own that name now, so the queue keeps only its explicit +> spellings: `inference-tasks`, `inference-task`, `itasks`, `itask`. Bare +> `tasks`/`task` list work items — see [epics.md](epics.md). +> +> The same split applies to RBAC: this queue is authorized by +> `view:inference-tasks`, work items by `view:tasks`. Bindings written before +> the split are rewritten once at startup, so an existing `view:tasks` grant +> keeps working here and does **not** silently become a grant over work items. + ```sh -mcpctl get tasks # table view: ID, STATUS, POOL, LLM, MODEL, STREAM, AGE, WORKER -mcpctl get task # describe one -mcpctl get task -o yaml # full row, yaml format +mcpctl get inference-tasks # table view: ID, STATUS, POOL, LLM, MODEL, STREAM, AGE, WORKER +mcpctl get itasks # short alias +mcpctl get inference-task # describe one +mcpctl get inference-task -o yaml # full row, yaml format # Enqueue async via chat-llm: mcpctl chat-llm vllm-local-qwen3 --async -m "summarize the changelog" diff --git a/src/cli/src/commands/apply.ts b/src/cli/src/commands/apply.ts index 36fd16a..4c906a2 100644 --- a/src/cli/src/commands/apply.ts +++ b/src/cli/src/commands/apply.ts @@ -232,6 +232,10 @@ const GitProviderSpecSchema = z.object({ type: z.string().optional(), description: z.string().optional(), credRef: z.object({ name: z.string(), key: z.string() }).nullable().optional(), + // Key in the credential Secret holding the webhook signing secret. The + // webhook endpoint fails closed without it, so a provider that omits this + // silently never reopens tasks whose merge broke CI. + webhookSecretKey: z.string().nullable().optional(), skipProbe: z.boolean().optional(), }); diff --git a/src/cli/src/commands/create.ts b/src/cli/src/commands/create.ts index ed9ab5c..23417d6 100644 --- a/src/cli/src/commands/create.ts +++ b/src/cli/src/commands/create.ts @@ -131,11 +131,13 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { .option('--description ', 'Description') .option('--secret ', 'Secret holding the API token') .option('--key ', 'Key within that secret', 'TOKEN') + .option('--webhook-secret-key ', 'Key in that secret holding the webhook signing secret; without it incoming webhooks are refused') .option('--skip-probe', 'Skip the credential check at registration time') .action(async (name: string, opts) => { const body: Record = { name, url: opts.url, type: opts.type }; if (opts.description !== undefined) body.description = opts.description; if (opts.secret !== undefined) body.credRef = { name: opts.secret, key: opts.key }; + if (opts.webhookSecretKey !== undefined) body.webhookSecretKey = opts.webhookSecretKey; if (opts.skipProbe === true) body.skipProbe = true; const created = await client.post<{ name: string; type: string; url: string }>('/api/v1/gitproviders', body); log(`Created gitprovider '${created.name}' (${created.type} @ ${created.url})`); @@ -1019,7 +1021,7 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { const buf = await fs.readFile(full); // Reject non-UTF8 — v1 is text-only. const text = buf.toString('utf-8'); - if (text.includes('')) { + if (text.includes('\u0000')) { throw new Error(`File ${rel} contains a null byte; binaries aren't supported in v1`); } files[rel] = text; diff --git a/src/mcpd/src/services/git-provider.service.ts b/src/mcpd/src/services/git-provider.service.ts index 038df36..235820a 100644 --- a/src/mcpd/src/services/git-provider.service.ts +++ b/src/mcpd/src/services/git-provider.service.ts @@ -86,6 +86,7 @@ export class GitProviderService { ownerId, type, ...(data.description !== undefined ? { description: data.description } : {}), + ...(data.webhookSecretKey !== undefined ? { webhookSecretKey: data.webhookSecretKey } : {}), ...cred, }); } @@ -114,6 +115,7 @@ export class GitProviderService { ...(data.url !== undefined ? { url: data.url } : {}), ...(data.type !== undefined ? { type: data.type } : {}), ...(data.description !== undefined ? { description: data.description } : {}), + ...(data.webhookSecretKey !== undefined ? { webhookSecretKey: data.webhookSecretKey } : {}), ...cred, }); } diff --git a/src/mcpd/src/validation/git-provider.schema.ts b/src/mcpd/src/validation/git-provider.schema.ts index 38c3df1..da79706 100644 --- a/src/mcpd/src/validation/git-provider.schema.ts +++ b/src/mcpd/src/validation/git-provider.schema.ts @@ -11,12 +11,21 @@ const InstanceUrlSchema = z.string().url().refine( { message: 'url must be http:// or https://' }, ); +/** + * Key within the provider's credential Secret holding the webhook signing + * secret. Without it the webhook endpoint refuses every delivery (it fails + * closed), so post-merge CI failures never reopen their task — leaving the + * board claiming `merged` for work that broke the build. + */ +const WebhookSecretKeySchema = z.string().min(1).max(200); + export const CreateGitProviderSchema = z.object({ name: z.string().min(1).max(100).regex(NAME_RE, 'Name must be lowercase alphanumeric with hyphens'), url: InstanceUrlSchema, type: z.enum(GIT_PROVIDER_TYPES).optional(), description: z.string().max(500).optional(), credRef: CredRefSchema.optional(), + webhookSecretKey: WebhookSecretKeySchema.optional(), /** Skip the registration-time credential probe (wiring config before infra exists). */ skipProbe: z.boolean().optional(), }); @@ -26,6 +35,9 @@ export const UpdateGitProviderSchema = z.object({ type: z.enum(GIT_PROVIDER_TYPES).optional(), description: z.string().max(500).optional(), credRef: CredRefSchema.nullable().optional(), + // Nullable so webhook verification can be turned back off without deleting + // the provider. + webhookSecretKey: WebhookSecretKeySchema.nullable().optional(), skipProbe: z.boolean().optional(), }); diff --git a/src/mcpd/tests/services/git-provider.test.ts b/src/mcpd/tests/services/git-provider.test.ts index f7cc410..7fbd0a4 100644 --- a/src/mcpd/tests/services/git-provider.test.ts +++ b/src/mcpd/tests/services/git-provider.test.ts @@ -441,3 +441,64 @@ describe('GiteaDriver — pull requests', () => { await expect(new GiteaDriver().getPullRequest(CTX, REF)).rejects.toThrow(GitProviderAuthError); }); }); + +describe('GitProviderService — webhook signing key', () => { + beforeEach(() => { vi.clearAllMocks(); }); + + /** + * `webhookSecretKey` existed on the schema and was read by the webhook route, + * but nothing could ever write it: it was absent from the create schema, the + * update schema, the service, the CLI and the apply spec. Since the endpoint + * fails closed on a null key, post-merge CI failures could never reopen their + * task — the board kept claiming `merged` for work that broke the build. + * + * These pin the write path that makes the feature reachable at all. + */ + it('persists the key given at registration', async () => { + const { service, providerRepo } = svc(); + await service.create( + { + name: 'mysources', + url: 'https://mysources.co.uk', + credRef: { name: 'gitea-token', key: 'TOKEN' }, + webhookSecretKey: 'WEBHOOK', + skipProbe: true, + }, + 'user-1', + ); + + expect(providerRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ webhookSecretKey: 'WEBHOOK' }), + ); + }); + + it('leaves the column untouched when the flag is omitted', async () => { + // Omission must not be read as "clear it" — that would silently disarm the + // webhook on any unrelated edit. + const { service, providerRepo } = svc(); + await service.update('gp-1', { description: 'renamed', skipProbe: true }); + + const patch = (providerRepo.update as ReturnType).mock.calls[0]?.[1] as Record; + expect(patch).not.toHaveProperty('webhookSecretKey'); + }); + + it('turns verification back off when explicitly nulled', async () => { + const { service, providerRepo } = svc(); + await service.update('gp-1', { webhookSecretKey: null, skipProbe: true }); + + expect(providerRepo.update).toHaveBeenCalledWith( + 'gp-1', + expect.objectContaining({ webhookSecretKey: null }), + ); + }); + + it('rejects an empty key rather than storing a falsy signing secret', async () => { + const { service } = svc(); + await expect( + service.create( + { name: 'mysources', url: 'https://mysources.co.uk', webhookSecretKey: '', skipProbe: true }, + 'user-1', + ), + ).rejects.toThrow(); + }); +});