Repository navigation
Cloud: recover completed repository attachments - #30452
teamleaderleo wants to merge 0 commit into
Conversation
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5-5-medium): SHIP at c24b5f5
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmux-tui/crates/cmux-tui-core/src/cloud_repos/store.rs">
<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/cloud_repos/store.rs:224">
P2: This retires any job for the same repo, including a `running` one. Check for `"done"` so an in-flight clone stays visible to `pending()` and keeps its progress.</violation>
</file>
<file name="cmux-tui/crates/cmux-tui-core/src/cloud_repos/runtime.rs">
<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/cloud_repos/runtime.rs:165">
P3: The `repo.needs_github` early return discards an install link already received from an earlier page's `connect` reply. Fall back to `catalog.install_url` when the error carries no valid link.</violation>
<violation number="2" location="cmux-tui/crates/cmux-tui-core/src/cloud_repos/runtime.rs:249">
P2: Completed-clone recovery is case-sensitive even though repository matching is case-insensitive. A retry with different repository capitalization misses `completed` and starts another clone; look up the saved job case-insensitively or normalize the job key before this branch.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // Attachment and job retirement share one CAS. A failed write leaves | ||
| // the completed job available for retry, never an occupied orphan. | ||
| if let Some(name) = repo["repo"].as_str().and_then(|repo| repo.rsplit('/').next()) { | ||
| if doc["jobs"][session][name]["repo"] == repo["repo"] { |
There was a problem hiding this comment.
P2: This retires any job for the same repo, including a running one. Check for "done" so an in-flight clone stays visible to pending() and keeps its progress.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/cmux-tui-core/src/cloud_repos/store.rs, line 224:
<comment>This retires any job for the same repo, including a `running` one. Check for `"done"` so an in-flight clone stays visible to `pending()` and keeps its progress.</comment>
<file context>
@@ -218,6 +218,19 @@ pub(super) fn record(mux: &Mux, session: &str, repo: &Value) -> Result<(), Failu
+ // Attachment and job retirement share one CAS. A failed write leaves
+ // the completed job available for retry, never an occupied orphan.
+ if let Some(name) = repo["repo"].as_str().and_then(|repo| repo.rsplit('/').next()) {
+ if doc["jobs"][session][name]["repo"] == repo["repo"] {
+ if let Some(jobs) = doc
+ .get_mut("jobs")
</file context>
| if doc["jobs"][session][name]["repo"] == repo["repo"] { | |
| if doc["jobs"][session][name]["state"] == "done" && doc["jobs"][session][name]["repo"] == repo["repo"] { |
| "Choose a different chat for this repository or branch; another add already uses that folder.", | ||
| )); | ||
| } | ||
| if let Some(completed) = existing.get("completed") { |
There was a problem hiding this comment.
P2: Completed-clone recovery is case-sensitive even though repository matching is case-insensitive. A retry with different repository capitalization misses completed and starts another clone; look up the saved job case-insensitively or normalize the job key before this branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/cmux-tui-core/src/cloud_repos/runtime.rs, line 249:
<comment>Completed-clone recovery is case-sensitive even though repository matching is case-insensitive. A retry with different repository capitalization misses `completed` and starts another clone; look up the saved job case-insensitively or normalize the job key before this branch.</comment>
<file context>
@@ -244,6 +246,19 @@ impl Backend for Runtime<'_> {
"Choose a different chat for this repository or branch; another add already uses that folder.",
));
}
+ if let Some(completed) = existing.get("completed") {
+ // The seed job can expire after clone completion. Retain its
+ // result until attachment recording succeeds, so recovery
</file context>
| repos: Vec::new(), | ||
| install_url: Some(self.install_url(error)?), | ||
| repos: catalog.repos, | ||
| install_url: self.optional_install_url(error)?, |
There was a problem hiding this comment.
P3: The repo.needs_github early return discards an install link already received from an earlier page's connect reply. Fall back to catalog.install_url when the error carries no valid link.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/cmux-tui-core/src/cloud_repos/runtime.rs, line 165:
<comment>The `repo.needs_github` early return discards an install link already received from an earlier page's `connect` reply. Fall back to `catalog.install_url` when the error carries no valid link.</comment>
<file context>
@@ -160,9 +157,13 @@ impl Backend for Runtime<'_> {
- repos: Vec::new(),
- install_url: Some(self.install_url(error)?),
+ repos: catalog.repos,
+ install_url: self.optional_install_url(error)?,
+ disconnected: true,
});
</file context>
| install_url: self.optional_install_url(error)?, | |
| install_url: self.optional_install_url(error)?.or(catalog.install_url), |
24f8991 to
8306a56
Compare
c24b5f5 to
9c13b04
Compare
Leo asked for “this kind of recovery” when attaching a repository to a Cloud chat.
A completed clone now persists its result until the chat attachment is recorded. Retrying after a record failure reuses that saved result even when the seed job has expired, and attachment recording clears the job atomically.
Follow-up 4 to #30361. A regression test injects record failure, shuts down the fake seed socket, and verifies retry records the completed clone without another start or status call. Attachment recording clears the saved job in the same projection transaction.
Repository discovery also retains pages already fetched if authentication expires on a later page. An unsafe installation link is omitted without hiding the repositories; disconnected state is tracked separately so it cannot falsely complete the Connect GitHub wait. Fake socket tests check that partial catalog and the actual pending clone progress before completion, addressing the recovery review follow-ups.
Validation: Rust fleet checks pending; formatting and static prepush pass.
Agent session: agent session
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
A completed repository clone now survives attachment-record failures, so retrying after the seed job expires reuses the saved result instead of cloning again. Repository discovery also keeps pages already fetched when authentication expires mid-pagination, and an unsafe installation link no longer hides those repositories.
Written for commit 9c13b04. Summary will update on new commits.