fix: preserve gateguard concurrent state writes (#1623)

This commit is contained in:
Affaan Mustafa 2026-04-29 19:31:11 -04:00 committed by GitHub
parent 468c755abd
commit c3ea7a1e5e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 80 additions and 5 deletions

View File

@ -129,12 +129,33 @@ function saveState(state) {
const stateFile = getStateFile();
let tmpFile = null;
try {
state.last_active = Date.now();
state.checked = pruneCheckedEntries(state.checked);
fs.mkdirSync(STATE_DIR, { recursive: true });
let mergedChecked = Array.isArray(state.checked) ? state.checked : [];
let mergedLastActive = typeof state.last_active === 'number' ? state.last_active : 0;
try {
if (fs.existsSync(stateFile)) {
const diskState = JSON.parse(fs.readFileSync(stateFile, 'utf8'));
if (Array.isArray(diskState.checked)) {
mergedChecked = Array.from(new Set([...diskState.checked, ...mergedChecked]));
}
if (typeof diskState.last_active === 'number') {
mergedLastActive = Math.max(mergedLastActive, diskState.last_active);
}
}
} catch (_) {
/* ignore malformed or transient disk state */
}
const finalState = {
checked: pruneCheckedEntries(mergedChecked),
last_active: Math.max(mergedLastActive, Date.now())
};
// Atomic write: temp file + rename prevents partial reads
tmpFile = stateFile + '.tmp.' + process.pid;
fs.writeFileSync(tmpFile, JSON.stringify(state, null, 2), 'utf8');
tmpFile = `${stateFile}.tmp.${process.pid}.${crypto.randomBytes(4).toString('hex')}`;
fs.writeFileSync(tmpFile, JSON.stringify(finalState, null, 2), 'utf8');
try {
fs.renameSync(tmpFile, stateFile);
} catch (error) {
@ -149,6 +170,7 @@ function saveState(state) {
throw error;
}
}
tmpFile = null;
} catch (_) {
if (tmpFile) {
try {
@ -183,7 +205,8 @@ function isChecked(key) {
const files = fs.readdirSync(STATE_DIR);
const now = Date.now();
for (const f of files) {
if (!f.startsWith('state-') || !f.endsWith('.json')) continue;
const isStateFile = f.startsWith('state-') && (f.endsWith('.json') || f.includes('.json.tmp.'));
if (!isStateFile) continue;
const fp = path.join(STATE_DIR, f);
try {
const stat = fs.statSync(fp);

View File

@ -752,6 +752,58 @@ function runTests() {
assert.ok(reason.includes('evil.js'), 'sanitized path should retain visible filename text');
})) passed++; else failed++;
// --- Test 28: saveState preserves concurrent disk updates ---
clearState();
if (test('merges state written by another process during save', () => {
const hook = loadDirectHook();
const originalMkdirSync = fs.mkdirSync;
let injected = false;
fs.mkdirSync = function patchedMkdirSync(target) {
const result = originalMkdirSync.apply(fs, arguments);
if (!injected && path.resolve(String(target)) === path.resolve(stateDir)) {
injected = true;
fs.writeFileSync(stateFile, JSON.stringify({
checked: ['/src/concurrent.js'],
last_active: Date.now()
}), 'utf8');
}
return result;
};
try {
const result = hook.run({
tool_name: 'Edit',
tool_input: { file_path: '/src/new-edit.js', old_string: 'a', new_string: 'b' }
});
const output = parseOutput(result.stdout);
assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', 'first edit should still be gated');
} finally {
fs.mkdirSync = originalMkdirSync;
}
const persisted = JSON.parse(fs.readFileSync(stateFile, 'utf8'));
assert.ok(persisted.checked.includes('/src/concurrent.js'), 'concurrent disk entry should be preserved');
assert.ok(persisted.checked.includes('/src/new-edit.js'), 'new in-memory entry should be persisted');
})) passed++; else failed++;
// --- Test 29: stale temp files from interrupted writes are pruned ---
clearState();
if (test('prunes stale state temp files at module load', () => {
fs.mkdirSync(stateDir, { recursive: true });
const staleTmp = path.join(stateDir, `${path.basename(stateFile)}.tmp.1234.abcd`);
const freshState = path.join(stateDir, 'state-fresh-session.json');
fs.writeFileSync(staleTmp, '{}', 'utf8');
fs.writeFileSync(freshState, '{}', 'utf8');
const staleTime = new Date(Date.now() - (61 * 60 * 1000));
fs.utimesSync(staleTmp, staleTime, staleTime);
loadDirectHook();
assert.ok(!fs.existsSync(staleTmp), 'stale temp state file should be pruned');
assert.ok(fs.existsSync(freshState), 'fresh state file should remain');
})) passed++; else failed++;
// Cleanup only the temp directory created by this test file.
try {
if (fs.existsSync(stateDir)) {