From 44eea64084343bacb73d1273feafe563fe4555c9 Mon Sep 17 00:00:00 2001 From: test Date: Thu, 13 Aug 2026 10:25:34 -0700 Subject: [PATCH] put_page: report push state honestly instead of implying it via committed (#3936) Wave-assembled from PR #3936 by @dovstern. Conflict resolution: kept master's resolveRepoRoot() block AND the PR's exported currentBranch in src/core/brain-repo-durability.ts. Adaptation: the new serial test now writes the simulated push log under $GBRAIN_HOME/.gbrain (CX2-8 parent-dir semantics landed on master after the PR's base). Co-Authored-By: Dov Stern --- src/core/brain-repo-durability.ts | 54 +++++++- src/core/operations.ts | 7 +- src/core/write-through.ts | 40 +++++- test/put-page-push-reporting.serial.test.ts | 144 ++++++++++++++++++++ 4 files changed, 237 insertions(+), 8 deletions(-) create mode 100644 test/put-page-push-reporting.serial.test.ts diff --git a/src/core/brain-repo-durability.ts b/src/core/brain-repo-durability.ts index b1bb6b72a..9cdd2d504 100644 --- a/src/core/brain-repo-durability.ts +++ b/src/core/brain-repo-durability.ts @@ -450,6 +450,58 @@ export function commitWriteThroughFile(repoPath: string, absPath: string, slug: } } +// ── Push-state query (D14) ─────────────────────────────────────────────────── + +export type PushLogStatus = 'ok' | 'needs_attention' | 'unknown'; + +export interface PushLogOutcome { + status: PushLogStatus; + detail: string; + /** UTC timestamp parsed from the log line, when found. */ + at?: string; +} + +const PUSH_LOG_OK = /^(\S+) \[push\] (?:ok|ok-after-rebase) (\S+)\b/; +const PUSH_LOG_LOCAL_ONLY = /^(\S+) \[push\] LOCAL-ONLY, NEEDS ATTENTION: (\S+) /; +const PUSH_LOG_LOCK_TIMEOUT = /^(\S+) \[push\] lock-timeout (\S+)\b/; + +/** + * Best-effort read of the most recently logged push outcome for `branch`, + * from the shared hook log ($GBRAIN_HOME/brain-push.log). The push itself + * runs detached in the background (see `renderPostCommitHook`), so nothing + * synchronous ever learns whether a given commit's own push landed — this is + * the queryable substitute: "as of the last thing the hook logged for this + * branch, were pushes landing?" + * + * The log is host-wide and keyed only by branch name, not repo path, so two + * different hardened repos sharing a branch name (e.g. both on `main`) share + * this signal. That's an acceptable approximation for a liveness check, not + * a per-repo guarantee. + */ +export function getLastPushOutcome(branch: string): PushLogOutcome { + const log = pushLogPath(); + if (!existsSync(log)) return { status: 'unknown', detail: 'no push attempts logged yet' }; + + let lines: string[]; + try { + lines = readFileSync(log, 'utf-8').split('\n'); + } catch (e) { + return { status: 'unknown', detail: `push log unreadable: ${(e as Error).message}` }; + } + + for (let i = lines.length - 1; i >= 0; i--) { + const line = lines[i]; + if (!line) continue; + let m = line.match(PUSH_LOG_OK); + if (m && m[2] === branch) return { status: 'ok', detail: line.trim(), at: m[1] }; + m = line.match(PUSH_LOG_LOCAL_ONLY); + if (m && m[2] === branch) return { status: 'needs_attention', detail: line.trim(), at: m[1] }; + m = line.match(PUSH_LOG_LOCK_TIMEOUT); + if (m && m[2] === branch) return { status: 'needs_attention', detail: line.trim(), at: m[1] }; + } + return { status: 'unknown', detail: `no push attempt logged yet for branch '${branch}'` }; +} + // ── Committed helper ──────────────────────────────────────────────────────── function installHelper(repoPath: string, dryRun: boolean): { status: StepStatus; detail: string } { @@ -784,7 +836,7 @@ function resolveRepoRoot(path: string): string { } } -function currentBranch(repoPath: string): string { +export function currentBranch(repoPath: string): string { try { return execFileSync('git', ['-C', repoPath, 'rev-parse', '--abbrev-ref', 'HEAD'], { stdio: ['ignore', 'pipe', 'ignore'], timeout: 10_000, env: { ...process.env, ...GIT_ENV }, diff --git a/src/core/operations.ts b/src/core/operations.ts index 2e62f8b2a..337695399 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -10,7 +10,7 @@ import { clampSearchLimit } from './engine.ts'; import type { GBrainConfig } from './config.ts'; import type { PageType } from './types.ts'; import { importFromContent } from './import-file.ts'; -import { writePageThrough } from './write-through.ts'; +import { writePageThrough, type WriteThroughResult } from './write-through.ts'; import { hybridSearch, hybridSearchCached, stampContentFlags, stampUnverifiedExtractions } from './search/hybrid.ts'; import { expandQuery } from './search/expansion.ts'; import { dedupResults } from './search/dedup.ts'; @@ -1323,7 +1323,10 @@ const put_page: Operation = { // Trust gating: // - Subagent sandbox (viaSubagent without allowedSlugPrefixes) → DB-only. // - All other writes → write-through. - let writeThrough: { written: boolean; path?: string; skipped?: string; error?: string } | undefined; + // put_page's own trust-gating produces two skip reasons ('subagent_sandbox', + // 'dry_run') that never come out of writePageThrough itself — widen the + // field rather than losing the commit/pushed/lastPushStatus typing. + let writeThrough: (Omit & { skipped?: WriteThroughResult['skipped'] | 'subagent_sandbox' | 'dry_run' }) | undefined; const isSandboxSubagent = ctx.viaSubagent === true && !(Array.isArray(ctx.allowedSlugPrefixes) && ctx.allowedSlugPrefixes.length > 0); if (!ctx.dryRun && result.status !== 'error' && !isSandboxSubagent) { diff --git a/src/core/write-through.ts b/src/core/write-through.ts index 00df92d21..67908bcf1 100644 --- a/src/core/write-through.ts +++ b/src/core/write-through.ts @@ -27,7 +27,10 @@ import { randomBytes } from 'crypto'; import type { BrainEngine } from './engine.ts'; import { serializePageToMarkdown, resolvePageFilePath } from './markdown.ts'; import { isWriteTargetContained } from './path-confine.ts'; -import { isDurabilityHardened, commitWriteThroughFile } from './brain-repo-durability.ts'; +import { + isDurabilityHardened, commitWriteThroughFile, currentBranch, getLastPushOutcome, + type PushLogOutcome, +} from './brain-repo-durability.ts'; /** Minimal logger surface — structurally compatible with operations.ts `Logger`. */ export interface WriteThroughLogger { @@ -39,11 +42,28 @@ export interface WriteThroughResult { path?: string; /** * True when the write was also committed to git (#2426). Only attempted on - * repos hardened via `gbrain sources harden` (durability hook installed); - * the hook then background-pushes the commit. Best-effort — a false/absent - * value never blocks the write. + * repos hardened via `gbrain sources harden` (durability hook installed). + * Commit-only — this says nothing about whether the commit ever reached the + * remote. Best-effort — a false/absent value never blocks the write. */ committed?: boolean; + /** + * Set alongside `committed: true`. The actual push runs detached in the + * post-commit hook (see brain-repo-durability.ts), so at the moment this + * result is returned the outcome for THIS commit is genuinely unknown — + * 'pending' is the only honest value. Check `lastPushStatus` (or + * $GBRAIN_HOME/brain-push.log directly) afterward to see whether pushes for + * this branch are landing. + */ + pushed?: 'pending'; + /** + * Best-effort snapshot of the most recently logged push outcome for this + * branch (read from the hook's shared log), taken right after the commit + * above. It reflects push history UP TO that point — not the push this + * write just queued — so callers and health tooling can tell "pushes for + * this branch have been failing" apart from "this write committed fine". + */ + lastPushStatus?: PushLogOutcome; /** * Non-error reasons the file was not written: * - no_repo_configured: the resolved target (source `local_path` or, for a @@ -284,13 +304,23 @@ export async function writePageThrough( // post-commit hook background-pushes the commit. Best-effort: a commit // failure never fails the write (the DB row + file are the durable sinks). let committed = false; + let pushed: 'pending' | undefined; + let lastPushStatus: PushLogOutcome | undefined; try { if (isDurabilityHardened(writeRoot)) { committed = commitWriteThroughFile(writeRoot, filePath, slug); + if (committed) { + pushed = 'pending'; + lastPushStatus = getLastPushOutcome(currentBranch(writeRoot)); + } } } catch { /* best-effort */ } - return { written: true, path: filePath, ...(committed ? { committed } : {}) }; + return { + written: true, + path: filePath, + ...(committed ? { committed, pushed, lastPushStatus } : {}), + }; } catch (e) { const msg = e instanceof Error ? e.message : String(e); opts.logger?.warn(`[write-through] failed for ${slug}: ${msg}`); diff --git a/test/put-page-push-reporting.serial.test.ts b/test/put-page-push-reporting.serial.test.ts new file mode 100644 index 000000000..6739527e9 --- /dev/null +++ b/test/put-page-push-reporting.serial.test.ts @@ -0,0 +1,144 @@ +/** + * put_page's write-through response on a durability-hardened repo reports + * `committed: true` for both a successful and a FAILED background push — the + * commit lands locally either way, but the push runs detached in the + * post-commit hook, so the caller had no field to distinguish "pushed fine" + * from "still local-only". This pins the honest contract: `committed` is + * commit-only, `pushed: 'pending'` says the push outcome isn't known yet, and + * `lastPushStatus` surfaces the hook's own log so a caller (or health + * tooling) can see whether pushes for this branch are currently landing. + */ + +import { describe, test, expect, beforeAll, beforeEach, afterEach, afterAll } from 'bun:test'; +import { mkdtempSync, writeFileSync, rmSync, mkdirSync, chmodSync, appendFileSync } from 'fs'; +import { execSync, execFileSync } from 'child_process'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { operations } from '../src/core/operations.ts'; +import type { OperationContext } from '../src/core/operations.ts'; +import { configureGateway, resetGateway, __setEmbedTransportForTests } from '../src/core/ai/gateway.ts'; + +const putPageOp = operations.find((o) => o.name === 'put_page')!; + +let engine: PGLiteEngine; +let repo: string; +let gbrainHome: string; +let oldGbrainHome: string | undefined; + +function git(cwd: string, ...args: string[]): string { + return execFileSync('git', ['-C', cwd, ...args], { + stdio: ['ignore', 'pipe', 'pipe'], encoding: 'utf-8', + }).trim(); +} + +/** Same fixture as write-through-commit.serial.test.ts: a hook file carrying + * the gbrain banner (the only thing `isDurabilityHardened` checks) with a + * no-op body so tests never attempt a real network push. */ +function installFakeDurabilityHook(repoPath: string): void { + const hooksDir = join(repoPath, '.git', 'hooks'); + mkdirSync(hooksDir, { recursive: true }); + const hookPath = join(hooksDir, 'post-commit'); + writeFileSync(hookPath, [ + '#!/usr/bin/env bash', + '# gbrain brain-durability post-commit hook (v0.42.44+)', + 'exit 0', + '', + ].join('\n')); + chmodSync(hookPath, 0o755); +} + +function makeCtx(opts: Partial = {}): OperationContext { + return { + engine, + config: { engine: 'pglite' as const }, + logger: { info: () => {}, warn: () => {}, error: () => {} }, + dryRun: false, + remote: false, + sourceId: 'default', + ...opts, + }; +} + +describe('put_page write-through — commit/push reporting on a hardened repo', () => { + beforeAll(async () => { + configureGateway({ + embedding_model: 'openai:text-embedding-3-large', + embedding_dimensions: 1536, + env: { ...process.env, OPENAI_API_KEY: process.env.OPENAI_API_KEY || 'sk-test-stub' }, + }); + __setEmbedTransportForTests(async ({ values }: any) => ({ + embeddings: values.map(() => new Array(1536).fill(0)), + usage: { tokens: 0 }, + }) as any); + + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + }, 60_000); + + afterAll(async () => { + await engine.disconnect(); + __setEmbedTransportForTests(null); + resetGateway(); + }, 60_000); + + beforeEach(async () => { + await resetPgliteState(engine); + repo = mkdtempSync(join(tmpdir(), 'gbrain-ppr-')); + execSync('git init -q -b main', { cwd: repo, stdio: 'pipe' }); + execSync('git config user.email "t@t.t"', { cwd: repo, stdio: 'pipe' }); + execSync('git config user.name "T"', { cwd: repo, stdio: 'pipe' }); + writeFileSync(join(repo, 'seed.md'), 'seed\n'); + execSync('git add -A && git commit -qm init', { cwd: repo, stdio: 'pipe' }); + installFakeDurabilityHook(repo); + await engine.setConfig('sync.repo_path', repo); + + gbrainHome = mkdtempSync(join(tmpdir(), 'gbrain-home-')); + oldGbrainHome = process.env.GBRAIN_HOME; + process.env.GBRAIN_HOME = gbrainHome; + }); + + afterEach(() => { + if (oldGbrainHome === undefined) delete process.env.GBRAIN_HOME; else process.env.GBRAIN_HOME = oldGbrainHome; + if (repo) rmSync(repo, { recursive: true, force: true }); + if (gbrainHome) rmSync(gbrainHome, { recursive: true, force: true }); + }); + + test('reports committed + pending push, not an implied success, with no push history yet', async () => { + const res: any = await putPageOp.handler(makeCtx(), { + slug: 'notes/ppr-fresh', + content: '---\ntype: concept\ntitle: PPR Fresh\n---\n\nbody', + }); + + expect(res.write_through.committed).toBe(true); + expect(res.write_through.pushed).toBe('pending'); + expect(res.write_through.lastPushStatus.status).toBe('unknown'); + }); + + test('surfaces a prior LOCAL-ONLY push failure instead of hiding it behind committed:true', async () => { + // Simulate the hook having already logged an unresolved push failure for + // this branch (e.g. from an earlier write in the same session). + // CX2-8: GBRAIN_HOME is a PARENT dir — the resolved home is + // $GBRAIN_HOME/.gbrain, so the push log lives under it. + mkdirSync(join(gbrainHome, '.gbrain'), { recursive: true }); + appendFileSync( + join(gbrainHome, '.gbrain', 'brain-push.log'), + '2025-01-01T00:00:00Z [push] LOCAL-ONLY, NEEDS ATTENTION: main @ deadbee could not reach origin. Run: gbrain sources pull && git push\n', + ); + + const res: any = await putPageOp.handler(makeCtx(), { + slug: 'notes/ppr-broken-push', + content: '---\ntype: concept\ntitle: PPR Broken Push\n---\n\nbody', + }); + + // The commit itself still succeeds (git commit doesn't touch the network) — + // that's the honest part of `committed: true`. What must NOT happen is the + // caller reading `committed: true` as "this is durably on the remote". + expect(res.write_through.committed).toBe(true); + expect(res.write_through.pushed).toBe('pending'); + expect(res.write_through.lastPushStatus.status).toBe('needs_attention'); + expect(res.write_through.lastPushStatus.detail).toContain('NEEDS ATTENTION'); + }); +});