diff --git a/README.md b/README.md index 5f7a0a7..4093db3 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,8 @@ from their close control, with Delete/Backspace while focused, or by middle-clic without changing local refs or `FETCH_HEAD`. Active PRs can be closed from a confirmed overflow action. Closed PRs can be reopened; merged PRs and every terminal discussion/thread surface remain read-only. + Completed Azure PR Code views remain available after the provider deletes + the source branch by using its recorded target and merge commits. GitHub and Azure DevOps Services authentication stays in the signed-in `gh` / `az` CLI. Azure DevOps Server 2020+ is available through an optional, independently updated, signed `strand-azdo` REST helper configured in Settings → Hosting; @@ -218,8 +220,9 @@ from their close control, with Delete/Backspace while focused, or by middle-clic Generation is cancellable, scans conservative sensitive-file signals before provider launch, keeps the generated description content-sized, and can retry explicitly with the other provider without changing your - default. Repository-family writing profiles keep terminology and style - consistent across worktrees. + default. Provider failures are reduced to concise, actionable hints; raw CLI + session, prompt, and patch transcripts are never displayed. Repository-family + writing profiles keep terminology and style consistent across worktrees. - **Fast by design** — reads go through [gix](https://github.com/GitoxideLabs/gitoxide), writes through git2 and your system `git`. Performance targets live in [`PRD.md`](./PRD.md) §8 and are measured in diff --git a/ROADMAP.md b/ROADMAP.md index 5507639..b546fcd 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2295,6 +2295,14 @@ only `v1.1.0`. Universal macOS notarization returned `Accepted` (request Authenticode `NotSigned`, and the Git tag has no cryptographic signature, so the external publisher and remaining release-checklist gates stay open. +**Linear bug sweep completed (2026-07-24):** DAN-24/25/26 are resolved. +Completed Azure PR Code views now use durable target/merge history, so +source-branch cleanup cannot blank the patch. AI vendor failures are reduced to +bounded, actionable messages and transcript-shaped output never exposes echoed +prompt or patch content. A real deleted-branch Git integration test covers the +Azure path, and a live WebView2 pass confirmed Pierre uses its light palette +when Strand resolves the system theme to light. + --- ## Cross-cutting tracks (run in parallel with all milestones) diff --git a/TASKS.md b/TASKS.md index dfb3277..878eb51 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1552,7 +1552,11 @@ tree: watch the agent work, review fast, accept or reject safely. diff; line totals stay in the selected-file header rather than crowding every tree row. The file header exposes the same persisted stacked/split controls in context. Azure comparisons - fetch source/target objects without updating repository refs or FETCH_HEAD. + fetch source/target objects without updating repository refs or FETCH_HEAD; + completed Azure PRs reconstruct the historical patch from + `lastMergeTargetCommit` → `lastMergeCommit`, so provider source-branch + cleanup cannot break Code (`completed_azure_diff_survives_a_deleted_source_branch`, + DAN-24). - ☑ 1.0 discussion threads and comment creation: Timeline reads GitHub issue comments, GitHub review-thread comments, and Azure thread comments (including inline file context) as safe @@ -2105,6 +2109,11 @@ extraction above as prerequisite. **Do not start before 1.0 ships** - ☑ Broken vendor-CLI installs stay distinct from signed-out sessions (`AiProviderStatus.error`, auth-failure classification, and `--version` login preflight prevent false “browser opened” messages) +- ☑ Vendor CLI failures become bounded, actionable messages instead of raw + session transcripts (`user_facing_cli_failure`, DAN-26): known rate-limit, + quota/billing, model, context, timeout, and network failures are classified; + prompt/patch text after vendor transcript boundaries is never used for + classification or shown to the user. - ☑ AI provider execution hardened and cancellable (`AiProviderAdapter`, canonical CLI resolution, isolated read-only Codex cwd/argv, bounded stdout/stderr, process-group/Windows Job Object teardown, and shared diff --git a/crates/strand-tauri/src/ai/bin.rs b/crates/strand-tauri/src/ai/bin.rs index 8238b91..0f6a9f5 100644 --- a/crates/strand-tauri/src/ai/bin.rs +++ b/crates/strand-tauri/src/ai/bin.rs @@ -660,49 +660,45 @@ mod tests { assert!(err.contains("timed out"), "unexpected error: {err}"); } - #[cfg(not(windows))] - #[test] - fn run_capture_cancels_process_group() { + fn assert_process_group_cancelled_promptly( + program: &'static str, + args: &'static [&'static str], + ) { let cancel = AiCancelHandle::new(); - let trigger = cancel.clone(); - std::thread::spawn(move || { - std::thread::sleep(Duration::from_millis(100)); - trigger.cancel(); + let worker_cancel = cancel.clone(); + let (started_tx, started_rx) = std::sync::mpsc::sync_channel(0); + let worker = std::thread::spawn(move || { + started_tx.send(()).unwrap(); + run_capture_cancellable( + Path::new(program), + args, + None, + None, + SUGGEST_TIMEOUT, + Some(&worker_cancel), + ) }); - let started = Instant::now(); - let err = run_capture_cancellable( - Path::new("/bin/sh"), - &["-c", "sleep 30 & wait"], - None, - None, - SUGGEST_TIMEOUT, - Some(&cancel), - ) - .unwrap_err(); + started_rx.recv().unwrap(); + std::thread::sleep(Duration::from_millis(100)); + let cancelled_at = Instant::now(); + cancel.cancel(); + let err = worker.join().unwrap().unwrap_err(); assert_eq!(err, "cancelled"); - assert!(started.elapsed() < Duration::from_secs(3)); + assert!(cancelled_at.elapsed() < Duration::from_secs(3)); + } + + #[cfg(not(windows))] + #[test] + fn run_capture_cancels_process_group() { + assert_process_group_cancelled_promptly("/bin/sh", &["-c", "sleep 30 & wait"]); } #[cfg(windows)] #[test] fn run_capture_cancels_process_group() { - let cancel = AiCancelHandle::new(); - let trigger = cancel.clone(); - std::thread::spawn(move || { - std::thread::sleep(Duration::from_millis(100)); - trigger.cancel(); - }); - let started = Instant::now(); - let err = run_capture_cancellable( - Path::new("cmd.exe"), + assert_process_group_cancelled_promptly( + "cmd.exe", &["/C", "ping", "-n", "30", "127.0.0.1"], - None, - None, - SUGGEST_TIMEOUT, - Some(&cancel), - ) - .unwrap_err(); - assert_eq!(err, "cancelled"); - assert!(started.elapsed() < Duration::from_secs(3)); + ); } } diff --git a/crates/strand-tauri/src/ai/mod.rs b/crates/strand-tauri/src/ai/mod.rs index 9ea1570..b519943 100644 --- a/crates/strand-tauri/src/ai/mod.rs +++ b/crates/strand-tauri/src/ai/mod.rs @@ -72,15 +72,84 @@ pub(crate) fn map_cli_failure( provider_label: &str, err: String, ) -> String { + let diagnostic = cli_diagnostic_prefix(&err); if let Some(health_error) = &status.error { health_error.clone() - } else if !status.logged_in || is_auth_failure(&err) { + } else if !status.logged_in || is_auth_failure(diagnostic) { format!("{AI_AUTH_REQUIRED} {provider_label} is not signed in.") } else { - err + user_facing_cli_failure(provider_label, diagnostic, diagnostic.len() == err.len()) } } +fn cli_diagnostic_prefix(err: &str) -> &str { + [" String { + let lower = err.to_ascii_lowercase(); + if lower.contains("rate limit") || lower.contains("too many requests") || lower.contains("429") { + return format!("{provider_label} is rate-limited right now. Wait a moment, then try again."); + } + if lower.contains("quota") + || lower.contains("billing") + || lower.contains("insufficient credits") + { + return format!( + "{provider_label} could not generate a suggestion because the provider quota or billing limit was reached." + ); + } + if (lower.contains("model") && lower.contains("not found")) + || lower.contains("unsupported model") + || lower.contains("invalid model") + { + return format!( + "{provider_label} does not support the selected model. Choose another model in Settings → AI and try again." + ); + } + if lower.contains("context length") + || lower.contains("context window") + || lower.contains("maximum context") + || lower.contains("too many tokens") + { + return format!( + "{provider_label} could not fit these changes in its context window. Stage or select fewer changes, then try again." + ); + } + if lower.contains("timed out") { + return format!("{provider_label} took too long to respond. Check your connection and try again."); + } + if lower.contains("network") + || lower.contains("connection reset") + || lower.contains("connection refused") + || lower.contains("could not resolve host") + || lower.contains("dns") + { + return format!("{provider_label} could not be reached. Check your connection and try again."); + } + + // Vendor CLIs can emit their full session preamble and echoed stdin on a + // failed run. That stdin contains repository paths and patches, so never + // surface or persist an unclassified transcript. A short single-line + // diagnostic is safe and usually already actionable. + let detail = err.trim(); + if allow_detail + && !detail.is_empty() + && detail.len() <= 240 + && !detail.contains(['\n', '\r']) + && !detail.contains("secret".into(), + ); + assert_eq!( + err, + "Codex does not support the selected model. Choose another model in Settings → AI and try again." + ); + assert!(!err.contains("secret")); + } + + #[test] + fn map_cli_failure_does_not_classify_words_echoed_from_the_prompt() { + let status = AiProviderStatus { + provider: AiProvider::Openai, + installed: true, + logged_in: true, + account_hint: None, + error: None, + }; + let err = map_cli_failure( + &status, + "Codex", + "session preamble\nnot logged in; invalid model; rate limit".into(), + ); + assert_eq!( + err, + "Codex could not generate a suggestion. Check the selected model, provider status, and connection, then try again." + ); + } + #[test] fn truncates_recent_subjects_on_utf8_boundaries() { let subject = "é".repeat(100); diff --git a/crates/strand-tauri/src/pull_requests.rs b/crates/strand-tauri/src/pull_requests.rs index ca68185..ac866bd 100644 --- a/crates/strand-tauri/src/pull_requests.rs +++ b/crates/strand-tauri/src/pull_requests.rs @@ -3042,29 +3042,45 @@ fn diff_azure_value(cwd: &str, remote: String, value: Value) -> Result { let source_remote = text(value.pointer("/forkSource/repository/remoteUrl")); let local = Repo::discover(cwd).map_err(|error| error.to_string())?; - if source_remote - .as_deref() - .is_none_or(|source| source == remote) - { - local - .fetch_refs_for_read(&remote, &[target_ref.as_str(), source_ref.as_str()]) - .map_err(|error| error.to_string())?; - } else { + let completed_merge = text(value.get("status")) + .is_some_and(|status| status.eq_ignore_ascii_case("completed")) + .then(|| text(value.pointer("/lastMergeCommit/commitId"))) + .flatten(); + let (base, head) = if let Some(merge_commit) = completed_merge { + // Azure keeps the immutable result commit after completion even when + // completion deletes the source branch. The target branch reaches both + // this result and the pre-merge target commit, so historical Code views + // need only the durable target ref. local .fetch_refs_for_read(&remote, &[target_ref.as_str()]) .map_err(|error| error.to_string())?; - local - .fetch_refs_for_read( - source_remote.as_deref().expect("checked above"), - &[source_ref.as_str()], - ) + (target_commit, merge_commit) + } else { + if source_remote + .as_deref() + .is_none_or(|source| source == remote) + { + local + .fetch_refs_for_read(&remote, &[target_ref.as_str(), source_ref.as_str()]) + .map_err(|error| error.to_string())?; + } else { + local + .fetch_refs_for_read(&remote, &[target_ref.as_str()]) + .map_err(|error| error.to_string())?; + local + .fetch_refs_for_read( + source_remote.as_deref().expect("checked above"), + &[source_ref.as_str()], + ) + .map_err(|error| error.to_string())?; + } + let base = local + .merge_base(&target_commit, &source_commit) .map_err(|error| error.to_string())?; - } - let base = local - .merge_base(&target_commit, &source_commit) - .map_err(|error| error.to_string())?; + (base, source_commit) + }; let files = local - .diff_between(&base, &source_commit) + .diff_between(&base, &head) .map_err(|error| error.to_string())?; let patch = files .into_iter() @@ -4448,6 +4464,67 @@ mod tests { let _ = std::fs::remove_dir_all(base); } + #[test] + fn completed_azure_diff_survives_a_deleted_source_branch() { + let base = std::env::temp_dir().join(format!( + "strand-pr-completed-diff-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let publisher = base.join("publisher"); + let consumer = base.join("consumer"); + let remote = base.join("remote.git"); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&publisher).unwrap(); + git(&publisher, &["init", "-q", "-b", "main"]); + git(&publisher, &["config", "user.name", "Test"]); + git(&publisher, &["config", "user.email", "test@example.com"]); + git(&publisher, &["config", "commit.gpgsign", "false"]); + std::fs::write(publisher.join("a.txt"), "one\n").unwrap(); + git(&publisher, &["add", "a.txt"]); + git(&publisher, &["commit", "-q", "-m", "base"]); + let target_commit = git(&publisher, &["rev-parse", "HEAD"]); + git(&publisher, &["checkout", "-q", "-b", "topic"]); + std::fs::write(publisher.join("a.txt"), "two\n").unwrap(); + git(&publisher, &["commit", "-qam", "topic"]); + let source_commit = git(&publisher, &["rev-parse", "HEAD"]); + git(&publisher, &["checkout", "-q", "main"]); + git(&publisher, &["merge", "-q", "--no-ff", "topic", "-m", "merge"]); + let merge_commit = git(&publisher, &["rev-parse", "HEAD"]); + + git(&base, &["init", "-q", "--bare", remote.to_str().unwrap()]); + git( + &publisher, + &["remote", "add", "origin", remote.to_str().unwrap()], + ); + git(&publisher, &["push", "-q", "origin", "main", "topic"]); + git(&publisher, &["push", "-q", "origin", "--delete", "topic"]); + + std::fs::create_dir_all(&consumer).unwrap(); + git(&consumer, &["init", "-q", "-b", "main"]); + git( + &consumer, + &["remote", "add", "origin", remote.to_str().unwrap()], + ); + let patch = diff_azure_value( + consumer.to_str().unwrap(), + "origin".into(), + serde_json::json!({ + "status": "completed", + "sourceRefName": "refs/heads/topic", + "targetRefName": "refs/heads/main", + "lastMergeSourceCommit": { "commitId": source_commit }, + "lastMergeTargetCommit": { "commitId": target_commit }, + "lastMergeCommit": { "commitId": merge_commit } + }), + ) + .unwrap(); + assert!(patch.contains("-one")); + assert!(patch.contains("+two")); + + let _ = std::fs::remove_dir_all(base); + } + #[test] fn normalizes_github_and_azure_payloads() { let github: Value = serde_json::from_str( diff --git a/docs/learnings.md b/docs/learnings.md index ec94d6d..65bda29 100644 --- a/docs/learnings.md +++ b/docs/learnings.md @@ -1949,3 +1949,19 @@ file document is standalone or embedded; revision, binary, size, encoding, and backend safety checks are the only read-only gates. An embedded clean document may follow watcher refreshes, but a watcher tick must never replace an unsaved draft; rely on the optimistic stale-write check to protect newer disk content. + +**Completed Azure PR diffs must use provider-recorded merge history +(2026-07-24).** Azure commonly deletes a completed PR's source branch, so never +reconstruct its Code view by fetching that ref. Fetch the durable target ref +without updating local refs or `FETCH_HEAD`, then compare +`lastMergeTargetCommit` to `lastMergeCommit`. Keep a real remote-deletion +integration test because a fixture with both refs cannot reproduce this +provider lifecycle. + +**Raw AI CLI stderr is untrusted content, not a user-facing diagnostic +(2026-07-24).** The general rule to preserve provider diagnostics verbatim +still applies to Git and hosting operations, but AI vendor CLIs may emit session +metadata plus echoed stdin containing repository paths, prompts, and patches. +Preserve only bounded single-line diagnostics; classify known failures from the +diagnostic prefix before any prompt boundary, and replace every unclassified +transcript with a stable Strand-authored recovery hint. diff --git a/website/docs/pull-requests.md b/website/docs/pull-requests.md index 866873d..e950b28 100644 --- a/website/docs/pull-requests.md +++ b/website/docs/pull-requests.md @@ -224,7 +224,9 @@ edge to edge beneath the same compact, collapsible file header used by Local Changes. Use the two layout buttons in that header to switch between stacked and split diffs; the choice is saved per repository. Only one file diff is mounted at a time to keep large PRs responsive. Provider patches larger than -16 MB are not rendered. +16 MB are not rendered. For completed Azure DevOps PRs, Code reconstructs the +patch from the provider's recorded target and merge commits, so deleting the +source branch does not remove the historical diff. Code also tracks review progress locally for the exact pull-request head and each file's rendered patch. Choose **Mark viewed** or press `v`; if that file or diff --git a/website/docs/settings.md b/website/docs/settings.md index 26bb0a4..78719fe 100644 --- a/website/docs/settings.md +++ b/website/docs/settings.md @@ -157,7 +157,10 @@ in, the action stays clickable and the hint appears inline. Sign-in starts the provider's browser or CLI flow, and once you complete it you run the action again. If a CLI launcher is present but its packaged executable is broken, Strand keeps that distinct from “signed out” and shows a repair hint beside the -form rather than claiming that sign-in opened. +form rather than claiming that sign-in opened. Generation failures use concise +hints for recognized provider limits, model problems, timeouts, and connection +errors. Strand never displays a raw vendor CLI session transcript because it +can contain the prompt, repository paths, and patch content. Strand never runs an automatic provider-status subprocess from Local Changes. Provider checks remain explicit here. Writing generation is user-initiated,