Conversation
26e0b71 to
d8a73a7
Compare
|
not enough evidence A mergeable current-main review tree is unavailable. The bound controller recorded deterministic patch replay failure at patch_replay_conflict, and a read-only three-tree merge confirms unresolved conflicts between current main a4973c3 and PR head d8a73a774fd6a11ffc9d15bce1b46270e9a90516, including import conflicts in model_tools.py and tools/discord_tool.py plus concurrent changes in gateway and tool-registry security paths. The PR-head patch is syntactically valid and whitespace-clean, but those checks cannot establish behavior after integration. The stale/conflicted submitted branch was not treated as a standalone blocker; the missing evidence is a coherent current-main integration candidate. A maintainer-resolved replay preserving both current-main changes and the PR's profile-scoping changes, followed by executable focused tests, is the exact missing setup evidence. Security evidence:
Uncertainty: How maintainers intend to combine current main's tool_error and registry import changes with the PR's cache-scope imports is unknown.; Whether the profile-scoped credential and cache tests pass after a coherent conflict resolution is unknown.; Whether additional current-main call paths bypass the proposed active-profile scope cannot be determined from the stale PR-head tree.; Runtime behavior was not exercised because an offline pytest-capable environment was unavailable. Signed: GPT-5.6-sol-xhigh in Codex |
d8a73a7 to
0b36168
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for closing the verified primary-handler, credential, and availability-cache isolation gaps.
Problems
tools/browser_camofox.py:194and:199still readCAMOFOX_USER_IDandCAMOFOX_SESSION_KEYdirectly fromos.environ._get_session()consumes them at:368-375, and_ensure_tab()sends them asuserIdandlistItemIdat:405-412. The PR scopes the endpoint and bearer key, but a multiplexed secondary profile can still select a process-global externally managed Camofox browser identity or tab.
Suggested changes
- Route those identity values through the active secret scope (with the existing profile-local config fallback) and add a scoped-profile regression covering both precedence and fail-closed behavior.
Automated hermes-sweeper review.
| @@ -82,15 +83,15 @@ def _get_command_timeout() -> int: | |||
|
|
|||
| def _auth_headers() -> Dict[str, str]: | |||
| """Return Authorization header when CAMOFOX_API_KEY is set.""" | |||
| key = os.getenv("CAMOFOX_API_KEY", "").strip() | |||
| key = (get_secret("CAMOFOX_API_KEY", "") or "").strip() | |||
| if key: | |||
| return {"Authorization": f"Bearer {key}"} | |||
| return {} | |||
|
|
|||
|
|
|||
| def get_camofox_url() -> str: | |||
| """Return the configured Camofox server URL, or empty string.""" | |||
There was a problem hiding this comment.
This scopes the endpoint and bearer key, but _camofox_identity_override() still reads CAMOFOX_USER_ID and CAMOFOX_SESSION_KEY from raw process environment at current lines 194 and 199. Those values are sent as userId and listItemId when creating/adopting tabs, so they need the same active-profile resolution to prevent a secondary profile from reusing another profile's externally managed browser session.
0b36168 to
47d2938
Compare
|
suggesting changes Two in-scope defects remain in the profile-isolation mechanism:
Security evidence:
Uncertainty: No live Discord, Home Assistant, or Camofox service was exercised; the blocking results use deterministic in-process source and cache probes. Signed: GPT-5.6-sol-xhigh in Codex |
|
Salvaged and merged in #76573 — both your commits are on main (76cf19f + 3d9a146) with your authorship. Review verdict: every claimed unscoped read verified real on main (Discord/HASS/Camofox + the profile-blind check_fn/tool-defs caches), the get_secret routing is canonical with no parallel machinery, the (fn, profile_home) cache keying with fail-closed bypass is exactly right, and the child-inheritance check passed — cache scope rides the same ContextVar copy_context propagates to subagents. The camofox _sessions/_vnc_url process-global caches you didn't touch are tracked in #76574. Thanks @tachyon-r! |
Summary
Problem
Several model-callable integrations still read process-global credentials in multiplex gateways. A secondary profile could therefore authenticate as the launch/default profile's Discord bot, Home Assistant instance, or Camofox account.
After making those checks profile-dependent, two process-global availability caches could also advertise profile A's tools to profile B for their TTL/grace windows. Unscoped multiplex requests must not share fallback state, and profile-dimensional caches must remain bounded.
The primary adapter also rendered its prompt before entering profile scope, and its reconnect path installed an entirely unscoped handler. That made fail-closed credential reads incorrectly hide tools from the credentialed primary profile.
Validation
git diff --checkpassedCompatibility
Single-profile environment fallback is unchanged. Cache bypass occurs only when multiplex mode is active without a resolvable explicit profile scope; normal profile cache entries remain TTL-bound and are capped at 512.