Skip to content

[fix] Require config for MCP cleanup tools#298

Open
Jia-Ethan wants to merge 1 commit into
lugassawan:mainfrom
Jia-Ethan:codex/mcp-require-config
Open

[fix] Require config for MCP cleanup tools#298
Jia-Ethan wants to merge 1 commit into
lugassawan:mainfrom
Jia-Ethan:codex/mcp-require-config

Conversation

@Jia-Ethan

Copy link
Copy Markdown

Summary

  • Require initialized rimba config before MCP clean, remove, and status handlers do any work.
  • Reuse the loaded config's default source for clean/status main-branch resolution.
  • Add regression coverage for the uninitialized-repo path on all three MCP tools.

Test Plan

  • Unit tests pass (make test)
  • Linter passes (make lint)
  • E2E tests pass (make test-e2e)

Additional focused checks run:

GOPROXY=https://goproxy.cn,direct go test ./internal/mcp -run 'RequiresConfig' -count=1
GOPROXY=https://goproxy.cn,direct go test ./internal/mcp -count=1
GOPROXY=https://goproxy.cn,direct go test ./... -count=1

Risk and scope: scoped to MCP handler preflight checks and tests. Existing required-argument errors still run before the config gate, while valid tool calls now fail with the same friendly initialization message used by the other MCP tools when .rimba/settings.toml is unavailable.

Issue

Closes #267

@Jia-Ethan Jia-Ethan force-pushed the codex/mcp-require-config branch from a475fbb to 6768d25 Compare June 11, 2026 01:46
return mcp.NewToolResultError("mode is required"), nil
}

cfg, cfgErr := hctx.requireConfig()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

High — prune mode now unnecessarily requires config

mcpCleanPrune calls only git.Prune and git.PruneRemotes — it never reads cfg.DefaultSource. By placing requireConfig() before the switch, all three modes (including prune) now fail without an initialized repo, extending the blast radius beyond the issue scope.

prune was previously usable in any repo; this is a silent behavioral regression.

Suggested fix: move the config gate inside the merged and stale case arms, where cfg.DefaultSource is actually consumed:

switch mode {
case "prune":
    return mcpCleanPrune(ctx, r, dryRun)
case "merged":
    cfg, cfgErr := hctx.requireConfig()
    if cfgErr != nil {
        return mcp.NewToolResultError(cfgErr.Error()), nil
    }
    return mcpCleanMerged(ctx, r, cfg.DefaultSource, dryRun, force)
case "stale":
    cfg, cfgErr := hctx.requireConfig()
    if cfgErr != nil {
        return mcp.NewToolResultError(cfgErr.Error()), nil
    }
    return mcpCleanStale(ctx, r, cfg.DefaultSource, dryRun, staleDays, force)

Then update TestCleanToolRequiresConfig to use modeMerged or modeStale (since modePrune would no longer require config).

@lugassawan lugassawan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review Decision: COMMENT

Reviewed by reviewer (swe-workbench). Posted 1 inline comment, deduped 0.

Informational (out-of-diff)

internal/mcp/tool_list.gohandleListArchived bypasses the config gate (pre-existing)

handleList short-circuits to handleListArchived before calling requireConfig() when archived=true. handleListArchived uses the nil-safe configDefault(hctx) helper, so list --archived still works without an initialized repo. This PR does not introduce or worsen it — it predates this change — but it is the same class of gap issue #267 addressed on the other three tools. Worth a follow-up issue.

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.

[bug] MCP clean/remove/status tools skip the requireConfig gate

2 participants