mirror of
https://github.com/block/buzz.git
synced 2026-11-08 02:11:36 +00:00
fix(agents): wire publish-only retry seam and remove pooled-instance name suppression
IMPORTANT 1: When save+publish succeeds on disk but the publication step throws, the coordinator now attempts a publish-only retry via set_persona_shared(id, true) — reads the current on-disk definition without writing any field changes. On retry success the coordinator falls through to full success and closes normally. On retry failure (or no seam provided), reports a terminal 'saved locally, not published to the catalog — it will be retried automatically' without the false 'reopen to retry publishing' copy that the prior pass showed. The generic 'some changes may not have persisted' setSaveError message in useAgentEditMergedSubmit is also updated to avoid implying persistence is unverified on this branch where it is proven. IMPORTANT 2: Remove the hasNamePool suppression from nameChanged in agentFormModel.ts and the pool exemption from the canSubmit instanceName blank-guard in AgentEditMergedDialog.tsx. The suppression was not load-bearing for the definition-rename safeguard: when only a D-field changes, seeded instanceName already equals inst.name so the diff is already false without it. Removing the guard lets pooled instances be explicitly renamed or cleared the same as non-pooled, with the canSubmit blank guard blocking submission on a cleared pooled name. Regressions added: - Coordinator family 11 (3 tests): retry succeeds → full success; retry fails → terminal warning without 'reopen to retry'; no-seam → same terminal - agentFormModel: test_pooled_explicit_rename_emits_agentInput (mutation-a RED), test_pooled_clear_emits_agentInput (mutation-a RED), test_cleared_instance_name_emits_empty_not_displayName_fallback (mutation-b RED) - Family-10 assertions updated to match the new terminal toast copy TS suite: 5904/5904 pass (6 new tests added) Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
@@ -567,9 +567,11 @@ export function AgentEditMergedDialog({
|
||||
(selectedRuntimeId !== "custom" ||
|
||||
inheritHarness ||
|
||||
agentCommand.trim().length > 0) &&
|
||||
// Block submission when the instance name field controls the name and
|
||||
// the user has left it blank (no name pool — pool-named instances skip).
|
||||
((def?.namePool?.length ?? 0) > 0 || instanceName.trim().length > 0)
|
||||
// Block submission when the instance name field is blank — applies to
|
||||
// all instances including pool-named ones. Pooled instances can be
|
||||
// explicitly renamed; the blank guard prevents submitting an empty name
|
||||
// regardless of pool membership.
|
||||
instanceName.trim().length > 0
|
||||
: true);
|
||||
|
||||
// ── Runtime/model/provider handlers (extracted to useAgentEditRuntimeHandlers) ──
|
||||
|
||||
@@ -117,6 +117,112 @@ test("test_pooled_name_instance_name_not_materialized_even_when_names_differ", (
|
||||
);
|
||||
});
|
||||
|
||||
// ── Regression test 1b: pooled instances — explicit name change emits (Carl round-5, IMPORTANT 2) ──
|
||||
//
|
||||
// The hasNamePool guard previously suppressed ALL instance-name diffs when the
|
||||
// definition had a name pool. This made it impossible for a user to rename their
|
||||
// pooled instance: the change would be silently swallowed. The guard is NOT
|
||||
// load-bearing for the definition-rename safeguard — when only a D-field
|
||||
// changes, instanceName stays equal to inst.name in the seed, so the diff is
|
||||
// already false without it. Removing the guard allows explicit pooled-instance
|
||||
// renames to reach the backend.
|
||||
|
||||
test("test_pooled_explicit_rename_emits_agentInput", () => {
|
||||
// A pool-named instance: user explicitly changes the visible instance name.
|
||||
const definition = makeDefinition({
|
||||
displayName: "Bot",
|
||||
namePool: ["Scout", "Ranger"],
|
||||
});
|
||||
const instance = makeInstance({ name: "Scout" });
|
||||
|
||||
const ctx = { kind: "instance-with-definition", definition, instance };
|
||||
const saved = seedAgentFormModel(ctx);
|
||||
|
||||
// User renames the pooled instance — sets instanceName to a new value.
|
||||
const next = { ...saved, instanceName: "Scout-alpha" };
|
||||
const emit = emitAgentFormDiff(saved, next, ctx);
|
||||
|
||||
// D-field unchanged — no definition update.
|
||||
assert.equal(
|
||||
emit.personaInput,
|
||||
null,
|
||||
"pooled rename must not update the definition",
|
||||
);
|
||||
// I-field: explicit rename must emit agentInput with the new name.
|
||||
assert.ok(
|
||||
emit.agentInput,
|
||||
"explicit pooled instance rename must emit agentInput",
|
||||
);
|
||||
assert.equal(
|
||||
emit.agentInput.name,
|
||||
"Scout-alpha",
|
||||
"agentInput.name must equal the new instance name",
|
||||
);
|
||||
});
|
||||
|
||||
test("test_pooled_clear_emits_agentInput", () => {
|
||||
// Clearing the pooled instance name results in nameChanged=true (the canSubmit
|
||||
// gate blocks blank names, but the model emits the diff regardless — the gate
|
||||
// prevents submission before we reach emit, but the model must still be
|
||||
// correct about what changed if it were somehow reached).
|
||||
const definition = makeDefinition({
|
||||
displayName: "Bot",
|
||||
namePool: ["Scout", "Ranger"],
|
||||
});
|
||||
const instance = makeInstance({ name: "Scout" });
|
||||
|
||||
const ctx = { kind: "instance-with-definition", definition, instance };
|
||||
const saved = seedAgentFormModel(ctx);
|
||||
|
||||
// User clears the pooled instance name (empty string).
|
||||
const next = { ...saved, instanceName: "" };
|
||||
const emit = emitAgentFormDiff(saved, next, ctx);
|
||||
|
||||
// The clear emits as a name change (empty string != "Scout").
|
||||
assert.ok(
|
||||
emit.agentInput,
|
||||
"a cleared pooled instance name must emit agentInput (canSubmit gate blocks it before submission)",
|
||||
);
|
||||
assert.equal(
|
||||
emit.agentInput.name,
|
||||
"",
|
||||
"emitted name must be the cleared empty string",
|
||||
);
|
||||
});
|
||||
|
||||
test("test_cleared_instance_name_emits_empty_not_displayName_fallback", () => {
|
||||
// When instanceName is cleared (undefined in the model, from buildNextAgentFormModel
|
||||
// returning undefined for a trimmed empty string), the emit must use "" NOT
|
||||
// fall back to displayName.trim(). The old fallback (next.instanceName ??
|
||||
// next.displayName.trim()) silently renamed the instance to the definition name
|
||||
// instead of treating the clear as an intended empty.
|
||||
const definition = makeDefinition({
|
||||
displayName: "Alice",
|
||||
namePool: [],
|
||||
});
|
||||
const instance = makeInstance({ name: "Alice" });
|
||||
|
||||
const ctx = { kind: "instance-with-definition", definition, instance };
|
||||
const saved = seedAgentFormModel(ctx);
|
||||
|
||||
// Simulate buildNextAgentFormModel with an empty instanceName field:
|
||||
// " ".trim() || undefined = undefined.
|
||||
const next = { ...saved, instanceName: undefined };
|
||||
const emit = emitAgentFormDiff(saved, next, ctx);
|
||||
|
||||
// A cleared instanceName (undefined) should emit agentInput.name = "" (the
|
||||
// intended "clear"), not fall back to displayName.trim() = "Alice".
|
||||
assert.ok(
|
||||
emit.agentInput,
|
||||
"undefined instanceName != inst.name must emit agentInput",
|
||||
);
|
||||
assert.equal(
|
||||
emit.agentInput.name,
|
||||
"",
|
||||
"cleared instanceName must emit empty string, not the definition displayName",
|
||||
);
|
||||
});
|
||||
|
||||
test("test_no_name_pool_explicit_instance_name_change_emits_agentInput", () => {
|
||||
// Without a name pool, explicitly editing the instanceName field should
|
||||
// produce an agentInput with the new name.
|
||||
|
||||
@@ -540,12 +540,13 @@ export function emitAgentFormDiff(
|
||||
|
||||
const nameChanged =
|
||||
isI("instanceName") &&
|
||||
!hasNamePool &&
|
||||
// Use the raw instanceName value — do not fall back to displayName when
|
||||
// instanceName is empty string. An empty instanceName means the user
|
||||
// deliberately cleared the field; the canSubmit gate blocks submission
|
||||
// when it is blank, so if we reach here with "" the instance is
|
||||
// definition-only and has no name change to emit.
|
||||
// Pool-named instances: the hasNamePool guard previously suppressed every
|
||||
// instanceName diff, but it was not load-bearing for the definition-rename
|
||||
// case: when only a D-field changes the seeded instanceName still equals
|
||||
// inst.name, so the diff is already false without the guard. Removing the
|
||||
// guard lets an explicit user rename/clear on a pooled instance emit
|
||||
// correctly. The canSubmit gate blocks blank names (pool or not), so an
|
||||
// accidental clear cannot reach this path with an empty string.
|
||||
(next.instanceName ?? "") !== inst.name;
|
||||
const systemPromptChanged =
|
||||
isI("systemPrompt") &&
|
||||
|
||||
@@ -1631,8 +1631,8 @@ test("test_publish_throws_post_persist_does_not_settle_as_full_success", async (
|
||||
assert.equal(warnings.length, 1, "exactly one warning toast");
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/saved.*publishing.*failed|publishing.*failed|not published/i,
|
||||
"warning must indicate the persona saved but publication failed",
|
||||
/saved locally.*not.*published|could not be published|saved.*not published/i,
|
||||
"warning must indicate the persona saved locally but publication failed",
|
||||
);
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
@@ -1680,7 +1680,7 @@ test("test_publish_throws_post_persist_returns_false_not_partial_failure_toast",
|
||||
);
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/publishing.*failed|catalog.*failed|not published/i,
|
||||
/could not be published|saved locally.*not.*published|not published/i,
|
||||
"warning must indicate publishing was the failure, not the save itself",
|
||||
);
|
||||
// Must NOT use the partial-failure wording "profile saved. profile failed"
|
||||
@@ -1693,3 +1693,159 @@ test("test_publish_throws_post_persist_returns_false_not_partial_failure_toast",
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
// ── Test family 11: P1-1 publish retry seam (Carl round-5 pass-2) ─────────────
|
||||
//
|
||||
// When the initial save+publish command throws after the persona is on disk, the
|
||||
// coordinator now attempts a publish-only retry via publishRetry(def.id) before
|
||||
// reporting failure. This keeps the toast copy honest:
|
||||
//
|
||||
// - If the retry succeeds: full-success path fires — onDone is called and the
|
||||
// success toast names the persona and publication status.
|
||||
// - If the retry also fails: terminal warning without "reopen to retry"
|
||||
// (the flush loop holds the enqueued head and will retry automatically).
|
||||
// - If no publishRetry seam is provided: same terminal warning, no false
|
||||
// "reopen to retry" copy.
|
||||
//
|
||||
// The existing family-10 tests remain unchanged: they verify the publishFailed
|
||||
// flag is set and onDone is not called on the initial throw. These family-11
|
||||
// tests cover the retry layer on top.
|
||||
|
||||
test("test_publish_retry_succeeds_settles_as_full_success", async () => {
|
||||
// Initial updatePersonaAndPublish throws (retain step failed), but the
|
||||
// persona IS observed as persisted. publishRetry succeeds → coordinator
|
||||
// should call onDone and return true.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persisted = makeDefinition({ displayName: "Alice" });
|
||||
let retryCalls = 0;
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "definition-only",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("failed to open retention db");
|
||||
},
|
||||
refetchStores: async () => ({ persona: persisted, agent: null }),
|
||||
publishRetry: async () => {
|
||||
retryCalls++;
|
||||
return { persona: persisted, publicationStatus: "published" };
|
||||
},
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(
|
||||
result,
|
||||
true,
|
||||
"coordinator must return true when the publish retry succeeds",
|
||||
);
|
||||
assert.equal(
|
||||
opts._calls.onDone,
|
||||
1,
|
||||
"onDone must be called on retry success",
|
||||
);
|
||||
assert.equal(retryCalls, 1, "publishRetry must be called exactly once");
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 0, "no warning toast when retry succeeds");
|
||||
const successes = cap.captured.filter((c) => c.kind === "success");
|
||||
assert.equal(successes.length, 1, "success toast must fire after retry");
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test("test_publish_retry_failure_shows_terminal_warning_not_reopen", async () => {
|
||||
// Both the initial publish and the retry fail — should report a terminal
|
||||
// state without "reopen to retry publishing".
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persisted = makeDefinition({ displayName: "Alice" });
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "definition-only",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("relay unreachable: connection refused");
|
||||
},
|
||||
refetchStores: async () => ({ persona: persisted, agent: null }),
|
||||
publishRetry: async () => {
|
||||
throw new Error("relay still unreachable");
|
||||
},
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(
|
||||
result,
|
||||
false,
|
||||
"coordinator must return false when retry also fails",
|
||||
);
|
||||
assert.equal(opts._calls.onDone, 0, "onDone must not be called");
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 1, "exactly one warning toast");
|
||||
// Must say "saved locally" or similar — NOT "reopen to retry".
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/saved locally|saved.*not.*published|could not be published/i,
|
||||
"warning must indicate saved locally but not published",
|
||||
);
|
||||
assert.doesNotMatch(
|
||||
warnings[0].message,
|
||||
/reopen to retry publishing/i,
|
||||
"terminal state must not tell the user to reopen — that is a lie",
|
||||
);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test("test_no_publish_retry_seam_shows_terminal_warning_not_reopen", async () => {
|
||||
// No publishRetry provided — coordinator must still not say "reopen to retry".
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persisted = makeDefinition({ displayName: "Alice" });
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "definition-only",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("failed to open retention db");
|
||||
},
|
||||
refetchStores: async () => ({ persona: persisted, agent: null }),
|
||||
// publishRetry intentionally omitted (no seam available).
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(
|
||||
result,
|
||||
false,
|
||||
"coordinator must return false with no retry seam",
|
||||
);
|
||||
assert.equal(opts._calls.onDone, 0, "onDone must not be called");
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 1, "exactly one warning toast");
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/saved locally|saved.*not.*published|could not be published/i,
|
||||
"must indicate saved locally not published",
|
||||
);
|
||||
assert.doesNotMatch(
|
||||
warnings[0].message,
|
||||
/reopen to retry publishing/i,
|
||||
"no-seam terminal state must not say reopen to retry",
|
||||
);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
@@ -79,6 +79,16 @@ export type SaveCoordinatorOptions = {
|
||||
updatePersonaAndPublish: (
|
||||
input: UpdatePersonaInput,
|
||||
) => Promise<PersonaSharePublicationResult>;
|
||||
/**
|
||||
* Publish the current on-disk definition to the catalog without writing any
|
||||
* field changes. Called when the initial save+publish succeeded on disk but
|
||||
* the publication step threw — the persona is persisted, so the retry only
|
||||
* needs to re-attempt the relay submission. Uses the share-toggle strict
|
||||
* path (`set_persona_shared`) which reads the definition from disk, so no
|
||||
* field payload is needed. If omitted the coordinator falls back to a
|
||||
* terminal "saved locally, not published" report.
|
||||
*/
|
||||
publishRetry?: (personaId: string) => Promise<PersonaSharePublicationResult>;
|
||||
updateManagedAgent: (
|
||||
input: UpdateManagedAgentInput,
|
||||
) => Promise<{ agent: ManagedAgent; profileSyncError: string | null }>;
|
||||
@@ -122,6 +132,7 @@ export async function runAgentSaveCoordinator(
|
||||
updateManagedAgent,
|
||||
setAutoRestart,
|
||||
setStartOnAppLaunch,
|
||||
publishRetry,
|
||||
refetchStores,
|
||||
onDone,
|
||||
onSavedWhileStopped,
|
||||
@@ -418,18 +429,46 @@ export async function runAgentSaveCoordinator(
|
||||
|
||||
if (!observedRemainder && publishFailed) {
|
||||
// The persona persisted but the publish command threw before returning a
|
||||
// status — the relay was never reached. "Save and publish" did not deliver
|
||||
// its promise: report "saved but not published" and keep the dialog open so
|
||||
// the user can retry (re-submitting will re-attempt the publish path).
|
||||
// status — the relay was never reached. Attempt a publish-only retry via
|
||||
// set_persona_shared (reads the current on-disk definition; no field write
|
||||
// needed). If the retry succeeds, fall through to full success. If it
|
||||
// fails (or no retry seam is available), report a terminal "saved locally,
|
||||
// not published" state — the flush loop will re-attempt on reconnect, and
|
||||
// the user can re-open and save again to force a retry.
|
||||
const personaName =
|
||||
observedPersona?.displayName ??
|
||||
personaInput?.displayName ??
|
||||
def?.displayName ??
|
||||
"Agent";
|
||||
toast.warning(
|
||||
`${personaName} saved, but publishing to the catalog failed: ${firstError ?? "unknown error"} — reopen to retry publishing.`,
|
||||
);
|
||||
return false;
|
||||
if (publishRetry && def) {
|
||||
let retryError: string | null = null;
|
||||
try {
|
||||
const retryResult = await publishRetry(def.id);
|
||||
publicationStatus = retryResult.publicationStatus;
|
||||
// Retry succeeded — the persona is published. Clear publishFailed and
|
||||
// fall through to the full-success branch below.
|
||||
publishFailed = false;
|
||||
} catch (err) {
|
||||
retryError = err instanceof Error ? err.message : "unknown error";
|
||||
}
|
||||
if (publishFailed) {
|
||||
// Retry also failed — terminal state: the relay was not reached in
|
||||
// either attempt. The flush loop holds the enqueued head and will
|
||||
// re-publish on reconnect. Surface an accurate message without
|
||||
// implying the user can fix it by reopening.
|
||||
toast.warning(
|
||||
`${personaName} saved locally, but could not be published to the catalog: ${retryError ?? firstError ?? "unknown error"} — it will be retried automatically.`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
// publishFailed is now false — fall through to full success.
|
||||
} else {
|
||||
// No retry seam available — terminal state (same as retry failure).
|
||||
toast.warning(
|
||||
`${personaName} saved locally, but could not be published to the catalog: ${firstError ?? "unknown error"} — it will be retried automatically.`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
if (!observedRemainder) {
|
||||
|
||||
@@ -24,6 +24,7 @@ import type {
|
||||
UpdateManagedAgentInput,
|
||||
UpdatePersonaInput,
|
||||
} from "@/shared/api/types";
|
||||
import { setPersonaShared } from "@/shared/api/tauriPersonas";
|
||||
import type { PersonaSharePublicationResult } from "@/shared/api/tauriPersonas";
|
||||
import {
|
||||
seedAgentFormModel,
|
||||
@@ -286,6 +287,12 @@ export function useAgentEditMergedSubmit(
|
||||
runtimes: s.runtimes.length > 0 ? s.runtimes : undefined,
|
||||
updatePersona: s.updatePersona,
|
||||
updatePersonaAndPublish: s.updatePersonaAndPublish,
|
||||
// Publish-only retry: when save+publish succeeds on disk but the
|
||||
// publication step throws, re-publish the current on-disk head via
|
||||
// set_persona_shared without writing any field changes. The persona
|
||||
// is already shared=true (it was shared when the save+publish path
|
||||
// was reached), so passing true re-publishes the current head.
|
||||
publishRetry: (id: string) => setPersonaShared(id, true),
|
||||
updateManagedAgent: async (upd) => {
|
||||
if (!inst)
|
||||
throw new Error("No instance in definition-only context");
|
||||
@@ -317,8 +324,13 @@ export function useAgentEditMergedSubmit(
|
||||
});
|
||||
|
||||
if (!success) {
|
||||
// The coordinator already reported the specific failure via toast.
|
||||
// This error banner is a secondary indicator for users who miss the
|
||||
// toast — use an accurate summary that covers all false-return paths.
|
||||
setSaveError(
|
||||
new Error("Some changes may not have persisted. Reopen to retry."),
|
||||
new Error(
|
||||
"Some changes may not have fully saved — see the notification above for details.",
|
||||
),
|
||||
);
|
||||
}
|
||||
if (success && inst) {
|
||||
|
||||
Reference in New Issue
Block a user