Repository navigation
bugfix/menu fixes - #8
Merged
Merged
Conversation
…and user experience.
…d monitoring. Added detailed debug and error messages in GetQuarantinedServers and ListQuarantinedUpstreamServers methods, along with improvements in state management for tray UI updates.
…osity and improve clarity. Removed redundant debug messages and streamlined cache checks for better performance.
…s, enhancing visual clarity and consistency.
rannow
pushed a commit
to rannow/mcpproxy-go
that referenced
this pull request
Sep 23, 2025
- Bug fix for Quarantine Servers (0) after app start. Improvements in state management for tray UI updates. - Update autostart menu item icons for macOS to improve visual clarity and user experience.
Dumbris
added a commit
that referenced
this pull request
Nov 3, 2025
… (Issue #8) Added detailed structured logging with metrics tags for complete observability of Docker recovery operations. All recovery events now emit metrics for monitoring and analysis. Changes: - Added startup logging of Docker recovery configuration - Log existing recovery state on startup for visibility - Added detailed success metrics logging: - Total attempts, duration, average interval - Resumed from attempt count - Intervals used during recovery - Added retry metrics logging for each attempt: - Current attempt number - Elapsed time since recovery started - Next retry interval - Error details - Added reconnection phase metrics: - Success: reconnect attempt, max attempts - Failure: attempts exhausted - All metrics logs tagged with "metric_type" for easy filtering: - docker_recovery_success - docker_recovery_retry - docker_recovery_reconnect_success - docker_recovery_reconnect_failure Benefits: - Easy monitoring via log aggregation tools (grep, Splunk, etc.) - Complete audit trail of recovery attempts - Performance analysis of recovery intervals - Failure investigation with detailed context - Persistent state file provides queryable metrics snapshot - All E2E tests passing (25/25) Example metrics query: grep 'metric_type.*docker_recovery' /path/to/logs/*.log 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 16, 2026
…oped resolver never lists, path-free sibling refusals (FR-012) Critique 1/2 #1 (MUST): GET /api/v1/code/scripts answered any agent token with every stored name, host path and the scripts directory — the exact enumeration the missing-script refusal now withholds, and the published code_execution description claimed the listing was administrator-only. handleListScripts opens with requireAdminRead (the same !IsAdmin predicate the MCP refusal keys on): admin key / tray socket / nil context pass, every agent token gets 403 with a body naming nothing. @failure 403 added and oas/{swagger.yaml,docs.go} regenerated (diff limited to that route). Critique 1 #2/#3 (SHOULD): the scoped refusal was built by resolving with the administrator form (List → one lstat per stored script) and stripping the result afterwards, and the ambiguous / unusable / unreadable-directory refusals still carried the host paths and the raw OS error. codescripts gains ResolveScoped: a shared resolve(…, disclose) body where the scoped not-found error is constructed without touching the directory (listForNotFound seam + witness test proves 0 listings) and AmbiguousError / InvalidError get an Undisclosed form (name + reason, no Path/Paths/ Detail) with typed identity preserved for the REST classifier. The server picks Resolve / ResolveScoped by auth.IsScopedCaller; the post-hoc copy and the debug log of the withheld count are gone (#6). Critique 1 #4 / 2 #3: the anonymous /mcp caller under require_mcp_auth=false is administrator-shaped by design (spec FR-002); code unchanged, docs now say who counts as an administrator and how to close that door. Critique 1 #5: the nested-b positive control now has a real hidden b (counting upstream): a-only refusal byte-equal to the nonexistent-b run, zero upstream calls, admin control reaches b. Critique 2 #2: agent-tokens.md invariant carries a rollout-status note (lands surface by surface; present-tense rules are the enforced ones). #4: REST classification rows for the three non-disclosing forms. #5: stale comments in code_exec.go / code_cmd.go. #6: T064/T067 wording. #7: reuse jsonRPCHandler and a configure-hook fixture. #8: errors.As through a %w wrapper instead of on the bare pointer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 17, 2026
…oped resolver never lists, path-free sibling refusals (FR-012) Critique 1/2 #1 (MUST): GET /api/v1/code/scripts answered any agent token with every stored name, host path and the scripts directory — the exact enumeration the missing-script refusal now withholds, and the published code_execution description claimed the listing was administrator-only. handleListScripts opens with requireAdminRead (the same !IsAdmin predicate the MCP refusal keys on): admin key / tray socket / nil context pass, every agent token gets 403 with a body naming nothing. @failure 403 added and oas/{swagger.yaml,docs.go} regenerated (diff limited to that route). Critique 1 #2/#3 (SHOULD): the scoped refusal was built by resolving with the administrator form (List → one lstat per stored script) and stripping the result afterwards, and the ambiguous / unusable / unreadable-directory refusals still carried the host paths and the raw OS error. codescripts gains ResolveScoped: a shared resolve(…, disclose) body where the scoped not-found error is constructed without touching the directory (listForNotFound seam + witness test proves 0 listings) and AmbiguousError / InvalidError get an Undisclosed form (name + reason, no Path/Paths/ Detail) with typed identity preserved for the REST classifier. The server picks Resolve / ResolveScoped by auth.IsScopedCaller; the post-hoc copy and the debug log of the withheld count are gone (#6). Critique 1 #4 / 2 #3: the anonymous /mcp caller under require_mcp_auth=false is administrator-shaped by design (spec FR-002); code unchanged, docs now say who counts as an administrator and how to close that door. Critique 1 #5: the nested-b positive control now has a real hidden b (counting upstream): a-only refusal byte-equal to the nonexistent-b run, zero upstream calls, admin control reaches b. Critique 2 #2: agent-tokens.md invariant carries a rollout-status note (lands surface by surface; present-tense rules are the enforced ones). #4: REST classification rows for the three non-disclosing forms. #5: stale comments in code_exec.go / code_cmd.go. #6: T064/T067 wording. #7: reuse jsonRPCHandler and a configure-hook fixture. #8: errors.As through a %w wrapper instead of on the bare pointer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 18, 2026
… PR H0, FR-012) (#1285) * test(scope): Spec 105 PR H0 red phase — FR-012 stored-script enumeration is admin-only Failing tests for gaps FR01x-G1 and FR01x-G2 (tasks T062/T063), plus the spec.md:116 positive controls as regression pins: - TestCodeExecution_ScriptNotFound_AgentTokenNonDisclosing (G1, RED): an agent token ["*"] requesting a missing stored script with alpha-SENTINEL.js present gets an error naming neither the sentinel, the other script nor "Available scripts (N)", byte-equal to the same caller's refusal against the empty directory; proven at the handler and through the /mcp and /mcp/code JSON-RPC seams. The administrator control keeps the enumeration (TestCodeExecution_ScriptNotFoundListsAvailable stays as the nil-auth admin control). - TestCodeExecutionDescriptions_EnumerationIsAdminOnly (G2, RED): the live code_execution.description / script.description no longer advertise discovery-by-failed-call and name the administrator-only rule; the three goldens differ from the new frozen testdata/toolslist_goldens/pre105/ baseline in exactly those two strings of the code_execution entry and in nothing else (tool set and every other entry byte-equal). - TestCodeExecution_StoredScript_ScopedPositiveControls (pin, green): an a-only token runs a constant-returning stored script and gets the constant; a stored script calling b is refused at the nested call with ACCESS_DENIED. - TestScopedInitialize_PublishesCustomInstructions (pin, green): a scoped initialize on /mcp and the direct server publishes custom instructions mentioning b:private_search verbatim, identical for admin — documented operator-published content. pre105/ is a byte copy of the current goldens (not a regeneration); .gitattributes pins LF for the frozen sub-directories. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(scope): Spec 105 PR H0 — stored-script enumeration is administrator-only (FR-012) Green phase for tasks T064-T067 (gaps FR01x-G1..G3), closing the red tests committed in bcf691b. - Caller-kind branch (T064): resolveCodeExecutionSource passes a stored-script resolution failure through scopeStoredScriptRefusal. For a scoped caller (auth.IsScopedCaller — the caller KIND decides, never AllowedServers) the Spec 097 FR-004 NotFoundError is swapped for its NonDisclosing() copy, whose text carries only the requested name: no other script names, no count, no directory path, and byte-equal for an empty and a populated directory. The typed identity survives, so the REST classifier still answers 404 SCRIPT_NOT_FOUND. An absent auth context (in-process caller) and administrators keep today's "Available scripts (N): ..." enumeration (SC-005 named exception; TestCodeExecution_ScriptNotFoundListsAvailable kept as the admin control). - Definition text (T065): code_execution.description and the script parameter description no longer advertise discovery-by-failed-call and say enumeration is administrator-only / agent-token callers must already know the name. The three tools-list goldens were regenerated with MCPPROXY_WRITE_TOOLSLIST_GOLDENS=testdata/toolslist_goldens (the ONE narrow golden exception of Spec 105); TestCodeExecutionDescriptions_ EnumerationIsAdminOnly proves the delta against the frozen pre105/ copy is limited to exactly those two strings of the code_execution entry. - Docs (T066): docs/code_execution/{overview,cookbook,troubleshooting, api-reference}.md state the administrator-only rule and show the agent-token refusal; docs/features/agent-tokens.md gains the "What a scoped token cannot learn" section (invariant sentence, covered-surface list, retained-effects list, custom-instructions/stored-scripts secrets warning); docs/configuration.md warns that `instructions` is published to every caller. Links use docs.mcpproxy.app URLs. - codescripts: TestNotFoundError_NonDisclosing pins the stripped form. - tasks.md T062-T067 ticked; ROADMAP.md regenerated by scripts/gen-roadmap.py. Verification: go test -race -count=1 (with the internal/server skip list) ./internal/server/... ./internal/codescripts/... ./internal/httpapi/... green; go build ./cmd/mcpproxy and -tags server clean; gofmt/goimports/vet clean on touched files; git diff --stat origin/main -- internal/server/testdata shows only the three regenerated goldens plus the frozen pre105/ baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 critique round 1 — gate the REST script listing, scoped resolver never lists, path-free sibling refusals (FR-012) Critique 1/2 #1 (MUST): GET /api/v1/code/scripts answered any agent token with every stored name, host path and the scripts directory — the exact enumeration the missing-script refusal now withholds, and the published code_execution description claimed the listing was administrator-only. handleListScripts opens with requireAdminRead (the same !IsAdmin predicate the MCP refusal keys on): admin key / tray socket / nil context pass, every agent token gets 403 with a body naming nothing. @failure 403 added and oas/{swagger.yaml,docs.go} regenerated (diff limited to that route). Critique 1 #2/#3 (SHOULD): the scoped refusal was built by resolving with the administrator form (List → one lstat per stored script) and stripping the result afterwards, and the ambiguous / unusable / unreadable-directory refusals still carried the host paths and the raw OS error. codescripts gains ResolveScoped: a shared resolve(…, disclose) body where the scoped not-found error is constructed without touching the directory (listForNotFound seam + witness test proves 0 listings) and AmbiguousError / InvalidError get an Undisclosed form (name + reason, no Path/Paths/ Detail) with typed identity preserved for the REST classifier. The server picks Resolve / ResolveScoped by auth.IsScopedCaller; the post-hoc copy and the debug log of the withheld count are gone (#6). Critique 1 #4 / 2 #3: the anonymous /mcp caller under require_mcp_auth=false is administrator-shaped by design (spec FR-002); code unchanged, docs now say who counts as an administrator and how to close that door. Critique 1 #5: the nested-b positive control now has a real hidden b (counting upstream): a-only refusal byte-equal to the nonexistent-b run, zero upstream calls, admin control reaches b. Critique 2 #2: agent-tokens.md invariant carries a rollout-status note (lands surface by surface; present-tense rules are the enforced ones). #4: REST classification rows for the three non-disclosing forms. #5: stale comments in code_exec.go / code_cmd.go. #6: T064/T067 wording. #7: reuse jsonRPCHandler and a configure-hook fixture. #8: errors.As through a %w wrapper instead of on the bare pointer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 codex round 1 — scoped script resolution never reads the directory (FR-012) candidatesFor still os.ReadDir'd and walked every entry on the way to the count-independent not-found refusal, so a scoped miss cost ~11 µs in an empty scripts directory and ~4.7 ms with 10k entries: a timing-class oracle the non-disclosing-refusal definition forbids. It now Lstat's the two constructed candidate paths and never lists; the byte-exact name rule the listing existed for on case-insensitive volumes is kept by a single-entry entryName primitive (darwin O_SYMLINK + F_GETPATH, Windows FindFirstFile, other Unix trusts the exact-name Lstat). Tests: readDir/lstat seam counters — a scoped hit or miss reads zero directories (the administrator miss exactly one), and an empty vs a 10 000-entry directory perform identical filesystem calls. Existing case-exactness tests pin the primitive (mutation-checked on APFS). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(scope): PR H0 — make the FR-012 tests portable to Windows The administrator-listing control compared the raw JSON body with the OS path (backslashes are escaped on Windows); decode data.dir instead. The unreadable-directory cell relies on chmod 0, which Windows ignores; skip it there like the root case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 codex round 2 — administrator Resolve keeps its directory-based candidate decision (SC-005) Round 1 routed both Resolve and ResolveScoped through the constant-cost Lstat probe, which changed administrator execution: a scripts directory that is searchable but not listable (0111) refused every administrator run before Spec 105 (os.ReadDir → permission denied) and executed the script after round 1. candidatesFor is restored to the origin/main directory-read implementation and remains the administrator resolver's decision; the path probe becomes probeCandidates and is used by ResolveScoped alone (FR-012 timing class). Control test TestResolve_SearchableUnreadableDirectoryIsStillUnreadableForAdmins (skipped on Windows and as root) pins the pre-105 refusal; the two case-exactness tests now run over both resolvers so the scoped probe's fold guard stays covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 codex round 3 — scoped resolution fails closed on a Linux case-folding mount (FR-012) The Linux/BSD entryName returned the caller-constructed basename, so the scoped probe's exact-spelling check could never fire there: on ext4 casefold, vfat, or a bind mount from a case-insensitive host (a Docker Desktop mount of ~/.mcpproxy into mcpproxy-server), Lstat("backdoor.js") finds a stored `backdoor.JS` that List and the administrator's Resolve reject, and an agent-token `script:"backdoor"` executed it. Reproduced on real Linux in Docker against an APFS bind mount. Linux has no single-entry call that reports how an entry is spelled on disk — readlink of /proc/self/fd/N echoes the spelling that was looked up — and the only exact answer is the directory listing the scoped resolver must not perform. So the fold is proven instead: foldsCase Lstats the same name with every ASCII letter's case swapped (one extra constant-cost probe; names and extensions are ASCII), and the same entry answering under both spellings means the lookup folds. The Linux/BSD entryName then reports errSpellingUnverifiable and probeCandidates refuses the candidate with the ordinary non-disclosing not-found — fail closed rather than execute a file no discovery surface reports. darwin (F_GETPATH) and Windows (FindFirstFile) keep their exact-spelling checks; the administrator's directory-based decision is untouched (SC-005). Tests: TestFoldsCase (every platform) and the Linux-tagged TestResolveScoped_FailsClosedOnAFoldingDirectory / TestEntryName_ExactOnACaseSensitiveLookup, simulating the folding lookup through the lstat seam; the case-distinct test gains the fail-closed scoped branch and three scoped tests skip on such a mount. Verified in Docker natively with -race and with TMPDIR on the folding bind mount. Operator note in docs/code_execution/troubleshooting.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 4 — an exactly named script runs on a Linux case-folding mount; the fold pays one listing (FR-012) Round 3 made the Linux/BSD entryName report the spelling as unverifiable whenever the case-swapped name resolved to the same file, and the scoped resolver refused the candidate. That refused correctly named scripts too: on a Docker Desktop bind mount, vfat, or an ext4 casefold directory an agent-token `script:"daily"` got SCRIPT_NOT_FOUND while the administrator ran it — a compatibility regression FR-012 does not permit (codex r4 #1). Maintainer's decision applied: on a case-insensitive mount the exact spelling cannot be verified without listing, so the ONE directory listing happens only in that branch — after the constant-cost probe has proven the fold — and the stored basename is matched byte for byte (a case-folded entry is reported as stored and probeCandidates refuses it, as before). Case-sensitive lookups, which every native Linux volume is, keep the O(1) probe and never list. A listing the process cannot perform, or an entry gone between probe and listing, still reports unverifiable and fails closed. darwin (F_GETPATH) and Windows (FindFirstFile) are untouched, as is the administrator's directory-based Resolve (SC-005). Retained effect, documented in agent-tokens.md (retained-effects list), overview.md and troubleshooting.md: a Linux case-folding mount pays one listing per existing candidate; a missing name pays none and the refusal shape is unchanged. Tests: TestResolveScoped_OnAFoldingDirectory (renamed; the exact-name cell is inverted — runs for scoped callers and administrators alike with exactly one ReadDir; backdoor.JS still refused non-disclosing; entryName reports the on-disk spelling; a failed listing fails closed) and TestEntryName_ExactOnACaseSensitiveLookup now counts 0 ReadDir. The round-3 skips and the scoped fail-closed branch of the case-distinct test are removed — both resolvers agree everywhere; the no-listing count test skips only where entryName itself must list. Verified in Docker natively with -race and with GOTMPDIR/TMPDIR on a folding bind mount; mutation check (entryName never lists) fails 4 subtests natively and the ExtensionCaseIsExact/CaseDistinct scoped cells on the mount. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 5 — the scoped resolver answers from a directory-generation-validated exact-name index on Linux/BSD (FR-012) The round-4 fallback listing was paid only when the case-folded probe hit, so on a Linux/BSD case-folding mount the presence of `backdoor.JS` cost O(directory) while absence cost O(1): a timing oracle on the stored names, and the "retained exception" that documented it is not a Spec 105 retained effect. The scoped resolver now keeps a per-scripts-directory index of the exact spellings, listed once per directory generation (mtime+ctime+size+ino, one Lstat of the directory per request, a 2 s settle window for coarse timestamps) and answered by an O(1) set lookup; only exact hits are probed. The listing is paid when the directory changes, never per request, whatever name is asked for. darwin (F_GETPATH), Windows (FindFirstFile) and the administrator Resolve are unchanged; the fold-probe code is deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(codescripts): drop the dead Windows skip from the Linux-tagged index test staticcheck SA4032: the file is built with !darwin && !windows, so the runtime.GOOS == "windows" guard could never fire. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 6 — the scoped stored-script index is maintained off the request path; no scoped request ever lists (FR-012) Codex round 6 #1 (confirmed by probe: a cold scoped miss on Linux cost 24 µs in an empty scripts directory and 4.5 ms in one holding 10 000 entries, one inline listing on the request goroutine): the first scoped request, the first after a directory-generation change, and every request inside the 2 s settle window rebuilt the stored-name index INLINE, so a refusal's timing class followed the directory population. Structural answer (Linux/BSD only; the darwin/Windows probe path and the administrator resolver are untouched): - codescripts.Warm(dir) builds the index synchronously on the caller's goroutine (a rebuild already in flight is waited for, then Warm lists again). NewMCPProxyServer warms once the scripts directory is known and scriptsDir re-warms, single-flight and on its own goroutine, when the active config file path moves. - The request path is exactly one Lstat of the directory plus the O(1) set lookup. A moved generation, a never-built index or an unsettled one past its refresh window schedules ONE asynchronous single-flight rebuild (in-flight flag, landed channel, no ticker) and the request is answered from the index that exists: an exact hit is re-probed by Lstat and opened no-follow (removed/replaced fails closed), a script added since the listing is refused until the rebuild lands, a never-built index misses every name, a failed build refuses as unreadable — where the administrator's directory read refuses too. - The rebuild holds no lock across readDir, stamps before listing, installs the map atomically and re-checks the generation under the lock after listing (list-then-stamp race), listing again if it moved. The settle window changes from "re-list every request" to "schedule one async refresh per window". Tests (Linux-tagged; each proven to bite by mutation in Docker): cold request with 10 000 entries vs none — zero listings on the request goroutine, equal Lstat counts, one rebuild scheduled, executes once it lands; generation change — the request lists nothing, the async rebuild lands (held seam and the live goroutine), the next request sees the script; removed script fails closed before the rebuild lands; unsettled index refreshes at most once per window at unchanged request cost; Warm lists after an in-flight rebuild; unlistable directory — cold not-found, then non-disclosing unreadable, administrator same reason. Seam-changing helpers quiesce in-flight rebuilds first. Shared scoped tests warm the index as the server does; the server fixture lands the refresh after each write. Docs state the off-request-path index and the milliseconds window for scoped callers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 8 — the stored-script index answers only for its own generation, and a hit's directory generation is rechecked after the open (FR-012) codex r7 found two genuine issues in the Linux/BSD stored-name index: 1. MUST-FIX: storedNamesFor scheduled a rebuild when it found the index behind the directory's generation, but still answered from the stale index while that rebuild was merely scheduled or in flight. A stale index that once listed an entry under an EARLIER spelling stayed good enough to authorize it — on a case-folding mount, warm `report.js`, rename it to `REPORT.JS` (moving the directory's generation), and the stale index's own Lstat of `report.js` still succeeds by folding onto the renamed file. The index now answers ONLY for the generation it was built against; any mismatch (never built, behind, or a rebuild merely scheduled/in flight) refuses exactly as a never-built index refuses, fail closed, until its own rebuild lands off the request path. A second race closed the same way: after an exact-set hit's no-follow open, the directory generation is read once more and compared to the one read before the lookup, closing the descriptor and refusing on a mismatch (gen-before == index.gen == gen-after proves the opened entry is the one the index vouched for). 2. SHOULD: rebuild() re-listed without bound while the directory's generation kept moving, and Warm could block on it forever. One rebuild now gives up after maxRebuildAttempts (3) listings — the next request's own generation check finds it stale and refuses fail-closed rather than this loop spinning to prove something it never can — and scheduleRebuildLocked withholds a new rebuild goroutine for rebuildBackoff (1s) after the previous one ends, so a continuously changing directory cannot resume the same unbounded listing cost one goroutine later. Warm's own explicit rebuild does not consume this backoff, so it never delays the very first real change shortly after startup. Red tests first (storednames_other_test.go): a stale index refuses a renamed entry rather than authorizing it (the round 7 finding's exact repro, via the lstat/generation seams); a generation change between the candidate probe and the open refuses and never reads the swapped content; a continuously changing directory bounds one rebuild's listings and lets Warm return; the backoff withholds a new rebuild until it elapses. Round-6 tests that asserted the old "answers from the existing index while stale" behavior are updated to the new fail-closed contract (one directory Lstat decides staleness, no candidate probe) with the round noted inline; the two successful-hit lstat counts gain the post-open recheck. Verified: go test -race -count=1 ./internal/codescripts/... (darwin, and Linux via golang:1.26 in Docker — root, non-root/uid1000, and against a real case-folding bind mount from the macOS host); go test -race -count=1 ./internal/httpapi/...; go test -race -count=1 -skip 'E2E|Binary| MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic| TestSocketInfoEndpoint' ./internal/server/...; go test -tags server ./internal/serveredition/...; both editions build; go vet clean on darwin, GOOS=linux, GOOS=windows, GOOS=freebsd; gofmt clean on touched files; internal/server/testdata goldens untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): an unreadable scripts directory is refused as unreadable on the first scoped request Round 8's "stale or never-built index answers not-found" rule made a directory that lost its read bit after the index was built report not-found until an async rebuild recorded the error — the refusal shape depended on index state, and the cross-platform unreadable-directory tests failed on Linux CI (non-root). The request path now opens the directory (one constant-cost syscall, no readdir) and answers the unreadable form immediately, the same reason the administrator's listing gives, on every platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 9 — the index authorizes only once settled, the descriptor's spelling is proven at open on darwin/Windows, the generation includes the device, and the index map is bounded (FR-012) Four codex round-8 findings, all MUST/SHOULD-fixed: 1. An unsettled stored-name index (Linux/BSD) could authorize a case-renamed file when the rename landed within the coarse-timestamp settle window, because a matching generation alone was trusted before the stamp was old enough to rule out a write still landing on it. The index now authorizes a hit only when settled; the rebuild-scheduling cadence is unchanged. 2. On darwin/Windows the exact-spelling probe ran only before the open, so a case-rename racing openScriptFile's own open could execute the wrong spelling, and a probe-call failure was treated as a match (fails open). The opened descriptor's own stored spelling is now proven post-open (F_GETPATH on the executed fd / GetFinalPathNameByHandle), authoritatively, on every platform; a probe failure no longer admits a candidate. 3. dirGeneration omitted the device, so a bind-mount swap to another filesystem colliding on inode/size/mtime/ctime could reuse a stale index. st_dev is now part of the generation tuple on every unix variant. 4. The stored-name index map retained an entry for every scripts directory ever used. Warm now keeps only the active directory's index, and the map caps at 4 entries (LRU) for bare (never-Warmed) use. Fixing (1) exposed a Linux-only internal/server test regression (fixtures write-then-resolve within the same test, always inside the settle window) caught only by the maintainer's own non-root-Linux Docker repro; fixed with an exported, test-only codescripts.SetIndexClockForTest seam. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(codescripts): exercise the post-open spelling proof with a rename Windows allows Windows refuses to rename a file another handle holds open, so the rename-after-open race cannot happen there and the test failed with a sharing violation. The portable half of the race — case-rename between the pre-open probe and the open, which APFS and NTFS fold onto the renamed entry — now carries the assertion on both platforms; the after-open variant stays darwin-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 11 — bind scoped resolution to one directory descriptor per call, refuse a following Windows reparse point, and make async rebuilds cancellable (FR-012) Three findings from round 9's audit, all fixed per the maintainer's full structural decision: 1. MUST-FIX (Linux/BSD directory-path ABA): every earlier round's scoped resolution re-resolved scriptsDir BY PATH at each step of one request — the generation read, the candidate probe, the open, and the post-open recheck were four independent lookups. A replaceable symlink, ancestor directory, or bind mount retargeted between two of those steps and back before the next let each step separately agree with a different directory than the others saw; st_dev (round 9) rules out a substitution visible during one snapshot, never an alternation across several. Fixed by binding the whole request to ONE retained directory descriptor (dirfd_other.go): opened by path exactly once (openScopedDir), then every generation read (fstatDirGeneration), the candidate probe (fstatatEntry, AT_SYMLINK_NOFOLLOW) and the open itself (openatEntry, O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC) are relative to that same descriptor — no path is resolved twice. The rebuild goroutine lists through the identical construction (listScopedDirOnce), so the listing and the generation it records for it come from one open. Request cost stays O(1): one open, two fstats, one fstatat, one openat for a hit. 2. MUST-FIX (Windows reparse-point race): openScriptFile Lstat'ed the path and then called os.Open, which DOES follow a reparse point planted (on the candidate or a symlinked ancestor) between the two; the post-open proof compared only a basename, which any identically-named file reached through the reparse point satisfied. Fixed: openScriptFile now opens via windows.CreateFile with FILE_FLAG_OPEN_REPARSE_POINT (the Windows equivalent of O_NOFOLLOW — no check-then-open window) and refuses a reparse point or directory via GetFileInformationByHandle (open_windows.go); the scoped post-open proof compares the opened descriptor's FULL normalized path (openedFinalPath) against the scripts directory's own final path — obtained from a handle opened once per request with FILE_FLAG_BACKUP_SEMANTICS — plus the exact basename (entryname_windows.go, storedspellings_probe_windows.go), so a retargeted ancestor cannot substitute an outside file under the same name. 3. SHOULD (cancellable rebuilds): each stored-name index now owns a context; LRU eviction and Warm's pruning of every other directory cancel it before dropping the index, and the ASYNC (request-scheduled) rebuild goroutine checks it between listing attempts and once more before installing, so a cancelled rebuild stops promptly and installs nothing for a directory nobody will query through it any longer. Warm's own synchronous rebuild is deliberately NOT cancellable (the caller is blocked on it and trusts its result) — only the async path honours ctx. Linux-tagged tests (storednames_other_test.go) cover: a real symlink retarget between a session's own steps proving the ABA fix (TestResolveScoped_DirectoryPathABA); the identity check refusing a same-stamp-but-different-device descriptor (TestStoredNamesFor_IdentityMismatchIsAMiss, TestDirFdGeneration_ReadsTheDevice); eviction stopping and not installing from an in-flight async rebuild via a WaitGroup seam (TestStoredNames_EvictionCancelsAnInFlightRebuild); every existing round 4-9 test re-pointed at the new fd-bound primitives and their own bounded cost counters. Windows: GOOS=windows go vet and go test -c both compile; a real Windows run is not available here, so the Windows code mirrors the darwin structure and stays minimal. darwin is unchanged beyond the shared five-return signature. Docs (overview.md, troubleshooting.md, agent-tokens.md) note the single-descriptor binding and the Windows full-path/no-reparse proof. Verified: go build (both editions); gofmt; go vet on darwin/linux/freebsd/ windows for internal/codescripts; GOOS=windows go test -c; go test -race -count=1..3 -shuffle=on natively on darwin; go test -race -count=2 non-root in golang:1.26 Docker for internal/codescripts (the required H0 command); go test -race -count=1, non-root, full internal/server suite with the project's skip regex (294.8s, matching prior rounds' baseline) — no regression; go test ./cmd/mcpproxy/; go test -tags server ./internal/serveredition/...; git diff -- internal/server/testdata is empty (no golden touched). golangci-lint v2 could not run — pre-existing environment issue (its Go 1.25 build refuses this repo's Go 1.26 toolchain line), unrelated to this change and unchanged since round 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 13 — unify darwin/Windows onto the shared index + retained-descriptor design (FR-012) Maintainer decision on codex round-10's six findings: rather than patch darwin and Windows separately, unify every platform onto the same design. darwin joins the Linux/BSD fd-bound implementation (dirfd_other.go, storednames_other.go: build tag `!darwin && !windows` -> `unix`) — the per-directory exact-spelling index, off the request path, answers a scoped lookup in O(1) regardless of whether a differently-cased entry exists (closes the finding-3 timing oracle for darwin). F_GETPATH survives as an additional, belt-and-suspenders basename proof on top (extraVerifyOpened, entryname_darwin.go), since openat's own identity binding already proves the parent (closes finding 1: a retargeted scriptsDir symlink cannot redirect an already-open fd). Windows gets a parallel index implementation (storednames_windows.go) built from a directory HANDLE retained for the whole request; the candidate probe and the actual open are both performed RELATIVE TO THAT HANDLE via windows.NtCreateFile (OBJECT_ATTRIBUTES.RootDirectory), never a fresh path lookup a rename or reparse point could redirect (closes finding 2's ABA repro structurally: NT handles, like Unix fds, are bound to the file object, not the path). Sharing widened to READ|WRITE|DELETE on both the new relative opens and open_windows.go's administrator path (finding 4). Finding 5 (SHOULD): a process-wide semaphore (rebuildsemaphore.go, cap 2) bounds concurrent async rebuild goroutines across both platform index implementations — a rebuild that cannot acquire a slot is skipped, not queued, and the next request retries. Finding 6 (SHOULD): the `unix` build tag (plus open_unix.go, which had the identical latent gap) excludes plan9/js/wasip1, which now build via a new fail-closed fallback (fallback_other.go). Sibling sweep: Windows's CreateFile+FILE_FLAG_BACKUP_SEMANTICS does not itself refuse a non-directory the way unix's O_DIRECTORY does — the scripts directory replaced by a plain file — so defaultWinOpenScopedDir now checks FILE_ATTRIBUTE_DIRECTORY explicitly. Verified: full native darwin suite (-race, shuffle, real symlink-retarget ABA test) green; non-root Linux in Docker green; go vet clean for linux/freebsd/windows; GOOS=windows go test -c compiles; GOOS=plan9 and GOOS=js/wasm builds succeed (previously failing outright). Both editions build; internal/server (skip regex applied), internal/httpapi, internal/serveredition and cmd/mcpproxy all green. No tool-surface golden touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 — a directory candidate is non-regular on Windows, never unreadable Without FILE_FLAG_BACKUP_SEMANTICS (deliberately not requested: it lets a SeBackupPrivilege holder read past ACLs, which os.Open never did) CreateFile refuses a directory with ERROR_ACCESS_DENIED before any attribute is visible, so TestResolve_Directory reported "unreadable" where the pre-105 administrator answer is "non-regular" (SC-005). Classify that case from the attributes; the scoped relative open maps STATUS_FILE_IS_A_DIRECTORY the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 14 — Windows path-size off-by-one and fallback-platform test compilation (FR-012) Two codex r11 SHOULD findings: 1. entryname_windows.go's finalPathOfHandle retried the GetFinalPathNameByHandle call only when n > len(buf). Windows reports the required size INCLUDING the null terminator when the buffer was too small, so a resolved path whose length makes the first call report n == len(buf) also needs a retry — that boundary previously fell through and returned a truncated/unspecified buffer, wrongly refusing a valid exact-length scoped script as not found. Fixed to n >= len(buf); GetFinalPathNameByHandle itself is now a seam (getFinalPathNameByHandle) so a table test can drive the retry at exactly that boundary (one under, exact, one over) without needing a real handle whose resolved path happens to be exactly 1024 UTF-16 units. 2. GOOS=plan9 and GOOS=js test compilation for ./internal/codescripts failed before reaching fallback_other.go's fail-closed behavior: codescripts_test.go's shared tests call warmStoredNames and quiesceIndexRebuilds, but those helpers existed only in the unix-tagged and windows-tagged test files, and open_fifo_unix_test.go was tagged !windows despite using syscall.Mkfifo (Unix-only). Added fallback_other_test.go (!unix && !windows) with trivial helpers matching the unix/windows ones' signatures, and retagged open_fifo_unix_test.go to unix. Verified: GOOS=windows/plan9(amd64)/js(wasm) go test -c -o /dev/null all compile; go vet clean on darwin/linux/freebsd/windows; both editions build; native darwin go test -race -shuffle=on and non-root golang:1.26 Docker go test -race -count=2 both green for ./internal/codescripts/; full ./internal/server/... suite (skip regex applied) green. gofmt clean; no tool-surface goldens touched. Final self-review sweep of all H0 commits found no further high-confidence issues. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(codescripts): force the directory mtime forward in the Windows race test TestStoredSpellingsOf_PostOpenProofCatchesARaceOnTheOpenedDescriptor wrote a second file into the directory and expected LastWriteTime to have moved by the time verifyUnchanged re-read it, but NTFS on the CI runner did not reliably surface a distinct timestamp within the test's tight window (fatal error: "An error is expected but got nil"). Explicitly force the mtime forward with os.Chtimes, the same mitigation the unix counterpart (TestResolveScoped_GenerationChangeBetweenLookupAndOpenRefuses) already uses for the identical class of coarse-clock flake — the assertion is about the recheck logic, not filesystem timestamp granularity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 15 — bound the Windows final-path resize loop against a growing path (FR-012) finalPathOfHandle (entryname_windows.go) retried GetFinalPathNameByHandle exactly once on an oversized-result report, then unconditionally sliced buf[:n]. If the resolved path grew again between that first oversized call and the retry (e.g. another process extends the path while the delete-shareable handle stays open), the retry's own n could again exceed the just-resized buffer, and buf[:n] panicked with a slice-bounds error. Turn the single retry into a bounded loop (finalPathNameMaxAttempts = 4), slicing buf[:n] only once a call's n actually fits the buffer it was given; exhausting the bound returns a plain error instead of slicing a stale buffer. The sole caller (winOpenedBaseName -> verifyUnchanged) already folds any non-nil error here into errSpellingUnproven, the existing non-disclosing refusal (SC-005), so no new error path needed threading through. New table test TestFinalPathOfHandle_GrowingPathDoesNotPanic covers exact-fit, one-retry, forced second growth (proves no panic and a clean error when the bound is exhausted), and growth that settles on the last permitted attempt. Left the round-12 SHOULD finding (unbounded per-rebuild index memory) untouched per maintainer directive -- accepted residual, now recorded as such in research.md's new H0 section. The round's third reported MUST-FIX (SEARCH behavior deletion in internal/index/bleve.go) does not apply to this PR: that file is untouched by this branch's diff against origin/main and already carries the underscore-segment fix on the base this branch is rebased on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): PR H0 review round 17 — Warm blocks on the shared rebuildSlots semaphore (FR-012) Warm's own synchronous, population-sized directory listing bypassed the process-wide rebuildSlots semaphore that async rebuilds already respect, so the documented "at most maxConcurrentRebuilds concurrent listings, process-wide" bound did not actually hold once more than one Warm call was in flight for different directories (e.g. two active-config-path moves each spawning their own async warmStoredScripts goroutine). Warm now acquires a rebuildSlots slot too, but with a blocking acquire (it cannot skip the work like the non-blocking async scheduler does), taken outside idx.mu so it can never hold up another goroutine. Applied identically to the Windows mirror, which shares the same semaphore. A second, lower-severity half of the same finding — an older Warm(A) can be pruned by a concurrent Warm(B) and finish rebuilding an unreachable index — is an accepted, self-healing residual (wasted work only, never stale/incorrect data) and is documented in research.md rather than fixed with more locking. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): LanguageMismatchError was the one refusal that ignored disclose (FR-012) codex gpt-5.6-sol cross-model review round 1 (post-main-merge) found that resolve()'s LanguageMismatchError branch was the only refusal in codescripts.go that never checked the disclose flag: NotFoundError, AmbiguousError and InvalidError all thread it through to their own NonDisclosing() form, but an explicit `language` argument that contradicts a found script's real extension went straight to the administrator-detail error unconditionally. A scoped caller who always sends a language no real script could satisfy could use the resulting type split (LanguageMismatchError vs the non-disclosing NotFoundError) as a per-guess exists/not-exists oracle, cheaper and side-effect-free compared to the already-accepted probe of trying to execute a guessed name. Fix mirrors the existing AmbiguousError/InvalidError pattern exactly: add Undisclosed + NonDisclosing() to LanguageMismatchError (withholding the host filesystem facts — Extension, Derived — while keeping the caller's own Requested input and the distinct INVALID_LANGUAGE classification, consistent with how Ambiguous/Invalid keep their own SCRIPT_UNUSABLE classification rather than collapsing into SCRIPT_NOT_FOUND), and gate the resolve() call site on `disclose` the same way the sibling branches already are. Other review findings evaluated and not actioned in this PR: - tool-calls activity log exposing code_execution Arguments to a "*"-scoped token (internal/runtime/runtime.go) — pre-existing, outside files this PR touches, and the REST management API is explicitly out of scope for Spec 105 (spec.md "Transport and caller scope"; tracked separately as the #1166 entitlement follow-up already named in GetToolCalls's own doc comment). - anonymous /mcp caller under the default require_mcp_auth=false reaching the administrator path — the named, spec-blessed FR-001 caller-kind-first exception (spec.md amendment 6, SC-005), matching this repo's documented security model for the /mcp surface generally, not something this PR introduces. - storednames rebuild scheduling as a cache-timing side channel — real (probeCandidates alone uses the index; the administrator path still reads the directory directly) but only signals "the directory changed recently," not which names exist; consistent with the accepted residuals research.md already documents for this same subsystem (rebuildSlots contention, Warm/prune race). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <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
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.
Uh oh!
There was an error while loading. Please reload this page.