Skip to content

Commit fc3cc28

Browse files
sd0xdevsd0xdev
committed
fix: Extend sidecar .blocked protection to doc review path
The .blocked sidecar fail-closed mechanism only protected the code review path (DUAL_GATE_PASSED), leaving doc review unguarded during lock contention. This caused stop-guard and auto-loop hooks to miss pending doc reviews when state writes were lost to race conditions. - stop-guard: sidecar now forces DOC_REVIEW_PASSED=false + skips stale-state reconciliation when sidecar present - 3 auto-loop hooks: add sidecar check with HAS_* fail-closed forcing and stale-state skip - post-edit-format: merge 5 sequential jq writes into 1 atomic call for doc section; conditional sidecar clear on write success - post-tool-review-state: clear stale sidecar only after successful locked update_state write
1 parent 82a7711 commit fc3cc28

10 files changed

Lines changed: 215 additions & 16 deletions

‎hooks/post-compact-auto-loop.sh‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,22 @@ CODE_PASSED=$(jq -r '.code_review.passed // false' "$STATE_FILE" 2>/dev/null ||
5151
DOC_PASSED=$(jq -r '.doc_review.passed // false' "$STATE_FILE" 2>/dev/null || echo "false")
5252
PRE_PASSED=$(jq -r '.precommit.passed // false' "$STATE_FILE" 2>/dev/null || echo "false")
5353

54+
# === Sidecar fail-closed marker ===
55+
if [[ -f "${STATE_FILE}.blocked" ]]; then
56+
CODE_PASSED="false"
57+
DOC_PASSED="false"
58+
PRE_PASSED="false"
59+
# Fail-closed: if no change flags set, sidecar means state write failed — assume changes exist
60+
[[ "$HAS_CODE" != "true" && "$HAS_DOC" != "true" ]] && { HAS_CODE="true"; HAS_DOC="true"; }
61+
fi
62+
5463
# Stale-state reconciliation (one-way: true→false only, same as stop-guard)
55-
GIT_PORCELAIN=$(git status --porcelain -uno 2>/dev/null || echo "__GIT_UNAVAILABLE__")
64+
# Skip when sidecar present — would undo fail-closed HAS_* forcing
65+
if [[ -f "${STATE_FILE}.blocked" ]]; then
66+
GIT_PORCELAIN="__GIT_UNAVAILABLE__"
67+
else
68+
GIT_PORCELAIN=$(git status --porcelain -uno 2>/dev/null || echo "__GIT_UNAVAILABLE__")
69+
fi
5670
if [[ "$GIT_PORCELAIN" != "__GIT_UNAVAILABLE__" ]]; then
5771
if [[ "$HAS_CODE" == "true" ]]; then
5872
if ! echo "$GIT_PORCELAIN" | grep -qE '\.(ts|tsx|js|jsx|mjs|cjs|py|pyw|go|rs|java|kt|kts|rb|php|swift|c|cpp|cc|h|hpp|cs|scala|ex|exs)($|\s|")'; then

‎hooks/post-edit-format.sh‎

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -349,22 +349,61 @@ fi
349349
# Track doc changes (.md, .mdx)
350350
if echo "$file_path" | grep -Eq '\.(md|mdx)$'; then
351351
if _lock; then
352-
update_change_flag "has_doc_change"
352+
# Atomic: merge flag set + review invalidation + aggregate gate reset (3 ops → 1 jq call)
353+
init_state_file
354+
_doc_now=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
355+
_doc_has_agg=$(jq 'has("aggregate_gate")' "$STATE_FILE" 2>/dev/null || echo "false")
356+
_doc_tmp=$(mktemp)
357+
_doc_write_ok=false
358+
if [[ "$_doc_has_agg" == "true" ]]; then
359+
jq --arg now "$_doc_now" '
360+
.has_doc_change = true
361+
| .updated_at = $now
362+
| .doc_review.passed = false
363+
| .aggregate_gate.executed = false
364+
| .aggregate_gate.gate = null
365+
| .aggregate_gate.reason = null
366+
' "$STATE_FILE" > "$_doc_tmp" && mv "$_doc_tmp" "$STATE_FILE" && _doc_write_ok=true
367+
else
368+
jq --arg now "$_doc_now" '
369+
.has_doc_change = true
370+
| .updated_at = $now
371+
| .doc_review.passed = false
372+
' "$STATE_FILE" > "$_doc_tmp" && mv "$_doc_tmp" "$STATE_FILE" && _doc_write_ok=true
373+
fi
374+
# Non-critical array appends (graceful, own size guards)
353375
_track_changed_file "$file_path"
354376
_track_session_touched_file "$file_path" || true
355-
invalidate_review "doc_review"
356-
invalidate_aggregate_gate
357-
# Clear any stale sidecar marker (successful locked write supersedes prior lock-failure markers)
358-
rm -f "${STATE_FILE}.blocked" 2>/dev/null || true
377+
# Clear sidecar only on successful write (fail-closed: preserve sidecar if write failed)
378+
if [[ "$_doc_write_ok" == "true" ]]; then
379+
rm -f "${STATE_FILE}.blocked" 2>/dev/null || true
380+
fi
359381
_unlock
360382
echo "[Edit Hook] Doc change detected: $file_path" >&2
361383
echo "[Edit Hook] Invalidated doc_review + aggregate_gate" >&2
362384
else
363-
# Fail-closed: sidecar marker (atomic) + best-effort unlocked writes
385+
# Fail-closed: sidecar marker (atomic) + best-effort single unlocked jq write
364386
echo "edit_lock_contention" > "${STATE_FILE}.blocked" 2>/dev/null || true
365-
update_change_flag "has_doc_change" 2>/dev/null || true
366-
invalidate_review "doc_review" 2>/dev/null || true
367-
invalidate_aggregate_gate 2>/dev/null || true
387+
init_state_file
388+
_doc_now=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
389+
_doc_has_agg=$(jq 'has("aggregate_gate")' "$STATE_FILE" 2>/dev/null || echo "false")
390+
_doc_tmp=$(mktemp)
391+
if [[ "$_doc_has_agg" == "true" ]]; then
392+
jq --arg now "$_doc_now" '
393+
.has_doc_change = true
394+
| .updated_at = $now
395+
| .doc_review.passed = false
396+
| .aggregate_gate.executed = false
397+
| .aggregate_gate.gate = null
398+
| .aggregate_gate.reason = null
399+
' "$STATE_FILE" > "$_doc_tmp" 2>/dev/null && mv "$_doc_tmp" "$STATE_FILE" 2>/dev/null || rm -f "$_doc_tmp" 2>/dev/null
400+
else
401+
jq --arg now "$_doc_now" '
402+
.has_doc_change = true
403+
| .updated_at = $now
404+
| .doc_review.passed = false
405+
' "$STATE_FILE" > "$_doc_tmp" 2>/dev/null && mv "$_doc_tmp" "$STATE_FILE" 2>/dev/null || rm -f "$_doc_tmp" 2>/dev/null
406+
fi
368407
echo "[Edit Hook] Doc change detected (degraded — lock contention, sidecar marker set): $file_path" >&2
369408
fi
370409
fi

‎hooks/post-skill-auto-loop.sh‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,22 @@ CODE_PASSED=$(jq -r '.code_review.passed // false' "$STATE_FILE" 2>/dev/null ||
5050
DOC_PASSED=$(jq -r '.doc_review.passed // false' "$STATE_FILE" 2>/dev/null || echo "false")
5151
PRE_PASSED=$(jq -r '.precommit.passed // false' "$STATE_FILE" 2>/dev/null || echo "false")
5252

53+
# === Sidecar fail-closed marker ===
54+
if [[ -f "${STATE_FILE}.blocked" ]]; then
55+
CODE_PASSED="false"
56+
DOC_PASSED="false"
57+
PRE_PASSED="false"
58+
# Fail-closed: if no change flags set, sidecar means state write failed — assume changes exist
59+
[[ "$HAS_CODE" != "true" && "$HAS_DOC" != "true" ]] && { HAS_CODE="true"; HAS_DOC="true"; }
60+
fi
61+
5362
# Stale-state reconciliation (one-way: true→false only, same as stop-guard/post-compact)
54-
GIT_PORCELAIN=$(git status --porcelain -uno 2>/dev/null || echo "__GIT_UNAVAILABLE__")
63+
# Skip when sidecar present — would undo fail-closed HAS_* forcing
64+
if [[ -f "${STATE_FILE}.blocked" ]]; then
65+
GIT_PORCELAIN="__GIT_UNAVAILABLE__"
66+
else
67+
GIT_PORCELAIN=$(git status --porcelain -uno 2>/dev/null || echo "__GIT_UNAVAILABLE__")
68+
fi
5569
if [[ "$GIT_PORCELAIN" != "__GIT_UNAVAILABLE__" ]]; then
5670
if [[ "$HAS_CODE" == "true" ]]; then
5771
if ! echo "$GIT_PORCELAIN" | grep -qE '\.(ts|tsx|js|jsx|mjs|cjs|py|pyw|go|rs|java|kt|kts|rb|php|swift|c|cpp|cc|h|hpp|cs|scala|ex|exs)($|\s|")'; then

‎hooks/post-tool-review-state.sh‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,12 +178,15 @@ update_state() {
178178
# Update using jq
179179
local tmp
180180
tmp=$(mktemp)
181-
jq --arg key "$key" \
181+
if jq --arg key "$key" \
182182
--argjson executed "$executed" \
183183
--argjson passed "$passed" \
184184
--arg now "$now" \
185185
'.[$key].executed = $executed | .[$key].passed = $passed | .[$key].last_run = $now | .updated_at = $now' \
186-
"$STATE_FILE" > "$tmp" && mv "$tmp" "$STATE_FILE"
186+
"$STATE_FILE" > "$tmp" && mv "$tmp" "$STATE_FILE"; then
187+
# Successful locked write → state is consistent, clear stale sidecar
188+
rm -f "${STATE_FILE}.blocked" 2>/dev/null || true
189+
fi
187190
_unlock
188191
else
189192
# Fail-open for review state (not as critical as aggregate gate)

‎hooks/stop-guard.sh‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,11 @@ if [[ -f "$STATE_FILE" ]]; then
115115
GUARD_MODE="strict"
116116
SIDECAR_REASON=$(cat "${STATE_FILE}.blocked" 2>/dev/null || echo "unknown")
117117
echo "[Stop Guard] Sidecar blocked marker found (reason: $SIDECAR_REASON)" >&2
118-
# Force aggregate gate to BLOCKED regardless of JSON state
118+
# Force aggregate gate + doc review to BLOCKED regardless of JSON state
119119
DUAL_GATE_PASSED="false"
120+
DOC_REVIEW_PASSED="false"
121+
# Fail-closed: sidecar means state may be corrupted, assume changes exist
122+
[[ "$HAS_CODE_CHANGE" != "true" && "$HAS_DOC_CHANGE" != "true" ]] && { HAS_CODE_CHANGE="true"; HAS_DOC_CHANGE="true"; }
120123
fi
121124

122125
# === Dual mode: prefer aggregate_gate + force strict blocking ===
@@ -153,7 +156,12 @@ if [[ -f "$STATE_FILE" ]]; then
153156
fi
154157

155158
# === Stale-state git check (with cross-platform timeout) ===
156-
if command -v timeout &>/dev/null; then
159+
# Skip stale-state reconciliation when sidecar is present — sidecar means state is
160+
# unreliable, so git-based downgrade would undo the fail-closed HAS_* forcing above.
161+
if [[ -f "${STATE_FILE}.blocked" ]]; then
162+
# Sidecar present → skip stale-state reconciliation (would undo fail-closed HAS_* forcing)
163+
GIT_PORCELAIN="__GIT_UNAVAILABLE__"
164+
elif command -v timeout &>/dev/null; then
157165
GIT_PORCELAIN=$(timeout 5 git status --porcelain -uno 2>/dev/null || echo "__GIT_UNAVAILABLE__")
158166
elif command -v gtimeout &>/dev/null; then
159167
GIT_PORCELAIN=$(gtimeout 5 git status --porcelain -uno 2>/dev/null || echo "__GIT_UNAVAILABLE__")

‎hooks/user-prompt-review-guard.sh‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,15 @@ CODE_PASSED=$(jq -r '.code_review.passed // false' "$STATE_FILE" 2>/dev/null ||
4848
DOC_PASSED=$(jq -r '.doc_review.passed // false' "$STATE_FILE" 2>/dev/null || echo "false")
4949
PRE_PASSED=$(jq -r '.precommit.passed // false' "$STATE_FILE" 2>/dev/null || echo "false")
5050

51+
# === Sidecar fail-closed marker ===
52+
if [[ -f "${STATE_FILE}.blocked" ]]; then
53+
CODE_PASSED="false"
54+
DOC_PASSED="false"
55+
PRE_PASSED="false"
56+
# Fail-closed: if no change flags set, sidecar means state write failed — assume changes exist
57+
[[ "$HAS_CODE" != "true" && "$HAS_DOC" != "true" ]] && { HAS_CODE="true"; HAS_DOC="true"; }
58+
fi
59+
5160
# Early exit: no changes tracked = nothing to remind
5261
if [[ "$HAS_CODE" != "true" && "$HAS_DOC" != "true" ]]; then
5362
exit 0
@@ -56,7 +65,10 @@ fi
5665
# --- Stale-state reconciliation (one-way: true→false only) ---
5766
# Only run git status when state has pending changes (performance optimization)
5867
# Include untracked files (no -uno) to avoid false downgrade when only new files exist
59-
if command -v timeout &>/dev/null; then
68+
# Skip when sidecar present — would undo fail-closed HAS_* forcing
69+
if [[ -f "${STATE_FILE}.blocked" ]]; then
70+
GIT_PORCELAIN="__GIT_UNAVAILABLE__"
71+
elif command -v timeout &>/dev/null; then
6072
GIT_PORCELAIN=$(timeout 3 git status --porcelain 2>/dev/null || echo "__GIT_UNAVAILABLE__")
6173
elif command -v gtimeout &>/dev/null; then
6274
GIT_PORCELAIN=$(gtimeout 3 git status --porcelain 2>/dev/null || echo "__GIT_UNAVAILABLE__")

‎test/hooks/post-compact-auto-loop.test.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,25 @@ test('R10 strategic reset injected at threshold', () => {
510510
);
511511
});
512512

513+
test('sidecar .blocked marker forces doc review injection', () => {
514+
const cwd = makeTempDir('sd0x-pc-sidecar-doc-');
515+
const binDir = setupStubBin();
516+
writeStateFile(cwd, {
517+
has_code_change: false,
518+
has_doc_change: true,
519+
code_review: { passed: false },
520+
doc_review: { passed: true },
521+
precommit: { passed: false },
522+
});
523+
writeFileSync(join(cwd, '.claude_review_state.json.blocked'), 'lock_failure');
524+
const result = runHook({ cwd, binDir });
525+
assert.equal(result.status, 0);
526+
assert.ok(
527+
result.stdout.includes('/codex-review-doc'),
528+
'sidecar should force doc review injection despite doc_review.passed=true'
529+
);
530+
});
531+
513532
test('R10 strategic reset fires only once', () => {
514533
const cwd = makeTempDir('sd0x-pc-r10-once-');
515534
const binDir = setupStubBin();

‎test/hooks/post-edit-format.test.js‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,21 @@ if (query && query.includes('.passed = false') && vars.key) {
8686
process.exit(0);
8787
}
8888
89+
// Handle atomic doc write: .has_doc_change = true | .updated_at = $now | .doc_review.passed = false [| aggregate_gate reset]
90+
if (query && query.includes('has_doc_change = true') && query.includes('doc_review.passed = false')) {
91+
data.has_doc_change = true;
92+
data.updated_at = vars.now || '';
93+
if (!data.doc_review) data.doc_review = {};
94+
data.doc_review.passed = false;
95+
if (query.includes('aggregate_gate.executed = false') && data.aggregate_gate) {
96+
data.aggregate_gate.executed = false;
97+
data.aggregate_gate.gate = null;
98+
data.aggregate_gate.reason = null;
99+
}
100+
process.stdout.write(JSON.stringify(data));
101+
process.exit(0);
102+
}
103+
89104
// Handle has("aggregate_gate") check
90105
if (query && query.includes('has("aggregate_gate")')) {
91106
process.stdout.write(Object.prototype.hasOwnProperty.call(data, 'aggregate_gate') ? 'true' : 'false');
@@ -655,6 +670,35 @@ test('code edit clears sidecar .blocked marker', () => {
655670
);
656671
});
657672

673+
test('doc edit atomic state update — flag + review + aggregate in single write', () => {
674+
const workDir = makeTempDir('sd0x-format-doc-atomic-');
675+
const binDir = setupStubBin();
676+
writeFileSync(
677+
join(workDir, '.claude_review_state.json'),
678+
JSON.stringify({
679+
has_doc_change: false,
680+
has_code_change: false,
681+
code_review: { executed: false, passed: false, last_run: '' },
682+
doc_review: { executed: true, passed: true, last_run: 'T1' },
683+
precommit: { executed: false, passed: false, last_run: '' },
684+
aggregate_gate: { executed: true, gate: 'READY', reason: null },
685+
})
686+
);
687+
runHook({
688+
cwd: workDir,
689+
binDir,
690+
filePath: '/project/docs/readme.md',
691+
env: { HOOK_NO_FORMAT: '1' },
692+
});
693+
const state = readState(workDir);
694+
assert.ok(state, 'state file should exist');
695+
assert.equal(state.has_doc_change, true, 'should set has_doc_change');
696+
assert.equal(state.doc_review.passed, false, 'should invalidate doc_review');
697+
assert.equal(state.aggregate_gate.executed, false, 'should reset aggregate_gate.executed');
698+
assert.equal(state.aggregate_gate.gate, null, 'should null aggregate_gate.gate');
699+
assert.ok(state.updated_at, 'should set updated_at');
700+
});
701+
658702
test('doc edit does NOT invalidate code_review', () => {
659703
const workDir = makeTempDir('sd0x-format-doc-no-code-invalidate-');
660704
const binDir = setupStubBin();

‎test/hooks/stop-guard.test.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1159,6 +1159,34 @@ test('dual mode: sidecar .blocked marker forces block', () => {
11591159
assert.equal(payload.ok, false);
11601160
});
11611161

1162+
test('sidecar .blocked marker forces doc review block (doc-only change)', () => {
1163+
const workDir = makeTempDir('sd0x-stop-guard-sidecar-doc-');
1164+
const binDir = setupStubBin();
1165+
const transcriptPath = join(workDir, 'transcript.json');
1166+
writeFileSync(transcriptPath, '[]');
1167+
writeFileSync(
1168+
join(workDir, '.claude_review_state.json'),
1169+
JSON.stringify({
1170+
has_code_change: false,
1171+
has_doc_change: true,
1172+
doc_review: { passed: true },
1173+
code_review: { passed: false },
1174+
precommit: { passed: false },
1175+
})
1176+
);
1177+
// Sidecar should override doc_review.passed to false
1178+
writeFileSync(join(workDir, '.claude_review_state.json.blocked'), 'lock_failure');
1179+
const result = runHook({
1180+
cwd: workDir,
1181+
binDir,
1182+
input: { transcript_path: transcriptPath },
1183+
env: { STOP_GUARD_MODE: 'strict' },
1184+
});
1185+
assert.equal(result.status, 2, 'sidecar should block doc-only change');
1186+
const payload = parseJson(result.stdout);
1187+
assert.equal(payload.ok, false);
1188+
});
1189+
11621190
test('backward compat: no review_mode field behaves as single mode', () => {
11631191
const workDir = makeTempDir('sd0x-stop-guard-compat-');
11641192
const binDir = setupStubBin();

‎test/hooks/user-prompt-review-guard.test.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,24 @@ test('stale state: git clean but state says code changed → reconcile to false
165165
assert.equal(result.stdout.trim(), '', 'should reconcile stale state and stay silent');
166166
});
167167

168+
test('sidecar .blocked marker forces doc review reminder', () => {
169+
const { dir, cooldownFile } = createWorkDir({
170+
has_doc_change: true,
171+
has_code_change: false,
172+
doc_review: { passed: true },
173+
code_review: { passed: false },
174+
precommit: { passed: false },
175+
});
176+
writeFileSync(join(dir, 'README.md'), '# modified');
177+
writeFileSync(join(dir, '.claude_review_state.json.blocked'), 'lock_failure');
178+
const result = runHook(dir, { REVIEW_GUARD_COOLDOWN: '0', REVIEW_GUARD_COOLDOWN_FILE: cooldownFile });
179+
assert.equal(result.status, 0);
180+
assert.ok(
181+
result.stdout.includes('/codex-review-doc'),
182+
'sidecar should force doc review reminder despite doc_review.passed=true'
183+
);
184+
});
185+
168186
test('hook always exits 0 (non-blocking)', () => {
169187
const { dir, cooldownFile } = createWorkDir({
170188
has_code_change: true,

0 commit comments

Comments
 (0)