mcp-gate monitor: accounting and marker follow-ups — must land before the ladder is wired into the request path
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 4.5k
- Forks
- 603
- Avg merge
- 23h 32m
- Merged PRs (30d)
- 59
Description
Follow-ups from the WP33 audits (#919, merged as 3d171d69c2). None is a bypass — the mandatory-inspection guarantee held at 456,400 checks, zero bypasses across the audit rounds.
Blocking condition: these must land before monitor is wired into McpGateTools. Today the module has no call site in the request path, so nothing is protected either way and none of this can hurt anyone. Item 2 goes from theoretical to material the moment the gate is deciding on real traffic, because a never-escalating ladder that looks correctly configured is the failure mode this program keeps rediscovering. If the wiring work is queued, it should depend on this issue rather than race it.
1. Declared latency is double-counted (MEDIUM)
monitor/mod.rs:378-379 charges each rung's declared latency inside run; mod.rs:294-297 separately charges the wall-clock of the whole run in inspect. Both land in monitoring_us.
A fixture returns instantly so the two don't overlap and the number reads clean. A real investigator that actually takes its declared latency is billed twice: measured 737,589 µs vs 300,041 µs for one 300 ms rung with max_rounds: 1 and identical rung counts — 2.46×.
Direction is conservative (it overstates, so it cannot conceal a budget breach) but every published overhead figure is therefore a declared-cost model rather than a measurement.
Fix: stop the clock around the investigate call, or subtract rung.spec.latency_us from the elapsed figure, or document fraction() as a model. (The doc half is already done in #923.)
2. The calibration divergence is bounded, not removed (MEDIUM)
Sourcing uncertainty from TinyDetector::expected_uncertainty() removed the config field, not the divergence: nothing cross-checks the declared figure against what score() actually carries.
Demonstrated: a single 0.50 rung at τ=0.1 with value_of_success: 5.0. An honest detector reporting σ=0.05 is correctly refused as Miscalibrated. The same ladder with a detector declaring the maximum legal 1.0 while scoring 0.05 builds successfully and then purchases 0 rungs on a non-mandatory operation sitting exactly at the escalation threshold. Residual inflation at the legal maximum: 2× at σ=0.5, 6× at σ=0.2, 44× at σ=0.05, 1000× at σ=0.01.
Related, same root: runtime uncertainty is unbounded. RiskSignal::new(0.5, 1e9) is Ok. A detector declaring 0.2 at construction and returning σ=1e9 from score() drove 20 zero-risk benign operations to 80 rungs and 2050% overhead. Construction bounds the declared figure to (0,1]; nothing bounds the produced one.
Fix: compare the first real RiskSignal's uncertainty against expected_uncertainty() and halt on gross mismatch, or compute the ceiling lazily from the first real signal. Bound the produced uncertainty the same way the declared one is.
3. dd markers catch 5 of 13 realistic invocations (LOW)
dd if= / dd of= only fire when the operand sits immediately after the command. Missed: dd bs=1M if=… of=… (the common form — most real dd calls specify bs=), dd count=1 …, dd status=progress …, dd conv=notrunc of=…, plus tab/newline/double-space separators and bare dd.
Bare dd is not the fix — it fires on 12 of 12 ordinary phrases tried, including "add two integers", which is literally the benign workload in the overhead test. Even "dd " with a trailing space still hits 4 of 12.
Fix: match the operand, not the command. Adding if=/dev/ and of=/dev/ catches 13 of 13 with 0 false positives, including forms the current markers miss. Keep the existing two and add these — the dangerous thing about dd is where it writes, not how it is spelled.
4. impersonation does not match impersonate (LOW)
"impersonation of the service account" classifies as non-mandatory — impersonation ends -ati, not -ate. The other new verbs' inflected forms work (unlinking ⊃ unlink, shredding ⊃ shred). Changing the marker to impersonat catches both at no cost.
5. A test asserts a property the module disclaims (LOW)
measured_overhead_on_a_synthetic_workload_is_under_five_percent asserts fraction < 0.05, and passes only because its fixture returns 0.2. Swept: 0.35 → 5.1% fails, 0.45 → 10.1% fails, 0.5 → 107.6% fails.
The module docs now explicitly say <5% is not guaranteed and depends on whether investigators resolve ambiguity. Rename it to what it measures, or drop the assertion the way measured_overhead_with_honest_latency_pricing sensibly did.
Related: #921 (extract voi into a leaf crate so the gate stops linking bundled SQLite) — also worth landing before the wiring.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in monitor/mod.rs, especially the run and inspect accounting paths, then trace how monitor will be wired into McpGateTools. Review the marker checks and the measured_overhead_on_a_synthetic_workload_is_under_five_percent test. Done means latency is not double-counted, runtime uncertainty is bounded and calibrated, all listed marker forms classify correctly, and the test matches the module's documented guarantee.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- security, testing
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100