Studio: persistent per-user trust_remote_code approval cache - #6551
Conversation
The consent gate pins each approval to a content fingerprint (sha256 over every repo .py), but nothing was persisted, so the dialog reappeared on every fresh load of the same unchanged repo. This adds an on-disk, per-user approval cache that lets the gate skip the dialog when the same user reloads the same code, while keeping the safety guarantees intact. Two-tier validation, both must hold or the user is re-prompted: - Commit SHA (cheap, one HfApi.model_info().sha, no download): a match means a byte-identical tree to the approved revision, so the scan/download is skipped. - Content fingerprint (authoritative): used whenever the SHA is unavailable (local path / offline) and always recomputed on a SHA miss. A new or edited .py changes both the SHA and the fingerprint, so it is caught in every mode. Safety: - Keyed per subject; one user's approval never auto-runs code for another. - CRITICAL is never stored or honored (guarded on both write and read), so a hand-edited store cannot smuggle in an auto-approval. - The malware (HF unsafe-file) gate stays unconditional. - Fail-safe: a corrupt store, an unresolvable SHA, or any error degrades to "ask again", never to "auto-approve". UNSLOTH_TRC_APPROVAL_CACHE_DISABLE=1 turns the cache off entirely. New module utils/security/remote_code_approvals.py holds the store (studio_root()/security/remote_code_approvals.json, atomic write, 0600, RLock) plus the SHA resolvers. Recording happens at the single gate chokepoint when the caller supplies the matching fingerprint, so subject is just threaded through inference/training/export (orchestrators, routes, workers). The scan endpoint returns already_approved so the frontend can skip the dialog on a cache hit. Tests: new tests/test_trc_approval_cache.py covers cache miss, SHA-match skip, SHA-moved re-scan, new-file re-consent, CRITICAL never cached (write + forged read), disable flag, subject isolation, combined adapter+base key, corrupt store, and no-subject bypass. Full security suite: 101 passed.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a persistent, per-user trust_remote_code approval cache to prevent redundant consent prompts when loading unchanged repositories. It propagates the user's identity (subject) through the backend's export, inference, and training pipelines, allowing the frontend to skip the consent dialog if a matching commit SHA and content fingerprint have already been approved. Feedback on the changes highlights a potential race condition in the cache storage mechanism, where the use of a thread-level RLock is insufficient for multi-process deployments, and recommends implementing a cross-process file lock instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| logger = get_logger(__name__) | ||
|
|
||
| _SCHEMA_VERSION = 1 | ||
| _lock = threading.RLock() |
There was a problem hiding this comment.
The _lock is currently a threading.RLock, which only synchronizes threads within the same OS process. Since FastAPI/Uvicorn applications are typically deployed with multiple worker processes (e.g., via Gunicorn or Uvicorn's --workers flag), concurrent requests handled by different worker processes will not be synchronized by this lock. This can lead to a race condition (lost updates) where one process overwrites the other's approvals during the read-modify-write cycle in record or forget.
To prevent this, consider using a cross-process file lock (e.g., using fcntl.flock on Unix systems) on a dedicated lock file (like remote_code_approvals.json.lock). Note that locking the JSON file itself is not sufficient because _save uses atomic replacement (os.replace), which changes the file's inode and breaks locks held on the old file descriptor.
![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: d213a19e0e
ℹ️ 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".
| if cache_key in _sha_cache: | ||
| return _sha_cache[cache_key] |
There was a problem hiding this comment.
When a Hub repo is approved, record() has already populated _sha_cache with that repo's current SHA. On later loads in the same backend process, this branch returns the cached SHA without contacting the Hub, so if the default branch moves from sha1 to sha2, evaluate_remote_code_consent_for_targets() still sees sha1 == stored.commit_sha and takes the no-scan cache path. That can run newly updated remote code without the re-consent this cache is meant to enforce; please avoid indefinite memoization, or add invalidation/TTL, for mutable refs.
Useful? React with 👍 / 👎.
| for target in dict.fromkeys(targets): | ||
| if not target: | ||
| continue | ||
| sha = resolve_commit_sha(target, hf_token) | ||
| if sha is None: |
There was a problem hiding this comment.
For models whose auto_map points to an external repo such as owner/other--module.Class, repo_remote_code_files() includes that external repo's Python in the fingerprint, but this SHA fast path only resolves the primary adapter/base targets. If the external repo changes while the model/base commits stay the same, _sha == _stored.commit_sha still auto-approves without recomputing the fingerprint, so changed third-party code skips the dialog. Include the external repo revisions in the combined SHA or disable the SHA shortcut when external refs are present.
Useful? React with 👍 / 👎.
| if not subject or cache_disabled(): | ||
| return None |
There was a problem hiding this comment.
In the hand-edited-store case called out by this change, this guard only rejects entries whose stored max_severity is literally CRITICAL. Because the approval file is editable JSON, changing or omitting that field while keeping the public combined SHA lets lookup() return the entry and the SHA fast path auto-approve without scanning, so a CRITICAL repo is no longer hard-blocked. Treat untrusted/invalid severity as non-cacheable or otherwise verify the entry before allowing the no-scan path.
Useful? React with 👍 / 👎.
| _sha = remote_code_approvals.resolve_combined_sha(targets, hf_token) | ||
| if _sha is not None and _sha == _stored.commit_sha: | ||
| logger.info("trust_remote_code approved from cache (sha match) for '%s'", primary) | ||
| return _auto_approved_decision(primary, _stored) | ||
| approved_fingerprint = approved_fingerprint or _stored.fingerprint |
There was a problem hiding this comment.
When the scanner rules change after a user approved unchanged content, this cache still either returns before scanning on a SHA match or seeds the old fingerprint into the current scan. In that upgrade scenario, code that is newly classified as HIGH/MEDIUM is auto-approved without showing the new finding, and a SHA hit can even bypass a newly-CRITICAL finding. Store a scanner/rules version with the approval or force a fresh prompt when the scan policy changes.
Useful? React with 👍 / 👎.
…he scan Codex found that the SHA "no-scan" fast path could run untrusted code without re-consent. Removed it; the gate now always re-scans and the cache only seeds the authoritative fingerprint check, so it can skip the dialog but never the scan. - CRITICAL is hard-blocked on every load (the scan always runs), so a hand-edited store that downgrades a CRITICAL repo's severity can no longer auto-run it (P2: do not trust editable severity for SHA approvals). - The fingerprint covers external auto_map repos, so changed third-party code always re-prompts even when the primary commit SHA is unchanged; there is no longer a SHA path that bypasses the fingerprint (P1: external auto_map repos). - resolve_commit_sha is resolved fresh on every call (no memoization), so a repo whose default branch moves after approval re-prompts instead of reusing a stale cached SHA (P1: revalidate mutable Hub SHAs). The SHA is now only a conservative secondary gate: a fresh resolvable SHA must match the approved revision, else the seed is withheld; a None (local/offline) falls back to the fingerprint. - Approvals record the scanner ruleset version (SCAN_RULES_VERSION); the gate ignores approvals from an older ruleset so reclassified bytes are re-scanned and re-shown instead of silently auto-approved (P2: invalidate on scan-policy change). Tests: test_trc_approval_cache.py rewritten around the prompt-skip semantics (unchanged repo still scans; SHA move / changed code / scanner-version bump / disable flag all re-prompt; forged downgraded severity still blocks CRITICAL). 105 passed with test_consent_gate.py.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Addressed the review feedback. The core change: the cache now skips only the dialog, never the scan, so every load re-scans and the cache cannot run untrusted code.
Tests rewritten around the prompt-skip semantics; 105 passed with the consent gate suite. |
|
@codex review |
for more information, see https://pre-commit.ci
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d9dbde7f0
ℹ️ 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".
| "resume_from_checkpoint": kwargs.get("resume_from_checkpoint"), | ||
| "trust_remote_code": kwargs.get("trust_remote_code", False), | ||
| "approved_remote_code_fingerprint": kwargs.get("approved_remote_code_fingerprint"), | ||
| "subject": kwargs.get("subject"), |
There was a problem hiding this comment.
Adding subject to the training config means _sanitize_db_config() will persist it into config_json because only tokens and s3_config are filtered, and GET /training-history/runs/{run_id} returns that config verbatim while only requiring authentication. In multi-user Studio installs, any authenticated user who can view a run detail now sees the username/API-key owner that started the run; keep subject as worker-only metadata or filter it before saving.
Useful? React with 👍 / 👎.
| if not subject or not fingerprint or cache_disabled() or max_severity == CRITICAL: | ||
| return | ||
| with _lock: | ||
| data = _load() |
There was a problem hiding this comment.
Because inference/export/training approvals are recorded from separate worker subprocesses, this RLock only protects threads inside one process. If two loads approve different repos or users concurrently, each process can read the same old JSON here and then _save() its own copy, so the later os.replace() drops the earlier approval and that user is prompted again; use an inter-process file lock or merge-on-write before replacing the store.
Useful? React with 👍 / 👎.
…ites Threading subject (the run owner's username / API-key id) into the training config meant _sanitize_db_config persisted it into config_json, which training-history GET returns to any authenticated user, leaking who started a run in multi-user installs. Filter subject alongside the token fields; the worker still receives it from the live config. The approval store's RLock only guards one process, but approvals are recorded from separate inference/export/training subprocesses, so concurrent writers could clobber each other on os.replace and drop an approval (re-prompt). Hold a best-effort cross-process file lock around the read-modify-write.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d7cb32c8
ℹ️ 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".
| if isinstance(data, dict) and data.get("version") == _SCHEMA_VERSION: | ||
| return data |
There was a problem hiding this comment.
If the approval file is valid JSON with the current version but has a malformed shape such as {"version": 1, "subjects": []}, _load() returns it as usable data; the next lookup() chains .get(subject, {}) on that list and raises instead of ignoring the cache. In that corrupted/hand-edited-store scenario, remote-code scan/load requests fail until the file is manually removed, even though this cache is intended to fail safe by re-prompting.
Useful? React with 👍 / 👎.
A store with the right version but a non-dict shape (e.g. a hand-edited "subjects": []) passed _load()'s check, then lookup chained .get() on a list and raised, breaking every remote-code load until the file was removed. Validate that subjects is a dict in _load(), and tolerate a non-dict per-subject entry in lookup/record/forget, so a corrupt store fails safe (re-prompt) instead.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 095a617234
ℹ️ 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".
| "resume_from_checkpoint": kwargs.get("resume_from_checkpoint"), | ||
| "trust_remote_code": kwargs.get("trust_remote_code", False), | ||
| "approved_remote_code_fingerprint": kwargs.get("approved_remote_code_fingerprint"), | ||
| "subject": kwargs.get("subject"), |
There was a problem hiding this comment.
When MLX training runs with enable_wandb enabled, _run_mlx_training initializes W&B with the whole config minus only hf_token, wandb_token, and s3_config (worker.py around 1920-1923). Adding subject here means the authenticated username/API subject is uploaded to W&B as run config, even though this patch only strips it from DB history; please either avoid putting it in the training config sent to the worker or add it to the W&B-sensitive filter.
Useful? React with 👍 / 👎.
_run_mlx_training uploads the whole training config to W&B minus a sensitive set that only listed hf_token/wandb_token/s3_config, so the authenticated subject (username / API-key id) was sent to W&B as run config even though DB history already strips it. Add subject to the W&B-sensitive filter, mirroring training._sanitize_db_config.
|
Good catch. Added subject to the MLX W&B-sensitive filter in _run_mlx_training so it is excluded from the uploaded run config, matching the DB-history strip in _sanitize_db_config. @codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # studio/frontend/src/features/security/api/remote-code-api.ts # studio/frontend/src/features/security/hooks/use-remote-code-consent.ts # studio/frontend/src/features/security/types.ts
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # studio/backend/core/inference/worker.py
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
The Studio consent gate pins each
trust_remote_codeapproval to a content fingerprint (sha256 over every repo.py), but nothing was persisted, so the dialog reappeared on every fresh load of the same unchanged repo. This adds an on-disk, per-user approval cache that lets the gate skip the dialog when the same user reloads the same code, while keeping the safety guarantees intact.This is the consent-cache half of the work motivated by #6541 (the tier-selection half is in the sibling PR #6550); the two subsystems are independent.
Two-tier validation
Both must hold, or the user is re-prompted:
HfApi.model_info().sha, no download): a match means a byte-identical tree to the approved revision, so the scan/download is skipped entirely..pychanges both the SHA and the fingerprint, so it is caught in every mode.Safety
UNSLOTH_TRC_APPROVAL_CACHE_DISABLE=1turns the cache off entirely.Changes
utils/security/remote_code_approvals.py: the store (studio_root()/security/remote_code_approvals.json, atomic write,0600,RLock) plus the commit-SHA resolvers (resolve_commit_sha/resolve_combined_sha, token-keyed memo, local/offline -> None).utils/security/consent.py: asubject-gated fast path after thetrust_remote_codecheck (SHA match -> auto-approve; otherwise seed the stored fingerprint so the existing content check auto-approves an unchanged repo and re-prompts a changed one), and recording at that single chokepoint when the caller supplies the matching fingerprint.subjectthreaded through inference / training / export (orchestrators, routes, workers); recording stays centralized in the gate.routes/models.py) returnsalready_approvedso the frontend can skip the dialog on a cache hit; malware gate stays unconditional.already_approvedplumbed throughtypes.ts/remote-code-api.ts, with a skip-dialog branch inuse-remote-code-consent.ts.Tests
New
tests/test_trc_approval_cache.pycovers cache miss, SHA-match skip (no re-download), SHA-moved re-scan, new-.pyre-consent, CRITICAL never cached (write guard + forged-store read guard), disable flag, subject isolation, combined adapter+base key, corrupt store, and no-subject bypass.Frontend
npx tsc --noEmitclean.