History

Address PR review feedback for git actions

Julius Marminge committed 9 months ago

Commit 533dbdf1 parent 98eb87f6 files · +135 −21Browse files

Unified · Split

6 files
  1. apps/server/src/codexTextGenerator.ts +7 −2
  2. apps/server/src/git.test.ts +1 −1
  3. apps/server/src/git.ts +10 −4
  4. apps/server/src/gitManager.ts +22 −6
  5. apps/server/src/processRunner.ts +79 −4
  6. apps/web/src/components/ChatView.tsx +16 −4
apps/server/src/codexTextGenerator.ts+7−2
@@ -117,9 +117,11 @@ async function runCodexJson<T>({ · 116 unmodified lines
117117 "codex-schema",
118118 JSON.stringify(outputSchemaJson),
119119 );
120− const outputPath = await writeTempFile("codex-output", "");
120+ let outputPath: string | null = null;
121121
122122 try {
123+ outputPath = await writeTempFile("codex-output", "");
124+
123125 await runProcess(
124126 "codex",
125127 [
@@ -154,7 +156,10 @@ async function runCodexJson<T>({ · 28 unmodified lines
154156
155157 return parse(parsedJson);
156158 } finally {
157− await Promise.all([safeUnlink(schemaPath), safeUnlink(outputPath)]);
159+ await Promise.all([
160+ safeUnlink(schemaPath),
161+ ...(outputPath ? [safeUnlink(outputPath)] : []),
162+ ]);
158163 }
159164 }
160165
apps/server/src/git.test.ts+1−1
@@ -462,7 +462,7 @@ describe("git integration", () => { · 461 unmodified lines
462462 await using remote = await makeTmpDir();
463463 await initRepoWithCommit(tmp.path);
464464 await git(remote.path, "init --bare");
465− await git(tmp.path, `remote add origin '${remote.path}'`);
465+ await git(tmp.path, `remote add origin ${JSON.stringify(remote.path)}`);
466466 await createGitBranch({ cwd: tmp.path, branch: "feature/core-push" });
467467 await checkoutGitBranch({ cwd: tmp.path, branch: "feature/core-push" });
468468
apps/server/src/git.ts+10−4
@@ -102,12 +102,15 @@ function commandLabel(args: readonly string[]): string { · 101 unmodified lines
102102 return `git ${args.join(" ")}`;
103103 }
104104
105−function normalizeGitSpawnError(error: unknown): Error {
105+function normalizeGitSpawnError(cwd: string, error: unknown): Error {
106106 if (!(error instanceof Error)) {
107107 return new Error("Failed to run git command.");
108108 }
109109 const code = (error as NodeJS.ErrnoException).code;
110110 if (code === "ENOENT") {
111+ if (!fs.existsSync(cwd)) {
112+ return new Error(`Working directory does not exist: ${cwd}`);
113+ }
111114 return new Error("Git is required but not available on PATH.");
112115 }
113116 return new Error(`Failed to run git command: ${error.message}`);
@@ -135,10 +138,13 @@ async function runGitOrThrow( · 21 unmodified lines
135138 try {
136139 result = await runGit(args, cwd, options.timeoutMs);
137140 } catch (error) {
138− throw normalizeGitSpawnError(error);
141+ throw normalizeGitSpawnError(cwd, error);
139142 }
140143
141− if (!options.allowNonZeroExit && (result.code !== 0 || result.timedOut)) {
144+ if (result.timedOut) {
145+ throw normalizeGitExecutionError(args, result);
146+ }
147+ if (!options.allowNonZeroExit && result.code !== 0) {
142148 throw normalizeGitExecutionError(args, result);
143149 }
144150 return result;
@@ -242,7 +248,7 @@ export class GitCoreService { · 97 unmodified lines
242248 throw new Error("Cannot push from detached HEAD.");
243249 }
244250
245− if (details.hasUpstream && details.aheadCount === 0) {
251+ if (details.hasUpstream && details.aheadCount === 0 && details.behindCount === 0) {
246252 return {
247253 status: "skipped_up_to_date",
248254 branch,
apps/server/src/gitManager.ts+22−6
@@ -21,6 +21,8 @@ import { CodexTextGenerator } from "./codexTextGenerator"; · 20 unmodified lines
2121 import { GitCoreService } from "./git";
2222 import { type ProcessRunOptions, runProcess } from "./processRunner";
2323
24+const GH_CLI_TIMEOUT_MS = 30_000;
25+
2426 interface GitManagerDeps {
2527 runProcess?: (
2628 command: string,
@@ -107,10 +109,21 @@ function sanitizeCommitMessage( · 80 unmodified lines
107109
108110 function extractBranchFromRef(ref: string): string {
109111 const normalized = ref.trim();
110− const afterSlash = normalized.includes("/")
111− ? normalized.slice(normalized.lastIndexOf("/") + 1)
112− : normalized;
113− return afterSlash.trim();
112+
113+ if (normalized.startsWith("refs/remotes/")) {
114+ const withoutPrefix = normalized.slice("refs/remotes/".length);
115+ const firstSlash = withoutPrefix.indexOf("/");
116+ if (firstSlash === -1) {
117+ return withoutPrefix.trim();
118+ }
119+ return withoutPrefix.slice(firstSlash + 1).trim();
120+ }
121+
122+ const firstSlash = normalized.indexOf("/");
123+ if (firstSlash === -1) {
124+ return normalized;
125+ }
126+ return normalized.slice(firstSlash + 1).trim();
114127 }
115128
116129 function asCommandNotFound(
@@ -401,7 +414,7 @@ export class GitManager { · 284 unmodified lines
401414
402415 private async runGh(cwd: string, args: readonly string[]): Promise<void> {
403416 try {
404− await this.run("gh", args, { cwd });
417+ await this.run("gh", args, { cwd, timeoutMs: GH_CLI_TIMEOUT_MS });
405418 } catch (error) {
406419 throw (
407420 asCommandNotFound("gh", error) ??
@@ -413,7 +426,10 @@ export class GitManager { · 5 unmodified lines
413426
414427 private async runGhStdout(cwd: string, args: readonly string[]): Promise<string> {
415428 try {
416− const result = await this.run("gh", args, { cwd });
429+ const result = await this.run("gh", args, {
430+ cwd,
431+ timeoutMs: GH_CLI_TIMEOUT_MS,
432+ });
417433 return result.stdout;
418434 } catch (error) {
419435 throw (
apps/server/src/processRunner.ts+79−4
@@ -6,6 +6,7 @@ export interface ProcessRunOptions { · 5 unmodified lines
66 env?: NodeJS.ProcessEnv | undefined;
77 stdin?: string | undefined;
88 allowNonZeroExit?: boolean | undefined;
9+ maxBufferBytes?: number | undefined;
910 }
1011
1112 export interface ProcessRunResult {
@@ -52,12 +53,39 @@ function normalizeExitError( · 40 unmodified lines
5253 return new Error(`${commandLabel(command, args)} ${reason}.${detail}`);
5354 }
5455
56+function normalizeStdinError(
57+ command: string,
58+ args: readonly string[],
59+ error: unknown,
60+): Error {
61+ if (!(error instanceof Error)) {
62+ return new Error(`Failed to write stdin for ${commandLabel(command, args)}.`);
63+ }
64+ return new Error(
65+ `Failed to write stdin for ${commandLabel(command, args)}: ${error.message}`,
66+ );
67+}
68+
69+function normalizeBufferError(
70+ command: string,
71+ args: readonly string[],
72+ stream: "stdout" | "stderr",
73+ maxBufferBytes: number,
74+): Error {
75+ return new Error(
76+ `${commandLabel(command, args)} exceeded ${stream} buffer limit (${maxBufferBytes} bytes).`,
77+ );
78+}
79+
80+const DEFAULT_MAX_BUFFER_BYTES = 8 * 1024 * 1024;
81+
5582 export async function runProcess(
5683 command: string,
5784 args: readonly string[],
5885 options: ProcessRunOptions = {},
5986 ): Promise<ProcessRunResult> {
6087 const timeoutMs = options.timeoutMs ?? 60_000;
88+ const maxBufferBytes = options.maxBufferBytes ?? DEFAULT_MAX_BUFFER_BYTES;
6189
6290 return new Promise<ProcessRunResult>((resolve, reject) => {
6391 const child = spawn(command, args, {
@@ -68,6 +96,8 @@ export async function runProcess( · 4 unmodified lines
6896
6997 let stdout = "";
7098 let stderr = "";
99+ let stdoutBytes = 0;
100+ let stderrBytes = 0;
71101 let timedOut = false;
72102 let settled = false;
73103 let forceKillTimer: ReturnType<typeof setTimeout> | null = null;
@@ -90,12 +120,47 @@ export async function runProcess( · 16 unmodified lines
90120 callback();
91121 };
92122
123+ const fail = (error: Error): void => {
124+ child.kill("SIGTERM");
125+ finalize(() => {
126+ reject(error);
127+ });
128+ };
129+
130+ const appendOutput = (
131+ stream: "stdout" | "stderr",
132+ chunk: Buffer | string,
133+ ): Error | null => {
134+ const text = chunk.toString();
135+ const byteLength = Buffer.byteLength(text);
136+ if (stream === "stdout") {
137+ stdout += text;
138+ stdoutBytes += byteLength;
139+ if (stdoutBytes > maxBufferBytes) {
140+ return normalizeBufferError(command, args, "stdout", maxBufferBytes);
141+ }
142+ } else {
143+ stderr += text;
144+ stderrBytes += byteLength;
145+ if (stderrBytes > maxBufferBytes) {
146+ return normalizeBufferError(command, args, "stderr", maxBufferBytes);
147+ }
148+ }
149+ return null;
150+ };
151+
93152 child.stdout.on("data", (chunk: Buffer | string) => {
94− stdout += chunk.toString();
153+ const error = appendOutput("stdout", chunk);
154+ if (error) {
155+ fail(error);
156+ }
95157 });
96158
97159 child.stderr.on("data", (chunk: Buffer | string) => {
98− stderr += chunk.toString();
160+ const error = appendOutput("stderr", chunk);
161+ if (error) {
162+ fail(error);
163+ }
99164 });
100165
101166 child.once("error", (error) => {
@@ -125,10 +190,20 @@ export async function runProcess( · 23 unmodified lines
125190 });
126191 });
127192
193+ child.stdin.once("error", (error) => {
194+ fail(normalizeStdinError(command, args, error));
195+ });
196+
128197 if (options.stdin !== undefined) {
129− child.stdin.write(options.stdin);
198+ child.stdin.write(options.stdin, (error) => {
199+ if (error) {
200+ fail(normalizeStdinError(command, args, error));
201+ return;
202+ }
203+ child.stdin.end();
204+ });
205+ return;
130206 }
131207 child.stdin.end();
132208 });
133209 }
134−
apps/web/src/components/ChatView.tsx+16−4
@@ -196,6 +196,7 @@ export default function ChatView() { · 195 unmodified lines
196196 const modelMenuRef = useRef<HTMLDivElement>(null);
197197 const editorMenuRef = useRef<HTMLDivElement>(null);
198198 const gitMenuRef = useRef<HTMLDivElement>(null);
199+ const latestGitCwdRef = useRef<string | null>(null);
199200
200201 const activeThread = state.threads.find((t) => t.id === state.activeThreadId);
201202 const activeProject = state.projects.find((p) => p.id === activeThread?.projectId);
@@ -302,19 +303,27 @@ export default function ChatView() { · 100 unmodified lines
302303 !api || !gitCwd || !gitStatus || isGitActionRunning;
303304 const pushActionDisabled = gitActionsDisabled || gitStatus.branch === null;
304305
306+ useEffect(() => {
307+ latestGitCwdRef.current = gitCwd;
308+ }, [gitCwd]);
309+
305310 const refreshGitStatus = useCallback(async () => {
306− if (!api || !gitCwd) {
311+ const requestCwd = gitCwd;
312+ if (!api || !requestCwd) {
307313 setGitStatus(null);
308314 return;
309315 }
310316
311− const nextStatus = await api.git.status({ cwd: gitCwd });
317+ const nextStatus = await api.git.status({ cwd: requestCwd });
318+ if (latestGitCwdRef.current !== requestCwd) return;
312319 setGitStatus(nextStatus);
320+ setGitActionError(null);
313321 }, [api, gitCwd]);
314322
315323 const runGitAction = useCallback(
316324 async (action: GitStackedAction) => {
317325 if (!api || !gitCwd) return;
326+ const actionCwd = gitCwd;
318327
319328 setIsGitMenuOpen(false);
320329 setIsGitActionRunning(true);
@@ -322,7 +331,7 @@ export default function ChatView() { · 1 unmodified line
322331 setGitActionNotice(null);
323332 try {
324333 const result = await api.git.runStackedAction({
325− cwd: gitCwd,
334+ cwd: actionCwd,
326335 action,
327336 });
328337 setGitActionNotice(summarizeGitActionResult(result));
@@ -333,7 +342,9 @@ export default function ChatView() { · 4 unmodified lines
333342 } finally {
334343 setIsGitActionRunning(false);
335344 try {
336− await refreshGitStatus();
345+ if (latestGitCwdRef.current === actionCwd) {
346+ await refreshGitStatus();
347+ }
337348 } catch {
338349 setGitStatus(null);
339350 }
@@ -404,6 +415,7 @@ export default function ChatView() { · 64 unmodified lines
404415 const nextStatus = await api.git.status({ cwd: gitCwd });
405416 if (!cancelled) {
406417 setGitStatus(nextStatus);
418+ setGitActionError(null);
407419 }
408420 } catch (error) {
409421 if (!cancelled) {