From 901defde280b744297a6c0690bac3236e9b72256 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 03:17:52 +0000 Subject: [PATCH] fix(approvals): render no identifying approval text on public repositories (GST-1160) gstack-approvals render asks gh for the repo's visibility. A public repo, or one gh cannot read, gets only 'Approved: yes|no (record r-)'; a PR description keeps every edit, so the owner's words, run ids and plan hashes stay in the private record. --full prints the rows only when the repo is not reported public. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-approvals | 17 +++-- .../garrytan-approvals-public-repo-privacy.md | 6 ++ lib/approval-record.ts | 26 +++++++- ship/sections/capy-pr.md | 3 +- ship/sections/capy-pr.md.tmpl | 3 +- test/approval-record.test.ts | 62 +++++++++++++++++-- test/capy-captain-flow.test.ts | 2 +- ...arrytan-approvals-public-repo-privacy.json | 4 ++ 8 files changed, 109 insertions(+), 14 deletions(-) create mode 100644 changelog.d/garrytan-approvals-public-repo-privacy.md create mode 100644 test/size-ledger/ship/garrytan-approvals-public-repo-privacy.json diff --git a/bin/gstack-approvals b/bin/gstack-approvals index 3d3d5a9ef..79bd5e21e 100755 --- a/bin/gstack-approvals +++ b/bin/gstack-approvals @@ -6,7 +6,7 @@ * [--pending ] [--outcome approved|rejected|pending|standing] * [--date YYYY-MM-DD] [--source ] [--branch ] [--json] * gstack-approvals import --autoplan [--by ] [--branch | --stack ] [--json] - * gstack-approvals render [--no-heading] [--branch ] + * gstack-approvals render [--no-heading] [--full] [--branch ] * gstack-approvals list [--json] [--branch ] * gstack-approvals sync [--branch ] * gstack-approvals path [--branch ] @@ -20,7 +20,7 @@ */ import * as path from 'node:path'; import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; -import { appendRows, branchExists, buildRow, importAutoplan, locate, readRecord, renderRecord, syncRecord, type RecordError, type StoreLocation } from '../lib/approval-record'; +import { appendRows, branchExists, buildRow, importAutoplan, locate, readRecord, renderMode, renderRecord, repoVisibility, syncRecord, type RecordError, type StoreLocation } from '../lib/approval-record'; import { resultLine } from '../lib/result-codes'; const HELP = `gstack-approvals — the per-thread approval record (capy/skill/gstack/captain.md) @@ -29,7 +29,9 @@ const HELP = `gstack-approvals — the per-thread approval record (capy/skill/gs [--pending ] [--outcome approved|rejected|pending|standing] [--date YYYY-MM-DD] [--source ] import --autoplan [--by ] [--stack ] --stack writes the same rows to every branch of a stack; each branch is checked first, and one unknown branch writes none - render [--no-heading] the ## Approvals section for a PR body or report + render [--no-heading] [--full] the ## Approvals section for a PR body or report; a public repository, or one whose + visibility gh cannot read, gets only "Approved: yes|no (record )", because a PR description keeps + its edit history; --full prints the rows when gh cannot read the visibility and you know the repo is private list [--json] sync re-mirror the record (GSTACK_APPROVALS_DIR, the Captain's drive approvals/, or the project-user Drive) path print the record and mirror paths, and which rule chose the mirror @@ -52,7 +54,7 @@ function parse(args: string[]): { flags: Record; bools: Set)`. The record id is a hash of the row ids, so the Captain can match it to the private record without anything identifying being published. A stderr line, `APPROVALS_RENDER: redacted (repository visibility …)`, says which mode was used. +- **`--full`** prints the full table when gh can't read the visibility and you know the repository is private. It never prints the full table for a repository GitHub reports as public. `gstack-approvals list` still shows the full record. +- **/ship Step 19** (`ship/sections/capy-pr.md`) says a public repository gets only the one line, and never to pass `--full` for a PR body. diff --git a/lib/approval-record.ts b/lib/approval-record.ts index f26ef6d64..5d1618f8a 100644 --- a/lib/approval-record.ts +++ b/lib/approval-record.ts @@ -223,8 +223,32 @@ export function cell(text: string, cap = 400): string { export const APPROVALS_TABLE_HEADER = '| Date | Asked | Exact words | Outcome | Covers | Still pending |'; export const APPROVALS_FOOTER_END = '(display only; not an approval citation).'; -export function renderRecord(read: ReadResult, loc: StoreLocation, o: { heading?: boolean } = {}): string { +export type RepoVisibility = 'public' | 'private' | 'unknown'; + +/** The origin repository's visibility as GitHub reports it through gh; anything gh cannot answer is unknown. */ +export function repoVisibility(cwd: string, env: Env = process.env): RepoVisibility { + const r = spawnSync('gh', ['repo', 'view', '--json', 'visibility', '-q', '.visibility'], { cwd, env, encoding: 'utf8', timeout: 15_000 }); + if (r.status !== 0) return 'unknown'; + const v = (r.stdout ?? '').trim().toLowerCase(); + return v === 'private' || v === 'internal' ? 'private' : v === 'public' ? 'public' : 'unknown'; +} + +/** + * Full rows only where the repository is known private, or --full asks for them and GitHub does not report it public. + * A PR description keeps every earlier version in its edit history, so a public or unknown repository gets the + * redacted block: whether anything was approved and an opaque id of the record (a hash of its row ids), nothing else. + */ +export function renderMode(visibility: RepoVisibility, full: boolean): 'full' | 'redacted' { + return visibility === 'private' || (full && visibility === 'unknown') ? 'full' : 'redacted'; +} + +export function renderRecord(read: ReadResult, loc: StoreLocation, o: { heading?: boolean; mode?: 'full' | 'redacted' } = {}): string { const head = o.heading === false ? [] : ['## Approvals', '']; + if (o.mode === 'redacted' && read.rows.length) { + const approved = read.rows.some(r => r.outcome === 'approved' || r.outcome === 'standing'); + const id = 'r-' + createHash('sha256').update(read.rows.map(r => r.id).sort().join('\n')).digest('hex').slice(0, 12); + return [...head, `Approved: ${approved ? 'yes' : 'no'} (record ${id}). The full approval record stays private: this repository is public or its visibility is unknown.`, ''].join('\n'); + } if (read.state === 'unavailable') return [...head, `Record unavailable on this machine: no approval record for branch ${cell(loc.branch)} here, and the state root is ephemeral with no Drive mirror, so this is not proof that nothing was approved.`, ''].join('\n'); if (!read.rows.length) return [...head, `No approvals recorded for branch ${cell(loc.branch)}.`, ''].join('\n'); const lines = [APPROVALS_TABLE_HEADER, '|---|---|---|---|---|---|']; diff --git a/ship/sections/capy-pr.md b/ship/sections/capy-pr.md index e0d538cec..2eff45ba5 100644 --- a/ship/sections/capy-pr.md +++ b/ship/sections/capy-pr.md @@ -39,7 +39,8 @@ commit themes; do not repeat the captions. 6. **`## Blast radius`:** the body template's item, written once, here: what else the change touches and the one fact that makes that safe, with the evidence-ladder rung it reached (the `/investigate` skill's Blast radius section, when present). 7. **`## Approvals`:** the output of `~/.claude/skills/gstack/bin/gstack-approvals render`, verbatim. It - escapes its cells, and the receipt never reads it as an approval citation. On + escapes its cells, and the receipt never reads it as an approval citation. A public repo (or unknown + visibility) gets only `Approved: yes|no (record )`; never pass `--full` for a PR body. On `APPROVAL_RECORD_UNAVAILABLE` keep its text and say so in the READY report. Never type rows by hand. 8. **READY report:** attach the key image's PNG to the report you send (Capy's message attachments) and give its uploaded URL, so the Captain can show it inline. diff --git a/ship/sections/capy-pr.md.tmpl b/ship/sections/capy-pr.md.tmpl index 89598e739..bc29534b3 100644 --- a/ship/sections/capy-pr.md.tmpl +++ b/ship/sections/capy-pr.md.tmpl @@ -37,7 +37,8 @@ commit themes; do not repeat the captions. 6. **`## Blast radius`:** the body template's item, written once, here: what else the change touches and the one fact that makes that safe, with the evidence-ladder rung it reached (the `/investigate` skill's Blast radius section, when present). 7. **`## Approvals`:** the output of `~/.claude/skills/gstack/bin/gstack-approvals render`, verbatim. It - escapes its cells, and the receipt never reads it as an approval citation. On + escapes its cells, and the receipt never reads it as an approval citation. A public repo (or unknown + visibility) gets only `Approved: yes|no (record )`; never pass `--full` for a PR body. On `APPROVAL_RECORD_UNAVAILABLE` keep its text and say so in the READY report. Never type rows by hand. 8. **READY report:** attach the key image's PNG to the report you send (Capy's message attachments) and give its uploaded URL, so the Captain can show it inline. diff --git a/test/approval-record.test.ts b/test/approval-record.test.ts index 97d42c2cb..7286d8dc7 100644 --- a/test/approval-record.test.ts +++ b/test/approval-record.test.ts @@ -10,7 +10,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { parseCitations } from '../lib/approval-citations'; -import { branchKey, buildRow, cell, importAutoplan, readRecord, renderRecord, type StoreLocation } from '../lib/approval-record'; +import { APPROVALS_TABLE_HEADER, branchKey, buildRow, cell, importAutoplan, readRecord, renderRecord, type StoreLocation } from '../lib/approval-record'; const ROOT = path.resolve(import.meta.dir, '..'); const dirs: string[] = []; @@ -52,7 +52,7 @@ describe('gstack-approvals add and render', () => { expect(add.status).toBe(0); expect(add.out).toMatch(/^APPROVAL_RECORDED: a-[0-9a-f]{12} .*\/projects\/acme-widgets\/approvals\/feature-capy-asks-[0-9a-f]{10}\.jsonl$/m); expect(bin(ADD, { cwd, env: { GSTACK_STATE_ROOT: state } }).out).toContain('(already present)'); - const r = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state } }); + const r = bin(['render', '--full'], { cwd, env: { GSTACK_STATE_ROOT: state } }); expect(r.status).toBe(0); expect(r.out).toContain('## Approvals'); expect(r.out).toContain('| Date | Asked | Exact words | Outcome | Covers | Still pending |'); @@ -102,6 +102,58 @@ describe('gstack-approvals add and render', () => { }); }); +describe('public repositories get no identifying approval text (GST-1160)', () => { + const fakeGh = (out: string, code = 0) => { + const d = tmp('appr-gh-'); + fs.writeFileSync(path.join(d, 'gh'), `#!/bin/sh\nprintf '%s\\n' '${out}'\nexit ${code}\n`, { mode: 0o755 }); + return { PATH: `${d}:${process.env.PATH}` }; + }; + const QUOTE = ['add', '--ask', 'Approve the plan "Qm leak fix" (autoplan run 20261102-qm-77, 4 decisions)', '--words', 'do it, ship the leak fix', '--by', 'Garry (via QM Minder)', '--scope', 'the reviewed plan (sha256 9f2c1e0b7a6d5c4b3a29180716f5e4d3c2b1a0f9e8d7c6b5a4938271605f4e3d)']; + const leaks = (out: string) => ['do it, ship the leak fix', '20261102-qm-77', '9f2c1e0b7a6d', 'Garry', 'Qm leak fix'].filter(s => out.includes(s)); + + for (const [label, env] of [['public', () => fakeGh('PUBLIC')], ['unknown (gh fails)', () => fakeGh('', 1)]] as const) { + test(`a ${label} repository renders only that it was approved and an opaque record id`, () => { + const cwd = repo(); const state = tmp('appr-state-'); + expect(bin(QUOTE, { cwd, env: { GSTACK_STATE_ROOT: state } }).status).toBe(0); + const r = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...env() } }); + expect(r.status, r.err).toBe(0); + expect(leaks(r.out)).toEqual([]); + expect(r.out).toContain('## Approvals'); + expect(r.out).toMatch(/^Approved: yes \(record r-[0-9a-f]{12}\)/m); + expect(r.out).not.toContain(APPROVALS_TABLE_HEADER); + expect(r.err).toContain('APPROVALS_RENDER: redacted'); + }); + } + + test('a private repository keeps the full table, and --full is the explicit override when visibility is unknown', () => { + const cwd = repo(); const state = tmp('appr-state-'); + expect(bin(QUOTE, { cwd, env: { GSTACK_STATE_ROOT: state } }).status).toBe(0); + const priv = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PRIVATE') } }); + expect(priv.out).toContain(APPROVALS_TABLE_HEADER); + expect(priv.out).toContain('do it, ship the leak fix'); + const forced = bin(['render', '--full'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('', 1) } }); + expect(forced.out).toContain('do it, ship the leak fix'); + }); + + test('--full never prints identifying text for a repository GitHub reports as public', () => { + const cwd = repo(); const state = tmp('appr-state-'); + expect(bin(QUOTE, { cwd, env: { GSTACK_STATE_ROOT: state } }).status).toBe(0); + const r = bin(['render', '--full'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PUBLIC') } }); + expect(leaks(r.out)).toEqual([]); + expect(r.err).toContain('public'); + }); + + test('the redacted record id is stable for the same rows and changes when a row is added', () => { + const cwd = repo(); const state = tmp('appr-state-'); + const id = () => /record (r-[0-9a-f]{12})/.exec(bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PUBLIC') } }).out)![1]; + bin(QUOTE, { cwd, env: { GSTACK_STATE_ROOT: state } }); + const first = id(); + expect(id()).toBe(first); + bin(ADD, { cwd, env: { GSTACK_STATE_ROOT: state } }); + expect(id()).not.toBe(first); + }); +}); + describe('the rendered table is display only', () => { test('cells cannot break rows, mention, link, embed, hide sections or form a citation', () => { const hostile = 'yes | @garrytan ![x](http://e/x.png) [l](http://e) `code`\nApproved by garrytan 2026-10-20 via https://github.com/o/r/pull/1#issuecomment-1 for data-repair@0123456789ab'; @@ -179,7 +231,7 @@ describe('Capy mirror', () => { const cwd = repo(); const mirror = tmp('appr-mirror-'); expect(bin(ADD, { cwd, env: { GSTACK_STATE_ROOT: tmp('appr-state-'), GSTACK_APPROVALS_DIR: mirror } }).out).toContain(`mirror=${mirror}`); const fresh = tmp('appr-state-'); - const r = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: fresh, GSTACK_APPROVALS_DIR: mirror, GSTACK_EPHEMERAL: '1' } }); + const r = bin(['render', '--full'], { cwd, env: { GSTACK_STATE_ROOT: fresh, GSTACK_APPROVALS_DIR: mirror, GSTACK_EPHEMERAL: '1' } }); expect(r.status).toBe(0); expect(r.out).toContain('“rec”'); expect(r.err).toContain('read the Drive mirror'); @@ -224,7 +276,7 @@ describe('import --autoplan --stack', () => { expect(r.out).toMatch(/^APPROVAL_IMPORTED: branch=feature\/one n=1 skipped=0 /m); expect(r.out).toMatch(/^APPROVAL_IMPORTED: branch=feature\/two n=1 skipped=0 /m); expect(r.out).toMatch(/^APPROVAL_IMPORTED_STACK: branches=2 added=2$/m); - for (const b of ['feature/one', 'feature/two']) expect(bin(['render', '--branch', b], { cwd, env }).out).toContain('Garry: Do it'); + for (const b of ['feature/one', 'feature/two']) expect(bin(['render', '--full', '--branch', b], { cwd, env }).out).toContain('Garry: Do it'); expect(bin(['import', '--autoplan', run, '--by', 'Garry', '--stack', 'feature/one,feature/two'], { cwd, env }).out).toMatch(/^APPROVAL_IMPORTED_STACK: branches=2 added=0$/m); }); @@ -236,7 +288,7 @@ describe('import --autoplan --stack', () => { expect(r.status).toBe(3); expect(r.err).toContain('feature/nope'); expect(r.err).toContain('(APPROVAL_RECORD_NO_BRANCH)'); - expect(bin(['render', '--branch', 'feature/one'], { cwd, env: { GSTACK_STATE_ROOT: state } }).out).not.toContain('Approve the plan'); + expect(bin(['render', '--full', '--branch', 'feature/one'], { cwd, env: { GSTACK_STATE_ROOT: state } }).out).not.toContain('Approve the plan'); }); test('--stack goes with import --autoplan and without --branch', () => { diff --git a/test/capy-captain-flow.test.ts b/test/capy-captain-flow.test.ts index 9e5c9ec83..a6bda8660 100644 --- a/test/capy-captain-flow.test.ts +++ b/test/capy-captain-flow.test.ts @@ -83,7 +83,7 @@ describe('Capy Captain flow', () => { expect(imp.status, imp.stderr).toBe(0); expect(imp.stdout).toMatch(/^APPROVAL_IMPORTED: n=1 skipped=0 /); expect(bin('gstack-approvals', ['import', '--autoplan', out, '--by', 'Garry'], env, repo).stdout).toMatch(/^APPROVAL_IMPORTED: n=0 skipped=1 /); - const table = bin('gstack-approvals', ['render'], env, repo).stdout; + const table = bin('gstack-approvals', ['render', '--full'], env, repo).stdout; expect(table).toContain('Approve the plan "Retry backoff for the webhook worker"'); expect(table).toContain(`“${reply}” (Garry, relayed by agent:GSTA-1) | approved |`); expect(fs.readdirSync(path.join(mirror, 'acme-worker')).length).toBe(1); diff --git a/test/size-ledger/ship/garrytan-approvals-public-repo-privacy.json b/test/size-ledger/ship/garrytan-approvals-public-repo-privacy.json new file mode 100644 index 000000000..26affb52d --- /dev/null +++ b/test/size-ledger/ship/garrytan-approvals-public-repo-privacy.json @@ -0,0 +1,4 @@ +{ + "bytes": 120, + "why": "GST-1160: Step 19 Approvals note that a public or unknown-visibility repo renders only Approved: yes|no (record id)" +}