[Audit] argus_mcp benchmark: budget_sec never decides completion, so a task finished after its budget is scored as complete #508

Context

#497 (Add fixed navigation task benchmark, 6140e7c) added tools/argus_mcp/src/benchmark.rs and benchmarks/dm4_navigation.json. Each task in the manifest declares a budget_sec (validated as 2..=60), and the README pitches the benchmark as an "absolute yardstick". But budget_sec is only used to size the harness's wall-clock wait (budget_sec + 3.0) and the engine's planned run time. Neither the QuakeC side nor the scoring compares the finish time against it.

Discovery Path

I read the #497 diff, then grepped budget_sec in benchmark.rs. It appears only at validation (l.109), wall_limit = budget_sec + 3.0 (l.238), the fallback game_sec for a missing outcome (l.254), and the engine duration sum (l.281). On the QuakeC side, Argus_TaskControl (src/argus.qc) always sets bot.ar_taskdeadline = time + 60, and the ARGTASK … complete branch prints and clears whatever the elapsed time is. summarize counts completed straight from that print.

Failure Mechanism

1. Task pit_green_to_mega has budget_sec: 20.
2. The harness assigns the goal (3842 + impulse 217), pumps for 400 ms, and only then starts started = Instant::now(). Wall clock and game clock are already 0.4 s+ apart at that point.
3. The bot reaches the marker at game time 22.4 s. QuakeC's deadline is 60 s, so it prints ARGTASK 0 complete 22.4.
4. The harness is still inside wall_limit = 23 s, parses the print, and records completed: true, game_sec: 22.4.
5. summarize counts the task as complete, so completion_rate goes up even though the bot missed its budget by 12%.

The grace isn't a fixed amount either. It's 3 s of wall clock plus the setup pumps, measured against game time, so it moves with engine pacing (host load, host_framerate, slow frames). A noise-sensitive comparison is exactly where that matters, because two builds with the same route times can get different completion rates.

Reproducible Counterexample

// Same scoring path as run_connected/summarize, with one over-budget finish:
let suite = /* dm4_navigation.json, task budget_sec = 20.0 */;
let results = vec![TaskResult {
    name: "pit_green_to_mega".into(),
    split: "train".into(),
    completed: true,          // parsed from "ARGTASK 0 complete 22.4"
    game_sec: Some(22.4),     // > budget_sec, still counted
    wall_sec: 22.9,
}];
let r = report(&suite, results);
assert_eq!(r.train.completion_rate, 1.0); // passes today; expected 0.0

Upstream Reference & Modern Standard

There are no deprecation claims here. The contract comes from the repo itself: the budget_sec field and its validation in benchmark.rs, the "absolute yardstick" wording in README.md and CHANGELOG.md for #384, and the hard-coded time + 60 deadline in Argus_TaskControl (src/argus.qc).

Proposed Resolution

Score against the manifest instead of the harness timeout. In run_connected, treat an outcome as completed only if completed && game_sec <= task.budget_sec, and keep the raw game_sec so over-budget finishes stay visible (for example as over_budget: true). It would also be cleaner to pass the budget into QuakeC (ar_taskdeadline = time + budget) so the engine prints timeout on game time, with the wall-clock limit kept only as a hang guard. Add the unit case above to the existing benchmark tests.