feat(agents): global Agent defaults and restart-on-Save (slice 4/5) - #329
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 254351600e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| environment_keys: self.environment.keys().cloned().collect(), | ||
| } | ||
| } |
There was a problem hiding this comment.
When the user switches harnesses and deliberately re-enters the same model or effort used by the previous harness—such as gpt-4o or high—stale cannot distinguish that new input from a carried-over value and silently clears it during Save. This contradicts the stated intent that values entered with the new harness are retained; treat submitted values as explicit or include enough edit-state/CAS information to identify an actually carried-over value.
Useful? React with 👍 / 👎.
| const defaultProvider = | ||
| inherited?.provider || | ||
| (buzzAgent ? state.data?.agentDefaults?.provider : undefined); | ||
| const defaultModel = | ||
| inherited?.model || |
There was a problem hiding this comment.
For an agent with a hidden write-only selector such as GOOSE_PROVIDER, GOOSE_MODEL, BUZZ_AGENT_MODEL, or DATABRICKS_MODEL, a matching global default now supplies the “Use agent defaults (…)” provider/model hint even though the saved environment override determines the next launch. In particular, inherited?.model bypasses modelDefaultKnown entirely. Gate these inherited hints on the relevant environmentKeys and pending removal patches so the editor does not misstate the effective configuration.
Useful? React with 👍 / 👎.
| if let Some((eligible, refusal)) = guard { | ||
| if !host | ||
| .controller | ||
| .snapshot()? | ||
| .agents |
There was a problem hiding this comment.
When credential access blocks on an OS prompt and the listener exits after this check, the callback resumes without evaluating needs_save_restart again and invokes Action::Restart, resurrecting an agent that is no longer running. The promised locked late guard currently runs only before credentials.read; repeat it after credential acquisition, while holding the host lock, immediately before action_with_key.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
Three actionable P2 findings, detailed inline: recovery Stop is disabled during Save-triggered credential waits; automatic-restart failures are hidden by successful save feedback; inherited Databricks workspace/filter settings do not traverse the full Browse path. These are non-blocking comments; human reviewers decide disposition.
Reviewed head 254351600ee66dea7f3d156c40058bdd46dd63d8 against base/merge-base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. Read the 34-file diff, applicable instructions/product contracts, native persistence/effective-setting/restart lifecycle, editor/model-discovery callers, and regression-test source. Source blobs were hash-verified; no dirty working-tree inputs or delegates.
Validation: source analysis only—no PR code, tests, app, credential prompts, or live services executed. A single hosted exact-head snapshot showed CI required, JavaScript, Rust/tool integration, browser measurements, all six browser journey shards, DCO and security checks successful; Windows native validation was skipped. Those results do not establish native restart/Keychain behavior, Goose/Pi live launches, or human acceptance. The findings above are source-demonstrated, not runtime reproductions.
| if (!native.saveDefaults) | ||
| throw new Error("Agent defaults are unavailable."); | ||
| return native.saveDefaults(edit); | ||
| }, ready), |
There was a problem hiding this comment.
[P2] Keep recovery Stop available while Save waits for restart credentials
Both save and saveDefaults still call run(..., ready) without a pending-launch/credential marker, although the new native save_and_restart now awaits start_guarded and its OS credential read. During that wait busy is true, so canStopAgent (lines 248–253) returns false for every agent; the editor also disables Close because neither pending marker is set. A user saving a running agent can therefore no longer Stop its existing work or cancel the queued restart through the app while the Keychain prompt remains unresolved, even though native Stop already fences that ticket. Treat both save operations as interruptible credential/restart work, preserving the existing superseded-result/busy fencing, and add deferred-save tests that admit Stop and reject the late save result.
| None, | ||
| Some((needs_save_restart, "Agent no longer needs a save restart")), | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
[P2] Surface automatic-restart failures separately from a successful save
This loop discards every Err from start_guarded and every successful snapshot whose target is not running, then returns success with only a restart count. A credential refusal or an invalid new launch configuration can leave the old agent running with stale settings, or stop it and fail to replace it, but Settings → Agents reports only “Saved.” (or “Saved. Restarted N agents.”). That page does not render snapshot.data.agents[*].error, so the person changing device-wide defaults gets no indication that agents failed to adopt them. Preserve per-agent restart failures in the save result and show a partial-success warning in the defaults card/editor; distinguish benign no-longer-needed/explicit-Stop skips from actual restart failures. Cover a failed restart after a successful persistence write.
| self.with(|host| match (id, revision) { | ||
| (Some(id), Some(revision)) => host.controller.model_context(id, revision, edit), | ||
| (None, None) => Controller::draft_model_context(edit), | ||
| (None, None) => Controller::draft_model_context(host.controller.effective_draft(edit)?), |
There was a problem hiding this comment.
[P2] Carry inherited model context through the complete Browse path
Resolving global defaults here is insufficient for the existing model-request callers. With a global DATABRICKS_HOST and no build/per-agent host, AgentModelPicker still derives host only from draft.databricks or compiled defaults (lines 42–43), then returns before IPC when it is blank (98–103). If the compiled host or filter differs from the inherited environment, agent_models::resolve (213–222) instead rejects the request as a conflict. Consequently Create/Edit cannot browse using the newly inherited workspace/filter without manually repeating those write-only defaults in each form, despite native launch using them. Integrate inheritance with the full UI/native request contract while retaining native-only environment values and explicit-conflict checks; add a test through Browse/agent_models_run, not just the model_context helper.
|
Tested |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: four P2 correctness findings, detailed inline. The default-selection and recovery paths do not yet meet the 9/10 bar. Merge criteria: resolve those four findings and add their targeted regressions; no redesign of the documented inheritance rules is requested.
Reviewed 43bae5fde5733c5b371236293b8b4a72623b5ce7 against 85d6bf82c54d1c8d930d58444597a1fe31cc8975, including the existing review fixes. Recovery Stop admission, late restart eligibility, restart-failure counts, and inherited Browse are addressed; the inline findings are remaining cases.
Validation: required CI is green at this head. I ran four focused mounted-UI/controller diagnostic probes against unchanged production source, with synthetic host responses; they reproduce the requests, misleading hint, and save-status behavior described inline. Native failure consequences were source-traced. I did not exercise a real desktop Save → supervisor/Keychain restart or live inherited Goose/Pi launch. Those acceptance gaps remain explicitly unverified, not covered by the focused probes or CI. No internal-information leaks or accidental review media were found in the full PR diff and current description.
| return Err(format!("{label} is too long or invalid")); | ||
| } | ||
| } | ||
| crate::config::validate_environment(&self.environment) |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
[P2] Reject an invalid Pi default selection before committing it
With a running Pi agent whose provider/model are blank, start with Buzz Agent defaults containing a provider, switch the defaults card to Pi, and Save. The card clears model/effort but retains the provider; this validation accepts that provider-only Pi configuration. It becomes the agent's effective settings and triggers automatic Restart. Controller::action_with_key stops the existing process before PiContext::adapter_args calls validate_selection, which rejects a nonempty provider with an empty model. A harness-only defaults change therefore takes the previously working Pi agent down and prevents subsequent starts until defaults are repaired.
Validate this Pi selection before persistence/restart, with actionable feedback, or prevent the form from submitting that invalid pair. Cover the harness-switch case with a running blank-selector Pi agent; it must not be stopped for a configuration already known to be invalid. The submitted pair was reproduced in mounted UI; the stop-before-validation consequence is source-traced, not a live Pi run.
| ...(!external && | ||
| ((inheritedWorkspace?.host && !host) || | ||
| (inheritedWorkspace?.filter && !filter)) | ||
| ? { inheritWorkspace: true } |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
[P2] Resolve inherited workspace identity for Disconnect too
Set global DATABRICKS_HOST, leave the agent's workspace unset, and Browse/sign in; then, with agents using that workspace stopped, choose Disconnect. The picker sends host: "", inheritWorkspace: true, and no edit. Unlike Browse/Refresh, native Disconnect bypasses inheritance and calls origin(&request.host) directly (agent_models.rs:344–351), so it fails with “Databricks workspace is not configured” before removing this app's credentials. The response host is deliberately hidden, so the UI cannot recover the URL from the catalog either.
Carry a native-resolvable workspace identity through Disconnect without exposing the write-only value, while preserving recovery when unrelated draft/provider settings are invalid. Add a Browse → Disconnect regression for an inherited workspace. A mounted-UI probe reproduced the empty-host request at this head; rejection is established by the native source path.
| setNewValue(""); | ||
| setNotice(savedMessage(snapshot.restarted, snapshot.restartFailures)); | ||
| }, | ||
| () => setError("Agent defaults weren’t saved. Try again."), |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
[P2] Preserve the uncertain-write outcome instead of claiming nothing saved
Save persists defaults before awaiting restart credentials. If recovery Stop overtakes that wait, control.run deliberately rejects the superseded result, even though the defaults are already committed. This catch converts its “Could not confirm” explanation into “Agent defaults weren’t saved. Try again.” A mounted card/controller probe reproduced that message while the saved model had already changed. It tells the user that device-wide changes did not apply and encourages replay instead of checking the committed settings.
Display the sanitized error supplied by the controller, as the agent editor does, and retain the refresh/check-saved-settings guidance. This also preserves the useful native validation reason for invalid/reserved environment names, currently discarded by the same catch. Cover committed Save → recovery Stop → late completion in the card, not only in the controller.
| ? undefined | ||
| : inherited?.provider || | ||
| (buzzAgent ? state.data?.agentDefaults?.provider : undefined); | ||
| const defaultModel = modelHidden |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
[P2] Include the inherited provider when choosing the build-model hint
For Buzz Agent with blank per-agent provider/model, set global provider to anthropic and leave the global model blank, on a build whose provider/model floor is Databricks. The provider field correctly says “Use agent defaults (anthropic)”, but this fallback shows the Databricks build model: buzzProvider above consults only the draft and build provider, skipping inherited.provider. Native applies the inherited provider first, so BuildDefaults::resolve does not select that Databricks model. The reverse case also hides a build model that will actually be used.
Determine the effective visible provider using draft → same-harness global default → build floor before deciding whether to show the build model, retaining the environment-redaction guards. Add both cross-provider cases to the hint matrix. The misleading anthropic/Databricks combination was reproduced in a mounted component at this head.
814932e to
d732963
Compare
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…harness change Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…ompt Signed-off-by: Salman Mohammed <smohammed@squareup.com>
… credentials Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…owse Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…wse host Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…aults Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
A denied credential prompt or failed stop returns a snapshot with the old process still running, which was reported as "Restarted". Require an empty restart diff so it is reported as a restart failure instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
d732963 to
5afe9c8
Compare
#326 asserted one filter per channel, but #325 reads each batch with a single filter whose #h holds the whole batch, so main's CI is red. Same correction as the other open PRs carry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com> # Conflicts: # src-tauri/src/agents.rs
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: all four prior P2 findings are resolved; no remaining code blockers found in the fixes or latest supervisor merge.
Reviewed head 7f4e4ba9d596df0e46e2101232b47019d4f6b1e2 against base 1108ab64cf232acfc0c8087db54b74ce73098bfe, focused on the findings from my previous review.
- Pi provider-only defaults now fail before persistence and restart. Restart accounting requires the saved settings to be running; Stop/late-eligibility guards remain intact.
- Inherited-workspace Disconnect resolves its host natively without a draft and keeps the host out of IPC output.
- The defaults card preserves uncertain-save guidance, and the model hint respects the same-harness inherited provider.
Verification: source and regression-test inspection, including the merge resolution; no local tests or live app runs in this re-review. Current-head CI was still running when checked. The PR’s reported local passes belong to 5afe9c82, not this head. Live supervisor/Keychain restart, inherited Goose/Pi launches and human validation remain outstanding, not established by this review. One optional UI validation improvement is noted inline; it does not reopen the prior blockers or change the agreed inheritance rules.
Unchanged limitation, deferred under the agreed scope: removing a global Pi model that an agent-owned provider relies on can still yield an invalid effective pair and a failed restart. The fix rejects invalid defaults pairs; it does not redesign independent inheritance or add per-agent preflight for every defaults change.
| const [error, setError] = useState(""); | ||
| if (!saved || !control.saveDefaults) return null; | ||
| const current = draft ?? draftFrom(saved); | ||
| const disabled = state.busy || state.status !== "ready"; |
There was a problem hiding this comment.
Optional, non-blocking: switching from a defaults harness with a provider to Pi clears the model but keeps the provider. Native now correctly refuses this pair before saving, but the shared controller enters status: "error", disabling the card until Check again. Consider validating this pair in the card before submitting, with an actionable message while leaving the fields editable, and covering correction/retry in a mounted test. Keep native validation as the backstop; clearing the provider on harness change is not required.
Slice 4/5 of the Harnesses stack, following #272, #277 and #279: global Agent defaults and restart-on-Save.
Design
defaults.jsonunder app-dataagent-controller/, written atomically with mode 0600. Writes and reads both enforce the 1 MiB limit; an oversized edit leaves the previous record intact.agent_defaults.rs, validates the record.nullremoves it, a string replaces it.agent_defaults::effective)DATABRICKS_HOST/DATABRICKS_MODEL_FILTERpair; an agent without its own pair inherits the global values.BuildDefaults::resolve()still runs and fills Buzz Agent blanks only. So the order is: agent value, then global default, then build floor.BUZZ_ACP_MODELpaths. Unsaved Create model-browsing drafts also resolve the native defaults before discovery.high), sent asBUZZ_ACP_EFFORT_LEVEL. It applies only when the agent uses the default harness.effort_levelstays the per-agent override.restartDiffnow compares effective launch selectors, build floor, environment overrides and effort. Environment-derived selector values stay native and are masked in the IPC diff. Changes to global Databricks host/filter do not restart an agent with its own workspace/filter.inheritWorkspaceonly when no per-agent workspace/filter is set, native fills them in, and returns a blank catalog host for inherited requests. An explicit workspace wins and differing values still conflict. Disconnect resolves an inherited workspace natively without the draft and returns a blank host. The build-model hint follows draft, then same-harness default, then build provider. The defaults card shows the controller's sanitized error, including the uncertain-save guidance when Stop overtakes a committed save. Discard also clears unfinished write-only environment input.agent-control.mdnow describes this as current behaviour (settings.mdwas removed on main in Remove local project context from docs #350). Pi one-click install is still planned.Checks (on
5afe9c82c32c6c230ed4b31f7085832137154771, rebased on main1f71ee94)pnpm check,cargo fmt --check, andcargo clippy --workspace --all-targets -D warnings(workspace andsrc-tauri) all pass.buzz-agent-controller: 93 + 5 + 11 passed (one library test ignored).src-tauri --libwith--test-threads=1: 118 passed (three ignored).src/app,src/bundled/agentsandsrc/features/agents: 426 passed. I didn't rerun the full Vitest suite or the browser specs after the rebase; they passed on the previous head.Not verified
running_settings). The Tauri-level tests cover the stopped path, disabled/late-restart guard and unsaved Create model context. A live Tauri-level supervisor restart is still absent.