fix(review): preserve PR 507 remediation stack - #520
Draft
jmagar wants to merge 10 commits into
Draft
Conversation
… backstop Three review findings from PR #507. `gateway.add`, `gateway.import`, and `gateway.reload` were reachable from a protected subset route. None takes an upstream name `ensure_visible` can check — the name does not exist yet, or there is none — so an admin-scoped token minted for an unrelated subset could add a stdio upstream and reload it into existence. `gateway.test` already refuses an inline spec for exactly this execution risk. Adds `GatewayEnrichmentScope::deny_on_subset_route` and applies it to all three. `gateway.skills.list` ignored the route scope entirely: it validated the `upstream` filter against the full config and enumerated every upstream's skills. Its `not_found` message also confirmed which names exist. Scope is now enforced before the existence probe, so a hidden upstream is indistinguishable from an absent one, and the unfiltered listing is restricted to route-visible upstreams. The HTTP transport backstop applied a fixed 30s cap to every non-`/mcp` route. `/v1` handlers relay upstream calls too (palette execute, gateway dispatch, `gateway.oauth.wait` at 120s), so this returned a bare 504 for calls that had already succeeded — the regression documented on `LabConfig::http_request_timeout` — and blinded the upstream circuit breaker, which declines to count `Cancelled` as a failure precisely because the backstop is derived to exceed the upstream deadline. Since HTTP_REQUEST_TIMEOUT_MARGIN is 30s the configured value is always >= the old cap, so the path split was dead logic; the backstop is now config-derived on every route. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `land:` merges behind #502/#503 dropped `mod` declarations while leaving the files on disk. Where something referenced the missing symbols that showed up as a broken build; where nothing did, it was silent. `paginate.rs`, `listing_bounds_tests.rs`, and `listing_timeout_tests.rs` were orphaned that way and eleven tests stopped running without a single warning — rustc never sees an undeclared file, so there is no `dead_code` lint to fire. Adds `xtask::module_reachability`, which walks `crates/*/src` and asserts every `.rs` file is declared by its parent (or attached by a sibling `#[path]`, as `dispatch.rs` does for `dispatch_tests.rs`). Wired into `just lint`. Resolves the three existing orphans it found: - `listing_timeout_tests.rs` is declared again; both tests pass unchanged. - `paginate.rs` is deleted. `catalog_pagination.rs::collect_bounded` supersedes it with strictly tighter bounds (64-page cap plus per-page item, cursor-byte, and deadline caps), and the wall-clock cap it also owned was already restored into `resources_list.rs` as `catalog_listing_timeout`. - `listing_bounds_tests.rs` is deleted rather than revived: 8 of its 9 tests assert the superseded contract, in which a looping cursor yielded a truncated but successful listing. `collect_bounded` deliberately makes a bounds breach a hard error — see `a_bounds_breach_is_still_an_error_not_an_empty_catalog`. `upstream/CLAUDE.md` documented `paginate.rs` as live, including a wall-clock guarantee that had not been compiled in months; corrected to describe `catalog_pagination.rs`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ox probe Two regressions where the land merges dropped *call sites*, and the repair completed the deletion instead of restoring the callers. `project_operator_skills` emitted five of its seven fields. `identity` and the structured `exposure` decision are a documented contract — the worked JSON in docs/guides/SKILLS_AND_LOADOUTS.md shows both, and the reason strings are named API vocabulary — and dropping them left `SkillExposureDecision::status` / `reason` / `matched_pattern` dead on the operator path, so an operator could no longer tell "no pattern matched" from "never advertised". `sandbox_absent` lost its two call sites and was then deleted outright. `msb remove` reports failure both when cleanup genuinely broke and when the sandbox was already gone (swept by a prior pass, or a crashed microVM); only the listing separates them. Without the probe every benign already-absent sweep recorded `cleanup_failed`, emitted a warning, and left the owner permanently accounted as leaked. Both branches probe again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI never validated this branch — every job died on the runner's missing `python3` — so none of this was caught. Reverts the `gateway.add` / `gateway.import` / `gateway.reload` subset-route deny added in 02fb557. Investigation showed it was wrong: the PR's own docs/runtime/OAUTH.md enumerates the restricted operations and these are deliberately not among them, and `gateway_add_through_mcp_protected_route_suppresses_hidden_enrichment_suggestion` explicitly asserts a subset route may add a gateway. The route bounds which named upstreams you may touch; it does not forbid creating new ones when the token carries gateway admin. (The residual asymmetry — `gateway.test` refuses an inline stdio spec while `gateway.add` reconciles one into existence — is left for the maintainer to judge.) The `gateway.skills.list` scoping from that commit stands: it is a discovery operation over existing upstreams, which the doc does cover. `gateway.import_pending.approve` gained an `ensure_visible` check that contradicts the pre-existing `gateway_pending_import_approve_through_mcp_protected_route_suppresses_hidden_enrichment_suggestion`. A pending import names a *discovered* server that is not configured yet, so it is never in the route's visible set and the check could only ever reject. Approving creates an upstream exactly as `gateway.add` does; the scope is threaded through so the enrichment suggestion is suppressed, which is the actual intent. Reverted, and the action dropped from the enumeration test that asserted the opposite. `Cancelled` was threaded through five layers with dedicated tests proving the kind survives each hop, then folded into `internal_error` at the MCP surface. The resource handler additionally collapsed `Cancelled`, `Timeout`, and `TooLarge` into one `upstream_error`. These are distinct documented kinds with different recovery advice (docs/dev/ERRORS.md) — `cancelled` is explicitly not automatically retryable, so an agent told only "could not be fetched" retries work the pool already classified as withdrawn. Cancellation is caller-driven and no longer logs at ERROR, which was paging operators for healthy upstreams on client disconnects. `handler_operational_failures_are_redacted_upstream_errors` now asserts the oversized and upstream-fault branches separately, which its loop always exercised but could not distinguish. `trusted_read_only_tools` has no production reader — the read-only gate uses the upstream's own `readOnlyHint` — but stays settable via `gateway.code_mode.set` and advertised in the catalog. Setting it now warns rather than silently accepting an inert security control. Also reverts the `sandbox_absent` restoration from 68c67eb: the probe was superseded by this branch's `reconcile_failed_cleanups`, and `absent_failed_cleanup_is_reconciled_and_active_count_recovers` asserts a failed cleanup stays accounted until absence is proven by reconciliation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Takes the suite from 6 failures to 0. These were invisible because CI never
ran on this branch, and `main` has not compiled for 30 commits, so nothing
here had been executed in a long time.
Microsandbox reconciliation decided liveness with `Path::new("/proc/{pid}")
.exists()`. `/proc` is Linux-only, so on every other Unix each owner read as
dead and reconciliation deleted sandboxes belonging to *live* processes,
including the running one. Replaced with a `kill(pid, 0)` probe that fails
closed: `ESRCH` is death, `EPERM` (owned by another user) is life, and anything
we cannot classify is left alone. A pid outside the platform's range cannot
name a live process, so that is treated as proof of death — which is how the
reconciler still collects the dead owner the test plants.
Four tests read the developer's machine instead of a fixture:
- `settings_config_update_dispatch_persists_and_rejects_stale_previous` loaded
the real `~/.labby/.env`. On any host that actually runs Labby that file
defines `LABBY_MCP_HTTP_PORT`, so `mcp.port` reported as env-shadowed and the
test failed for reasons unrelated to it. Now pins `set_test_lab_home`, as its
sibling test already did.
- `gateway_import_result_has_correct_shape` claims to exercise "empty
discovery" but walked the real home for editor MCP configs, then tried to
import whatever it found. Adds a `#[cfg(test)]` discovery-home override
mirroring `set_test_config_toml_path`, and pins it at an empty directory.
- `disabled_code_mode_app_denies_cached_resource_reads` and
`stateless_http_fresh_tools_list_observes_code_mode_app_state_changes`
disabled the app through the mirrored session atomic. A manager-backed server
reads the published config directly so a config mutation cannot race that
atomic, and `mcp_ui_enabled` now defaults to false, so neither test was
toggling anything. Both drive the config, preserving their actual intent.
`timeout_and_cancellation_clean_private_staging_and_release_capacity` waited a
fixed 5ms for staging cleanup to follow an aborted acquisition, which is not
reliably enough under parallel load. Polls to a deadline instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correctness - access/store.rs: a Skill Library commit that succeeded was reported as a failure when the lease rollback failed, so callers retried and duplicated work that had already landed. The executor outcome is now authoritative and the rollback failure is recorded as observability. - access/store.rs: arbitrary executor closures now run under the connection mutex, so one panic poisoned it and made every subsequent authorization permanently unavailable. Recover from poisoning instead; an unwind drops the transaction, which rolls back. - artifacts/store.rs: a failed workspace promotion or symlink rejection leaked the fully materialized staging tree with nothing to reap it. - Cargo.toml: the non-optional labby-gateway dev-dependency enabled `testkit` for every test build, making `proxy-testkit` inert and pulling the gateway crate into slice builds the feature contract exists to isolate. Gate the testkit-dependent tests on `proxy-testkit` instead. Observability - api/router.rs: four authorization failure arms on the new project-scoped protected-route binding logged nothing, so an empty project tool list had no discoverable cause. Also replaces a request-path `expect` with a graceful rejection. - skill_library/auth.rs: log the concrete AccessStoreError before collapsing Locked/Corrupt/DiskFull into one opaque kind; audit and log malformed canonical identifiers; promote a `debug_assert` guarding a hardcoded permission to a real check. - gateway/projection.rs: redact upstream error text before it reaches an operator-visible warning, as every other upstream-derived string here does. - gateway/dispatch.rs: `gateway.code_mode.set` now discards the retired `trusted_read_only_tools` value rather than persisting it, and the field is no longer serialized back out, so no surface presents it as a live control. Deletes the matcher it left behind. Tests - Regression coverage for the read-only Code Mode gate (both directions), the pid-liveness portability fix, workspace rollback and recovery-tree cleanup, and bounded weak-generation growth. - Replaces a tautological timeout test and a wall-clock "did not finish in 500ms" linearization assertion with assertions that can actually fail. - RAII guards for the process-global test home overrides, which leaked to unrelated tests on any panic or early return. - xtask module-reachability now reports a directory no module file can declare; previously an entire orphaned subtree passed the gate silently. Docs - CODE_MODE.md: state plainly that the read-only gate now rests entirely on the upstream's own readOnlyHint, with no operator-held second allowlist. - OAUTH.md: complete the scoped-operation list and document the creation carve-out (`gateway.add`, `import_pending.approve`) and the gateway-global mutation surface that scope alone gates. - upstream/CLAUDE.md: add rows for the three restored pool modules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Preserve and review the complete PR #507 remediation lineage as one superset branch.
Why This Exists
Nine shared remediation commits plus a final review-fix commit were local-only and at risk of becoming orphaned.
Resolution
The complete ten-commit superset is published here; the shorter review-fixes-507 branch is retained only as historical ancestry.
Reviewer Considerations
Confirm which fixes already landed through other histories before rebasing. Pay particular attention to protected routes, stable error kinds, skill exposure, source reachability, and test isolation.
Behavior Changes
Restores and hardens runtime, gateway, authorization, skill exposure, and portability behavior from the review stack.
Verification
Risk
High overlap with later branches; deduplicate before making ready.