Skip to content

Commit 306adb1

Browse files
committed
Merge remote-tracking branch 'origin/main' into jamadeo/dedupe-sources
2 parents ad06590 + 5f3c63a commit 306adb1

46 files changed

Lines changed: 5268 additions & 2346 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.agents/skills/code-review/SKILL.md

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,16 @@ You are a senior engineer conducting a thorough code review. Review **only the l
9292
- **AnimatePresence**: Is it used properly with unique keys for dialog/modal transitions?
9393
- **Reduced Motion**: Is `useReducedMotion()` respected for accessibility?
9494

95+
### Async State, Defaults & Persistence
96+
- **Async Source of Truth**: During async provider/model/session mutations, does UI/session/localStorage state update only after the backend accepts the change? If the UI updates optimistically, is there an explicit rollback path?
97+
- **UI/Backend Drift**: Could the UI show provider/model/project/persona X while the backend is still on Y after a failed mutation, delayed prepare, or pending-to-real session handoff?
98+
- **Requested vs Fallback Authority**: Do explicit user or caller selections stay authoritative over sticky defaults, saved preferences, aliases, or fallback resolution?
99+
- **Dependent State Invalidation**: When a parent selection changes (provider/project/persona/workspace/etc.), are dependent values like `modelId`, `modelName`, defaults, or cached labels cleared or recomputed so stale state does not linger?
100+
- **Persisted Preference Validation**: Are stored selections validated against current inventory/capabilities before reuse, and do stale values fail soft instead of breaking creation flows?
101+
- **Compatibility of Fallbacks**: Are default or sticky selections guaranteed to remain compatible with the active concrete provider/backend, instead of leaking across providers?
102+
- **Best-Effort Lookups**: Do inventory/config/default-resolution lookups degrade gracefully on transient failure, or can they incorrectly block a primary flow that should still work with a safe fallback?
103+
- **Draft/Home/Handoff Paths**: If the product has draft, Home, pending, or pre-created sessions, did you review those handoff paths separately from the already-active session path?
104+
95105
### General Code Quality
96106
- **Error Handling**: Are errors handled gracefully with user-friendly messages?
97107
- **Loading States**: Are loading states shown during async operations?
@@ -104,13 +114,18 @@ You are a senior engineer conducting a thorough code review. Review **only the l
104114

105115
### Step 0: Run Quality Checks
106116

107-
Before reading any code, run the project's CI gate to establish a baseline:
117+
Before reading any code, run the project's CI gate to establish a baseline. Use **check-only** commands so the baseline never mutates the working tree — otherwise auto-formatters can introduce unstaged diffs and you'll end up reviewing formatter output instead of the author's actual changes.
118+
119+
Avoid `just check-everything` as the baseline in this repo: that recipe runs `cargo fmt --all` in write mode and will modify the working tree. Run the non-mutating equivalents instead:
108120

109121
```bash
110-
just ci
122+
cargo fmt --all -- --check
123+
cargo clippy --all-targets -- -D warnings
124+
(cd ui/desktop && pnpm run lint:check)
125+
./scripts/check-openapi-schema.sh
111126
```
112127

113-
This runs: `pnpm check` (Biome lint/format + file sizes), `pnpm typecheck`, `just clippy` (Rust linting), `pnpm test`, `pnpm build`, and `just tauri-check` (Rust type checking).
128+
If the project has a stronger pre-push or CI gate than this helper set, run that fuller gate when the review is meant to be PR-ready, but only after confirming it is also non-mutating (or run it from a clean stash). In this repo, targeted tests for the changed area plus the pre-push checks are often the practical follow-up.
114129

115130
Report the results as pass/fail. Any failures are automatically **P0** issues and should appear at the top of the findings list. Do not skip this step even if the user only wants a quick review.
116131

@@ -120,7 +135,8 @@ For each file in the list:
120135

121136
1. Run `git diff main...HEAD -- <file>` to get the exact lines that changed
122137
2. Review **only those changed lines** against the Review Checklist — do not flag issues in unchanged code
123-
3. Note the file path and line numbers from the diff output for each issue found
138+
3. For stateful UI or async flow changes, trace the full path end to end: user selection -> local/session state update -> persistence -> backend prepare/set/update call -> failure/rollback path
139+
4. Note the file path and line numbers from the diff output for each issue found
124140

125141
### Step 2: Categorize Issues
126142

@@ -152,16 +168,17 @@ After reviewing all files, provide:
152168

153169
### Step 3b: Self-Check
154170

155-
Before presenting findings to the user, silently review the issue list two more times:
171+
Before presenting findings to the user, silently review the issue list three times:
156172

157173
1. **Pass 1**: For each issue, ask — is this genuinely a problem, or could it be intentional/acceptable? Remove false positives.
158174
2. **Pass 2**: For each remaining issue, ask — does the recommended fix actually improve the code, or is it a matter of preference?
175+
3. **Pass 3**: For async state/default-resolution issues, ask — can the UI, persisted state, and backend ever disagree after a failure, fallback, or session handoff?
159176

160-
After both passes, tag each surviving issue as one of:
177+
After these passes, tag each surviving issue as one of:
161178
- **[Must Fix]** — clear violation, will likely get flagged in PR review
162179
- **[Your Call]** — valid concern but may be intentional or a reasonable tradeoff (e.g. stepping outside the design system for a specific reason). Present it but let the user decide.
163180

164-
Only present issues that survived both passes.
181+
Only present issues that survived these passes.
165182

166183
### Step 4: Fix Issues
167184

@@ -189,7 +206,7 @@ Once all issues are fixed, display:
189206

190207
**✅ Code review complete! All issues have been addressed.**
191208

192-
Your code is ready to commit and push. Lefthook will run the full CI gate (`just ci`) automatically when you push.
209+
Your code is ready to commit and push. Lefthook and CI will run the repo's configured gates when you push.
193210

194211
Next steps: generate a PR summary that explains the intent of this change, what files were modified and why, and how to verify the changes work.
195212

Cargo.lock

Lines changed: 10 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ resolver = "2"
99

1010
[workspace.package]
1111
edition = "2021"
12-
version = "1.31.0"
12+
version = "1.32.0"
1313
rust-version = "1.91.1"
1414
authors = ["AAIF <ai-oss-tools@block.xyz>"]
1515
license = "Apache-2.0"

crates/goose-cli/src/cli.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,8 @@ enum Command {
709709
/// Show verbose information including current configuration
710710
#[arg(short, long, help = "Show verbose information including config.yaml")]
711711
verbose: bool,
712+
#[arg(long, help = "Test provider connection and show status")]
713+
check: bool,
712714
},
713715

714716
#[command(about = "Check that your Goose setup is working")]
@@ -1765,7 +1767,7 @@ pub async fn cli() -> anyhow::Result<()> {
17651767
}
17661768
Some(Command::Configure {}) => handle_configure().await,
17671769
Some(Command::Doctor {}) => crate::commands::doctor::handle_doctor().await,
1768-
Some(Command::Info { verbose }) => handle_info(verbose),
1770+
Some(Command::Info { verbose, check }) => handle_info(verbose, check).await,
17691771
Some(Command::Mcp { server }) => handle_mcp_command(server).await,
17701772
Some(Command::Acp { builtins }) => goose::acp::server::run(builtins).await,
17711773
Some(Command::Serve {

crates/goose-cli/src/commands/info.rs

Lines changed: 182 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
1-
use anyhow::Result;
1+
use anyhow::{anyhow, Result};
22
use console::style;
33
use goose::config::paths::Paths;
44
use goose::config::Config;
5+
use goose::conversation::message::Message;
6+
use goose::providers::errors::ProviderError;
57
use goose::session::session_manager::{DB_NAME, SESSIONS_FOLDER};
68
use serde_yaml;
9+
use std::time::Duration;
710

811
fn print_aligned(label: &str, value: &str, width: usize) {
912
println!(" {:<width$} {}", label, value, width = width);
@@ -32,7 +35,74 @@ fn check_path_status(path: &Path) -> String {
3235
}
3336
}
3437

35-
pub fn handle_info(verbose: bool) -> Result<()> {
38+
struct ProviderCheckSuccess {
39+
provider: String,
40+
model: String,
41+
elapsed: Duration,
42+
}
43+
44+
enum ProviderCheckError {
45+
NotConfigured {
46+
label: &'static str,
47+
error: String,
48+
},
49+
InvalidModel(String),
50+
ProviderCreate {
51+
error: String,
52+
show_api_key_hint: bool,
53+
},
54+
ProviderRequest(ProviderError),
55+
}
56+
57+
async fn check_provider(
58+
config: &Config,
59+
) -> std::result::Result<ProviderCheckSuccess, ProviderCheckError> {
60+
let (provider, model) = match (config.get_goose_provider(), config.get_goose_model()) {
61+
(Ok(provider), Ok(model)) => (provider, model),
62+
(Err(e), _) => {
63+
return Err(ProviderCheckError::NotConfigured {
64+
label: "Provider:",
65+
error: e.to_string(),
66+
});
67+
}
68+
(_, Err(e)) => {
69+
return Err(ProviderCheckError::NotConfigured {
70+
label: "Model:",
71+
error: e.to_string(),
72+
});
73+
}
74+
};
75+
76+
let model_config = goose::model::ModelConfig::new(&model)
77+
.map_err(|e| ProviderCheckError::InvalidModel(e.to_string()))?
78+
.with_canonical_limits(&provider);
79+
80+
let provider_client = goose::providers::create(&provider, model_config, Vec::new())
81+
.await
82+
.map_err(|e| {
83+
let error = e.to_string();
84+
ProviderCheckError::ProviderCreate {
85+
show_api_key_hint: error.contains("not found") || error.contains("API_KEY"),
86+
error,
87+
}
88+
})?;
89+
90+
let test_msg = Message::user().with_text("Say 'ok'");
91+
let model_config = provider_client.get_model_config();
92+
let start = std::time::Instant::now();
93+
provider_client
94+
.complete(&model_config, "check", "", &[test_msg], &[])
95+
.await
96+
.map_err(ProviderCheckError::ProviderRequest)?;
97+
98+
Ok(ProviderCheckSuccess {
99+
provider,
100+
model,
101+
elapsed: start.elapsed(),
102+
})
103+
}
104+
105+
pub async fn handle_info(verbose: bool, check: bool) -> Result<()> {
36106
let logs_dir = Paths::in_state_dir("logs");
37107
let sessions_dir = Paths::in_data_dir(SESSIONS_FOLDER);
38108
let sessions_db = sessions_dir.join(DB_NAME);
@@ -90,5 +160,115 @@ pub fn handle_info(verbose: bool) -> Result<()> {
90160
}
91161
}
92162

163+
if check {
164+
println!("\n{}", style("Provider Check:").cyan().bold());
165+
166+
let result = check_provider(config).await;
167+
match &result {
168+
Ok(success) => {
169+
print_aligned("Provider:", &success.provider, label_padding);
170+
print_aligned("Model:", &success.model, label_padding);
171+
print_aligned("Auth:", &style("ok").green().to_string(), label_padding);
172+
print_aligned(
173+
"Connection:",
174+
&format!(
175+
"{} (verified in {:.1}s)",
176+
style("ok").green(),
177+
success.elapsed.as_secs_f64()
178+
),
179+
label_padding,
180+
);
181+
}
182+
Err(ProviderCheckError::NotConfigured { label, error }) => {
183+
print_aligned(
184+
label,
185+
&format!("{} {}", style("not configured:").red(), error),
186+
label_padding,
187+
);
188+
print_aligned(
189+
"Hint:",
190+
&format!("Run '{}'", style("goose configure").cyan()),
191+
label_padding,
192+
);
193+
}
194+
Err(ProviderCheckError::InvalidModel(error)) => {
195+
print_aligned(
196+
"Model:",
197+
&format!("{} {}", style("invalid:").red(), error),
198+
label_padding,
199+
);
200+
}
201+
Err(ProviderCheckError::ProviderCreate {
202+
error,
203+
show_api_key_hint,
204+
}) => {
205+
// Split auth failures (missing/invalid credential) from provider
206+
// construction failures (unknown provider, malformed provider
207+
// config). Labeling the latter as "Auth: FAILED" misdirects
208+
// troubleshooting toward rotating API keys.
209+
if *show_api_key_hint {
210+
print_aligned(
211+
"Auth:",
212+
&format!("{} {}", style("FAILED").red().bold(), error),
213+
label_padding,
214+
);
215+
print_aligned(
216+
"Hint:",
217+
&format!(
218+
"Set the API key in your environment or run '{}'",
219+
style("goose configure").cyan()
220+
),
221+
label_padding,
222+
);
223+
} else {
224+
print_aligned(
225+
"Provider:",
226+
&format!("{} {}", style("FAILED").red().bold(), error),
227+
label_padding,
228+
);
229+
print_aligned(
230+
"Hint:",
231+
&format!(
232+
"Check the provider name and config, or run '{}'",
233+
style("goose configure").cyan()
234+
),
235+
label_padding,
236+
);
237+
}
238+
}
239+
Err(ProviderCheckError::ProviderRequest(error)) => match error {
240+
ProviderError::Authentication(_) => {
241+
print_aligned(
242+
"Auth:",
243+
&format!("{} {}", style("FAILED").red().bold(), error),
244+
label_padding,
245+
);
246+
print_aligned(
247+
"Hint:",
248+
&format!(
249+
"Check your API key or run '{}'",
250+
style("goose configure").cyan()
251+
),
252+
label_padding,
253+
);
254+
}
255+
_ => {
256+
print_aligned(
257+
"Check:",
258+
&format!("{} {}", style("FAILED").red().bold(), error),
259+
label_padding,
260+
);
261+
}
262+
},
263+
}
264+
265+
// Propagate non-zero exit status so automation (CI scripts, install
266+
// checks, health probes) can rely on `goose info --check` as a
267+
// pre-flight verifier.
268+
if result.is_err() {
269+
return Err(anyhow!("provider check failed"));
270+
}
271+
}
272+
93273
Ok(())
94274
}

0 commit comments

Comments
 (0)