fix(electrobun): address all 20 Codex review findings
CRITICAL: - PTY leak: Terminal.svelte now calls pty.close on destroy, not just unsubscribe - Agent session cleanup: clearSession() removes done/error sessions, backend deletes after 60s grace period HIGH: - Clone branch passthrough: user's branch name flows through callback - Circular imports: extracted rpc.ts singleton, broke main.ts ↔ App.svelte cycle - Settings wired to runtime: Terminal reads cursor/scrollback from settings - Security disclaimer: added "prototype — not system keyring" notice - ThemeEditor: fixed basePalette → initialPalette reference MEDIUM: - Clone race: UUID suffix instead of count-based index - Silent failures: structured error returns from PTY handlers - WebKitGTK mount: only current + previous group mounted - Debug listeners: gated behind DEBUG, cleanup on destroy - NDJSON residual buffer parsed on process exit - Codex adapter: deduplicated tool_call/tool_result - extraEnv: rejects CLAUDE*/CODEX*/OLLAMA* keys - settings-db: runMigrations() with version tracking - active_group: persisted via settings.set LOW: - Removed dead demo code, unused variables - color-mix() fallbacks added
This commit is contained in:
Hibryda
parent
ef0183de7f
commit
29a3370e79
18 changed files with 331 additions and 114 deletions
|
|
@ -73,12 +73,16 @@ const rpc = BrowserView.defineRPC<PtyRPCSchema>({
|
|||
},
|
||||
|
||||
"pty.write": ({ sessionId, data }) => {
|
||||
if (!ptyClient.isConnected) return { ok: false };
|
||||
if (!ptyClient.isConnected) {
|
||||
console.error(`[pty.write] ${sessionId}: daemon not connected`);
|
||||
return { ok: false };
|
||||
}
|
||||
try {
|
||||
ptyClient.writeInput(sessionId, data);
|
||||
return { ok: true };
|
||||
} catch (err) {
|
||||
console.error(`[pty.write] ${sessionId}:`, err);
|
||||
const error = err instanceof Error ? err.message : String(err);
|
||||
console.error(`[pty.write] ${sessionId}: ${error}`);
|
||||
return { ok: false };
|
||||
}
|
||||
},
|
||||
|
|
@ -87,21 +91,27 @@ const rpc = BrowserView.defineRPC<PtyRPCSchema>({
|
|||
if (!ptyClient.isConnected) return { ok: true };
|
||||
try {
|
||||
ptyClient.resize(sessionId, cols, rows);
|
||||
} catch { /* ignore */ }
|
||||
} catch (err) {
|
||||
console.error(`[pty.resize] ${sessionId}:`, err);
|
||||
}
|
||||
return { ok: true };
|
||||
},
|
||||
|
||||
"pty.unsubscribe": ({ sessionId }) => {
|
||||
try {
|
||||
ptyClient.unsubscribe(sessionId);
|
||||
} catch { /* ignore */ }
|
||||
} catch (err) {
|
||||
console.error(`[pty.unsubscribe] ${sessionId}:`, err);
|
||||
}
|
||||
return { ok: true };
|
||||
},
|
||||
|
||||
"pty.close": ({ sessionId }) => {
|
||||
try {
|
||||
ptyClient.closeSession(sessionId);
|
||||
} catch { /* ignore */ }
|
||||
} catch (err) {
|
||||
console.error(`[pty.close] ${sessionId}:`, err);
|
||||
}
|
||||
return { ok: true };
|
||||
},
|
||||
|
||||
|
|
@ -227,7 +237,9 @@ const rpc = BrowserView.defineRPC<PtyRPCSchema>({
|
|||
}
|
||||
|
||||
const cloneIndex = existingClones.length + 1;
|
||||
const worktreePath = `${mainRepoPath}-wt-${cloneIndex}`;
|
||||
// Fix #8: Use UUID suffix to prevent race conditions between concurrent clones
|
||||
const wtSuffix = randomUUID().slice(0, 8);
|
||||
const worktreePath = `${mainRepoPath}-wt-${wtSuffix}`;
|
||||
|
||||
const gitResult = await gitWorktreeAdd(mainRepoPath, worktreePath, branchName);
|
||||
if (!gitResult.ok) {
|
||||
|
|
|
|||
|
|
@ -369,9 +369,11 @@ function adaptCodexItem(
|
|||
},
|
||||
];
|
||||
case "command_execution": {
|
||||
// Fix #13: Only emit tool_call on item.started, tool_result on item.completed
|
||||
// Prevents duplicate tool_call messages.
|
||||
const messages: AgentMessage[] = [];
|
||||
const toolUseId = str(item.id, uuid);
|
||||
if (eventType === "item.started" || eventType === "item.completed") {
|
||||
if (eventType === "item.started") {
|
||||
messages.push({
|
||||
id: `${uuid}-call`,
|
||||
type: "tool_call",
|
||||
|
|
|
|||
|
|
@ -110,12 +110,30 @@ export class SettingsDb {
|
|||
this.db.exec(SCHEMA);
|
||||
this.db.exec(SEED_GROUPS);
|
||||
|
||||
// Seed schema_version row if missing
|
||||
const version = this.db
|
||||
// Run version-tracked migrations
|
||||
this.runMigrations();
|
||||
}
|
||||
|
||||
/** Run version-tracked schema migrations. */
|
||||
private runMigrations(): void {
|
||||
const CURRENT_VERSION = 1;
|
||||
|
||||
const row = this.db
|
||||
.query<{ version: number }, []>("SELECT version FROM schema_version LIMIT 1")
|
||||
.get();
|
||||
if (!version) {
|
||||
this.db.exec("INSERT INTO schema_version (version) VALUES (1)");
|
||||
const currentVersion = row?.version ?? 0;
|
||||
|
||||
if (currentVersion < 1) {
|
||||
// Version 1 is the initial schema — already created above via SCHEMA.
|
||||
// Future migrations go here as version checks:
|
||||
// if (currentVersion < 2) { this.db.exec("ALTER TABLE ..."); }
|
||||
// if (currentVersion < 3) { this.db.exec("ALTER TABLE ..."); }
|
||||
}
|
||||
|
||||
if (!row) {
|
||||
this.db.exec(`INSERT INTO schema_version (version) VALUES (${CURRENT_VERSION})`);
|
||||
} else if (currentVersion < CURRENT_VERSION) {
|
||||
this.db.exec(`UPDATE schema_version SET version = ${CURRENT_VERSION}`);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -41,11 +41,25 @@ interface ActiveSession {
|
|||
onStatus: StatusCallback[];
|
||||
}
|
||||
|
||||
// ── Environment stripping ────────────────────────────────────────────────────
|
||||
// ── Environment stripping (Fix #14) ──────────────────────────────────────────
|
||||
|
||||
const STRIP_PREFIXES = ["CLAUDE", "CODEX", "OLLAMA", "ANTHROPIC_"];
|
||||
const WHITELIST_PREFIXES = ["CLAUDE_CODE_EXPERIMENTAL_"];
|
||||
|
||||
function validateExtraEnv(extraEnv: Record<string, string> | undefined): Record<string, string> | undefined {
|
||||
if (!extraEnv) return undefined;
|
||||
const clean: Record<string, string> = {};
|
||||
for (const [key, value] of Object.entries(extraEnv)) {
|
||||
const blocked = STRIP_PREFIXES.some((p) => key.startsWith(p));
|
||||
if (blocked) {
|
||||
console.warn(`[sidecar] Rejected extraEnv key "${key}" — provider-prefixed keys not allowed`);
|
||||
continue;
|
||||
}
|
||||
clean[key] = value;
|
||||
}
|
||||
return Object.keys(clean).length > 0 ? clean : undefined;
|
||||
}
|
||||
|
||||
function buildCleanEnv(extraEnv?: Record<string, string>, claudeConfigDir?: string): Record<string, string> {
|
||||
const clean: Record<string, string> = {};
|
||||
|
||||
|
|
@ -61,8 +75,10 @@ function buildCleanEnv(extraEnv?: Record<string, string>, claudeConfigDir?: stri
|
|||
if (claudeConfigDir) {
|
||||
clean["CLAUDE_CONFIG_DIR"] = claudeConfigDir;
|
||||
}
|
||||
if (extraEnv) {
|
||||
Object.assign(clean, extraEnv);
|
||||
// Apply validated extraEnv
|
||||
const validated = validateExtraEnv(extraEnv);
|
||||
if (validated) {
|
||||
Object.assign(clean, validated);
|
||||
}
|
||||
|
||||
return clean;
|
||||
|
|
@ -93,13 +109,11 @@ function findClaudeCli(): string | undefined {
|
|||
// ── Runner resolution ────────────────────────────────────────────────────────
|
||||
|
||||
function resolveRunnerPath(provider: ProviderId): string {
|
||||
// Sidecar runners live in the repo's sidecar/dist/ directory
|
||||
const repoRoot = join(import.meta.dir, "..", "..", "..");
|
||||
return join(repoRoot, "sidecar", "dist", `${provider}-runner.mjs`);
|
||||
}
|
||||
|
||||
function findNodeRuntime(): string {
|
||||
// Prefer Deno, fallback to Node.js (matching Tauri sidecar behavior)
|
||||
try {
|
||||
const result = Bun.spawnSync(["which", "deno"]);
|
||||
const path = new TextDecoder().decode(result.stdout).trim();
|
||||
|
|
@ -115,10 +129,15 @@ function findNodeRuntime(): string {
|
|||
return "node"; // last resort
|
||||
}
|
||||
|
||||
// ── Cleanup grace period ─────────────────────────────────────────────────────
|
||||
|
||||
const CLEANUP_GRACE_MS = 60_000; // 60s after done/error before removing session
|
||||
|
||||
// ── SidecarManager ───────────────────────────────────────────────────────────
|
||||
|
||||
export class SidecarManager {
|
||||
private sessions = new Map<string, ActiveSession>();
|
||||
private cleanupTimers = new Map<string, ReturnType<typeof setTimeout>>();
|
||||
private claudePath: string | undefined;
|
||||
private nodeRuntime: string;
|
||||
|
||||
|
|
@ -197,6 +216,8 @@ export class SidecarManager {
|
|||
if (s) {
|
||||
s.state.status = exitCode === 0 ? "done" : "error";
|
||||
this.emitStatus(sessionId, s.state.status, exitCode !== 0 ? `Exit code: ${exitCode}` : undefined);
|
||||
// Schedule cleanup (Fix #2)
|
||||
this.scheduleCleanup(sessionId);
|
||||
}
|
||||
});
|
||||
|
||||
|
|
@ -211,7 +232,7 @@ export class SidecarManager {
|
|||
maxTurns: options.maxTurns,
|
||||
permissionMode: options.permissionMode ?? "bypassPermissions",
|
||||
claudeConfigDir: options.claudeConfigDir,
|
||||
extraEnv: options.extraEnv,
|
||||
extraEnv: validateExtraEnv(options.extraEnv),
|
||||
};
|
||||
|
||||
this.writeToProcess(sessionId, queryMsg);
|
||||
|
|
@ -236,7 +257,7 @@ export class SidecarManager {
|
|||
s.controller.abort();
|
||||
s.state.status = "done";
|
||||
this.emitStatus(sessionId, "done");
|
||||
this.sessions.delete(sessionId);
|
||||
this.scheduleCleanup(sessionId);
|
||||
}
|
||||
}, 3000);
|
||||
|
||||
|
|
@ -290,6 +311,30 @@ export class SidecarManager {
|
|||
}
|
||||
this.sessions.delete(sessionId);
|
||||
}
|
||||
// Cancel any cleanup timer
|
||||
const timer = this.cleanupTimers.get(sessionId);
|
||||
if (timer) {
|
||||
clearTimeout(timer);
|
||||
this.cleanupTimers.delete(sessionId);
|
||||
}
|
||||
}
|
||||
|
||||
// ── Cleanup scheduling (Fix #2) ─────────────────────────────────────────
|
||||
|
||||
private scheduleCleanup(sessionId: string): void {
|
||||
// Cancel any existing timer
|
||||
const existing = this.cleanupTimers.get(sessionId);
|
||||
if (existing) clearTimeout(existing);
|
||||
|
||||
const timer = setTimeout(() => {
|
||||
this.cleanupTimers.delete(sessionId);
|
||||
const session = this.sessions.get(sessionId);
|
||||
if (session && (session.state.status === "done" || session.state.status === "error")) {
|
||||
this.sessions.delete(sessionId);
|
||||
}
|
||||
}, CLEANUP_GRACE_MS);
|
||||
|
||||
this.cleanupTimers.set(sessionId, timer);
|
||||
}
|
||||
|
||||
// ── Internal ───────────────────────────────────────────────────────────────
|
||||
|
|
@ -324,6 +369,12 @@ export class SidecarManager {
|
|||
this.handleNdjsonLine(sessionId, session, line);
|
||||
}
|
||||
}
|
||||
|
||||
// Fix #12: Parse any residual data left in the buffer after stream ends
|
||||
const residual = buffer.trim();
|
||||
if (residual) {
|
||||
this.handleNdjsonLine(sessionId, session, residual);
|
||||
}
|
||||
} catch (err) {
|
||||
// Stream closed — expected on process exit
|
||||
if (!session.controller.signal.aborted) {
|
||||
|
|
@ -339,7 +390,6 @@ export class SidecarManager {
|
|||
try {
|
||||
for await (const chunk of reader) {
|
||||
const text = decoder.decode(chunk, { stream: true });
|
||||
// Log sidecar stderr as debug output
|
||||
for (const line of text.split("\n")) {
|
||||
if (line.trim()) {
|
||||
console.log(`[sidecar:${sessionId}] ${line.trim()}`);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue