mirror of https://github.com/garrytan/gstack.git
fix(review): close Grok packaging fail-open paths from code review
Fail closed on ARCHIVE_PATH allowlist when SPAWN_PATH is missing, require non-empty monorepo root before runtime links, drop default --always-approve, prefer --prompt-file for large Grok benchmark prompts, and make compat-audit refuse theater-green when required bridge skills are absent.
This commit is contained in:
Nehr
parent
9f8929f414
commit
1480052e77
|
|
@ -16,7 +16,7 @@
|
|||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import { execSync } from 'child_process';
|
||||
import { execFileSync } from 'child_process';
|
||||
|
||||
const ROOT = path.resolve(import.meta.dir, '..');
|
||||
const args = process.argv.slice(2);
|
||||
|
|
@ -227,16 +227,24 @@ function auditBridges() {
|
|||
// Install prose alone must not claim primary READY when CLI missing
|
||||
const area = 'bridge-policy';
|
||||
|
||||
// benchmark-models: grok provider must exist in binary
|
||||
// Prefer installed runtime root binary (what users run); fall back to monorepo.
|
||||
const installedBench = path.join(GSTACK_ROOT, 'bin', 'gstack-model-benchmark');
|
||||
const monorepoBench = path.join(ROOT, 'bin', 'gstack-model-benchmark');
|
||||
const benchBin = exists(installedBench) ? installedBench : monorepoBench;
|
||||
if (!exists(installedBench) && exists(monorepoBench)) {
|
||||
warn(area, 'benchmark: using monorepo binary (installed runtime bin missing) — packaging incomplete?');
|
||||
}
|
||||
try {
|
||||
const help = execSync(`"${path.join(ROOT, 'bin/gstack-model-benchmark')}" --prompt "x" --models grok --dry-run 2>&1`, {
|
||||
// No shell: argv list avoids monorepo-path injection.
|
||||
const help = execFileSync(benchBin, ['--prompt', 'x', '--models', 'grok', '--dry-run'], {
|
||||
encoding: 'utf-8',
|
||||
timeout: 15000,
|
||||
maxBuffer: 2 * 1024 * 1024,
|
||||
});
|
||||
if (/unknown provider.*grok/i.test(help)) {
|
||||
fail(area, 'benchmark: grok still unknown provider');
|
||||
} else if (/grok:\s*(OK|NOT READY)/i.test(help)) {
|
||||
ok(area, 'benchmark: grok provider wired (dry-run READY/NOT READY is boolean auth)');
|
||||
ok(area, `benchmark: grok provider wired via ${benchBin} (dry-run READY/NOT READY is boolean auth)`);
|
||||
} else {
|
||||
warn(area, `benchmark dry-run unexpected output: ${help.slice(0, 200)}`);
|
||||
}
|
||||
|
|
@ -248,30 +256,34 @@ function auditBridges() {
|
|||
else warn(area, `benchmark dry-run error: ${String(msg).slice(0, 200)}`);
|
||||
}
|
||||
|
||||
// spec: Grok package should not default to claude -p outside execute-claude
|
||||
// spec: at least one installed package must exist and use Grok-native spawn
|
||||
let specSeen = 0;
|
||||
for (const name of ['gstack-spec', 'spec']) {
|
||||
const skill = path.join(SKILLS_DIR, name, 'SKILL.md');
|
||||
if (!exists(skill)) continue;
|
||||
specSeen++;
|
||||
const body = readText(skill);
|
||||
if (/\$\(cat\s+["']?\$ARCHIVE/.test(body) || /grok[^\n]*\$\(cat/.test(body)) {
|
||||
fail(area, `${name}: banned $(cat …) into grok argv`);
|
||||
}
|
||||
if (/Spawned:.*claude -p/.test(body) && !/--execute-claude/.test(body) && !/optional/i.test(body)) {
|
||||
// Default path still Claude — fail for Grok host package
|
||||
if (/grok --prompt-file/.test(body) || /Spawn \*\*Grok\*\*/.test(body)) {
|
||||
ok(area, `${name}: Grok-native spawn present`);
|
||||
} else {
|
||||
fail(area, `${name}: default execute still only claude -p (no Grok spawn)`);
|
||||
}
|
||||
} else if (/grok --prompt-file/.test(body)) {
|
||||
ok(area, `${name}: uses researched grok --prompt-file`);
|
||||
if (/grok --prompt-file/.test(body) || /Spawn \*\*Grok\*\*/.test(body)) {
|
||||
ok(area, `${name}: Grok-native spawn present`);
|
||||
} else if (/Spawned:.*claude -p/.test(body) && !/--execute-claude/.test(body)) {
|
||||
fail(area, `${name}: default execute still only claude -p (no Grok spawn)`);
|
||||
} else {
|
||||
fail(area, `${name}: missing researched grok --prompt-file spawn`);
|
||||
}
|
||||
}
|
||||
if (specSeen === 0) {
|
||||
fail(area, 'spec: neither gstack-spec nor spec package installed under skills dir');
|
||||
}
|
||||
|
||||
// setup-gbrain: must not hard-require which claude for success
|
||||
let gbrainSeen = 0;
|
||||
for (const name of ['gstack-setup-gbrain', 'setup-gbrain']) {
|
||||
const skill = path.join(SKILLS_DIR, name, 'SKILL.md');
|
||||
if (!exists(skill)) continue;
|
||||
gbrainSeen++;
|
||||
const body = readText(skill);
|
||||
if (/Grok Build success path/.test(body) || /no Claude required/i.test(body)) {
|
||||
ok(area, `${name}: Grok success path documented`);
|
||||
|
|
@ -279,11 +291,16 @@ function auditBridges() {
|
|||
warn(area, `${name}: missing explicit Grok success path (CLI+AGENTS)`);
|
||||
}
|
||||
}
|
||||
if (gbrainSeen === 0) {
|
||||
warn(area, 'setup-gbrain: package not installed (bridge not audited)');
|
||||
}
|
||||
|
||||
// pair-agent: local must not hard-require ngrok; remote has install+security+teardown
|
||||
let pairSeen = 0;
|
||||
for (const name of ['gstack-pair-agent', 'pair-agent']) {
|
||||
const skill = path.join(SKILLS_DIR, name, 'SKILL.md');
|
||||
if (!exists(skill)) continue;
|
||||
pairSeen++;
|
||||
const body = readText(skill);
|
||||
if (/teardown/i.test(body) && /ngrok/i.test(body) && /127\.0\.0\.1|security/i.test(body)) {
|
||||
ok(area, `${name}: remote ngrok install+security+teardown present`);
|
||||
|
|
@ -292,6 +309,9 @@ function auditBridges() {
|
|||
}
|
||||
if (/DEPENDENT/.test(body)) ok(area, `${name}: honest DEPENDENT labeling`);
|
||||
}
|
||||
if (pairSeen === 0) {
|
||||
warn(area, 'pair-agent: package not installed (bridge not audited)');
|
||||
}
|
||||
}
|
||||
|
||||
// ─── Main ─────────────────────────────────────────────────────
|
||||
|
|
|
|||
|
|
@ -25,29 +25,37 @@ if [ ! -f "$HOME/.grok/auth.json" ] && [ -z "\${XAI_API_KEY:-}\${GROK_API_KEY:-}
|
|||
echo "STOP: Grok not authenticated (no ~/.grok/auth.json and no XAI_API_KEY/GROK_API_KEY). Log in via \`grok\`, or use --no-execute."
|
||||
exit 1
|
||||
fi
|
||||
# ARCHIVE_PATH must stay under SPAWN_PATH parent or the gstack projects allowlist
|
||||
ARCHIVE_REAL=$(cd "$(dirname "$ARCHIVE_PATH")" && pwd -P)/$(basename "$ARCHIVE_PATH")
|
||||
# ARCHIVE_PATH must stay under SPAWN_PATH or the gstack projects allowlist (fail closed).
|
||||
if [ ! -f "$ARCHIVE_PATH" ]; then
|
||||
echo "STOP: ARCHIVE_PATH missing: $ARCHIVE_PATH"; exit 1
|
||||
fi
|
||||
if command -v realpath >/dev/null 2>&1; then
|
||||
ARCHIVE_REAL=$(realpath "$ARCHIVE_PATH")
|
||||
else
|
||||
ARCHIVE_REAL=$(cd "$(dirname "$ARCHIVE_PATH")" && pwd -P)/$(basename "$ARCHIVE_PATH")
|
||||
fi
|
||||
SPAWN_REAL=$(cd "$SPAWN_PATH" 2>/dev/null && pwd -P || echo "")
|
||||
if [ -z "$SPAWN_REAL" ]; then
|
||||
echo "STOP: SPAWN_PATH is not a real directory: $SPAWN_PATH"; exit 1
|
||||
fi
|
||||
STATE_PROJECTS="\${GSTACK_STATE_ROOT:-\$HOME/.gstack}/projects"
|
||||
case "$ARCHIVE_REAL" in
|
||||
"$HOME/.gstack/projects"/*|"$HOME/.gstack/projects"/*/*) ;; # allowlisted archive dir
|
||||
"$STATE_PROJECTS"/*|"$SPAWN_REAL"/*) ;; # allowlisted
|
||||
*)
|
||||
if [ -n "$SPAWN_REAL" ]; then
|
||||
case "$ARCHIVE_REAL" in
|
||||
"$SPAWN_REAL"/*) ;;
|
||||
*) echo "STOP: ARCHIVE_PATH realpath not under SPAWN_PATH or allowlisted archive dir."; exit 1 ;;
|
||||
esac
|
||||
fi
|
||||
echo "STOP: ARCHIVE_PATH realpath not under SPAWN_PATH or allowlisted archive dir ($STATE_PROJECTS)."; exit 1
|
||||
;;
|
||||
esac
|
||||
\`\`\`
|
||||
|
||||
**Security:** \`--always-approve\` auto-approves tool use (elevated). Only spawn
|
||||
when the user confirmed the D16 gate. Spec archives must not contain secrets.
|
||||
Third-party note: archive body is sent to xAI for processing.
|
||||
**Security:** default spawn does **not** pass \`--always-approve\` (elevated
|
||||
auto-approve). Only spawn after the user confirmed the D16 gate. If the user
|
||||
explicitly opts into unattended tool use, append \`--always-approve\` to the
|
||||
command below — never enable it by default. Spec archives must not contain
|
||||
secrets. Third-party note: archive body is sent to xAI for processing.
|
||||
|
||||
\`\`\`bash
|
||||
# Prefer --prompt-file (researched). Optional elevated approve is documented opt-in.
|
||||
(cd "$SPAWN_PATH" && grok --prompt-file "$ARCHIVE_PATH" --cwd "$SPAWN_PATH" --always-approve 2>&1) &
|
||||
# Prefer --prompt-file (researched). Elevated --always-approve is opt-in only.
|
||||
(cd "$SPAWN_PATH" && grok --prompt-file "$ARCHIVE_PATH" --cwd "$SPAWN_PATH" 2>&1) &
|
||||
SPAWN_PID=$!
|
||||
echo "Spawned: PID $SPAWN_PID in $SPAWN_PATH (branch $SPAWN_BRANCH)"
|
||||
echo "Follow with: cd $SPAWN_PATH && grok --continue"
|
||||
|
|
|
|||
17
setup
17
setup
|
|
@ -1012,8 +1012,23 @@ _grok_link_under_monorepo() {
|
|||
fi
|
||||
|
||||
local src_real monorepo_real
|
||||
src_real=$(cd "$(dirname "$src")" 2>/dev/null && pwd -P)/$(basename "$src")
|
||||
monorepo_real=$(cd "$monorepo_root" 2>/dev/null && pwd -P)
|
||||
# Fail closed: empty monorepo_real makes "$monorepo_real"/* expand to /* and
|
||||
# match every absolute path — never allow that.
|
||||
if [ -z "$monorepo_real" ] || [ ! -d "$monorepo_real" ]; then
|
||||
echo "error: refusing Grok runtime link — monorepo root unresolved: $monorepo_root" >&2
|
||||
return 1
|
||||
fi
|
||||
# Prefer full realpath when available so a final-component symlink cannot escape.
|
||||
if command -v realpath >/dev/null 2>&1; then
|
||||
src_real=$(realpath "$src" 2>/dev/null) || src_real=""
|
||||
else
|
||||
src_real=$(cd "$(dirname "$src")" 2>/dev/null && pwd -P)/$(basename "$src")
|
||||
fi
|
||||
if [ -z "$src_real" ]; then
|
||||
echo "error: refusing Grok runtime link — cannot realpath src: $src" >&2
|
||||
return 1
|
||||
fi
|
||||
case "$src_real" in
|
||||
"$monorepo_real"|"$monorepo_real"/*) ;;
|
||||
*)
|
||||
|
|
|
|||
|
|
@ -1,12 +1,12 @@
|
|||
import type { ProviderAdapter, RunOpts, RunResult, AvailabilityCheck } from './types';
|
||||
import { estimateCostUsd } from '../pricing';
|
||||
import { execFileSync } from 'child_process';
|
||||
import { execFileSync, spawnSync } from 'child_process';
|
||||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
|
||||
/**
|
||||
* Grok adapter — wraps the `grok` CLI via -p / --single.
|
||||
* Grok adapter — wraps the `grok` CLI via -p / --single / --prompt-file.
|
||||
*
|
||||
* Auth readiness is boolean only: CLI present + (~/.grok/auth.json OR
|
||||
* XAI_API_KEY / GROK_API_KEY env names present). Never log token values.
|
||||
|
|
@ -16,12 +16,14 @@ export class GrokAdapter implements ProviderAdapter {
|
|||
readonly family = 'grok' as const;
|
||||
|
||||
async available(): Promise<AvailabilityCheck> {
|
||||
// Boolean PATH presence only — never log secrets
|
||||
let hasBinary = false;
|
||||
try {
|
||||
execFileSync('which', ['grok'], { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] });
|
||||
hasBinary = true;
|
||||
} catch {
|
||||
// Boolean PATH presence only — never log secrets. Bound to ≤2s like peer adapters.
|
||||
const which = spawnSync('sh', ['-c', 'command -v grok'], {
|
||||
timeout: 2000,
|
||||
encoding: 'utf-8',
|
||||
stdio: ['ignore', 'pipe', 'ignore'],
|
||||
});
|
||||
let hasBinary = which.status === 0;
|
||||
if (!hasBinary) {
|
||||
try {
|
||||
execFileSync('grok', ['--version'], {
|
||||
encoding: 'utf-8',
|
||||
|
|
@ -55,8 +57,22 @@ export class GrokAdapter implements ProviderAdapter {
|
|||
|
||||
async run(opts: RunOpts): Promise<RunResult> {
|
||||
const start = Date.now();
|
||||
// Short single-turn: -p / --single. Prefer file-free short prompts for benchmark.
|
||||
const args = ['--single', opts.prompt];
|
||||
// Prefer --prompt-file for multi-line / large prompts (ARG_MAX + quoting).
|
||||
// Short single-line prompts use --single to avoid temp files.
|
||||
const useFile =
|
||||
opts.prompt.includes('\n') || opts.prompt.length > 2000 || Buffer.byteLength(opts.prompt, 'utf8') > 2000;
|
||||
let promptFile: string | null = null;
|
||||
let args: string[];
|
||||
if (useFile) {
|
||||
promptFile = path.join(
|
||||
os.tmpdir(),
|
||||
`gstack-grok-bench-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}.txt`,
|
||||
);
|
||||
fs.writeFileSync(promptFile, opts.prompt, 'utf-8');
|
||||
args = ['--prompt-file', promptFile, '--cwd', opts.workdir];
|
||||
} else {
|
||||
args = ['--single', opts.prompt];
|
||||
}
|
||||
if (opts.model) args.push('--model', opts.model);
|
||||
if (opts.extraArgs) args.push(...opts.extraArgs);
|
||||
|
||||
|
|
@ -79,19 +95,29 @@ export class GrokAdapter implements ProviderAdapter {
|
|||
const durationMs = Date.now() - start;
|
||||
const e = err as { code?: string; stderr?: Buffer; signal?: string; message?: string };
|
||||
const stderr = e.stderr?.toString() ?? '';
|
||||
// Never surface raw stderr for auth paths (may contain token-shaped text).
|
||||
const safeSlice = (s: string) => s.replace(/[A-Za-z0-9_\-]{20,}/g, '[redacted]').slice(0, 200);
|
||||
if (e.signal === 'SIGTERM' || e.code === 'ETIMEDOUT') {
|
||||
return this.emptyResult(durationMs, { code: 'timeout', reason: `exceeded ${opts.timeoutMs}ms` }, opts.model);
|
||||
}
|
||||
if (/unauthorized|auth|login|api.?key/i.test(stderr)) {
|
||||
return this.emptyResult(durationMs, { code: 'auth', reason: stderr.slice(0, 400) }, opts.model);
|
||||
return this.emptyResult(durationMs, { code: 'auth', reason: 'authentication failed (details redacted)' }, opts.model);
|
||||
}
|
||||
if (/rate[- ]?limit|429/i.test(stderr)) {
|
||||
return this.emptyResult(durationMs, { code: 'rate_limit', reason: stderr.slice(0, 400) }, opts.model);
|
||||
return this.emptyResult(durationMs, { code: 'rate_limit', reason: safeSlice(stderr) }, opts.model);
|
||||
}
|
||||
if (/ENOENT|not found/i.test(e.message ?? '') || e.code === 'ENOENT') {
|
||||
return this.emptyResult(durationMs, { code: 'binary_missing', reason: 'grok CLI not found' }, opts.model);
|
||||
}
|
||||
return this.emptyResult(durationMs, { code: 'unknown', reason: (e.message ?? stderr ?? 'unknown').slice(0, 400) }, opts.model);
|
||||
return this.emptyResult(durationMs, { code: 'unknown', reason: safeSlice(e.message ?? stderr ?? 'unknown') }, opts.model);
|
||||
} finally {
|
||||
if (promptFile) {
|
||||
try {
|
||||
fs.unlinkSync(promptFile);
|
||||
} catch {
|
||||
// best-effort cleanup of temp prompt file
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue