Skip to content

Commit 2099c17

Browse files
committed
fix(review): address 4 issues from independent code review
Issue 1 (post-edit-guard suppressor bypass): vg_filter_suppressed now tracks backtick template-literal depth and triple-double-quote string depth across lines. A disable comment inside a multiline string/template literal is ignored, closing the suppressor-bypass vector. Issue 2 (learn-evaluator tail-1000 truncation): removed the tail -1000 cap on the vg-helper path. The 30-minute cutoff is already enforced inside vg-helper session-metrics; the cap caused under-counting on busy sessions that produce >1000 log events per 30-minute window. Issue 3 (session_metrics.rs missing fields): Rust implementation now writes top_edited_files, avg_duration_ms, and slow_ops to match the Python reference (hooks/_lib/session_metrics.py:196-198). Downstream consumers (gc-scheduled.sh:369) no longer silently drop hot-file data. Issue 4 (CI over-broad token): removed workflow-level contents:write / pull-requests:write. Shell-heavy validate-and-test job now runs with contents:read only. Benchmark reporting is isolated to a new benchmark-report job that declares the write permissions it needs and receives bench-output.json via upload-artifact/download-artifact rather than inheriting a write token from the broad workflow scope. Constraint: GitHub Actions does not support per-step permission scopes, so job-level isolation via a separate job is the minimum granularity. Tested: cargo check clean; 88/88 hook regression tests pass.
1 parent bb362fe commit 2099c17

File tree

4 files changed

+75
-12
lines changed

4 files changed

+75
-12
lines changed

.github/workflows/ci.yml

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ on:
77
- main
88

99
permissions:
10-
contents: write
11-
pull-requests: write
10+
contents: read
1211

1312
defaults:
1413
run:
@@ -158,8 +157,38 @@ jobs:
158157
shell: bash
159158
run: bash tests/bench_hook_latency.sh --sla=500 --runs=3
160159

161-
- name: Store benchmark results
160+
- name: Upload benchmark results
162161
if: runner.os == 'Linux'
162+
uses: actions/upload-artifact@v4
163+
with:
164+
name: bench-output
165+
path: bench-output.json
166+
if-no-files-found: ignore
167+
168+
- name: VibeGuard Benchmark (fast)
169+
if: runner.os != 'Windows'
170+
shell: bash
171+
run: bash scripts/benchmark.sh --mode=fast
172+
173+
benchmark-report:
174+
name: Benchmark Report
175+
runs-on: ubuntu-latest
176+
needs: validate-and-test
177+
# Isolated job so write tokens are not exposed to the shell-heavy CI steps above.
178+
permissions:
179+
contents: write
180+
pull-requests: write
181+
steps:
182+
- name: Checkout
183+
uses: actions/checkout@v4
184+
185+
- name: Download benchmark results
186+
uses: actions/download-artifact@v4
187+
with:
188+
name: bench-output
189+
path: .
190+
191+
- name: Store benchmark results
163192
uses: benchmark-action/github-action-benchmark@v1
164193
with:
165194
name: Hook Latency (P95)
@@ -171,8 +200,3 @@ jobs:
171200
comment-on-alert: true
172201
fail-on-alert: false
173202
comment-always: ${{ github.event_name == 'pull_request' }}
174-
175-
- name: VibeGuard Benchmark (fast)
176-
if: runner.os != 'Windows'
177-
shell: bash
178-
run: bash scripts/benchmark.sh --mode=fast

hooks/learn-evaluator.sh

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@ fi
2828

2929
# Collect session metrics for the last 30 minutes of the current project + correct signal detection
3030
if [[ -n "$_VG_HELPER" ]]; then
31-
LEARN_SUGGESTION=$(tail -1000 "$VIBEGUARD_LOG_FILE" 2>/dev/null \
32-
| "$_VG_HELPER" session-metrics "$VIBEGUARD_SESSION_ID" "$VIBEGUARD_PROJECT_LOG_DIR" 2>/dev/null || true)
31+
# Pass the full log file — the 30-minute cutoff is enforced inside vg-helper,
32+
# so tail-limiting here would under-count events on busy sessions (>1000 events/30 min).
33+
LEARN_SUGGESTION=$("$_VG_HELPER" session-metrics "$VIBEGUARD_SESSION_ID" "$VIBEGUARD_PROJECT_LOG_DIR" \
34+
< "$VIBEGUARD_LOG_FILE" 2>/dev/null || true)
3335
else
3436
_SESSION_METRICS_SCRIPT="$(dirname "$0")/_lib/session_metrics.py"
3537
LEARN_SUGGESTION=$(VIBEGUARD_LOG_FILE="$VIBEGUARD_LOG_FILE" \

hooks/post-edit-guard.sh

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,23 @@ WARNINGS=""
4141
vg_filter_suppressed() {
4242
local rule="$1"
4343
awk -v rule="$rule" '
44-
BEGIN { suppress = 0 }
44+
BEGIN { suppress = 0; in_template = 0; in_triple_dq = 0 }
4545
{
46+
# Record multiline-string state at the START of this line so a
47+
# disable comment that is itself inside a string is not honoured.
48+
start_in_ml = (in_template || in_triple_dq)
49+
50+
# Track JS/TS template-literal depth via backtick parity.
51+
tmp = $0; n = gsub(/`/, "", tmp)
52+
if (n % 2 == 1) in_template = 1 - in_template
53+
54+
# Track triple-double-quote multi-line strings (Python, Rust raw).
55+
tmp = $0; n = gsub(/"""/, "", tmp)
56+
if (n % 2 == 1) in_triple_dq = 1 - in_triple_dq
57+
4658
if (suppress) { suppress = 0; next }
47-
if ($0 ~ "^[[:space:]]*(//|#)[[:space:]]*vibeguard-disable-next-line[[:space:]]+" rule "([[:space:]]|--|$)") {
59+
if (!start_in_ml &&
60+
$0 ~ "^[[:space:]]*(//|#)[[:space:]]*vibeguard-disable-next-line[[:space:]]+" rule "([[:space:]]|--|$)") {
4861
suppress = 1
4962
}
5063
print

vg-helper/src/session_metrics.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ pub fn run(args: &[String]) -> Result {
100100
let mut hooks: HashMap<String, u64> = HashMap::new();
101101
let mut tools: HashMap<String, u64> = HashMap::new();
102102
let mut edited_files: HashMap<String, u64> = HashMap::new();
103+
let mut durations_ms: Vec<u64> = Vec::new();
103104

104105
for e in &events {
105106
let d = e.get("decision").and_then(Value::as_str).unwrap_or("unknown");
@@ -116,8 +117,19 @@ pub fn run(args: &[String]) -> Result {
116117
}
117118
}
118119
}
120+
121+
if let Some(d_ms) = e.get("duration_ms").and_then(Value::as_u64) {
122+
durations_ms.push(d_ms);
123+
}
119124
}
120125

126+
let avg_duration_ms: u64 = if durations_ms.is_empty() {
127+
0
128+
} else {
129+
durations_ms.iter().sum::<u64>() / durations_ms.len() as u64
130+
};
131+
let slow_ops = durations_ms.iter().filter(|&&d| d > 5000).count();
132+
121133
let total = events.len() as f64;
122134
let negative = *decisions.get("warn").unwrap_or(&0)
123135
+ *decisions.get("block").unwrap_or(&0)
@@ -248,6 +260,15 @@ pub fn run(args: &[String]) -> Result {
248260
}
249261
}
250262

263+
// top_edited_files: top 5 by edit count (mirrors Python edited_files.most_common(5))
264+
let mut top_files: Vec<_> = edited_files.iter().collect();
265+
top_files.sort_by(|a, b| b.1.cmp(a.1));
266+
let top_edited_files: serde_json::Map<String, Value> = top_files
267+
.iter()
268+
.take(5)
269+
.map(|(k, v)| ((*k).clone(), json!(**v)))
270+
.collect();
271+
251272
// Write metrics
252273
let metrics = json!({
253274
"ts": chrono_now(),
@@ -256,6 +277,9 @@ pub fn run(args: &[String]) -> Result {
256277
"decisions": decisions,
257278
"hooks": hooks,
258279
"tools": tools,
280+
"top_edited_files": top_edited_files,
281+
"avg_duration_ms": avg_duration_ms,
282+
"slow_ops": slow_ops,
259283
"correction_signals": signals,
260284
"warn_ratio": (warn_ratio * 100.0).round() / 100.0,
261285
});

0 commit comments

Comments
 (0)