From f2aae26c9655cacb7a4ad621a5208cea9cc42794 Mon Sep 17 00:00:00 2001 From: Ersan Bilik Date: Fri, 23 Oct 2026 23:25:57 +0300 Subject: [PATCH 1/6] fix(mcp): advertise every field entry validation requires Entry validation (internal/entry/validate.go) requires session_id, branch, and commit for tasks, decisions, and learnings unless .ctxrc relaxes them, and section for tasks. extract.Opts reads all of them from the MCP arguments, but EntryAttrProps never declared the provenance fields, and the ctx_watch_update schema declared neither section nor priority. MCP clients only send what a tool schema advertises, so every ctx_add and ctx_watch_update call for those entry types failed with "missing required fields: --session-id, --branch, --commit" (#190). Declare section, priority, session_id, branch, and commit in EntryAttrProps, so both entry tools take them from one definition, and test that both schemas advertise every field validation can require. Spec: specs/fix-mcp-add-provenance.md Signed-off-by: Ersan Bilik --- internal/assets/commands/text/mcp.yaml | 6 + internal/config/embed/text/mcp_tool.go | 7 + internal/mcp/server/def/tool/prop.go | 38 ++++- internal/mcp/server/def/tool/tool.go | 11 -- internal/mcp/server/def/tool/tool_test.go | 31 +++++ specs/fix-mcp-add-provenance.md | 160 ++++++++++++++++++++++ 6 files changed, 239 insertions(+), 14 deletions(-) create mode 100644 specs/fix-mcp-add-provenance.md diff --git a/internal/assets/commands/text/mcp.yaml b/internal/assets/commands/text/mcp.yaml index 847658bab..078c62c2e 100644 --- a/internal/assets/commands/text/mcp.yaml +++ b/internal/assets/commands/text/mcp.yaml @@ -102,6 +102,12 @@ mcp.tool-drift-desc: short: 'Detect stale or invalid context: dead paths, missing files, staleness' mcp.tool-prop-application: short: How to apply this lesson (required for learnings) +mcp.tool-prop-session-id: + short: AI session ID for provenance (required for tasks, decisions, and learnings) +mcp.tool-prop-branch: + short: Git branch name for provenance (required for tasks, decisions, and learnings) +mcp.tool-prop-commit: + short: Git commit hash for provenance (required for tasks, decisions, and learnings) mcp.tool-prop-consequence: short: Consequence (required for decisions) mcp.tool-prop-content: diff --git a/internal/config/embed/text/mcp_tool.go b/internal/config/embed/text/mcp_tool.go index 6288c0eb0..d3f207fa1 100644 --- a/internal/config/embed/text/mcp_tool.go +++ b/internal/config/embed/text/mcp_tool.go @@ -60,6 +60,13 @@ const ( // DescKeyMCPToolPropApplication is the text key for mcp tool prop application // messages. DescKeyMCPToolPropApplication = "mcp.tool-prop-application" + // DescKeyMCPToolPropSessionID is the text key for mcp tool prop session id + // messages. + DescKeyMCPToolPropSessionID = "mcp.tool-prop-session-id" + // DescKeyMCPToolPropBranch is the text key for mcp tool prop branch messages. + DescKeyMCPToolPropBranch = "mcp.tool-prop-branch" + // DescKeyMCPToolPropCommit is the text key for mcp tool prop commit messages. + DescKeyMCPToolPropCommit = "mcp.tool-prop-commit" // DescKeyMCPToolPropQuery is the text key for mcp tool prop query messages. DescKeyMCPToolPropQuery = "mcp.tool-prop-query" // DescKeyMCPToolPropLimit is the text key for mcp tool prop limit messages. diff --git a/internal/mcp/server/def/tool/prop.go b/internal/mcp/server/def/tool/prop.go index e5b19ef32..eca5e2eb1 100644 --- a/internal/mcp/server/def/tool/prop.go +++ b/internal/mcp/server/def/tool/prop.go @@ -10,6 +10,8 @@ import ( "github.com/ActiveMemory/ctx/internal/assets/read/desc" "github.com/ActiveMemory/ctx/internal/config/cli" "github.com/ActiveMemory/ctx/internal/config/embed/text" + "github.com/ActiveMemory/ctx/internal/config/entry" + "github.com/ActiveMemory/ctx/internal/config/mcp/field" "github.com/ActiveMemory/ctx/internal/config/mcp/schema" "github.com/ActiveMemory/ctx/internal/mcp/proto" ) @@ -29,17 +31,21 @@ func MergeProps(dst, src map[string]proto.Property) map[string]proto.Property { return dst } -// EntryAttrProps returns the five entry-attribute properties shared by +// EntryAttrProps returns the entry-attribute properties shared by // tools that accept structured context entries (add, watch-update). // contextKey selects the DescKey for the "context" field since -// each tool describes it differently. +// each tool describes it differently. The task placement and +// provenance fields are declared here because entry validation +// requires them, and a client can only send what the schema +// advertises. // // Parameters: // - contextKey: DescKey selecting the description for the context field // // Returns: // - map[string]proto.Property: property map with context, rationale, -// consequence, lesson, and application fields +// consequence, lesson, application, section, priority, +// session_id, branch, and commit fields func EntryAttrProps(contextKey string) map[string]proto.Property { return map[string]proto.Property{ cli.AttrContext: { @@ -66,5 +72,31 @@ func EntryAttrProps(contextKey string) map[string]proto.Property { Description: desc.Text( text.DescKeyMCPToolPropApplication), }, + field.Section: { + Type: schema.String, + Description: desc.Text( + text.DescKeyMCPToolPropSection), + }, + field.Priority: { + Type: schema.String, + Description: desc.Text( + text.DescKeyMCPToolPropPriority), + Enum: entry.Priorities, + }, + field.SessionID: { + Type: schema.String, + Description: desc.Text( + text.DescKeyMCPToolPropSessionID), + }, + field.Branch: { + Type: schema.String, + Description: desc.Text( + text.DescKeyMCPToolPropBranch), + }, + field.Commit: { + Type: schema.String, + Description: desc.Text( + text.DescKeyMCPToolPropCommit), + }, } } diff --git a/internal/mcp/server/def/tool/tool.go b/internal/mcp/server/def/tool/tool.go index 5c40218a0..672b854eb 100644 --- a/internal/mcp/server/def/tool/tool.go +++ b/internal/mcp/server/def/tool/tool.go @@ -56,17 +56,6 @@ func Defs() []proto.Tool { Description: desc.Text( text.DescKeyMCPToolPropContent), }, - field.Priority: { - Type: schema.String, - Description: desc.Text( - text.DescKeyMCPToolPropPriority), - Enum: entry.Priorities, - }, - field.Section: { - Type: schema.String, - Description: desc.Text( - text.DescKeyMCPToolPropSection), - }, }, EntryAttrProps( text.DescKeyMCPToolPropContext)), Required: []string{cli.AttrType, field.Content}, diff --git a/internal/mcp/server/def/tool/tool_test.go b/internal/mcp/server/def/tool/tool_test.go index 14fce6a08..a0a36ca30 100644 --- a/internal/mcp/server/def/tool/tool_test.go +++ b/internal/mcp/server/def/tool/tool_test.go @@ -9,6 +9,8 @@ package tool import ( "testing" + "github.com/ActiveMemory/ctx/internal/config/cli" + "github.com/ActiveMemory/ctx/internal/config/mcp/field" cfgMcpTool "github.com/ActiveMemory/ctx/internal/config/mcp/tool" "github.com/ActiveMemory/ctx/internal/mcp/proto" ) @@ -107,6 +109,35 @@ func TestDefsAddRequiredFields(t *testing.T) { t.Error("add tool not found in Defs") } +// Entry validation (internal/entry/validate.go) can require each of +// these fields, and the handlers read priority alongside them. A +// client can only send what a schema advertises, so both entry tools +// must declare every one. +func TestDefsEntryToolsAdvertiseEntryFields(t *testing.T) { + missing := map[string]bool{ + cfgMcpTool.Add: true, cfgMcpTool.WatchUpdate: true, + } + for _, d := range Defs() { + if !missing[d.Name] { + continue + } + delete(missing, d.Name) + for _, key := range []string{ + field.Section, field.Priority, + field.SessionID, field.Branch, field.Commit, + cli.AttrContext, cli.AttrRationale, cli.AttrConsequence, + cli.AttrLesson, cli.AttrApplication, + } { + if _, ok := d.InputSchema.Properties[key]; !ok { + t.Errorf("%s schema missing prop: %s", d.Name, key) + } + } + } + for name := range missing { + t.Errorf("entry tool %s not found in Defs", name) + } +} + func TestDefsMergeProps(t *testing.T) { dst := map[string]proto.Property{ "a": {Type: "string"}, diff --git a/specs/fix-mcp-add-provenance.md b/specs/fix-mcp-add-provenance.md new file mode 100644 index 000000000..e3d7af8fb --- /dev/null +++ b/specs/fix-mcp-add-provenance.md @@ -0,0 +1,160 @@ +# Spec: MCP entry tools accept provenance + +Issue: ActiveMemory/ctx#190. + +`ctx_add` and `ctx_watch_update` cannot add a task, decision, or +learning for any schema-driven MCP client in a project without a +relaxing `.ctxrc`: entry validation demands fields the tool schemas +never advertise. + +## Problem + +### A. Provenance is required but not advertised + +`entry.Validate` (`internal/entry/validate.go:44-58`) requires +`session_id`, `branch`, and `commit` for tasks, decisions, and +learnings unless `.ctxrc` `provenance_required` relaxes them; the +`rc.Provenance*Required` accessors default to true. The MCP layer +reads the three values only from tool arguments (`extract.Opts`), but +`EntryAttrProps` (`internal/mcp/server/def/tool/prop.go`) never +declares them. MCP clients send only what a schema advertises, so +every such call fails: + + learning missing required fields: --session-id, --branch, --commit + +`ctx_watch_update` shares `EntryAttrProps` and the same validation, +and fails identically. + +### B. `ctx_watch_update` does not advertise `section` or `priority` + +Validation also requires `section` for tasks (`validate.go:62-65`). +`ctx_add` advertises `section` and `priority`; the `ctx_watch_update` +schema (`internal/mcp/server/def/tool/tool.go`) declares neither, +although its handler reads both. `ctx_watch_update type=task` fails +the same way for schema-driven clients: same defect class as A. + +### C. What an MCP client can supply is weak, absent, or unsafe + +- `session_id`: only the client can know it. Claude Code's hook relay + puts the real ID in the agent's context on every prompt + (`Session: … | Branch: … @ …`); clients without a relay have none. + The server has no session identity to fall back on: `MCPSession` + holds governance counters only, and `ctx_sessionevent start` merely + resets them (`internal/mcp/handler/tool.go:531-533`). +- `branch` / `commit`: the server can resolve them from git itself, + and its values are better than relayed ones: read at write time (a + relayed commit predates any commit the agent made since the prompt) + and never retyped by a model. +- `SanitizedOpts` (`internal/mcp/server/extract/extract.go`) promises + sanitization for every text field but leaves `priority`, `section`, + `branch`, and `commit` raw. `format.Task` writes priority, branch, + and commit into the task line, and `insert.TaskAfterSection` writes + an unmatched section as a new `### ` heading, so a line break in any + of them forges task lines or headings in `TASKS.md`. Advertising + `branch` and `commit` makes schema-driven clients send them. + +## Approach + +MCP layer only. The CLI, `entry.Validate`, and `.ctxrc` are unchanged. + +1. **Advertise** `section`, `priority`, `session_id`, `branch`, and + `commit` in `EntryAttrProps`, so both entry tools declare every + field validation can require from one definition. +2. **Resolve the git half server-side.** In the route layer + (`internal/mcp/server/route/tool/tool.go`), `add` and `watchUpdate` + build their options through one helper that wraps + `extract.SanitizedOpts` and fills `Branch` and `Commit` when they + are empty or `unknown`: `git.CurrentBranch()` and `git.ShortHead()`, + falling back to `unknown`. These are the values the hook relay + emits (`provenance.Emit`), so MCP-written provenance matches the + CLI-plus-relay path, including `unknown` when git cannot answer. + The fallback is `cmp.Or`; importing `provenance.DefaultVal` would + pull cobra into the MCP route for a one-line default. An explicit + value wins, for an agent working in another checkout than the + server's cwd. No cache: caching would freeze `commit` at first use, + the staleness this step removes. +3. **Flatten single-line fields.** A new `sanitize.Line(s, maxLen)` + strips every control character, including the tab, CR, and LF that + `StripControl` keeps for multi-line content, and caps the length on + a rune boundary. `SanitizedOpts` applies it to `priority`, + `section`, `branch`, and `commit`, capped by a new + `cfg.MaxOptsLineLen` (256) in the MCP-SAN.1 limits block. No + legitimate value contains a control character, so none changes. + Over-long values are truncated, not rejected, as `sanitize.SessionID` + does. `sanitize.Reflect` does not fit: it keeps line breaks. +4. **`session_id` stays caller-supplied and required.** Its schema + description directs the agent to the relayed `Session:` value, or an + explicit `"unknown"` when it has none: no silent default and no + fabricated ID. This is not a relaxation: omitting the field still + fails, and only `.ctxrc` relaxes the gate + (`docs/recipes/knowledge-capture.md`). `"unknown"` is what the relay + itself emits without a session ID (`provenance.ShortSessionID`), and + the CLI already accepts it. +5. **Describe** `branch` and `commit` as optional over MCP: omit them + to let the server resolve them from git. +6. **Prompts.** The `ctx-decision-add` and `ctx-learning-add` footers + say "call ctx_add with … all fields above", and `session_id` is not + among those fields; the footers name it. +7. **Document** the missing argument rows for both tools in + `docs/cli/mcp.md`, and note in `docs/home/configuration.md` that + `provenance_required` governs the MCP entry tools too. + +| Caller sends | Task line stores | +|----------------------------------|----------------------------------------------------| +| all three values | the values as sent, flattened | +| `session_id` only | that `session_id` plus the server's branch/commit | +| `branch: "unknown"` | the server's branch | +| `branch: "x\n- [x] forged"` | `#branch:x- [x] forged`, on one line | +| no `session_id` | nothing: `missing required fields: --session-id` | +| git absent, or no commits yet | `unknown` for whichever git cannot resolve | +| detached HEAD | `#branch:unknown`, as the relay emits | + +## Tests + +Each test below fails before its fix, except the explicit-values +case, which guards the "explicit value wins" rule: + +- `internal/mcp/server/def/tool`: both entry tools advertise + `section`, `priority`, `session_id`, `branch`, `commit`, `context`, + `rationale`, `consequence`, `lesson`, and `application`. +- `internal/sanitize`: `Line` drops line breaks, tabs, and U+2028, + keeps spaces and ref characters (`feat/x.y_z-1`), and caps length. +- `internal/mcp/server/extract`: `SanitizedOpts` flattens and caps + `priority`, `section`, `branch`, and `commit`. +- `internal/mcp/server`, end to end through `tools/call`: + - in a temporary git repository, for both tools, + `session_id: "unknown"` with `branch: "unknown"` and no `commit` + stores the repository's branch and short HEAD, read from git + rather than assumed (the default branch name varies); + - explicit values are stored verbatim, even in a repository whose + real branch differs; + - the issue's probe (`type=learning`, no `branch`/`commit`) with + `session_id: "unknown"` succeeds; + - without `session_id` the call still fails, naming only + `--session-id`. + +## Acceptance + +- The issue's JSON-RPC probe with `"session_id": "unknown"` added + returns `Added learning to LEARNINGS.md` on a fresh `ctx init`. +- `tools/list` shows `section`, `priority`, `session_id`, `branch`, + and `commit` on both entry tools. +- `golangci-lint run` is clean, and there are no new test failures + against a clean `upstream/main`. + +## Non-Goals + +- **Persisting provenance for decisions and learnings.** `entry.Write` + passes session/branch/commit only to `format.Task` + (`internal/entry/write.go:93-97`); `format.Decision` and + `format.Learning` take none, so `DECISIONS.md` and `LEARNINGS.md` + carry no provenance on any path. Persisting it, or narrowing the + gate to tasks, is a maintainer decision raised on #190. +- A server-minted session ID, or a server-side `unknown` default for + `session_id`. The latter is offered on #190 as a one-line + alternative. +- MCP-shaped field names in validation errors (`--session-id`): the + validator is shared with the CLI. +- Changing CLI provenance behavior or flag sanitization, `.ctxrc` + `provenance_required`, or the `ctx mcp serve` exemption from the + git gate. From c8e2ec8bce574ca032edfd54baf48fcb60c0aa9c Mon Sep 17 00:00:00 2001 From: Ersan Bilik Date: Fri, 23 Oct 2026 23:32:32 +0300 Subject: [PATCH 2/6] fix(mcp): resolve entry branch and commit from git Only the client knows its session ID, but the server can read branch and commit itself, and its values beat relayed ones: read at write time (a relayed commit predates any commit made since the prompt) and never retyped by a model. ctx_add and ctx_watch_update now fill branch and commit when the caller leaves them empty or "unknown", with the git calls the hook relay makes in provenance.Emit, falling back to "unknown" as it does, so MCP-written provenance matches the CLI path. An explicit value still wins, for an agent working in another checkout. session_id stays caller-supplied and required. Its description points the agent at the Session value ctx relays, or an explicit "unknown" when it has none, and the decision and learning prompts ask for it: their "all fields above" footers never listed it. Spec: specs/fix-mcp-add-provenance.md Signed-off-by: Ersan Bilik --- internal/assets/commands/text/mcp.yaml | 10 +- internal/mcp/server/provenance_test.go | 159 +++++++++++++++++++++++++ internal/mcp/server/route/tool/tool.go | 33 ++++- 3 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 internal/mcp/server/provenance_test.go diff --git a/internal/assets/commands/text/mcp.yaml b/internal/assets/commands/text/mcp.yaml index 078c62c2e..692a904e6 100644 --- a/internal/assets/commands/text/mcp.yaml +++ b/internal/assets/commands/text/mcp.yaml @@ -103,11 +103,11 @@ mcp.tool-drift-desc: mcp.tool-prop-application: short: How to apply this lesson (required for learnings) mcp.tool-prop-session-id: - short: AI session ID for provenance (required for tasks, decisions, and learnings) + short: 'AI session ID for provenance (required for tasks, decisions, and learnings): the Session value ctx relays into your context, or "unknown" if you have none' mcp.tool-prop-branch: - short: Git branch name for provenance (required for tasks, decisions, and learnings) + short: 'Git branch for provenance (omit to let the server resolve it from git; pass it only when working in another checkout)' mcp.tool-prop-commit: - short: Git commit hash for provenance (required for tasks, decisions, and learnings) + short: 'Git commit hash for provenance (omit to let the server resolve it from git; pass it only when working in another checkout)' mcp.tool-prop-consequence: short: Consequence (required for decisions) mcp.tool-prop-content: @@ -180,7 +180,7 @@ mcp.prompt-label-lesson: mcp.prompt-label-application: short: Application mcp.prompt-add-decision-footer: - short: Call ctx_add with type="decision" and all fields above. + short: Call ctx_add with type="decision", all fields above, and session_id. mcp.prompt-add-decision-header: short: 'Record this architectural decision using ctx_add:' mcp.prompt-add-decision-result-desc: @@ -190,7 +190,7 @@ mcp.prompt-add-learning-desc: mcp.prompt-add-learning-field-format: short: "- **%s**: %s\n" mcp.prompt-add-learning-footer: - short: Call ctx_add with type="learning" and all fields above. + short: Call ctx_add with type="learning", all fields above, and session_id. mcp.prompt-add-learning-header: short: 'Record this learning using ctx_add:' mcp.prompt-add-learning-result-desc: diff --git a/internal/mcp/server/provenance_test.go b/internal/mcp/server/provenance_test.go new file mode 100644 index 000000000..f233117b2 --- /dev/null +++ b/internal/mcp/server/provenance_test.go @@ -0,0 +1,159 @@ +// / ctx: https://ctx.ist +// ,'`./ do you remember? +// `.,'\ +// \ Copyright 2026-present Context contributors. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/ActiveMemory/ctx/internal/config/ctx" + "github.com/ActiveMemory/ctx/internal/mcp/proto" +) + +// callTool sends one tools/call request and returns its tool result. +func callTool( + t *testing.T, srv *Server, name string, args map[string]interface{}, +) proto.CallToolResult { + t.Helper() + resp := request(t, srv, "tools/call", proto.CallToolParams{ + Name: name, Arguments: args, + }) + if resp.Error != nil { + t.Fatalf("unexpected error: %v", resp.Error.Message) + } + raw, _ := json.Marshal(resp.Result) + var result proto.CallToolResult + if err := json.Unmarshal(raw, &result); err != nil { + t.Fatalf("unmarshal: %v", err) + } + return result +} + +// readContext returns the content of one context file. +func readContext(t *testing.T, contextDir, name string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(contextDir, name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + return string(b) +} + +// runGit runs git in dir and returns its trimmed output. +func runGit(t *testing.T, dir string, args ...string) string { + t.Helper() + //nolint:gosec // G204: test fixture, args are hardcoded by callers + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) +} + +// gitInit makes dir a repository with one commit, skipping the test +// when git is not installed. +func gitInit(t *testing.T, dir string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skipf("git not on PATH: %v", err) + } + runGit(t, dir, "init", "-q") + runGit(t, dir, "config", "user.email", "test@example.com") + runGit(t, dir, "config", "user.name", "Test User") + // A global commit.gpgsign must never reach for the GPG agent. + runGit(t, dir, "config", "commit.gpgsign", "false") + runGit(t, dir, "commit", "--allow-empty", "-q", "-m", "init") +} + +// Branch and commit come from git when the caller omits them or sends +// "unknown". Expected values are read from git, not assumed: the +// default branch name depends on the machine's git config. +func TestToolEntryResolvesGitProvenance(t *testing.T) { + for _, tool := range []string{"ctx_add", "ctx_watch_update"} { + t.Run(tool, func(t *testing.T) { + srv, contextDir := newTestServer(t) + root := filepath.Dir(contextDir) + gitInit(t, root) + result := callTool(t, srv, tool, map[string]interface{}{ + "type": "task", "content": "Resolved task", + "section": "Misc", "session_id": "unknown", + "branch": "unknown", + }) + if result.IsError { + t.Fatalf("unexpected tool error: %s", result.Content[0].Text) + } + want := "Resolved task #session:unknown" + + " #branch:" + runGit(t, root, "branch", "--show-current") + + " #commit:" + runGit(t, root, "rev-parse", "--short", "HEAD") + tasks := readContext(t, contextDir, ctx.Task) + if !strings.Contains(tasks, want) { + t.Errorf("want %q in TASKS.md, got:\n%s", want, tasks) + } + }) + } +} + +// An explicit value wins over the repository's own branch and commit. +func TestToolAddKeepsExplicitProvenance(t *testing.T) { + srv, contextDir := newTestServer(t) + gitInit(t, filepath.Dir(contextDir)) + result := callTool(t, srv, "ctx_add", map[string]interface{}{ + "type": "task", "content": "Explicit task", "section": "Misc", + "session_id": "test1234", "branch": "feature/x", + "commit": "abc1234", + }) + if result.IsError { + t.Fatalf("unexpected tool error: %s", result.Content[0].Text) + } + want := "Explicit task #session:test1234" + + " #branch:feature/x #commit:abc1234" + tasks := readContext(t, contextDir, ctx.Task) + if !strings.Contains(tasks, want) { + t.Errorf("want %q in TASKS.md, got:\n%s", want, tasks) + } +} + +// The probe from issue #190, from a client with no session ID to relay. +func TestToolAddLearningUnknownSession(t *testing.T) { + srv, contextDir := newTestServer(t) + result := callTool(t, srv, "ctx_add", map[string]interface{}{ + "type": "learning", "content": "Probe learning", + "context": "c", "lesson": "l", "application": "a", + "session_id": "unknown", + }) + if result.IsError { + t.Fatalf("unexpected tool error: %s", result.Content[0].Text) + } + if !strings.Contains( + readContext(t, contextDir, ctx.Learning), "Probe learning", + ) { + t.Error("learning not written to LEARNINGS.md") + } +} + +// The server resolves branch and commit, never the session ID. +func TestToolAddRequiresSessionID(t *testing.T) { + srv, _ := newTestServer(t) + result := callTool(t, srv, "ctx_add", map[string]interface{}{ + "type": "task", "content": "No session", "section": "Misc", + }) + if !result.IsError { + t.Fatal("expected a missing-field error without session_id") + } + text := result.Content[0].Text + if !strings.Contains(text, "--session-id") || + strings.Contains(text, "--branch") || + strings.Contains(text, "--commit") { + t.Errorf("want only --session-id reported missing, got: %s", text) + } +} diff --git a/internal/mcp/server/route/tool/tool.go b/internal/mcp/server/route/tool/tool.go index 55b381332..042d4ad00 100644 --- a/internal/mcp/server/route/tool/tool.go +++ b/internal/mcp/server/route/tool/tool.go @@ -7,6 +7,7 @@ package tool import ( + "cmp" "encoding/json" "fmt" "time" @@ -17,8 +18,10 @@ import ( "github.com/ActiveMemory/ctx/internal/config/entry" "github.com/ActiveMemory/ctx/internal/config/mcp/cfg" "github.com/ActiveMemory/ctx/internal/config/mcp/field" + cfgSession "github.com/ActiveMemory/ctx/internal/config/session" cfgTime "github.com/ActiveMemory/ctx/internal/config/time" "github.com/ActiveMemory/ctx/internal/entity" + "github.com/ActiveMemory/ctx/internal/exec/git" "github.com/ActiveMemory/ctx/internal/mcp/handler" "github.com/ActiveMemory/ctx/internal/mcp/proto" "github.com/ActiveMemory/ctx/internal/mcp/server/extract" @@ -54,7 +57,7 @@ func add( // MCP-SAN.3: Sanitize content before writing to .context/. content = sanitize.Content(content) - opts, optsErr := extract.SanitizedOpts(args) + opts, optsErr := entryOpts(args) if optsErr != nil { return out.ToolError(id, optsErr.Error()) } @@ -163,7 +166,7 @@ func watchUpdate( // MCP-SAN.3: Sanitize content before writing to .context/. content = sanitize.Content(content) - opts, optsErr := extract.SanitizedOpts(args) + opts, optsErr := entryOpts(args) if optsErr != nil { return out.ToolError(id, optsErr.Error()) } @@ -171,6 +174,32 @@ func watchUpdate( return out.ToolResult(id, t, updateErr) } +// entryOpts builds the sanitized entry options for [add] and +// [watchUpdate], resolving branch and commit from git when the caller +// left them empty or "unknown". The values are the ones the hook relay +// emits ([git.CurrentBranch], [git.ShortHead], "unknown" when git +// cannot answer), read at write time; an explicit value wins. +// +// Parameters: +// - args: MCP tool arguments with optional entry fields +// +// Returns: +// - entity.EntryOpts: sanitized options with branch and commit set +// - error: non-nil if any field exceeds its length limit +func entryOpts(args map[string]interface{}) (entity.EntryOpts, error) { + opts, sanitizeErr := extract.SanitizedOpts(args) + if sanitizeErr != nil { + return opts, sanitizeErr + } + if opts.Branch == "" || opts.Branch == cfgSession.IDUnknown { + opts.Branch = cmp.Or(git.CurrentBranch(), cfgSession.IDUnknown) + } + if opts.Commit == "" || opts.Commit == cfgSession.IDUnknown { + opts.Commit = cmp.Or(git.ShortHead(), cfgSession.IDUnknown) + } + return opts, nil +} + // compact extracts the archive flag and delegates to [handler.Compact]. // // Parameters: From a906ed1f9c8d0125924c577a11b221f88bea36e6 Mon Sep 17 00:00:00 2001 From: Ersan Bilik Date: Fri, 23 Oct 2026 23:40:55 +0300 Subject: [PATCH 3/6] fix(mcp): flatten single-line entry options at the boundary SanitizedOpts promised sanitization for every text field but passed priority, section, branch, and commit through raw. format.Task writes priority, branch, and commit into the task line, and an unmatched section becomes a new "### " heading, so a line break in any of them forged task lines or headings in TASKS.md. Advertising branch and commit makes schema-driven clients send them. sanitize.Line removes every control character, including the tab and line breaks that StripControl keeps for multi-line content, and the Unicode line and paragraph separators, then truncates on a rune boundary. SanitizedOpts applies it to the four fields with a new MaxOptsLineLen (256). Over-long values are truncated, not rejected, as session IDs already are: this is the length cap that specs/sanitize-hardening-followup.md deferred for these fields. Spec: specs/fix-mcp-add-provenance.md Signed-off-by: Ersan Bilik --- internal/config/mcp/cfg/config.go | 4 +++ internal/mcp/server/extract/extract.go | 10 +++++- internal/mcp/server/extract/extract_test.go | 32 +++++++++++++++++ internal/sanitize/doc.go | 5 +++ internal/sanitize/line.go | 38 +++++++++++++++++++++ internal/sanitize/line_test.go | 37 ++++++++++++++++++++ specs/fix-mcp-add-provenance.md | 5 ++- 7 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 internal/sanitize/line.go create mode 100644 internal/sanitize/line_test.go diff --git a/internal/config/mcp/cfg/config.go b/internal/config/mcp/cfg/config.go index d78df750c..a875f3eea 100644 --- a/internal/config/mcp/cfg/config.go +++ b/internal/config/mcp/cfg/config.go @@ -37,4 +37,8 @@ const ( // application). Tighter than MaxContentLen because these are // qualifiers, not primary content. MaxOptsFieldLen = 4_000 + // MaxOptsLineLen is the maximum byte length for single-line entry + // option fields (priority, section, branch, commit). Longer values + // are truncated, not rejected. + MaxOptsLineLen = 256 ) diff --git a/internal/mcp/server/extract/extract.go b/internal/mcp/server/extract/extract.go index 3f7ae8eb4..b141788f4 100644 --- a/internal/mcp/server/extract/extract.go +++ b/internal/mcp/server/extract/extract.go @@ -92,7 +92,9 @@ func Opts(args map[string]interface{}) entity.EntryOpts { // SanitizedOpts builds EntryOpts with content sanitization applied // to all text fields. Returns an error if any secondary field // exceeds [cfg.MaxOptsFieldLen]; length is checked on raw input -// before sanitization to prevent abuse via large payloads. +// before sanitization to prevent abuse via large payloads. The +// single-line fields (priority, section, branch, commit) are +// flattened to one line and truncated to [cfg.MaxOptsLineLen]. // // Parameters: // - args: MCP tool arguments with optional entry fields @@ -138,5 +140,11 @@ func SanitizedOpts( opts.Lesson = sanitize.Content(opts.Lesson) opts.Application = sanitize.Content(opts.Application) opts.SessionID = sanitize.SessionID(opts.SessionID) + // These land inside a task line or a section heading, where a + // line break would forge a new task or heading. + opts.Priority = sanitize.Line(opts.Priority, cfg.MaxOptsLineLen) + opts.Section = sanitize.Line(opts.Section, cfg.MaxOptsLineLen) + opts.Branch = sanitize.Line(opts.Branch, cfg.MaxOptsLineLen) + opts.Commit = sanitize.Line(opts.Commit, cfg.MaxOptsLineLen) return opts, nil } diff --git a/internal/mcp/server/extract/extract_test.go b/internal/mcp/server/extract/extract_test.go index 180bb418e..a43b6fd2d 100644 --- a/internal/mcp/server/extract/extract_test.go +++ b/internal/mcp/server/extract/extract_test.go @@ -181,6 +181,38 @@ func TestSanitizedOptsApplicationTooLong(t *testing.T) { } } +func TestSanitizedOptsFlattensLineFields(t *testing.T) { + // A line break in a single-line field would forge a task line or + // a section heading in TASKS.md. + forged := "x\n- [x] forged\r\n" + strings.Repeat("y", cfg.MaxOptsLineLen) + opts, err := SanitizedOpts(map[string]interface{}{ + "priority": forged, + "section": forged, + "branch": forged, + "commit": forged, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + for name, got := range map[string]string{ + "priority": opts.Priority, + "section": opts.Section, + "branch": opts.Branch, + "commit": opts.Commit, + } { + if strings.ContainsAny(got, "\r\n") { + t.Errorf("%s = %q, want a single line", name, got) + } + if len(got) > cfg.MaxOptsLineLen { + t.Errorf("%s length = %d, want <= %d", + name, len(got), cfg.MaxOptsLineLen) + } + if !strings.HasPrefix(got, "x- [x] forged") { + t.Errorf("%s = %q, want the text kept on one line", name, got) + } + } +} + func TestSanitizedOptsAtBoundary(t *testing.T) { // Exactly MaxOptsFieldLen bytes — must be accepted. args := map[string]interface{}{ diff --git a/internal/sanitize/doc.go b/internal/sanitize/doc.go index 2870164d3..e2800afe0 100644 --- a/internal/sanitize/doc.go +++ b/internal/sanitize/doc.go @@ -35,6 +35,11 @@ // a maximum length; used when reflecting untrusted // input back in error messages. // +// - [Line] flattens a single-line value: strips all +// control chars, line breaks and tabs included, +// and truncates to a maximum length; used for +// values written into a Markdown line or heading. +// // - [SessionID] converts an arbitrary string into a // path-safe session identifier: strips null bytes, // path traversal sequences, slashes, replaces diff --git a/internal/sanitize/line.go b/internal/sanitize/line.go new file mode 100644 index 000000000..ed7b71cd1 --- /dev/null +++ b/internal/sanitize/line.go @@ -0,0 +1,38 @@ +// / ctx: https://ctx.ist +// ,'`./ do you remember? +// `.,'\ +// \ Copyright 2026-present Context contributors. +// SPDX-License-Identifier: Apache-2.0 + +package sanitize + +import ( + "strings" + "unicode" + + cfgSan "github.com/ActiveMemory/ctx/internal/config/sanitize" +) + +// Line flattens s to a single line: it removes every control +// character, including the tab, line feed, and carriage return that +// [StripControl] keeps, and the Unicode line/paragraph separators, +// then truncates to maxLen bytes on a UTF-8 rune boundary. Used for +// single-line values written into a Markdown line or heading, where a +// line break would start a new one. +// +// Parameters: +// - s: untrusted single-line input +// - maxLen: maximum byte length; 0 or negative means no truncation +// +// Returns: +// - string: s on one line, control characters removed and length +// capped +func Line(s string, maxLen int) string { + return truncate(strings.Map(func(r rune) rune { + if unicode.IsControl(r) || + r == cfgSan.LineSeparator || r == cfgSan.ParagraphSeparator { + return -1 + } + return r + }, s), maxLen) +} diff --git a/internal/sanitize/line_test.go b/internal/sanitize/line_test.go new file mode 100644 index 000000000..7bf7f6531 --- /dev/null +++ b/internal/sanitize/line_test.go @@ -0,0 +1,37 @@ +// / ctx: https://ctx.ist +// ,'`./ do you remember? +// `.,'\ +// \ Copyright 2026-present Context contributors. +// SPDX-License-Identifier: Apache-2.0 + +package sanitize + +import ( + "testing" +) + +// TestLine tests the Line helper function. +func TestLine(t *testing.T) { + tests := []struct { + name string + input string + maxLen int + expected string + }{ + {"ref characters", "feat/x.y_z-1", 0, "feat/x.y_z-1"}, + {"spaces", "Phase 1", 0, "Phase 1"}, + {"line breaks", "x\n- [x] forged\r\n", 0, "x- [x] forged"}, + {"tab and separators", "a\tb
c
d", 0, "abcd"}, + {"length cap", "abcdef", 3, "abc"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := Line(tt.input, tt.maxLen) + if result != tt.expected { + t.Errorf("Line(%q, %d) = %q, want %q", + tt.input, tt.maxLen, result, tt.expected) + } + }) + } +} diff --git a/specs/fix-mcp-add-provenance.md b/specs/fix-mcp-add-provenance.md index e3d7af8fb..feceed515 100644 --- a/specs/fix-mcp-add-provenance.md +++ b/specs/fix-mcp-add-provenance.md @@ -81,7 +81,10 @@ MCP layer only. The CLI, `entry.Validate`, and `.ctxrc` are unchanged. `cfg.MaxOptsLineLen` (256) in the MCP-SAN.1 limits block. No legitimate value contains a control character, so none changes. Over-long values are truncated, not rejected, as `sanitize.SessionID` - does. `sanitize.Reflect` does not fit: it keeps line breaks. + does. `sanitize.Reflect` does not fit: it keeps line breaks. The + cap is the one `specs/sanitize-hardening-followup.md` deferred for + these fields ("tracked separately if needed"); advertising `branch` + and `commit` makes it needed. 4. **`session_id` stays caller-supplied and required.** Its schema description directs the agent to the relayed `Session:` value, or an explicit `"unknown"` when it has none: no silent default and no From 99d3195e7cd2d2314fd26f4d94fefc248f192432 Mon Sep 17 00:00:00 2001 From: Ersan Bilik Date: Fri, 23 Oct 2026 23:45:32 +0300 Subject: [PATCH 4/6] docs(mcp): document entry tool arguments and provenance docs/cli/mcp.md listed neither section nor the provenance arguments for ctx_add, and ctx_watch_update also lacked priority and section, although validation requires section for tasks and session_id for tasks, decisions, and learnings. Both tables now list every argument the tools advertise, with a note on what to pass for session_id and when branch and commit come from git. The provenance_required rows in the configuration reference now say how each key applies to the MCP entry tools. Spec: specs/fix-mcp-add-provenance.md Signed-off-by: Ersan Bilik --- docs/cli/mcp.md | 18 ++++++++++++++++++ docs/home/configuration.md | 6 +++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/cli/mcp.md b/docs/cli/mcp.md index 701133c5e..eafc4fc4e 100644 --- a/docs/cli/mcp.md +++ b/docs/cli/mcp.md @@ -150,11 +150,22 @@ Add a task, decision, learning, or convention to the context. | `type` | string | Yes | Entry type: task, decision, learning, convention | | `content` | string | Yes | Title or main content | | `priority` | string | No | Priority level (tasks only): high, medium, low | +| `section` | string | Conditional | Target section in TASKS.md (tasks only) | | `context` | string | Conditional | Context field (decisions and learnings) | | `rationale` | string | Conditional | Rationale (decisions only) | | `consequence` | string | Conditional | Consequence (decisions only) | | `lesson` | string | Conditional | Lesson learned (learnings only) | | `application` | string | Conditional | How to apply (learnings only) | +| `session_id` | string | Conditional | AI session ID (tasks, decisions, learnings) | +| `branch` | string | No | Git branch (default: resolved from git) | +| `commit` | string | No | Git commit hash (default: resolved from git) | + +Tasks, decisions, and learnings require `session_id` unless `.ctxrc` +[`provenance_required`](../home/configuration.md#option-reference) +relaxes it: pass the `Session` value ctx relays into the agent's +context, or `unknown` when there is none. When `branch` or `commit` is +omitted or `unknown`, the server reads it from git; pass it only when +working in another checkout. ### `ctx_complete` @@ -192,11 +203,18 @@ Human confirmation is required before calling. |---------------|--------|-------------|------------------------------------------------------------| | `type` | string | Yes | Entry type: task, decision, learning, convention, complete | | `content` | string | Yes | Main content | +| `priority` | string | No | Priority level (tasks only): high, medium, low | +| `section` | string | Conditional | Target section in TASKS.md (tasks only) | | `context` | string | Conditional | Context background (decisions/learnings) | | `rationale` | string | Conditional | Rationale (decisions only) | | `consequence` | string | Conditional | Consequence (decisions only) | | `lesson` | string | Conditional | Lesson learned (learnings only) | | `application` | string | Conditional | How to apply (learnings only) | +| `session_id` | string | Conditional | AI session ID (tasks, decisions, learnings) | +| `branch` | string | No | Git branch (default: resolved from git) | +| `commit` | string | No | Git commit hash (default: resolved from git) | + +Provenance works as for [`ctx_add`](#ctx_add). ### `ctx_compact` diff --git a/docs/home/configuration.md b/docs/home/configuration.md index 603eba20d..27d98c356 100644 --- a/docs/home/configuration.md +++ b/docs/home/configuration.md @@ -159,9 +159,9 @@ A commented `.ctxrc` showing all options and their defaults: | `hooks.enabled` | `bool` | `true` | Whether hook execution is enabled | | `statusline.enabled` | `bool` | `true` | Whether `ctx init` deploys the Claude Code status line (`ctx system statusline`) | | `statusline.show_cost` | `bool` | `true` | Whether the status line renders the session-cost (`$`) segment | -| `provenance_required.session_id` | `bool` | `true` | Require `--session-id` on `ctx add` for tasks, decisions, learnings | -| `provenance_required.branch` | `bool` | `true` | Require `--branch` on `ctx add` for tasks, decisions, learnings | -| `provenance_required.commit` | `bool` | `true` | Require `--commit` on `ctx add` for tasks, decisions, learnings | +| `provenance_required.session_id` | `bool` | `true` | Require `--session-id` on `ctx add`, and `session_id` on the MCP entry tools, for tasks, decisions, learnings | +| `provenance_required.branch` | `bool` | `true` | Require `--branch` on `ctx add` for tasks, decisions, learnings; the MCP entry tools resolve it from git | +| `provenance_required.commit` | `bool` | `true` | Require `--commit` on `ctx add` for tasks, decisions, learnings; the MCP entry tools resolve it from git | | `auto_prune_days` | `int` | `7` | Days before stale session-state files are auto-pruned on context load. Non-positive values fall back to the default (never prunes on `0` or negative) | | `agent_cooldown_minutes` | `int` | `10` | Minutes between repeated `ctx agent` context-packet emissions. An explicit `0` disables the cooldown (matches `--cooldown 0`); unset uses the default | | `task_budget_pct` | `number` | `0.40` | Fraction of the `ctx agent` token budget reserved for tasks (clamped to `0`–`1`; explicit `0` allocates none; unset uses the default) | From fc773f75dd48904e5ccbc5c443b27516e33138a2 Mon Sep 17 00:00:00 2001 From: Ersan Bilik Date: Sat, 24 Oct 2026 00:23:53 +0300 Subject: [PATCH 5/6] fix(mcp): resolve entry branch and commit as a pair Filling branch and commit independently let two checkouts share one provenance pair: an agent in another checkout that passed its branch but not its commit got the server's HEAD stored next to it, a pair no checkout ever had, in exactly the case explicit values exist for. Branch and commit are now resolved from git only when the caller gave neither (each empty or "unknown"). A caller that passes one passes both, or validation names the missing one, as it does for an omitted session_id. The schema descriptions and the MCP reference say so, and the provenance_required rows mention the "unknown" recorded when git cannot answer. The test helper also stops discarding its json.Marshal error. Spec: specs/fix-mcp-add-provenance.md Signed-off-by: Ersan Bilik --- docs/cli/mcp.md | 7 +++-- docs/home/configuration.md | 6 ++-- internal/assets/commands/text/mcp.yaml | 4 +-- internal/mcp/server/provenance_test.go | 25 ++++++++++++++++- internal/mcp/server/route/tool/tool.go | 15 ++++++---- specs/fix-mcp-add-provenance.md | 39 +++++++++++++++----------- 6 files changed, 65 insertions(+), 31 deletions(-) diff --git a/docs/cli/mcp.md b/docs/cli/mcp.md index eafc4fc4e..996bf1380 100644 --- a/docs/cli/mcp.md +++ b/docs/cli/mcp.md @@ -163,9 +163,10 @@ Add a task, decision, learning, or convention to the context. Tasks, decisions, and learnings require `session_id` unless `.ctxrc` [`provenance_required`](../home/configuration.md#option-reference) relaxes it: pass the `Session` value ctx relays into the agent's -context, or `unknown` when there is none. When `branch` or `commit` is -omitted or `unknown`, the server reads it from git; pass it only when -working in another checkout. +context, or `unknown` when there is none. When both `branch` and +`commit` are omitted or `unknown`, the server reads them from git, or +records `unknown` when git cannot answer. Pass both only when working +in another checkout: the server never fills one half of the pair. ### `ctx_complete` diff --git a/docs/home/configuration.md b/docs/home/configuration.md index 27d98c356..ae360595a 100644 --- a/docs/home/configuration.md +++ b/docs/home/configuration.md @@ -159,9 +159,9 @@ A commented `.ctxrc` showing all options and their defaults: | `hooks.enabled` | `bool` | `true` | Whether hook execution is enabled | | `statusline.enabled` | `bool` | `true` | Whether `ctx init` deploys the Claude Code status line (`ctx system statusline`) | | `statusline.show_cost` | `bool` | `true` | Whether the status line renders the session-cost (`$`) segment | -| `provenance_required.session_id` | `bool` | `true` | Require `--session-id` on `ctx add`, and `session_id` on the MCP entry tools, for tasks, decisions, learnings | -| `provenance_required.branch` | `bool` | `true` | Require `--branch` on `ctx add` for tasks, decisions, learnings; the MCP entry tools resolve it from git | -| `provenance_required.commit` | `bool` | `true` | Require `--commit` on `ctx add` for tasks, decisions, learnings; the MCP entry tools resolve it from git | +| `provenance_required.session_id` | `bool` | `true` | Require `--session-id` on `ctx add`, and `session_id` on the MCP entry tools, for tasks, decisions, learnings | +| `provenance_required.branch` | `bool` | `true` | Require `--branch` on `ctx add` for tasks, decisions, learnings; the MCP entry tools fill it from git (or `unknown`) when branch and commit are both omitted | +| `provenance_required.commit` | `bool` | `true` | Require `--commit` on `ctx add` for tasks, decisions, learnings; the MCP entry tools fill it from git (or `unknown`) when branch and commit are both omitted | | `auto_prune_days` | `int` | `7` | Days before stale session-state files are auto-pruned on context load. Non-positive values fall back to the default (never prunes on `0` or negative) | | `agent_cooldown_minutes` | `int` | `10` | Minutes between repeated `ctx agent` context-packet emissions. An explicit `0` disables the cooldown (matches `--cooldown 0`); unset uses the default | | `task_budget_pct` | `number` | `0.40` | Fraction of the `ctx agent` token budget reserved for tasks (clamped to `0`–`1`; explicit `0` allocates none; unset uses the default) | diff --git a/internal/assets/commands/text/mcp.yaml b/internal/assets/commands/text/mcp.yaml index 692a904e6..904aaab1b 100644 --- a/internal/assets/commands/text/mcp.yaml +++ b/internal/assets/commands/text/mcp.yaml @@ -105,9 +105,9 @@ mcp.tool-prop-application: mcp.tool-prop-session-id: short: 'AI session ID for provenance (required for tasks, decisions, and learnings): the Session value ctx relays into your context, or "unknown" if you have none' mcp.tool-prop-branch: - short: 'Git branch for provenance (omit to let the server resolve it from git; pass it only when working in another checkout)' + short: 'Git branch for provenance (omit it and commit to let the server resolve both from git; pass both only when working in another checkout)' mcp.tool-prop-commit: - short: 'Git commit hash for provenance (omit to let the server resolve it from git; pass it only when working in another checkout)' + short: 'Git commit hash for provenance (omit it and branch to let the server resolve both from git; pass both only when working in another checkout)' mcp.tool-prop-consequence: short: Consequence (required for decisions) mcp.tool-prop-content: diff --git a/internal/mcp/server/provenance_test.go b/internal/mcp/server/provenance_test.go index f233117b2..b09608dca 100644 --- a/internal/mcp/server/provenance_test.go +++ b/internal/mcp/server/provenance_test.go @@ -29,7 +29,10 @@ func callTool( if resp.Error != nil { t.Fatalf("unexpected error: %v", resp.Error.Message) } - raw, _ := json.Marshal(resp.Result) + raw, marshalErr := json.Marshal(resp.Result) + if marshalErr != nil { + t.Fatalf("marshal: %v", marshalErr) + } var result proto.CallToolResult if err := json.Unmarshal(raw, &result); err != nil { t.Fatalf("unmarshal: %v", err) @@ -123,6 +126,26 @@ func TestToolAddKeepsExplicitProvenance(t *testing.T) { } } +// Branch and commit come from one checkout: a branch passed from +// another checkout must not be stored with the server's commit. +func TestToolAddBranchWithoutCommit(t *testing.T) { + srv, contextDir := newTestServer(t) + gitInit(t, filepath.Dir(contextDir)) + result := callTool(t, srv, "ctx_add", map[string]interface{}{ + "type": "task", "content": "Half pair", "section": "Misc", + "session_id": "test1234", "branch": "feature/other", + }) + if !result.IsError { + t.Fatalf("expected a missing-field error, got: %s", + result.Content[0].Text) + } + text := result.Content[0].Text + if !strings.Contains(text, "--commit") || + strings.Contains(text, "--branch") { + t.Errorf("want only --commit reported missing, got: %s", text) + } +} + // The probe from issue #190, from a client with no session ID to relay. func TestToolAddLearningUnknownSession(t *testing.T) { srv, contextDir := newTestServer(t) diff --git a/internal/mcp/server/route/tool/tool.go b/internal/mcp/server/route/tool/tool.go index 042d4ad00..53a1e25bc 100644 --- a/internal/mcp/server/route/tool/tool.go +++ b/internal/mcp/server/route/tool/tool.go @@ -176,25 +176,28 @@ func watchUpdate( // entryOpts builds the sanitized entry options for [add] and // [watchUpdate], resolving branch and commit from git when the caller -// left them empty or "unknown". The values are the ones the hook relay +// left both empty or "unknown". The values are the ones the hook relay // emits ([git.CurrentBranch], [git.ShortHead], "unknown" when git -// cannot answer), read at write time; an explicit value wins. +// cannot answer), read at write time. They are resolved as a pair, so +// one checkout never supplies the branch and another the commit: a +// caller that passes one passes both, and validation reports the +// other when it is missing. // // Parameters: // - args: MCP tool arguments with optional entry fields // // Returns: -// - entity.EntryOpts: sanitized options with branch and commit set +// - entity.EntryOpts: sanitized options, branch and commit resolved +// when the caller gave neither // - error: non-nil if any field exceeds its length limit func entryOpts(args map[string]interface{}) (entity.EntryOpts, error) { opts, sanitizeErr := extract.SanitizedOpts(args) if sanitizeErr != nil { return opts, sanitizeErr } - if opts.Branch == "" || opts.Branch == cfgSession.IDUnknown { + if (opts.Branch == "" || opts.Branch == cfgSession.IDUnknown) && + (opts.Commit == "" || opts.Commit == cfgSession.IDUnknown) { opts.Branch = cmp.Or(git.CurrentBranch(), cfgSession.IDUnknown) - } - if opts.Commit == "" || opts.Commit == cfgSession.IDUnknown { opts.Commit = cmp.Or(git.ShortHead(), cfgSession.IDUnknown) } return opts, nil diff --git a/specs/fix-mcp-add-provenance.md b/specs/fix-mcp-add-provenance.md index feceed515..02508fd85 100644 --- a/specs/fix-mcp-add-provenance.md +++ b/specs/fix-mcp-add-provenance.md @@ -63,16 +63,19 @@ MCP layer only. The CLI, `entry.Validate`, and `.ctxrc` are unchanged. 2. **Resolve the git half server-side.** In the route layer (`internal/mcp/server/route/tool/tool.go`), `add` and `watchUpdate` build their options through one helper that wraps - `extract.SanitizedOpts` and fills `Branch` and `Commit` when they + `extract.SanitizedOpts` and fills `Branch` and `Commit` when both are empty or `unknown`: `git.CurrentBranch()` and `git.ShortHead()`, falling back to `unknown`. These are the values the hook relay emits (`provenance.Emit`), so MCP-written provenance matches the CLI-plus-relay path, including `unknown` when git cannot answer. The fallback is `cmp.Or`; importing `provenance.DefaultVal` would - pull cobra into the MCP route for a one-line default. An explicit - value wins, for an agent working in another checkout than the - server's cwd. No cache: caching would freeze `commit` at first use, - the staleness this step removes. + pull cobra into the MCP route for a one-line default. Explicit + values win, for an agent working in another checkout than the + server's cwd, and the pair is resolved as a whole: one checkout + never supplies the branch and another the commit, so a caller that + passes only one gets the validation error for the other. No cache: + caching would freeze `commit` at first use, the staleness this step + removes. 3. **Flatten single-line fields.** A new `sanitize.Line(s, maxLen)` strips every control character, including the tab, CR, and LF that `StripControl` keeps for multi-line content, and caps the length on @@ -93,8 +96,9 @@ MCP layer only. The CLI, `entry.Validate`, and `.ctxrc` are unchanged. (`docs/recipes/knowledge-capture.md`). `"unknown"` is what the relay itself emits without a session ID (`provenance.ShortSessionID`), and the CLI already accepts it. -5. **Describe** `branch` and `commit` as optional over MCP: omit them - to let the server resolve them from git. +5. **Describe** `branch` and `commit` as optional over MCP: omit both + to let the server resolve them from git, pass both from another + checkout. 6. **Prompts.** The `ctx-decision-add` and `ctx-learning-add` footers say "call ctx_add with … all fields above", and `session_id` is not among those fields; the footers name it. @@ -102,15 +106,16 @@ MCP layer only. The CLI, `entry.Validate`, and `.ctxrc` are unchanged. `docs/cli/mcp.md`, and note in `docs/home/configuration.md` that `provenance_required` governs the MCP entry tools too. -| Caller sends | Task line stores | -|----------------------------------|----------------------------------------------------| -| all three values | the values as sent, flattened | -| `session_id` only | that `session_id` plus the server's branch/commit | -| `branch: "unknown"` | the server's branch | -| `branch: "x\n- [x] forged"` | `#branch:x- [x] forged`, on one line | -| no `session_id` | nothing: `missing required fields: --session-id` | -| git absent, or no commits yet | `unknown` for whichever git cannot resolve | -| detached HEAD | `#branch:unknown`, as the relay emits | +| Caller sends | Task line stores | +|---------------------------------------|---------------------------------------------------| +| all three values | the values as sent, flattened | +| `session_id` only | that `session_id` plus the server's branch/commit | +| `branch: "unknown"`, no `commit` | the server's branch and commit | +| `branch` without `commit` | nothing: `missing required fields: --commit` | +| `branch: "x\n- [x] forged"`, `commit` | `#branch:x- [x] forged`, on one line | +| no `session_id` | nothing: `missing required fields: --session-id` | +| git absent, or no commits yet | `unknown` for whichever git cannot resolve | +| detached HEAD | `#branch:unknown`, as the relay emits | ## Tests @@ -131,6 +136,8 @@ case, which guards the "explicit value wins" rule: rather than assumed (the default branch name varies); - explicit values are stored verbatim, even in a repository whose real branch differs; + - a `branch` without a `commit` fails, naming only `--commit`, even + in a repository that could supply one; - the issue's probe (`type=learning`, no `branch`/`commit`) with `session_id: "unknown"` succeeds; - without `session_id` the call still fails, naming only From d1db398c115f046da630835577042bffedc9c5df Mon Sep 17 00:00:00 2001 From: Ersan Bilik Date: Sat, 24 Oct 2026 00:27:56 +0300 Subject: [PATCH 6/6] docs(mcp): describe section by what validation requires The ctx_add and ctx_watch_update rows called section "Target section in TASKS.md (tasks only)", but conventions take it too: it picks the ## section of CONVENTIONS.md the bullet goes under. Describe it as the schema does, required for tasks, without claiming it is tasks-only. Spec: specs/fix-mcp-add-provenance.md Signed-off-by: Ersan Bilik --- docs/cli/mcp.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cli/mcp.md b/docs/cli/mcp.md index 996bf1380..27788ee01 100644 --- a/docs/cli/mcp.md +++ b/docs/cli/mcp.md @@ -150,7 +150,7 @@ Add a task, decision, learning, or convention to the context. | `type` | string | Yes | Entry type: task, decision, learning, convention | | `content` | string | Yes | Title or main content | | `priority` | string | No | Priority level (tasks only): high, medium, low | -| `section` | string | Conditional | Target section in TASKS.md (tasks only) | +| `section` | string | Conditional | Target section (required for tasks) | | `context` | string | Conditional | Context field (decisions and learnings) | | `rationale` | string | Conditional | Rationale (decisions only) | | `consequence` | string | Conditional | Consequence (decisions only) | @@ -205,7 +205,7 @@ Human confirmation is required before calling. | `type` | string | Yes | Entry type: task, decision, learning, convention, complete | | `content` | string | Yes | Main content | | `priority` | string | No | Priority level (tasks only): high, medium, low | -| `section` | string | Conditional | Target section in TASKS.md (tasks only) | +| `section` | string | Conditional | Target section (required for tasks) | | `context` | string | Conditional | Context background (decisions/learnings) | | `rationale` | string | Conditional | Rationale (decisions only) | | `consequence` | string | Conditional | Consequence (decisions only) |