fix: model-target chats 404, and worktree rmdir races misreported as branch conflicts
Two unrelated production failures with a shared symptom of an opaque error. Chat: FN-8869 hoisted the agent-existence check out of its else branch, so it ran even when the client supplied an explicit model pair. Model-target chats send the client-only sentinel `__fn_agent__`, which is never an agent row, so every one of them 404'd behind the generic "Failed to create chat session" toast. The agent is now required only when it is the source of model resolution. Self-healing: a failed `tip-already-merged` cleanup was rethrown and classified `branch-conflict-unrecoverable`, failing and pausing tasks whose branch was already an ancestor of the integration ref. Every one of the 78 logged parks carried a `git worktree remove --force` / `ENOTEMPTY rmdir node_modules` message -- a pnpm race, not a conflict. Cleanup failure now retries on the next sweep, and prune runs before removal so a stale registration stops causing the failure it would prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -485,8 +485,17 @@ export function registerChatRoutes(ctx: ApiRoutesContext, deps: ChatRouteDeps):
|
||||
throw badRequest("Both modelProvider and modelId must be provided together, or neither should be provided");
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:ChatSessionCreate 2026-08-11-09:38:
|
||||
A chat may target a MODEL rather than an agent. The client marks that case with the synthetic sentinel id `__fn_agent__` (`app/hooks/useChat.ts`), which is deliberately never persisted as an agent row, and always sends an explicit `modelProvider`/`modelId` pair alongside it.
|
||||
FN-8869 hoisted this lookup out of the `else` branch below so it ran unconditionally, which 404'd every model-target chat ("Agent __fn_agent__ not found") and surfaced as the generic "Failed to create chat session" toast.
|
||||
|
||||
The agent is REQUIRED only when it is the source of the model resolution. When the client supplies a complete model pair, a missing agent is not an error — that is the pre-FN-8869 contract, and the rest of the stack already treats the sentinel as legitimately agent-less (ChatManager tolerates a missing agent on send; the UI hides agent identity for it).
|
||||
Do not re-hoist this check: match on the supplied model pair rather than hardcoding the sentinel, so the route stays agnostic to the client's marker value.
|
||||
*/
|
||||
const agent = await agentStore.getAgent(agentId);
|
||||
if (!agent) {
|
||||
const hasClientModel = hasClientModelProvider && hasClientModelId;
|
||||
if (!agent && !hasClientModel) {
|
||||
throw notFound(`Agent ${agentId} not found`);
|
||||
}
|
||||
const settings = await scopedStore.getSettings();
|
||||
@@ -496,18 +505,22 @@ export function registerChatRoutes(ctx: ApiRoutesContext, deps: ChatRouteDeps):
|
||||
let resolvedModelId: string | null = null;
|
||||
let inheritedThinkingLevel: string | undefined;
|
||||
|
||||
if (hasClientModelProvider && hasClientModelId) {
|
||||
if (hasClientModel) {
|
||||
// Use client-provided model
|
||||
resolvedProvider = modelProvider!.trim();
|
||||
resolvedModelId = modelId!.trim();
|
||||
} else {
|
||||
// Resolve from agent's runtimeConfig.model
|
||||
const resolved = resolvePermanentAgentEffectiveModel(agent, settings);
|
||||
// Resolve from agent's runtimeConfig.model. `agent` is non-null here: the
|
||||
// guard above only tolerates a missing agent when a client model pair exists.
|
||||
const resolved = resolvePermanentAgentEffectiveModel(agent!, settings);
|
||||
resolvedProvider = resolved.provider ?? null;
|
||||
resolvedModelId = resolved.modelId ?? null;
|
||||
inheritedThinkingLevel = resolvePermanentAgentEffectiveThinkingLevel(agent, settings);
|
||||
inheritedThinkingLevel = resolvePermanentAgentEffectiveThinkingLevel(agent!, settings);
|
||||
}
|
||||
inheritedThinkingLevel ??= resolvePermanentAgentEffectiveThinkingLevel(agent, settings);
|
||||
// Agent-less model sessions inherit nothing — there is no role to inherit from.
|
||||
inheritedThinkingLevel ??= agent
|
||||
? resolvePermanentAgentEffectiveThinkingLevel(agent, settings)
|
||||
: undefined;
|
||||
|
||||
// Create the chat session with projectId for multi-project scoping
|
||||
const session = await chatStore.createSession({
|
||||
|
||||
@@ -4422,6 +4422,18 @@ export class SelfHealingManager extends SelfHealingGitEvidence {
|
||||
}
|
||||
let reclaimedCleanly = false;
|
||||
try {
|
||||
/*
|
||||
FNXC:SelfHealingReclaim 2026-08-11-09:38:
|
||||
Prune BEFORE attempting removal. A stale worktree registration (recorded path no longer on disk) makes
|
||||
`git worktree remove --force` fail outright, and this sweep used to prune only AFTER the remove — so a
|
||||
dangling registration reliably produced the very failure pruning would have prevented. Observed on this
|
||||
repo at 89 registered worktrees against 20 present on disk.
|
||||
*/
|
||||
await execAsync("git worktree prune", {
|
||||
cwd: this.options.rootDir,
|
||||
timeout: 120_000,
|
||||
maxBuffer: 10 * 1024 * 1024,
|
||||
}).catch(() => undefined);
|
||||
if (inspection.livePath && existsSync(inspection.livePath)) {
|
||||
await removeWorktree({
|
||||
rootDir: this.options.rootDir,
|
||||
@@ -4513,10 +4525,25 @@ export class SelfHealingManager extends SelfHealingGitEvidence {
|
||||
log.warn(`Failed tip-already-merged cleanup for ${task.id}: ${message}`);
|
||||
}
|
||||
|
||||
if (reclaimedCleanly) {
|
||||
continue;
|
||||
/*
|
||||
FNXC:SelfHealingReclaim 2026-08-11-09:38:
|
||||
A failed `tip-already-merged` cleanup is NOT a branch conflict and must never be escalated as one.
|
||||
This verdict means the tip is already an ancestor of the integration ref — the branch has nothing unique to
|
||||
lose — so the only thing that can fail here is filesystem housekeeping, in practice `git worktree remove
|
||||
--force` losing a race with pnpm writing into the worktree's `node_modules` (`ENOTEMPTY ... rmdir`).
|
||||
Rethrowing handed that rmdir race to the outer catch, which classified it `branch-conflict-unrecoverable`
|
||||
and FAILED + paused a task whose branch was already fully merged, with the misleading error "Task branch
|
||||
conflict: <branch> is not safely reclaimable". 78 such parks were logged in 16 days, every one carrying a
|
||||
worktree-removal message rather than any git conflict.
|
||||
|
||||
The reclaim is idempotent, so the correct response is to leave the row untouched and retry on the next
|
||||
sweep — which is what actually resolved these tasks minutes later anyway. Only `live-foreign` below is a
|
||||
real conflict verdict.
|
||||
*/
|
||||
if (!reclaimedCleanly) {
|
||||
log.warn(`tip-already-merged cleanup for ${task.id} did not complete — retrying on next sweep (not a branch conflict)`);
|
||||
}
|
||||
throw new Error(`tip-already-merged cleanup failed for ${task.id}`);
|
||||
continue;
|
||||
}
|
||||
if (inspection.kind === "live-foreign") {
|
||||
throw inspection.error;
|
||||
|
||||
Reference in New Issue
Block a user