Skip to content

[REMOTE-1370] Phase 2b: driver credential writes and refresh loop#10153

Merged
jasonkeung merged 8 commits intomasterfrom
jason/remote-1370-phase2-impl
May 7, 2026
Merged

[REMOTE-1370] Phase 2b: driver credential writes and refresh loop#10153
jasonkeung merged 8 commits intomasterfrom
jason/remote-1370-phase2-impl

Conversation

@jasonkeung
Copy link
Copy Markdown
Contributor

@jasonkeung jasonkeung commented May 5, 2026

Description

Second half of Phase 2 of REMOTE-1370. Depends on: #10152.

Adds the actual credential management logic in the driver — startup writes, git config, and periodic refresh.

Changes:

  • New `driver/git_credentials.rs` with:
    • Atomic writes to `/.git-credentials` and `/.config/gh/hosts.yaml`
    • One-time git config setup (`credential.helper store`, SSH→HTTPS `url.insteadOf`)
    • Git user identity configuration from server-returned username/email
    • `refresh_loop()`: infinite async future that re-fetches and overwrites credentials every ~50 minutes
  • `fetch_secrets_and_attachments` calls `taskGitCredentials` as a 5th parallel fetch alongside the existing four (managed secrets, task attachments, task metadata, handoff snapshot); writes credential files and git config on completion
  • `run_internal` races harness execution against the refresh loop via `futures::select!` for both Oz and ThirdParty harness paths; the refresh future is dropped when the harness completes

Why 50 minutes: GitHub tokens expire after 1 hour. The server's `OAuthConnectionTokenSource` already refreshes with a 55-minute lead time, so each call returns a fresh token.

Phase 1 (warp-server) is deployed and stable, and #10152 is merged.

Linked Issue

Linear: REMOTE-1370

Testing

local demo
https://www.loom.com/share/0c4dc45062ba4909be41145f0c2dabc9

  • All existing tests pass (`cargo nextest run --no-fail-fast -p warp`)
  • `cargo fmt` and `cargo clippy` clean

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

Conversation: https://staging.warp.dev/conversation/e16dc3e2-8e2f-4499-8c9b-59b200e17c50

Co-Authored-By: Oz oz-agent@warp.dev

jasonkeung and others added 2 commits May 5, 2026 11:56
- Add taskGitCredentials types to schema.graphql (verified against staging)
  and add to clientQueries allowlist in client-schema.ts
- Add task_git_credentials.rs GraphQL query file (cynic) following the
  task_secrets pattern
- Add GitCredential struct and get_task_git_credentials to AIClient
  trait with a ServerApi implementation

Co-Authored-By: Oz <oz-agent@warp.dev>
- Add driver/git_credentials.rs: atomic writes to ~/.git-credentials
  and ~/.config/gh/hosts.yaml, one-time git config setup (credential.helper
  store, url.insteadOf), git identity configuration, and refresh_loop()
  which re-fetches credentials every 50 minutes
- Extend fetch_secrets_and_attachments to call taskGitCredentials in
  parallel at startup; writes credential files and git config
- Add futures::select! refresh loop in run_internal, racing the harness
  execution against the infinite refresh loop for both Oz and ThirdParty
  harnesses

Co-Authored-By: Oz <oz-agent@warp.dev>
@jasonkeung
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 5, 2026

@jasonkeung

I'm re-reviewing this pull request in response to a review request.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds cloud-agent git/gh credential file management, git config setup, and a periodic refresh loop around Oz and third-party harness execution.

Concerns

  • The credential file writes create token-bearing files with default process umask permissions instead of forcing owner-only permissions.
  • The second url.https://github.com/.insteadOf write overwrites the first rewrite, so one advertised SSH URL form will not be configured.

Security

  • GitHub tokens are written to plaintext credential files; those files must be created with restrictive permissions before merge.

Verdict

Found: 0 critical, 2 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread app/src/ai/agent_sdk/driver/git_credentials.rs Outdated
Comment thread app/src/ai/agent_sdk/driver/git_credentials.rs Outdated
- Write ~/.git-credentials and ~/.config/gh/hosts.yaml temp files with
  0600 permissions (mode + set_permissions) matching the existing pattern
  in write_codex_auth_json, preventing other users from reading tokens
- Add run_git_config_add (--add flag) so multi-value insteadOf entries
  coexist instead of the second overwriting the first
- Derive SSH→HTTPS URL rewrites from credential hosts rather than
  hardcoding github.com, using --add for both scp-style and ssh:// forms
  per host

Co-Authored-By: Oz <oz-agent@warp.dev>
@jasonkeung
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 5, 2026

@jasonkeung

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds driver-side GitHub credential setup for cloud agent sandboxes, including initial credential file writes, git/gh configuration, and a periodic refresh loop that is raced against Oz and third-party harness execution.

Concerns

  • No blocking code or security concerns found in the annotated diff.
  • The PR description states this should not merge until Phase 1 is deployed and #10152 is merged; keep that release dependency enforced outside this code review.

Verdict

Found: 0 critical, 0 important, 0 suggestions

Approve

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@jasonkeung jasonkeung marked this pull request as ready for review May 5, 2026 18:50
@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 5, 2026

@jasonkeung

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds cloud-agent GitHub credential file setup, git/gh configuration, and a periodic refresh loop for long-running harnesses.

Concerns

  • Empty credential responses are treated as no-ops, which can leave previously written GitHub credentials on disk in reused or revoked-auth scenarios.
  • Refresh failures sleep for another full refresh interval before retrying, so a single transient failure around the 50-minute refresh point can leave agents with expired credentials until the next 50-minute tick.

Security

  • Stale credential files should be cleared when the server returns no credentials; otherwise previous task credentials can remain available in the sandbox.

Verdict

Found: 0 critical, 2 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

log::info!("Git credentials configured from taskGitCredentials");
}
}
Ok(_) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [IMPORTANT] [SECURITY] Treating an empty authoritative response as a no-op leaves any existing ~/.git-credentials/hosts.yaml from a previous run in place; clear the credential files instead so reused sandboxes cannot retain stale repo access.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these credentials that are issued should expire within 60 minutes anyway, and I dont think we are reusing sandboxes either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed this is not an issue

Comment thread app/src/ai/agent_sdk/driver/git_credentials.rs Outdated
jasonkeung added a commit that referenced this pull request May 5, 2026
#10152)

## Description

First half of Phase 2 of REMOTE-1370. Adds the GraphQL plumbing for
\`taskGitCredentials\` so the driver can call it in the next PR.

**Changes:**
- Add \`taskGitCredentials\` types to \`schema.graphql\` and add to the
\`clientQueries\` allowlist in \`client-schema.ts\`
- New \`task_git_credentials.rs\` cynic query file, following the
\`task_secrets\` pattern
- Add \`GitCredential\` struct and \`get_task_git_credentials\` to
\`AIClient\` trait + \`ServerApi\` implementation

No behaviour change — nothing calls \`get_task_git_credentials\` yet;
that's in the follow-on PR.

**schema.graphql approach:** \`yarn generate -p staging\` was run to
verify the new types, but the output was not committed wholesale because
the current staging server also has other unrelated schema changes
(removed \`freeAvailableModels\`, new \`VOYAGE_4_512\` EmbeddingConfig
variant, etc.) that are out of scope and would break existing Rust
bindings. Instead, only the \`taskGitCredentials\` types were
cherry-picked — the field names, nullability, and doc comments were
verified to be identical to what the server generates.

Warp-server staging is deployed with the new query.

Followed by: #10153.

## Linked Issue

Linear: REMOTE-1370

## Testing

- \`cargo build -p warp\` clean
- \`cargo fmt\` and \`cargo clippy\` clean

## Agent Mode
- [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode

Conversation:
https://staging.warp.dev/conversation/e16dc3e2-8e2f-4499-8c9b-59b200e17c50

Co-Authored-By: Oz <oz-agent@warp.dev>

---------

Co-authored-by: Oz <oz-agent@warp.dev>
Co-authored-by: oz-for-oss[bot] <277970191+oz-for-oss[bot]@users.noreply.github.com>
Base automatically changed from jason/remote-1370-phase2-infra to master May 5, 2026 21:00
@jasonkeung jasonkeung requested a review from abhishekp106 May 6, 2026 17:20
jasonkeung and others added 4 commits May 6, 2026 13:26
Co-Authored-By: Oz <oz-agent@warp.dev>
On transient failure, retry up to 3 times with 1min, 2min, 4min delays,
keeping all retries within the ~10-minute buffer before token expiry.

Co-Authored-By: Oz <oz-agent@warp.dev>
Copy link
Copy Markdown
Contributor

@abhishekp106 abhishekp106 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Super great to see this working e2e


fn home_dir() -> Result<PathBuf> {
dirs::home_dir().ok_or_else(|| anyhow::anyhow!("Could not determine home directory"))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we have some utility crate that lets you get this IIRC?

tmp_path.display(),
path.display()
)
})?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can pull out this atomic operation (temp file writing and renaming) to be re-usable


if credentials.is_empty() {
log::debug!("No git credentials returned during refresh; skipping file write");
return Ok(());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider this a failure if the credentials received were empty

/// This future never resolves — it is designed to be raced with the harness
/// execution future via `futures::select!` and dropped when the harness
/// completes.
pub(crate) async fn refresh_loop(task_id: String, ai_client: Arc<dyn AIClient>) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

log::info!("Git credentials configured from taskGitCredentials");
}
}
Ok(_) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed this is not an issue

}
} else {
Self::run_harness(runner, &foreground, harness_exit_rx).await
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not blocking for this PR, but I wish the oz harness and third party harness could share this logic a bit more

@jasonkeung jasonkeung merged commit 0ba2ad3 into master May 7, 2026
25 checks passed
@jasonkeung jasonkeung deleted the jason/remote-1370-phase2-impl branch May 7, 2026 16:48
jasonkeung added a commit that referenced this pull request May 7, 2026
…ogfood only) (#10397)

## Summary

- Adds a `GitCredentialRefresh` feature flag to gate the
credential-writing and background refresh-loop behavior introduced in
#10153
- Flag is **dogfood-only** (added to `DOGFOOD_FLAGS`); off by default in
Preview and Stable builds
- When the flag is off, the `git_credentials` fetch future
short-circuits to `Ok(vec![])`, the credential file write block is
skipped, and `task_id_for_refresh` is forced to `None` so the background
refresh loop never spawns

## Files changed

| File | Change |
|------|--------|
| `crates/warp_features/src/lib.rs` | New `GitCredentialRefresh`
variant; added to `DOGFOOD_FLAGS` |
| `app/Cargo.toml` | New `git_credential_refresh = []` Cargo feature |
| `app/src/lib.rs` | Compile-time bridge: `#[cfg(feature =
"git_credential_refresh")] FeatureFlag::GitCredentialRefresh` |
| `app/src/ai/agent_sdk/mod.rs` | Guard fetch future and write block
behind flag |
| `app/src/ai/agent_sdk/driver.rs` | Guard refresh loop
(`task_id_for_refresh`) behind flag |

## Test plan

- [x] Build dogfood — credential fetch and refresh loop should be active
loop still working with flag enabled:
https://www.loom.com/share/14d64e5c90414328a9a9387d9820be7c
- [ ] Build stable/preview (without `git_credential_refresh` feature) —
`git_credentials` returns early, no credential files written, no refresh
loop spawned

_This PR was created by [Oz](https://warp.dev/oz) (running Claude
Code)._

Co-authored-by: Oz <oz-agent@warp.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants