Effect v4 review fixes: apps/api MCP domain - #292
Open
JeremyFunk wants to merge 1 commit into
Open
Conversation
Fixes from a full effect-review-v4 pass over apps/api/src/mcp (110 files): - Replace Schema.decodeUnknownSync on agent-supplied input with effectful decoding so invalid ids/filters return actionable tool errors instead of escaping as defects (update-dashboard, query-data) - Replace try/catch JSON.parse with Schema.fromJsonString decoding (resolve-tenant, create-dashboard); untrace the auth hot path - Log the cause before the inspect-widget catchCause fallbacks; map warehouse errors per-tag instead of collapsing the union - Annotate orgId + tool on every tool span centrally in the dispatcher (annotateCurrentSpan + annotateSpans); drop dead McpTenantError - Make resolveTimeRange/resolveDashboardTimeRange read the Effect Clock instead of DateTime.nowUnsafe, updating all tool call sites; time.test now asserts exact values under TestClock - queryWarehouse gains an optional rowSchema for validated rows - Migrate MCP tests to @effect/vitest idioms (it.effect, assert.*) - Add test coverage for inspect-widget helpers (33 tests) and the resolve-tenant auth path (29 tests) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
🍁 Maple PR previewThe preview is ready:
Deployed from |
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.
Domain 1 of the repo-wide Effect v4 review (
/effect-review-v4, one Opus reviewer per domain). Scope:apps/api/src/mcp— 110 files.Review verdict: FAIL (4 criticals, 16 warnings) → all criticals and warnings addressed; score was 7.5/10.
Criticals fixed
update-dashboard/query-data:Schema.decodeUnknownSyncon agent-supplied ids/filters threw defects throughcatchDefect; now effectful decodes returning typed tool errors with discovery-tool guidance.resolve-tenant: rawtry/catch JSON.parsein the auth module →Schema.fromJsonStringdecoding;resolveMcpTenantContextmoved toEffect.fnUntraced(per-request auth hot path).inspect-widget:Effect.catchCause(() => succeed(SKIPPED_SUMMARY))silently swallowed every failure/defect — now logs the cause first; the blanket warehouse-error catch now maps per-tag viatoMcpQueryError.Warnings fixed
resolveTimeRange/resolveDashboardTimeRangenow read the Effect Clock instead ofDateTime.nowUnsafe()(all ~20 tool call sites updated);time.test.tsasserts exact values underTestClock.orgId+ tool name annotated on every tool span centrally in the dispatcher (annotateCurrentSpanfor the dispatch span,annotateSpansso the handler's ownEffect.fnspan inherits it) — previously only 22/57 tools annotated.queryWarehousegains an optionalrowSchema(validated rows; unchanged behavior for existing callers).@effect/vitestidioms (it.effect,assert.*) in app/dispatcher/regression tests; service access viayield*instead of static accessors.inspect-widget.test.ts(33 tests over reduce aggregates, EMPTY_GROUPING/METRIC_NOT_FOUND, all skip/error outcomes) andresolve-tenant.test.ts(29 tests over internal-token, OAuth audience binding, scoped-key rejection, actor resolution).Also: dead
McpTenantErrordeleted (never constructed),catchTag+catchTagsfolded, misc info-level cleanups.Verification
apps/apitsc --noEmitcleanbun vitest run src/mcp: 18 files, 175 tests passed🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.