refactor(mcp-server): DRY envelopes, typed apiCall, explicit re-exports - #4
Merged
Merged
Conversation
Second-pass cleanup after the monolith split. Addresses every issue
from the code-review pass.
Core additions in src/api.ts:
- toMcpResult(data) + toMcpText(text): single source of truth for the
MCP text-content envelope (was ~87 duplicated literals)
- ApiError type + isApiError(v) guard: typed discriminated-union for
the error-by-value pattern; replaces open-coded shape checks
- apiCall<T = unknown>: generic so callers can document expected
response shape without unchecked "as" casts
Bulk cleanups across all 12 tools/*.ts:
- Every handler now returns toMcpResult(data) or toMcpText(text)
- Open-coded "typeof obj === 'object' && 'error' in obj" in
remote_agents.ts replaced with isApiError(v)
- Extracted initialCanvasPosition() helper out of
handleCreateWorkspace; explains why random seeding exists
- Added runtime/workspace_dir/workspace_access to create_workspace
zod schema (previously accepted by handler but hidden from clients)
src/index.ts:
- Replaced "export * from" with explicit named re-exports so the
public surface is auditable and future name collisions fail loudly
Tests:
- createServer() smoke test that records every srv.tool(...) call and
asserts 87 registered tools unique by name. Catches future PRs that
forget to wire a registerXxxTools(srv).
Docs:
- Fix broken relative links in sdk/python/molecule_agent/README.md
(was ../../examples/ from inside sdk/python/, should be ../examples/)
- Update stale "61 tools" -> "87 tools" in CLAUDE.md + main() log
Verification:
- npm run build clean
- npx jest -> 97/97 passed (was 96; +1 smoke test)
- grep "content: [{ type: \"text\" as const" src/tools/ -> 0 matches
- No file over 216 lines
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Apr 16, 2026
Code review fixes: - 🟡 #1: Replace python3 with jq in Dockerfile template stages (~50MB → ~2MB) - 🟡 #2: Add clone count verification to scripts/clone-manifest.sh (set -e + expected vs actual count check — fails build if any clone fails) - 🟡 #3: Drop 'unsafe-eval' from CSP (not needed for Next.js production standalone builds, only dev mode). Updated test assertion. - 🟡 #4: Remove broken pyproject.toml from workspace-template/ (it claimed to package as molecule-ai-workspace-runtime but the directory structure didn't match — the real package ships from the standalone repo) - 🔵 #1: Add version-pinning TODO comment to manifest.json - 🔵 #3: Add full repo URLs + test counts for SDK/MCP/CLI/runtime in CLAUDE.md Security (GitGuardian alert): - Removed Telegram bot token (8633739353:AA...) from template-molecule-dev pm/.env — replaced with ${TELEGRAM_BOT_TOKEN} placeholder - Removed Claude OAuth token (sk-ant-oat01-...) from template-molecule-dev root .env — replaced with ${CLAUDE_CODE_OAUTH_TOKEN} placeholder - Both tokens need immediate rotation by the operator Tests: Platform middleware tests updated + all pass.
This was referenced Apr 20, 2026
3 tasks
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 21, 2026
Code review fixes: - 🟡 #1: Replace python3 with jq in Dockerfile template stages (~50MB → ~2MB) - 🟡 #2: Add clone count verification to scripts/clone-manifest.sh (set -e + expected vs actual count check — fails build if any clone fails) - 🟡 #3: Drop 'unsafe-eval' from CSP (not needed for Next.js production standalone builds, only dev mode). Updated test assertion. - 🟡 #4: Remove broken pyproject.toml from workspace-template/ (it claimed to package as molecule-ai-workspace-runtime but the directory structure didn't match — the real package ships from the standalone repo) - 🔵 #1: Add version-pinning TODO comment to manifest.json - 🔵 #3: Add full repo URLs + test counts for SDK/MCP/CLI/runtime in CLAUDE.md Security (GitGuardian alert): - Removed Telegram bot token (8633739353:AA...) from template-molecule-dev pm/.env — replaced with ${TELEGRAM_BOT_TOKEN} placeholder - Removed Claude OAuth token (sk-ant-oat01-...) from template-molecule-dev root .env — replaced with ${CLAUDE_CODE_OAUTH_TOKEN} placeholder - Both tokens need immediate rotation by the operator Tests: Platform middleware tests updated + all pass.
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 22, 2026
…ession flagged - PR #1582 (staging-to-main-p0-fix): mergeable=True, 1155 commits, correct exec-form ✅ — add as item #12, recommend as primary path to main - PR #1583: REQUEST_CHANGES review posted — Cmd regression to string concat, same as F1502. Add as item #11. - Update item #3 (superseded by #1582) and #4 (merged) - Update branch HEAD to 034601c - Add PRs row to Affected Systems Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 23, 2026
4 tasks
4 tasks
4 tasks
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
Addresses both review subagents' REQUEST_CHANGES verdicts on PR #1929: Code review (correctness) - #1: Move schedule seeding to AFTER provisionWorkspaceAuto succeeds so the scheduler never fires cron rows against a workspace whose backend never wired. Failed-backend workspaces no longer end up with orphan template_schedules rows. - #2: seedTemplateSchedules now returns (seeded, skipped int) so the caller can observe partial-seed states; workspace.go Create logs the (seeded, skipped) pair when skipped > 0, surfacing silent partial-loss that the prior (int) return masked. Security review (hostile-template defenses) - #3 / #4: parseTemplateSchedules reads config.yaml through an io.LimitReader bounded by maxTemplateConfigYAMLBytes (1 MiB) and rejects files over the cap before yaml.Unmarshal runs. Defends against billion-laughs / anchor-explosion DoS. - #3: schedules slice length capped at maxTemplateSchedules (100, 10x the largest current production grid). Hostile template with 50k schedules now rejected at parse time, not after 50k inserts. - #3: cron_expr length capped at maxScheduleCronExprLen (128) per schedule; resolved prompt body capped at maxSchedulePromptBytes (16 KiB) per schedule. Oversized entries are skipped (counted as `skipped`) so one bad row doesn't break the rest. - #3: Seed loop honours ctx.Err() so an aborted Create request stops further inserts rather than running to completion on a dead goroutine. - #8: Schedule names quoted via %q in all log lines so CRLF in a hostile name can't injection-pollute stdout/Loki. Tests - TestParseTemplateSchedules_RejectsOversizeFile — gate against the LimitReader cap (1 MiB + 1 byte of '#'). - TestParseTemplateSchedules_RejectsTooManySchedules — gate against the schedule-count cap (maxTemplateSchedules + 1 minimal entries). - Full handlers test suite still green (17.4s). Non-fix surface - Code-review #3 (runtime-default fallback also seeds): runtime- default templates do not currently ship a schedules: block so this is benign in practice; documented behavior in the comment. - Code-review #4 (files_dir in workspace-template config.yaml): not part of the current template_registry schema; flagged for follow-up if templates start declaring files_dir. - Security-review #7 (cron prompt as agent self-message escalation vector): out of scope per security reviewer's own note; tracked separately. Will file an issue. Verified locally: go vet ./... → clean go build ./... → clean gofmt -d <changed files> → clean go test ./internal/handlers/ → PASS (7 unit tests for parser, full suite 17.4s) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
added a commit
that referenced
this pull request
Jun 12, 2026
…st, retire hardcoded provider vocab (#4/#5) P3 item 2. The canvas Provider/Model selector + Config-tab billing-mode now consume the registry-served GET /templates fields (registry_backed / registry_providers / registry_models from PR-A) instead of re-deriving provider knowledge client-side. Retires the hardcoded vocabularies as the PRIMARY path: - ProviderModelSelector (#4): new buildProviderCatalogFromRegistry(providers, models) builds the dropdown catalog from the registry payload — provider label = registry display_name, bucket = DERIVED provider, billing + auth_env from the registry — instead of inferVendor / VENDOR_LABELS / BARE_VENDOR_PATTERNS. The selector takes an optional pre-built `catalog` prop and uses it verbatim when supplied. inferVendor/buildProviderCatalog remain ONLY as the fallback for non-registry runtimes / older backends. - ConfigTab (#5): when the selected runtime is registry-backed, the provider catalog + selector models come from registry_providers/registry_models, and billingModeForSelectedProvider(provider, catalog) reads the DERIVED provider's billing_mode off the registry catalog. The hardcoded billingModeForProvider ('' | 'platform' → platform_managed else byok) stays as the fallback only. So the billing-mode the UI shows/sends reflects the DERIVED provider (folds in the closed #1931's canvas intent). Federation/back-compat preserved: a non-registry runtime (external/mock/kimi/ future third-party) or an older backend that doesn't serve the registry fields yields registry_backed=false → the canvas keeps the template-served models + its heuristic, unchanged. NO hard-reject (the canvas just can't render an option the registry didn't serve for registry-backed runtimes). Out of scope (per brief): the manifest runtime allowlist (SUPPORTED_RUNTIME_VALUES / FALLBACK_RUNTIME_OPTIONS) is NOT a provider vocabulary and is untouched; PUT /workspaces/:id/provider is NOT retired (that CTO #3 follow-through is a later phase). Stacked on PR-A (workspace-server registry-served /templates); re-target to main after PR-A merges. TDD: ProviderModelSelector.registry.test.tsx (catalog bucketed by derived provider, labelled from display_name, carries billing_mode + auth_env, no empty buckets), ConfigTab.registryBilling.test.tsx (billing reads registry catalog; falls back to the legacy rule with no catalog / unknown provider). Full canvas suite green (3380 passed / 1 skipped), tsc clean for touched files, eslint 0. internal#718 P3 — not merged; CTO merge-go after Five-Axis (UI-affecting). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses every finding from the post-split code review.
Added in `src/api.ts`
Cleanups across `src/tools/*.ts`
`src/index.ts`
New smoke test
Docs / counts
Test plan
Broader review outcome
Ran repo-wide code-smell scans alongside:
Remaining large files (>500 lines) — already queued for future PRs: `plugins.go` (981), `cli/update.go` (826), `cli/client.go` (755), `molecule_agent/client.py` (685), `provisioner.go` (666), `cli/view.go` (664), `FilesTab.tsx` (650), `org.go` (644), `telegram.go` (618), `cli/cmd_ops.go` (618), `workspace.go` (607).
🤖 Generated with Claude Code