diff --git a/.env.example b/.env.example index b3e6d982fc..a22867ef24 100644 --- a/.env.example +++ b/.env.example @@ -154,6 +154,10 @@ LOOPOVER_REVIEW_DRAFT=false # LOOPOVER_API_TOKEN= # server-to-server API bearer token # GITTENSORY_API_TOKEN= # no longer read (removed by #4777) # LOOPOVER_MCP_TOKEN= # shared MCP bearer token +# LOOPOVER_MCP_ADMIN_TOKEN= # separate, higher-privilege MCP admin token (config read/write +# # tools) -- distinct from LOOPOVER_MCP_TOKEN so a leaked ordinary +# # MCP credential can never rewrite fleet-wide gate policy. Inert +# # unless LOOPOVER_MCP_ADMIN_ENABLED is also set (see section 3). # GITTENSORY_MCP_TOKEN= # no longer read (removed by #4777) # INTERNAL_JOB_TOKEN= @@ -246,6 +250,14 @@ LOOPOVER_REVIEW_DRAFT=false # # silently falls back to built-in defaults). Set once you've # # confirmed this is intentional, e.g. a fresh install with no # # .loopover.yml written yet. Default false (warning shown). +# LOOPOVER_MCP_ADMIN_ENABLED= # registers the "admin" MCP tool category (read/write this instance's +# # own LOOPOVER_REPO_CONFIG_DIR config, list backups). Default OFF -- +# # the tools are not even registered, matching the +# # LOOPOVER_REVIEW_SCREENSHOTS/_ENRICHMENT/_OPS "inert when off" +# # convention. Also requires LOOPOVER_MCP_ADMIN_TOKEN (section 2) and, +# # for the write tools specifically, flipping the config bind mount in +# # docker-compose.yml from :ro to :rw yourself -- see that file's +# # comment above the mount. See self-hosting-configuration.mdx. # COMPOSE_PROJECT_NAME=loopover # Docker Compose's own project name; also labels the log stream # # Promtail ships to Loki. Change it to run two stacks on one host # # (#4896) -- Compose namespaces container names, named volumes, and diff --git a/apps/loopover-ui/content/docs/self-hosting-configuration.mdx b/apps/loopover-ui/content/docs/self-hosting-configuration.mdx index 6fa9a2ef3b..91071cda13 100644 --- a/apps/loopover-ui/content/docs/self-hosting-configuration.mdx +++ b/apps/loopover-ui/content/docs/self-hosting-configuration.mdx @@ -307,6 +307,47 @@ features: The `features:` block above overrides a deployment-wide `LOOPOVER_REVIEW_*` flag (rag, reputation, safety) for this one repo, with three states per key: `true` forces the capability on for this repo (still subject to the env flag itself being enabled — it can never turn on a capability the operator has fully disabled at the deployment level); `false` forces it off for this repo regardless of the env flag; and omitting the key entirely falls back to the `LOOPOVER_REVIEW_REPOS` allowlist default, i.e. today's behavior for an operator who hasn't set anything here. See [Tuning your reviews](/docs/tuning) for the full `LOOPOVER_REVIEW_*` flag list this overrides. +## MCP admin config tools + +By default the config above is edit-by-hand-and-restart only. Setting `LOOPOVER_MCP_ADMIN_ENABLED` additionally registers a small `admin` MCP tool category that reads and **writes** it: `loopover_admin_get_config`, `loopover_admin_write_config`, and `loopover_admin_list_config_backups`. Off by default, and three things have to be true before it does anything: + +1. **`LOOPOVER_MCP_ADMIN_ENABLED=1`** — registers the tools at all. Unset (or falsy) means they don't exist on the server, not just "reject the call" — the same "inert when off" convention `LOOPOVER_REVIEW_SCREENSHOTS`/`LOOPOVER_REVIEW_ENRICHMENT`/`LOOPOVER_REVIEW_OPS` already use elsewhere in this app. +2. **A separate `LOOPOVER_MCP_ADMIN_TOKEN`.** Generate and set it exactly like any other bearer token below — but never reuse `LOOPOVER_MCP_TOKEN`'s value for it. They're deliberately different credentials so a leaked, end-user-obtainable `LOOPOVER_MCP_TOKEN` (see the callout above) can never rewrite fleet-wide gate policy; calls authenticated with the ordinary token are rejected even once the flag above is on. +3. **The config mount itself flipped to `:rw`.** `docker-compose.yml` mounts `LOOPOVER_REPO_CONFIG_DIR` read-only regardless of the flag above — enabling the flag alone does not make the mount secretly writable. Edit the line yourself: + +`} +/> + + + +Restart the `loopover` service after changing either (`docker compose up -d --no-deps loopover`, or `./scripts/selfhost-update.sh`). Both tokens support the same `_FILE` secret-file convention as `LOOPOVER_MCP_TOKEN` — see `secrets/README.md`. + +`loopover_admin_write_config` validates against the same schema-aware validator `loopover_validate_config` already exposes (not a separate, looser check), writes a timestamped backup of whatever file it's about to overwrite first, and lands the new content via a temp-file-plus-rename atomic write. Pass `dryRun: true` to run that same validation and see what would happen without touching disk: + + + +Drop `dryRun` (or set it to `false`) to write for real once you're happy with the dry-run result. `scope` is `"global"` (the mount-root default file) or `"repo"` (pass `repoFullName`); `loopover_admin_get_config` additionally accepts `"effective"` to read the exact deep-merged view a real review sees, same as the "Private per-repo config" deep-merge described above. `loopover_admin_list_config_backups` takes the same `scope`/`repoFullName` pair and returns each backup's path and timestamp, newest first. + + + These tools only read and write `LOOPOVER_REPO_CONFIG_DIR`. They do not trigger a redeploy, and they do not touch the public dashboard or `/v1/app/*` settings surface — `LOOPOVER_MCP_ADMIN_TOKEN` cannot sign into the control panel or call the routes `ADMIN_GITHUB_LOGINS` gates. + + ## Config-as-code blocks with no dashboard equivalent Everything above has a dashboard row it mirrors. The fields below exist **only** in `.loopover.yml` — there is no DB column or dashboard toggle for them, so a self-host operator who never reads the example file may not know they exist. diff --git a/apps/loopover-ui/content/docs/self-hosting-security.mdx b/apps/loopover-ui/content/docs/self-hosting-security.mdx index 86eaca2828..c066ec3ccc 100644 --- a/apps/loopover-ui/content/docs/self-hosting-security.mdx +++ b/apps/loopover-ui/content/docs/self-hosting-security.mdx @@ -26,7 +26,7 @@ eyebrow: Self-hosting ]} /> -`docker-compose.yml` ships native Docker Compose `secrets:` mounts for the highest-value secrets (the GitHub App private key, webhook secret, API/MCP/internal-job tokens, the setup token, the two token-encryption master keys, the Orb enrollment secret, the PagerDuty routing key, and the Claude Code subscription token) — file-mounted at `/run/secrets/`, never exposed via `docker inspect` or `docker compose config` the way a plain `environment:`/`env_file` value is. This is purely additive: an inline `.env` value always takes priority if you set both, so you can migrate one secret at a time, or not at all. See `secrets/README.md` for the full file list. +`docker-compose.yml` ships native Docker Compose `secrets:` mounts for the highest-value secrets (the GitHub App private key, webhook secret, API/MCP/MCP-admin/internal-job tokens, the setup token, the two token-encryption master keys, the Orb enrollment secret, the PagerDuty routing key, and the Claude Code subscription token) — file-mounted at `/run/secrets/`, never exposed via `docker inspect` or `docker compose config` the way a plain `environment:`/`env_file` value is. This is purely additive: an inline `.env` value always takes priority if you set both, so you can migrate one secret at a time, or not at all. See `secrets/README.md` for the full file list. (Compose's default target). See the top-level `secrets:` # block above and secrets/README.md. @@ -172,6 +181,7 @@ services: - github_webhook_secret - loopover_api_token - loopover_mcp_token + - loopover_mcp_admin_token - internal_job_token - selfhost_setup_token - token_encryption_secret @@ -1327,6 +1337,8 @@ secrets: file: ./secrets/loopover_api_token.txt loopover_mcp_token: file: ./secrets/loopover_mcp_token.txt + loopover_mcp_admin_token: + file: ./secrets/loopover_mcp_admin_token.txt internal_job_token: file: ./secrets/internal_job_token.txt selfhost_setup_token: diff --git a/scripts/selfhost-init-secrets.sh b/scripts/selfhost-init-secrets.sh index 7431a03260..b0fb1b94f2 100755 --- a/scripts/selfhost-init-secrets.sh +++ b/scripts/selfhost-init-secrets.sh @@ -44,6 +44,7 @@ RANDOM_SECRET_FILES=( "github_webhook_secret.txt" "loopover_api_token.txt" "loopover_mcp_token.txt" + "loopover_mcp_admin_token.txt" "internal_job_token.txt" "selfhost_setup_token.txt" "token_encryption_secret.txt" diff --git a/secrets/README.md b/secrets/README.md index f149250802..2e3a8160fd 100644 --- a/secrets/README.md +++ b/secrets/README.md @@ -63,6 +63,7 @@ see the tradeoff explained above for why `600` breaks the app's own ability to r | `github_webhook_secret.txt` | `GITHUB_WEBHOOK_SECRET_FILE` | HMAC key GitHub webhook deliveries are verified against. | | `loopover_api_token.txt` | `LOOPOVER_API_TOKEN_FILE` | Server-to-server API bearer token. | | `loopover_mcp_token.txt` | `LOOPOVER_MCP_TOKEN_FILE` | Shared MCP bearer token. | +| `loopover_mcp_admin_token.txt` | `LOOPOVER_MCP_ADMIN_TOKEN_FILE` | Higher-privilege MCP admin token (config read/write tools); inert unless `LOOPOVER_MCP_ADMIN_ENABLED` is also set. | | `internal_job_token.txt` | `INTERNAL_JOB_TOKEN_FILE` | Gates internal-only routes (e.g. `/v1/internal/*`). | | `selfhost_setup_token.txt` | `SELFHOST_SETUP_TOKEN_FILE` | Unlocks the first-run `/setup` wizard. | | `token_encryption_secret.txt` | `TOKEN_ENCRYPTION_SECRET_FILE` | AES-256-GCM master secret for maintainer BYOK keys at rest. | @@ -80,7 +81,7 @@ of those too; add a matching `secrets:` entry in `docker-compose.yml` (or a ## Never commit real files here Everything in this directory except this README is gitignored. `scripts/selfhost-init-secrets.sh` -generates a real random value for the seven self-generatable files (so `docker compose build`/`up` +generates a real random value for each self-generatable file (so `docker compose build`/`up` never fails on a missing file, and boots without any manual `openssl` step) and creates only an **empty** placeholder for the four externally-issued ones it can't generate a usable value for. Either way, it only ever touches the *permissions* of a file that is still empty, never its content — the diff --git a/src/api/routes.ts b/src/api/routes.ts index 0fb4c9ddfa..9bce01678b 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -6783,7 +6783,10 @@ async function requireAppRole(c: ProtectedRouteContext, allowedRoles: ControlPan if (!identity) return c.json({ error: "unauthorized" }, 401); if (identity.kind !== "session") { // LOOPOVER_MCP_TOKEN is a shared end-user credential; it must not satisfy app-role gates implicitly. - if (identity.actor === "mcp") return c.json({ error: "insufficient_role" }, 403); + // LOOPOVER_MCP_ADMIN_TOKEN (#7721) is narrower still by design -- config read/write only, explicitly + // NOT the public dashboard/API settings surface these app-role gates protect -- so it's excluded here + // too, same as the ordinary mcp token. + if (identity.actor === "mcp" || identity.actor === "mcp-admin") return c.json({ error: "insufficient_role" }, 403); return null; } const summary = await loadControlPanelRoleSummary(c.env, identity.actor); diff --git a/src/auth/security.ts b/src/auth/security.ts index ac2ea13715..78f6939a6e 100644 --- a/src/auth/security.ts +++ b/src/auth/security.ts @@ -15,7 +15,7 @@ function nonBlank(value: string | undefined): string | undefined { } export type AuthIdentity = - | { kind: "static"; actor: "api" | "mcp" | "internal" } + | { kind: "static"; actor: "api" | "mcp" | "mcp-admin" | "internal" } | { kind: "session"; actor: string; session: AuthSessionRecord }; export const SESSION_TTL_SECONDS = 7 * 24 * 60 * 60; @@ -109,6 +109,11 @@ export function createOpaqueToken(prefix = "gts"): string { export async function authenticatePrivateToken(env: Env, token: string | undefined): Promise { if (!token) return null; if (await timingSafeEqual(token, nonBlank(env.LOOPOVER_API_TOKEN))) return { kind: "static", actor: "api" }; + // Checked before the general LOOPOVER_MCP_TOKEN: a distinct, higher-privilege credential (#7721) so a leaked + // ordinary MCP token can never reach the admin config-write tools, which gate on actor === "mcp-admin" + // specifically. Order doesn't change behavior here (the two secrets are never equal in a real deployment), + // but checking the more-privileged token first keeps this function reading top-to-bottom by privilege. + if (await timingSafeEqual(token, nonBlank(env.LOOPOVER_MCP_ADMIN_TOKEN))) return { kind: "static", actor: "mcp-admin" }; if (await timingSafeEqual(token, nonBlank(env.LOOPOVER_MCP_TOKEN))) return { kind: "static", actor: "mcp" }; return authenticateSessionToken(env, token); } diff --git a/src/env.d.ts b/src/env.d.ts index 3d9ab7e811..65cab3d7e0 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -283,6 +283,18 @@ declare global { LOOPOVER_API_TOKEN?: string; /** Shared MCP bearer token (src/auth/security.ts). */ LOOPOVER_MCP_TOKEN?: string; + /** Higher-privilege MCP bearer token (#7721), distinct from LOOPOVER_MCP_TOKEN so a leaked ordinary MCP + * credential can never reach the admin config-write tools -- checked first in authenticatePrivateToken, + * resolves to actor "mcp-admin". Self-host only in practice: the admin tools it unlocks operate on + * LOOPOVER_REPO_CONFIG_DIR, which is unset (and thus a no-op) on the hosted Cloudflare deployment. */ + LOOPOVER_MCP_ADMIN_TOKEN?: string; + /** Master opt-in (#7721, default OFF) for the "admin" MCP tool category (read/write a self-hosted instance's + * own private .loopover.yml config). Gates TOOL REGISTRATION itself, not just call-time authorization -- + * matching this repo's "truly inert when off, tool not even registered" convention -- so the surface is + * invisible in tools/list unless an operator explicitly turns it on. Each tool call still separately + * requires actor === "mcp-admin" (LOOPOVER_MCP_ADMIN_TOKEN), so enabling this flag alone grants nothing to + * a caller using the ordinary LOOPOVER_MCP_TOKEN. */ + LOOPOVER_MCP_ADMIN_ENABLED?: string; INTERNAL_JOB_TOKEN: string; /** Repos the shared LOOPOVER_MCP_TOKEN may propose/decide/manage actions on (comma/whitespace `owner/repo` * list, or `*`/`all` for every repo). Unset ⇒ none — LOOPOVER_MCP_TOKEN is a shared, end-user-obtainable diff --git a/src/mcp/private-config-admin-registry.ts b/src/mcp/private-config-admin-registry.ts new file mode 100644 index 0000000000..5eb452f93b --- /dev/null +++ b/src/mcp/private-config-admin-registry.ts @@ -0,0 +1,55 @@ +// Workers-safe registry for the admin config-read/write/list-backups capability (#7721), mirroring +// src/signals/focus-manifest-loader.ts's setLocalManifestReader pattern exactly: this module holds +// nullable function slots and never imports node:fs itself, so it's safe in the Cloudflare Workers +// bundle. Only the self-host Node entry (server.ts) fills the slots, with real fs-backed closures +// built from src/selfhost/private-config.ts's write helpers -- that module's own fs import never +// reaches the Workers bundle because nothing there imports it directly, only through this registry. +// Unset (cloud, or self-host without LOOPOVER_REPO_CONFIG_DIR) means every function here stays null, +// and src/mcp/server.ts's admin tools -- gated separately on LOOPOVER_MCP_ADMIN_ENABLED -- report a +// clear "not configured" result rather than throwing. +import type { + ConfigAdminScope, + ConfigBackupEntry, + ConfigWriteResult, +} from "../selfhost/private-config"; + +// None of these take a `dir` parameter -- LOOPOVER_REPO_CONFIG_DIR is a fixed, boot-time constant for a +// given deployment (there is exactly one self-hosted config dir per running instance), so server.ts +// closes over it once when building these functions, the same way makeLocalManifestReader(dir) already +// returns an already-closurized RepoFocusManifestFetcher rather than taking dir per call. +export type ConfigAdminReader = () => Promise<{ path: string; content: string } | null>; +export type ConfigAdminWriter = (content: string) => Promise; +export type ConfigAdminRepoWriter = (repoFullName: string, content: string) => Promise; +export type ConfigAdminRepoReader = (repoFullName: string) => Promise<{ path: string; content: string } | null>; +export type ConfigAdminBackupLister = (scope: ConfigAdminScope) => Promise; + +let readGlobal: ConfigAdminReader | null = null; +let readRepo: ConfigAdminRepoReader | null = null; +let writeGlobal: ConfigAdminWriter | null = null; +let writeRepo: ConfigAdminRepoWriter | null = null; +let listBackups: ConfigAdminBackupLister | null = null; + +export function setConfigAdminFunctions(functions: { + readGlobal: ConfigAdminReader; + readRepo: ConfigAdminRepoReader; + writeGlobal: ConfigAdminWriter; + writeRepo: ConfigAdminRepoWriter; + listBackups: ConfigAdminBackupLister; +} | null): void { + readGlobal = functions?.readGlobal ?? null; + readRepo = functions?.readRepo ?? null; + writeGlobal = functions?.writeGlobal ?? null; + writeRepo = functions?.writeRepo ?? null; + listBackups = functions?.listBackups ?? null; +} + +export function getConfigAdminFunctions(): { + readGlobal: ConfigAdminReader; + readRepo: ConfigAdminRepoReader; + writeGlobal: ConfigAdminWriter; + writeRepo: ConfigAdminRepoWriter; + listBackups: ConfigAdminBackupLister; +} | null { + if (!readGlobal || !readRepo || !writeGlobal || !writeRepo || !listBackups) return null; + return { readGlobal, readRepo, writeGlobal, writeRepo, listBackups }; +} diff --git a/src/mcp/server.ts b/src/mcp/server.ts index f6799b776c..02780a2e1d 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -112,6 +112,9 @@ import { buildRecommendationQualityReport } from "../services/recommendation-qua import { computeFleetAnalytics } from "../orb/analytics"; import { loadMaintainerNoiseReport, maintainerNoiseSummary } from "../services/maintainer-noise"; import { buildAmsMinerCohortComparison } from "../review/ams-miner-cohort"; +import { getConfigAdminFunctions } from "./private-config-admin-registry"; +import { getLocalManifestReader } from "../signals/focus-manifest-loader"; +import type { ConfigAdminScope } from "../selfhost/private-config"; import { buildMaintainerActivationPreview } from "../services/maintainer-activation"; import { loadLabelAudit, labelAuditSummary } from "../services/label-audit"; import { loadMaintainerLaneReport, maintainerLaneSummary } from "../services/maintainer-lane"; @@ -347,6 +350,23 @@ const validateConfigShape = { source: z.enum(["repo_file", "api_record", "none"]).optional(), }; +// #7721 admin tools — self-hosted-instance-only, gated behind LOOPOVER_MCP_ADMIN_ENABLED at +// registration and actor === "mcp-admin" at call time (see the tool descriptions and handlers below). +const adminConfigScopeShape = { + scope: z.enum(["effective", "global", "repo"]), + repoFullName: z.string().min(3).max(200).optional(), +}; +const adminWriteConfigShape = { + scope: z.enum(["global", "repo"]), + repoFullName: z.string().min(3).max(200).optional(), + content: z.string().max(256 * 1024), + dryRun: z.boolean().optional(), +}; +const adminListBackupsShape = { + scope: z.enum(["global", "repo"]), + repoFullName: z.string().min(3).max(200).optional(), +}; + const preflightShape = { repoFullName: z.string().min(3).max(PREFLIGHT_LIMITS.repoFullNameChars), contributorLogin: z.string().min(1).max(PREFLIGHT_LIMITS.contributorLoginChars).optional(), @@ -1634,6 +1654,27 @@ const validateConfigOutputSchema = { normalized: z.record(z.string(), z.unknown()).optional(), status: z.enum(["ok", "warn", "error"]).optional(), }; + +const adminGetConfigOutputSchema = { + configured: z.boolean(), + found: z.boolean().optional(), + path: z.string().nullable().optional(), + content: z.string().nullable().optional(), +}; +const adminWriteConfigOutputSchema = { + configured: z.boolean(), + ok: z.boolean().optional(), + dryRun: z.boolean().optional(), + path: z.string().optional(), + backupPath: z.string().nullable().optional(), + error: z.string().optional(), +}; +const adminListBackupsOutputSchema = { + configured: z.boolean(), + backups: z + .array(z.object({ name: z.string(), path: z.string(), mtimeMs: z.number() })) + .optional(), +}; // #550: output schemas for the remaining tools (preflight/score/local-branch/agent), so MCP clients can // machine-validate their results. Same lenient style as the schemas above — documented top-level keys, // all optional, complex values as z.unknown(). No behavior change; these mirror the existing payloads. @@ -1888,11 +1929,15 @@ async function describeMcpUsageRequest(request: Request, telemetryMetadata: Reco // list. The ids mirror the issue's suggested surfaces: contributor discovery/planning, local-branch // & PR prep, review/gate prediction, agent automation, maintainer/repo-owner, and registry/config // utility. Attached to each tool as MCP `_meta.category` at registration (see createServer). -export type McpToolCategory = "discovery" | "branch" | "review" | "agent" | "maintainer" | "utility"; +// "admin" (#7721) is the newest category: self-hosted-operator-only tools that read/write the +// instance's OWN private .loopover.yml config. Unlike every other category, its tools are only +// REGISTERED at all when LOOPOVER_MCP_ADMIN_ENABLED is truthy (see isMcpAdminEnabled below) -- every +// other category's tools always exist and are gated purely by identity/allowlist at call time. +export type McpToolCategory = "discovery" | "branch" | "review" | "agent" | "maintainer" | "utility" | "admin"; // Canonical category order for grouped rendering (contributor-facing surfaces first, operator ones // last). Kept as a single source of truth so a display/grouping consumer never invents its own order. -export const MCP_TOOL_CATEGORY_IDS: readonly McpToolCategory[] = ["discovery", "branch", "review", "agent", "maintainer", "utility"]; +export const MCP_TOOL_CATEGORY_IDS: readonly McpToolCategory[] = ["discovery", "branch", "review", "agent", "maintainer", "utility", "admin"]; // Every registered tool maps to exactly one category. Listed in registration order (matching // createServer) so a new tool without a category entry is easy to spot in review; the @@ -1998,8 +2043,18 @@ export const MCP_TOOL_CATEGORIES: Record = { loopover_agent_get_run: "agent", loopover_agent_explain_next_action: "agent", loopover_agent_prepare_pr_packet: "branch", + loopover_admin_get_config: "admin", + loopover_admin_write_config: "admin", + loopover_admin_list_config_backups: "admin", }; +/** Master opt-in for the "admin" tool category (#7721), default OFF. Same truthy-string convention as every + * other LOOPOVER_* flag in this repo. Gates tool REGISTRATION in createServer() below; each admin tool + * handler additionally requires actor === "mcp-admin" at call time regardless of this flag. */ +function isMcpAdminEnabled(env: Env): boolean { + return /^(1|true|yes|on)$/i.test((env.LOOPOVER_MCP_ADMIN_ENABLED ?? "").trim()); +} + export class LoopoverMcp { private accessScopePromise: Promise | null = null; @@ -3055,6 +3110,46 @@ export class LoopoverMcp { async (input) => this.toolResult(await this.agentPreparePrPacket(input)), ); + // ── Admin tools (#7721) ────────────────────────────────────────────── + // Registered only when LOOPOVER_MCP_ADMIN_ENABLED is truthy -- "not just gated at call time" per the + // issue, matching this repo's "truly inert when off, tool not even registered" convention (contrast + // every OTHER category above, whose tools always exist and are gated purely by identity/allowlist + // inside their handlers). Each handler ALSO independently requires actor === "mcp-admin" + // (requireMcpAdmin) -- defense in depth, so enabling this flag alone never grants anything to a caller + // still using the ordinary LOOPOVER_MCP_TOKEN. + if (isMcpAdminEnabled(this.env)) { + register( + "loopover_admin_get_config", + { + description: + "Self-hosted-operator only. Read this instance's own private .loopover.yml config: the merged effective config for a repo (shared base + global default + per-repo override), or just the raw global-default layer, or just the raw per-repo layer. Requires LOOPOVER_MCP_ADMIN_TOKEN. Returns configured=false if LOOPOVER_REPO_CONFIG_DIR is unset.", + inputSchema: adminConfigScopeShape, + outputSchema: adminGetConfigOutputSchema, + }, + async (input) => this.toolResult(await this.adminGetConfig(input)), + ); + register( + "loopover_admin_write_config", + { + description: + "Self-hosted-operator only. Write this instance's own private global-default or per-repo .loopover.yml config: validated, a timestamped backup of any existing file first, atomic write. Set dryRun=true to validate without writing. Requires LOOPOVER_MCP_ADMIN_TOKEN. The config mount stays read-only (:ro) by default in docker-compose.yml -- an operator must flip it to :rw themselves before a real (non-dry-run) write can succeed.", + inputSchema: adminWriteConfigShape, + outputSchema: adminWriteConfigOutputSchema, + }, + async (input) => this.toolResult(await this.adminWriteConfig(input)), + ); + register( + "loopover_admin_list_config_backups", + { + description: + "Self-hosted-operator only. List timestamped backups (newest first) created by loopover_admin_write_config for the global-default or a specific repo's config. Requires LOOPOVER_MCP_ADMIN_TOKEN.", + inputSchema: adminListBackupsShape, + outputSchema: adminListBackupsOutputSchema, + }, + async (input) => this.toolResult(await this.adminListConfigBackups(input)), + ); + } + // ── Miner planning prompts ─────────────────────────────────────────── server.registerPrompt( "loopover_select_contribution_issue", @@ -3806,6 +3901,114 @@ export class LoopoverMcp { }; } + /** actor === "mcp-admin" only -- a distinct, higher-privilege credential (LOOPOVER_MCP_ADMIN_TOKEN, #7721) + * from the ordinary shared `mcp` identity, so a leaked LOOPOVER_MCP_TOKEN can never reach these tools even + * though LOOPOVER_MCP_ADMIN_ENABLED already gates whether they're registered at all. Session identities + * (browser login) are never admin either -- this is a self-hosted-operator CLI/automation credential, not + * something a dashboard session inherits. */ + private requireMcpAdmin(): void { + if (this.identity.kind === "static" && this.identity.actor === "mcp-admin") return; + throw new Error("Forbidden: this tool requires the LOOPOVER_MCP_ADMIN_TOKEN credential."); + } + + private adminScopeRepoFullName(scope: string, repoFullName: string | undefined): string { + if (scope !== "repo" && scope !== "effective") return ""; + if (!repoFullName) throw new Error(`repoFullName is required when scope is "${scope}".`); + return repoFullName; + } + + private async adminGetConfig(input: { scope: "effective" | "global" | "repo"; repoFullName?: string | undefined }): Promise { + this.requireMcpAdmin(); + const functions = getConfigAdminFunctions(); + if (!functions) { + return { + summary: "LoopOver admin config tools: not configured (LOOPOVER_REPO_CONFIG_DIR is unset on this instance).", + data: { configured: false }, + }; + } + if (input.scope === "effective") { + const repoFullName = this.adminScopeRepoFullName(input.scope, input.repoFullName); + const reader = getLocalManifestReader(); + const loaded = reader ? await reader(repoFullName) : null; + const content = typeof loaded === "string" ? loaded : (loaded?.content ?? null); + return { + summary: content === null ? `LoopOver admin config: no effective config found for ${repoFullName}.` : `LoopOver admin config: effective config loaded for ${repoFullName}.`, + data: { configured: true, found: content !== null, path: null, content }, + }; + } + const hit = + input.scope === "global" + ? await functions.readGlobal() + : await functions.readRepo(this.adminScopeRepoFullName(input.scope, input.repoFullName)); + return { + summary: hit ? `LoopOver admin config: ${input.scope} config loaded from ${hit.path}.` : `LoopOver admin config: no ${input.scope} config found.`, + data: { configured: true, found: hit !== null, path: hit?.path ?? null, content: hit?.content ?? null }, + }; + } + + private async adminWriteConfig(input: { + scope: "global" | "repo"; + repoFullName?: string | undefined; + content: string; + dryRun?: boolean | undefined; + }): Promise { + this.requireMcpAdmin(); + if (input.scope === "repo" && !input.repoFullName) { + throw new Error('repoFullName is required when scope is "repo".'); + } + if (input.dryRun) { + // Reuses the richer, schema-aware validator loopover_validate_config already exposes (unknown-field + // warnings, not just "is this valid YAML/JSON") -- a dry run is meant to preview what a real write + // would accept, so it should apply the SAME bar an operator would otherwise only discover by writing + // for real. The actual write path below still runs its own independent structural check + // (validateConfigWriteContent in private-config.ts) before touching disk regardless. + const report = buildFocusManifestValidation({ content: input.content, source: "repo_file" }); + return { + summary: `LoopOver admin config dry run: ${report.status}.`, + data: { configured: true, dryRun: true, ...report } as unknown as Record, + }; + } + const functions = getConfigAdminFunctions(); + if (!functions) { + return { + summary: "LoopOver admin config tools: not configured (LOOPOVER_REPO_CONFIG_DIR is unset on this instance).", + data: { configured: false }, + }; + } + const result = + input.scope === "global" ? await functions.writeGlobal(input.content) : await functions.writeRepo(input.repoFullName!, input.content); + if (!result.ok) { + return { + summary: `LoopOver admin config write failed: ${result.error}`, + data: { configured: true, ok: false, error: result.error }, + }; + } + return { + summary: `LoopOver admin config written to ${result.path}${result.backupPath ? ` (backed up to ${result.backupPath})` : ""}.`, + data: { configured: true, ok: true, path: result.path, backupPath: result.backupPath }, + }; + } + + private async adminListConfigBackups(input: { scope: "global" | "repo"; repoFullName?: string | undefined }): Promise { + this.requireMcpAdmin(); + if (input.scope === "repo" && !input.repoFullName) { + throw new Error('repoFullName is required when scope is "repo".'); + } + const functions = getConfigAdminFunctions(); + if (!functions) { + return { + summary: "LoopOver admin config tools: not configured (LOOPOVER_REPO_CONFIG_DIR is unset on this instance).", + data: { configured: false }, + }; + } + const scope: ConfigAdminScope = input.scope === "global" ? { kind: "global" } : { kind: "repo", repoFullName: input.repoFullName! }; + const backups = await functions.listBackups(scope); + return { + summary: `LoopOver admin config: ${backups.length} backup(s) for ${input.scope === "global" ? "the global config" : input.repoFullName}.`, + data: { configured: true, backups: backups as unknown as Array> }, + }; + } + private async canAccessRepo(fullName: string): Promise { if (this.identity.kind === "session") return canLoginAccessRepo(this.env, this.identity.actor, fullName); // The static `mcp` identity is a shared, end-user-obtainable CLI credential — scope it to the operator's diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index 98478a8d50..a77aab52eb 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -32,8 +32,9 @@ // // The reserved shared-base folder `_shared` is never treated as a bare repo-name config folder for a real GitHub // repo named `_shared`; use the owner-qualified or flat owner__repo candidates for that repository instead. -import { readFile, readdir } from "node:fs/promises"; -import { join, resolve } from "node:path"; +import { readFile, readdir, mkdir, copyFile, rename, writeFile, stat } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { basename, dirname, join, resolve } from "node:path"; import { parse as parseYaml } from "yaml"; import { MAX_FOCUS_MANIFEST_BYTES } from "../signals/focus-manifest"; import type { @@ -354,3 +355,169 @@ export function makeLocalReviewContextReader(dir: string | undefined): RepoRevie return { guide: null, skills: [] }; }; } + +// --------------------------------------------------------------------------------------------- +// Admin write path (#7721). Everything above this point is 100% read-only, mirroring the module's +// original scope; these exports add the write half via the same candidate-path resolution the read +// path already uses, so a write always lands on the SAME file a read of that scope would return. +// --------------------------------------------------------------------------------------------- + +export type ConfigAdminScope = { kind: "global" } | { kind: "repo"; repoFullName: string }; + +export type ConfigWriteResult = + | { ok: true; path: string; backupPath: string | null } + | { ok: false; error: string }; + +export type ConfigValidationResult = { ok: true } | { ok: false; error: string }; + +export type ConfigBackupEntry = { name: string; path: string; mtimeMs: number }; + +/** Validate write content against the same YAML/JSON-mapping shape the read path's own + * {@link parseConfigMapping} enforces for merging, but with a specific, actionable error message instead of a + * bare null — a write rejection needs to tell the caller WHY, unlike a read fallback which just moves on to the + * next layer. Deliberately reuses the same MAX_FOCUS_MANIFEST_BYTES ceiling and JSON/YAML detection heuristic + * (leading `{`/`[`) as parseConfigMapping so a document that would merge cleanly on read also validates cleanly + * on write, and vice versa. */ +export function validateConfigWriteContent(text: string): ConfigValidationResult { + const trimmed = text.trim(); + if (!trimmed) return { ok: false, error: "Content is empty." }; + if (trimmed.length > MAX_FOCUS_MANIFEST_BYTES) { + return { ok: false, error: `Content is ${trimmed.length} bytes, exceeding the ${MAX_FOCUS_MANIFEST_BYTES}-byte manifest size limit.` }; + } + const looksLikeJson = trimmed.startsWith("{") || trimmed.startsWith("["); + let parsed: unknown; + try { + parsed = looksLikeJson ? JSON.parse(trimmed) : parseYaml(trimmed); + } catch (error) { + return { ok: false, error: `Failed to parse as ${looksLikeJson ? "JSON" : "YAML"}: ${error instanceof Error ? error.message : String(error)}` }; + } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + return { ok: false, error: "Content must parse to a YAML/JSON mapping (object) at the top level, not a scalar, array, or null." }; + } + return { ok: true }; +} + +/** Resolve the ONE relative path a read OR write of this scope resolves to, so the two never disagree: the + * currently-existing candidate if one is already present (global: {@link GLOBAL_CONFIG_CANDIDATES}; repo: + * {@link localConfigCandidates}'s same priority order the read path uses), else the preferred path a first + * write creates (`.loopover.yml` at the config-dir root for global; the "clean, human-readable" bare + * repo-name folder — see this module's header comment — for a repo with no config yet). Null only for an + * invalid repo full name (no per-repo candidates at all). */ +async function resolveConfigScopePath(base: string, scope: ConfigAdminScope): Promise { + if (scope.kind === "global") { + const existing = await readFirstExistingWithPath(base, GLOBAL_CONFIG_CANDIDATES); + return existing?.path ?? GLOBAL_CONFIG_CANDIDATES[0]!; + } + const candidates = localConfigCandidates(scope.repoFullName); + if (candidates.length === 0) return null; + const existing = await readFirstExistingWithPath(base, candidates); + if (existing) return existing.path; + const slash = scope.repoFullName.indexOf("/"); + const repo = scope.repoFullName.slice(slash + 1).toLowerCase(); + return join(repo, CONFIG_BASENAMES[0]!); +} + +/** Write `content` to `absPath`, backing up any existing file first and writing atomically (temp file in the + * same directory + rename, so a reader never observes a partially-written file). The backup is a plain copy + * named `.bak-` alongside the original -- e.g. `.loopover.yml.bak- + * 20260723T094512345Z` -- so {@link listConfigBackupsForScope} can find it with a simple prefix match, and an + * operator can `docker cp` it out or restore it by hand without any tool support. Creates the parent + * directory (a brand-new per-repo folder) if it doesn't exist yet. No existing file to back up (first write + * to this path) is not an error -- there is simply nothing to copy. */ +async function atomicWriteWithBackup(absPath: string, content: string): Promise<{ backupPath: string | null }> { + await mkdir(dirname(absPath), { recursive: true }); + let backupPath: string | null = null; + const backupAbsPath = `${absPath}.bak-${new Date().toISOString().replace(/[-:]/g, "").replace(".", "")}`; + try { + await copyFile(absPath, backupAbsPath); + backupPath = backupAbsPath; + } catch (error) { + // ENOENT (no existing file at this path yet -- first write, nothing to back up) is the only + // expected/safe case to swallow. Anything else (EACCES, a host/container uid mismatch on the bind + // mount -- the exact class of bug secrets/README.md documents hitting in production on edge-nl-01, + // and this mount is bind-mounted the same way) means a file DOES exist but couldn't be safely copied + // -- proceeding to overwrite it anyway would silently destroy the only copy. Fail the whole write + // instead of a backup-less overwrite. + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") throw error; + } + const tmpAbsPath = `${absPath}.tmp-${randomUUID()}`; + await writeFile(tmpAbsPath, content, "utf8"); + await rename(tmpAbsPath, absPath); + return { backupPath }; +} + +/** Write the global-default config (validated, backed up, atomic — see {@link atomicWriteWithBackup}). Lands + * on whichever global candidate already exists, or creates `.loopover.yml` at the config-dir root if none + * does yet. */ +export async function writeGlobalConfig(dir: string, content: string): Promise { + const validation = validateConfigWriteContent(content); + if (!validation.ok) return validation; + const base = resolve(dir); + const relPath = (await resolveConfigScopePath(base, { kind: "global" }))!; + const { backupPath } = await atomicWriteWithBackup(resolve(base, relPath), content); + return { ok: true, path: relPath, backupPath }; +} + +/** Write a per-repo config override (validated, backed up, atomic — see {@link atomicWriteWithBackup}). Lands + * on whichever of {@link localConfigCandidates}'s candidates already exists for this repo, or creates the + * bare repo-name folder form if none does yet. */ +export async function writeRepoConfig(dir: string, repoFullName: string, content: string): Promise { + const validation = validateConfigWriteContent(content); + if (!validation.ok) return validation; + const base = resolve(dir); + const relPath = await resolveConfigScopePath(base, { kind: "repo", repoFullName }); + if (relPath === null) return { ok: false, error: `Invalid repo full name: ${repoFullName}` }; + const { backupPath } = await atomicWriteWithBackup(resolve(base, relPath), content); + return { ok: true, path: relPath, backupPath }; +} + +/** Read the raw, single-layer (not merged) global-default config text, or null if none exists. Distinct from + * {@link makeLocalManifestReader}'s reader, which returns the MERGED effective config for a repo (shared base + * + global + per-repo folded together) — this is the "global" scope of #7721's admin read tool, and also + * what a caller should read-modify-write against when editing just the global layer. */ +export async function readGlobalConfigRaw(dir: string): Promise<{ path: string; content: string } | null> { + const hit = await readFirstExistingWithPath(resolve(dir), GLOBAL_CONFIG_CANDIDATES); + return hit ? { path: hit.path, content: hit.text } : null; +} + +/** Read the raw, single-layer (not merged) per-repo override text, or null if none exists (including an + * invalid repo full name). Distinct from {@link makeLocalManifestReader}'s merged effective config — this is + * the "repo" scope of #7721's admin read tool. */ +export async function readRepoConfigRaw(dir: string, repoFullName: string): Promise<{ path: string; content: string } | null> { + const candidates = localConfigCandidates(repoFullName); + if (candidates.length === 0) return null; + const hit = await readFirstExistingWithPath(resolve(dir), candidates); + return hit ? { path: hit.path, content: hit.text } : null; +} + +/** List backups for a scope, newest first. Only ever looks alongside the ONE path + * {@link resolveConfigScopePath} resolves for this scope (matching whatever a write to this scope would + * target), not every historical candidate path a repo's config might once have lived at — so this always + * agrees with what write/read report for the same scope. Empty (not an error) when the directory doesn't + * exist, is unreadable, or has no matching backups yet. */ +export async function listConfigBackupsForScope(dir: string, scope: ConfigAdminScope): Promise { + const base = resolve(dir); + const relPath = await resolveConfigScopePath(base, scope); + if (relPath === null) return []; + const absPath = resolve(base, relPath); + const absDir = dirname(absPath); + const prefix = `${basename(absPath)}.bak-`; + let entries: string[]; + try { + entries = await readdir(absDir); + } catch { + return []; + } + const relDir = dirname(relPath); + const backups: ConfigBackupEntry[] = []; + for (const entry of entries) { + if (!entry.startsWith(prefix)) continue; + try { + const info = await stat(join(absDir, entry)); + backups.push({ name: entry, path: relDir === "." ? entry : join(relDir, entry), mtimeMs: info.mtimeMs }); + } catch { + // Race: entry disappeared between readdir and stat — skip it rather than fail the whole listing. + } + } + return backups.sort((a, b) => b.mtimeMs - a.mtimeMs); +} diff --git a/src/server.ts b/src/server.ts index 8b2b69ae08..cdac606045 100644 --- a/src/server.ts +++ b/src/server.ts @@ -74,7 +74,13 @@ import { createS3BlobStore } from "./selfhost/s3-blob-store"; import { makeLocalManifestReader, makeLocalReviewContextReader, + readGlobalConfigRaw, + readRepoConfigRaw, + writeGlobalConfig, + writeRepoConfig, + listConfigBackupsForScope, } from "./selfhost/private-config"; +import { setConfigAdminFunctions } from "./mcp/private-config-admin-registry"; import { assertSelfHostPreflight } from "./selfhost/preflight"; import { buildSentryOpenTelemetryBridge, @@ -365,6 +371,24 @@ async function main(): Promise { // (or legacy `/review/CLAUDE.md`) + skills/*.md, injected into the reviewer prompt so reviews follow each // repo's conventions. Unset dir ⇒ null reader ⇒ no change. setLocalReviewContextReader(makeLocalReviewContextReader(repoConfigDir)); + // Admin config read/write (#7721): same repoConfigDir, wired unconditionally here (not gated on + // LOOPOVER_MCP_ADMIN_ENABLED) -- that flag instead gates whether src/mcp/server.ts even REGISTERS the + // admin tools that call these functions, so an operator flipping the flag off doesn't require a + // restart-order dance with this wiring. Unset dir ⇒ null functions ⇒ the admin tools report a clear + // "not configured" result rather than throwing. The write functions still respect docker-compose.yml's + // default `:ro` config mount at the OS level -- registering them here does not itself make the mount + // writable; an operator who wants this capability flips it to `:rw` themselves (documented separately). + setConfigAdminFunctions( + repoConfigDir + ? { + readGlobal: () => readGlobalConfigRaw(repoConfigDir), + readRepo: (repoFullName) => readRepoConfigRaw(repoConfigDir, repoFullName), + writeGlobal: (content) => writeGlobalConfig(repoConfigDir, content), + writeRepo: (repoFullName, content) => writeRepoConfig(repoConfigDir, repoFullName, content), + listBackups: (scope) => listConfigBackupsForScope(repoConfigDir, scope), + } + : null, + ); // Boot-time visibility (config-drift guardrail): state which config dir is actually in effect, unconditionally // -- neither reader above logs anything, so an operator previously had no way to confirm from the logs alone // which directory (if any) was live, which is exactly the ambiguity that let a stale, no-longer-mounted config diff --git a/src/services/control-panel-roles.ts b/src/services/control-panel-roles.ts index 323122c036..2b62840db2 100644 --- a/src/services/control-panel-roles.ts +++ b/src/services/control-panel-roles.ts @@ -169,7 +169,7 @@ export function buildControlPanelRoleSummary(args: RoleSummaryInputs): ControlPa }; } -export function buildStaticControlPanelRoleSummary(actor: "api" | "mcp" | "internal"): ControlPanelRoleSummary { +export function buildStaticControlPanelRoleSummary(actor: "api" | "mcp" | "mcp-admin" | "internal"): ControlPanelRoleSummary { return { login: actor, generatedAt: nowIso(), diff --git a/src/signals/focus-manifest-loader.ts b/src/signals/focus-manifest-loader.ts index 21975d493a..021ac9f517 100644 --- a/src/signals/focus-manifest-loader.ts +++ b/src/signals/focus-manifest-loader.ts @@ -47,6 +47,14 @@ export function setLocalManifestReader(reader: RepoFocusManifestFetcher | null): localManifestReader = reader; } +/** The currently-registered reader, or null if unset (cloud, or self-host without the dir). Exposed read-only + * so another Workers-safe consumer (the admin MCP tools' "effective" config scope, #7721) can reuse the SAME + * merged-config computation this loader already performs, instead of re-implementing shared/global/per-repo + * layering a second time. */ +export function getLocalManifestReader(): RepoFocusManifestFetcher | null { + return localManifestReader; +} + /** * Async source for a repo's review CONTEXT (#review-skills): the `review/CLAUDE.md` guide + `review/skills/*.md` rubric * modules from the container-private config dir. Registered once at boot by the Node entry (server.ts); the filesystem diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index c303491d04..df86220c3f 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { completeGitHubWebOAuth, createSessionFromGitHubToken, pollGitHubDeviceFlow, startGitHubDeviceFlow, startGitHubWebOAuth } from "../../src/auth/github-oauth"; import { enforceRateLimit, RateLimiter, routeClassForPath } from "../../src/auth/rate-limit"; import { authenticatePrivateToken, buildBrowserSessionCookie, createSessionForGitHubUser, extractCookieValue, isAuthorizedGitHubSessionLogin, isMcpActuationRepoAllowed, revokeSession, timingSafeEqual } from "../../src/auth/security"; +import { createApp } from "../../src/api/routes"; import { PRODUCT_USER_AGENT } from "../../src/github/client"; import { issueOrbEnrollment } from "../../src/orb/broker"; import { createTestEnv, type TestD1Database } from "../helpers/d1"; @@ -43,6 +44,30 @@ describe("private-beta auth and rate limiting", () => { await expect(authenticatePrivateToken(env, "test-api-token")).resolves.toBeNull(); }); + it("authenticates LOOPOVER_MCP_ADMIN_TOKEN to actor mcp-admin, distinct from and unaffected by the ordinary LOOPOVER_MCP_TOKEN (#7721)", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_TOKEN: "admin-token", LOOPOVER_MCP_TOKEN: "ordinary-token" }); + await expect(authenticatePrivateToken(env, "admin-token")).resolves.toMatchObject({ kind: "static", actor: "mcp-admin" }); + await expect(authenticatePrivateToken(env, "ordinary-token")).resolves.toMatchObject({ kind: "static", actor: "mcp" }); + // Unset LOOPOVER_MCP_ADMIN_TOKEN never accidentally accepts an empty/undefined credential. + const unset = createTestEnv(); + await expect(authenticatePrivateToken(unset, "")).resolves.toBeNull(); + await expect(authenticatePrivateToken(unset, undefined)).resolves.toBeNull(); + }); + + it("excludes both mcp and mcp-admin static identities from app-role-gated routes (#7721)", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_TOKEN: "admin-token" }); + const app = createApp(); + const mcpRes = await app.request("/v1/app/overview", { headers: { authorization: `Bearer ${env.LOOPOVER_MCP_TOKEN}` } }, env); + expect(mcpRes.status).toBe(403); + await expect(mcpRes.json()).resolves.toMatchObject({ error: "insufficient_role" }); + const adminRes = await app.request("/v1/app/overview", { headers: { authorization: "Bearer admin-token" } }, env); + expect(adminRes.status).toBe(403); + await expect(adminRes.json()).resolves.toMatchObject({ error: "insufficient_role" }); + // A genuine operator-owned static credential (api) is unaffected by this exclusion. + const apiRes = await app.request("/v1/app/overview", { headers: { authorization: `Bearer ${env.LOOPOVER_API_TOKEN}` } }, env); + expect(apiRes.status).not.toBe(403); + }); + it("scopes MCP static-token actuation to an explicit repo allowlist, denying by default (#2253)", () => { // Unset/empty ⇒ deny (fail closed — the shared LOOPOVER_MCP_TOKEN must not implicitly actuate everywhere). expect(isMcpActuationRepoAllowed(undefined, "owner/repo")).toBe(false); diff --git a/test/unit/mcp-admin-config-tools.test.ts b/test/unit/mcp-admin-config-tools.test.ts new file mode 100644 index 0000000000..6d6552ce57 --- /dev/null +++ b/test/unit/mcp-admin-config-tools.test.ts @@ -0,0 +1,217 @@ +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { LoopoverMcp } from "../../src/mcp/server"; +import { setConfigAdminFunctions } from "../../src/mcp/private-config-admin-registry"; +import { setLocalManifestReader } from "../../src/signals/focus-manifest-loader"; +import type { AuthIdentity } from "../../src/auth/security"; +import { createTestEnv } from "../helpers/d1"; + +const MCP_ADMIN_IDENTITY: AuthIdentity = { kind: "static", actor: "mcp-admin" }; +const MCP_ORDINARY_IDENTITY: AuthIdentity = { kind: "static", actor: "mcp" }; + +async function connect(env: Env, identity: AuthIdentity = MCP_ADMIN_IDENTITY) { + const server = new LoopoverMcp(env, identity).createServer(); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + const client = new Client({ name: "mcp-admin-config-test", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + return client; +} + +afterEach(() => { + // Both registries are module-level singletons (#7721's own registry + the pre-existing focus-manifest + // one), exactly like setLocalReviewContextReader elsewhere in this suite -- reset after every test so + // one test's injected fakes can never leak into the next. + setConfigAdminFunctions(null); + setLocalManifestReader(null); +}); + +describe("MCP admin config tools: registration gating (#7721)", () => { + it("are NOT registered at all when LOOPOVER_MCP_ADMIN_ENABLED is unset (default off)", async () => { + const client = await connect(createTestEnv()); + const { tools } = await client.listTools(); + expect(tools.map((t) => t.name)).not.toEqual(expect.arrayContaining(["loopover_admin_get_config", "loopover_admin_write_config", "loopover_admin_list_config_backups"])); + }); + + it("are NOT registered when LOOPOVER_MCP_ADMIN_ENABLED is explicitly false-ish", async () => { + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "false" })); + const { tools } = await client.listTools(); + expect(tools.some((t) => t.name.startsWith("loopover_admin_"))).toBe(false); + }); + + it("ARE registered, with the admin category, when LOOPOVER_MCP_ADMIN_ENABLED is truthy", async () => { + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const { tools } = await client.listTools(); + const names = tools.map((t) => t.name); + expect(names).toEqual(expect.arrayContaining(["loopover_admin_get_config", "loopover_admin_write_config", "loopover_admin_list_config_backups"])); + for (const name of ["loopover_admin_get_config", "loopover_admin_write_config", "loopover_admin_list_config_backups"]) { + const tool = tools.find((t) => t.name === name)!; + expect((tool._meta as { category?: string } | undefined)?.category).toBe("admin"); + } + }); +}); + +describe("MCP admin config tools: auth boundary (#7721)", () => { + it("rejects the ordinary mcp actor even when the flag is on and the registry is configured", async () => { + setConfigAdminFunctions({ + readGlobal: vi.fn(), + readRepo: vi.fn(), + writeGlobal: vi.fn(), + writeRepo: vi.fn(), + listBackups: vi.fn(), + }); + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + const client = await connect(env, MCP_ORDINARY_IDENTITY); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "global" } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toMatch(/Forbidden/i); + }); + + it("rejects a session identity too -- this is a static-credential-only surface", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + const client = await connect(env, { kind: "session", actor: "some-login", session: {} as never }); + const result = await client.callTool({ name: "loopover_admin_list_config_backups", arguments: { scope: "global" } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toMatch(/Forbidden/i); + }); +}); + +describe("MCP admin config tools: not-configured behavior (#7721)", () => { + it("reports configured=false for get/write/list-backups when the registry has no injected functions", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + const client = await connect(env); + const get = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "global" } }); + expect(get.isError).toBeFalsy(); + expect((get.structuredContent as { configured: boolean }).configured).toBe(false); + + const write = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "gate:\n mode: advisory\n" } }); + expect((write.structuredContent as { configured: boolean }).configured).toBe(false); + + const list = await client.callTool({ name: "loopover_admin_list_config_backups", arguments: { scope: "global" } }); + expect((list.structuredContent as { configured: boolean }).configured).toBe(false); + }); +}); + +describe("MCP admin config tools: get (#7721)", () => { + it("global scope calls readGlobal and reports found=false when it returns null", async () => { + const readGlobal = vi.fn().mockResolvedValue(null); + setConfigAdminFunctions({ readGlobal, readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "global" } }); + expect(readGlobal).toHaveBeenCalledTimes(1); + expect(result.structuredContent).toMatchObject({ configured: true, found: false, path: null, content: null }); + }); + + it("global scope returns the path+content readGlobal resolves", async () => { + const readGlobal = vi.fn().mockResolvedValue({ path: ".loopover.yml", content: "gate:\n mode: advisory\n" }); + setConfigAdminFunctions({ readGlobal, readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "global" } }); + expect(result.structuredContent).toMatchObject({ configured: true, found: true, path: ".loopover.yml", content: "gate:\n mode: advisory\n" }); + }); + + it("repo scope requires repoFullName and calls readRepo with it", async () => { + const readRepo = vi.fn().mockResolvedValue({ path: "loopover/.loopover.yml", content: "gate:\n mode: hold\n" }); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo, writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + + const missingRepo = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "repo" } }); + expect(missingRepo.isError).toBe(true); + + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "repo", repoFullName: "JSONbored/loopover" } }); + expect(readRepo).toHaveBeenCalledWith("JSONbored/loopover"); + expect(result.structuredContent).toMatchObject({ configured: true, found: true, path: "loopover/.loopover.yml" }); + }); + + it("effective scope reuses the registered focus-manifest reader, not the admin registry's read functions", async () => { + const readGlobal = vi.fn(); + setConfigAdminFunctions({ readGlobal, readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + setLocalManifestReader(async (repoFullName) => ({ content: `merged:${repoFullName}`, sharedConfigSource: null, warnings: [] })); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "effective", repoFullName: "JSONbored/loopover" } }); + expect(readGlobal).not.toHaveBeenCalled(); + expect(result.structuredContent).toMatchObject({ configured: true, found: true, content: "merged:JSONbored/loopover" }); + }); + + it("effective scope reports found=false when no reader is registered", async () => { + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "effective", repoFullName: "JSONbored/loopover" } }); + expect(result.structuredContent).toMatchObject({ found: false, content: null }); + }); +}); + +describe("MCP admin config tools: write (#7721)", () => { + it("dry run validates via the schema-aware validator WITHOUT calling the write function", async () => { + const writeGlobal = vi.fn(); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal, writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "gate:\n mode: advisory\n", dryRun: true } }); + expect(writeGlobal).not.toHaveBeenCalled(); + // "warn" (not "ok"/"error") from the SAME richer, schema-aware validator loopover_validate_config uses + // -- proves the real validator ran (a raw structural check would just say valid YAML, no opinion on + // field names), not that this specific content is pristine. + expect(result.structuredContent).toMatchObject({ configured: true, dryRun: true, status: "warn" }); + }); + + it("dry run still runs even with an unconfigured registry (pure validation, no fs dependency)", async () => { + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "not: [valid", dryRun: true } }); + expect((result.structuredContent as { status: string }).status).toBe("error"); + }); + + it("global write calls writeGlobal and surfaces its result", async () => { + const writeGlobal = vi.fn().mockResolvedValue({ ok: true, path: ".loopover.yml", backupPath: null }); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal, writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "gate:\n mode: advisory\n" } }); + expect(writeGlobal).toHaveBeenCalledWith("gate:\n mode: advisory\n"); + expect(result.structuredContent).toMatchObject({ configured: true, ok: true, path: ".loopover.yml", backupPath: null }); + }); + + it("surfaces a failed write (e.g. invalid content caught by the real validator) without throwing", async () => { + const writeGlobal = vi.fn().mockResolvedValue({ ok: false, error: "Content is empty." }); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal, writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "" } }); + expect(result.isError).toBeFalsy(); // a rejected write is a normal tool result, not an MCP protocol error + expect(result.structuredContent).toMatchObject({ configured: true, ok: false, error: "Content is empty." }); + }); + + it("repo scope requires repoFullName and calls writeRepo with it", async () => { + const writeRepo = vi.fn().mockResolvedValue({ ok: true, path: "loopover/.loopover.yml", backupPath: "loopover/.loopover.yml.bak-x" }); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo, listBackups: vi.fn() }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + + const missingRepo = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "repo", content: "gate:\n mode: advisory\n" } }); + expect(missingRepo.isError).toBe(true); + + const result = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "repo", repoFullName: "JSONbored/loopover", content: "gate:\n mode: advisory\n" } }); + expect(writeRepo).toHaveBeenCalledWith("JSONbored/loopover", "gate:\n mode: advisory\n"); + expect(result.structuredContent).toMatchObject({ ok: true, backupPath: "loopover/.loopover.yml.bak-x" }); + }); +}); + +describe("MCP admin config tools: list backups (#7721)", () => { + it("global scope calls listBackups with a global scope object", async () => { + const listBackups = vi.fn().mockResolvedValue([{ name: ".loopover.yml.bak-x", path: ".loopover.yml.bak-x", mtimeMs: 123 }]); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_list_config_backups", arguments: { scope: "global" } }); + expect(listBackups).toHaveBeenCalledWith({ kind: "global" }); + expect((result.structuredContent as { backups: unknown[] }).backups).toHaveLength(1); + }); + + it("repo scope requires repoFullName and calls listBackups with a repo scope object", async () => { + const listBackups = vi.fn().mockResolvedValue([]); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups }); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + + const missingRepo = await client.callTool({ name: "loopover_admin_list_config_backups", arguments: { scope: "repo" } }); + expect(missingRepo.isError).toBe(true); + + await client.callTool({ name: "loopover_admin_list_config_backups", arguments: { scope: "repo", repoFullName: "JSONbored/loopover" } }); + expect(listBackups).toHaveBeenCalledWith({ kind: "repo", repoFullName: "JSONbored/loopover" }); + }); +}); diff --git a/test/unit/mcp-tool-categories.test.ts b/test/unit/mcp-tool-categories.test.ts index e571fa16e5..664b48da47 100644 --- a/test/unit/mcp-tool-categories.test.ts +++ b/test/unit/mcp-tool-categories.test.ts @@ -5,7 +5,13 @@ import { LoopoverMcp, MCP_TOOL_CATEGORIES, MCP_TOOL_CATEGORY_IDS } from "../../s import { createTestEnv } from "../helpers/d1"; async function listRegisteredTools() { - const mcpServer = new LoopoverMcp(createTestEnv()).createServer(); + // LOOPOVER_MCP_ADMIN_ENABLED: true -- the "admin" category (#7721) is this server's first-ever + // CONDITIONALLY-registered tool set (every other tool always registers). The default-off test env + // would otherwise make this file's own "exact sync" test below permanently fail: those 3 tool names + // are legitimately always present in MCP_TOOL_CATEGORIES (a static map), but never actually + // registered unless this flag is on. Enabling it here exercises the FULL possible tool surface, which + // is what "every map entry has a real, registered tool" should mean. + const mcpServer = new LoopoverMcp(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })).createServer(); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); await mcpServer.connect(serverTransport); const client = new Client({ name: "tool-category-test", version: "0.1.0" }, { capabilities: {} }); diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index cdd71d4c60..6196578edc 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -1,8 +1,24 @@ -import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { describe, expect, it } from "vitest"; -import { GLOBAL_CONFIG_CANDIDATES, isReviewSkillEnabled, localConfigCandidates, makeLocalManifestReader, makeLocalReviewContextReader, mergeConfigOverlay, parseReviewSkill, SHARED_BASE_CONFIG_CANDIDATES, type LocalManifestLoadResult } from "../../src/selfhost/private-config"; +import { + GLOBAL_CONFIG_CANDIDATES, + isReviewSkillEnabled, + listConfigBackupsForScope, + localConfigCandidates, + makeLocalManifestReader, + makeLocalReviewContextReader, + mergeConfigOverlay, + parseReviewSkill, + readGlobalConfigRaw, + readRepoConfigRaw, + SHARED_BASE_CONFIG_CANDIDATES, + validateConfigWriteContent, + writeGlobalConfig, + writeRepoConfig, + type LocalManifestLoadResult, +} from "../../src/selfhost/private-config"; import { loadRepoReviewContext, setLocalReviewContextReader, type RepoFocusManifestFetcher } from "../../src/signals/focus-manifest-loader"; import { MAX_FOCUS_MANIFEST_BYTES, parseFocusManifestContent } from "../../src/signals/focus-manifest"; @@ -576,6 +592,164 @@ describe("makeLocalReviewContextReader (#review-skills)", () => { }); }); +describe("validateConfigWriteContent (#7721)", () => { + it("rejects empty content", () => { + expect(validateConfigWriteContent("")).toEqual({ ok: false, error: "Content is empty." }); + expect(validateConfigWriteContent(" \n ")).toEqual({ ok: false, error: "Content is empty." }); + }); + it("rejects content over the manifest byte ceiling", () => { + const result = validateConfigWriteContent("gate:\n mode: advisory\n" + "x".repeat(MAX_FOCUS_MANIFEST_BYTES)); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("exceeding the"); + }); + it("rejects unparseable YAML with the parser's own error message", () => { + const result = validateConfigWriteContent("gate: [unterminated"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("Failed to parse as YAML"); + }); + it("rejects unparseable JSON when the content looks JSON-shaped", () => { + const result = validateConfigWriteContent("{ not valid json"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("Failed to parse as JSON"); + }); + it("rejects a top-level scalar, array, or null", () => { + expect(validateConfigWriteContent("just a string").ok).toBe(false); + expect(validateConfigWriteContent("[1, 2, 3]").ok).toBe(false); + expect(validateConfigWriteContent("null").ok).toBe(false); + }); + it("accepts a valid YAML mapping", () => { + expect(validateConfigWriteContent("gate:\n mode: advisory\n")).toEqual({ ok: true }); + }); + it("accepts a valid JSON mapping", () => { + expect(validateConfigWriteContent('{"gate": {"mode": "advisory"}}')).toEqual({ ok: true }); + }); +}); + +describe("writeGlobalConfig / readGlobalConfigRaw (#7721)", () => { + it("creates .loopover.yml at the config-dir root on first write, no backup", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + const result = await writeGlobalConfig(dir, "gate:\n mode: advisory\n"); + expect(result).toEqual({ ok: true, path: GLOBAL_CONFIG_CANDIDATES[0], backupPath: null }); + expect(readFileSync(join(dir, GLOBAL_CONFIG_CANDIDATES[0]!), "utf8")).toBe("gate:\n mode: advisory\n"); + expect(await readGlobalConfigRaw(dir)).toEqual({ path: GLOBAL_CONFIG_CANDIDATES[0], content: "gate:\n mode: advisory\n" }); + }); + + it("writes back to an already-existing .loopover.yaml instead of creating a new .loopover.yml", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + writeFileSync(join(dir, ".loopover.yaml"), "gate:\n mode: hold\n"); + const result = await writeGlobalConfig(dir, "gate:\n mode: advisory\n"); + expect(result.ok && result.path).toBe(".loopover.yaml"); + expect(existsSync(join(dir, ".loopover.yml"))).toBe(false); // no duplicate created + expect(readFileSync(join(dir, ".loopover.yaml"), "utf8")).toBe("gate:\n mode: advisory\n"); + }); + + it("backs up the existing file before overwriting, and the backup keeps the ORIGINAL content", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + await writeGlobalConfig(dir, "gate:\n mode: hold\n"); + const second = await writeGlobalConfig(dir, "gate:\n mode: advisory\n"); + expect(second.ok).toBe(true); + if (!second.ok) return; + expect(second.backupPath).not.toBeNull(); + expect(readFileSync(second.backupPath!, "utf8")).toBe("gate:\n mode: hold\n"); + expect(readFileSync(join(dir, GLOBAL_CONFIG_CANDIDATES[0]!), "utf8")).toBe("gate:\n mode: advisory\n"); + }); + + it("leaves no stray .tmp-* file behind after a successful write", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + await writeGlobalConfig(dir, "gate:\n mode: advisory\n"); + expect(readdirSync(dir).some((name) => name.includes(".tmp-"))).toBe(false); + }); + + it("propagates a non-ENOENT backup failure instead of silently overwriting without a backup", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + // A directory where a file is expected: copyFile throws EISDIR, not ENOENT -- the write must fail + // loudly rather than treat this the same as "no existing file, nothing to back up." + mkdirSync(join(dir, GLOBAL_CONFIG_CANDIDATES[0]!)); + await expect(writeGlobalConfig(dir, "gate:\n mode: advisory\n")).rejects.toThrow(); + }); + + it("rejects invalid content and writes nothing", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + const result = await writeGlobalConfig(dir, "not: [valid"); + expect(result.ok).toBe(false); + expect(existsSync(join(dir, ".loopover.yml"))).toBe(false); + }); + + it("readGlobalConfigRaw returns null when nothing exists", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + expect(await readGlobalConfigRaw(dir)).toBeNull(); + }); +}); + +describe("writeRepoConfig / readRepoConfigRaw (#7721)", () => { + it("creates the bare repo-name folder on first write for this repo", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + const result = await writeRepoConfig(dir, "JSONbored/loopover", "gate:\n mode: advisory\n"); + expect(result).toEqual({ ok: true, path: join("loopover", ".loopover.yml"), backupPath: null }); + expect(await readRepoConfigRaw(dir, "JSONbored/loopover")).toEqual({ + path: join("loopover", ".loopover.yml"), + content: "gate:\n mode: advisory\n", + }); + }); + + it("writes back to an existing owner-qualified file instead of creating the bare-folder form", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + mkdirSync(join(dir, "jsonbored__loopover"), { recursive: true }); + writeFileSync(join(dir, "jsonbored__loopover", ".loopover.yml"), "gate:\n mode: hold\n"); + const result = await writeRepoConfig(dir, "JSONbored/loopover", "gate:\n mode: advisory\n"); + expect(result.ok && result.path).toBe(join("jsonbored__loopover", ".loopover.yml")); + expect(existsSync(join(dir, "loopover", ".loopover.yml"))).toBe(false); // no duplicate in the bare folder + }); + + it("rejects an invalid repo full name without touching the filesystem", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + const result = await writeRepoConfig(dir, "no-slash", "gate:\n mode: advisory\n"); + expect(result).toEqual({ ok: false, error: "Invalid repo full name: no-slash" }); + }); + + it("rejects invalid content before ever resolving a path", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + const result = await writeRepoConfig(dir, "JSONbored/loopover", ""); + expect(result).toEqual({ ok: false, error: "Content is empty." }); + }); + + it("readRepoConfigRaw returns null for a repo with no config and for an invalid repo name", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-write-")); + expect(await readRepoConfigRaw(dir, "JSONbored/nonexistent")).toBeNull(); + expect(await readRepoConfigRaw(dir, "no-slash")).toBeNull(); + }); +}); + +describe("listConfigBackupsForScope (#7721)", () => { + it("empty for a scope with no backups yet", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-backups-")); + expect(await listConfigBackupsForScope(dir, { kind: "global" })).toEqual([]); + expect(await listConfigBackupsForScope(dir, { kind: "repo", repoFullName: "JSONbored/loopover" })).toEqual([]); + }); + + it("lists global backups newest-first after repeated writes, and never mixes in repo-scope backups", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-backups-")); + await writeGlobalConfig(dir, "gate:\n mode: hold\n"); + await writeGlobalConfig(dir, "gate:\n mode: warn\n"); // 1st backup, of "hold" + await new Promise((r) => setTimeout(r, 2)); // ensure a distinct mtime ordering + await writeGlobalConfig(dir, "gate:\n mode: advisory\n"); // 2nd backup, of "warn" + await writeRepoConfig(dir, "JSONbored/loopover", "gate:\n mode: hold\n"); // unrelated repo-scope write + + const globalBackups = await listConfigBackupsForScope(dir, { kind: "global" }); + expect(globalBackups.length).toBe(2); + expect(globalBackups[0]!.mtimeMs).toBeGreaterThanOrEqual(globalBackups[1]!.mtimeMs); // newest first + expect(globalBackups.every((b) => b.name.startsWith(`${GLOBAL_CONFIG_CANDIDATES[0]}.bak-`))).toBe(true); + + const repoBackups = await listConfigBackupsForScope(dir, { kind: "repo", repoFullName: "JSONbored/loopover" }); + expect(repoBackups).toEqual([]); // first write to this repo scope — nothing to back up yet + }); + + it("empty (not an error) for an invalid repo full name", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-admin-backups-")); + expect(await listConfigBackupsForScope(dir, { kind: "repo", repoFullName: "no-slash" })).toEqual([]); + }); +}); + describe("loadRepoReviewContext + setLocalReviewContextReader (#review-skills)", () => { it("empty with no reader; uses the registered reader; degrades to empty on error", async () => { setLocalReviewContextReader(null);