feat(tasks): per-actor sandbox GitHub identity on multiplayer transitions#71941
feat(tasks): per-actor sandbox GitHub identity on multiplayer transitions#71941VojtechBartos wants to merge 10 commits into
Conversation
f385546 to
ceda299
Compare
8b6e210 to
92badb0
Compare
06a7d5a to
a14762d
Compare
92badb0 to
feec41d
Compare
a14762d to
7394f7f
Compare
54a3240 to
02268db
Compare
c2fbfe4 to
a397d6f
Compare
454d497 to
0294158
Compare
a397d6f to
a5c899a
Compare
a5c899a to
0675986
Compare
e1ae71b to
6dcb4e9
Compare
VojtechBartos
changed the title
c9b507c to
4a7fbb2
Compare
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| continue | ||
| rows.append((str(run.id), sandbox_id, run.task.repository)) | ||
| return rows | ||
|
|
There was a problem hiding this comment.
Originally flagged line 204; mapped to the nearest line in the diff (194).
The marker is checked while building the propagation list, but it is not checked again before the owner token is written. The credential-refresh activity runs concurrently with follow-up delivery: it can select a sandbox while it is still bound to the owner, then a different Slack actor is logged out/rebound and marked, and finally this stale iteration writes the owner's token after that transition. A user without repository access can time a follow-up with the owner's token rotation and have their prompt execute using the owner's GitHub identity.
Prompt To Fix With AI
Make the transition and owner-token propagation mutually exclusive per sandbox (for example, use a sandbox-scoped Redis lock shared by _refresh_sandbox_github and _propagate_user_token). While holding it, re-read the github-identity marker immediately before writing; if it no longer identifies task.created_by_id, do not apply the owner token. Treat lock/read failures as no propagation.Severity: high | Confidence: 91% | React with 👍 if useful or 👎 if not
|
|
||
| if token: | ||
| try: | ||
| apply_github_credentials_to_sandbox(sandbox, repository, token) |
There was a problem hiding this comment.
apply_github_credentials_to_sandbox ignores the False results from both set_git_remote_token and update_sandbox_env_file, so it normally returns without raising even when the sandbox refused either write. This branch then records the new actor and delivers the prompt, while the prior actor's remote URL and/or live env token remain active. A follow-up actor with a less-privileged GitHub account can thus execute git or gh operations using the prior actor's credentials after a write failure.
Prompt To Fix With AI
Have apply_github_credentials_to_sandbox return success only if every applicable remote and env-file update succeeded (and treat exceptions as failure). In _refresh_sandbox_github, mark the actor and return True only on that confirmed result; otherwise clear both credential locations and fail the follow-up if the clear cannot be confirmed. Add tests for a nonzero remote update and a nonzero env-file write.Severity: high | Confidence: 92% | React with 👍 if useful or 👎 if not
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis pull request adds per-actor GitHub identities to task sandboxes when multiplayer users transition control of a task. Two issues have been addressed, but one significant identity-lifetime gap remains. Because actor binding expires before the sandbox does, a delayed GitHub command may run with the owner’s refreshed token rather than the active actor’s identity. The binding should persist for the full sandbox lifetime, or an unknown binding after a transition should block owner-token refreshes. Open issues (1)
Fixed/addressed: 2 · PR risk: 7/10 |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/temporal/process_task/tests/test_sandbox_credentials.py |
0.0% | 207, 211, 213–217, 219–220, 222–225, 257, 293, 334, 345–351, 353–354, 356, 358–363, 365–366, 368–369, 371, 373–374, 376–379, 381–382, 384–385, 387, 389–393, 395–397, 399, 404, 406, 411–412, 414–422, 424, 427–428, 650, 655–659, 661–662, 665, 667–671, 674, 677, 686–689, 691, 693 |
products/tasks/backend/temporal/process_task/tests/test_send_followup_to_sandbox.py |
0.0% | 8, 320, 358, 361–366, 371–373, 375–379, 381, 384–385, 387–390, 392, 395–399, 401–405, 407, 410–414, 416–419, 421, 424–429, 431–433, 435, 441–446, 448–451, 453, 459–461, 463–467, 469, 472–476, 478–479, 481, 484–488, 490–491, 493, 498, 500–504, 506–509 |
products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py |
9.2% | 228–230, 461–464, 468–471, 475–476, 498–499, 501–504, 506–508, 510–511, 517–518, 520–523, 531, 542, 550, 555–558, 560–566, 569–572, 579–589 |
products/tasks/backend/temporal/process_task/sandbox_credentials.py |
30.9% | 125, 127–128, 136–138, 165–166, 182, 190–193, 195–199, 209, 212, 224–226, 229–232, 236–237, 262, 282–284, 292, 294, 297, 321, 324, 326–327, 394–396, 400, 463, 481, 489 |
products/tasks/backend/temporal/process_task/utils.py |
41.7% | 399, 403, 407, 416, 422, 434, 440 |
products/tasks/backend/logic/services/modal_sandbox.py |
50.0% | 456–458 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 30081252023 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
tasks |
██████████████░░░░░░ 69.0% |
30,115 / 43,643 |
signals |
████████████████░░░░ 80.0% |
22,090 / 27,597 |
cdp |
████████████████░░░░ 81.0% |
3,146 / 3,883 |
data_modeling |
█████████████████░░░ 82.7% |
5,547 / 6,708 |
notebooks |
█████████████████░░░ 85.3% |
7,275 / 8,531 |
agent_platform |
█████████████████░░░ 86.4% |
3,807 / 4,405 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
██████████████████░░ 87.8% |
4,488 / 5,114 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
exports |
██████████████████░░ 88.4% |
6,951 / 7,863 |
data_warehouse |
██████████████████░░ 88.9% |
11,887 / 13,378 |
conversations |
██████████████████░░ 89.3% |
16,875 / 18,894 |
engineering_analytics |
██████████████████░░ 89.4% |
6,386 / 7,145 |
dashboards |
██████████████████░░ 89.4% |
5,983 / 6,693 |
error_tracking |
██████████████████░░ 89.7% |
10,168 / 11,336 |
alerts |
██████████████████░░ 90.0% |
4,056 / 4,508 |
early_access_features |
██████████████████░░ 90.1% |
1,031 / 1,144 |
mcp_analytics |
██████████████████░░ 90.1% |
2,763 / 3,065 |
streamlit_apps |
██████████████████░░ 90.4% |
2,501 / 2,767 |
slack_app |
██████████████████░░ 90.7% |
9,028 / 9,951 |
marketing_analytics |
██████████████████░░ 91.0% |
11,792 / 12,964 |
stamphog |
██████████████████░░ 91.1% |
4,056 / 4,450 |
product_analytics |
███████████████████░ 92.6% |
5,852 / 6,322 |
ai_observability |
███████████████████░ 92.8% |
15,041 / 16,214 |
surveys |
███████████████████░ 93.0% |
5,734 / 6,167 |
web_analytics |
███████████████████░ 93.1% |
14,426 / 15,492 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
workflows |
███████████████████░ 93.6% |
6,414 / 6,851 |
endpoints |
███████████████████░ 94.1% |
8,640 / 9,177 |
review_hog |
███████████████████░ 94.7% |
6,814 / 7,199 |
skills |
███████████████████░ 94.7% |
3,300 / 3,483 |
logs |
███████████████████░ 95.4% |
10,012 / 10,498 |
experiments |
███████████████████░ 95.7% |
25,158 / 26,282 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
replay_vision |
███████████████████░ 96.2% |
15,260 / 15,864 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.5% |
17,112 / 17,736 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
9,047 / 9,317 |
warehouse_sources |
███████████████████░ 97.1% |
330,460 / 340,224 |
data_catalog |
███████████████████░ 97.4% |
2,367 / 2,429 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 5 consider. Published 5 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 5 issues
Files (4)
products/tasks/backend/constants.pyproducts/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.pyproducts/tasks/backend/temporal/process_task/sandbox_credentials.pyproducts/tasks/backend/temporal/process_task/utils.py
| try: | ||
| sandbox = Sandbox.get_by_id(sandbox_id) | ||
| return sandbox if sandbox.is_running() else None | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Silent exception swallowing in _resolve_live_sandbox hides the root cause of fail-closed follow-up rejections
Why we think it's a valid issue
- Checked:
_resolve_live_sandbox(send_followup_to_sandbox.py:449-467), its sole caller_refresh_sandbox_github(501-509), the fail-closed raise at 224-226, and the module's logging conventions (grepped allexcept/logger.sites). - Found: line 466-467 is a fully silent
except Exception: return None— no log, no re-raise. TheNoneit returns feeds_refresh_sandbox_github, which returnsFalse(501-509) and makes the caller raiseRuntimeError(224-226), rejecting the follow-up. The consequential error paths in this same module all preserve cause —error=str(e)at line 366,exc_info=Trueat lines 198 and 540 (and_propagate_user_tokenin sandbox_credentials.py:211-216 likewise) — so discarding the exception here is a real inconsistency, not the module norm. The benign silent swallows at lines 105/120 are non-decision helpers (heartbeat loop, attempt default), which is why they don't log. - Found: the collapsed causes have different remediation, and the code's own comment at 503-507 names both ('dead sandbox, or a transient control-plane lookup failure'); the swallow at 466 destroys the only evidence (exception type/message/stack) that distinguishes them.
- Impact: a spike in fail-closed follow-up rejections is diagnosable only down to
refresh_github_no_sandbox_handle_fail_closed(line 508) with no root cause, forcing on-call guesswork between a benign dead sandbox and an infra incident. This is a genuine, if low-severity, observability defect on a security-relevant fail-closed path — the failure is already loud (RuntimeError + info log), so this improves root-cause diagnosis rather than un-hiding a silent failure, which fits the reviewer's 'consider' tier. Fix is a one-line log consistent with the module's existing convention.
Issue description
_resolve_live_sandbox's except Exception: return None discards the underlying error without any logging, unlike almost every other error path in this module (e.g. refresh_github_apply_failed, refresh_mcp_token_mint_failed) which logs with context. Since this return value directly determines whether an actor-transition follow-up is rejected (_refresh_sandbox_github fails closed with a RuntimeError when the handle can't be resolved), an on-call engineer investigating a spike in rejected follow-ups has no way to distinguish "sandbox genuinely terminated" from "a transient control-plane/network error while resolving the handle" — the two have very different remediation paths.
Suggested fix
Log the caught exception (e.g. logger.warning("resolve_live_sandbox_failed", sandbox_id=sandbox_id, exc_info=True)) before returning None, so fail-closed decisions triggered by this path are diagnosable from logs.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py#L463-467
<issue_description>
`_resolve_live_sandbox`'s `except Exception: return None` discards the underlying error without any logging, unlike almost every other error path in this module (e.g. `refresh_github_apply_failed`, `refresh_mcp_token_mint_failed`) which logs with context. Since this return value directly determines whether an actor-transition follow-up is rejected (`_refresh_sandbox_github` fails closed with a `RuntimeError` when the handle can't be resolved), an on-call engineer investigating a spike in rejected follow-ups has no way to distinguish "sandbox genuinely terminated" from "a transient control-plane/network error while resolving the handle" — the two have very different remediation paths.
</issue_description>
<issue_validation>
- **Checked:** `_resolve_live_sandbox` (send_followup_to_sandbox.py:449-467), its sole caller `_refresh_sandbox_github` (501-509), the fail-closed raise at 224-226, and the module's logging conventions (grepped all `except`/`logger.` sites).
- **Found:** line 466-467 is a fully silent `except Exception: return None` — no log, no re-raise. The `None` it returns feeds `_refresh_sandbox_github`, which returns `False` (501-509) and makes the caller raise `RuntimeError` (224-226), rejecting the follow-up. The consequential error paths in this same module all preserve cause — `error=str(e)` at line 366, `exc_info=True` at lines 198 and 540 (and `_propagate_user_token` in sandbox_credentials.py:211-216 likewise) — so discarding the exception here is a real inconsistency, not the module norm. The benign silent swallows at lines 105/120 are non-decision helpers (heartbeat loop, attempt default), which is why they don't log.
- **Found:** the collapsed causes have different remediation, and the code's own comment at 503-507 names both ('dead sandbox, or a transient control-plane lookup failure'); the swallow at 466 destroys the only evidence (exception type/message/stack) that distinguishes them.
- **Impact:** a spike in fail-closed follow-up rejections is diagnosable only down to `refresh_github_no_sandbox_handle_fail_closed` (line 508) with no root cause, forcing on-call guesswork between a benign dead sandbox and an infra incident. This is a genuine, if low-severity, observability defect on a security-relevant fail-closed path — the failure is already loud (RuntimeError + info log), so this improves root-cause diagnosis rather than un-hiding a silent failure, which fits the reviewer's 'consider' tier. Fix is a one-line log consistent with the module's existing convention.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Log the caught exception (e.g. `logger.warning("resolve_live_sandbox_failed", sandbox_id=sandbox_id, exc_info=True)`) before returning `None`, so fail-closed decisions triggered by this path are diagnosable from logs.
</potential_solution>
| if get_sandbox_github_identity_user(scope) == actor_user.id: | ||
| return True # sandbox already reflects this actor — cheapest check first |
There was a problem hiding this comment.
Cached github-identity marker prevents re-checking access for the same actor once logged out
Why we think it's a valid issue
- Checked: the fast path and both marking sites in
_refresh_sandbox_github(send_followup_to_sandbox.py:494-495, 544, 552-554), the marker helper docstring and TTL (utils.py:361, 401-410), and the scheduled refresh's actor-transition guard (sandbox_credentials.py:315-323). - Found:
mark_sandbox_github_identity(scope, actor_user.id)is written identically for rebind (544) and logout (553), and utils.py:401-410 confirms the conflation is intentional — so the 494-495 fast path (marker == actor_user.id → return True, no token re-check) short-circuits a logged-out actor exactly like a rebound one. The marker TTL is 3h (TOKEN_EXPIRATION_SECONDS / 2, utils.py:361), and the scheduled refresh skips any non-owner bound actor (315-323), so within that window nothing re-evaluates a logged-out non-owner's access. - Impact: a previously-denied actor who is added to the repo mid-thread keeps hitting the fast path and runs follow-ups with no git creds (git/gh silently fail) until the marker expires — up to ~3h — in the exact multiplayer scenario this PR targets. This is a real, concretely-triggered mishandled edge case, not speculation, which clears the bar for the lowest tier.
- Impact (why only 'consider'): the direction is fail-closed (the actor is denied longer, never wrongly granted) and self-healing after ≤3h; the inverse, security-relevant direction (a de-authorized actor keeping access) is separately blocked by GitHub-side authz and by there being no retained token after a logout. So it is a minor UX-latency defect, correctly rated 'consider'.
Issue description
mark_sandbox_github_identity(scope, actor_user.id) is called with the same actor id whether they were rebound (have access) or logged out (denied access) — by design, per the docstring, so the 'same actor, no change' fast path at line 494 short-circuits both cases identically. This means once an actor is logged out for lacking repo access, every subsequent follow-up from that same actor within the marker's freshness window (MCP_TOKEN_REFRESH_INTERVAL_SECONDS, 3h) takes the 'already reflects this actor' fast path and never re-attempts get_sandbox_github_token — even if that actor's GitHub access changes (e.g. they get added to the repo) during the window. The scheduled GitHubSandboxCredential.refresh() loop doesn't fill this gap either: its new actor-transition guard (sandbox_credentials.py lines 310-323) compares the marker against task.created_by_id, so for a non-owner actor it keeps skipping for as long as the marker persists, regardless of the actor's current access.
Suggested fix
If this staleness window is acceptable, consider noting it explicitly in the docstring; otherwise shorten the TTL for a 'logged out' marking specifically (vs. a confirmed rebind), or have the scheduled refresh re-evaluate access for the current actor (not just skip based on task.created_by_id) so a newly-authorized actor doesn't have to wait out the full window.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py#L494-495
@products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py#L552-554
<issue_description>
mark_sandbox_github_identity(scope, actor_user.id) is called with the same actor id whether they were rebound (have access) or logged out (denied access) — by design, per the docstring, so the 'same actor, no change' fast path at line 494 short-circuits both cases identically. This means once an actor is logged out for lacking repo access, every subsequent follow-up from that same actor within the marker's freshness window (MCP_TOKEN_REFRESH_INTERVAL_SECONDS, 3h) takes the 'already reflects this actor' fast path and never re-attempts get_sandbox_github_token — even if that actor's GitHub access changes (e.g. they get added to the repo) during the window. The scheduled GitHubSandboxCredential.refresh() loop doesn't fill this gap either: its new actor-transition guard (sandbox_credentials.py lines 310-323) compares the marker against task.created_by_id, so for a non-owner actor it keeps skipping for as long as the marker persists, regardless of the actor's current access.
</issue_description>
<issue_validation>
- **Checked:** the fast path and both marking sites in `_refresh_sandbox_github` (send_followup_to_sandbox.py:494-495, 544, 552-554), the marker helper docstring and TTL (utils.py:361, 401-410), and the scheduled refresh's actor-transition guard (sandbox_credentials.py:315-323).
- **Found:** `mark_sandbox_github_identity(scope, actor_user.id)` is written identically for rebind (544) and logout (553), and utils.py:401-410 confirms the conflation is intentional — so the 494-495 fast path (`marker == actor_user.id → return True`, no token re-check) short-circuits a logged-out actor exactly like a rebound one. The marker TTL is 3h (`TOKEN_EXPIRATION_SECONDS / 2`, utils.py:361), and the scheduled refresh skips any non-owner bound actor (315-323), so within that window nothing re-evaluates a logged-out non-owner's access.
- **Impact:** a previously-denied actor who is added to the repo mid-thread keeps hitting the fast path and runs follow-ups with no git creds (git/gh silently fail) until the marker expires — up to ~3h — in the exact multiplayer scenario this PR targets. This is a real, concretely-triggered mishandled edge case, not speculation, which clears the bar for the lowest tier.
- **Impact (why only 'consider'):** the direction is fail-closed (the actor is denied longer, never wrongly granted) and self-healing after ≤3h; the inverse, security-relevant direction (a de-authorized actor keeping access) is separately blocked by GitHub-side authz and by there being no retained token after a logout. So it is a minor UX-latency defect, correctly rated 'consider'.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
If this staleness window is acceptable, consider noting it explicitly in the docstring; otherwise shorten the TTL for a 'logged out' marking specifically (vs. a confirmed rebind), or have the scheduled refresh re-evaluate access for the current actor (not just skip based on task.created_by_id) so a newly-authorized actor doesn't have to wait out the full window.
</potential_solution>
| if token: | ||
| applied = False | ||
| try: | ||
| applied = apply_github_credentials_to_sandbox(sandbox, repository, token) | ||
| except Exception: | ||
| logger.warning("refresh_github_apply_failed", run_id=run_id, exc_info=True) | ||
| if applied: | ||
| # Record the new actor only on a fully-confirmed rebind. A partial write leaves one | ||
| # credential location on the prior actor's token, so fall through to logout instead. | ||
| mark_sandbox_github_identity(scope, actor_user.id) | ||
| logger.info("refresh_github_rebound", run_id=run_id, user_id=actor_user.id) | ||
| return True | ||
| logger.warning("refresh_github_apply_incomplete", run_id=run_id, user_id=actor_user.id) | ||
|
|
||
| # No usable rebind (no token, or the rebind write could not be confirmed): log the sandbox | ||
| # out. Fail closed only if even the clear can't be confirmed — the previous actor's | ||
| # credentials might still be live. | ||
| if clear_github_credentials_from_sandbox(sandbox, repository): | ||
| mark_sandbox_github_identity(scope, actor_user.id) | ||
| logger.info("refresh_github_logged_out", run_id=run_id, user_id=actor_user.id) | ||
| return True | ||
| logger.warning("refresh_github_logout_failed", run_id=run_id, user_id=actor_user.id) | ||
| return False |
There was a problem hiding this comment.
Logout path can raise an unhandled sandbox exception, unlike the guarded rebind path right above it
Why we think it's a valid issue
- Checked:
_refresh_sandbox_githubrebind vs logout branches (send_followup_to_sandbox.py:535-557), the credential-clear chain (sandbox_credentials.py:57-77, 80-122, 143-149),sandbox.executesemantics (modal_sandbox.py:746-796), the caller's fail-closed raise (224-226), and the activity wrapper (84-114) plus post-delivery error handling (278-319). - Found: the asymmetry is real —
execute()raisesSandboxNotRunningError/SandboxTimeoutError/SandboxExecutionError(modal 751-756, 779-796), the rebind at 538 catches it (539-540) but the logoutclear_github_credentials_from_sandboxat 552 is unguarded and reachessandbox.execute/write_file(sandbox_credentials.py:67, 89, 115), so a stop/timeout race throws out of the function. - Impact / why not should_fix: both failure modes fail closed identically —
_refresh_sandbox_githubruns at line 224 beforesend_user_message(239), so neither delivers the follow-up; no security or correctness difference. The finding's rationale is overstated: the thrown errors are classified custom exceptions that are logged (logger.errormodal 789) or Sentry-captured (780) and recorded by Temporal as typed activity failures — not 'raw, unclassified' or 'not surfaced'. The 'no _write_error_and_complete' claim is not a differentiator either, since the controlled RuntimeError path (224-226) also skips it (that helper only runs post-delivery at 307/317). - Impact (what actually remains): the only genuine delta is observability/consistency — the run-scoped
refresh_github_logout_failedwarning isn't emitted on the exception sub-case, and the error type differs from the sibling branch. Wrapping the logout call to mirror line 538-540 is a sensible small improvement on the fail-safe path, but real-but-minor. - Priority: lowering should_fix → consider: functional and fail-closed behavior is identical between the two paths and the sandbox exceptions are already classified and logged/captured, so this is an error-handling-consistency/observability nit, not a reliability defect that must be fixed.
Issue description
In _refresh_sandbox_github, the rebind call apply_github_credentials_to_sandbox(sandbox, repository, token) (line 538) is wrapped in try/except Exception, but the logout/clear call two branches down, clear_github_credentials_from_sandbox(sandbox, repository) (line 552), is called unguarded. Both functions ultimately call sandbox.execute(...), which (see products/tasks/backend/logic/services/modal_sandbox.py:746-796) can raise SandboxNotRunningError, SandboxTimeoutError, or SandboxExecutionError rather than just returning a non-zero exit code. A very plausible race — the sandbox stopping/crashing in the window between _resolve_live_sandbox's is_running() check and this call, or simply timing out — will throw one of these from clear_github_credentials_from_sandbox, propagating as a raw, unclassified exception straight out of _refresh_sandbox_github and _deliver_followup, bypassing the function's own controlled False return (and the RuntimeError("Could not rebind or clear...") the caller raises on that path). This is exactly the fail-safe/logout branch the whole feature depends on, so an ungraceful crash here is worse for operability than a controlled failure: no consistent log signal, no classified error surfaced to the workflow, and no _write_error_and_complete call before the activity dies.
Suggested fix
Wrap clear_github_credentials_from_sandbox(sandbox, repository) in the same try/except Exception pattern used for the rebind call above it, logging (e.g. refresh_github_logout_exception) and falling through to the existing return False fail-closed path on failure, so both credential-mutation calls in this function handle sandbox execution errors identically.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py#L535-557
<issue_description>
In `_refresh_sandbox_github`, the rebind call `apply_github_credentials_to_sandbox(sandbox, repository, token)` (line 538) is wrapped in `try/except Exception`, but the logout/clear call two branches down, `clear_github_credentials_from_sandbox(sandbox, repository)` (line 552), is called unguarded. Both functions ultimately call `sandbox.execute(...)`, which (see `products/tasks/backend/logic/services/modal_sandbox.py:746-796`) can raise `SandboxNotRunningError`, `SandboxTimeoutError`, or `SandboxExecutionError` rather than just returning a non-zero exit code. A very plausible race — the sandbox stopping/crashing in the window between `_resolve_live_sandbox`'s `is_running()` check and this call, or simply timing out — will throw one of these from `clear_github_credentials_from_sandbox`, propagating as a raw, unclassified exception straight out of `_refresh_sandbox_github` and `_deliver_followup`, bypassing the function's own controlled `False` return (and the `RuntimeError("Could not rebind or clear...")` the caller raises on that path). This is exactly the fail-safe/logout branch the whole feature depends on, so an ungraceful crash here is worse for operability than a controlled failure: no consistent log signal, no classified error surfaced to the workflow, and no `_write_error_and_complete` call before the activity dies.
</issue_description>
<issue_validation>
- **Checked:** `_refresh_sandbox_github` rebind vs logout branches (send_followup_to_sandbox.py:535-557), the credential-clear chain (sandbox_credentials.py:57-77, 80-122, 143-149), `sandbox.execute` semantics (modal_sandbox.py:746-796), the caller's fail-closed raise (224-226), and the activity wrapper (84-114) plus post-delivery error handling (278-319).
- **Found:** the asymmetry is real — `execute()` raises `SandboxNotRunningError`/`SandboxTimeoutError`/`SandboxExecutionError` (modal 751-756, 779-796), the rebind at 538 catches it (539-540) but the logout `clear_github_credentials_from_sandbox` at 552 is unguarded and reaches `sandbox.execute`/`write_file` (sandbox_credentials.py:67, 89, 115), so a stop/timeout race throws out of the function.
- **Impact / why not should_fix:** both failure modes fail closed identically — `_refresh_sandbox_github` runs at line 224 before `send_user_message` (239), so neither delivers the follow-up; no security or correctness difference. The finding's rationale is overstated: the thrown errors are classified custom exceptions that are logged (`logger.error` modal 789) or Sentry-captured (780) and recorded by Temporal as typed activity failures — not 'raw, unclassified' or 'not surfaced'. The 'no _write_error_and_complete' claim is not a differentiator either, since the controlled RuntimeError path (224-226) also skips it (that helper only runs post-delivery at 307/317).
- **Impact (what actually remains):** the only genuine delta is observability/consistency — the run-scoped `refresh_github_logout_failed` warning isn't emitted on the exception sub-case, and the error type differs from the sibling branch. Wrapping the logout call to mirror line 538-540 is a sensible small improvement on the fail-safe path, but real-but-minor.
- **Priority:** lowering should_fix → consider: functional and fail-closed behavior is identical between the two paths and the sandbox exceptions are already classified and logged/captured, so this is an error-handling-consistency/observability nit, not a reliability defect that must be fixed.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Wrap `clear_github_credentials_from_sandbox(sandbox, repository)` in the same `try/except Exception` pattern used for the rebind call above it, logging (e.g. `refresh_github_logout_exception`) and falling through to the existing `return False` fail-closed path on failure, so both credential-mutation calls in this function handle sandbox execution errors identically.
</potential_solution>
| if actor_user is None: | ||
| return True | ||
|
|
||
| run_id = str(task_run.id) | ||
| scope = sandbox_identity_scope(run_id, state) | ||
| if get_sandbox_github_identity_user(scope) == actor_user.id: | ||
| return True # sandbox already reflects this actor — cheapest check first | ||
|
|
||
| task = task_run.task | ||
| if get_pr_authorship_mode(task, state) != PrAuthorshipMode.USER: | ||
| return True |
There was a problem hiding this comment.
GitHub identity fast-path never actually short-circuits on the common case, forcing the full sandbox round trip on every run's first follow-up and every ~3h afterward
Why we think it's a valid issue
- Checked: every
mark_sandbox_github_identity/mark_sandbox_mcp_sessioncall site (repo-wide grep), the GitHub fast path (send_followup_to_sandbox.py:489-499), the boot seeding in start_agent_server.py, the fall-through cost (_resolve_live_sandbox449-467,get_sandbox_github_token,apply_github_credentials_to_sandbox→ sandbox_credentials.py:57-77/80-122/125-134), and the TTL (utils.py:361). - Found: the sibling MCP gate seeds its marker at boot —
start_agent_server.py:349-350,mark_sandbox_mcp_session(sandbox.id, params.actor_user_id), with the comment 'Record the boot identity so same-actor follow-ups ... skip the redundant refresh.' The GitHub marker is set only inside_refresh_sandbox_github(544, 553), never at provisioning. Soget_sandbox_github_identity_userisNoneon message Reports that users will need #1 of every USER run, the fast path at 494-495 misses, and it runs the full path (control-planeget_by_id+is_running, token resolution, and ~3 in-sandbox execs for git-remote + env-file rewrite) to re-apply the token already bound at launch. With the 3h TTL it recurs every ~3h for a single-actor conversation. Premise fully confirmed, including the MCP-vs-GitHub asymmetry the finding implies. - Impact: a real, avoidable, common-case redundancy that defeats the fast path's stated intent, fixable in one line mirroring line 350 — so it clears the keep bar and is not overengineering (it targets the 100% common path, not an edge case).
- Priority: lowering should_fix → consider: the redundant work is O(1) per follow-up (not N+1/unbounded/quadratic), on a low-frequency human-paced interactive path with per-run-isolated sandboxes and a cache-backed token lookup, and it is functionally correct (just redundant). The added latency is modest relative to the follow-up's own cost, making this a real-but-minor efficiency/consistency improvement rather than a scale-biting performance defect.
Issue description
_refresh_sandbox_github's comment at line 495 calls the identity-marker check the "cheapest check first," intended to skip the expensive sandbox work when the actor hasn't changed. But mark_sandbox_github_identity is only ever called from inside this same function (verified via repo-wide grep — it's never seeded during initial task/sandbox provisioning), so get_sandbox_github_identity_user(scope) returns None for the very first follow-up of literally every USER-authored run, even when the sender is the run's own creator who already has valid credentials from launch. None == actor_user.id is false, so the fast path never triggers on message #1, and the code falls through to _resolve_live_sandbox (a control-plane Sandbox.get_by_id + is_running() call), get_sandbox_github_token (a token resolution/refresh that can itself hit GitHub's API), and apply_github_credentials_to_sandbox (a git-remote rewrite plus an env-file read+write inside the sandbox) — all to redundantly re-apply the same token that's already correctly bound. Because the marker also shares the generic MCP_TOKEN_REFRESH_INTERVAL_SECONDS TTL (3 hours, products/tasks/backend/temporal/process_task/utils.py:361), the same full cold path re-fires every ~3 hours for any long-lived, single-actor conversation, not only at genuine actor transitions.
Suggested fix
Seed the github-identity marker at sandbox provisioning time (wherever the initial GitHub credentials are first applied to a run's sandbox), so the fast path actually matches the intended "no transition happened" case from message #1 onward, instead of paying the full round trip on every run's first follow-up and periodically thereafter purely due to TTL expiry.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py#L489-499
<issue_description>
`_refresh_sandbox_github`'s comment at line 495 calls the identity-marker check the "cheapest check first," intended to skip the expensive sandbox work when the actor hasn't changed. But `mark_sandbox_github_identity` is only ever called from inside this same function (verified via repo-wide grep — it's never seeded during initial task/sandbox provisioning), so `get_sandbox_github_identity_user(scope)` returns `None` for the very first follow-up of literally every USER-authored run, even when the sender is the run's own creator who already has valid credentials from launch. `None == actor_user.id` is false, so the fast path never triggers on message #1, and the code falls through to `_resolve_live_sandbox` (a control-plane `Sandbox.get_by_id` + `is_running()` call), `get_sandbox_github_token` (a token resolution/refresh that can itself hit GitHub's API), and `apply_github_credentials_to_sandbox` (a git-remote rewrite plus an env-file read+write inside the sandbox) — all to redundantly re-apply the same token that's already correctly bound. Because the marker also shares the generic `MCP_TOKEN_REFRESH_INTERVAL_SECONDS` TTL (3 hours, `products/tasks/backend/temporal/process_task/utils.py:361`), the same full cold path re-fires every ~3 hours for any long-lived, single-actor conversation, not only at genuine actor transitions.
</issue_description>
<issue_validation>
- **Checked:** every `mark_sandbox_github_identity` / `mark_sandbox_mcp_session` call site (repo-wide grep), the GitHub fast path (send_followup_to_sandbox.py:489-499), the boot seeding in start_agent_server.py, the fall-through cost (`_resolve_live_sandbox` 449-467, `get_sandbox_github_token`, `apply_github_credentials_to_sandbox` → sandbox_credentials.py:57-77/80-122/125-134), and the TTL (utils.py:361).
- **Found:** the sibling MCP gate seeds its marker at boot — `start_agent_server.py:349-350`, `mark_sandbox_mcp_session(sandbox.id, params.actor_user_id)`, with the comment 'Record the boot identity so same-actor follow-ups ... skip the redundant refresh.' The GitHub marker is set only inside `_refresh_sandbox_github` (544, 553), never at provisioning. So `get_sandbox_github_identity_user` is `None` on message #1 of every USER run, the fast path at 494-495 misses, and it runs the full path (control-plane `get_by_id`+`is_running`, token resolution, and ~3 in-sandbox execs for git-remote + env-file rewrite) to re-apply the token already bound at launch. With the 3h TTL it recurs every ~3h for a single-actor conversation. Premise fully confirmed, including the MCP-vs-GitHub asymmetry the finding implies.
- **Impact:** a real, avoidable, common-case redundancy that defeats the fast path's stated intent, fixable in one line mirroring line 350 — so it clears the keep bar and is not overengineering (it targets the 100% common path, not an edge case).
- **Priority:** lowering should_fix → consider: the redundant work is O(1) per follow-up (not N+1/unbounded/quadratic), on a low-frequency human-paced interactive path with per-run-isolated sandboxes and a cache-backed token lookup, and it is functionally correct (just redundant). The added latency is modest relative to the follow-up's own cost, making this a real-but-minor efficiency/consistency improvement rather than a scale-biting performance defect.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Seed the github-identity marker at sandbox provisioning time (wherever the initial GitHub credentials are first applied to a run's sandbox), so the fast path actually matches the intended "no transition happened" case from message #1 onward, instead of paying the full round trip on every run's first follow-up and periodically thereafter purely due to TTL expiry.
</potential_solution>
| try: | ||
| token = get_sandbox_github_token( | ||
| task.github_integration_id, | ||
| run_id=run_id, | ||
| state=state, | ||
| task=task, | ||
| actor_user=actor_user, | ||
| repository=repository, | ||
| ) | ||
| except ReauthorizationRequired as e: | ||
| # New actor has no usable GitHub access to this repo (missing/invalid integration, | ||
| # no app installation, or no repo permission), so we log the sandbox out rather than | ||
| # run under the prior actor's creds. Reauthorization is surfaced elsewhere. | ||
| logger.info( | ||
| "refresh_github_actor_reauthorization_required", | ||
| run_id=run_id, | ||
| user_id=actor_user.id, | ||
| repository=repository, | ||
| reason=str(e), | ||
| ) | ||
| token = None |
There was a problem hiding this comment.
Token-mint failures other than ReauthorizationRequired are unhandled in the new follow-up GitHub gate
Why we think it's a valid issue
- Checked: the except clause in
_refresh_sandbox_github(send_followup_to_sandbox.py:512-533), the full call chain ofget_sandbox_github_token(utils.py:913-1022) andget_github_token(utils.py:669-681), the sibling handlerGitHubSandboxCredential.refresh(sandbox_credentials.py:357-378), and every failure branch in_deliver_followup. - Found: the gate catches only
ReauthorizationRequired(522).get_github_tokenraisesIntegration.DoesNotExist(utils.py:670) andCredentialUnavailableError(674), and is reached fromget_sandbox_github_tokenin the USER path (984, 1010) and BOT path (1013), so both propagate uncaught here. The sibling refresh explicitly catches(Integration.DoesNotExist, UserIntegration.DoesNotExist)andReauthorizationRequired(367-378), confirming these are expected, handled-elsewhere failure modes. - Found (impact correction): the finding's 'unlike every other failure branch, which calls
_write_error_and_complete' is inaccurate — the adjacent MCP gate (218-219) and the GitHub gate's own controlledreturn False→RuntimeErrorpath (224-226) both raise without_write_error_and_complete. So the uncaught exception's SSE-stream behavior is identical to the gate's intentional fail-closed path, not a unique 'client hangs' regression. - Impact: the genuine defect is that these 'no usable token' exceptions bypass the gate's own graceful-degradation logic —
ReauthorizationRequiredis caught, logs out, and still delivers the follow-up (logged-out), whereasIntegration.DoesNotExist/CredentialUnavailableErrorcrash the activity. On a real-but-uncommon mid-run integration disconnect or GitHub-App suspension, the follow-up hard-fails (and retries) instead of degrading. Delivery is fail-closed either way (no wrong-actor push, no data loss), so it's a real reliability/consistency gap matching the 'missing handling for a failure mode that will occur' category, but bounded. - Priority: lowering should_fix → consider: the trigger is an uncommon mid-run integration/installation change, the consequence is a fail-closed hard-failed follow-up rather than a breach or data loss, and the reviewer's severity rests partly on the overstated 'unique SSE hang' impact that the gate's own controlled path shares.
Issue description
get_sandbox_github_token(...) is only guarded against ReauthorizationRequired here. But the same call chain can raise Integration.DoesNotExist (via get_github_token, utils.py:670, when the task's integration row was deleted mid-run) or CredentialUnavailableError (via installation_unavailable() in the same function) — both of which the periodic-refresh sibling GitHubSandboxCredential.refresh in sandbox_credentials.py (lines 367-378) explicitly catches and converts to a controlled CredentialUnavailableError. In this new per-follow-up gate, neither is caught, so they propagate as raw, unhandled exceptions out of _deliver_followup — unlike every other failure branch in this function (TaskRun.DoesNotExist, missing artifacts, Slack actor unavailable, retry-exhausted delivery), which explicitly calls _write_error_and_complete before raising. The SSE stream never receives a terminal event, so the client-side follow-up appears to hang instead of surfacing a clear, classified error.
Suggested fix
Catch (Integration.DoesNotExist, UserIntegration.DoesNotExist, CredentialUnavailableError) around the get_sandbox_github_token call in _refresh_sandbox_github, mirroring how GitHubSandboxCredential.refresh handles the same exceptions, and treat it the same way ReauthorizationRequired is treated here (log and fall through to logout) rather than letting it escape uncaught.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py#L513-533
<issue_description>
`get_sandbox_github_token(...)` is only guarded against `ReauthorizationRequired` here. But the same call chain can raise `Integration.DoesNotExist` (via `get_github_token`, `utils.py:670`, when the task's integration row was deleted mid-run) or `CredentialUnavailableError` (via `installation_unavailable()` in the same function) — both of which the periodic-refresh sibling `GitHubSandboxCredential.refresh` in `sandbox_credentials.py` (lines 367-378) explicitly catches and converts to a controlled `CredentialUnavailableError`. In this new per-follow-up gate, neither is caught, so they propagate as raw, unhandled exceptions out of `_deliver_followup` — unlike every other failure branch in this function (`TaskRun.DoesNotExist`, missing artifacts, Slack actor unavailable, retry-exhausted delivery), which explicitly calls `_write_error_and_complete` before raising. The SSE stream never receives a terminal event, so the client-side follow-up appears to hang instead of surfacing a clear, classified error.
</issue_description>
<issue_validation>
- **Checked:** the except clause in `_refresh_sandbox_github` (send_followup_to_sandbox.py:512-533), the full call chain of `get_sandbox_github_token` (utils.py:913-1022) and `get_github_token` (utils.py:669-681), the sibling handler `GitHubSandboxCredential.refresh` (sandbox_credentials.py:357-378), and every failure branch in `_deliver_followup`.
- **Found:** the gate catches only `ReauthorizationRequired` (522). `get_github_token` raises `Integration.DoesNotExist` (utils.py:670) and `CredentialUnavailableError` (674), and is reached from `get_sandbox_github_token` in the USER path (984, 1010) and BOT path (1013), so both propagate uncaught here. The sibling refresh explicitly catches `(Integration.DoesNotExist, UserIntegration.DoesNotExist)` and `ReauthorizationRequired` (367-378), confirming these are expected, handled-elsewhere failure modes.
- **Found (impact correction):** the finding's 'unlike every other failure branch, which calls `_write_error_and_complete`' is inaccurate — the adjacent MCP gate (218-219) and the GitHub gate's own controlled `return False`→`RuntimeError` path (224-226) both raise without `_write_error_and_complete`. So the uncaught exception's SSE-stream behavior is identical to the gate's intentional fail-closed path, not a unique 'client hangs' regression.
- **Impact:** the genuine defect is that these 'no usable token' exceptions bypass the gate's own graceful-degradation logic — `ReauthorizationRequired` is caught, logs out, and still delivers the follow-up (logged-out), whereas `Integration.DoesNotExist`/`CredentialUnavailableError` crash the activity. On a real-but-uncommon mid-run integration disconnect or GitHub-App suspension, the follow-up hard-fails (and retries) instead of degrading. Delivery is fail-closed either way (no wrong-actor push, no data loss), so it's a real reliability/consistency gap matching the 'missing handling for a failure mode that will occur' category, but bounded.
- **Priority:** lowering should_fix → consider: the trigger is an uncommon mid-run integration/installation change, the consequence is a fail-closed hard-failed follow-up rather than a breach or data loss, and the reviewer's severity rests partly on the overstated 'unique SSE hang' impact that the gate's own controlled path shares.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Catch `(Integration.DoesNotExist, UserIntegration.DoesNotExist, CredentialUnavailableError)` around the `get_sandbox_github_token` call in `_refresh_sandbox_github`, mirroring how `GitHubSandboxCredential.refresh` handles the same exceptions, and treat it the same way `ReauthorizationRequired` is treated here (log and fall through to logout) rather than letting it escape uncaught.
</potential_solution>
a556b58 to
7e5552b
Compare
| identity must treat a partial write as an unconfirmed rebind: leaving one location on the | ||
| previous actor's token would let a follow-up actor act as them. | ||
| """ | ||
| remote_applied = set_git_remote_token(sandbox, repository, github_token) if repository else True |
There was a problem hiding this comment.
Transitions can race scheduled refresh or token propagation because the marker check and credential writes aren’t atomic, so the owner token could overwrite the new actor.
Can we serialize all per-sandbox credential writers around check, writes, and marker update? Just want to avoid regression upon token refresh mechanism, people get pissed (rightfully so) whenever they lose work in their cloud workflows :sad
| # unsets then re-exports GH_TOKEN/GITHUB_TOKEN from the env file. | ||
| if [ -f /tmp/agentsh-bash-env.sh ]; then | ||
| # shellcheck source=/dev/null | ||
| . /tmp/agentsh-bash-env.sh |
There was a problem hiding this comment.
/tmp/agentsh-bash-env.sh is writable state in the shared, root-run sandbox (and the launch code explicitly treats a snapshot-persisted copy as untrusted). A first Slack actor can replace it with a payload that reads /tmp/agent-github-env; when a later actor is rebound and invokes gh, this new . executes that payload after the later actor's token has been written. This lets an earlier actor exfiltrate a later actor's GitHub credential despite the identity transition.
Prompt To Fix With AI
Do not execute `/tmp/agentsh-bash-env.sh` from the gh shim. Move the minimal credential-file parsing needed by gh into a root-owned, immutable script/image layer, or otherwise have a trusted supervisor pass the current token to gh without sourcing any sandbox-writable script. Ensure a prior actor cannot alter the code that reads a later actor's credential file.Severity: high | Confidence: 91% | React with 👍 if useful or 👎 if not
fe655fe to
c9c19da
Compare
|
|
||
| # Only the fast in-sandbox writes run under this lock (token resolution stays outside), so a short | ||
| # TTL comfortably covers them; the wait stays well under the refresh activity's 2 min timeout. | ||
| _CREDENTIAL_LOCK_TTL_SECONDS = 30 |
There was a problem hiding this comment.
This lock also protects normal cloud-run credential refreshes, but it can expire while the protected writes are still running because the remote update and chmod may each take 30 seconds.
CAn we renew the lock or increase its TTL to cover the complete worst-case operation so concurrent refreshes cannot interleave, it was a big issue back a month or so ago
There was a problem hiding this comment.
ahh interesting, yeah, we can, what TTL you are suggesting, like 5 minutes would be enough?
There was a problem hiding this comment.
Five minutes seems okay fort now, yup. Could we also document how it’s derived and add a test covering a writer that exceeds the old 30-second lease? Longer term, we would need some lock renewal but for now I think that;s okay
| # Install the gh shim at runtime too (see agentsh.GH_GUARD_INSTALL_PATH): a resume from a | ||
| # pre-shim filesystem snapshot — or any window where the base image lags this backend — | ||
| # would otherwise leave gh with no token once the frozen launch-env token is unset. | ||
| self.write_file(GH_GUARD_INSTALL_PATH, read_gh_guard_script()) |
There was a problem hiding this comment.
The runtime-installed gh shim sources /tmp/agentsh-bash-env.sh before it invokes gh. That file is in the shared sandbox filesystem and is writable by the previous Slack actor's agent. An attacker can persist commands in it, then when a more-privileged user sends the next follow-up and their token is written to /tmp/agent-github-env, the shim executes those commands in the privileged command environment. This lets the prior actor use or exfiltrate the new actor's GitHub credential, defeating the per-actor rebind on resumed snapshots.
Prompt To Fix With AI
Do not execute any script from the shared sandbox filesystem as part of credential delivery. On an actor transition, use a freshly provisioned/isolated execution environment for the new actor, or make the credential loader and its token source inaccessible and immutable to prior-agent code. Reinstalling a known script alone is insufficient unless every hook and process state that can run before `gh` is also reset and isolated.Severity: high | Confidence: 91% | React with 👍 if useful or 👎 if not
ce9dc2b to
9471020
Compare
…itions Follow-ups are gated per-actor for MCP; do the same for GitHub. On an actor transition, re-inject the new actor's GitHub token if they have usable access, otherwise log the sandbox out (strip the token from the git remote and env) so the previous actor's GitHub identity can't be used by a follow-up actor who lacks it. Reauthorization for that actor is surfaced by the existing credential-refresh path, unchanged. Fail closed only when the sandbox can be neither rebound nor cleared. Only USER-authored runs carry per-actor identity; BOT runs share one installation token.
Two runtime gaps let a follow-up actor keep the previous actor's GitHub identity after a per-message logout: - The agent-server was launched with GITHUB_TOKEN/GH_TOKEN in its process env, frozen for the process lifetime. Clearing the live /tmp/agent-env file could not revoke that copy, so in-process tools resurrected it. Add both vars to SANDBOX_AGENT_LAUNCH_UNSET_ENV_VARS; the token is still delivered per command via the file (re-sourced by BASH_ENV, seeded before the unset). - The periodic user-token refresh loop re-applied the run owner's token to every live sandbox keyed on the owner's integration, overwriting a transition. Skip runs whose sandbox-github-identity marker is bound to a different actor. Pairs with the agent-server fix (PostHog/code) that treats an emptied env file as an explicit logout instead of falling back to the process env.
Record why the github identity gate logs the sandbox out (missing/invalid integration, no app installation, or no repo permission) so the transition outcome is debuggable.
Address review findings on the per-actor GitHub identity gate: - The scheduled per-run refresh (GitHubSandboxCredential.refresh) resolved the actor from the startup context and re-applied the owner token without checking the sandbox-github-identity marker, restoring the original actor about one refresh interval after a transition. Skip the refresh when the sandbox is bound to a different actor, mirroring _propagate_user_token. - apply_github_credentials_to_sandbox now reports whether both writes succeeded; the gate records the new actor only on a confirmed rebind and falls through to logout on a partial write. - Fail the follow-up closed when the live sandbox handle cannot be resolved on a transition, rather than delivering under the prior actor s retained creds.
Address ReviewHog observability/robustness findings: - Log the cause in _resolve_live_sandbox before returning None, so a fail-closed follow-up rejection is diagnosable (dead sandbox vs transient lookup error). - Guard the logout clear call like the rebind above it: a sandbox exec that raises (stopped/timed out mid-call) now fails closed instead of escaping. - Broaden the token-resolution except to the credential-unavailable family (CredentialUnavailableError, Integration/UserIntegration.DoesNotExist), not only ReauthorizationRequired, so a mid-run disconnect logs out cleanly rather than propagating uncontrolled.
Centralize the duplicated per-actor identity check (sibling propagation and scheduled refresh both read the same marker) into _actor_rebound_away_from_owner so the two copies cannot drift, and name it distinctly from the loop-owner eligibility gate it sits beside.
Scheduled refresh, sibling propagation, and the per-message actor gate all write a sandbox's GitHub credentials. Without mutual exclusion, a slow owner-token write can land after a follow-up rebound the sandbox to a different actor, resurrecting the owner's identity for that actor. Serialize every writer on a per-sandbox redis lock and re-check the actor binding inside it before writing, so the check, write, and marker update run atomically.
The backend delivers the per-actor GitHub token through BASH_ENV, which only non-interactive `bash -c` honors. The agent runs its tool commands in an interactive shell, so `gh` there had no token and PR creation failed once the frozen process-env token was removed. Add a gh PATH shim (mirroring git-guard) installed first on PATH that sources the same credential script on every call, so gh authenticates as the current actor regardless of shell mode and honors logout (an emptied file exports nothing). gh is the only env-dependent consumer: git uses the remote URL and the signed-commit tool reads the file in-process.
The 30s lease could expire mid-write: each managed write is a chain of in-sandbox execs bounded by a 30s timeout, and the per-message gate can run two chains back-to-back (apply the new token, then log out), so a slow sandbox can hold the lock ~2 min. Raise the TTL to 5 min so the lease cannot expire mid-write and let a concurrent refresh acquire and interleave. Document the derivation and add a test covering a writer past the old 30s lease.
The gh PATH shim is baked into new base images, but a resume from a pre-shim filesystem snapshot — or any window where the base image lags this backend — would lack it, leaving gh with no token once the frozen launch-env token is unset. Write and chmod the shim from start_agent_server (reading its single source of truth) so it's present on every launch regardless of image age.
9471020 to
73b4353
Compare
|
|
||
|
|
||
| def _mark_sandbox_identity(kind: str, scope: str, user_id: int) -> None: | ||
| get_tasks_cache().set(_sandbox_identity_cache_key(kind, scope), user_id, timeout=MCP_TOKEN_REFRESH_INTERVAL_SECONDS) |
There was a problem hiding this comment.
The sole record that this sandbox was transitioned to a non-owner expires after three hours. Once it expires, _actor_rebound_away_from_owner returns None; the scheduled refresh, which uses the startup owner's context, accepts that as safe and _apply_owner_token_locked writes the owner's token back. The next follow-up from an actor without repo access then takes the prior marker/credential path and can execute GitHub operations as the owner.
Prompt To Fix With AI
Store the current GitHub credential actor/binding durably for the sandbox lifetime (including an explicit logged-out state), or ensure owner-scoped scheduled refresh and propagation fail closed when the identity marker is absent after a possible actor transition. Do not treat an expired/evicted cache marker as owner-safe. Add a test that transitions to a different actor, expires/removes the marker, runs GitHubSandboxCredential.refresh(), and asserts the owner token is not applied.Severity: high | Confidence: 94% | React with 👍 if useful or 👎 if not
| absent entry reads as "must re-establish", which is always safe because | ||
| re-establishing re-applies or clears rather than trusting stale creds. | ||
| """ | ||
| _mark_sandbox_identity("github-identity", scope, user_id) |
There was a problem hiding this comment.
High: Actor binding expires before the sandbox
This marker expires after three hours, is not renewed by the same-actor fast path, and the sandbox can live for six hours. An actor can keep the run active and leave a delayed GitHub command; after the marker expires, _apply_owner_token_locked interprets the missing value as owner-bound and the periodic refresh injects the owner's token. Persist the binding for the sandbox's lifetime, or make owner refreshes treat an unknown binding as unsafe once a transition has occurred.
3 participants
Problem
In multiplayer Slack threads, several people send follow-ups to one shared sandbox that is authenticated as whoever started it. Without per-actor identity, a follow-up from a different user runs with the starter's GitHub token, so a user with no repo access could push or open PRs as the starter.
sequenceDiagram actor A as User A (has access) actor B as User B (no access) participant S as Sandbox (shared) participant G as GitHub A->>S: starts task, clones repo Note over S: bound to A's token B->>S: follow-up: commit and open a PR Note over S: transition A to B S->>S: B has no access, so LOG OUT (strip remote + env file) B->>S: agent tries to push Note over S,G: no token to fall back on, push blocked (was: pushed as A)Changes
_refresh_sandbox_github): resolve the sender; if they have repo access, rebind the sandbox to their token; if not, log out (strip the git remote + GitHub env file) so the prior actor's identity can't be reused. USER-authored runs only./tmp/agent-github-envfile instead), and skip refresh for runs bound to a different actor.Companion PR (required together)
PostHog/code#3611 (agent-server): treats an emptied env file as an explicit logout (no fallback to process env) and runs
ghcalls as the current actor.How did you test this code?
End-to-end on a dedicated GitHub org where both test users have app-mediated access to the same repo:
Agent context
Model: Claude Opus 4.8.