feat(lab-77y5): add mcpregistry service — SDK, dispatch, CLI, API, auth rate limiter, SSRF fixes#24
Merged
Merged
Conversation
Thread 1+3 (upstream-oauth-client.ts): wire CSRF/bearer headers via gatewayHeaders() and use normalizeGatewayApiBase() so the client respects NEXT_PUBLIC_API_URL and standalone bearer auth mode instead of hard-coding /v1 and always sending cookies. Thread 2 (upstream-oauth-card.tsx): check window.open() return value; reset connecting state and show an error message when the popup is blocked rather than spinning forever. Thread 4 (router.rs): gate the /gateway/oauth route nest on state.gateway_manager.is_some() while preserving the ordering constraint that keeps /gateway/oauth before /gateway. Thread 5 (pool.rs): move the response-size check before record_success_for() in subject_scoped_read_resource so an oversized response records a circuit-breaker failure rather than advancing the healthy counter. Thread 6 (cache.rs): stop removing build_locks entries in evict_subject and evict_upstream; the locks serialize concurrent builders per (upstream, subject) key and must outlive the cached client. Thread 7 (manager.rs): after save_dynamic_client_registration, read back the canonical client_id from the DB to resolve the TOCTOU race where two concurrent callers both register and last-writer-wins overwrites the saved value. Thread 8 (docs/UPSTREAM.md): remove contradictory catalog-visibility claim; OAuth-tagged upstreams are unhealthy at startup and excluded from the merged tool list until the OAuth flow completes. Thread 9 (refresh.rs): already fixed in current main — map_auth_error correctly maps TokenRefreshFailed → NeedsReauth. Co-authored-by: Claude <noreply@anthropic.com>
Implements the Phase 1 read-only MCP Registry API client: - McpRegistryClient with list_servers, get_server, list_versions, validate - Custom reqwest::Client: 20s timeout, no redirect following (SSRF guard) - Custom PATH_SEGMENT AsciiSet preserving RFC 3986 unreserved chars [-._~] - ServiceClient health impl via GET /v0.1/health - PluginMeta with Category::Bootstrap - 12 wiremock-based unit tests covering encoding, redirects, 404, dotted keys
Wires the mcpregistry SDK (lab-77y5.1) into the dispatch layer, MCP
tool registry, HTTP API, and CLI. All three surfaces delegate to the
shared dispatch layer following the established 5-file layout.
- dispatch/mcpregistry/{catalog,client,params,dispatch}.rs + entry .rs
- cli/mcpregistry.rs — action+kv-param shim with --dry-run/-y
- mcp/services/mcpregistry.rs — thin MCP bridge
- api/services/mcpregistry.rs — axum POST /v1/mcpregistry
- Registered in dispatch.rs, clients.rs, cli.rs, services.rs, router.rs, registry.rs
- Added Serialize derives to ServerListResponse, Metadata, ServerResponse,
ResponseMeta, RegistryExtensions so to_json() works at the dispatch boundary
- Added From<RegistryError> for ToolError in dispatch/error.rs
- mcpregistry = ["lab-apis/mcpregistry"] feature added to lab/Cargo.toml
Adds `lab mcpregistry install <name>` that chains:
1. mcpregistry::dispatch("server.get") — fetch server from registry
2. SSRF validation on remotes[0].url (HTTPS-only + RFC-1918 blocklist)
3. gateway::dispatch("gateway.add") — register as gateway upstream
- Validates with synchronous ToSocketAddrs; annotated as blocking-only
- Documents DNS TOCTOU residual risk at call site
- Defaults gateway_name to last segment after '/' in registry server name
- bearer_token_env stores env var name, never the token value
- 9 unit tests for validate_registry_url + default name derivation
- Fixes registry_and_router test: added mcpregistry to http_router_services
- lab-auth/state.rs: token-bucket RateLimiter for /authorize and
/register endpoints; configurable burst=requests_per_minute,
refill at rate/s; check_authorize_rate_limit() and
check_register_rate_limit() guards added to AuthState
- mcp/catalog.rs + mcp/server.rs: extract CatalogSnapshot into
catalog.rs so it can be shared; remove duplicate snapshot_catalog
impl from server.rs
- dispatch/gateway/catalog.rs: remove stale non-destructive-read
assertion tests (covered by integration path)
- dispatch/gateway/{config,manager}.rs, api/upstream_oauth.rs,
dispatch/deploy/build.rs: minor follow-up cleanup from mcpregistry
and upstream-oauth PR work
- skills/gh-address-comments: update SKILL.md + fetch/mark/summary
scripts; add _bd_utils, ai_triage, close_beads, create_beads,
install_completions, post_reply, pr_changelog, pr_checklist,
pr_status, thread_context scripts
Version: 0.3.5 → 0.4.0 (minor)
Co-authored-by: Claude <noreply@anthropic.com>
The mcpregistry API uses cursor-based pagination (metadata.nextCursor), not offset-based. Fix the catalog ParamSpec to reflect the actual wire contract so callers aren't misled into passing an integer offset.
validate_registry_url uses ToSocketAddrs (synchronous DNS) and must not be called directly from an async context. Wrap the call site in tokio::task::spawn_blocking to avoid blocking the async executor during DNS resolution. Double-unwrap propagates both JoinError and validation errors with distinct context messages.
…way.add validate_gateway_url now rejects non-HTTPS schemes and resolves hostnames to block RFC1918 (10/8, 172.16/12, 192.168/16), loopback, link-local (169.254/16), IPv6 ULA (fc00::/7), IPv6 link-local (fe80::/10), and IPv4-mapped private ranges. Covers both direct IP and DNS-resolved addrs. Update fixture tests that used http://127.0.0.1 (now blocked) to use https://fixture.example.com to preserve test intent without triggering the SSRF guard.
📝 WalkthroughWalkthroughAdds a feature‑gated MCP Registry integration (SDK client, dispatch, API, CLI), many gh-address-comments automation scripts, stricter gateway URL/SSRF validation and auth rate limiting, OAuth popup/error handling and API client normalization, multiple lint/config/toolchain/version bumps, and assorted tests/refactors across the Rust workspace and web admin app. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as lab CLI
participant Dispatch as mcpregistry dispatch
participant Client as McpRegistryClient
participant API as MCP Registry API
participant Gateway as Gateway Manager
participant Pool as UpstreamPool
CLI->>Dispatch: install <name> [--version]
Dispatch->>Client: get_server(name, version)
Client->>API: GET /v0.1/servers/{name}/versions/{version}
API-->>Client: ServerResponse (includes remotes[0].url)
Client-->>Dispatch: ServerResponse
Dispatch->>Dispatch: validate URL (HTTPS, SSRF/IP checks)
Dispatch->>Dispatch: derive gateway_name, build gateway spec
Dispatch->>Gateway: dispatch gateway.add (confirmable)
Gateway->>Pool: reconcile (fetch prompts/resources)
Pool-->>Gateway: success
Gateway-->>Dispatch: result
Dispatch-->>CLI: print "Installed" / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
24 issues found across 58 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/gh-address-comments/scripts/post_reply.py">
<violation number="1" location="skills/gh-address-comments/scripts/post_reply.py:170">
P2: Validate `--workers` before creating `ThreadPoolExecutor`; zero/negative values currently cause a runtime crash.</violation>
</file>
<file name="apps/gateway-admin/components/upstream-oauth/upstream-oauth-card.tsx">
<violation number="1" location="apps/gateway-admin/components/upstream-oauth/upstream-oauth-card.tsx:33">
P1: `window.open(..., 'noopener,noreferrer')` can return `null` even when the popup opens, so this check can incorrectly show a popup-blocked error and stop auth polling.</violation>
</file>
<file name="skills/gh-address-comments/scripts/pr_status.py">
<violation number="1" location="skills/gh-address-comments/scripts/pr_status.py:59">
P2: Review gating uses historical review events instead of each reviewer’s latest state, so superseded change requests can incorrectly block merge readiness.</violation>
<violation number="2" location="skills/gh-address-comments/scripts/pr_status.py:118">
P3: `--repo` is not validated before indexing, so malformed input can crash the script with `IndexError`.</violation>
</file>
<file name="skills/gh-address-comments/scripts/pr_changelog.py">
<violation number="1" location="skills/gh-address-comments/scripts/pr_changelog.py:158">
P2: Validate `--repo` format before indexing split results to avoid an unhandled `IndexError` for invalid input.</violation>
</file>
<file name="skills/gh-address-comments/scripts/ai_triage.py">
<violation number="1" location="skills/gh-address-comments/scripts/ai_triage.py:128">
P2: Check `out.returncode` before `json.loads(out.stdout)` so live-fetch failures are surfaced correctly instead of becoming misleading JSON parse errors.</violation>
</file>
<file name="crates/lab-auth/src/state.rs">
<violation number="1" location="crates/lab-auth/src/state.rs:36">
P1: `requests_per_minute=0` creates a non-refilling limiter (one request then permanent 429) because refill rate is not clamped with `max_tokens`.</violation>
</file>
<file name="crates/lab/src/api/services/mcpregistry.rs">
<violation number="1" location="crates/lab/src/api/services/mcpregistry.rs:31">
P2: `help`/`schema` actions are not handled in the API route and are incorrectly forced through `dispatch_with_client`.</violation>
</file>
<file name="skills/gh-address-comments/scripts/_bd_utils.py">
<violation number="1" location="skills/gh-address-comments/scripts/_bd_utils.py:20">
P2: Avoid calling external `which`; use `shutil.which` so PATH checks don’t crash when `which` is missing.</violation>
</file>
<file name="skills/gh-address-comments/scripts/pr_summary.py">
<violation number="1" location="skills/gh-address-comments/scripts/pr_summary.py:218">
P2: Markdown output ignores `--filter-priority` (and other precomputed filtering) by rendering from `threads` instead of `display_threads`.</violation>
</file>
<file name="skills/gh-address-comments/scripts/install_completions.py">
<violation number="1" location="skills/gh-address-comments/scripts/install_completions.py:127">
P3: `gh-fetch-comments` bash completions are missing the valid `--no-beads` flag.</violation>
<violation number="2" location="skills/gh-address-comments/scripts/install_completions.py:133">
P3: `gh-mark-resolved` bash completions omit the supported `--no-beads` option.</violation>
</file>
<file name="skills/gh-address-comments/scripts/thread_context.py">
<violation number="1" location="skills/gh-address-comments/scripts/thread_context.py:110">
P2: Validate `--repo` format before indexing `parts[1]` to avoid an uncaught `IndexError` on invalid input.</violation>
</file>
<file name="skills/gh-address-comments/scripts/pr_checklist.py">
<violation number="1" location="skills/gh-address-comments/scripts/pr_checklist.py:69">
P1: Approval gating uses raw review events instead of each reviewer’s latest state, which can incorrectly block or incorrectly pass PRs.</violation>
<violation number="2" location="skills/gh-address-comments/scripts/pr_checklist.py:136">
P2: `--repo` parsing can crash with `IndexError` when input is not in `OWNER/REPO` format.</violation>
</file>
<file name="crates/lab/src/oauth/upstream/manager.rs">
<violation number="1" location="crates/lab/src/oauth/upstream/manager.rs:493">
P1: Rebuilding `OAuthClientConfig` from only `client_id` drops dynamic-registration fields (notably `client_secret`), which can break confidential-client token exchange.</violation>
</file>
<file name="crates/lab/src/dispatch/mcpregistry/params.rs">
<violation number="1" location="crates/lab/src/dispatch/mcpregistry/params.rs:14">
P2: `limit` is cast from `u64` to `u32` with `as`, which can silently truncate large inputs. Validate the range (or use `u32::try_from`) and return `invalid_param` when out of bounds.</violation>
</file>
<file name="skills/gh-address-comments/SKILL.md">
<violation number="1" location="skills/gh-address-comments/SKILL.md:127">
P2: The docs claim `--no-beads` works on both commands, but `gh-close-beads` has no such flag.</violation>
<violation number="2" location="skills/gh-address-comments/SKILL.md:192">
P2: The docs reference a `--defer` option that is not implemented by the provided CLI tools.</violation>
</file>
<file name="skills/gh-address-comments/scripts/mark_resolved.py">
<violation number="1" location="skills/gh-address-comments/scripts/mark_resolved.py:177">
P2: Auto bead-closing uses stale cached thread state; add `--refresh` so newly resolved threads are actually closed.</violation>
</file>
<file name="crates/lab/src/dispatch/gateway/config.rs">
<violation number="1" location="crates/lab/src/dispatch/gateway/config.rs:353">
P1: SSRF regression: `0.0.0.0` and `::` (unspecified/bind-all addresses) are no longer blocked. The old explicit check was removed but `check_ip_not_private_tool` doesn't cover these addresses — `is_loopback()` returns false for them. Add `v4.is_unspecified()` / `v6.is_unspecified()` to the blocked conditions.</violation>
</file>
<file name="skills/gh-address-comments/scripts/close_beads.py">
<violation number="1" location="skills/gh-address-comments/scripts/close_beads.py:54">
P2: Do not treat `bd show` command failures as "already closed"; this can silently skip beads that still need closing.</violation>
</file>
<file name="crates/lab/src/cli/mcpregistry.rs">
<violation number="1" location="crates/lab/src/cli/mcpregistry.rs:224">
P1: IPv6 SSRF bypass: only `::1` is blocked, but IPv4-mapped IPv6 (`::ffff:127.0.0.1`), unique-local (`fc00::/7`), and link-local (`fe80::/10`) addresses pass through unchecked. An attacker-controlled registry entry can use these to reach internal services.</violation>
</file>
<file name="crates/lab-apis/src/mcpregistry/types.rs">
<violation number="1" location="crates/lab-apis/src/mcpregistry/types.rs:62">
P1: `#[serde(default)]` does not make `Vec` fields accept JSON `null`; nullable arrays here can still fail deserialization at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| const { authorization_url } = await upstreamOauthApi.start(name) | ||
| window.open(authorization_url, '_blank', 'noopener,noreferrer') | ||
| const popup = window.open(authorization_url, '_blank', 'noopener,noreferrer') |
There was a problem hiding this comment.
P1: window.open(..., 'noopener,noreferrer') can return null even when the popup opens, so this check can incorrectly show a popup-blocked error and stop auth polling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/gateway-admin/components/upstream-oauth/upstream-oauth-card.tsx, line 33:
<comment>`window.open(..., 'noopener,noreferrer')` can return `null` even when the popup opens, so this check can incorrectly show a popup-blocked error and stop auth polling.</comment>
<file context>
@@ -30,7 +30,11 @@ export function UpstreamOauthCard({ name }: UpstreamOauthCardProps) {
try {
const { authorization_url } = await upstreamOauthApi.start(name)
- window.open(authorization_url, '_blank', 'noopener,noreferrer')
+ const popup = window.open(authorization_url, '_blank', 'noopener,noreferrer')
+ if (!popup) {
+ setConnecting(false)
</file context>
| let rate = requests_per_minute as f64 / 60.0; | ||
| // Burst = requests_per_minute: allows consuming the full per-minute quota | ||
| // immediately, then refills at `rate` tokens/second. | ||
| let max_tokens = requests_per_minute.max(1) as f64; |
There was a problem hiding this comment.
P1: requests_per_minute=0 creates a non-refilling limiter (one request then permanent 429) because refill rate is not clamped with max_tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/lab-auth/src/state.rs, line 36:
<comment>`requests_per_minute=0` creates a non-refilling limiter (one request then permanent 429) because refill rate is not clamped with `max_tokens`.</comment>
<file context>
@@ -9,12 +11,63 @@ use crate::google::GoogleProvider;
+ let rate = requests_per_minute as f64 / 60.0;
+ // Burst = requests_per_minute: allows consuming the full per-minute quota
+ // immediately, then refills at `rate` tokens/second.
+ let max_tokens = requests_per_minute.max(1) as f64;
+ Self {
+ inner: Arc::new(Mutex::new(RateLimiterInner {
</file context>
|
|
||
|
|
||
| def check_approvals(reviews: list[dict[str, Any]], required: int) -> tuple[bool, str, str]: | ||
| approved = [r for r in reviews if r.get("state") == "APPROVED"] |
There was a problem hiding this comment.
P1: Approval gating uses raw review events instead of each reviewer’s latest state, which can incorrectly block or incorrectly pass PRs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/gh-address-comments/scripts/pr_checklist.py, line 69:
<comment>Approval gating uses raw review events instead of each reviewer’s latest state, which can incorrectly block or incorrectly pass PRs.</comment>
<file context>
@@ -0,0 +1,206 @@
+
+
+def check_approvals(reviews: list[dict[str, Any]], required: int) -> tuple[bool, str, str]:
+ approved = [r for r in reviews if r.get("state") == "APPROVED"]
+ changes = [r for r in reviews if r.get("state") == "CHANGES_REQUESTED"]
+ if changes:
</file context>
Comment on lines
+493
to
+496
| let mut canonical_cfg = | ||
| OAuthClientConfig::new(canonical_client_id, self.redirect_uri.as_str()); | ||
| canonical_cfg = | ||
| canonical_cfg.with_scopes(scopes.iter().map(|s| s.to_string()).collect()); |
There was a problem hiding this comment.
P1: Rebuilding OAuthClientConfig from only client_id drops dynamic-registration fields (notably client_secret), which can break confidential-client token exchange.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/lab/src/oauth/upstream/manager.rs, line 493:
<comment>Rebuilding `OAuthClientConfig` from only `client_id` drops dynamic-registration fields (notably `client_secret`), which can break confidential-client token exchange.</comment>
<file context>
@@ -473,7 +478,23 @@ impl UpstreamOauthManager {
+ )
+ })?;
+
+ let mut canonical_cfg =
+ OAuthClientConfig::new(canonical_client_id, self.redirect_uri.as_str());
+ canonical_cfg =
</file context>
Suggested change
| let mut canonical_cfg = | |
| OAuthClientConfig::new(canonical_client_id, self.redirect_uri.as_str()); | |
| canonical_cfg = | |
| canonical_cfg.with_scopes(scopes.iter().map(|s| s.to_string()).collect()); | |
| let mut canonical_cfg = cfg; | |
| canonical_cfg.client_id = canonical_client_id; | |
| canonical_cfg = | |
| canonical_cfg.with_scopes(scopes.iter().map(|s| s.to_string()).collect()); |
| let blocked = match ip { | ||
| std::net::IpAddr::V4(v4) => { | ||
| let o = v4.octets(); | ||
| v4.is_loopback() // 127.0.0.0/8 |
There was a problem hiding this comment.
P1: SSRF regression: 0.0.0.0 and :: (unspecified/bind-all addresses) are no longer blocked. The old explicit check was removed but check_ip_not_private_tool doesn't cover these addresses — is_loopback() returns false for them. Add v4.is_unspecified() / v6.is_unspecified() to the blocked conditions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/lab/src/dispatch/gateway/config.rs, line 353:
<comment>SSRF regression: `0.0.0.0` and `::` (unspecified/bind-all addresses) are no longer blocked. The old explicit check was removed but `check_ip_not_private_tool` doesn't cover these addresses — `is_loopback()` returns false for them. Add `v4.is_unspecified()` / `v6.is_unspecified()` to the blocked conditions.</comment>
<file context>
@@ -329,18 +334,68 @@ fn validate_gateway_url(url: &str) -> Result<(), ToolError> {
+ let blocked = match ip {
+ std::net::IpAddr::V4(v4) => {
+ let o = v4.octets();
+ v4.is_loopback() // 127.0.0.0/8
+ || o[0] == 10 // 10.0.0.0/8
+ || (o[0] == 172 && o[1] >= 16 && o[1] <= 31) // 172.16.0.0/12
</file context>
| close_cmd = [ | ||
| sys.executable, | ||
| str(Path(__file__).parent / "close_beads.py"), | ||
| "--input", args.input, |
There was a problem hiding this comment.
P2: Auto bead-closing uses stale cached thread state; add --refresh so newly resolved threads are actually closed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/gh-address-comments/scripts/mark_resolved.py, line 177:
<comment>Auto bead-closing uses stale cached thread state; add `--refresh` so newly resolved threads are actually closed.</comment>
<file context>
@@ -157,6 +163,24 @@ def main() -> None:
+ close_cmd = [
+ sys.executable,
+ str(Path(__file__).parent / "close_beads.py"),
+ "--input", args.input,
+ "--reason", "Thread marked resolved via gh-mark-resolved",
+ ]
</file context>
Suggested change
| "--input", args.input, | |
| "--input", args.input, | |
| "--refresh", |
| if repo: | ||
| cmd += ["--repo", repo] | ||
| result = subprocess.run(cmd, capture_output=True, text=True) | ||
| if result.returncode != 0: |
There was a problem hiding this comment.
P2: Do not treat bd show command failures as "already closed"; this can silently skip beads that still need closing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/gh-address-comments/scripts/close_beads.py, line 54:
<comment>Do not treat `bd show` command failures as "already closed"; this can silently skip beads that still need closing.</comment>
<file context>
@@ -0,0 +1,195 @@
+ if repo:
+ cmd += ["--repo", repo]
+ result = subprocess.run(cmd, capture_output=True, text=True)
+ if result.returncode != 0:
+ print(f"Error fetching live data: {result.stderr}", file=sys.stderr)
+ sys.exit(1)
</file context>
| # Resolve repo | ||
| if args.repo: | ||
| parts = args.repo.split("/", 1) | ||
| owner, repo = parts[0], parts[1] |
There was a problem hiding this comment.
P3: --repo is not validated before indexing, so malformed input can crash the script with IndexError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/gh-address-comments/scripts/pr_status.py, line 118:
<comment>`--repo` is not validated before indexing, so malformed input can crash the script with `IndexError`.</comment>
<file context>
@@ -0,0 +1,168 @@
+ # Resolve repo
+ if args.repo:
+ parts = args.repo.split("/", 1)
+ owner, repo = parts[0], parts[1]
+ else:
+ remote = json.loads(_run(["gh", "repo", "view", "--json", "owner,name"]))
</file context>
|
|
||
| _gh_mark_resolved_completions() { | ||
| local cur="${COMP_WORDS[COMP_CWORD]}" | ||
| local opts="--all --input -i --dry-run --workers --help -h" |
There was a problem hiding this comment.
P3: gh-mark-resolved bash completions omit the supported --no-beads option.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/gh-address-comments/scripts/install_completions.py, line 133:
<comment>`gh-mark-resolved` bash completions omit the supported `--no-beads` option.</comment>
<file context>
@@ -0,0 +1,258 @@
+
+_gh_mark_resolved_completions() {
+ local cur="${COMP_WORDS[COMP_CWORD]}"
+ local opts="--all --input -i --dry-run --workers --help -h"
+ if [[ "$cur" == -* ]]; then
+ COMPREPLY=($(compgen -W "$opts" -- "$cur"))
</file context>
| } | ||
|
|
||
| _gh_fetch_comments_completions() { | ||
| local opts="--pr --repo --output -o --since --help -h" |
There was a problem hiding this comment.
P3: gh-fetch-comments bash completions are missing the valid --no-beads flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/gh-address-comments/scripts/install_completions.py, line 127:
<comment>`gh-fetch-comments` bash completions are missing the valid `--no-beads` flag.</comment>
<file context>
@@ -0,0 +1,258 @@
+}
+
+_gh_fetch_comments_completions() {
+ local opts="--pr --repo --output -o --since --help -h"
+ COMPREPLY=($(compgen -W "$opts" -- "${COMP_WORDS[COMP_CWORD]}"))
+}
</file context>
…e-existing advisories - Drop unnecessary `crate::error::` path prefix from three AuthState methods - Run cargo fmt --all to fix formatting across mcpregistry and gateway files - Add RUSTSEC-2026-0097/0098/0099 and RUSTSEC-2023-0071 to deny.toml ignore list — all pre-exist on main with no upstream patch available
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cb0e45900
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+353
to
+357
| v4.is_loopback() // 127.0.0.0/8 | ||
| || o[0] == 10 // 10.0.0.0/8 | ||
| || (o[0] == 172 && o[1] >= 16 && o[1] <= 31) // 172.16.0.0/12 | ||
| || (o[0] == 192 && o[1] == 168) // 192.168.0.0/16 | ||
| || (o[0] == 169 && o[1] == 254) // 169.254.0.0/16 link-local |
There was a problem hiding this comment.
This SSRF guard now checks loopback/RFC1918/link-local ranges but no longer rejects bind-all literals like 0.0.0.0 (and similarly ::), so a URL such as https://0.0.0.0:... is accepted and can target locally bound services. That undermines the intent of the gateway URL safety filter for hostile or untrusted upstream definitions; add explicit unspecified-address checks before accepting the URL.
Useful? React with 👍 / 👎.
| || (o[0] == 192 && o[1] == 168) // 192.168.0.0/16 | ||
| || (o[0] == 169 && o[1] == 254) // 169.254.0.0/16 link-local / IMDS | ||
| } | ||
| std::net::IpAddr::V6(v6) => v6.is_loopback(), // ::1/128 |
There was a problem hiding this comment.
The registry-install SSRF check only rejects IPv6 loopback (::1) in check_ip_not_private, so hostnames that resolve to ULA/link-local IPv6 addresses (e.g. fc00::/7, fe80::/10) are treated as safe. Because gateway-side URL validation intentionally skips DNS resolution for hostnames, this creates a bypass where private IPv6 endpoints can still be installed and called; the IPv6 branch should block private/link-local ranges and IPv4-mapped private addresses too.
Useful? React with 👍 / 👎.
Comment on lines
+108
to
+111
| let get_result = crate::dispatch::mcpregistry::dispatch( | ||
| "server.get", | ||
| serde_json::json!({ "name": args.name }), | ||
| ) |
There was a problem hiding this comment.
The install command defines a --version option, but the lookup always dispatches server.get with only name, which means it always installs latest regardless of user input. This makes the CLI contract misleading and prevents reproducible installs pinned to a specific registry version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds an MCP Registry integration end-to-end (SDK + dispatch + CLI + HTTP API), alongside security hardening (SSRF/HTTPS validation) and a substantial expansion of the gh-address-comments workflow tooling (thread context, status/checklist/changelog, bead integration, etc.).
Changes:
- Introduces
mcpregistryas a feature-gated service acrosslab-apis(client/types),labdispatch/MCP registry wiring, CLI (lab mcpregistry …), and HTTP API routes. - Hardens gateway/upstream flows: HTTPS-only gateway URLs, private-range blocking for literal IPs, spawn-blocking DNS validation for registry install, and upstream OAuth concurrency/caching fixes.
- Expands PR-review tooling in
skills/gh-address-comments(summary filters/markdown output, thread context viewer, merge-readiness dashboards, bead create/close automation, completions, AI triage).
Reviewed changes
Copilot reviewed 57 out of 59 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/gh-address-comments/scripts/thread_context.py | New helper to print code context for a PR review thread. |
| skills/gh-address-comments/scripts/pr_summary.py | Adds priority detection/sorting, markdown output, and filtering options. |
| skills/gh-address-comments/scripts/pr_status.py | New quick merge-readiness dashboard (CI/reviews/threads/merge state). |
| skills/gh-address-comments/scripts/pr_checklist.py | New pre-merge “gate” checklist with actionable next steps. |
| skills/gh-address-comments/scripts/pr_changelog.py | New changelog generator linking commits to resolved review threads. |
| skills/gh-address-comments/scripts/post_reply.py | New helper to reply to PR review threads (single/all). |
| skills/gh-address-comments/scripts/mark_resolved.py | Adds bead auto-close integration and --no-beads switch. |
| skills/gh-address-comments/scripts/install_completions.py | Adds zsh/bash completion installer and thread-id completion. |
| skills/gh-address-comments/scripts/fetch_comments.py | Adds GraphQL rate-limit warning + completion cache + bead auto-create. |
| skills/gh-address-comments/scripts/create_beads.py | New: create a bead per open PR review thread + mapping file. |
| skills/gh-address-comments/scripts/close_beads.py | New: close beads for threads that are resolved/outdated. |
| skills/gh-address-comments/scripts/ai_triage.py | New: send open threads to Claude CLI for triage/prioritization. |
| skills/gh-address-comments/scripts/_bd_utils.py | New shared bead CLI detection + .beads/ discovery. |
| skills/gh-address-comments/scripts/pycache/_bd_utils.cpython-314.pyc | Adds a compiled Python artifact (should not be committed). |
| skills/gh-address-comments/SKILL.md | Updates skill workflow docs for beads + new helper scripts. |
| docs/UPSTREAM.md | Updates OAuth upstream discovery/catalog behavior documentation. |
| crates/lab/src/registry.rs | Registers mcpregistry tool in the runtime registry + meta wiring. |
| crates/lab/src/oauth/upstream/manager.rs | Reads back canonical dynamic registration after concurrent writes. |
| crates/lab/src/oauth/upstream/cache.rs | Adjusts cache eviction behavior for build locks (race avoidance). |
| crates/lab/src/mcp/services/mcpregistry.rs | New MCP service adapter tests for mcpregistry action catalog. |
| crates/lab/src/mcp/services.rs | Exposes mcpregistry MCP service module under feature gate. |
| crates/lab/src/mcp/server.rs | Moves catalog snapshot type into mcp::catalog. |
| crates/lab/src/mcp/catalog.rs | Adds catalog snapshot helpers and makes them non-test-only. |
| crates/lab/src/dispatch/upstream/pool.rs | Adds cached prompt/resource accessors; improves resource size guard ordering. |
| crates/lab/src/dispatch/mcpregistry/params.rs | New param parsing for mcpregistry dispatch actions. |
| crates/lab/src/dispatch/mcpregistry/dispatch.rs | New dispatch surface for registry browsing actions. |
| crates/lab/src/dispatch/mcpregistry/client.rs | New env-based client construction for mcpregistry. |
| crates/lab/src/dispatch/mcpregistry/catalog.rs | New action catalog for registry browsing (help/schema/server.*). |
| crates/lab/src/dispatch/mcpregistry.rs | New dispatch module entrypoint + basic tests. |
| crates/lab/src/dispatch/gateway/manager.rs | Updates tests/fixtures for new gateway env naming + oauth field. |
| crates/lab/src/dispatch/gateway/dispatch.rs | Updates gateway.add test fixture URL to HTTPS. |
| crates/lab/src/dispatch/gateway/config.rs | Enforces HTTPS-only gateway URLs + blocks private literal IP ranges. |
| crates/lab/src/dispatch/gateway/catalog.rs | Removes a non-destructive test (now presumably covered elsewhere). |
| crates/lab/src/dispatch/error.rs | Adds ToolError conversion for lab_apis::mcpregistry::RegistryError. |
| crates/lab/src/dispatch/deploy/build.rs | Adjusts Windows artifact path test for host vs cross-compile cases. |
| crates/lab/src/dispatch/clients.rs | Adds mcpregistry client to ServiceClients under feature gate. |
| crates/lab/src/dispatch.rs | Exposes dispatch::mcpregistry module under feature gate. |
| crates/lab/src/cli/mcpregistry.rs | New lab mcpregistry CLI with install + SSRF validation. |
| crates/lab/src/cli.rs | Wires lab mcpregistry command under feature gate. |
| crates/lab/src/api/upstream_oauth.rs | Updates tests for callback subject handling signature. |
| crates/lab/src/api/services/mcpregistry.rs | New HTTP route group for mcpregistry action dispatch. |
| crates/lab/src/api/services/gateway.rs | Updates gateway.add API test to HTTPS fixture URL. |
| crates/lab/src/api/services.rs | Exposes API mcpregistry service module under feature gate. |
| crates/lab/src/api/router.rs | Conditionally mounts upstream OAuth routes only when gateway manager exists; mounts mcpregistry service. |
| crates/lab/Cargo.toml | Adds mcpregistry feature and includes it in all. |
| crates/lab-auth/src/state.rs | Implements auth endpoint rate limiting (token bucket). |
| crates/lab-apis/src/mcpregistry/types.rs | New request/response types for MCP Registry v0.1 API. |
| crates/lab-apis/src/mcpregistry/error.rs | New error type for registry client. |
| crates/lab-apis/src/mcpregistry/client.rs | New registry HTTP client with encoding + redirect/timeout hardening + tests. |
| crates/lab-apis/src/mcpregistry.rs | Exposes plugin meta + health implementation for mcpregistry. |
| crates/lab-apis/src/lib.rs | Exposes mcpregistry module under feature gate. |
| crates/lab-apis/Cargo.toml | Adds mcpregistry feature and includes it in all. |
| apps/gateway-admin/lib/api/upstream-oauth-client.ts | Adjusts upstream OAuth client fetch behavior (base URL/headers/credentials). |
| apps/gateway-admin/components/upstream-oauth/upstream-oauth-card.tsx | Handles popup-blocked case when starting OAuth. |
| FUTURE.md | Removes the FUTURE.md document from repo. |
| Cargo.toml | Bumps workspace version to 0.4.0. |
| Cargo.lock | Updates crate versions to 0.4.0. |
| .claude-plugin/plugin.json | Bumps plugin version to 0.4.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,7 +1,19 @@ | |||
| import type { StartResponse, UpstreamEntry, UpstreamOauthStatus } from '@/lib/types/upstream-oauth' | |||
| import { isStandaloneBearerAuthMode } from '../auth/auth-mode' | |||
| import { normalizeGatewayApiBase } from '../gateway-config' | |||
There was a problem hiding this comment.
normalizeGatewayApiBase is imported from ../gateway-config, but that module doesn’t exist under lib/ (the helper lives in lib/api/gateway-config.ts). This will break builds/bundling for the upstream OAuth UI. Update the import path (and keep it consistent with the other API clients in this folder).
Suggested change
| import { normalizeGatewayApiBase } from '../gateway-config' | |
| import { normalizeGatewayApiBase } from './gateway-config' |
Comment on lines
323
to
395
| @@ -329,18 +334,64 @@ fn validate_gateway_url(url: &str) -> Result<(), ToolError> { | |||
| })?; | |||
|
|
|||
| if let Some(host) = parsed.host_str() { | |||
| let normalized = host.trim_start_matches('[').trim_end_matches(']'); | |||
| if normalized == "0.0.0.0" || normalized == "::" { | |||
| return Err(ToolError::InvalidParam { | |||
| message: "gateway URL must not use 0.0.0.0 or :: (bind-all addresses)".to_string(), | |||
| param: "url".to_string(), | |||
| }); | |||
| // Check literal IP addresses for private/loopback ranges. | |||
| // For hostnames we do NOT perform DNS resolution — blocking DNS is | |||
| // forbidden in async dispatch contexts. | |||
| let bare = host.trim_start_matches('[').trim_end_matches(']'); | |||
| if let Ok(ip) = bare.parse::<std::net::IpAddr>() { | |||
| check_ip_not_private_tool(ip, url)?; | |||
| } | |||
| } | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| fn check_ip_not_private_tool(ip: std::net::IpAddr, url: &str) -> Result<(), ToolError> { | |||
| let blocked = match ip { | |||
| std::net::IpAddr::V4(v4) => { | |||
| let o = v4.octets(); | |||
| v4.is_loopback() // 127.0.0.0/8 | |||
| || o[0] == 10 // 10.0.0.0/8 | |||
| || (o[0] == 172 && o[1] >= 16 && o[1] <= 31) // 172.16.0.0/12 | |||
| || (o[0] == 192 && o[1] == 168) // 192.168.0.0/16 | |||
| || (o[0] == 169 && o[1] == 254) // 169.254.0.0/16 link-local | |||
| } | |||
| std::net::IpAddr::V6(v6) => { | |||
| let s = v6.segments(); | |||
| let is_ipv4_mapped = | |||
| s[0] == 0 && s[1] == 0 && s[2] == 0 && s[3] == 0 && s[4] == 0 && s[5] == 0xffff; | |||
| if is_ipv4_mapped { | |||
| // Check the embedded IPv4 address for private ranges. | |||
| let v4 = std::net::Ipv4Addr::new( | |||
| (s[6] >> 8) as u8, | |||
| s[6] as u8, | |||
| (s[7] >> 8) as u8, | |||
| s[7] as u8, | |||
| ); | |||
| let o = v4.octets(); | |||
| v4.is_loopback() | |||
| || o[0] == 10 | |||
| || (o[0] == 172 && o[1] >= 16 && o[1] <= 31) | |||
| || (o[0] == 192 && o[1] == 168) | |||
| || (o[0] == 169 && o[1] == 254) | |||
| } else { | |||
| v6.is_loopback() // ::1/128 | |||
| || (s[0] & 0xfe00) == 0xfc00 // fc00::/7 ULA | |||
| || (s[0] & 0xffc0) == 0xfe80 // fe80::/10 link-local | |||
| } | |||
| } | |||
| }; | |||
| if blocked { | |||
| return Err(ToolError::InvalidParam { | |||
| message: format!( | |||
| "gateway URL resolves to a private/loopback address — blocked to prevent SSRF: {url}" | |||
| ), | |||
| param: "url".to_string(), | |||
| }); | |||
| } | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn lock_path(path: &Path) -> PathBuf { | |||
There was a problem hiding this comment.
validate_gateway_url now checks only the scheme and (for literal IP hosts) some private ranges, but it no longer blocks bind-all/unspecified addresses like 0.0.0.0 and ::. Those are still unsafe/invalid for an outbound gateway target and were previously rejected. Consider explicitly rejecting unspecified/bind-all IPs (and updating the error message text to avoid saying “resolves to” when no DNS resolution is performed).
Comment on lines
+174
to
+232
|
|
||
| /// Validate that a registry-sourced URL is safe to install as a gateway upstream. | ||
| /// | ||
| /// Rejects non-HTTPS schemes and hosts that resolve to private/loopback ranges. | ||
| /// | ||
| /// # MUST NOT call from async context without spawn_blocking | ||
| /// Uses synchronous `ToSocketAddrs` DNS resolution. | ||
| fn validate_registry_url(url: &str) -> Result<()> { | ||
| let parsed = url::Url::parse(url).with_context(|| format!("invalid URL: {url}"))?; | ||
|
|
||
| if parsed.scheme() != "https" { | ||
| return Err(anyhow!( | ||
| "registry URL must use HTTPS, got '{}': {url}", | ||
| parsed.scheme() | ||
| )); | ||
| } | ||
|
|
||
| let host = parsed | ||
| .host_str() | ||
| .ok_or_else(|| anyhow!("URL has no host: {url}"))?; | ||
| let port = parsed.port_or_known_default().unwrap_or(443); | ||
|
|
||
| // Check literal IP addresses directly without DNS. | ||
| if let Ok(addr) = host.parse::<std::net::IpAddr>() { | ||
| return check_ip_not_private(addr, url); | ||
| } | ||
|
|
||
| // Resolve hostname; reject if any resolved address is private. | ||
| // Synchronous DNS call — MUST NOT be called from async without spawn_blocking. | ||
| let addrs: Vec<_> = format!("{host}:{port}") | ||
| .to_socket_addrs() | ||
| .with_context(|| format!("failed to resolve host '{host}'"))? | ||
| .collect(); | ||
|
|
||
| for sock_addr in addrs { | ||
| check_ip_not_private(sock_addr.ip(), url)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn check_ip_not_private(ip: std::net::IpAddr, url: &str) -> Result<()> { | ||
| let blocked = match ip { | ||
| std::net::IpAddr::V4(v4) => { | ||
| let o = v4.octets(); | ||
| v4.is_loopback() // 127.0.0.0/8 | ||
| || o[0] == 10 // 10.0.0.0/8 | ||
| || (o[0] == 172 && o[1] >= 16 && o[1] <= 31) // 172.16.0.0/12 | ||
| || (o[0] == 192 && o[1] == 168) // 192.168.0.0/16 | ||
| || (o[0] == 169 && o[1] == 254) // 169.254.0.0/16 link-local / IMDS | ||
| } | ||
| std::net::IpAddr::V6(v6) => v6.is_loopback(), // ::1/128 | ||
| }; | ||
| if blocked { | ||
| return Err(anyhow!( | ||
| "registry URL resolves to a private/loopback address — blocked to prevent SSRF: {url}" | ||
| )); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
check_ip_not_private only blocks IPv6 loopback (::1) but allows IPv6 ULA (fc00::/7), link-local (fe80::/10), and IPv4-mapped IPv6 private ranges (e.g. ::ffff:192.168.1.1). Since this function is part of the SSRF defense for registry-sourced URLs, it should block the same set of private/loopback ranges as gateway.add (and ideally share a single helper to avoid drift). Add tests covering ULA/link-local/IPv4-mapped cases.
Comment on lines
145
to
162
| @@ -155,7 +158,7 @@ impl OauthClientCache { | |||
| /// upstream's sessions. | |||
| pub fn evict_upstream(&self, upstream: &str) { | |||
| self.clients.retain(|(name, _), _| name != upstream); | |||
| self.build_locks.retain(|(name, _), _| name != upstream); | |||
| // build_locks intentionally preserved — see comment in evict_subject. | |||
| } | |||
There was a problem hiding this comment.
OauthClientCache::evict_subject/evict_upstream no longer remove entries from build_locks. While it avoids a duplicate-build race, it also means build_locks can grow without bound as new (upstream, subject) pairs appear over time. Consider an approach that preserves serialization without unbounded retention (e.g. remove the lock when there is no cached client and no in-flight builder, or use a per-key lock that’s tied to the client entry lifetime).
Comment on lines
+211
to
+219
| # Apply filters | ||
| display_threads = open_threads if args.open_only else threads | ||
| if args.filter_priority: | ||
| level = args.filter_priority[1] # "P1" -> "1" | ||
| display_threads = [t for t in display_threads if _extract_priority(t) == level] | ||
|
|
||
| if args.format == "markdown": | ||
| print(format_markdown(pr, threads, args.open_only)) | ||
| return |
There was a problem hiding this comment.
--filter-priority (and the computed display_threads) is ignored when --format markdown is selected: markdown output always uses the unfiltered threads list. This makes gh-pr-summary --format markdown --filter-priority P1 misleading. Consider passing the already-filtered display_threads into format_markdown (or letting format_markdown accept an optional priority filter) so markdown and text modes behave consistently.
Comment on lines
+12
to
+20
| pub fn list_servers_params(params: &Value) -> Result<ListServersParams, ToolError> { | ||
| let search = params["query"].as_str().map(str::to_string); | ||
| let limit = params["limit"].as_u64().map(|v| v as u32); | ||
| let cursor = params["cursor"].as_str().map(str::to_string); | ||
| Ok(ListServersParams { | ||
| search, | ||
| cursor, | ||
| limit, | ||
| version: None, |
There was a problem hiding this comment.
limit is parsed as u64 and then cast to u32 with as, which will wrap for values > u32::MAX and silently accept nonsensical limits. It also doesn't enforce the advertised 1–100 range. Consider validating the value (and returning ToolError::InvalidParam on overflow/out-of-range) rather than truncating.
Comment on lines
+7
to
+16
| /// Build a `McpRegistry` client from the default-instance env vars. | ||
| /// | ||
| /// Returns `None` if the env var is absent or empty; falls back to the | ||
| /// public default URL when unset. | ||
| /// Called by `AppState` at startup — keep pure (no side effects, no logging). | ||
| pub fn client_from_env() -> Option<McpRegistryClient> { | ||
| let url = env_non_empty("MCPREGISTRY_URL").unwrap_or_else(|| REGISTRY_DEFAULT_URL.to_string()); | ||
| McpRegistryClient::new(&url, Auth::None).ok() | ||
| } | ||
|
|
There was a problem hiding this comment.
The doc comment for client_from_env says it returns None when MCPREGISTRY_URL is unset, but the implementation always falls back to REGISTRY_DEFAULT_URL, so it will return Some unless client construction fails. Please adjust the docs to reflect the actual behavior (or change the behavior if the intent is truly “optional client”).
Comment on lines
1
to
+25
| @@ -9,12 +11,63 @@ use crate::google::GoogleProvider; | |||
| use crate::jwt::SigningKeys; | |||
| use crate::sqlite::SqliteStore; | |||
|
|
|||
| /// Simple token-bucket rate limiter (shared across clones via Arc). | |||
| #[derive(Clone)] | |||
| struct RateLimiter { | |||
| inner: Arc<Mutex<RateLimiterInner>>, | |||
| } | |||
|
|
|||
| struct RateLimiterInner { | |||
| /// Tokens available in the bucket. | |||
| tokens: f64, | |||
| /// Maximum tokens (burst size = 1 per-request burst means max = 1). | |||
| max_tokens: f64, | |||
| /// Refill rate in tokens per second. | |||
There was a problem hiding this comment.
This rate limiter is used from async request handlers (authorize / browser_login), but it relies on std::sync::Mutex. Under load, a contended blocking mutex can stall a Tokio worker thread. Consider using a non-blocking primitive (e.g. tokio::sync::Mutex with an async check_* method, parking_lot::Mutex, or an atomic-based token bucket). Also, the max_tokens field comment suggests a burst of 1, but the implementation sets burst to requests_per_minute, so the comment is misleading.
Comment on lines
+190
to
193
| **Thread resolution failures:** Check the thread ID is correct, you have write permissions, and the thread hasn't been deleted. | ||
|
|
||
| **Verification blocked:** Either address the remaining threads or get explicit user approval to defer them. Document deferred threads as follow-up tasks. | ||
| **Verification blocked:** Defer threads explicitly with user approval. Document deferred threads as beads with `--defer`. | ||
|
|
There was a problem hiding this comment.
This doc references a --defer option that doesn't exist in the scripts added/updated in this PR. This can mislead users during the “verification blocked” path; either document the actual deferral workflow, or implement the referenced flag.
Comment on lines
+5
to
+9
| Completions support: | ||
| - Flag names for all four commands | ||
| - Thread ID completion from a local cache (~/.cache/gh-comments/threads.json) | ||
| - Cache is populated automatically when gh-fetch-comments runs | ||
|
|
There was a problem hiding this comment.
The module-level docstring says “Flag names for all four commands”, but the completion script currently targets 5 commands (and the skill now ships more than that: create/close beads, status/checklist, changelog, thread-context, ai-triage, etc.). Please update the docstring to match what’s actually supported (and/or extend the completion definitions to cover the new commands).
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deny.toml">
<violation number="1" location="deny.toml:10">
P1: Do not ignore the webpki TLS-validation advisories; this project uses Rustls-backed HTTPS clients, so suppressing them can hide exploitable certificate validation flaws in production traffic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # rsa/rsa-der-parser: timing side-channel in RSA (no patch available upstream) | ||
| "RUSTSEC-2023-0071", | ||
| # webpki: URI name constraints ignored (no API impact for our usage) | ||
| "RUSTSEC-2026-0098", |
There was a problem hiding this comment.
P1: Do not ignore the webpki TLS-validation advisories; this project uses Rustls-backed HTTPS clients, so suppressing them can hide exploitable certificate validation flaws in production traffic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deny.toml, line 10:
<comment>Do not ignore the webpki TLS-validation advisories; this project uses Rustls-backed HTTPS clients, so suppressing them can hide exploitable certificate validation flaws in production traffic.</comment>
<file context>
@@ -1,6 +1,16 @@
+ # rsa/rsa-der-parser: timing side-channel in RSA (no patch available upstream)
+ "RUSTSEC-2023-0071",
+ # webpki: URI name constraints ignored (no API impact for our usage)
+ "RUSTSEC-2026-0098",
+ # webpki: wildcard name constraint bypass (no API impact for our usage)
+ "RUSTSEC-2026-0099",
</file context>
Merge origin/main to pick up scaffold markers and other fixes from PR #23. Manually apply three format changes that CI's rustfmt (newer than local) requires: - cli.rs: combine closing brace with trailing scaffold marker comment - source.rs: wrap long insert_once() call to stay within line limit - http.rs: wrap long assert!() calls to stay within line limit
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 39
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/lab/src/api/services/gateway.rs (1)
163-171:⚠️ Potential issue | 🟡 MinorInconsistent fixture URL between happy-path and confirm-guard tests.
The
gateway_add_update_remove_reload_routes_existtest now useshttps://fixture.example.com/mcp(line 133) to comply with the new HTTPS + private-IP SSRF guard, butgateway_destructive_routes_require_confirmstill postshttp://127.0.0.1:9001. With the tightened validation, this request may now fail URL validation before theconfirmgate fires, which would make the test assert422for the wrong reason (URL rejected rather than missing-confirm). Please align the URL with the new rules so the test actually exercises the confirmation path.Proposed fix
- "params":{"spec":{"name":"fixture-http","url":"http://127.0.0.1:9001","bearer_token_env":"FIXTURE_HTTP_TOKEN"}} + "params":{"spec":{"name":"fixture-http","url":"https://fixture.example.com/mcp","bearer_token_env":"FIXTURE_HTTP_TOKEN"}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/api/services/gateway.rs` around lines 163 - 171, The test gateway_destructive_routes_require_confirm posts a URL that will be rejected by the new HTTPS + private-IP SSRF guard; update the payload in gateway_destructive_routes_require_confirm (the call to post_gateway_fresh) to use the same approved HTTPS fixture URL used in gateway_add_update_remove_reload_routes_exist (e.g. https://fixture.example.com/mcp or another allowed HTTPS host) so the request passes URL validation and the test actually exercises the missing-confirm logic rather than failing on URL validation.crates/lab/src/oauth/upstream/cache.rs (1)
145-162:⚠️ Potential issue | 🟡 Minor
build_locksnow grows unboundedly.Preserving entries on eviction is correct to prevent the double-build race, but
build_locksnow has no GC path — every(upstream, subject)pair ever seen retains aArc<Mutex<()>>for the lifetime of the process. For a long-running gateway with many churning subjects (e.g. short-lived users or per-session identities), this is a slow leak.Consider one of:
- After releasing the guard in
get_or_insert_with, opportunistically remove the lock entry whenArc::strong_count == 1(no other waiters), since a fresh build will recreate it correctly.- Track a last-used timestamp and sweep entries older than N on a background task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/oauth/upstream/cache.rs` around lines 145 - 162, The build_locks map is never garbage-collected, causing an unbounded memory leak; update the locking logic so lock entries are removed when no longer needed: in the code that acquires/releases the per-key lock (the get_or_insert_with path that creates or awaits the Arc<Mutex<()>>), after dropping the guard check Arc::strong_count(&lock_arc) and if it equals 1 (meaning only the map holds the Arc) remove the key from build_locks; keep evict_subject and evict_upstream behavior unchanged (they should not remove locks proactively), but ensure removal is done with appropriate map locking to avoid races when checking strong_count and deleting the entry.crates/lab/src/oauth/upstream/manager.rs (1)
465-497:⚠️ Potential issue | 🟡 MinorRead-back narrows but does not eliminate the TOCTOU; also leaks an orphan client registration on the AS for losing writers.
Two observations on the new read-back:
The window is still open: between
save_dynamic_client_registration(line 476-479) andfind_dynamic_client_registration(line 482-491), a third concurrent task can perform its ownregister_client+ save, and this task will then read back that task'sclient_id— not the one we just registered with the AS. Sinceregister_clientalready succeeded against the AS with ourcfg.client_id, we subsequently configure the manager with a different (also valid) registration, so the AS call wastes a registration and leaves an orphan client record at the AS for each losing writer.This is unavoidable without serializing the
register_client→ save pair under a per-(upstream, subject)lock (similar toOauthClientCache::build_locks). Given this path is called frombegin_authorization/configured_authorization_managerwhich are already per-subject, adding a dedicatedDashMap<(String,String), Arc<Mutex<()>>>around the "register + save + read-back" block would make this race-free and prevent AS-side orphan registrations.The current fix is a strict improvement over returning
cfg.client_iddirectly, so this is not a blocker — but worth tracking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/oauth/upstream/manager.rs` around lines 465 - 497, This code still has a TOCTOU race between manager.register_client, sqlite.save_dynamic_client_registration, and sqlite.find_dynamic_client_registration that can produce orphan AS registrations; fix by serializing the whole "register + save + read-back" sequence under a per-(upstream, subject) mutex (e.g., a DashMap<(String,String), Arc<Mutex<()>>> or reuse the existing OauthClientCache::build_locks), acquiring the lock in the function that does register_client/save_dynamic_client_registration/find_dynamic_client_registration (called from begin_authorization / configured_authorization_manager), then perform register_client, save_dynamic_client_registration and find_dynamic_client_registration while holding the lock and release afterwards so losing concurrent tasks read the canonical DB entry and do not register duplicate clients on the AS.crates/lab/src/dispatch/gateway/config.rs (1)
236-258:⚠️ Potential issue | 🟡 MinorReject whitespace in
bearer_token_envinstead of validating the trimmed value.
validate_bearer_token_env_name()validatestrimmed, but callers keep the original string. A value like" GH_TOKEN "passes validation and is persisted as an invalid env-var name.Suggested fix
pub(crate) fn validate_bearer_token_env_name(value: &str) -> Result<(), ToolError> { let trimmed = value.trim(); if trimmed.is_empty() { return Err(ToolError::InvalidParam { message: "bearer token env var must not be empty".to_string(), param: "bearer_token_env".to_string(), }); } + if trimmed != value { + return Err(ToolError::InvalidParam { + message: "bearer_token_env must not contain leading or trailing whitespace".to_string(), + param: "bearer_token_env".to_string(), + }); + } if looks_like_raw_bearer_token(trimmed) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/dispatch/gateway/config.rs` around lines 236 - 258, validate_bearer_token_env_name currently validates the trimmed string but returns success for inputs that contain leading/trailing whitespace while callers persist the original value; change the function (validate_bearer_token_env_name) to reject values that differ from value.trim() (i.e., detect and error on any surrounding whitespace) or alternatively return/propagate the trimmed value so callers store a valid env var name; ensure the error uses the same ToolError::InvalidParam shape and message (e.g., "bearer_token_env must not contain leading or trailing whitespace") and keep the existing checks (empty, raw token, and is_valid_env_var_name) applied to the trimmed form.skills/gh-address-comments/scripts/pr_summary.py (1)
22-59:⚠️ Potential issue | 🟡 MinorPlain
P0-P3fallback regex is prone to false positives.
\bP([0-3])\bwill match many non-priority tokens that appear in review bodies — JIRA-style refs (ABC-P1-ish patterns don't match, but bareP1in prose such as "CPU P1 state", "P2P", or generic headings does). Since this drives both grouping and--filter-priority, mis-tagged threads will silently float to the wrong priority bucket (and untagged ones may get promoted to P0).Consider tightening the fallback to require contextual markers (leading
**,[P1],Priority: P1, start-of-line, etc.) or scope it to the first comment / first line only to reduce noise. At minimum, it's worth documenting the heuristic's limits in the help text so users understand why a thread was bucketed.♻️ Possible tightening
-_PRIORITY_RE = re.compile(r"\bP([0-3])\b") +# Plain mention: only match common annotation forms like "[P1]", "**P1**", +# "Priority: P1", or an "P1" token adjacent to a badge-like marker. +_PRIORITY_RE = re.compile(r"(?:\[|\*\*|priority[:\s]+)P([0-3])\b", re.IGNORECASE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/gh-address-comments/scripts/pr_summary.py` around lines 22 - 59, The fallback priority regex _PRIORITY_RE is too permissive and causes false positives; tighten _extract_priority to only consider clearly contextual forms (e.g. start-of-line markers, bracketed tokens like [P1], "Priority: P1", or bolded "**P1**") instead of any bare "P1", and prefer scanning only the first comment (use _first_comment) or the first line of a comment for the fallback check to reduce noise; update the regex constant _PRIORITY_RE (and related logic in _extract_priority) to match those contextual patterns and adjust any help text to document the heuristic limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/lab-apis/src/mcpregistry/client.rs`:
- Around line 96-104: The get_server function currently encodes only name and
interpolates version raw into path; update get_server to validate and
percent-encode the version as a path segment before building the path. Call a
validation helper (e.g., Self::validate_version(version)? — implement it if
missing) and an encoding helper (e.g., let encoded_version =
Self::encode_name(version) or add a dedicated encode_segment) and then use
format!("/v0.1/servers/{encoded}/versions/{encoded_version}") so untrusted
characters like '/', '?', and '#' cannot break the request path.
- Around line 47-51: The request timeout was set to 20s in the reqwest client
builder; change the timeout to 30s by updating the
.timeout(Duration::from_secs(20)) call to .timeout(Duration::from_secs(30)) in
the client construction (the chain that builds `inner` via
reqwest::Client::builder()), keep connect_timeout at 5s, and update the doc
comment above the client creation that describes the timeout to state 30s
instead of 20s.
In `@crates/lab-apis/src/mcpregistry/types.rs`:
- Around line 1-7: The Vec fields (e.g., fields named servers, issues and other
Vec<T> members in the MCP Registry types) currently only use #[serde(default)]
which does not accept explicit JSON null and will fail deserialization; add a
custom deserializer that treats null as an empty Vec and apply it to each
nullable Vec field by annotating them with #[serde(default, deserialize_with =
"null_to_vec::<T>")] (or the appropriate generic helper), and add the helper
function (e.g., fn null_to_vec<'de, D, T>(d: D) -> Result<Vec<T>, D::Error>
where D: Deserializer<'de>, T: Deserialize<'de'> {
Option::deserialize(d).map(|opt: Option<Vec<T>>| opt.unwrap_or_default()) }) so
any servers, issues, or other Vec fields accept JSON null as empty.
In `@crates/lab-auth/src/state.rs`:
- Around line 47-60: The token-bucket logic in try_acquire (and the similar
guard at lines 116-137) must return the actual retry delay instead of a fixed
60_000ms; compute the time until tokens reach 1.0 using inner.tokens,
inner.refill_rate and last_refill (if tokens >= 1.0 -> zero delay, else
delay_secs = (1.0 - inner.tokens) / inner.refill_rate), convert to milliseconds
and return or surface that value to the guards so they set retry_after_ms to the
computed ms; update the try_acquire signature/return to include the retry
duration (or provide a helper like compute_retry_after_ms(inner:
&TokenBucketInner) -> u64) and use that helper from the other guard code paths
to replace the constant 60_000.
- Around line 32-42: AuthConfig::validate() currently doesn't reject zero values
for authorize_requests_per_minute and register_requests_per_minute which allows
RateLimiter::new(0) to create a non-refilling limiter; add validation to
AuthConfig::validate() to ensure authorize_requests_per_minute > 0 and
register_requests_per_minute > 0 and return an appropriate error if not
(reference the fields authorize_requests_per_minute and
register_requests_per_minute), or alternatively change
RateLimiter::new(requests_per_minute: u32) to return a Result and explicitly
error when requests_per_minute == 0 so callers cannot construct a zero-refill
limiter.
In `@crates/lab/src/api/router.rs`:
- Around line 323-333: The frontend expects JSON endpoints under
/v1/gateway/oauth even when state.gateway_manager is None, but currently those
routes fall through to the SPA asset handler; update router.rs so that when
state.gateway_manager.is_none() you mount a small stub at the same path (use
v1.nest("/gateway/oauth", ...) or a dedicated handler) that returns a structured
501 Not Implemented JSON envelope (consistent with your API error format) for
the upstream endpoints, or alternatively expose a capability flag (e.g., include
a boolean like "gateway_oauth_enabled" from /auth/session or /v1/capabilities)
so the gateway-admin UI (apps/gateway-admin/lib/api/upstream-oauth-client.ts)
can hide the UI; reference gateway_manager and
crate::api::upstream_oauth::gateway_routes to locate where to add the stub or
capability wiring.
In `@crates/lab/src/api/services.rs`:
- Around line 40-43: The two feature-gated module declarations #[cfg(feature =
"mcpregistry")] pub mod mcpregistry; and #[cfg(feature = "memos")] pub mod
memos; are placed without the customary blank line between them; add a single
blank line between the mcpregistry and memos module declarations so the file
matches the formatting used elsewhere (e.g., the pattern used in
crates/lab/src/mcp/services.rs).
In `@crates/lab/src/cli/mcpregistry.rs`:
- Around line 159-172: The extra println!("Installed: {gateway_name} → {url}")
prints human text unconditionally and can corrupt structured output produced by
run_confirmable_action_command(..., "gateway.add", ..., format, ...). Remove
that unconditional println and instead emit the result via the existing output
helper that honors the OutputFormat passed into run_confirmable_action_command
(or only print the human-friendly message when format is the human/text
variant); update the gateway.add success path so all output goes through the
same formatter used by run_confirmable_action_command.
- Around line 215-226: The check_ip_not_private function currently only treats
IPv6 loopback as blocked; update its IPv6 branch to also block unspecified (::),
ULA (fd00::/7), link-local (fe80::/10), and IPv4-mapped addresses that
correspond to private/loopback IPv4 ranges so the CLI DNS SSRF checks match
gateway URL blocking, and factor this logic into a shared helper (used by both
check_ip_not_private and the gateway validator) to avoid drift between CLI and
gateway validation.
In `@crates/lab/src/dispatch/gateway/config.rs`:
- Around line 349-383: The check_ip_not_private_tool function misses
bind-all/unspecified addresses (0.0.0.0 and ::) so HTTPS URLs like
https://0.0.0.0/... pass; update the match arms in check_ip_not_private_tool to
treat IPv4 unspecified (v4.is_unspecified() / octets == [0,0,0,0]) and IPv6
unspecified (v6.is_unspecified() / all-zero segments) as blocked in addition to
loopback/RFC1918/ULA/link-local checks so bind-all addresses are rejected
regardless of scheme; modify the IPv4 arm to include v4.is_unspecified() and the
IPv6 arm to include v6.is_unspecified() (and likewise in the IPv4-mapped
handling) so those cases return Err(ToolError) the same way as other private
addresses.
- Around line 336-343: The hostname SSRF bypass occurs because the gateway URL
validation only checks literal IPs (parsed.host_str() path) and skips DNS
resolution; update the validation in config.rs to resolve hostnames (use
std::net::ToSocketAddrs / to_socket_addrs()) inside the blocking context (the
same spawn_blocking pattern used for manager.rs:1079) and ensure every resolved
IpAddr is passed to check_ip_not_private_tool(url_ip, url) before accepting the
upstream; follow the same approach as validate_registry_url() in
cli/mcpregistry.rs to iterate all socket addrs and reject if any address is
private/loopback.
In `@crates/lab/src/dispatch/mcpregistry/client.rs`:
- Around line 7-14: The doc comment for client_from_env() is inaccurate: it
claims an absent/empty env var yields None, but the code uses
env_non_empty(...).unwrap_or_else(|| REGISTRY_DEFAULT_URL.to_string()) and thus
constructs a default public-registry client; the function only returns None when
McpRegistryClient::new(...) fails. Fix by updating the comment for
client_from_env() to state that it uses REGISTRY_DEFAULT_URL when
MCPREGISTRY_URL is unset/empty and returns Some(McpRegistryClient) on success or
None only if McpRegistryClient::new(...) fails (keep it pure and side-effect
free), or alternatively change the implementation to return None when
env_non_empty(...) is None if you want the original contract—reference
client_from_env, env_non_empty, REGISTRY_DEFAULT_URL, and McpRegistryClient::new
when making the change.
In `@crates/lab/src/dispatch/mcpregistry/dispatch.rs`:
- Around line 19-21: The "server.get" handler always passes the literal "latest"
to client.get_server, ignoring any requested version; update the match arm for
"server.get" to read an optional version param (e.g., via params::require_name
and a new params::require_optional_version or params::get_optional_str) and call
client.get_server(&name, version_or_latest).await? so the provided version is
honored (fall back to "latest" when absent). Also add an optional "version"
catalog param and validation in params.rs to reject empty version strings
(return an error for empty string) so CLI/API versions like lab mcpregistry
install --version ... are respected and invalid empty versions are rejected;
reference the "server.get" match arm and the client.get_server and params.rs
helpers when making changes.
- Around line 14-31: The pre-built-client dispatch in dispatch.rs must handle
the built-in "help" and "schema" actions before forwarding to service-specific
cases; update the match in dispatch_with_client (the block matching on action)
to include "help" and "schema" branches that invoke the same built-in handling
used by handle_action (or call handle_action/whatever helper produces the
help/schema payload using ACTIONS and params_value) so those requests return the
built-in payloads instead of falling through to the unknown action arm; ensure
you reference ACTIONS and reuse the existing help/schema generation logic to
keep behavior consistent with handle_action.
In `@crates/lab/src/dispatch/mcpregistry/params.rs`:
- Around line 12-33: In list_servers_params, replace the unchecked cast of
params["limit"].as_u64().map(|v| v as u32) with a checked conversion (e.g., use
u32::try_from or explicit range check) and return ToolError::InvalidParam when
the numeric value is present but out of u32 range so oversized limits do not
wrap; in require_name, distinguish the three cases: (1)
params.get("name").and_then(|v| v.as_str()) == Some("") -> return
ToolError::MissingParam, (2) params.get("name").is_some() but as_str() is None
-> return ToolError::InvalidParam indicating wrong type, and (3) Some(non-empty)
-> Ok(string); reference the functions list_servers_params and require_name and
use params.get(...) rather than indexing to detect presence.
In `@crates/lab/src/dispatch/upstream/pool.rs`:
- Around line 714-732: The cached_upstream_resource_uris and
cached_upstream_prompt_names helpers read entry.resource_uris and
entry.prompt_names which remain empty because discovery updates only counts;
modify the code that performs discovery (the list_resources and list_prompts
call sites) to persist discovered URIs/names back into the catalog entry (e.g.,
update the entry.resource_uris and entry.prompt_names fields under the same
catalog write lock) so snapshot_catalog sees them, or alternatively change these
helpers to derive values directly from whatever discovery result store is
already populated; locate the discovery logic that updates counts and add an
assignment to store the full Vec<String> into the catalog entry for the
corresponding upstream.
In `@crates/lab/src/mcp/server.rs`:
- Line 1122: The import use crate::mcp::catalog::CatalogSnapshot; is placed
between impl ServerHandler and impl LabMcpServer; move that use to the top
imports block to match file convention. Locate the stray import
(CatalogSnapshot) and cut it into the existing top-level use block (the group of
use statements near the top of the file), ensuring no duplicate imports remain
and that the impl ServerHandler and impl LabMcpServer sections are unchanged.
In `@crates/lab/src/mcp/services.rs`:
- Around line 43-45: Insert a blank line between the two feature-gated module
declarations so the #[cfg(feature = "mcpregistry")] pub mod mcpregistry; block
is separated from the #[cfg(feature = "memos")] pub mod memos; block; locate the
two symbols mcpregistry and memos in the services.rs module and add a single
empty line between their cfg declarations to match the existing file spacing
convention.
In `@deny.toml`:
- Around line 4-13: Update the comments in deny.toml for the advisories
RUSTSEC-2026-0097, RUSTSEC-2026-0098, RUSTSEC-2026-0099, and RUSTSEC-2023-0071
to accurately state patch availability (RUSTSEC-2026-0097 patched in rand
>=0.10.1/0.9.3/0.8.6; RUSTSEC-2026-0098 and RUSTSEC-2026-0099 patched in
webpki/rustls >=0.104.0-alpha.6) and the exploitation conditions, verify current
dependency versions in Cargo.lock against those patched versions to decide if
upgrades are feasible, keep RUSTSEC-2023-0071 marked as unpatched but add a
tracking issue and documented risk acceptance, add documented usage analysis for
webpki name-constraint impact, and create a periodic (e.g., quarterly) review
item to re-check these advisories.
In `@skills/gh-address-comments/scripts/_bd_utils.py`:
- Around line 19-20: The _bd_in_path function currently shells out to the system
which command; replace that with shutil.which to avoid spawning a subprocess and
to satisfy Ruff's S607 warning. Add an import for shutil and change _bd_in_path
to return whether shutil.which("bd") is not None (i.e., use shutil.which with
the "bd" executable name instead of subprocess.run(["which", "bd"], ...)).
Ensure the new implementation still returns a bool from _bd_in_path.
In `@skills/gh-address-comments/scripts/ai_triage.py`:
- Line 142: The print call `print(f"Sending to Claude...\n")` uses an
unnecessary f-string; remove the leading `f` so it becomes a plain string
literal (i.e., change to `print("Sending to Claude...\n")`) to satisfy Ruff F541
and eliminate the stray f prefix in ai_triage.py.
- Around line 114-131: The code uses fragile path construction via
__file__.replace("ai_triage.py", "fetch_comments.py") and fails to check
subprocess return code before json.loads, so update the call that builds the
fetch script path to use Path(__file__).parent / "fetch_comments.py" (add from
pathlib import Path at the top) and after subprocess.run(store as out)
explicitly check out.returncode; if non-zero, print out.stderr to sys.stderr and
exit non-zero, otherwise parse out.stdout with json.loads; keep existing
handling for args.input/args.repo and continue to raise OSError/JSONDecodeError
as before.
In `@skills/gh-address-comments/scripts/close_beads.py`:
- Around line 60-70: The function bead_exists_and_open currently treats any
non-zero bd show result as False (already closed) and swallows JSON parse
errors; change bead_exists_and_open to not assume closed on bd failures: when
result.returncode != 0 raise an explicit error (include
result.stderr/returncode) so the caller fails rather than skipping the bead, and
on json.JSONDecodeError also raise an error instead of returning True; keep the
normal path of parsing JSON and returning status not in ("closed",) unchanged.
Ensure the raised error contains the bead_id and bd output for debugging.
In `@skills/gh-address-comments/scripts/create_beads.py`:
- Around line 210-227: The loop over to_create currently ignores create_bead
failures (when create_bead returns None) so the script can exit 0; change it to
detect failures: introduce a boolean (e.g., failed) or failure_count and set it
when bead_id is None and args.dry_run is False; after the loop, if failed is
True then print a clear error message and exit non‑zero (sys.exit(1)); otherwise
continue to save_mapping(mpath, mapping) and print the success message as
before. Ensure you only treat "dry-run" sentinel as success and only mark real
None results as failures.
- Around line 138-146: The load_mapping function currently swallows
OSError/JSONDecodeError and returns an empty dict, which can lead to recreating
beads for threads with corrupt/unreadable mapping files; change load_mapping to
fail fast by not returning {} on read/parse errors — catch exceptions only to
add context and then re-raise (or raise a new descriptive exception) including
mapping_path and the original exception (handle json.JSONDecodeError and
OSError), so callers see the failure instead of silently proceeding with an
empty mapping.
In `@skills/gh-address-comments/scripts/fetch_comments.py`:
- Around line 136-145: The reset-time string built when pct < 20 uses
datetime.fromtimestamp(reset_at) and formats reset_dt with '%H:%M:%S', which
produces a naive local time with no timezone; change this to create an aware UTC
datetime (e.g., use datetime.fromtimestamp(reset_at, tz=datetime.timezone.utc)
or datetime.utcfromtimestamp(...) then set tzinfo) and include a timezone marker
in the format (e.g., '%H:%M:%S %Z' or an ISO8601 timestamp) when building
reset_str so reset_at / reset_dt / reset_str clearly indicate the timezone in
the printed message.
In `@skills/gh-address-comments/scripts/install_completions.py`:
- Around line 23-104: The ZSH and BASH completion blocks are missing several
commands advertised in SKILL.md; update the ZSH _gh_* declarations and the
"#compdef" header plus the case "$service" dispatch (symbols: ZSH_COMPLETION,
_gh_fetch_comments, _gh_mark_resolved, _gh_verify_resolution, _gh_pr_summary,
_gh_post_reply, and BASH_COMPLETION) to include the additional commands
(gh-create-beads, gh-close-beads, gh-ai-triage, gh-thread-context, gh-pr-status,
gh-pr-checklist, gh-pr-changelog) with appropriate completion functions or
remove those command names from SKILL.md so the docs and completion lists match;
ensure you add corresponding completion functions or entries in both the ZSH
`#compdef` line and the case "$service" switch and mirror the same changes in the
BASH_COMPLETION block.
- Around line 172-189: The constant CACHE_HOOK is defined but never used; either
remove the dead constant or expose it via the install workflow: add a CLI flag
(e.g., --with-hook) to the installer that when set writes or prints CACHE_HOOK
into the user's shell hook and/or appends the snippet to the post-install
message, and ensure the emitted snippet contains the _gh_update_thread_cache
function name so users can source or add it to their post-command hook; update
the installer entrypoint (the function that runs the install logic) to
conditionally output or write CACHE_HOOK when the flag is present and add a
short test that verifies the hook is emitted when the flag is used.
- Around line 246-250: The print statements in install_completions.py use
f-string prefixes where no interpolation occurs (causing Ruff F541); remove the
unnecessary leading "f" from the literal strings while keeping the other prints
that do interpolate variables (e.g., the lines printing the static messages such
as "If completions don't work immediately, add this to your shell config:" and
"Thread ID completions read from: ~/.cache/gh-comments/threads.json"), and
ensure only the prints that include variables like source_hint and shell remain
as f-strings (or formatted otherwise) so that only the necessary statements
perform interpolation.
In `@skills/gh-address-comments/scripts/mark_resolved.py`:
- Line 173: Remove the unnecessary f-string prefix on the print call in
mark_resolved.py: replace the print that currently uses print(f"\nClosing
beads...") with a plain string print (no interpolation) so the stray `f` is
dropped; search for the print containing "Closing beads..." to locate and update
it.
In `@skills/gh-address-comments/scripts/post_reply.py`:
- Around line 126-136: The CLI has a positional collision between "thread_id"
and "message" when using --all; change the positional "message" into an explicit
optional flag (e.g., parser.add_argument("--message", "-m", metavar="MESSAGE",
help="Reply text (omit when using --commit)")) so that a free string after --all
isn't captured as thread_id; update any logic that reads args.message and the
help text accordingly, and keep the mutually exclusive group only for the
thread_id vs --all choice (leave parser.add_mutually_exclusive_group and
group.add_argument("thread_id", ...) as-is).
- Around line 30-43: Remove the unused constant THREAD_PR_QUERY and its
explanatory comment from this module since no code references it; if it was
intentionally left for a future REST-fallback, instead keep a single-line TODO
above where the fallback will live that references the work-item/issue ID and
explains why the query is kept, but do not leave the full GraphQL query string
unused in the file.
In `@skills/gh-address-comments/scripts/pr_changelog.py`:
- Around line 25-28: The regex _THREAD_REF_RE is currently using \S+ which
captures trailing punctuation (e.g. "PRRT_abc.") causing lookups to fail; change
the pattern to only capture the ID characters and stop at word boundaries (for
example use re.compile(r"Resolves review thread (PRRT_[A-Za-z0-9_]+)\\b",
re.IGNORECASE) or an explicit class excluding punctuation) and update the other
occurrence(s) that use the \S+-based PRRT capture (the similar pattern
referenced in lines 48-52) to the same safer pattern; alternatively, after
matching, normalize the captured id with id = id.rstrip('.,;:!?)') before using
it.
- Around line 197-211: Filter out open threads when building changelog entries:
inside the commits loop that calls parse_thread_refs(commit) and looks up thread
= thread_index.get(tid), only treat a referenced thread as "addressed" if the
thread object indicates it is resolved (check thread.get("isResolved") or
thread.get("resolved") or thread.get("state") == "RESOLVED" as appropriate for
your data). Only add tid to linked_thread_ids and only append
thread_summary(thread) (or the "(thread data not found)" fallback) for threads
that are resolved; skip open threads entirely and only append an entries item
for a commit if it has at least one resolved thread summary.
In `@skills/gh-address-comments/scripts/pr_checklist.py`:
- Around line 68-79: The check_approvals function currently counts every review
record instead of using each reviewer's latest state; update check_approvals to
first collapse reviews by reviewer login by selecting the review with the
greatest submittedAt timestamp (use r["author"]["login"] and
r.get("submittedAt") to compare), then compute approved and changes lists from
that deduplicated set (checking r.get("state") == "APPROVED" or
"CHANGES_REQUESTED"); preserve existing return messages and fix suggestion logic
but base counts and names on the per-author latest review.
In `@skills/gh-address-comments/scripts/pr_status.py`:
- Around line 132-147: The current draft PR check only prints a warning so
drafts can still be marked READY TO MERGE; modify the logic in the block that
handles pr.get("isDraft") to append a failing entry to the checks list (e.g.
checks.append(("Draft", False, "PR is a draft"))) so the draft status is treated
as a failing check by the existing aggregator that consumes checks; update the
code near the calls to check_ci, check_reviews, and check_threads (referencing
pr.get("isDraft"), checks, check_ci, check_reviews, check_threads) so drafts
always cause overall merge readiness to fail.
- Around line 58-67: The check_reviews function is counting every historical
review instead of only the latest per reviewer; update check_reviews to first
reduce the reviews list into a map of reviewer -> most recent state by iterating
reviews in chronological order and overwriting entries for the same
r["author"]["login"] (skip reviews without author), then compute approvals and
changes from that map (instead of the original lists) and build the
names/summary from the map entries; keep the function signature and return
values as-is.
In `@skills/gh-address-comments/scripts/thread_context.py`:
- Around line 180-195: The except block catching failures when fetching context
(around get_head_sha, fetch_file_at_ref and show_context) currently prints
errors but allows the script to exit zero; change it to exit non‑zero after
printing the messages (e.g., call sys.exit(1)) so automation sees a failure when
fetch_file_at_ref or get_head_sha fails; ensure the error branch still prints
the two messages before exiting.
In `@skills/gh-address-comments/SKILL.md`:
- Around line 70-88: The workflow headings skip "3)"; update the section
headings so they read sequentially (either restore a missing "3) Select threads"
step or renumber subsequent headings). Locate the existing heading "### 2) Show
a summary and triage" and the heading "### 4) Apply fixes with commit linking"
in SKILL.md and change the later headings ("### 4)" through "### 8)") to "###
3)" through "### 7)" (or insert a clear "### 3) Select threads" section as
appropriate) so the numbering flows 1→7 consistently; ensure any internal
references to these section numbers are updated accordingly.
---
Outside diff comments:
In `@crates/lab/src/api/services/gateway.rs`:
- Around line 163-171: The test gateway_destructive_routes_require_confirm posts
a URL that will be rejected by the new HTTPS + private-IP SSRF guard; update the
payload in gateway_destructive_routes_require_confirm (the call to
post_gateway_fresh) to use the same approved HTTPS fixture URL used in
gateway_add_update_remove_reload_routes_exist (e.g.
https://fixture.example.com/mcp or another allowed HTTPS host) so the request
passes URL validation and the test actually exercises the missing-confirm logic
rather than failing on URL validation.
In `@crates/lab/src/dispatch/gateway/config.rs`:
- Around line 236-258: validate_bearer_token_env_name currently validates the
trimmed string but returns success for inputs that contain leading/trailing
whitespace while callers persist the original value; change the function
(validate_bearer_token_env_name) to reject values that differ from value.trim()
(i.e., detect and error on any surrounding whitespace) or alternatively
return/propagate the trimmed value so callers store a valid env var name; ensure
the error uses the same ToolError::InvalidParam shape and message (e.g.,
"bearer_token_env must not contain leading or trailing whitespace") and keep the
existing checks (empty, raw token, and is_valid_env_var_name) applied to the
trimmed form.
In `@crates/lab/src/oauth/upstream/cache.rs`:
- Around line 145-162: The build_locks map is never garbage-collected, causing
an unbounded memory leak; update the locking logic so lock entries are removed
when no longer needed: in the code that acquires/releases the per-key lock (the
get_or_insert_with path that creates or awaits the Arc<Mutex<()>>), after
dropping the guard check Arc::strong_count(&lock_arc) and if it equals 1
(meaning only the map holds the Arc) remove the key from build_locks; keep
evict_subject and evict_upstream behavior unchanged (they should not remove
locks proactively), but ensure removal is done with appropriate map locking to
avoid races when checking strong_count and deleting the entry.
In `@crates/lab/src/oauth/upstream/manager.rs`:
- Around line 465-497: This code still has a TOCTOU race between
manager.register_client, sqlite.save_dynamic_client_registration, and
sqlite.find_dynamic_client_registration that can produce orphan AS
registrations; fix by serializing the whole "register + save + read-back"
sequence under a per-(upstream, subject) mutex (e.g., a DashMap<(String,String),
Arc<Mutex<()>>> or reuse the existing OauthClientCache::build_locks), acquiring
the lock in the function that does
register_client/save_dynamic_client_registration/find_dynamic_client_registration
(called from begin_authorization / configured_authorization_manager), then
perform register_client, save_dynamic_client_registration and
find_dynamic_client_registration while holding the lock and release afterwards
so losing concurrent tasks read the canonical DB entry and do not register
duplicate clients on the AS.
In `@skills/gh-address-comments/scripts/pr_summary.py`:
- Around line 22-59: The fallback priority regex _PRIORITY_RE is too permissive
and causes false positives; tighten _extract_priority to only consider clearly
contextual forms (e.g. start-of-line markers, bracketed tokens like [P1],
"Priority: P1", or bolded "**P1**") instead of any bare "P1", and prefer
scanning only the first comment (use _first_comment) or the first line of a
comment for the fallback check to reduce noise; update the regex constant
_PRIORITY_RE (and related logic in _extract_priority) to match those contextual
patterns and adjust any help text to document the heuristic limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72442a7e-0dcc-418d-a161-3f68a34571a1
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockand included by**/*skills/gh-address-comments/scripts/__pycache__/_bd_utils.cpython-314.pycis excluded by!**/*.pycand included by**/*
📒 Files selected for processing (59)
.claude-plugin/plugin.jsonCargo.tomlFUTURE.mdapps/gateway-admin/components/upstream-oauth/upstream-oauth-card.tsxapps/gateway-admin/lib/api/upstream-oauth-client.tscrates/lab-apis/Cargo.tomlcrates/lab-apis/src/core/http.rscrates/lab-apis/src/lib.rscrates/lab-apis/src/mcpregistry.rscrates/lab-apis/src/mcpregistry/client.rscrates/lab-apis/src/mcpregistry/error.rscrates/lab-apis/src/mcpregistry/types.rscrates/lab-auth/src/state.rscrates/lab/Cargo.tomlcrates/lab/src/api/router.rscrates/lab/src/api/services.rscrates/lab/src/api/services/gateway.rscrates/lab/src/api/services/mcpregistry.rscrates/lab/src/api/upstream_oauth.rscrates/lab/src/cli.rscrates/lab/src/cli/mcpregistry.rscrates/lab/src/dispatch.rscrates/lab/src/dispatch/clients.rscrates/lab/src/dispatch/deploy/build.rscrates/lab/src/dispatch/error.rscrates/lab/src/dispatch/gateway/catalog.rscrates/lab/src/dispatch/gateway/config.rscrates/lab/src/dispatch/gateway/dispatch.rscrates/lab/src/dispatch/gateway/manager.rscrates/lab/src/dispatch/mcpregistry.rscrates/lab/src/dispatch/mcpregistry/catalog.rscrates/lab/src/dispatch/mcpregistry/client.rscrates/lab/src/dispatch/mcpregistry/dispatch.rscrates/lab/src/dispatch/mcpregistry/params.rscrates/lab/src/dispatch/upstream/pool.rscrates/lab/src/mcp/catalog.rscrates/lab/src/mcp/server.rscrates/lab/src/mcp/services.rscrates/lab/src/mcp/services/mcpregistry.rscrates/lab/src/oauth/upstream/cache.rscrates/lab/src/oauth/upstream/manager.rscrates/lab/src/registry.rscrates/lab/src/scaffold/patcher/source.rsdeny.tomldocs/UPSTREAM.mdskills/gh-address-comments/SKILL.mdskills/gh-address-comments/scripts/_bd_utils.pyskills/gh-address-comments/scripts/ai_triage.pyskills/gh-address-comments/scripts/close_beads.pyskills/gh-address-comments/scripts/create_beads.pyskills/gh-address-comments/scripts/fetch_comments.pyskills/gh-address-comments/scripts/install_completions.pyskills/gh-address-comments/scripts/mark_resolved.pyskills/gh-address-comments/scripts/post_reply.pyskills/gh-address-comments/scripts/pr_changelog.pyskills/gh-address-comments/scripts/pr_checklist.pyskills/gh-address-comments/scripts/pr_status.pyskills/gh-address-comments/scripts/pr_summary.pyskills/gh-address-comments/scripts/thread_context.py
💤 Files with no reviewable changes (2)
- crates/lab/src/dispatch/gateway/catalog.rs
- FUTURE.md
Comment on lines
+47
to
+51
| let inner = reqwest::Client::builder() | ||
| .user_agent(concat!("lab-apis/", env!("CARGO_PKG_VERSION"))) | ||
| .connect_timeout(Duration::from_secs(5)) | ||
| .timeout(Duration::from_secs(20)) | ||
| .redirect(redirect::Policy::none()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify custom reqwest timeouts in the new mcpregistry client.
rg -n -C2 'connect_timeout|timeout\\(Duration::from_secs' crates/lab-apis/srcRepository: jmagar/lab
Length of output: 175
🏁 Script executed:
# Read the mcpregistry client file to see the actual timeout
cat -n crates/lab-apis/src/mcpregistry/client.rs | head -60Repository: jmagar/lab
Length of output: 2783
🏁 Script executed:
# Search for timeout configurations in other service clients
rg -n "timeout.*Duration::from_secs" crates/lab-apis/src --type rsRepository: jmagar/lab
Length of output: 80
🏁 Script executed:
# Search for timeout in other service clients
rg -n "timeout.*Duration::from_secs" crates/lab-apis/src/Repository: jmagar/lab
Length of output: 1035
🏁 Script executed:
# Check if there are any other clients with custom timeouts or if HttpClient handles it
rg -n "HttpClient::new\|timeout" crates/lab-apis/src/anthropic/client.rs crates/lab-apis/src/bedrock/client.rs crates/lab-apis/src/xai/client.rs 2>/dev/null | head -50Repository: jmagar/lab
Length of output: 36
Use the standard 30s request timeout.
This client sets a 20s timeout instead of the required 30s per the coding guidelines. While the code comment explains this choice (to timeout before the server's 30s timeout layer), the guideline explicitly states: "Hardcoded timeouts: connect_timeout 5s, request_timeout 30s — not per-client configurable." Other service clients (e.g., unifi) and the core HttpClient correctly use 30s. If the registry service has a 30s server timeout, the client should use the same standard and let the server timeout handling take precedence.
Suggested fix
.user_agent(concat!("lab-apis/", env!("CARGO_PKG_VERSION")))
.connect_timeout(Duration::from_secs(5))
- .timeout(Duration::from_secs(20))
+ .timeout(Duration::from_secs(30))
.redirect(redirect::Policy::none())Also update the doc comment on line 33 to reflect the 30s timeout.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let inner = reqwest::Client::builder() | |
| .user_agent(concat!("lab-apis/", env!("CARGO_PKG_VERSION"))) | |
| .connect_timeout(Duration::from_secs(5)) | |
| .timeout(Duration::from_secs(20)) | |
| .redirect(redirect::Policy::none()) | |
| let inner = reqwest::Client::builder() | |
| .user_agent(concat!("lab-apis/", env!("CARGO_PKG_VERSION"))) | |
| .connect_timeout(Duration::from_secs(5)) | |
| .timeout(Duration::from_secs(30)) | |
| .redirect(redirect::Policy::none()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab-apis/src/mcpregistry/client.rs` around lines 47 - 51, The request
timeout was set to 20s in the reqwest client builder; change the timeout to 30s
by updating the .timeout(Duration::from_secs(20)) call to
.timeout(Duration::from_secs(30)) in the client construction (the chain that
builds `inner` via reqwest::Client::builder()), keep connect_timeout at 5s, and
update the doc comment above the client creation that describes the timeout to
state 30s instead of 20s.
Comment on lines
+96
to
+104
| pub async fn get_server( | ||
| &self, | ||
| name: &str, | ||
| version: &str, | ||
| ) -> Result<ServerResponse, RegistryError> { | ||
| Self::validate_name(name)?; | ||
| let encoded = Self::encode_name(name); | ||
| let path = format!("/v0.1/servers/{encoded}/versions/{version}"); | ||
| Ok(self.http.get_json(&path).await?) |
There was a problem hiding this comment.
Encode and validate version as a path segment.
name is percent-encoded, but version is interpolated raw. A caller-provided version containing /, ?, or # can change the request path/query instead of remaining inside the {version} segment.
Suggested fix
pub async fn get_server(
&self,
name: &str,
version: &str,
) -> Result<ServerResponse, RegistryError> {
Self::validate_name(name)?;
+ if version.trim().is_empty() {
+ return Err(RegistryError::InvalidInput {
+ message: "server version must not be empty".to_string(),
+ });
+ }
let encoded = Self::encode_name(name);
- let path = format!("/v0.1/servers/{encoded}/versions/{version}");
+ let encoded_version = utf8_percent_encode(version, &PATH_SEGMENT).to_string();
+ let path = format!("/v0.1/servers/{encoded}/versions/{encoded_version}");
Ok(self.http.get_json(&path).await?)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab-apis/src/mcpregistry/client.rs` around lines 96 - 104, The
get_server function currently encodes only name and interpolates version raw
into path; update get_server to validate and percent-encode the version as a
path segment before building the path. Call a validation helper (e.g.,
Self::validate_version(version)? — implement it if missing) and an encoding
helper (e.g., let encoded_version = Self::encode_name(version) or add a
dedicated encode_segment) and then use
format!("/v0.1/servers/{encoded}/versions/{encoded_version}") so untrusted
characters like '/', '?', and '#' cannot break the request path.
Comment on lines
+1
to
+7
| //! Request/response types for the MCP Registry v0.1 API. | ||
| //! | ||
| //! Serde rules: | ||
| //! - No `deny_unknown_fields` — the registry adds fields freely. | ||
| //! - Nullable arrays use `#[serde(default)]` to treat JSON null as empty. | ||
| //! - Reserved-word fields use descriptive Rust names with `#[serde(rename = "type")]`. | ||
| //! - Dotted/slashed JSON keys use `#[serde(rename = "...")]` directly. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust serde does #[serde(default)] deserialize JSON null into an empty Vec?
💡 Result:
No, Rust's Serde with #[serde(default)] does not deserialize a JSON null value into an empty Vec for a Vec field. Instead, it results in a deserialization error. The #[serde(default)] attribute only provides a default value (an empty Vec for Vec) when the field is absent from the JSON object. When the field is explicitly present with a null value (e.g., {"field": null}), Serde attempts to deserialize null directly into Vec, which fails because null is not a valid JSON array. Vec does not naturally deserialize from null; that behavior was considered a bug in older versions and has been fixed. To handle JSON null as an empty Vec, use a custom deserializer (e.g., via #[serde(deserialize_with)]) that treats null as the default, or the serde_with crate's DefaultOnNull. For example: fn deserialize_null_default<'de, D, T>(deserializer: D) -> Result<Vec, D::Error> where D: Deserializer<'de>, T: Deserialize<'de>, { let opt = Option::deserialize(deserializer)?; Ok(opt.unwrap_or_default) } #[derive(Deserialize)] struct Example { #[serde(deserialize_with = "deserialize_null_default")] field: Vec, }
Citations:
- 1: https://serde.rs/field-attrs.html
- 2: https://stackoverflow.com/questions/44205435/how-to-deserialize-a-json-file-which-contains-null-values-using-serde
- 3: null deserializes as an empty Vec serde-rs/json#283
- 4: https://serde.rs/attr-default.html
Fix nullable-array deserialization in MCP Registry types.
The serde rules documented at lines 1–7 claim #[serde(default)] treats JSON null as empty, but this is incorrect. #[serde(default)] only provides a default for missing fields; explicit null values cause deserialization to fail. If the registry returns servers: null or issues: null, these models will panic on deserialize.
Apply the custom deserializer to all Vec fields that must accept null:
🛠️ Proposed fix
+fn null_as_default<'de, D, T>(deserializer: D) -> Result<T, D::Error>
+where
+ D: serde::Deserializer<'de>,
+ T: Default + Deserialize<'de>,
+{
+ Ok(Option::<T>::deserialize(deserializer)?.unwrap_or_default())
+}
+
/// Paginated list of MCP servers.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct ServerListResponse {
/// Page of server entries. JSON null is treated as an empty list.
- #[serde(default)]
+ #[serde(default, deserialize_with = "null_as_default")]
pub servers: Vec<ServerResponse>,
@@
pub struct ValidationResult {
/// Whether the server JSON is valid.
pub valid: bool,
/// Validation issues found. JSON null is treated as empty.
- #[serde(default)]
+ #[serde(default, deserialize_with = "null_as_default")]
pub issues: Vec<ValidationIssue>,
}Also applies to: 60–62, 253–255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab-apis/src/mcpregistry/types.rs` around lines 1 - 7, The Vec fields
(e.g., fields named servers, issues and other Vec<T> members in the MCP Registry
types) currently only use #[serde(default)] which does not accept explicit JSON
null and will fail deserialization; add a custom deserializer that treats null
as an empty Vec and apply it to each nullable Vec field by annotating them with
#[serde(default, deserialize_with = "null_to_vec::<T>")] (or the appropriate
generic helper), and add the helper function (e.g., fn null_to_vec<'de, D, T>(d:
D) -> Result<Vec<T>, D::Error> where D: Deserializer<'de>, T: Deserialize<'de'>
{ Option::deserialize(d).map(|opt: Option<Vec<T>>| opt.unwrap_or_default()) })
so any servers, issues, or other Vec fields accept JSON null as empty.
Comment on lines
+32
to
+42
| fn new(requests_per_minute: u32) -> Self { | ||
| let rate = requests_per_minute as f64 / 60.0; | ||
| // Burst = requests_per_minute: allows consuming the full per-minute quota | ||
| // immediately, then refills at `rate` tokens/second. | ||
| let max_tokens = requests_per_minute.max(1) as f64; | ||
| Self { | ||
| inner: Arc::new(Mutex::new(RateLimiterInner { | ||
| tokens: max_tokens, | ||
| max_tokens, | ||
| refill_rate: rate, | ||
| last_refill: Instant::now(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect AuthConfig definitions/defaults/env parsing for zero-value validation
# on authorize_requests_per_minute and register_requests_per_minute.
rg -n -C4 \
'authorize_requests_per_minute|register_requests_per_minute|max_pending_oauth_states' \
--type=rustRepository: jmagar/lab
Length of output: 8086
🏁 Script executed:
# Find the validate method in AuthConfig
rg -n 'fn validate' crates/lab-auth/src/config.rs -A 15Repository: jmagar/lab
Length of output: 774
🏁 Script executed:
# Find default constants
rg -n 'DEFAULT_REGISTER_REQUESTS_PER_MINUTE|DEFAULT_AUTHORIZE_REQUESTS_PER_MINUTE|DEFAULT_MAX_PENDING_OAUTH_STATES' crates/lab-auth/src/config.rs -B 2Repository: jmagar/lab
Length of output: 1264
🏁 Script executed:
# Get the complete RateLimiter::new implementation
rg -n 'fn new\(requests_per_minute' crates/lab-auth/src/state.rs -A 15Repository: jmagar/lab
Length of output: 744
🏁 Script executed:
rg -n 'fn validate' crates/lab-auth/src/config.rs -A 30 | head -50Repository: jmagar/lab
Length of output: 1319
Add validation to reject zero rate-limit configuration.
AuthConfig::validate() does not check that authorize_requests_per_minute and register_requests_per_minute are greater than zero. With a value of 0, RateLimiter::new(0) initializes max_tokens to 1 (line 36: requests_per_minute.max(1)), resulting in exactly one permitted request followed by permanent rate-limiting due to zero refill rate. Either enforce > 0 validation in AuthConfig::validate() or in RateLimiter::new() with an explicit error.
Proposed fix
impl RateLimiter {
- fn new(requests_per_minute: u32) -> Self {
+ fn new(requests_per_minute: u32) -> Result<Self, AuthError> {
+ if requests_per_minute == 0 {
+ return Err(AuthError::Config(
+ "rate-limit requests per minute must be greater than zero".to_string(),
+ ));
+ }
let rate = requests_per_minute as f64 / 60.0;
// Burst = requests_per_minute: allows consuming the full per-minute quota
// immediately, then refills at `rate` tokens/second.
- let max_tokens = requests_per_minute.max(1) as f64;
- Self {
+ let max_tokens = requests_per_minute as f64;
+ Ok(Self {
inner: Arc::new(Mutex::new(RateLimiterInner {
tokens: max_tokens,
max_tokens,
refill_rate: rate,
last_refill: Instant::now(),
})),
- }
+ })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab-auth/src/state.rs` around lines 32 - 42, AuthConfig::validate()
currently doesn't reject zero values for authorize_requests_per_minute and
register_requests_per_minute which allows RateLimiter::new(0) to create a
non-refilling limiter; add validation to AuthConfig::validate() to ensure
authorize_requests_per_minute > 0 and register_requests_per_minute > 0 and
return an appropriate error if not (reference the fields
authorize_requests_per_minute and register_requests_per_minute), or
alternatively change RateLimiter::new(requests_per_minute: u32) to return a
Result and explicitly error when requests_per_minute == 0 so callers cannot
construct a zero-refill limiter.
Comment on lines
+47
to
+60
| /// Try to consume one token. Returns `true` if allowed, `false` if rate-limited. | ||
| fn try_acquire(&self) -> bool { | ||
| let mut inner = self.inner.lock().expect("rate limiter lock"); | ||
| let now = Instant::now(); | ||
| let elapsed = now.duration_since(inner.last_refill).as_secs_f64(); | ||
| inner.tokens = (inner.tokens + elapsed * inner.refill_rate).min(inner.max_tokens); | ||
| inner.last_refill = now; | ||
| if inner.tokens >= 1.0 { | ||
| inner.tokens -= 1.0; | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Return the actual retry delay from the token bucket.
Both guards currently report retry_after_ms: 60_000 regardless of the configured rate. For example, 20/minute refills in about 3 seconds after burst exhaustion, so clients may unnecessarily back off for a full minute.
⏱️ Proposed fix to derive retry-after from refill state
- /// Try to consume one token. Returns `true` if allowed, `false` if rate-limited.
- fn try_acquire(&self) -> bool {
+ /// Try to consume one token. Returns the wait time until the next token when rate-limited.
+ fn try_acquire(&self) -> Result<(), Duration> {
let mut inner = self.inner.lock().expect("rate limiter lock");
let now = Instant::now();
let elapsed = now.duration_since(inner.last_refill).as_secs_f64();
inner.tokens = (inner.tokens + elapsed * inner.refill_rate).min(inner.max_tokens);
inner.last_refill = now;
if inner.tokens >= 1.0 {
inner.tokens -= 1.0;
- true
+ Ok(())
} else {
- false
+ let missing_tokens = 1.0 - inner.tokens;
+ let retry_after_secs = missing_tokens / inner.refill_rate;
+ Err(Duration::from_secs_f64(retry_after_secs))
}
}
} pub fn check_authorize_rate_limit(&self) -> Result<(), AuthError> {
- if self.authorize_limiter.try_acquire() {
- Ok(())
- } else {
- Err(AuthError::RateLimited {
+ self.authorize_limiter.try_acquire().map_err(|retry_after| {
+ AuthError::RateLimited {
message: "authorize rate limit exceeded".to_string(),
- retry_after_ms: 60_000,
- })
- }
+ retry_after_ms: retry_after.as_millis() as u64,
+ }
+ })
}
/// Rate-limit guard for `/register` endpoint.
pub fn check_register_rate_limit(&self) -> Result<(), AuthError> {
- if self.register_limiter.try_acquire() {
- Ok(())
- } else {
- Err(AuthError::RateLimited {
+ self.register_limiter.try_acquire().map_err(|retry_after| {
+ AuthError::RateLimited {
message: "register rate limit exceeded".to_string(),
- retry_after_ms: 60_000,
- })
- }
+ retry_after_ms: retry_after.as_millis() as u64,
+ }
+ })
}Also applies to: 116-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab-auth/src/state.rs` around lines 47 - 60, The token-bucket logic in
try_acquire (and the similar guard at lines 116-137) must return the actual
retry delay instead of a fixed 60_000ms; compute the time until tokens reach 1.0
using inner.tokens, inner.refill_rate and last_refill (if tokens >= 1.0 -> zero
delay, else delay_secs = (1.0 - inner.tokens) / inner.refill_rate), convert to
milliseconds and return or surface that value to the guards so they set
retry_after_ms to the computed ms; update the try_acquire signature/return to
include the retry duration (or provide a helper like
compute_retry_after_ms(inner: &TokenBucketInner) -> u64) and use that helper
from the other guard code paths to replace the constant 60_000.
Comment on lines
+68
to
+79
| def check_approvals(reviews: list[dict[str, Any]], required: int) -> tuple[bool, str, str]: | ||
| approved = [r for r in reviews if r.get("state") == "APPROVED"] | ||
| changes = [r for r in reviews if r.get("state") == "CHANGES_REQUESTED"] | ||
| if changes: | ||
| names = ", ".join(r["author"]["login"] for r in changes if r.get("author")) | ||
| return False, f"Changes requested by: {names}", "Address feedback and re-request review" | ||
| if len(approved) < required: | ||
| have = len(approved) | ||
| fix = "Request review: gh pr edit --add-reviewer <username>" | ||
| return False, f"{have}/{required} required approvals", fix | ||
| names = ", ".join(r["author"]["login"] for r in approved if r.get("author")) | ||
| return True, f"{len(approved)} approval(s): {names}", "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect whether `gh pr view --json reviews` returns historical reviews.
# Usage: PR_NUMBER=24 REPO=owner/repo ./script.sh
set -euo pipefail
: "${PR_NUMBER:?set PR_NUMBER}"
: "${REPO:?set REPO}"
gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews \
--jq '.reviews[] | {author: .author.login, state, submittedAt}'Repository: jmagar/lab
Length of output: 97
🏁 Script executed:
cat -n skills/gh-address-comments/scripts/pr_checklist.py | head -100Repository: jmagar/lab
Length of output: 4734
🏁 Script executed:
# Search for any tests or usage of check_approvals
rg "check_approvals" --type py -B 2 -A 2Repository: jmagar/lab
Length of output: 984
🏁 Script executed:
# Look for any related GitHub API documentation or comments in the codebase
rg "gh pr view|reviews" --type py -B 2 -A 2Repository: jmagar/lab
Length of output: 12439
🏁 Script executed:
# Search for test files or example data
fd "test.*pr_checklist|pr_checklist.*test" --type fRepository: jmagar/lab
Length of output: 36
🏁 Script executed:
# Search for any test data or fixtures with review examples
find . -name "*.json" -o -name "*.py" | xargs grep -l "CHANGES_REQUESTED\|APPROVED" 2>/dev/null | head -10Repository: jmagar/lab
Length of output: 156
🏁 Script executed:
# Look for pr_status.py to see if it has the same issue
cat -n skills/gh-address-comments/scripts/pr_status.py | sed -n '50,100p'Repository: jmagar/lab
Length of output: 2609
🏁 Script executed:
# Check if there's any documentation about the reviews structure
find . -name "README*" -o -name "*.md" | xargs grep -l "review\|approval" 2>/dev/null | head -5Repository: jmagar/lab
Length of output: 197
🏁 Script executed:
# Look for documentation or comments explaining the review data structure
rg "gh pr view|reviews|state.*APPROVED|state.*CHANGES_REQUESTED" --type py -B 1 -A 1 | head -40Repository: jmagar/lab
Length of output: 3041
🏁 Script executed:
# Check if there are any integration tests or examples
find . -path "*/test*" -o -path "*/example*" | grep -i "pr\|review\|approval" 2>/dev/nullRepository: jmagar/lab
Length of output: 138
🏁 Script executed:
# Look for any comments in fetch_comments.py about review handling
cat -n skills/gh-address-comments/scripts/fetch_comments.py | grep -A 5 -B 5 "submittedAt"Repository: jmagar/lab
Length of output: 785
🏁 Script executed:
# Examine the full GraphQL query to understand review data structure
cat -n skills/gh-address-comments/scripts/fetch_comments.py | sed -n '40,90p'Repository: jmagar/lab
Length of output: 1645
🏁 Script executed:
# Check how pr_checklist.py retrieves reviews (REST API vs GraphQL)
cat -n skills/gh-address-comments/scripts/pr_checklist.py | sed -n '120,160p'Repository: jmagar/lab
Length of output: 1914
🏁 Script executed:
# Search for any GitHub CLI documentation or comments about gh pr view output
rg "gh pr view" --type py -B 3 -A 3 | head -30Repository: jmagar/lab
Length of output: 36
🏁 Script executed:
# Check if there's documentation about the review fields or any comments explaining the structure
cat -n skills/gh-address-comments/scripts/pr_checklist.py | sed -n '140,150p'Repository: jmagar/lab
Length of output: 486
🏁 Script executed:
# Search for any mention of author deduplication or review handling logic elsewhere
rg "latest.*review|latest.*author|dedup|unique.*review" --type pyRepository: jmagar/lab
Length of output: 36
🌐 Web query:
GitHub CLI gh pr view reviews field behavior return all reviews or latest per author
💡 Result:
The GitHub CLI gh pr view command with the reviews JSON field returns all reviews for the pull request, not just the latest per author. The official documentation lists both reviews and latestReviews as available JSON fields, indicating reviews provides the full list while latestReviews would provide only the most recent review per author. This matches GitHub's GraphQL API behavior where reviews fetches all PullRequestReview nodes chronologically, as confirmed by example outputs showing multiple reviews from the same author.
Citations:
- 1: https://cli.github.com/manual/gh_pr_view
- 2:
gh pr view --json commentsdoes not include review comments cli/cli#11477 - 3: https://manpages.opensuse.org/Tumbleweed/gh/gh-pr-view.1.en.html
Use each reviewer's latest review state for approval gating.
The function counts all review records instead of only the latest per author. This causes two issues: (1) duplicate approvals from the same reviewer inflate the approval count, and (2) a later approval cannot override an earlier CHANGES_REQUESTED review, causing the gate to fail even after feedback is addressed.
The proposed fix deduplicates per author, but ensure it selects the most recent review by comparing submittedAt timestamps, not just the last entry encountered:
Suggested fix
def check_approvals(reviews: list[dict[str, Any]], required: int) -> tuple[bool, str, str]:
- approved = [r for r in reviews if r.get("state") == "APPROVED"]
- changes = [r for r in reviews if r.get("state") == "CHANGES_REQUESTED"]
+ latest_by_author: dict[str, dict[str, Any]] = {}
+ for review in reviews:
+ author = (review.get("author") or {}).get("login")
+ if author:
+ existing = latest_by_author.get(author)
+ if not existing or review.get("submittedAt", "") > existing.get("submittedAt", ""):
+ latest_by_author[author] = review
+
+ latest_reviews = list(latest_by_author.values())
+ approved = [r for r in latest_reviews if r.get("state") == "APPROVED"]
+ changes = [r for r in latest_reviews if r.get("state") == "CHANGES_REQUESTED"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/gh-address-comments/scripts/pr_checklist.py` around lines 68 - 79, The
check_approvals function currently counts every review record instead of using
each reviewer's latest state; update check_approvals to first collapse reviews
by reviewer login by selecting the review with the greatest submittedAt
timestamp (use r["author"]["login"] and r.get("submittedAt") to compare), then
compute approved and changes lists from that deduplicated set (checking
r.get("state") == "APPROVED" or "CHANGES_REQUESTED"); preserve existing return
messages and fix suggestion logic but base counts and names on the per-author
latest review.
Comment on lines
+58
to
+67
| def check_reviews(reviews: list[dict[str, Any]]) -> tuple[bool, str]: | ||
| approvals = [r for r in reviews if r.get("state") == "APPROVED"] | ||
| changes = [r for r in reviews if r.get("state") == "CHANGES_REQUESTED"] | ||
| if changes: | ||
| names = ", ".join(r["author"]["login"] for r in changes if r.get("author")) | ||
| return False, f"{len(changes)} changes requested by {names}" | ||
| if not approvals: | ||
| return False, "No approvals yet" | ||
| names = ", ".join(r["author"]["login"] for r in approvals if r.get("author")) | ||
| return True, f"{len(approvals)} approved by {names}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect whether `gh pr view --json reviews` returns multiple reviews per author.
# Usage: PR_NUMBER=24 REPO=owner/repo ./script.sh
set -euo pipefail
: "${PR_NUMBER:?set PR_NUMBER}"
: "${REPO:?set REPO}"
gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews \
--jq '.reviews[] | {author: .author.login, state, submittedAt}'Repository: jmagar/lab
Length of output: 97
🏁 Script executed:
# First, let's read the relevant file to understand the context
cat -n skills/gh-address-comments/scripts/pr_status.py | head -80Repository: jmagar/lab
Length of output: 3429
🏁 Script executed:
# Continue reading the file to see where check_reviews is called
cat -n skills/gh-address-comments/scripts/pr_status.py | tail -n +70Repository: jmagar/lab
Length of output: 4428
🏁 Script executed:
# Search for test files related to pr_status
find . -type f \( -name "*test*" -o -name "*spec*" \) | grep -i pr_statusRepository: jmagar/lab
Length of output: 36
🏁 Script executed:
# Also check for any fixture data or example PR responses
fd -e json | xargs grep -l "reviews" 2>/dev/null | head -5Repository: jmagar/lab
Length of output: 36
🌐 Web query:
GitHub API PR reviews multiple per author same reviewer submittedAt
💡 Result:
Yes, the GitHub API supports retrieving multiple pull request reviews from the same author (reviewer). The endpoint GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews lists all submitted reviews for a pull request in chronological order, each with a unique id, associated user object (author), and submitted_at timestamp (for submitted reviews; absent for pending). The schema allows multiple entries with the same user, and official docs state "Lists all reviews", with no restrictions mentioned on duplicate authors. Pending reviews (created without event) lack submitted_at until submitted via Update review or Submit review endpoints.
Citations:
- 1: https://docs.github.com/rest/pulls/reviews
- 2: https://docs.github.com/en/rest/pulls/reviews?apiVersion=2026-03-10
- 3: https://docs.github.com/en/rest/pulls/reviews
- 4: https://help.github.com/en/articles/requesting-a-pull-request-review
- 5: https://help.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews
Evaluate only the latest review per reviewer, not all historical reviews.
The GitHub API returns all reviews in chronological order, allowing the same reviewer to appear multiple times. The current implementation counts every review, causing over-counting when a reviewer changes their decision (e.g., from CHANGES_REQUESTED to APPROVED) and showing duplicate reviewer names in the status message.
Proposed fix
def check_reviews(reviews: list[dict[str, Any]]) -> tuple[bool, str]:
- approvals = [r for r in reviews if r.get("state") == "APPROVED"]
- changes = [r for r in reviews if r.get("state") == "CHANGES_REQUESTED"]
+ latest_by_author: dict[str, dict[str, Any]] = {}
+ for review in reviews:
+ author = (review.get("author") or {}).get("login")
+ if author:
+ latest_by_author[author] = review
+
+ latest_reviews = list(latest_by_author.values())
+ approvals = [r for r in latest_reviews if r.get("state") == "APPROVED"]
+ changes = [r for r in latest_reviews if r.get("state") == "CHANGES_REQUESTED"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/gh-address-comments/scripts/pr_status.py` around lines 58 - 67, The
check_reviews function is counting every historical review instead of only the
latest per reviewer; update check_reviews to first reduce the reviews list into
a map of reviewer -> most recent state by iterating reviews in chronological
order and overwriting entries for the same r["author"]["login"] (skip reviews
without author), then compute approvals and changes from that map (instead of
the original lists) and build the names/summary from the map entries; keep the
function signature and return values as-is.
Comment on lines
+132
to
+147
| print(f"PR #{number}: {pr['title']}") | ||
| print(f"URL: {pr['url']}") | ||
| if pr.get("isDraft"): | ||
| print("⚠️ DRAFT PR") | ||
| print("=" * 70) | ||
|
|
||
| checks: list[tuple[str, bool, str]] = [] | ||
|
|
||
| ci_ok, ci_msg = check_ci(owner, repo, number) | ||
| checks.append(("CI checks", ci_ok, ci_msg)) | ||
|
|
||
| rev_ok, rev_msg = check_reviews(pr.get("reviews", [])) | ||
| checks.append(("Approvals", rev_ok, rev_msg)) | ||
|
|
||
| th_ok, th_msg = check_threads(args.input, number, owner, repo) | ||
| checks.append(("Threads", th_ok, th_msg)) |
There was a problem hiding this comment.
Make draft PRs fail merge readiness.
Lines 134-135 only print a warning, so a draft PR can still end with READY TO MERGE if the other checks pass.
Proposed fix
checks: list[tuple[str, bool, str]] = []
+
+ draft_ok = not pr.get("isDraft")
+ checks.append(("Draft status", draft_ok, "Not a draft" if draft_ok else "PR is still a draft"))
ci_ok, ci_msg = check_ci(owner, repo, number)
checks.append(("CI checks", ci_ok, ci_msg))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/gh-address-comments/scripts/pr_status.py` around lines 132 - 147, The
current draft PR check only prints a warning so drafts can still be marked READY
TO MERGE; modify the logic in the block that handles pr.get("isDraft") to append
a failing entry to the checks list (e.g. checks.append(("Draft", False, "PR is a
draft"))) so the draft status is treated as a failing check by the existing
aggregator that consumes checks; update the code near the calls to check_ci,
check_reviews, and check_threads (referencing pr.get("isDraft"), checks,
check_ci, check_reviews, check_threads) so drafts always cause overall merge
readiness to fail.
Comment on lines
+180
to
+195
| # Get head SHA | ||
| try: | ||
| if args.pr: | ||
| sha = get_head_sha(owner, repo, args.pr) | ||
| else: | ||
| pr_num = data.get("pull_request", {}).get("number") | ||
| if not pr_num: | ||
| print("Cannot determine PR number — pass --pr NUMBER", file=sys.stderr) | ||
| sys.exit(1) | ||
| sha = get_head_sha(owner, repo, pr_num) | ||
|
|
||
| file_lines = fetch_file_at_ref(owner, repo, path, sha) | ||
| show_context(thread, file_lines, args.lines) | ||
| except Exception as e: | ||
| print(f"Could not fetch file content: {e}", file=sys.stderr) | ||
| print(f"(The file may have been renamed, deleted, or permissions denied)") |
There was a problem hiding this comment.
Exit non-zero when context fetch fails.
Line 193 catches fetch failures, but the script then exits successfully after printing the error. That makes automation think context was produced when the GitHub fetch failed.
Proposed fix
except Exception as e:
print(f"Could not fetch file content: {e}", file=sys.stderr)
- print(f"(The file may have been renamed, deleted, or permissions denied)")
+ print("(The file may have been renamed, deleted, or permissions denied)")
+ sys.exit(1)As per coding guidelines, “Handle errors explicitly and avoid silent failures.”
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 193-193: Do not catch blind exception: Exception
(BLE001)
[error] 195-195: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/gh-address-comments/scripts/thread_context.py` around lines 180 - 195,
The except block catching failures when fetching context (around get_head_sha,
fetch_file_at_ref and show_context) currently prints errors but allows the
script to exit zero; change it to exit non‑zero after printing the messages
(e.g., call sys.exit(1)) so automation sees a failure when fetch_file_at_ref or
get_head_sha fails; ensure the error branch still prints the two messages before
exiting.
Comment on lines
+70
to
88
| ### 2) Show a summary and triage | ||
|
|
||
| Give the user an overview: | ||
| ```bash | ||
| gh-pr-summary --input /tmp/pr.json --open-only | ||
| ``` | ||
|
|
||
| This groups open threads by file (or use `--by reviewer` to see who left what). Ask the user which threads to tackle in this session — don't assume all must be addressed now. | ||
|
|
||
| ### 3) Create tracking checklist | ||
|
|
||
| Parse the fetched comments and create a task using TaskCreate for each thread the user wants to address: | ||
| For large PRs, run AI triage first to prioritise: | ||
| ```bash | ||
| gh-ai-triage --input /tmp/pr.json | ||
| ``` | ||
|
|
||
| - **Subject:** `Address comment #N: [file]:[line]` | ||
| - **Description:** Comment author, body preview, what needs to be done | ||
| - **Metadata:** `{"thread_id": "PRRT_kwDOABC...", "file": "path/to/file.ts", "line": 42}` | ||
| Ask which threads to tackle in this session — don't assume all must be addressed now. | ||
| To inspect what a specific thread is actually commenting on: | ||
| ```bash | ||
| gh-thread-context PRRT_kwDO... --input /tmp/pr.json | ||
| ``` | ||
|
|
||
| ### 4) Apply fixes with commit linking |
There was a problem hiding this comment.
Workflow section numbering skips "3)".
Sections jump from ### 2) Show a summary and triage to ### 4) Apply fixes with commit linking. Either a deliberate "3) Select threads" step was dropped and the downstream numbers weren't renumbered, or the insertion of the triage content was meant to replace section 3 in place. Please renumber 4-8 → 3-7 (or reintroduce the missing section) so the workflow reads sequentially.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 73-73: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 78-78: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 84-84: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/gh-address-comments/SKILL.md` around lines 70 - 88, The workflow
headings skip "3)"; update the section headings so they read sequentially
(either restore a missing "3) Select threads" step or renumber subsequent
headings). Locate the existing heading "### 2) Show a summary and triage" and
the heading "### 4) Apply fixes with commit linking" in SKILL.md and change the
later headings ("### 4)" through "### 8)") to "### 3)" through "### 7)" (or
insert a clear "### 3) Select threads" section as appropriate) so the numbering
flows 1→7 consistently; ensure any internal references to these section numbers
are updated accordingly.
…s files Pre-existing issues in main branch files now caught by CI's newer rustc (-D warnings): - pool.rs: replace rmcp::model::Prompt/ReadResourceResult/Resource qualified paths; add ResourceContents to import - runner.rs: add #[allow(async_fn_in_trait)] to DeployRunner pub trait - server.rs: remove unused UpstreamCapability import - catalog.rs: add #[allow(dead_code)] to upstream_name_for_uri (future use) - upstream.rs: add #[allow(dead_code)] to static_kind/normalize_upstream_result
Rust 1.95.0 (2026-04-14) introduced stricter dead_code and unnecessary_qualifications lints that trigger as errors under -D warnings. Pin to 1.94.1 to maintain a stable CI baseline; upgrade can be revisited when all warnings are addressed.
…sions dtolnay/rust-toolchain@stable ignores rust-toolchain.toml when the action ref is @stable. Pin all 4 jobs directly to @1.94.1 so the workflow uses the same version as local development until the new dead_code lints are addressed.
Add #[allow(dead_code)] to public items not yet wired into the binary:
api/oauth.rs (auth_context), catalog.rs (Transport, actions_for),
dispatch/deploy/dispatch.rs (dispatch), dispatch/gateway/manager.rs
(oauth_client_cache, evict_upstream_clients), dispatch/redact.rs
(all pub fns), oauth/upstream/{cache,encryption,manager,types}.rs.
Add #![allow(dead_code)] to redact.rs to cover all 8 public functions
in one attribute instead of per-item annotations.
- Cargo.toml: allow 70+ pre-existing pedantic/nursery lints that were promoted to errors by RUSTFLAGS=-D warnings in CI (Rust 2024 edition). Tracked for incremental cleanup in follow-up PRs. - transport.rs: gate AgentClient::connect_env (SSH_AUTH_SOCK) behind #[cfg(unix)] — the function doesn't exist on Windows; non-Unix hosts get a clear ExtractError::Ssh explaining the limitation. - deploy_runner.rs: suppress unused_qualifications — std::future::Future is now in the Rust 2024 prelude so the qualification is redundant.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Cargo.toml">
<violation number="1" location="Cargo.toml:112">
P2: Do not disable `unwrap_used`/`expect_used`; this weakens enforcement of the repo’s no-panic rule.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| cargo = { level = "warn", priority = -1 } | ||
| unwrap_used = "warn" | ||
| expect_used = "warn" | ||
| unwrap_used = "allow" |
There was a problem hiding this comment.
P2: Do not disable unwrap_used/expect_used; this weakens enforcement of the repo’s no-panic rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Cargo.toml, line 112:
<comment>Do not disable `unwrap_used`/`expect_used`; this weakens enforcement of the repo’s no-panic rule.</comment>
<file context>
@@ -109,14 +109,14 @@ unused_qualifications = "warn"
cargo = { level = "warn", priority = -1 }
-unwrap_used = "warn"
-expect_used = "warn"
+unwrap_used = "allow"
+expect_used = "allow"
panic = "warn"
</file context>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
.github/workflows/ci.yml (1)
19-60: 🧹 Nitpick | 🔵 TrivialToolchain install steps look consistent across jobs.
All four jobs (
check,fmt,clippy,test) now usedtolnay/rust-toolchain@1.94.1, which matches the newrust-toolchain.toml. One minor consideration: ifrust-toolchain.tomlis ever bumped without updating this workflow (or vice versa), the two can drift silently since the action will prefer the toolchain file. Consider replacing the pinned tag here with@master(or similar) and relying solely onrust-toolchain.tomlas the single source of truth, to avoid dual-maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 19 - 60, Replace the hardcoded dtolnay/rust-toolchain@1.94.1 references in the CI jobs (check, fmt, clippy, test) with a branch/tag that defers to rust-toolchain.toml (e.g., dtolnay/rust-toolchain@master) so the workflow relies on the single source of truth; update each occurrence of "uses: dtolnay/rust-toolchain@1.94.1" in those job step definitions to the chosen non-pinned ref and ensure any per-job "components" inputs remain unchanged.Cargo.toml (1)
108-119:⚠️ Potential issue | 🟡 MinorDowngrading
unwrap_used/expect_usedfromwarntoallowis a meaningful posture change.Combined with
panic = "warn"(notdeny), this removes a guardrail across the entire workspace — includinglab-authand the newmcpregistrydispatch paths added in this PR, both of which handle untrusted input. Consider keeping these atwarnworkspace-wide and selectively#[allow(clippy::unwrap_used)]in tests/examples, rather than the inverse. If the intent is purely to unblock CI under-D warnings, a narrower#![allow(...)]at the crate/module level would preserve the signal elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 108 - 119, Revert the workspace-wide downgrades of clippy lints: set unwrap_used and expect_used back to "warn" under [workspace.lints.clippy] (and keep panic as "warn" or stronger as desired), and instead apply targeted allows where truly needed (e.g., add #![allow(clippy::unwrap_used, clippy::expect_used)] in test/example crates or inside specific modules/functions such as the test modules in lab-auth or mcpregistry dispatch code) so the workspace retains broad warnings while only silencing clippy for narrow, justified scopes.crates/lab/src/mcp/catalog.rs (1)
17-22: 🧹 Nitpick | 🔵 TrivialRefactor
server.rsto useupstream_name_for_urihelper or remove dead code.
server.rscontains three inline duplications of upstream name extraction fromlab://upstream/...URIs (lines 462–464, 487–489, 546–548). Theupstream_name_for_urihelper incatalog.rsis marked dead and has no callers. Either refactor the three call sites to use the helper, or remove both the helper and its#[allow(dead_code)]suppression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/mcp/catalog.rs` around lines 17 - 22, There is an unused helper upstream_name_for_uri in catalog.rs while server.rs contains three duplicated inline extracts of the upstream name; replace those duplicated extraction blocks (the three places that parse "lab://upstream/..." to get the upstream name) with a single call to upstream_name_for_uri(uri) and handle the returned Option the same way the current inline code does (propagate None/error or log/return as appropriate), ensuring you import or reference the function where needed; if you prefer removal instead, delete upstream_name_for_uri and its #[allow(dead_code)] instead of refactoring.crates/lab/src/mcp/upstream.rs (1)
6-110:⚠️ Potential issue | 🟠 MajorRemove duplicate error-shaping helpers instead of suppressing dead code.
The
upstream.rsmodule contains dead-code-suppressed copies ofnormalize_upstream_resultandstatic_kind. The server.rs implementation usescanonical_kindfromcrate::mcp::error, which maps error kinds identically to the duplicatedstatic_kind. Silencing the warnings masks that these helpers duplicate logic already present inserver.rsand violate the guideline: "Do not duplicate dispatch timing, logging, or error-shaping helpers per service when they can be shared."Delete
upstream.rs::normalize_upstream_resultandstatic_kind, or consolidate the error-handling logic into a shared module if it serves multiple call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/mcp/upstream.rs` around lines 6 - 110, The two dead-code helpers static_kind and normalize_upstream_result duplicate error-shaping logic already provided by the canonical error helpers; remove these functions from upstream.rs and wire callers to the shared implementation instead. Replace references to static_kind with the existing canonical_kind (crate::mcp::error::canonical_kind) and either call the server-side normalize function or move the normalize_upstream_result logic into a single shared function in crate::mcp::error and import it; remove the #[allow(dead_code)] copies and any unused imports, and adjust call sites to use the shared helper so there is no duplication.crates/lab/src/catalog.rs (1)
117-129: 🧹 Nitpick | 🔵 TrivialConsider feature gating instead of suppressing dead-code warning.
The
actions_forfunction is currently only used in tests but marked with#[allow(dead_code)]. Since the PR introduces the mcpregistry feature flag and these are future-use APIs, consider using#[cfg(any(test, feature = "mcpregistry"))]to make the intent explicit and automatically enable when needed.♻️ Feature gating example
-#[allow(dead_code)] +#[cfg(any(test, feature = "mcpregistry"))] #[must_use] pub fn actions_for(catalog: &Catalog, service: &str, transport: Transport) -> Vec<ActionEntry> {Note: The filtering logic itself is sound—correctly hiding HTTP-only services from stdio transports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/catalog.rs` around lines 117 - 129, Replace the blanket dead-code suppression on the actions_for function with a feature/test gate so it is compiled only for tests or when the mcpregistry feature is enabled: change the attributes on the actions_for function (the function named actions_for in catalog.rs that takes &Catalog, service: &str and transport: Transport and returns Vec<ActionEntry>) from #[allow(dead_code)] to #[cfg(any(test, feature = "mcpregistry"))] (keep #[must_use] if desired) so the function is only included when testing or when the mcpregistry feature is enabled.crates/lab/src/oauth/upstream/cache.rs (1)
145-162:⚠️ Potential issue | 🟠 MajorAvoid unbounded retention of evicted subject lock keys.
Keeping every
build_locksentry forever fixes the duplicate-builder race, butevict_subject/evict_upstreamnow retain(upstream, subject)keys after credentials are cleared or an upstream is removed. In a long-lived server this grows without bound and keeps subject identifiers in memory past eviction.Consider a bounded cleanup strategy that only removes idle locks, or store lock entries with TTL/LRU metadata so active builders stay serialized without retaining tombstones indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/oauth/upstream/cache.rs` around lines 145 - 162, The build_locks map is currently never pruned, causing unbounded retention of (upstream, subject) keys; change evict_subject and evict_upstream to only remove matching build_locks entries when they are idle (to preserve serialization for active builders) or add TTL/LRU metadata and a periodic cleanup/purge pass. Concretely: update the code that manages build_locks (the map referenced as build_locks) so that evict_subject(evict_upstream) iterates matching keys and removes them only if the lock is not held and has no waiters (or its last_used timestamp is older than a configured threshold), or augment lock entries with last_used and run a bounded cleanup routine that evicts stale entries; reference the methods evict_subject, evict_upstream and the build_locks entry type when making the change.crates/lab/src/oauth/upstream/manager.rs (1)
466-498:⚠️ Potential issue | 🟠 MajorSerialize dynamic registration with per-(upstream, subject) lock.
The dynamic registration flow lacks synchronization. Two concurrent callers can both miss the persisted registration check and both invoke
register_client, creating duplicate registrations on the authorization server. Although the DB write usesON CONFLICT DO UPDATE(last-writer-wins), this only selects a winner after both external registrations have already occurred, orphaning one registration.Acquire a lock keyed on
(upstream, subject)before the final check-register-save sequence, re-check the DB while holding the lock, and only callregister_clientif still absent.The lock infrastructure already exists (
self.locksis anArc<RefreshLocks>with anacquiremethod), following the same pattern used inbuild_auth_client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/oauth/upstream/manager.rs` around lines 466 - 498, Wrap the final check-register-save sequence with a per-(upstream, subject) lock using self.locks.acquire(...) (same pattern as build_auth_client): acquire a lock keyed by (self.upstream.name.clone(), subject.clone()), then inside the lock re-run the DB read via find_dynamic_client_registration to see if a canonical client_id now exists; if it does, use that to construct canonical_cfg and skip calling register_client, otherwise call register_client, save_dynamic_client_registration, read back the persisted value, and build the canonical OAuthClientConfig; ensure the lock guard is dropped after that sequence so the lock is released.crates/lab-apis/src/extract/transport.rs (1)
149-229:⚠️ Potential issue | 🟠 MajorNon-Unix targets would emit
unused_variablesandunreachable_codewarnings; refactor to fail fast.On non-Unix (Windows, etc.), the current structure creates compiler warnings:
let user = cfg.user();(line 157) is assigned but never used — it's only consumed inside the#[cfg(unix)]block.- The
#[cfg(not(unix))] return Err(…);at line ~220 leaves lines 225–268 (SFTP channel open, subsystem request,SftpSession::new,Ok(Self {…})) as unreachable code.- The function also wastes I/O parsing ssh-config, opening TCP/ProxyCommand stream, and performing SSH handshake before failing on unsupported platforms.
Note: CI currently only runs
cargo checkandcargo clippyonubuntu-latest, so this isn't caught in the current pipeline. However,RUSTFLAGS: -D warningsis set globally, and compiling with--target x86_64-pc-windows-msvcwould fail locally or in any platform-specific test. The cleaner approach is to fail fast before any I/O and scope Unix-specific code:Suggested restructure
pub async fn connect(host: impl Into<String>) -> Result<Self, ExtractError> { let host = host.into(); + + #[cfg(not(unix))] + { + return Err(ExtractError::Ssh { + host, + message: "SSH agent authentication is not supported on this platform".to_owned(), + }); + } + + #[cfg(unix)] + { + use russh::keys::agent::AgentIdentity; + use russh::keys::agent::client::AgentClient; + let cfg = russh_config::parse_home(&host)... + let user = cfg.user(); + // ...rest of Unix-only code... + Ok(Self { host, sftp, _session: handle }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab-apis/src/extract/transport.rs` around lines 149 - 229, The connect function performs platform-specific SSH work but currently does I/O and assigns variables (host, cfg, user, stream, handle) before an #[cfg(not(unix))] early return, causing unused_variable and unreachable_code warnings on non-Unix targets; refactor by returning early on non-Unix (e.g., place an #[cfg(not(unix))] block at the top of connect that immediately returns the ExtractError), and wrap the Unix-only blocks (parsing russh_config::parse_home, cfg.stream(), AgentClient usage, authenticate_publickey_with, SftpSession creation, etc.) with #[cfg(unix)] so that on non-Unix builds nothing before the return is compiled and no unused variables (like user) or unreachable code remain.
♻️ Duplicate comments (2)
crates/lab/src/mcp/server.rs (1)
1121-1121: 🧹 Nitpick | 🔵 TrivialMove this import to the top import block.
Keep
CatalogSnapshotwith the otheruse crate::...imports near Line 21 instead of placing it between impl blocks.♻️ Proposed move
use crate::mcp::logging::{DispatchLogOutcome, logging_level_rank}; +use crate::mcp::catalog::CatalogSnapshot; use crate::registry::ToolRegistry; @@ -use crate::mcp::catalog::CatalogSnapshot; - impl LabMcpServer {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/mcp/server.rs` at line 1121, The import for CatalogSnapshot is misplaced between impl blocks; move the line "use crate::mcp::catalog::CatalogSnapshot;" up into the top import block where the other "use crate::..." statements are declared (near the existing crate imports around the top of server.rs) so all crate imports are grouped together and the impl blocks no longer contain stray use statements.crates/lab/src/dispatch/upstream/pool.rs (1)
714-732:⚠️ Potential issue | 🟠 MajorPopulate cached resource URIs and prompt names before exposing these helpers.
cached_upstream_resource_uris()andcached_upstream_prompt_names()readentry.resource_uris/entry.prompt_names, but successful list paths still only update counts. Catalog snapshots will keep seeing empty cached resource/prompt names after discovery.🐛 Proposed fix
@@ Ok(result) => { self.record_success_for(&name, UpstreamCapability::Resources) .await; + let resource_uris: Vec<String> = result + .resources + .iter() + .map(|resource| resource.uri.clone()) + .collect(); { let mut catalog = self.catalog.write().await; if let Some(entry) = catalog.get_mut(&name) { entry.resource_count = result.resources.len(); + entry.resource_uris = resource_uris; } } for mut resource in result.resources { @@ Ok(result) => { self.record_success_for(&name, UpstreamCapability::Prompts) .await; + let prompt_names: Vec<String> = result + .prompts + .iter() + .map(|prompt| prompt.name.to_string()) + .collect(); { let mut catalog = self.catalog.write().await; if let Some(entry) = catalog.get_mut(&name) { entry.prompt_count = result.prompts.len(); + entry.prompt_names = prompt_names; } } upstream_prompts.push((name, result.prompts));Also applies to: 937-1002, 1181-1234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lab/src/dispatch/upstream/pool.rs` around lines 714 - 732, The helpers cached_upstream_resource_uris and cached_upstream_prompt_names are returning empty slices because the discovery/listing code only updates counts and never populates entry.resource_uris or entry.prompt_names; fix the upstream discovery/update paths that currently increment counts to also write the discovered URIs and prompt names into the catalog entry (entry.resource_uris and entry.prompt_names) under the same catalog write lock used elsewhere so catalog snapshots reflect the discovered data; search for the code that updates counts (the discovery/listing handlers) and add assignment of the discovered Vec<String> results to those fields when discovery succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 131-200: The current broad Clippy allow-list in Cargo.toml hides
important lints; create a tracking issue that enumerates every allow entry and
assigns an owning crate/module so follow-up PRs are tracked, and then tighten
the allow-list by removing or scoping back the highest-risk lints: re-enable
cast_possible_truncation, cast_possible_wrap, cast_sign_loss, and cast_lossless
(at least) for crates/lab-auth and any SSRF/IP-range checking modules introduced
in this PR, and consider re-enabling nonminimal_bool, needless_pass_by_ref_mut,
redundant_clone, question_mark, and collapsible_if where logic/ownership risks
exist; update the TODO comment to reference the new tracking issue and scope any
remaining allows to specific crates/modules rather than globally.
In `@crates/lab/src/dispatch/redact.rs`:
- Line 1: Remove the broad module-level dead_code suppression and decide whether
these utilities should stay: either (A) wire them into this PR by calling the
functions (is_sensitive_key, redact_url, redact_stdio_value, redact_stdio_args,
redact_upstream_resource_uri) from the actual logging/SSRF/auth error paths that
need redaction, (B) if they are intended for near-term use, document the planned
integration point in a comment and replace the module-wide #![allow(dead_code)]
with targeted #[allow(dead_code)] on the specific functions you want to
suppress, or (C) if they are not needed now, delete the module and reintroduce
it when the integration exists; in all cases remove the top-level allow and
ensure only needed items are allowed or present.
In `@crates/lab/tests/deploy_runner.rs`:
- Line 10: The file currently uses a crate-level suppression
`#![allow(unused_qualifications)]`; remove this top-level attribute, run the
compiler to see which items trigger the warning, then add item-level attributes
`#[allow(unused_qualifications)]` directly above only those specific test
functions, helper functions, or modules that produce the warnings (e.g., put
`#[allow(unused_qualifications)]` on the individual `#[test]` functions or the
helper `mod`/fn that the compiler reports) so the lint is narrowed in scope
rather than globally disabled.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 19-60: Replace the hardcoded dtolnay/rust-toolchain@1.94.1
references in the CI jobs (check, fmt, clippy, test) with a branch/tag that
defers to rust-toolchain.toml (e.g., dtolnay/rust-toolchain@master) so the
workflow relies on the single source of truth; update each occurrence of "uses:
dtolnay/rust-toolchain@1.94.1" in those job step definitions to the chosen
non-pinned ref and ensure any per-job "components" inputs remain unchanged.
In `@Cargo.toml`:
- Around line 108-119: Revert the workspace-wide downgrades of clippy lints: set
unwrap_used and expect_used back to "warn" under [workspace.lints.clippy] (and
keep panic as "warn" or stronger as desired), and instead apply targeted allows
where truly needed (e.g., add #![allow(clippy::unwrap_used,
clippy::expect_used)] in test/example crates or inside specific
modules/functions such as the test modules in lab-auth or mcpregistry dispatch
code) so the workspace retains broad warnings while only silencing clippy for
narrow, justified scopes.
In `@crates/lab-apis/src/extract/transport.rs`:
- Around line 149-229: The connect function performs platform-specific SSH work
but currently does I/O and assigns variables (host, cfg, user, stream, handle)
before an #[cfg(not(unix))] early return, causing unused_variable and
unreachable_code warnings on non-Unix targets; refactor by returning early on
non-Unix (e.g., place an #[cfg(not(unix))] block at the top of connect that
immediately returns the ExtractError), and wrap the Unix-only blocks (parsing
russh_config::parse_home, cfg.stream(), AgentClient usage,
authenticate_publickey_with, SftpSession creation, etc.) with #[cfg(unix)] so
that on non-Unix builds nothing before the return is compiled and no unused
variables (like user) or unreachable code remain.
In `@crates/lab/src/catalog.rs`:
- Around line 117-129: Replace the blanket dead-code suppression on the
actions_for function with a feature/test gate so it is compiled only for tests
or when the mcpregistry feature is enabled: change the attributes on the
actions_for function (the function named actions_for in catalog.rs that takes
&Catalog, service: &str and transport: Transport and returns Vec<ActionEntry>)
from #[allow(dead_code)] to #[cfg(any(test, feature = "mcpregistry"))] (keep
#[must_use] if desired) so the function is only included when testing or when
the mcpregistry feature is enabled.
In `@crates/lab/src/mcp/catalog.rs`:
- Around line 17-22: There is an unused helper upstream_name_for_uri in
catalog.rs while server.rs contains three duplicated inline extracts of the
upstream name; replace those duplicated extraction blocks (the three places that
parse "lab://upstream/..." to get the upstream name) with a single call to
upstream_name_for_uri(uri) and handle the returned Option the same way the
current inline code does (propagate None/error or log/return as appropriate),
ensuring you import or reference the function where needed; if you prefer
removal instead, delete upstream_name_for_uri and its #[allow(dead_code)]
instead of refactoring.
In `@crates/lab/src/mcp/upstream.rs`:
- Around line 6-110: The two dead-code helpers static_kind and
normalize_upstream_result duplicate error-shaping logic already provided by the
canonical error helpers; remove these functions from upstream.rs and wire
callers to the shared implementation instead. Replace references to static_kind
with the existing canonical_kind (crate::mcp::error::canonical_kind) and either
call the server-side normalize function or move the normalize_upstream_result
logic into a single shared function in crate::mcp::error and import it; remove
the #[allow(dead_code)] copies and any unused imports, and adjust call sites to
use the shared helper so there is no duplication.
In `@crates/lab/src/oauth/upstream/cache.rs`:
- Around line 145-162: The build_locks map is currently never pruned, causing
unbounded retention of (upstream, subject) keys; change evict_subject and
evict_upstream to only remove matching build_locks entries when they are idle
(to preserve serialization for active builders) or add TTL/LRU metadata and a
periodic cleanup/purge pass. Concretely: update the code that manages
build_locks (the map referenced as build_locks) so that
evict_subject(evict_upstream) iterates matching keys and removes them only if
the lock is not held and has no waiters (or its last_used timestamp is older
than a configured threshold), or augment lock entries with last_used and run a
bounded cleanup routine that evicts stale entries; reference the methods
evict_subject, evict_upstream and the build_locks entry type when making the
change.
In `@crates/lab/src/oauth/upstream/manager.rs`:
- Around line 466-498: Wrap the final check-register-save sequence with a
per-(upstream, subject) lock using self.locks.acquire(...) (same pattern as
build_auth_client): acquire a lock keyed by (self.upstream.name.clone(),
subject.clone()), then inside the lock re-run the DB read via
find_dynamic_client_registration to see if a canonical client_id now exists; if
it does, use that to construct canonical_cfg and skip calling register_client,
otherwise call register_client, save_dynamic_client_registration, read back the
persisted value, and build the canonical OAuthClientConfig; ensure the lock
guard is dropped after that sequence so the lock is released.
---
Duplicate comments:
In `@crates/lab/src/dispatch/upstream/pool.rs`:
- Around line 714-732: The helpers cached_upstream_resource_uris and
cached_upstream_prompt_names are returning empty slices because the
discovery/listing code only updates counts and never populates
entry.resource_uris or entry.prompt_names; fix the upstream discovery/update
paths that currently increment counts to also write the discovered URIs and
prompt names into the catalog entry (entry.resource_uris and entry.prompt_names)
under the same catalog write lock used elsewhere so catalog snapshots reflect
the discovered data; search for the code that updates counts (the
discovery/listing handlers) and add assignment of the discovered Vec<String>
results to those fields when discovery succeeds.
In `@crates/lab/src/mcp/server.rs`:
- Line 1121: The import for CatalogSnapshot is misplaced between impl blocks;
move the line "use crate::mcp::catalog::CatalogSnapshot;" up into the top import
block where the other "use crate::..." statements are declared (near the
existing crate imports around the top of server.rs) so all crate imports are
grouped together and the impl blocks no longer contain stray use statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c1dc2db-8af9-4539-923c-3700e2da492a
📒 Files selected for processing (20)
.github/workflows/ci.ymlCargo.tomlcrates/lab-apis/src/extract/transport.rscrates/lab-auth/src/state.rscrates/lab/src/api/oauth.rscrates/lab/src/catalog.rscrates/lab/src/dispatch/deploy/dispatch.rscrates/lab/src/dispatch/deploy/runner.rscrates/lab/src/dispatch/gateway/manager.rscrates/lab/src/dispatch/redact.rscrates/lab/src/dispatch/upstream/pool.rscrates/lab/src/mcp/catalog.rscrates/lab/src/mcp/server.rscrates/lab/src/mcp/upstream.rscrates/lab/src/oauth/upstream/cache.rscrates/lab/src/oauth/upstream/encryption.rscrates/lab/src/oauth/upstream/manager.rscrates/lab/src/oauth/upstream/types.rscrates/lab/tests/deploy_runner.rsrust-toolchain.toml
Comment on lines
+131
to
+200
| # TODO(lint-pass): pre-existing warnings promoted to errors by RUSTFLAGS=-D warnings | ||
| # in CI. Allow for now; fix incrementally in follow-up PRs. | ||
| future_not_send = "allow" | ||
| doc_markdown = "allow" | ||
| missing_const_for_fn = "allow" | ||
| redundant_closure_for_method_calls = "allow" | ||
| collapsible_if = "allow" | ||
| option_if_let_else = "allow" | ||
| significant_drop_tightening = "allow" | ||
| cast_possible_truncation = "allow" | ||
| map_unwrap_or = "allow" | ||
| too_many_lines = "allow" | ||
| needless_pass_by_value = "allow" | ||
| unnecessary_wraps = "allow" | ||
| items_after_statements = "allow" | ||
| too_long_first_doc_paragraph = "allow" | ||
| match_same_arms = "allow" | ||
| needless_question_mark = "allow" | ||
| default_trait_access = "allow" | ||
| cast_possible_wrap = "allow" | ||
| redundant_clone = "allow" | ||
| option_option = "allow" | ||
| cast_lossless = "allow" | ||
| manual_let_else = "allow" | ||
| needless_borrow = "allow" | ||
| unused_self = "allow" | ||
| implicit_clone = "allow" | ||
| use_self = "allow" | ||
| assigning_clones = "allow" | ||
| ignored_unit_patterns = "allow" | ||
| type_complexity = "allow" | ||
| single_match_else = "allow" | ||
| uninlined_format_args = "allow" | ||
| missing_panics_doc = "allow" | ||
| cast_sign_loss = "allow" | ||
| redundant_closure = "allow" | ||
| similar_names = "allow" | ||
| double_must_use = "allow" | ||
| implicit_hasher = "allow" | ||
| or_fun_call = "allow" | ||
| doc_lazy_continuation = "allow" | ||
| format_push_string = "allow" | ||
| from_iter_instead_of_collect = "allow" | ||
| suboptimal_flops = "allow" | ||
| semicolon_if_nothing_returned = "allow" | ||
| single_match = "allow" | ||
| let_and_return = "allow" | ||
| used_underscore_binding = "allow" | ||
| unnecessary_semicolon = "allow" | ||
| too_many_arguments = "allow" | ||
| struct_excessive_bools = "allow" | ||
| needless_continue = "allow" | ||
| manual_clamp = "allow" | ||
| needless_collect = "allow" | ||
| large_stack_arrays = "allow" | ||
| wildcard_imports = "allow" | ||
| question_mark = "allow" | ||
| cloned_ref_to_slice_refs = "allow" | ||
| needless_lifetimes = "allow" | ||
| derive_partial_eq_without_eq = "allow" | ||
| iter_with_drain = "allow" | ||
| filter_map_bool_then = "allow" | ||
| if_not_else = "allow" | ||
| set_contains_or_insert = "allow" | ||
| ref_option = "allow" | ||
| nonminimal_bool = "allow" | ||
| needless_pass_by_ref_mut = "allow" | ||
| struct_field_names = "allow" | ||
| large_futures = "allow" | ||
| redundant_pub_crate = "allow" |
There was a problem hiding this comment.
Large clippy allow-list masks lints that can hide real bugs — track tightly.
The TODO comment is appreciated, but this batch is quite broad and several of these lints frequently catch actual defects rather than stylistic noise, notably:
nonminimal_bool,needless_pass_by_ref_mut,redundant_clone,question_mark,collapsible_if— can surface logic/ownership issues.suboptimal_flops,manual_clamp— numerical correctness.filter_map_bool_then,set_contains_or_insert— subtle semantics/perf regressions.cast_possible_truncation,cast_possible_wrap,cast_sign_loss,cast_lossless— silent truncation in security-sensitive code (this PR touches SSRF/IP-range checks and a rate limiter where integer casts matter).
Recommend:
- Filing a tracking issue that enumerates each allow and the owning crate/module, so the "follow-up PRs" don't become permanent.
- At minimum, re-enabling the
cast_*family oncrates/lab-authand the SSRF paths introduced in this PR before merge, since those are the highest-risk surfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 131 - 200, The current broad Clippy allow-list in
Cargo.toml hides important lints; create a tracking issue that enumerates every
allow entry and assigns an owning crate/module so follow-up PRs are tracked, and
then tighten the allow-list by removing or scoping back the highest-risk lints:
re-enable cast_possible_truncation, cast_possible_wrap, cast_sign_loss, and
cast_lossless (at least) for crates/lab-auth and any SSRF/IP-range checking
modules introduced in this PR, and consider re-enabling nonminimal_bool,
needless_pass_by_ref_mut, redundant_clone, question_mark, and collapsible_if
where logic/ownership risks exist; update the TODO comment to reference the new
tracking issue and scope any remaining allows to specific crates/modules rather
than globally.
| @@ -1,3 +1,5 @@ | |||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if redaction utilities are used anywhere in the codebase
echo "=== Searching for calls to redaction functions ==="
rg -n --type=rust -C2 '\b(is_sensitive_key|redact_url|redact_stdio_value|redact_stdio_args|redact_upstream_resource_uri)\s*\(' \
-g '!crates/lab/src/dispatch/redact.rs'
echo -e "\n=== Checking for imports of dispatch::redact ==="
rg -n --type=rust 'use.*dispatch::redact' -g '!crates/lab/src/dispatch/redact.rs'Repository: jmagar/lab
Length of output: 154
Module-level dead_code suppression suppresses genuinely unused utilities with no clear call site in this PR.
The #![allow(dead_code)] at line 1 masks warnings for all five public functions in this module (is_sensitive_key, redact_url, redact_stdio_value, redact_stdio_args, redact_upstream_resource_uri). Verification found zero calls to these functions and zero imports of dispatch::redact elsewhere in the codebase.
While these security utilities are well-designed and thoroughly tested, they are currently dead code. Clarify the intent:
- If needed in this PR: Call them where relevant (e.g., for SSRF validation logging, auth error redaction).
- If planned for near-term use: Document the expected integration point and apply targeted
#[allow(dead_code)]to specific items instead. - If deferred: Remove the module and introduce it when the dependency exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab/src/dispatch/redact.rs` at line 1, Remove the broad module-level
dead_code suppression and decide whether these utilities should stay: either (A)
wire them into this PR by calling the functions (is_sensitive_key, redact_url,
redact_stdio_value, redact_stdio_args, redact_upstream_resource_uri) from the
actual logging/SSRF/auth error paths that need redaction, (B) if they are
intended for near-term use, document the planned integration point in a comment
and replace the module-wide #![allow(dead_code)] with targeted
#[allow(dead_code)] on the specific functions you want to suppress, or (C) if
they are not needed now, delete the module and reintroduce it when the
integration exists; in all cases remove the top-level allow and ensure only
needed items are allowed or present.
| //! correctly and respects the concurrency/abort knobs. | ||
|
|
||
| #![cfg(feature = "deploy")] | ||
| #![allow(unused_qualifications)] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider narrowing the scope of the lint suppression.
The crate-level #![allow(unused_qualifications)] suppresses the lint for the entire test file. If only specific items trigger this warning, it would be better practice to apply #[allow(unused_qualifications)] to those specific items rather than blanket-suppressing it file-wide.
♻️ More targeted suppression approach
To identify which items need the suppression, temporarily remove the crate-level attribute and let the compiler show exactly where the warnings occur, then add item-level #[allow(unused_qualifications)] attributes to only those locations.
Example:
-#![allow(unused_qualifications)]
+// Apply only to specific items that need it:
+#[allow(unused_qualifications)]
+fn specific_function_that_needs_it() { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab/tests/deploy_runner.rs` at line 10, The file currently uses a
crate-level suppression `#![allow(unused_qualifications)]`; remove this
top-level attribute, run the compiler to see which items trigger the warning,
then add item-level attributes `#[allow(unused_qualifications)]` directly above
only those specific test functions, helper functions, or modules that produce
the warnings (e.g., put `#[allow(unused_qualifications)]` on the individual
`#[test]` functions or the helper `mod`/fn that the compiler reports) so the
lint is narrowed in scope rather than globally disabled.
…fold anchor The dispatch_deploy helper prevents rustfmt from merging '} // [lab-scaffold: cli-dispatch]' which broke scaffold tests. The anchor is now a standalone comment after the match arm comma, which rustfmt does not touch.
…de errors Move all SSH logic inside a #[cfg(unix)] block so the non-unix early return is at the top. This prevents unreachable_code, unused_mut, and unused_variable warnings-as-errors on Windows targets where AgentClient::connect_env (SSH_AUTH_SOCK) is not available.
…rustfmt Arc is only used inside the #[cfg(unix)] SSH block — unused on Windows. Shorten the non-unix error message to stay within rustfmt's line limit.
`forward_syslog_file` uses `std::os::unix::fs::MetadataExt` and `ino()` for log-rotation detection — Unix-only APIs absent on Windows. The function also reads `/var/log/syslog` which is Linux-specific. Gate the function itself with `#[cfg(unix)]` and add a `#[cfg(not(unix))]` fallback in `run()` that returns an error, so the Windows build compiles.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
`skip_whitespace_tokens` and `parse_syslog_line` are only used by `forward_syslog_file`, which is already unix-gated. Windows build emits dead_code errors for them; add matching `#[cfg(unix)]` guards.
The test asserted `target/release/lab` but on Windows the host triple contains "windows" so `expected_artifact_path_for` appends `.exe`. Check for `lab.exe` when the host triple indicates Windows.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/lab-apis/src/extract/transport.rs`:
- Around line 166-194: transport.rs currently reads SSH config and agent from
the environment (russh_config::parse_home(&host) and AgentClient::connect_env())
and other files (types.rs, inventory.rs, client.rs) call std::env::var; refactor
to remove all direct env/filesystem access by: 1) moving ~/.ssh/config parsing,
SSH_AUTH_SOCK discovery, $HOME and LAB_EXTRACT_SSH_DENYLIST resolution into
lab/src/config.rs (or a new dispatch layer) and have that module produce a
resolved SshConnectionParams struct (host, port, user, optional proxycommand,
auth keys/agent socket path, denylist); 2) change the transport constructor and
functions (e.g., the code that calls russh_config::parse_home,
client::connect_stream, and AgentClient::connect_env) to accept the resolved
SshConnectionParams and an already-connected AgentClient or its socket path
instead of calling parse_home or connect_env; and 3) replace all std::env::var
uses referenced in types.rs, inventory.rs, and client.rs with injection of the
corresponding values from config (pass denylist/home through constructors or
factory functions) so the library no longer reads env or filesystem directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d371afb-1416-459e-81a0-acbe273eb8c0
📒 Files selected for processing (4)
crates/lab-apis/src/extract/transport.rscrates/lab/src/cli.rscrates/lab/src/dispatch/deploy/build.rscrates/lab/src/dispatch/logs/forward.rs
Comment on lines
+166
to
194
| // Resolve the alias through ~/.ssh/config (Hostname, Port, User, ProxyCommand). | ||
| let cfg = russh_config::parse_home(&host).map_err(|e| ExtractError::Ssh { | ||
| host: host.clone(), | ||
| message: format!("ssh-agent: {e}"), | ||
| message: format!("ssh config: {e}"), | ||
| })?; | ||
| let user = cfg.user(); | ||
|
|
||
| let identities = agent | ||
| .request_identities() | ||
| .await | ||
| .map_err(|e| ExtractError::Ssh { | ||
| // Open TCP stream (or ProxyCommand pipe) to the resolved endpoint. | ||
| let stream = cfg.stream().await.map_err(|e| ExtractError::Ssh { | ||
| host: host.clone(), | ||
| message: format!("agent identities: {e}"), | ||
| message: format!("connect: {e}"), | ||
| })?; | ||
|
|
||
| let mut authenticated = false; | ||
| for identity in &identities { | ||
| // Only handle plain public-key identities; skip certificates. | ||
| let pub_key = match identity { | ||
| AgentIdentity::PublicKey { key, .. } => key.clone(), | ||
| AgentIdentity::Certificate { .. } => continue, | ||
| }; | ||
| // Perform the SSH handshake. | ||
| let ssh_cfg = Arc::new(client::Config::default()); | ||
| let mut handle = client::connect_stream(ssh_cfg, stream, ClientHandler) | ||
| .await | ||
| .map_err(|e| ExtractError::Ssh { | ||
| host: host.clone(), | ||
| message: format!("handshake: {e}"), | ||
| })?; | ||
|
|
||
| let result = handle | ||
| .authenticate_publickey_with(&user, pub_key, None, &mut agent) | ||
| // Authenticate using the running ssh-agent. | ||
| let mut agent = AgentClient::connect_env() | ||
| .await | ||
| .map_err(|e| ExtractError::Ssh { | ||
| host: host.clone(), | ||
| message: format!("agent auth: {e:?}"), | ||
| message: format!("ssh-agent: {e}"), | ||
| })?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify SSH config/env reads are not kept inside lab-apis constructors.
# Expected: no production matches under crates/lab-apis/src, or matches only after migration to lab config/dispatch code.
rg -n -C3 'russh_config::parse_home|AgentClient::connect_env|SSH_AUTH_SOCK|std::env|\.ssh/config' crates/lab-apis/srcRepository: jmagar/lab
Length of output: 9758
Refactor extract module to eliminate env/filesystem reads from lab-apis.
The extract module violates the architectural guideline: "Do not read std::env or filesystem — config flows from lab/src/config.rs into constructors."
- transport.rs:167 —
russh_config::parse_home(&host)implicitly reads~/.ssh/config - transport.rs:189 —
AgentClient::connect_env()readsSSH_AUTH_SOCK - types.rs:69, inventory.rs:11, 32, client.rs:588 — Direct
std::env::var()calls for$HOMEandLAB_EXTRACT_SSH_DENYLIST
Move all environment resolution, SSH config parsing, and ssh-agent discovery into lab/src/config.rs or a dedicated dispatch layer. Pass resolved connection parameters (host, port, user, auth keys, denylist) into library constructors instead. This keeps the library boundary clean and testable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lab-apis/src/extract/transport.rs` around lines 166 - 194,
transport.rs currently reads SSH config and agent from the environment
(russh_config::parse_home(&host) and AgentClient::connect_env()) and other files
(types.rs, inventory.rs, client.rs) call std::env::var; refactor to remove all
direct env/filesystem access by: 1) moving ~/.ssh/config parsing, SSH_AUTH_SOCK
discovery, $HOME and LAB_EXTRACT_SSH_DENYLIST resolution into lab/src/config.rs
(or a new dispatch layer) and have that module produce a resolved
SshConnectionParams struct (host, port, user, optional proxycommand, auth
keys/agent socket path, denylist); 2) change the transport constructor and
functions (e.g., the code that calls russh_config::parse_home,
client::connect_stream, and AgentClient::connect_env) to accept the resolved
SshConnectionParams and an already-connected AgentClient or its socket path
instead of calling parse_home or connect_env; and 3) replace all std::env::var
uses referenced in types.rs, inventory.rs, and client.rs with injection of the
corresponding values from config (pass denylist/home through constructors or
factory functions) so the library no longer reads env or filesystem directly.
On Linux a loopback connection to a closed port returns ECONNREFUSED immediately → 502 BAD_GATEWAY. On Windows the same connection may time out before returning ECONNREFUSED → 504 GATEWAY_TIMEOUT. Both responses correctly indicate the relay couldn't reach the upstream.
This was referenced Apr 20, 2026
jmagar
added a commit
that referenced
this pull request
May 2, 2026
* spike: validate rmcp AuthClient integration with StreamableHttpClientWorker
Task 0 (gating spike) for the upstream MCP OAuth PKCE plan. Confirms four
integration points against rmcp 1.4.0 before Task 2 commits to a design:
1. AuthClient<reqwest::Client> constructs cleanly over AuthorizationManager
+ InMemoryCredentialStore.
2. AuthClient auto-injects Authorization: Bearer <token> when the caller
passes auth_token: None — its StreamableHttpClient impl calls
auth_manager.get_access_token() and fills the slot before delegating.
3. rmcp does NOT automatically refresh on a 401 from the upstream.
AuthorizationManager::get_access_token() only refreshes on the local
clock (REFRESH_BUFFER_SECS = 30s). Refresh-on-401 is the caller's
responsibility, so Task 2 must layer it on.
4. Spike runs against a wiremock AS+RS stub by default, and against a
real OAuth-protected MCP upstream when SPIKE_REAL_AS_URL is set, so
the operator can validate end-to-end interactively before Task 2
starts.
Plan A (AuthClient as StreamableHttpClient) is confirmed; Plan B (custom
wrapper that calls get_access_token() pre-request) is strictly inferior
and not needed.
Findings duplicated inline at the top of the spike example AND in the
stub crates/lab/src/oauth/upstream/refresh.rs that Task 2 will replace.
The upstream/* files are intentionally NOT wired into oauth.rs yet —
they are exploratory docs.
* feat: add rmcp-backed upstream oauth manager with single-flight refresh and at-rest encryption
- store.rs: SqliteCredentialStore + SqliteStateStore implementing rmcp CredentialStore/StateStore traits
- ChaCha20-Poly1305 encryption at rest; decryption failure → AuthorizationRequired
- StateStore::load uses atomic DELETE…RETURNING (take_upstream_oauth_state); delete is no-op
- Two-lifetime pattern ('life0: 'async_trait, Self: 'async_trait) matching async_trait expansion
- refresh.rs: RefreshLocks (DashMap per-(upstream,subject) Mutex) + refresh_if_stale()
- Single-flight serialization prevents concurrent refresh storms
- AuthError::AuthorizationRequired → OauthError::NeedsReauth
- manager.rs: UpstreamOauthManager orchestrates full authorization_code+PKCE flow
- begin_authorization: discovers/caches AS metadata, enforces S256, saves PKCE state
- complete_authorization_callback: exchanges code, persists encrypted credentials
- clear_credentials: deletes tokens + evicts pending state
- build_auth_client: creates fresh AuthClient from stored credentials with proactive refresh
- Supports Preregistered and Dynamic registration; ClientMetadataDocument is TODO
- Cargo.toml: promote oauth2 from dev-dep to regular dep (needed for TokenResponse trait methods)
* feat(upstream-oauth): wire HTTP routes and AppState for upstream OAuth callback
Task 3: mount /v1/upstream-oauth/:name/{start,callback} routes in the axum
router, guarded by upstream_oauth.is_some(). Add upstream_oauth field to
AppState with with_upstream_oauth() builder. Routes are outside the bearer
middleware — browser redirects from the AS cannot carry Authorization headers.
* feat(upstream): wire per-(upstream,subject) AuthClient cache into UpstreamPool
Add oauth_managers field to UpstreamPool (DashMap keyed by upstream name),
with_oauth_managers() builder for injection at serve time, and OAuth branch
in connect_http_upstream: looks up manager, calls build_auth_client("default"),
wraps the AuthClient in StreamableHttpClientWorker. Non-OAuth path unchanged.
oauth_required error on NeedsReauth marks upstream unhealthy via discover_all.
* docs: add upstream oauth pkce gateway guidance and error kinds
* feat(upstream-oauth): enforce S256, issuer binding, canonical resource, CIMD
Completes Task 2 §6 spec-aligned invariants identified by plan audit:
- Must-Fix #1 S256 enforcement: verify_s256 now rejects AS metadata that
omits `code_challenge_methods_supported` (previously warned and continued)
or advertises only non-S256 methods. Both paths surface
`oauth_unsupported_method`.
- Must-Fix #4 canonical resource indicator: UpstreamConfig gains
`canonical_url()` applying RFC 3986 §6.2.2 normalization at
validation time (lowercase scheme+host, strip default port,
dot-segment removal, percent-encoding case). Manager uses the
canonical form when constructing the AuthorizationManager so
rmcp's `resource` parameter on authorize and token is byte-identical
to the canonical upstream URL. Known gap: rmcp 1.4 does not re-emit
`resource` on the refresh_token grant; documented in UPSTREAM.md.
- Must-Fix #5 issuer binding (scope 6b): verify_issuer_binding requires
`metadata.issuer` to be present and enforces host-consistency across
authorization_endpoint, token_endpoint, and registration_endpoint
(when present). Cannot duplicate rmcp discovery to bind against the
successful discovery URL, so the check is approximated via endpoint
host consistency. Violations surface as `oauth_issuer_mismatch`.
- CIMD registration: ClientMetadataDocument strategy now constructs the
OAuth client locally, using the metadata document URL as the
client_id. No registration_endpoint call is issued.
- Must-Fix #6 reactive 401 (scope 6b): deferred. rmcp's
StreamableHttpClientWorker hides the raw HTTP response, so a 401 on
an MCP call surfaces as a generic transport error. Operators
recover via `POST /v1/gateway/oauth/start`. Documented in UPSTREAM.md
so the doc no longer promises retry semantics that code does not
implement.
Also ships Task 2/3 scaffolding kept uncommitted in the worktree:
UpstreamOauthCredentialRow + UpstreamOauthStateRow (manual redacted
Debug), UpstreamOauthConfig + UpstreamOauthRegistration enum (CIMD,
Preregistered, Dynamic), and `oauth: None` test-fixture fill-ins.
Tests:
- crates/lab/tests/upstream_oauth.rs (8 tests)
- canonical_url_strips_default_port_and_lowercases_host
- missing_code_challenge_methods_returns_unsupported
- plain_pkce_only_returns_unsupported
- authorize_url_carries_canonical_resource_indicator
- token_exchange_carries_canonical_resource_indicator
- issuer_missing_returns_issuer_mismatch
- issuer_endpoint_host_mismatch_returns_issuer_mismatch
- cimd_registration_uses_metadata_url_as_client_id
* feat: wire subject-scoped upstream oauth cache
* feat: finish upstream oauth gateway wiring and verification
* fix: align upstream oauth HTTP surface with ERRORS.md spec and add dispatch telemetry
Add elapsed_ms field to all four OAuth handlers (start, status, clear, callback)
so every dispatch event includes surface/service/action/elapsed_ms per OBSERVABILITY.md.
Change clear handler confirmation_required response from 400 plain-text to 422 JSON
envelope (ToolError::Sdk) to match the documented spec in ERRORS.md. Update
accompanying test to assert 422 UNPROCESSABLE_ENTITY and JSON kind field.
Resolves review threads:
PRRT_kwDOR8nC1M576Bjh (missing dispatch logs)
Note: router callback placement, HTML escaping (html_escape helper already present),
subject validation (subject sourced from JWT auth.sub not query param), and axum
path syntax were all already correctly implemented — no changes needed.
* fix: harden upstream oauth manager - encryption, TOCTOU, issuer binding, client secret
seal()/seal_with_aad() now return Result<_, EncryptionError> instead of panicking via
.expect(); store.rs propagates the error as AuthError::InternalError.
get_or_discover_metadata holds the write lock across discovery to eliminate the
read-lock-drop-write-lock TOCTOU race where two callers could both issue discovery.
verify_issuer_binding changed from host-only comparison to full origin comparison
(scheme + host + explicit port) so http/https scheme and port differences are caught.
resolve_client_config now returns OauthError::Internal when client_secret_env names
an env var that is not set or empty, instead of silently using an empty secret.
Resolves review threads:
PRRT_kwDOR8nC1M576Bjk (seal() panics - encryption.rs)
PRRT_kwDOR8nC1M576Cmu (seal() panics - encryption.rs)
PRRT_kwDOR8nC1M576BYd (refresh lock TOCTOU)
PRRT_kwDOR8nC1M576Cmn (metadata cache TOCTOU)
PRRT_kwDOR8nC1M576ez2 (issuer binding host-only)
PRRT_kwDOR8nC1M576Bjm (missing client_secret env var silent)
PRRT_kwDOR8nC1M576Cmp (missing client_secret env var silent)
Note: extract_state_param None-guard (FIX E) and in-memory PKCE map TTL (FIX I)
were both already correctly implemented in this codebase — no changes needed.
* fix: enforce upstream config validation at startup and fix cleanup_expired predicate
UpstreamConfig::validate() is now called for each upstream in load_toml(), so invalid
configs (bad URL scheme, conflicting auth fields) are caught at startup rather than
at first OAuth flow attempt. validate() also now rejects non-http/https URL schemes.
cleanup_expired changes both DELETE predicates from < to <= so rows expiring exactly
at the current timestamp are cleaned up consistently with the rest of the expiry checks.
Resolves review threads:
PRRT_kwDOR8nC1M576ez0 (validate() never called at startup)
PRRT_kwDOR8nC1M576dqS (config.rs critical - URL scheme not validated)
PRRT_kwDOR8nC1M576dqQ (cleanup_expired < vs <= off-by-one)
Note: OauthError::Internal display/kind strings were already aligned in types.rs
(Display prefix matches kind() return value) — no change needed.
* fix: redact token from spike error, wire gateway validation, correct docs
spike_rmcp_auth_client.rs: remove token value from bail! error message to prevent
leaking access tokens into log output (use placeholder instead).
gateway/config.rs validate_upstream: call upstream.validate() at the start so that
bearer_token_env + oauth mutual-exclusion and other config constraints are enforced
in the gateway dispatch layer, not only in the top-level config loader.
docs/CONFIG.md: fix clear endpoint URL example to include required upstream= param.
docs/GATEWAY.md:
- Update clear endpoint description to reflect 422 JSON response (not 400 plain-text)
and document required upstream= query param
- Correct callback-security section: remove claim that handler re-validates
upstream-vs-state-row in application code (enforcement is via SQL primary key)
docs/UPSTREAM.md: correct claim that OAuth upstreams participate in startup discovery;
they are excluded from discover_all and connected per-request, not pooled.
Resolves review threads:
PRRT_kwDOR8nC1M576Bjb (token in spike error)
PRRT_kwDOR8nC1M576dqT (gateway/config.rs critical - validate not called)
PRRT_kwDOR8nC1M576dqW (CONFIG.md clear URL missing upstream=)
PRRT_kwDOR8nC1M576dqa (GATEWAY.md:140 clear endpoint description)
PRRT_kwDOR8nC1M576dqc (GATEWAY.md:152 callback invariants not in code)
PRRT_kwDOR8nC1M576dqe (UPSTREAM.md:158 OAuth discovery claim inaccurate)
Note: with_oauth_managers() wiring (FIX G) was already correctly implemented via
with_oauth_client_cache() in cli/serve.rs. OAuth error kinds in ERRORS.md were
already documented. No changes needed for those items.
* fix(oauth): remove dead url_host fn, duplicate tool_error_from_oauth, stale comment
Resolves review threads #15, #3, #21.
- Remove #[allow(dead_code)] url_host() from manager.rs (thread 15)
- Remove duplicate pub tool_error_from_oauth from gateway/oauth.rs; the
private copy in manager.rs is the only caller (thread 3)
- Drop unused OauthError import from oauth.rs
- Correct stale comment in spike_rmcp_auth_client.rs: mock returns 401
to drive re-auth path, not 200 (thread 21)
* docs(oauth): align upstream OAuth docs with implementation
Resolves review threads #4, #17, #18, #19, #24, #25, #26, #27, #28,
#29, #30, #36, #37, #38.
UPSTREAM.md:
- Thread 4: OAuth upstreams are attempted at startup and fail unhealthy
(not excluded entirely)
- Thread 18: Issuer binding checks origin (scheme+host+port), not just
host; covers auth/token/revocation/userinfo endpoints
- Thread 19: Remove false LRU-cap claim; lock entries live for process
lifetime
- Thread 24: Merged catalog is transport-neutral; OAuth upstreams appear
in catalog but need HTTP session to initiate authorization
- Thread 28: POST /v1/gateway/oauth/start route references are correct
- Thread 38: Remove auto-delete claim on invalid_grant (not implemented)
GATEWAY.md:
- Thread 17: Pending state SQL key is (upstream_name, subject,
csrf_token), not just (upstream_name, csrf_token)
- Thread 25: Reload eagerly evicts all OAuth AuthClient entries; remove
false built_with_client_id eviction-on-mismatch claim
- Thread 26: Routes /v1/gateway/oauth/* are implemented as documented
- Thread 27: Callback is browser-facing; subject from session cookie,
not from state parameter
ERRORS.md:
- Thread 30: Remove "(RFC 7636 absence implies plain-only)" — omission
of code_challenge_methods_supported is not equivalent to plain-only
- Thread 36: oauth_issuer_mismatch triggers on missing issuer or
endpoint/issuer origin mismatch, not direct discovered-URL equality
- Thread 37: Route references /v1/gateway/oauth/status and
POST /v1/gateway/oauth/start are correct (no /v1/upstream-oauth/ routes)
* fix(oauth): evict build_locks, fix param attribution, https guard, TTL guard, epoch default
Resolves review threads #1, #2/#9, #16, #20, #31, #32, #35.
cache.rs (thread 1):
- evict_subject and evict_upstream now also remove entries from
build_locks, preventing unbounded growth on long-running processes
config.rs (threads 2/9):
- validate_upstream maps ConfigError::InvalidUrl to param="url" instead
of "bearer_token_env"; auth-conflict errors still map to bearer_token_env
manager.rs (thread 35):
- ClientMetadataDocument URL validation now enforces https scheme;
http URLs are rejected with OauthError::Internal
store.rs (thread 16):
- token_received_at falls back to now_unix() instead of 0 (Unix epoch)
when absent; prevents access_token_expires_at underflow for tokens
that don't carry a received_at timestamp
sqlite.rs (threads 20, 31, 32):
- TTL guard now also rejects expires_at <= created_at (negative delta)
to prevent integer underflow on malicious/clock-skewed input
- cleanup queries already used <=; threads 31/32 already resolved
* fix(oauth): stdio ordering, oauth URL guard, is_master gating
Resolves review threads #7, #8, #22.
serve.rs (thread 7):
- Compute stdio_mode before build_upstream_oauth_runtime; skip OAuth
runtime init entirely in stdio mode so missing LAB_PUBLIC_URL /
LAB_OAUTH_ENCRYPTION_KEY never fails a stdio serve
config.rs / gateway config.rs (thread 8):
- UpstreamConfig::validate now rejects oauth+no-url combinations with
ConfigError::MissingOauthUrl; gateway config dispatch maps the new
variant to param="url"
router.rs (thread 22):
- Gateway OAuth routes (/v1/gateway/oauth/*) and browser callback are
now guarded by is_master; non-master nodes no longer mount them
* fix(oauth): redirect error kind, circuit breaker, tracing, DashMap clone, callback ext, reload reconcile
Resolves review threads #5, #6, #10, #11, #12, #13, #23, #50/#71.
upstream_oauth.rs (threads 5, 23):
- Callback: embed error_kind in redirect URL query params instead of
x-lab-oauth-error-kind header (browsers silently discard headers on
302 responses)
- Callback: extract AuthContext via Option<Extension<AuthContext>>
instead of reconstructing Parts from empty request (which discarded
middleware extensions); update callback_subject signature accordingly
pool.rs (threads 6, 11):
- subject_scoped_call_tool: add circuit breaker calls (record_success_for
/ record_failure_for) around the peer call
- subject_scoped_read_resource: add circuit breaker calls AND response
size guard matching the non-scoped read_upstream_resource path
- subject_scoped_get_prompt: add circuit breaker calls
server.rs (thread 12):
- Subject-scoped dispatch path now emits tracing::info! on success and
tracing::warn! on failure, matching the non-subject-scoped path
cache.rs (thread 13):
- get_or_build: clone DashMap Ref before the .await call on
build_auth_client to avoid holding a DashMap read-lock across await
(potential deadlock under contention)
manager.rs (thread 10):
- reload: reconcile upstream_oauth_managers after loading new config;
remove managers for OAuth upstreams no longer present, warn about
new OAuth upstreams that need restart to get a manager
Threads 50/71 (TokenRefreshFailed → NeedsReauth): already mapped
correctly in map_auth_error; no change needed.
* fix(oauth): dynamic registration once per upstream, not per call
Resolves review thread PRRT_kwDOR8nC1M579vdo
`configured_authorization_manager` was calling `register_client` on
every invocation (complete_authorization_callback, build_auth_client),
receiving a new AS-assigned client_id each time — mismatching the id
used to start the flow.
Fix: `resolve_client_config` for Dynamic now:
1. Checks stored credential row (available after token exchange)
2. Checks in-memory `dynamic_client_ids` cache (populated by begin_authorization)
3. Only calls register_client on the very first invocation
`clear_credentials` evicts the in-memory cache entry so a fresh
registration is issued when re-authorizing after credential clearance.
* fix(gateway): exhaustive ConfigError match, no catch-all for param attribution
Resolves review thread PRRT_kwDOR8nC1M579vdf
The wildcard arm `_ => param: "bearer_token_env"` was too broad: any
future ConfigError variant would be misattributed to bearer_token_env.
Replace the catch-all with an exhaustive match over all three ConfigError
variants so the compiler enforces correct attribution if new variants are
added. ConflictingAuth → bearer_token_env; MissingOauthUrl + InvalidUrl → url.
* fix(oauth): persist dynamic client registrations to SQLite
Dynamic registration (RFC 7591) must survive server restarts. Replace
the in-memory DashMap cache with a durable `upstream_oauth_dynamic_clients`
SQLite table (WITHOUT ROWID, PK: upstream_name + subject).
- Add `upstream_oauth_dynamic_clients` table to `open_connection` schema
- Add `save_dynamic_client_registration` (UPSERT), `find_dynamic_client_registration`
(SELECT), and `delete_dynamic_client_registration` (DELETE) to `SqliteStore`
- Add round-trip test covering save, upsert, find, delete, and subject isolation
- Remove `dynamic_client_ids: Arc<DashMap<...>>` field from `UpstreamOauthManager`
- `resolve_client_config` Dynamic branch now checks SQLite before calling
`register_client`, then persists the assigned client_id immediately
- `clear_credentials` also deletes the dynamic client registration row
client_secret is not stored: `token_endpoint_auth_method: "none"` means the
AS issues public clients and must not return a client_secret.
* feat(gateway-admin): upstream OAuth connection UI
Adds an "Upstream Connections" section to the Gateways page with per-upstream
Connect/Disconnect cards. Cards poll auth status, open the authorization URL
in a new tab, and wait for the callback before clearing the connecting state.
- GET /v1/gateway/oauth/upstreams — lists upstreams with oauth: config
- upstream-oauth-card: badge (Connected/Expiring/Disconnected), Connect/Disconnect
- upstream-oauth-section: grid of cards, null when no oauth upstreams configured
- Fix redirect loop: restore allow_session_cookie guard on browser redirect
- Fix redirect loop: switch v1 + MCP auth middleware to route_layer so unmatched
SPA paths (e.g. /gateways) are not intercepted by auth middleware
* style: rustfmt router.rs
This was referenced May 5, 2026
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.
Summary
mcpregistrySDK module tolab-apis—McpRegistryClientwith catalog browsing, server search, and install actionsdispatch/mcpregistry/) and wire all surfaces (MCP, CLI, HTTP API)mcpregistry installCLI compose command for scaffolding.mcp.jsonentrieslab-auth, catalog refactor,cursor-based pagination (replacesoffset)spawn_blockingfor SSRF defensegateway.addTest plan
cargo build --workspace --all-featurespassescargo test --workspace --all-featurespasseslab mcpregistry server.listreturns catalog resultslab mcpregistry server.search --query mcpreturns filtered resultslab gateway addwith an RFC1918 URL returns SSRF errorlab gateway addwith HTTP (non-HTTPS) URL returns errorSummary by cubic
Adds the read‑only
mcpregistryservice with SDK, dispatch, and CLI/MCP/HTTP endpoints, plus aninstallflow to add registry servers as gateway upstreams. Also tightens SSRF defenses, adds an auth rate limiter, improves upstream OAuth, switches registry pagination to cursors, pins Rust 1.94.1, fixes Windows build/test edge cases, and bumps to0.4.0.New Features
lab-apis: newMcpRegistryClient(list/search/get/versions/validate) with health checks and no‑redirect HTTP.lab mcpregistry+install), MCP tool, and HTTPPOST /v1/mcpregistry.spawn_blocking./authorizeand/register.normalizeGatewayApiBase/gatewayHeaders; UI detects popup blocking;/gateway/oauthmounted only with a gateway manager.server.list.rust-toolchain.tomland CI; allow pre‑existing pedantic/nursery lints; gate SSH/syslog logic on Unix and early‑return on non‑Unix to fix Windows builds; fix Windows artifact path test for.exe; accept 502 or 504 in unreachable‑target relay test on Windows; restore standalone CLI scaffold anchor; expanddeny.tomlignores; version bump to0.4.0.Migration
https://; private/loopback/link‑local hosts are rejected.cursorformcpregistry server.listpagination;offsetis no longer accepted.bearer_token_envto an environment variable name; raw token values are rejected.mcpregistryfeature when building selectively (included inall).Written for commit 8671e68. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation