mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-10 23:19:09 +02:00
Merge origin/main (v1.64.1.0 code-smell wave) into test-evals-ci-speedup
Both sides shipped overlapping test-infra work in parallel; resolutions compose intent rather than picking sides: - free-tests.yml (both added): keep this branch's lane (canonical strict-parallel runner, secretless, plain runner, ~2min) over main's per-file-serial container loop (45min budget, hand-curated skip list, needs GITHUB_TOKEN); ported main's git safe.directory insight. - Dockerfile.ci Bun install: main discovered the installer IGNORES the BUN_VERSION env var (the old form silently installed latest) — main's arg-form mechanism + this branch's 1.3.13 target. - parity baseline: both sides rebased after hitting the same silent drift; adopted main's v1.64.1.0 union-normalized fixture and dropped this branch's interim v1.64.0.0 capture. carve-guards caps: main's tighter re-ratchets win (all four). - touchfiles: kept this branch's three-file facade split; ported main's pure-data removals (dead sidebar-agent entries, spec judge entry, ship-idempotency) into touchfiles-data.ts. - ship-idempotency SDK variant: main deliberately removed it as redundant with the real-PTY test; adopted — dropped this branch's rehomed copy and its periodic matrix row (the zombie-monolith deletion stands; coverage-audit + triage rehomes verified untouched by main). - e2e-tier-alignment: taught the new parent-mapper hard check main's consolidated describeE2ETier()/e2eTierEnabled() self-gate shapes (the helper's header names this file as a required recognizer). - browse/test/compare-board.test.ts: quarantined behind GSTACK_COMPARE_BOARD_TESTS=1 — all 16 tests fail identically on origin/main solo on dev machines (blame protocol receipts in-file); main's own CI lane skip-lists it. An always-red file would block every PR now that free-tests is a required check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,76 @@
|
||||
/**
|
||||
* Atomic file writes — the ONE implementation of tmp-write-then-rename.
|
||||
*
|
||||
* Before this module, the pattern was reimplemented ~20 times across lib/,
|
||||
* bin/, and browse/src with three different tmp-suffix conventions — one of
|
||||
* which (a bare `.tmp`) carries a real collision race that browse's
|
||||
* server.ts documented after hitting it in production: two writers (batch
|
||||
* subcommands, /tunnel/start handlers, or any combination) collide on the
|
||||
* rename when the tmp filename is deterministic. The suffix here includes
|
||||
* pid AND a random component so concurrent writers in the SAME process
|
||||
* (async interleavings) can't collide either.
|
||||
*
|
||||
* Contract:
|
||||
* - atomicWriteSync ALWAYS throws on failure, after best-effort tmp cleanup.
|
||||
* Callers own the error. Use it everywhere except shutdown paths.
|
||||
* - atomicWriteQuiet swallows everything (returns false on failure). ONLY
|
||||
* for shutdown/emergency-cleanup paths where a throw would abort the rest
|
||||
* of cleanup — same philosophy as browse's safeUnlinkQuiet.
|
||||
* - `mode` applies to the tmp file at creation (0600 for sensitive state),
|
||||
* so the final file never exists with looser permissions.
|
||||
* - The tmp file is created in the target's directory (same filesystem, so
|
||||
* rename stays atomic). Parent dirs are NOT created — callers that need
|
||||
* mkdir own that decision (and its mode).
|
||||
*/
|
||||
import * as fs from 'fs';
|
||||
import * as crypto from 'crypto';
|
||||
|
||||
export interface AtomicWriteOpts {
|
||||
/** File mode for the tmp file at creation (e.g. 0o600). Default: umask. */
|
||||
mode?: number;
|
||||
}
|
||||
|
||||
function tmpPathFor(target: string): string {
|
||||
return `${target}.tmp.${process.pid}.${crypto.randomBytes(4).toString('hex')}`;
|
||||
}
|
||||
|
||||
/** Atomic write. Throws on failure (after best-effort tmp cleanup). */
|
||||
export function atomicWriteSync(
|
||||
target: string,
|
||||
data: string | NodeJS.ArrayBufferView,
|
||||
opts: AtomicWriteOpts = {},
|
||||
): void {
|
||||
const tmp = tmpPathFor(target);
|
||||
try {
|
||||
if (opts.mode !== undefined) {
|
||||
fs.writeFileSync(tmp, data, { mode: opts.mode });
|
||||
} else {
|
||||
fs.writeFileSync(tmp, data);
|
||||
}
|
||||
fs.renameSync(tmp, target);
|
||||
} catch (err) {
|
||||
try {
|
||||
fs.unlinkSync(tmp);
|
||||
} catch {
|
||||
// Best-effort cleanup; the original error is the one that matters.
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomic write that swallows all errors. Returns true on success.
|
||||
* ONLY for shutdown/emergency paths — a throw there aborts remaining cleanup.
|
||||
*/
|
||||
export function atomicWriteQuiet(
|
||||
target: string,
|
||||
data: string | NodeJS.ArrayBufferView,
|
||||
opts: AtomicWriteOpts = {},
|
||||
): boolean {
|
||||
try {
|
||||
atomicWriteSync(target, data, opts);
|
||||
return true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
@@ -41,10 +41,9 @@ import {
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
readFileSync,
|
||||
renameSync,
|
||||
statSync,
|
||||
writeFileSync,
|
||||
} from "fs";
|
||||
import { atomicWriteSync } from "./fs-atomic";
|
||||
import { homedir } from "os";
|
||||
import { dirname, join } from "path";
|
||||
import { buildGbrainEnv, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec";
|
||||
@@ -254,9 +253,7 @@ function writeCache(status: LocalEngineStatus, key: CacheEntry["key"]): void {
|
||||
};
|
||||
try {
|
||||
mkdirSync(dirname(cacheFilePath()), { recursive: true });
|
||||
const tmp = cacheFilePath() + ".tmp." + process.pid;
|
||||
writeFileSync(tmp, JSON.stringify(entry, null, 2), "utf-8");
|
||||
renameSync(tmp, cacheFilePath());
|
||||
atomicWriteSync(cacheFilePath(), JSON.stringify(entry, null, 2));
|
||||
} catch {
|
||||
// Cache write failure is non-fatal — we re-probe next call.
|
||||
}
|
||||
|
||||
@@ -16,7 +16,8 @@
|
||||
import { join } from "path";
|
||||
import { homedir } from "os";
|
||||
import { randomUUID } from "crypto";
|
||||
import { writeFileSync, renameSync, existsSync, readFileSync, appendFileSync, statSync, openSync, closeSync, unlinkSync } from "fs";
|
||||
import { existsSync, readFileSync, appendFileSync, statSync, openSync, closeSync, unlinkSync } from "fs";
|
||||
import { atomicWriteSync } from "./fs-atomic";
|
||||
import { appendJsonl, readJsonl, hasInjection } from "./jsonl-store";
|
||||
import { scan } from "./redact-engine";
|
||||
|
||||
@@ -224,9 +225,7 @@ export function readEvents(paths: DecisionPaths): DecisionEvent[] {
|
||||
* O(active), not O(history).
|
||||
*/
|
||||
export function writeSnapshot(paths: DecisionPaths, active: ActiveDecision[]): void {
|
||||
const tmp = `${paths.snapshot}.tmp.${process.pid}`;
|
||||
writeFileSync(tmp, JSON.stringify(active), "utf-8");
|
||||
renameSync(tmp, paths.snapshot);
|
||||
atomicWriteSync(paths.snapshot, JSON.stringify(active));
|
||||
}
|
||||
|
||||
/** Read the bounded active snapshot. Returns [] if missing/corrupt (caller may rebuild). */
|
||||
@@ -308,9 +307,7 @@ export function compact(paths: DecisionPaths): CompactResult {
|
||||
appendFileSync(paths.archive, superseded.map((e) => JSON.stringify(e)).join("\n") + "\n", "utf-8");
|
||||
}
|
||||
|
||||
const tmp = `${paths.log}.tmp.${process.pid}`;
|
||||
writeFileSync(tmp, active.map((d) => JSON.stringify(d)).join("\n") + (active.length ? "\n" : ""), "utf-8");
|
||||
renameSync(tmp, paths.log);
|
||||
atomicWriteSync(paths.log, active.map((d) => JSON.stringify(d)).join("\n") + (active.length ? "\n" : ""));
|
||||
writeSnapshot(paths, active);
|
||||
|
||||
return { activeCount: active.length, archivedCount: superseded.length, expungedCount: redactedIds.size };
|
||||
|
||||
@@ -17,7 +17,8 @@
|
||||
* helper warns once and returns an empty findings list — fail-safe defaults.
|
||||
*/
|
||||
|
||||
import { existsSync, readFileSync, writeFileSync, mkdirSync, statSync, appendFileSync } from "fs";
|
||||
import { existsSync, readFileSync, writeFileSync, mkdirSync, statSync } from "fs";
|
||||
import { appendJsonl } from "./jsonl-store";
|
||||
import { dirname, join } from "path";
|
||||
import { execFileSync } from "child_process";
|
||||
import { homedir } from "os";
|
||||
@@ -268,11 +269,7 @@ function logGbrainError(kind: string, detail: string): void {
|
||||
try {
|
||||
const path = errorLogPath();
|
||||
mkdirSync(dirname(path), { recursive: true });
|
||||
appendFileSync(
|
||||
path,
|
||||
JSON.stringify({ ts: new Date().toISOString(), kind, detail: detail.slice(0, 500) }) + "\n",
|
||||
"utf-8"
|
||||
);
|
||||
appendJsonl(path, { ts: new Date().toISOString(), kind, detail: detail.slice(0, 500) });
|
||||
} catch { /* logging is best-effort */ }
|
||||
}
|
||||
|
||||
@@ -505,7 +502,7 @@ function logErrorContext(entry: ErrorContextEntry): void {
|
||||
try {
|
||||
const path = errorLogPath();
|
||||
mkdirSync(dirname(path), { recursive: true });
|
||||
appendFileSync(path, JSON.stringify(entry) + "\n", "utf-8");
|
||||
appendJsonl(path, entry);
|
||||
} catch {
|
||||
// Logging failure is non-fatal — never block the op.
|
||||
}
|
||||
|
||||
+24
-12
@@ -1,17 +1,22 @@
|
||||
/**
|
||||
* jsonl-store — shared, audited plumbing for gstack's append-only JSONL stores.
|
||||
* jsonl-store — shared plumbing for gstack's append-only JSONL stores in
|
||||
* lib/ and bin/. (browse/src keeps its own appenders by design — the
|
||||
* compiled-binary surface has different logging semantics and its own
|
||||
* secure-append helper.)
|
||||
*
|
||||
* Single source of truth for the three things every JSONL store must get right:
|
||||
* 1. Injection sanitization (the prompt-injection patterns that must NOT survive
|
||||
* into agent context when a record is later resurfaced).
|
||||
* The three things a JSONL store must get right:
|
||||
* 1. Injection screening — SEE THE CONTRACT BELOW: appendJsonl does NOT
|
||||
* screen; callers that store free text MUST pre-check with
|
||||
* hasInjection()/firstInjectionMatch() and reject. Enforcing callers
|
||||
* today: bin/gstack-learnings-log, bin/gstack-decision-log (via
|
||||
* lib/gstack-decision.ts), bin/gstack-question-log.
|
||||
* 2. Atomic single-line append (concurrent agents must not corrupt the file).
|
||||
* 3. Tolerant read (a partially-written tail or one corrupt line must not take
|
||||
* down the whole read).
|
||||
* 3. Tolerant read (a partially-written tail or one corrupt line must not
|
||||
* take down the whole read).
|
||||
*
|
||||
* Extracted from `bin/gstack-learnings-log` (D2A) so `gstack-learnings-*` and the
|
||||
* new `gstack-decision-*` bins share ONE audited path — a new injection pattern or
|
||||
* a write-atomicity fix lands in both at once, never drifts. Per the
|
||||
* `squash-with-regen` / DRY discipline + the eng-review D2A decision.
|
||||
* Extracted from `bin/gstack-learnings-log` (D2A) so the learnings/decision/
|
||||
* question stores share ONE audited path — a new injection pattern or a
|
||||
* write-atomicity fix lands in all at once.
|
||||
*/
|
||||
|
||||
import { appendFileSync, readFileSync, existsSync } from "fs";
|
||||
@@ -60,12 +65,19 @@ export function firstInjectionMatch(text: string): RegExp | null {
|
||||
* Caveat: a record larger than PIPE_BUF loses the cross-process atomicity guarantee.
|
||||
* Keep records line-bounded; very large free-text should be truncated by the caller.
|
||||
*/
|
||||
export function appendJsonl(path: string, obj: unknown): void {
|
||||
export function appendJsonl(path: string, obj: unknown, opts: { mode?: number } = {}): void {
|
||||
const line = JSON.stringify(obj);
|
||||
if (line.includes("\n")) {
|
||||
throw new Error("jsonl-store: record serialized to multiple lines (embedded newline)");
|
||||
}
|
||||
appendFileSync(path, line + "\n", { encoding: "utf-8" });
|
||||
// `mode` applies only when the append CREATES the file (POSIX open(2)
|
||||
// semantics) — pass 0o600 for stores holding sensitive content so the
|
||||
// file never exists world-readable.
|
||||
if (opts.mode !== undefined) {
|
||||
appendFileSync(path, line + "\n", { encoding: "utf-8", mode: opts.mode });
|
||||
} else {
|
||||
appendFileSync(path, line + "\n", { encoding: "utf-8" });
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -18,6 +18,7 @@ import * as fs from "fs";
|
||||
import * as os from "os";
|
||||
import * as path from "path";
|
||||
import { createHash } from "crypto";
|
||||
import { appendJsonl } from "./jsonl-store";
|
||||
|
||||
export interface SemanticReviewEntry {
|
||||
ts: string;
|
||||
@@ -43,7 +44,9 @@ export function appendSemanticReview(entry: SemanticReviewEntry): void {
|
||||
const dir = securityDir();
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
const file = path.join(dir, "semantic-reviews.jsonl");
|
||||
fs.appendFileSync(file, JSON.stringify(entry) + "\n");
|
||||
// 0600 at create via appendJsonl's mode opt; the chmod backstop covers
|
||||
// files created looser by pre-mode versions.
|
||||
appendJsonl(file, entry, { mode: 0o600 });
|
||||
try {
|
||||
fs.chmodSync(file, 0o600);
|
||||
} catch {
|
||||
|
||||
+4
-3
@@ -13,6 +13,7 @@ import { spawnSync } from 'child_process';
|
||||
import * as crypto from 'crypto';
|
||||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import { atomicWriteSync } from './fs-atomic';
|
||||
import * as os from 'os';
|
||||
|
||||
// --- Interfaces ---
|
||||
@@ -84,9 +85,9 @@ function loadDedupIndex(): DedupIndex {
|
||||
function saveDedupIndex(index: DedupIndex): void {
|
||||
const dir = path.dirname(getDedupPath());
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
const tmp = getDedupPath() + '.tmp';
|
||||
fs.writeFileSync(tmp, JSON.stringify(index, null, 2));
|
||||
fs.renameSync(tmp, getDedupPath());
|
||||
// Was a bare '.tmp' suffix — the deterministic-tmp collision race the
|
||||
// shared helper exists to prevent.
|
||||
atomicWriteSync(getDedupPath(), JSON.stringify(index, null, 2));
|
||||
}
|
||||
|
||||
// --- WorktreeManager ---
|
||||
|
||||
Reference in New Issue
Block a user