Repository navigation
feat(profiles): mcpproxy access suggest turns blocked calls into profile changes - #1718
Conversation
…ile changes (smart-mcp-proxy#1700) Reads the profile refusals in the audit log, groups them by profile, tool and block reason, re-checks each group with the access explainer so reachable tools drop out, and lists the fix. A deny rule is reported, never widened; an unannotated tool never gets a tier; code_execution and management refusals carry a warning. Nothing is applied. --patch adds the proposed profile document. One service (ProfilesService.Suggest); the CLI, GET /api/v1/access/suggest and the profiles MCP tool's suggest operation render it.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Pushed a merge of main onto your branch with the OpenAPI artifacts regenerated ( |
…rent deny rule, drains the stream on cancel - A built-in refusal (code_execution, management_tools) drops out once the profile setting is on, like any other already-fixed group. - A tool the fresh explanation stops at a deny rule for is reported, never offered the explainer's allow fix, whatever the recorded reason was. - An early return on context cancellation drains StreamActivities so its producer ends its BBolt read transaction instead of blocking forever.
|
Thanks for following the plan from #1700 so closely; the service/renders split and the per-group re-explain are exactly what I wanted. I verified the branch on a merge with main and cross-reviewed it, and pushed one small commit (6e5b959) for three things the review turned up, rather than send you back for them:
Three tests cover them. Merging once CI is green on the new head. |
A day count past 3650 overflowed time.Duration: 300000d wrapped to about 86 days and was accepted, 200000d wrapped negative and was refused with a misleading message.
|
And one tiny follow-up (afda7f6): the |
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Verified on a merge with main: build, CI lint config, race tests for runtime, httpapi, cmd/mcpproxy and the profile/access/suggest tests in internal/server; admin gate proven for the MCP operation and the REST route against agent tokens, a pinned token and no auth; cross-reviewed twice. The three review findings are fixed on the branch.
|
Merged, and #1700 is closed with it. Thank you, @james-crowley! Contributions like this are what keep MCPProxy moving, and they matter a lot to the project. One small ask, only if you feel like it: a short public note about your change (a tweet, a LinkedIn post, a line in a community you are part of) helps more people discover MCPProxy. No pressure at all, and I would be glad to repost it. |
## Description The Build Binaries job in `pr-build.yml` runs `go test ./...` with Go's default 10-minute per-package timeout. `internal/server` now takes close to that on a loaded runner, so the job failed on PR #1718 (run 39776808883) with `panic: test timed out after 10m0s` while every test was passing; the test running at that moment had been going for 10 seconds. The same job passed on the previous head of that PR and on the run in between, so this is a budget, not a hang. Set `-timeout 20m` on both `go test` lines, matching what `unit-tests.yml` already uses. CI-only change.
## Hazard `storage.Manager.StreamActivities` walked the activity bucket inside a BBolt read transaction and sent every matching record into a channel buffered at 100, with no context or done channel. Any consumer that stopped reading early — the 24h profile stats loop breaking on `ctx.Err()`, an activity export whose client disconnected mid-stream — left the producer blocked on the send for the life of the process, holding the read transaction and the mmap read lock: `Stats().OpenTxN` stayed at 1, later writes could not remap the file, and `db.Close` hung on shutdown. #1718 worked around it in its own consumer with a drain goroutine (`drainActivities`); the two older consumers still leaked. ## Fix - `StreamActivities(ctx, filter)` at the source: the producer checks `ctx.Err()` before every send and selects on `ctx.Done()` around it, so cancellation returns from `db.View` promptly and closes the channel. Cancellation is not logged as a stream error. The signature changed everywhere (storage → runtime → server → httpapi controller interface and its test doubles); no external callers. - Consumers pass their request/loop context: the httpapi export cancels a per-request context on its write-error return, summary and scoped usage pass `r.Context()`, `Stats24h` and `Suggest` pass their `ctx`. `Stats24h` no longer caches a truncated rollup after cancellation; `Suggest` returns the context error after the loop instead of a partial window (a pre-cancelled context now yields zero records, so the in-loop check alone would have returned a clean empty result). - `drainActivities` and its test are removed. ## Tests - `internal/storage/activity_stream_cancel_test.go`: abandon the stream after one record with 300 matching records, assert `OpenTxN` returns to 0 and `Close` returns; the no-cancel variant proves the transaction really is pinned without cancellation (guards the first test against passing vacuously); pre-cancelled context yields nothing; live context streams all. The first test hung `Close` for the full timeout before the fix. - `TestProfilesService_Stats24hDoesNotCacheTruncatedRollup`, `TestSuggest_CancelledContextReturnsWithRecordsLeft` (updated). - Green under `-race`: `internal/storage`, `internal/httpapi`, `internal/runtime`. `golangci-lint` (CI config, bare and `--build-tags server`) reports only pre-existing findings in untouched lines.
Description
Closes #1700. Adds
mcpproxy access suggest: it turns the calls your profiles blocked into reviewed profile changes. It follows the plan in the maintainer's comment on the issue.ProfilesService.Suggestdoes the work. The CLI,GET /api/v1/access/suggestand thesuggestoperation of theprofilesMCP tool only render its result.tool_callandpolicy_decisionrecords in the audit log over--since(a duration such as90m,24h,7d; default24h, the retention is the only limit), filtered by--profile/--token.profile_tier: allow the tool (profile update <p> --add-allow).profile_unannotated: classify it. The tier is never pre-filled (<read|write|destructive>,needs_input: ["tier"]) and it is left out of the patch.profile_server_scope(nestedcode_executioncalls): add the server.profile_rule: reported, never offered for widening; the explainer's allow fix is not shown.profile_code_execution,profile_management: reported with a warning naming the profile field; no command.access explainis the tool for those.profile_changerecord is written, and the CLI only does GETs.access explain(requireServerOp(ConfigWrite)) and is classifiedscopeRefusedin the route table guard, so server-scope suggestions (which name servers a profile cannot see) are never shown to a scoped caller.--patch(optional). Adds, per profile, the stored profile with every apply-ready suggestion merged in (tools.allow += …,servers += …). That is mcpproxy's own profile document: the bodyPUT /api/v1/profiles/{name}takes and aprofiles[]entry in the config file. It is not a client permission file for Claude, Codex or similar. The end-to-end test PUTs the returned document unchanged and checks the tool drops out of the next run.tools/listgoldens gain thesuggestoperation,sinceandpatchon theprofilestool. OAS regenerated. Docs (docs/cli/profile-commands.md,docs/features/profiles.md) and CHANGELOG updated.Choices I made that you may want different:
github:get_*). Every row is one tool.block_reason(the non-disclosing row), so they never appear here. Only nestedprofile_server_scoperefusals do.outwith a reason), say so and I will add it with the spec edit.Testing
I have tested these changes locally
I have added/updated tests that prove my fix is effective or my feature works
All existing tests pass (see below)
internal/runtime: grouping, counts, ordering, the window, profile and token filters, request-id dedupe, re-check drop-out, removed profile, deny rule, unannotated, built-ins, server scope, patch merge, nothing written, window parser.internal/httpapi: admin gate (403 for a scoped token), query reaches the service, badsinceis 400, 503 without an evaluator, wire shape, route table guard.internal/server: MCPsuggestequals the REST data, schema and visibility tests, and an end-to-end test with real refusals from a pinned token, the real explainer and the real REST router.cmd/mcpproxy: query mapping, table output,--patch,-o json, exit 1 on a refusal.go test ./internal/runtime ./internal/httpapi ./internal/profile ./cmd/mcpproxypass. In the full./internal/serverrun the failures are the ones that also fail on a cleanupstream/mainhere:TestE2E_CredentialsLifecycle_SecretSinks,TestCodeExecClientModeE2E(daemon not ready), theTestBinary*/TestMCPProtocol*tests (they need a builtmcpproxybinary) andTestSearchServers_CatalogOrderMatchesTheRESTGolden(passes alone, fails in the full run onmaintoo).