Skip to content

Add oddkit_write tool definition (stub)#57

Open
klappy wants to merge 13 commits intomainfrom
claude/fix-oddkit-write-RHpHK
Open

Add oddkit_write tool definition (stub)#57
klappy wants to merge 13 commits intomainfrom
claude/fix-oddkit-write-RHpHK

Conversation

@klappy
Copy link
Copy Markdown
Owner

@klappy klappy commented Feb 28, 2026

  • Add write action to tool-registry.js with files, message, branch, pr, repo params
  • Add case in actions.js switch (returns not_implemented)
  • Follows D0017: One action, progressive protection

Note

High Risk
Introduces a new destructive write action that can commit to arbitrary GitHub repos (including branch creation and PR opening) using an env-provided token. Any bugs in validation, target resolution, or API error handling could result in unintended repo modifications or data loss.

Overview
Adds a new oddkit_write capability end-to-end (tool registry, CLI, and MCP) that writes one or more files to a GitHub repo with a required commit message, optional branch creation, optional PR opening, and optional author/provenance metadata in the commit footer.

Implements the write path in core/actions.js backed by new helpers in utils/githubApi.js (Contents API for single-file writes, Git Data API for atomic multi-file commits, basic retry + conflict reporting) plus utils/writeValidation.js governance/path validation that blocks unsafe paths and reports non-blocking content warnings. Also bumps package version to 0.15.0.

Written by Cursor Bugbot for commit 2881cca. This will update automatically on new commits. Configure here.

Oddie and others added 7 commits February 28, 2026 18:51
- Add write action to tool-registry.js with files, message, branch, pr, repo params
- Add case in actions.js switch (returns not_implemented)
- Follows D0017: One action, progressive protection
- Forward write-specific args (files, message, branch, pr, repo) in MCP
  server Layer 1 and Layer 2 handleAction calls
- Forward write-specific options in both CLI handleAction call sites
- Remove colliding short flags from write cliFlags: -f (collides with
  --format), -b (collides with --baseline), -m (collides with --message
  alias consumed as input)
- Rename write --message to --commit-message to avoid being consumed by
  the input resolution fallback (options.message)
- Add src/utils/githubApi.js - GitHub REST API helper using native fetch
- Add src/utils/writeValidation.js - Governance validation for canon/odd/docs
- Update src/core/actions.js with full write implementation:
  - Parse baseline_url to get owner/repo
  - Validate files against governance constraints
  - Add provenance metadata in commit footer
  - Write via GitHub Contents API
  - Support branch and PR creation
- Follows IMPL-oddkit-write.md spec
…, and URL regex

- Actually call createBranch when target branch does not exist (was detected but never created)
- Pass actual default branch to createPR instead of hardcoded main
- Honor providedRepo param instead of silently ignoring it
- Fix github.com URL regex to allow dots in repo names (e.g. klappy.dev)
- Add Content-Type application/json header to githubRequest
- Use correct env var ODDKIT_BASELINE instead of ODDKIT_BASELINE_URL
- Auto-generate branch name when pr is true but branch is omitted
- URL-encode branch names in branchExists and getBranchSha
…rsal, report last commit, pass surface from callers, update orchestrator annotations

- Remove hardcoded klappy.dev fallback for destructive write action
- Block writes when path traversal sequences detected in file paths
- Report last commit SHA/URL instead of first for multi-file writes
- Pass surface param (cli/mcp) from both callers for accurate provenance
- Update orchestrator annotations to destructiveHint:true, idempotentHint:false
…ation, conflict handling

Spec-compliant rewrite of oddkit_write (IMPL-oddkit-write, D0017):

Tier 1 (Contents API): Single-file writes use PUT /contents/{path}.
Tier 2 (Git Data API): Multi-file writes use blob→tree→commit→ref
  for atomic commits. No more one-commit-per-file for multi-file ops.
Tier 3 (Branches/PRs): Branch creation and PR opening layered on top.
  Supports pr as boolean or structured { title, body, draft } object.

Validation: Required frontmatter fields expanded from [title, uri] to
  [title, uri, audience, tier, tags, epoch] per spec. Added header
  quality check (flags generic headers like "Background", "Details").
  Added absolute path rejection.

Schema: Added encoding field (reserved for future base64 binary support),
  author { name, email } for git author override, and provenance
  { session_id, surface } for structured traceability metadata.

Error handling:
  - 409 Conflict returns structured error with current file content
    and guidance text for merge resolution
  - Network failures retry once, then preserve file contents in
    error response so calling AI can present them to the user
  - Auth/permission errors return descriptive messages

Provenance footer built from structured provenance param when present,
falls back to surface from caller context (cli/mcp).

Output interface includes pr_updated field (false for now — orphan
prevention detection deferred to Layer 4, documented with TODO).

All existing tests pass.

https://claude.ai/code/session_01WB7hEQrCPziXWcSkiLjnns
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Feb 28, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
oddkit 2881cca Commit Preview URL

Branch Preview URL
Mar 01 2026, 03:52 AM

Comment thread src/core/tool-registry.js Outdated
Comment thread src/core/tool-registry.js Outdated
Comment thread src/utils/githubApi.js Outdated
…emove encodeURIComponent from git ref paths

- Revert orchestrator destructiveHint to false and idempotentHint to true since the
  dedicated oddkit_write tool already carries correct destructive annotations
- Filter write action out of ACTION_NAMES so the orchestrator enum does not advertise
  an action whose required params (files, message) are not in the orchestrator schema
- Remove encodeURIComponent from getRef, updateRef, and getBranchSha since the GitHub
  Git References API expects slashes as literal path separators in ref names
Comment thread src/core/tool-registry.js
…g allowlist

ACTION_NAMES excluded write to keep it out of the orchestrator enum,
but VALID_ACTIONS was aliased to ACTION_NAMES, blocking write in
handleAction. Add ALL_ACTION_NAMES (includes write) and use it as
the routing allowlist while keeping ACTION_NAMES for orchestrator only.
Comment thread src/utils/writeValidation.js

// Required fields — spec: title, uri, audience, tier, tags, epoch
const requiredFields = ["title", "uri", "audience", "tier", "tags", "epoch"];
const missingFields = requiredFields.filter((field) => !frontmatter.includes(`${field}:`));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontmatter field check uses substring matching

Low Severity

The required-field detection uses frontmatter.includes(${field}:) which performs substring matching. This causes false negatives — for example, "subtitle:".includes("title:") is true, so a document with only subtitle: and no title: would incorrectly pass the title check. The same applies to uri: matching inside security_uri:, tier: inside frontier:, and tags: inside hashtags:. The codebase already uses subtitle as a frontmatter field in buildIndex.js. A line-start–anchored regex match would avoid this.

Fix in Cursor Fix in Web

Comment thread src/utils/githubApi.js
body: JSON.stringify({ sha: commitSha, force: false }),
});
} catch (err) {
if (err.status === 409) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflict handling checks wrong HTTP status code

Medium Severity

updateRef checks for err.status === 409 to detect concurrent branch updates, but GitHub's Update Reference API returns 422 (not 409) for non-fast-forward failures. Additionally, githubRequest only sets err.status for 409 responses — a 422 produces a plain Error without .status. This makes the entire conflict-handling path in updateRef (and the downstream conflictData branch in actions.js) unreachable for multi-file atomic commits, so users get a generic error instead of the designed structured conflict guidance.

Additional Locations (1)

Fix in Cursor Fix in Web

Bugbot's review had two issues to correct:

1. Tilde (~) removal from path safety check was wrong — paths with ~
   are suspicious (shell home dir expansion) and the spec says paths
   must be "reasonable." Restored as defense-in-depth for write action.

2. encodeURIComponent was inconsistent across git API functions —
   bugbot removed it from getRef/updateRef/getBranchSha (correct,
   since branch names with / would break) but left it in branchExists.
   Removed from branchExists for consistency.

Bugbot's other changes were correct and kept:
- write excluded from orchestrator enum (has its own MCP tool)
- ALL_ACTION_NAMES used for routing allowlist
- Orchestrator annotations non-destructive (since write excluded)

https://claude.ai/code/session_01WB7hEQrCPziXWcSkiLjnns
Comment thread src/utils/githubApi.js Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: UTF-8 validation check can never fail for strings
    • Replaced dead Buffer.from try/catch with a regex that detects unpaired surrogates, which are valid in JS strings but not valid UTF-8.
  • ✅ Fixed: PR failure after successful commit loses commit info
    • Wrapped PR creation in its own try/catch inside the commit try block so a PR failure returns commit_sha, commit_url, and branch with a committed_pr_failed status instead of losing all commit metadata.
Preview (2881cca720)
diff --git a/package-lock.json b/package-lock.json
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,12 +1,12 @@
 {
   "name": "oddkit",
-  "version": "0.14.0",
+  "version": "0.15.0",
   "lockfileVersion": 3,
   "requires": true,
   "packages": {
     "": {
       "name": "oddkit",
-      "version": "0.14.0",
+      "version": "0.15.0",
       "license": "MIT",
       "dependencies": {
         "@modelcontextprotocol/sdk": "^1.0.0",

diff --git a/src/cli.js b/src/cli.js
--- a/src/cli.js
+++ b/src/cli.js
@@ -210,6 +210,11 @@
           return;
         }
 
+        // Build author from CLI flags if provided
+        const author = (options.authorName && options.authorEmail)
+          ? { name: options.authorName, email: options.authorEmail }
+          : undefined;
+
         const result = await handleAction({
           action: tool.name,
           input: input || "",
@@ -217,6 +222,13 @@
           mode: options.mode,
           baseline: options.baseline,
           repoRoot: options.repo,
+          files: options.files ? JSON.parse(options.files) : undefined,
+          message: options.commitMessage,
+          branch: options.branch,
+          pr: options.pr,
+          repo: options.repoTarget,
+          author,
+          surface: "cli",
         });
 
         outputActionResult(tool.name, result, format, quiet);
@@ -536,6 +548,11 @@
           return;
         }
 
+        // Build author from CLI flags if provided
+        const author = (options.authorName && options.authorEmail)
+          ? { name: options.authorName, email: options.authorEmail }
+          : undefined;
+
         const result = await handleAction({
           action: tool.name,
           input: input || "",
@@ -543,6 +560,13 @@
           mode: options.mode,
           baseline: options.baseline,
           repoRoot: options.repo,
+          files: options.files ? JSON.parse(options.files) : undefined,
+          message: options.commitMessage,
+          branch: options.branch,
+          pr: options.pr,
+          repo: options.repoTarget,
+          author,
+          surface: "cli",
         });
         const ok = !isActionError(result);
         console.log(JSON.stringify(wrapToolJson(tool.name, result, ok)));

diff --git a/src/core/actions.js b/src/core/actions.js
--- a/src/core/actions.js
+++ b/src/core/actions.js
@@ -18,7 +18,13 @@
 import { buildBM25Index, searchBM25 } from "../search/bm25.js";
 import { buildIndex, loadIndex, saveIndex, INDEX_VERSION } from "../index/buildIndex.js";
 import { ensureBaselineRepo, getSessionSha } from "../baseline/ensureBaselineRepo.js";
-import { ACTION_NAMES } from "./tool-registry.js";
+import { ALL_ACTION_NAMES } from "./tool-registry.js";
+import { validateFiles } from "../utils/writeValidation.js";
+import {
+  parseBaselineUrl, getFileSha, writeFile,
+  getDefaultBranch, getRef, branchExists, createBranch, createPR,
+  atomicMultiFileCommit,
+} from "../utils/githubApi.js";
 import { readFileSync, existsSync } from "fs";
 import { createRequire } from "module";
 import matter from "gray-matter";
@@ -30,7 +36,7 @@
 // Valid actions — derived from the shared tool registry (single source of truth)
 // ──────────────────────────────────────────────────────────────────────────────
 
-export const VALID_ACTIONS = ACTION_NAMES;
+export const VALID_ACTIONS = ALL_ACTION_NAMES;
 
 // ──────────────────────────────────────────────────────────────────────────────
 // State management
@@ -478,6 +484,222 @@
         };
       }
 
+      case "write": {
+        // oddkit_write — one action, progressive protection
+        // Tier 1: Contents API for single file
+        // Tier 2: Git Data API for multi-file atomic commits
+        // Tier 3: Branch creation and PR support (layers on top)
+        const { files, message, pr, repo: providedRepo, author, provenance } = params;
+        let { branch } = params;
+
+        // --- Input validation ---
+        if (!files || !Array.isArray(files) || files.length === 0) {
+          return {
+            action: "write",
+            result: { error: "No files provided. Expected array of {path, content} objects." },
+            assistant_text: "No files provided. Please provide an array of files with path and content.",
+            debug: makeDebug(),
+          };
+        }
+
+        if (!message) {
+          return {
+            action: "write",
+            result: { error: "Commit message required." },
+            assistant_text: "Commit message is required.",
+            debug: makeDebug(),
+          };
+        }
+
+        if (pr && !branch) {
+          branch = `oddkit-write/${Date.now()}`;
+        }
+
+        // --- Resolve target repo ---
+        let owner, repoName;
+
+        if (providedRepo) {
+          const parts = providedRepo.split("/");
+          if (parts.length !== 2 || !parts[0] || !parts[1]) {
+            return {
+              action: "write",
+              result: { error: `Invalid repo format: "${providedRepo}". Expected "owner/repo".` },
+              assistant_text: `Invalid repo format: "${providedRepo}". Expected "owner/repo".`,
+              debug: makeDebug(),
+            };
+          }
+          owner = parts[0];
+          repoName = parts[1];
+        } else {
+          const baselineUrl = baseline || process.env.ODDKIT_BASELINE;
+          if (!baselineUrl) {
+            return {
+              action: "write",
+              result: { error: "No target repo specified. Provide repo param or set ODDKIT_BASELINE." },
+              assistant_text: "Write requires an explicit target repo. Provide the repo parameter (owner/repo) or set ODDKIT_BASELINE.",
+              debug: makeDebug(),
+            };
+          }
+          try {
+            const parsed = parseBaselineUrl(baselineUrl);
+            owner = parsed.owner;
+            repoName = parsed.repo;
+          } catch (err) {
+            return {
+              action: "write",
+              result: { error: err.message },
+              assistant_text: `Failed to parse baseline URL: ${err.message}. Set ODDKIT_BASELINE or use repo parameter.`,
+              debug: makeDebug(),
+            };
+          }
+        }
+
+        // --- Validate files against governance constraints ---
+        const validation = validateFiles(files);
+
+        // Block writes if any path is unsafe (traversal sequences)
+        const unsafePaths = validation.results
+          .filter((r) => r.checks.some((c) => c.name === "path_safe" && !c.passed))
+          .map((r) => r.file);
+        if (unsafePaths.length > 0) {
+          return {
+            action: "write",
+            result: { error: `Unsafe path(s) detected: ${unsafePaths.join(", ")}`, validation },
+            assistant_text: `Write blocked: unsafe path detected in ${unsafePaths.join(", ")}. Remove '..' or '~' sequences.`,
+            debug: makeDebug(),
+          };
+        }
+
+        // --- Build provenance footer ---
+        // Use structured provenance param when present, fall back to surface from caller context
+        const surfaceValue = provenance?.surface || params.surface || "mcp";
+        const provenanceLines = [`oddkit-surface: ${surfaceValue}`];
+        if (provenance?.session_id) {
+          provenanceLines.push(`oddkit-session: ${provenance.session_id}`);
+        }
+        provenanceLines.push(`oddkit-timestamp: ${new Date().toISOString()}`);
+        const commitMessage = `${message}\n\n---\n${provenanceLines.join("\n")}`;
+
+        // --- Determine author ---
+        const gitAuthor = author || null;
+
+        try {
+          // --- Resolve target branch ---
+          let targetBranch = branch;
+          let defaultBranch = null;
+          let status = "committed";
+
+          if (!targetBranch) {
+            defaultBranch = await getDefaultBranch(owner, repoName);
+            targetBranch = defaultBranch;
+          } else {
+            const exists = await branchExists(owner, repoName, targetBranch);
+            if (!exists) {
+              defaultBranch = await getDefaultBranch(owner, repoName);
+              const { sha: sourceSha } = await getRef(owner, repoName, defaultBranch);
+              await createBranch(owner, repoName, targetBranch, sourceSha);
+              status = "branch_created";
+            }
+          }
+
+          // --- Write files ---
+          let commitResult;
+
+          if (files.length === 1) {
+            // Tier 1: Contents API — single file
+            const file = files[0];
+            const sha = await getFileSha(owner, repoName, file.path, targetBranch);
+            const result = await writeFile(
+              owner, repoName, file.path, file.content,
+              commitMessage, targetBranch, sha, gitAuthor,
+            );
+            commitResult = { commit_sha: result.commit_sha, commit_url: result.commit_url };
+          } else {
+            // Tier 2: Git Data API — multi-file atomic commit
+            commitResult = await atomicMultiFileCommit(
+              owner, repoName, targetBranch, files, commitMessage, gitAuthor,
+            );
+          }
+
+          // --- Handle PR if requested (Tier 3) ---
+          let prResult = null;
+          let prError = null;
+          // TODO: Orphan prevention (Layer 4) — before creating a new PR, check for
+          // existing open PRs from oddkit on the same branch or targeting the same files.
+          // If found, push to the existing branch instead (the PR updates automatically).
+          // The output interface supports this via pr_updated. Deferred until Layer 4.
+          if (pr && branch) {
+            try {
+              const prOpts = typeof pr === "object" ? pr : {};
+              const prTitle = prOpts.title || message;
+              const prBody = prOpts.body || `Files:\n${files.map((f) => `- ${f.path}`).join("\n")}\n\n---\nWritten via oddkit_write`;
+              const prDraft = prOpts.draft || false;
+              const baseBranch = defaultBranch || await getDefaultBranch(owner, repoName);
+              prResult = await createPR(owner, repoName, prTitle, prBody, branch, baseBranch, prDraft);
+              status = "pr_opened";
+            } catch (prErr) {
+              prError = prErr.message;
+              status = "committed_pr_failed";
+            }
+          }
+
+          const filesWritten = files.map((f) => f.path);
+          const validationWarnings = !validation.passed
+            ? validation.results.map((r) => r.checks.filter((c) => !c.passed).map((c) => c.name).join(", ")).filter((x) => x).join("; ")
+            : "";
+
+          return {
+            action: "write",
+            result: {
+              status,
+              commit_sha: commitResult.commit_sha,
+              commit_url: commitResult.commit_url,
+              branch: targetBranch,
+              files_written: filesWritten,
+              pr_url: prResult?.pr_url || undefined,
+              pr_number: prResult?.pr_number || undefined,
+              pr_error: prError || undefined,
+              pr_updated: false, // TODO: set to true when orphan prevention detects existing PR
+              validation,
+            },
+            assistant_text: `Successfully wrote ${filesWritten.length} file(s) to ${owner}/${repoName} on branch ${targetBranch}. Commit: ${commitResult.commit_url}${prResult ? `\nPR: ${prResult.pr_url}` : ""}${prError ? `\nPR creation failed: ${prError}` : ""}${validationWarnings ? `\n\nValidation warnings: ${validationWarnings}` : ""}`,
+            debug: makeDebug({ files_count: files.length, tier: files.length === 1 ? 1 : 2, validation_passed: validation.passed }),
+          };
+
+        } catch (err) {
+          // --- Conflict handling ---
+          if (err.status === 409 && err.conflictData) {
+            return {
+              action: "write",
+              result: {
+                status: "conflict",
+                error: err.message,
+                conflict: err.conflictData,
+                validation,
+              },
+              assistant_text: `${err.conflictData.guidance || err.message}`,
+              debug: makeDebug({ files_count: files.length }),
+            };
+          }
+
+          // --- Network failure: preserve content so it's not lost ---
+          const preserved = err.retryFailed
+            ? files.map((f) => ({ path: f.path, content: f.content }))
+            : undefined;
+
+          return {
+            action: "write",
+            result: {
+              error: err.message,
+              validation,
+              preserved_content: preserved,
+            },
+            assistant_text: `Write failed: ${err.message}${preserved ? "\n\nFile contents have been preserved in the response so they are not lost." : ""}${!validation.passed ? "\nValidation warnings: " + validation.results.map((r) => r.checks.filter((c) => !c.passed).map((c) => c.name).join(", ")).filter((x) => x).join("; ") : ""}`,
+            debug: makeDebug({ files_count: files.length }),
+          };
+        }
+      }
+
       default:
         return {
           action: "error",

diff --git a/src/core/tool-registry.js b/src/core/tool-registry.js
--- a/src/core/tool-registry.js
+++ b/src/core/tool-registry.js
@@ -265,15 +265,86 @@
     annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: true, openWorldHint: false },
     cliFlags: {},
   },
+  {
+    name: "write",
+    mcpName: "oddkit_write",
+    description: "Write files to the GitHub repo. Accepts file path(s), content, commit message. Validates against governance constraints. Single-file uses Contents API; multi-file uses Git Data API for atomic commits. Supports branches and PRs optionally.",
+    inputSchema: {
+      type: "object",
+      properties: {
+        files: {
+          type: "array",
+          items: {
+            type: "object",
+            properties: {
+              path: { type: "string", description: "Repo-relative file path (e.g., docs/decisions/D0017.md)" },
+              content: { type: "string", description: "File content (UTF-8 text)" },
+              encoding: { type: "string", description: "Content encoding. Default: \"utf-8\". Reserved for future: \"base64\" for binary files." },
+            },
+            required: ["path", "content"],
+          },
+          description: "Array of files to write",
+        },
+        message: { type: "string", description: "Commit message" },
+        branch: { type: "string", description: "Optional: target branch. If omitted, writes to default branch. If provided and doesn't exist, creates it from default branch HEAD." },
+        pr: {
+          oneOf: [
+            { type: "boolean" },
+            {
+              type: "object",
+              properties: {
+                title: { type: "string", description: "PR title. Default: commit message." },
+                body: { type: "string", description: "PR body. Default: auto-generated from file list." },
+                draft: { type: "boolean", description: "Open as draft PR. Default: false." },
+              },
+            },
+          ],
+          description: "Optional: if true or object, opens a PR from branch to default branch.",
+        },
+        repo: { type: "string", description: "Optional: GitHub repo (owner/repo). Defaults to baseline repo." },
+        author: {
+          type: "object",
+          properties: {
+            name: { type: "string", description: "Git author name." },
+            email: { type: "string", description: "Git author email." },
+          },
+          required: ["name", "email"],
+          description: "Optional: git author override. Default: authenticated user.",
+        },
+        provenance: {
+          type: "object",
+          properties: {
+            session_id: { type: "string", description: "Session identifier for traceability." },
+            surface: { type: "string", description: "Calling surface: \"claude-chat\", \"voice-agent\", \"team-chat\", \"cli\", \"mcp\", etc." },
+          },
+          description: "Optional: traceability metadata stored in commit message footer.",
+        },
+      },
+      required: ["files", "message"],
+    },
+    annotations: { readOnlyHint: false, destructiveHint: true, idempotentHint: false, openWorldHint: false },
+    cliFlags: {
+      files: { flag: "--files <json>", description: "JSON array of {path, content} objects" },
+      commitMessage: { flag: "--commit-message <text>", description: "Commit message", required: true },
+      branch: { flag: "--branch <name>", description: "Optional branch name" },
+      pr: { flag: "--pr", description: "Open PR after commit" },
+      repo: { flag: "--repo-target <owner/repo>", description: "Target GitHub repo (owner/repo)" },
+      authorName: { flag: "--author-name <name>", description: "Git author name" },
+      authorEmail: { flag: "--author-email <email>", description: "Git author email" },
+    },
+  },
 ];
 
 // ──────────────────────────────────────────────────────────────────────────────
 // Derived constants — single source of truth is TOOLS above
 // ──────────────────────────────────────────────────────────────────────────────
 
-/** Canonical list of action names, derived from TOOLS. */
-export const ACTION_NAMES = TOOLS.map((t) => t.name);
+/** All action names — used as the routing allowlist in handleAction. */
+export const ALL_ACTION_NAMES = TOOLS.map((t) => t.name);
 
+/** Orchestrator-only action names (excludes write — it has its own dedicated MCP tool with correct schema). */
+export const ACTION_NAMES = TOOLS.filter((t) => t.name !== "write").map((t) => t.name);
+
 /** Orchestrator tool definition with action enum derived from TOOLS. */
 export const ORCHESTRATOR_TOOL = buildOrchestratorTool(ACTION_NAMES);
 

diff --git a/src/mcp/server.js b/src/mcp/server.js
--- a/src/mcp/server.js
+++ b/src/mcp/server.js
@@ -192,6 +192,14 @@
         state: args.state,
         baseline: args.canon_url,
         include_metadata: args.include_metadata,
+        files: args.files,
+        message: args.message,
+        branch: args.branch,
+        pr: args.pr,
+        repo: args.repo,
+        author: args.author,
+        provenance: args.provenance,
+        surface: "mcp",
       });
       return {
         content: [{ type: "text", text: JSON.stringify(result, null, 2) }],
@@ -209,7 +217,14 @@
         canon_url: args.canon_url,
         baseline: args.canon_url,
         include_metadata: args.include_metadata,
-        // No state for individual tools
+        files: args.files,
+        message: args.message,
+        branch: args.branch,
+        pr: args.pr,
+        repo: args.repo,
+        author: args.author,
+        provenance: args.provenance,
+        surface: "mcp",
       });
       return {
         content: [{ type: "text", text: JSON.stringify(result, null, 2) }],

diff --git a/src/utils/githubApi.js b/src/utils/githubApi.js
new file mode 100644
--- /dev/null
+++ b/src/utils/githubApi.js
@@ -1,0 +1,431 @@
+/**
+ * GitHub API helper for oddkit_write
+ *
+ * Provides GitHub REST API interactions for writing files to repos.
+ * Three tiers:
+ *   Tier 1: Contents API (single file)
+ *   Tier 2: Git Data API (multi-file atomic commits)
+ *   Tier 3: Branches and PRs (layers on top of Tier 1/2)
+ *
+ * Uses Node.js native fetch (available in Node 18+).
+ */
+
+const GITHUB_API_BASE = "https://api.github.com";
+
+/**
+ * Get GitHub token from environment
+ */
+function getGitHubToken() {
+  const token = process.env.ODDKIT_GITHUB_TOKEN || process.env.GITHUB_TOKEN;
+  if (!token) {
+    throw new Error("GitHub token not configured. Set ODDKIT_GITHUB_TOKEN environment variable.");
+  }
+  return token;
+}
+
+/**
+ * Parse baseline_url to extract owner and repo
+ * e.g., https://raw.githubusercontent.com/klappy/klappy.dev/main -> { owner: "klappy", repo: "klappy.dev" }
+ */
+export function parseBaselineUrl(baselineUrl) {
+  let match;
+
+  // Try raw.githubusercontent.com format
+  match = baselineUrl.match(/raw\.githubusercontent\.com\/([^/]+)\/([^/]+)/);
+  if (match) {
+    return { owner: match[1], repo: match[2] };
+  }
+
+  // Try github.com format
+  match = baselineUrl.match(/github\.com\/([^/]+)\/([^/]+?)(?:\.git)?$/);
+  if (match) {
+    return { owner: match[1], repo: match[2] };
+  }
+
+  throw new Error(`Could not parse owner/repo from baseline_url: ${baselineUrl}`);
+}
+
+// ──────────────────────────────────────────────────────────────────────────────
+// Core request with retry
+// ──────────────────────────────────────────────────────────────────────────────
+
+/**
+ * Check if an error is a network error (not an HTTP status error).
+ * Network errors warrant retry; HTTP 4xx/5xx do not.
+ */
+function isNetworkError(err) {
+  if (err && err.type === "system") return true;
+  const msg = err?.message || "";
+  return (
+    msg.includes("fetch failed") ||
+    msg.includes("ECONNRESET") ||
+    msg.includes("ECONNREFUSED") ||
+    msg.includes("ETIMEDOUT") ||
+    msg.includes("ENOTFOUND") ||
+    msg.includes("network") ||
+    msg.includes("socket hang up")
+  );
+}
+
+/**
+ * Make authenticated GitHub API request with one retry on network errors.
+ *
+ * - Retries once on network failures (not on 4xx/5xx).
+ * - On 409 Conflict, throws a ConflictError with response details.
+ * - On auth/permission errors, throws descriptive messages.
+ */
+async function githubRequest(endpoint, options = {}) {
+  const token = getGitHubToken();
+  const url = `${GITHUB_API_BASE}${endpoint}`;
+
+  const fetchOptions = {
+    ...options,
+    headers: {
+      "Authorization": `Bearer ${token}`,
+      "Accept": "application/vnd.github+json",
+      "Content-Type": "application/json",
+      "X-GitHub-Api-Version": "2022-11-28",
+      ...options.headers,
+    },
+  };
+
+  async function attempt() {
+    const response = await fetch(url, fetchOptions);
+
+    if (!response.ok) {
+      const errorBody = await response.json().catch(() => ({}));
+
+      if (response.status === 401) {
+        throw new Error("GitHub token is invalid or expired. Check ODDKIT_GITHUB_TOKEN.");
+      }
+      if (response.status === 403) {
+        throw new Error(`Token doesn't have write access. The token needs \`repo\` scope. GitHub says: ${errorBody.message || ""}`);
+      }
+      if (response.status === 409) {
+        const err = new Error(`Conflict: ${errorBody.message || "resource was modified"}`);
+        err.status = 409;
+        err.body = errorBody;
+        throw err;
+      }
+
+      throw new Error(`GitHub API error: ${response.status} ${response.statusText} - ${errorBody.message || ""}`);
+    }
+
+    if (response.status === 204) return null;
+    return response.json();
+  }
+
+  // First attempt
+  try {
+    return await attempt();
+  } catch (err) {
+    // Only retry on network errors, not HTTP status errors
+    if (isNetworkError(err)) {
+      try {
+        return await attempt();
+      } catch (retryErr) {
+        // Final failure — attach the file contents context for callers
+        retryErr.retryFailed = true;
+        throw retryErr;
+      }
+    }
+    throw err;
+  }
+}
+
+// ──────────────────────────────────────────────────────────────────────────────
+// Tier 1: Contents API (single file)
+// ──────────────────────────────────────────────────────────────────────────────
+
+/**
+ * Get current file SHA (required for updates via Contents API).
+ * Returns null if the file doesn't exist.
+ */
+export async function getFileSha(owner, repo, path, branch = null) {
+  const endpoint = `/repos/${owner}/${repo}/contents/${path}${branch ? `?ref=${branch}` : ""}`;
+  try {
+    const data = await githubRequest(endpoint);
+    return data.sha;
+  } catch (err) {
+    if (err.message.includes("404")) return null;
+    throw err;
+  }
+}
+
+/**
+ * Get current file content and SHA (for conflict resolution).
+ * Returns { sha, content } or null if file doesn't exist.
+ */
+export async function getFileContent(owner, repo, path, branch = null) {
+  const endpoint = `/repos/${owner}/${repo}/contents/${path}${branch ? `?ref=${branch}` : ""}`;
+  try {
+    const data = await githubRequest(endpoint);
+    return {
+      sha: data.sha,
+      content: Buffer.from(data.content, "base64").toString("utf-8"),
+    };
+  } catch (err) {
+    if (err.message.includes("404")) return null;
+    throw err;
+  }
+}
+
+/**
+ * Write a single file via Contents API (Tier 1).
+ * Supports optional author override.
+ */
+export async function writeFile(owner, repo, path, content, message, branch = null, sha = null, author = null) {
+  const encodedContent = Buffer.from(content, "utf-8").toString("base64");
+
+  const body = { message, content: encodedContent };
+  if (sha) body.sha = sha;
+  if (branch) body.branch = branch;
+  if (author) {
+    body.committer = { name: author.name, email: author.email };
+    body.author = { name: author.name, email: author.email };
+  }
+
+  const endpoint = `/repos/${owner}/${repo}/contents/${path}`;
+
+  try {
+    const data = await githubRequest(endpoint, {
+      method: "PUT",
+      body: JSON.stringify(body),
+    });
+
+    return {
+      commit_sha: data.commit.sha,
+      commit_url: data.commit.html_url,
+      file_path: data.content.path,
+      sha: data.content.sha,
+    };
+  } catch (err) {
+    // Handle 409 Conflict — file was modified since last read
+    if (err.status === 409) {
+      const current = await getFileContent(owner, repo, path, branch);
+      const conflictErr = new Error("Merge conflict: the file was modified since you last read it.");
+      conflictErr.status = 409;
+      conflictErr.conflictData = {
+        path,
+        current_sha: current?.sha || null,
+        current_content: current?.content || null,
+        your_content: content,
+        guidance: "The file was modified since you last read it. Here's the current version — want to merge your changes?",
+      };
+      throw conflictErr;
+    }
+    throw err;
+  }
+}
+
+// ──────────────────────────────────────────────────────────────────────────────
+// Tier 2: Git Data API (multi-file atomic commits)
+// ──────────────────────────────────────────────────────────────────────────────
+
+/**
+ * Get a git ref (branch HEAD SHA).
+ * Returns { sha, url } for the ref object.
+ */
+export async function getRef(owner, repo, branch) {
+  const data = await githubRequest(`/repos/${owner}/${repo}/git/ref/heads/${branch}`);
+  return {
+    sha: data.object.sha,
+    url: data.object.url,
+  };
+}
+
+/**
+ * Get a commit's tree SHA.
+ */
+export async function getCommitTree(owner, repo, commitSha) {
+  const data = await githubRequest(`/repos/${owner}/${repo}/git/commits/${commitSha}`);
+  return data.tree.sha;
+}
+
+/**
+ * Create a blob for a file.
+ */
+export async function createBlob(owner, repo, content, encoding = "utf-8") {
+  const data = await githubRequest(`/repos/${owner}/${repo}/git/blobs`, {
+    method: "POST",
+    body: JSON.stringify({ content, encoding }),
+  });
+  return data.sha;
+}
+
+/**
+ * Create a tree with multiple file entries.
+ * @param {string} baseTreeSha - The base tree to build on
+ * @param {Array<{path: string, blobSha: string}>} entries - File entries
+ */
+export async function createTree(owner, repo, baseTreeSha, entries) {
+  const tree = entries.map((e) => ({
+    path: e.path,
+    mode: "100644", // regular file
+    type: "blob",
+    sha: e.blobSha,
+  }));
+
+  const data = await githubRequest(`/repos/${owner}/${repo}/git/trees`, {
+    method: "POST",
+    body: JSON.stringify({ base_tree: baseTreeSha, tree }),
+  });
+  return data.sha;
+}
+
+/**
+ * Create a commit.
+ * @param {Object} opts
+ * @param {string} opts.treeSha - Tree SHA to commit
+ * @param {string[]} opts.parentShas - Parent commit SHAs
+ * @param {string} opts.message - Commit message
+ * @param {Object} [opts.author] - Optional author { name, email }
+ */
+export async function createCommit(owner, repo, { treeSha, parentShas, message, author }) {
+  const body = {
+    message,
+    tree: treeSha,
+    parents: parentShas,
+  };
+  if (author) {
+    body.author = { name: author.name, email: author.email };
+    body.committer = { name: author.name, email: author.email };
+  }
+
+  const data = await githubRequest(`/repos/${owner}/${repo}/git/commits`, {
+    method: "POST",
+    body: JSON.stringify(body),
+  });
+  return {
+    sha: data.sha,
+    url: data.html_url,
+  };
+}
+
+/**
+ * Update a branch ref to point to a new commit.
+ * Handles 409 conflict (branch was updated concurrently).
+ */
+export async function updateRef(owner, repo, branch, commitSha) {
+  try {
+    await githubRequest(`/repos/${owner}/${repo}/git/refs/heads/${branch}`, {
+      method: "PATCH",
+      body: JSON.stringify({ sha: commitSha, force: false }),
+    });
+  } catch (err) {
+    if (err.status === 409) {
+      const conflictErr = new Error("Merge conflict: the branch was updated since the commit was created.");
+      conflictErr.status = 409;
+      conflictErr.conflictData = {
+        branch,
+        attempted_sha: commitSha,
+        guidance: "The branch was updated concurrently. Fetch the latest and retry.",
+      };
+      throw conflictErr;
+    }
+    throw err;
+  }
+}
+
+/**
+ * Perform an atomic multi-file commit via the Git Data API.
+ *
+ * Flow:
+ *   1. GET ref → commit SHA → tree SHA
+ *   2. Create blobs for each file
+ *   3. Create new tree with all blobs
+ *   4. Create commit pointing to new tree
+ *   5. Update branch ref
+ *
+ * @param {string} owner
+ * @param {string} repo
... diff truncated: showing 800 of 1038 lines

Comment thread src/utils/writeValidation.js Outdated
Comment thread src/core/actions.js
Bug 1: Replace no-op Buffer.from try/catch with real unpaired surrogate
detection. Buffer.from(string, utf-8) never throws for JS strings, making
the previous check dead code. The new regex detects lone surrogates which
are valid in JS strings but not valid UTF-8.

Bug 2: Wrap PR creation in its own try/catch so a PR failure (network,
permissions, rate limit) does not lose the successful commit metadata.
Previously commitResult was scoped inside try and inaccessible from catch,
causing the error response to report Write failed with no commit info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants