feat: remove write tools — read-only fork for LoyaltyLion - #1
Conversation
There was a problem hiding this comment.
This is a clean, well-scoped surface reduction. The removals go all the way down — Tool entries, dispatch branches, handler functions, and tests are all in lockstep, so a stale client requesting a removed tool just falls into the unknown_tool branch in server.py rather than hitting half-stripped code. Module docstrings flag the fork state, the README has a prominent fork notice, and the PR description is explicit about the threat model (no Temporal authorizer configured → physical removal in the image is the gate). The defense-in-depth framing is sound; I have no concerns with the security shape of the change.
A few small follow-ups worth landing now or in a quick follow-up — none are blockers:
- The
exceptions.pywrite-error formatters are now unreachable. The two write-only error subclasses (WorkflowAlreadyStartedError,ScheduleAlreadyRunningError) can no longer be raised because every code path that would have created a workflow or schedule is gone. The imports and helpers can be removed to keep the surface area honest. - A couple of README spots still describe the upstream surface ("manage Temporal workflows" in the Overview, the Distributions section pointing at upstream PyPI/Docker without inline disambiguation). The top-of-file notice covers this for an attentive reader, but a skim-reader could miss it.
pyproject.tomlkeeps the upstreamversion = "1.1.1"and drops theIssuesURL. Since you're pinning consumers by SHA the version is mostly cosmetic, but worth a thought on whether to bump it (e.g.1.1.1+loyaltylion.1) and to point Issues at the fork repo's issue tracker.
Tests look right — only the removed-tool tests went away; the read-path tests remain (54 passing matches the PR description). Good shape.
| @@ -1,14 +1,14 @@ | |||
| [project] | |||
| name = "temporal-mcp-server" | |||
| version = "1.1.1" | |||
There was a problem hiding this comment.
Fork ships as version = "1.1.1" — identical to upstream's last release. Consumers pin by SHA per the PR description so this is mostly cosmetic, but it can create confusion if anything ever surfaces the version (e.g. pip show, --version output). Consider a fork-distinguishing form like 1.1.1+loyaltylion.1 (PEP 440 local version segment) so the lineage stays legible without affecting install ordering.
| Issues = "https://github.com/GethosTheWalrus/temporal-mcp/issues" | ||
| Homepage = "https://github.com/loyaltylion/temporal-mcp" | ||
| Repository = "https://github.com/loyaltylion/temporal-mcp" | ||
| Upstream = "https://github.com/GethosTheWalrus/temporal-mcp" |
There was a problem hiding this comment.
Issues = "https://github.com/GethosTheWalrus/temporal-mcp/issues" was dropped from [project.urls]. Given the fork lives at loyaltylion/temporal-mcp, consider re-adding Issues = "https://github.com/loyaltylion/temporal-mcp/issues" so downstream consumers (or a future Slack-found PyPI lookup) have a canonical place to file bugs against the fork rather than against upstream.
frangz
left a comment
There was a problem hiding this comment.
2 thoughts:
- This is a significant change over the upstream repo. It won't be straightforward to bring in new changes. Is there a more minimal way to remove or block write tools? e.g. just do not register them, but keep the code in place.
- I do wonder if write tools would be handy. e.g. currently we can only start workflows using a luna or remus console, which is terribly annoying. It would be great if we could do it via Claude. Similarly to stop a workflow: you first need to find it on the UI, which is also not friendly. But I get the safety considerations, so you can move ahead.
There was a problem hiding this comment.
The delta since my last review is a single commit (chore: drop CI workflows) that deletes .github/dependabot.yml and every workflow under .github/workflows/ (lint, test, security, dependabot-auto-merge, labels). For a SHA-pinned read-only fork with no PyPI publish path, dropping the inherited upstream CI is a defensible call — the surface that mattered (the release workflow) was already gone in the previous commit, and downstream consumers run their own CI on the pinned SHA. The two unresolved comments from my previous review (pyproject.toml version and Issues URL) are not addressed by this commit; I've left them as-is since they're nits that the author is welcome to defer.
One small consistency note: the PR description's test plan still has "Lint workflow green on this PR" as a checkbox, which is now untestable. Worth a quick edit to the PR body to either drop that bullet or note that CI was intentionally removed in the latest commit.
Otherwise, no new issues — happy for this to merge.
| @@ -1,4 +1,8 @@ | |||
| # Temporal MCP Server | |||
| # Temporal MCP Server — LoyaltyLion read-only fork | |||
There was a problem hiding this comment.
Respond to this comment to discuss this review with me
Summary
This downstream fork strips every mutating tool from
GethosTheWalrus/temporal-mcpso AI clients (Claude Code, in-VPC agents) can only inspect our self-hosted Temporal cluster. Our self-hosted Temporal has no authorizer / claim mapper configured, so anything that can reach the frontend on7233can do anything — physically removing the write code in the image is the safest gate.What's exposed (6 tools)
describe_workflowget_workflow_historyget_workflow_resultlist_workflowsquery_workflowlist_schedulesWhat was removed
start_workflow,signal_workflow,cancel_workflow,terminate_workflow,continue_as_newbatch_signal,batch_cancel,batch_terminatecreate_schedule,pause_schedule,unpause_schedule,delete_schedule,trigger_scheduleDeletions go all the way down — no Tool entries in
tool_definitions.py, noelifbranches in the dispatch inserver.py, no handler functions in thehandlers/modules.batch_handlers.pyis gone entirely.Other cleanup
.github/workflows/release.ymlremoved — upstream's auto-publish-to-PyPI workflow is dangerous on a downstream fork. We install viapip install git+https://github.com/loyaltylion/temporal-mcp@<sha>, not from PyPI.pyproject.tomlURLs point at the fork; description / keywords note read-only.README.mdhas a top-of-file fork notice and a trimmed Tools section.Test plan
pytest -vpasses locally (54 / 54)aws-infrastructureDockerfile pins this fork SHA and the resulting image builds in BuildkiteTracking upstream
The fork's
mainwill trackGethosTheWalrus/temporal-mcp/mainas a courtesy. Rebases happen on theloyaltylion/read-onlyfamily of branches, not main, so upstream history stays clean. We pin downstream consumers to specific SHAs, never to a branch name.🤖 Generated with Claude Code