Quality review + bugfix: v0.3 surface (#76)

A consolidation pass over the v0.3 surface (VS Code extension + leaderboard) before docs and the release cut.

- Inline diagnostics now attach to the document holding the generated script (the applied target or a scratch doc), never the stale pre-apply buffer, and are not dropped on the no-editor path; a strict rejection opens the draft for review.
- Diagnostic columns map gmat-script's 1-indexed UTF-8 byte offsets to VS Code's 0-indexed UTF-16 units.
- Single-pass drafts are cancellable after the provider returns; a re-entry guard stops a cancelled-but-running draft from wedging the worker; `shutdown` stops the worker's read loop.
- The held-out content firewall (assert_no_leak) is wired into the gated leaderboard build, scanning the serialized board for held-out request/intent text; the CLI reports a detected leak as exit 1.
- recorded_usage no longer drops float token totals; the Space meta line no longer promises a held-out prompt count it never carries.

Coverage 94% to 96.5% (cli.py 79% to 92%); mypy --strict and ruff clean.

Closes #50

Author: djankov

editors/vscode/src/extension.ts

@@ -24,6 +24,10 @@ let output: vscode.OutputChannel;
 /** The progress reporter of an in-flight draft, so worker progress notifications can update it. */
 let activeProgress: vscode.Progress<{ message?: string }> | undefined;
 
+/** Guards against re-entrant drafts: one generation at a time keeps the single worker from wedging
+ * (a cancelled draft's provider call still occupies the worker) and the progress reporter unshared. */
+let draftInFlight = false;
+
 /** A read-only scheme backing the apply-to-file diff preview. */
 const DRAFT_SCHEME = "gmat-copilot-draft";
 const draftContents = new Map<string, string>();
@@ -90,6 +94,12 @@ async function draftCommand(source: "input" | "selection"): Promise<void> {
   if (!activeWorker) {
     return;
   }
+  if (draftInFlight) {
+    vscode.window.showInformationMessage(
+      "GMAT Copilot: a draft is already in progress — finish or dismiss it before starting another.",
+    );
+    return;
+  }
   const editor = vscode.window.activeTextEditor;
   const intent = await resolveIntent(source, editor);
   if (!intent) {
@@ -108,36 +118,36 @@ async function draftCommand(source: "input" | "selection"): Promise<void> {
     dryRun: config.get<boolean>("dryRun", false),
   };
 
-  let result: DraftResult;
+  draftInFlight = true;
   try {
-    result = await vscode.window.withProgress(
-      { location: vscode.ProgressLocation.Notification, cancellable: true, title: "GMAT Copilot" },
-      async (progress, token) => {
-        activeProgress = progress;
-        progress.report({ message: "Generating the mission script…" });
-        try {
-          return await activeWorker.draft(params, token);
-        } finally {
-          activeProgress = undefined;
-        }
-      },
-    );
-  } catch (err) {
-    if (!isCancellation(err)) {
-      showWorkerError(err);
+    let result: DraftResult;
+    try {
+      result = await vscode.window.withProgress(
+        {
+          location: vscode.ProgressLocation.Notification,
+          cancellable: true,
+          title: "GMAT Copilot",
+        },
+        async (progress, token) => {
+          activeProgress = progress;
+          progress.report({ message: "Generating the mission script…" });
+          try {
+            return await activeWorker.draft(params, token);
+          } finally {
+            activeProgress = undefined;
+          }
+        },
+      );
+    } catch (err) {
+      if (!isCancellation(err)) {
+        showWorkerError(err);
+      }
+      return;
     }
-    return;
-  }
-
-  applyDiagnostics(editor?.document, result.diagnostics);
-  if (result.rejected) {
-    vscode.window.showWarningMessage(
-      "GMAT Copilot: the draft did not validate clean and was not applied. See the Problems panel — " +
-        "switch to permissive mode or refine the prompt.",
-    );
-    return;
+    await presentResult(editor, result);
+  } finally {
+    draftInFlight = false;
   }
-  await reviewAndApply(editor, result);
 }
 
 async function resolveIntent(
@@ -162,20 +172,53 @@ async function resolveIntent(
 }
 
 // -------------------------------------------------------------------- apply-to-current-file UX
-async function reviewAndApply(
+// The worker's diagnostics index into the *generated script*, so they are attached to a document
+// that actually holds that script — the applied target or a scratch document — never the pre-apply
+// active buffer (whose content is still the user's old text, so the squiggles would land on the
+// wrong lines), and never dropped when the draft opens in a fresh document.
+async function presentResult(
   editor: vscode.TextEditor | undefined,
   result: DraftResult,
 ): Promise<void> {
-  const script = result.script;
+  if (result.rejected) {
+    // Strict rejected the draft, so the user's file is left untouched. Open the best-effort draft in
+    // a scratch document so its findings line up with the text they point at, in the Problems panel.
+    const doc = await openDraft(result.script);
+    applyDiagnostics(doc, result.diagnostics);
+    vscode.window.showWarningMessage(rejectionMessage(result));
+    return;
+  }
   if (!editor) {
-    const doc = await vscode.workspace.openTextDocument({ language: "gmat", content: script });
-    await vscode.window.showTextDocument(doc);
+    const doc = await openDraft(result.script);
+    applyDiagnostics(doc, result.diagnostics);
     vscode.window.showInformationMessage(
       "GMAT Copilot: no active editor to apply to — opened the draft in a new document for review.",
     );
     return;
   }
+  await reviewAndApply(editor, result);
+}
 
+/** Open the generated script in a fresh editor document for review (no active file to apply to). */
+async function openDraft(script: string): Promise<vscode.TextDocument> {
+  const doc = await vscode.workspace.openTextDocument({ language: "gmat", content: script });
+  await vscode.window.showTextDocument(doc);
+  return doc;
+}
+
+/** Why a strict draft was rejected — a lint failure or, with the dry-run on, a runtime failure. */
+function rejectionMessage(result: DraftResult): string {
+  const dryRun = result.dryRun;
+  const why =
+    dryRun && !dryRun.ok ? `did not pass the ${dryRun.tier}-tier dry-run` : "did not lint clean";
+  return (
+    `GMAT Copilot: the draft ${why} and was not applied. It is open for review with the findings ` +
+    "in the Problems panel — switch to permissive mode or refine the prompt."
+  );
+}
+
+async function reviewAndApply(editor: vscode.TextEditor, result: DraftResult): Promise<void> {
+  const script = result.script;
   const target = editor.document;
   const label = target.isUntitled ? "untitled" : path.basename(target.fileName);
   const draftUri = vscode.Uri.parse(`${DRAFT_SCHEME}:/${draftCounter++}/${label}`);
@@ -194,6 +237,7 @@ async function reviewAndApply(
       "Discard",
     );
     if (choice !== "Apply") {
+      // Discarded: leave the user's file and any existing diagnostics on it untouched.
       return;
     }
     const edit = new vscode.WorkspaceEdit();
@@ -203,6 +247,10 @@ async function reviewAndApply(
     );
     edit.replace(target.uri, fullRange, script);
     const applied = await vscode.workspace.applyEdit(edit);
+    if (applied) {
+      // The target now holds the generated script, so the diagnostics line up with its content.
+      applyDiagnostics(target, result.diagnostics);
+    }
     vscode.window.showInformationMessage(
       applied
         ? "GMAT Copilot: draft applied to the active file."

leaderboard/space/board.js

@@ -175,10 +175,10 @@ function renderMeta(board) {
       ["Judge model", board.judge_model],
       ["Public set", `${publicSet.n_prompts ?? "?"} prompts · committed · reproduces offline`],
       [
+        // The held-out size is deliberately not published — revealing it serves the overfitter, not
+        // the reader, and the board never carries the count anyway (anti-overfitting hygiene, D16).
         "Held-out set",
-        `${heldOutSet.n_prompts ?? "?"} prompts · never committed · ${
-          heldOutSet.store || "private store, scored in gated CI"
-        }`,
+        `never committed · ${heldOutSet.store || "private store, scored in gated CI"}`,
       ],
     ])
   );

src/gmat_copilot/cli.py

@@ -362,13 +362,17 @@ def _leaderboard_build(args: argparse.Namespace) -> int:
         print(f"gmat-copilot leaderboard build: cannot read {args.config}: {exc}", file=sys.stderr)
         return 2
     held_out_root = Path(args.held_out) if args.held_out else None
-    board, notes = build_from_config(
-        config,
-        root=Path(args.root),
-        generated_at=args.generated_at or _now_utc(),
-        tool_version=__version__,
-        held_out_root=held_out_root,
-    )
+    try:
+        board, notes = build_from_config(
+            config,
+            root=Path(args.root),
+            generated_at=args.generated_at or _now_utc(),
+            tool_version=__version__,
+            held_out_root=held_out_root,
+        )
+    except LeaderboardError as exc:
+        print(f"gmat-copilot leaderboard build: {exc}", file=sys.stderr)
+        return 1
     for note in notes:
         print(f"gmat-copilot leaderboard: {note}", file=sys.stderr)
     text = dumps(board)

src/gmat_copilot/eval/leaderboard.py

@@ -32,6 +32,7 @@
 from ..providers import ProviderError
 from .judge import JUDGE_MODEL
 from .lift import LiftReport, run_recorded_lift
+from .prompts import load_prompts
 from .runner import EvalReport, run_recorded
 
 __all__ = [
@@ -46,6 +47,7 @@
     "build_leaderboard",
     "bundle_sha16",
     "dumps",
+    "held_out_secrets",
     "score_entry",
     "summarize",
 ]
@@ -219,8 +221,10 @@ def recorded_usage(bundle: Path, *, model: str, n_votes: int) -> dict[str, int]:
             continue
         generations += 1
         for field, value in (entry.get("usage") or {}).items():
-            if isinstance(value, int):
-                totals[field] = totals.get(field, 0) + value
+            # Token counts recorded as floats (e.g. 1234.0) must be summed, not silently dropped;
+            # bool is an int subclass, so exclude it explicitly.
+            if isinstance(value, (int, float)) and not isinstance(value, bool):
+                totals[field] = totals.get(field, 0) + int(value)
     return {"generation_calls": generations, "judge_calls": generations * n_votes, **totals}
 
 
@@ -340,6 +344,27 @@ def assert_no_leak(serialized: str, secrets: Iterable[str]) -> None:
             raise LeaderboardError("a held-out gold leaked into the published board")
 
 
+def held_out_secrets(config: dict[str, Any], held_out_root: Path) -> list[str]:
+    """The held-out gold strings the published board must never contain — every held-out prompt's
+    request and intent text, read from the private store under *held_out_root*.
+
+    Fed to :func:`assert_no_leak` so the firewall scans the bytes that ship, not just their key
+    names (the structural :func:`assert_aggregate_only` check). Held-out bundles that have not been
+    fetched are skipped, so this is a no-op offline and the realistic content scan only in gated CI.
+    """
+    secrets: list[str] = []
+    for seed in config.get("seeds", []):
+        rel = seed.get("held_out_bundle")
+        if not rel:
+            continue
+        prompts_path = held_out_root / rel / "prompts.json"
+        if not prompts_path.exists():
+            continue
+        for prompt in load_prompts(prompts_path):
+            secrets.extend((prompt.request, prompt.intent))
+    return secrets
+
+
 def build_from_config(
     config: dict[str, Any],
     *,
@@ -396,4 +421,9 @@ def build_from_config(
         public_set=config.get("public_set", {}),
         held_out_set=config.get("held_out_set", {}),
     )
+    # Defence in depth over the structural firewall: when the private held-out store is present (the
+    # gated-CI build), scan the serialized board for any held-out request/intent text, so a gold
+    # smuggled inside an allowed aggregate field is caught, not just an unexpected key name.
+    if held_out_root is not None:
+        assert_no_leak(dumps(board), held_out_secrets(config, held_out_root))
     return board, notes

src/gmat_copilot/generate.py

@@ -219,11 +219,14 @@ def draft(
     :param gmat_root: GMAT install root forwarded to the dry-run (else ``GMAT_ROOT`` / discovery).
     :param dry_run_fn: a dynamic-tier dry-run to use in place of the real gmat-run subprocess (the
         eval's deterministic replay seam, decision D7); ``None`` uses the real dry-run.
-    :param cancel: an optional predicate polled at each repair-attempt boundary (decision D15); when
-        it returns true before an attempt begins, generation stops with :class:`DraftCancelled`. An
-        in-flight provider call or dry-run runs to completion, so a single pass (``repair=0``) has
-        no boundary to cancel at.
-    :raises DraftCancelled: when *cancel* returns true before an attempt begins.
+    :param cancel: an optional predicate polled before each attempt begins and again after the
+        provider returns, before validation (decision D15); when it returns true generation stops
+        with :class:`DraftCancelled`. The in-flight provider call (and a running dry-run) still
+        completes — cancelling does not abort an HTTP request mid-flight — but a cancel observed
+        after generation skips the potentially expensive dry-run, so even a single pass
+        (``repair=0``) is cancellable between its generate and validate phases.
+    :raises DraftCancelled: when *cancel* returns true before an attempt begins or after the
+        provider returns.
     :raises DraftRejected: in strict mode, when the final draft still has blocking diagnostics.
     :raises ProviderError: when no model is resolved — either *model* is ``None`` with no provider
         to apply it to, or :func:`~gmat_copilot.providers.select` cannot resolve the selector.
@@ -254,6 +257,10 @@ def draft(
         last = provider.complete(
             prompt, model=model, temperature=temperature, max_tokens=max_tokens
         )
+        if cancel is not None and cancel():
+            raise DraftCancelled(
+                f"generation cancelled after attempt {attempt + 1} of {repair + 1}"
+            )
         script = _extract_script(last.text)
         verdict = evaluate(script, dry_run=dry_run, gmat_root=gmat_root, dry_run_fn=dry_run_fn)
         attempts.append(

src/gmat_copilot/worker.py

@@ -97,32 +97,45 @@ def write_message(writer: BinaryIO, message: Mapping[str, Any]) -> None:
 
 
 # ------------------------------------------------------------------- engine result -> VS Code shape
-def _line_end_char(text: str, line_1indexed: int) -> int:
-    """The 0-indexed end-of-line character, so a start-only diagnostic gets a visible squiggle.
+def _utf16_len(text: str) -> int:
+    """The length of *text* in UTF-16 code units — VS Code's position unit."""
+    return len(text.encode("utf-16-le")) // 2
 
-    ``LintDiagnostic`` keeps only the start position; widening to the line end gives the Problems
-    panel a non-empty range to underline.
+
+def _byte_col_to_utf16(line_text: str, column_1indexed: int) -> int:
+    """Convert a gmat-script 1-indexed UTF-8 *byte* column into a 0-indexed UTF-16 code-unit offset.
+
+    gmat-script positions are 1-indexed byte offsets within their line (its compiler convention),
+    while VS Code ranges are 0-indexed UTF-16 code units. The two coincide only for ASCII — any
+    multi-byte character earlier on the line (a unicode comment, a degree sign) would otherwise
+    shift the squiggle. ``errors="ignore"`` keeps a malformed byte from crashing the surface; a byte
+    column that splits a code point falls back to the nearest valid prefix.
     """
-    lines = text.splitlines()
-    idx = line_1indexed - 1
-    return len(lines[idx]) if 0 <= idx < len(lines) else 0
+    byte_offset = max(column_1indexed - 1, 0)
+    prefix = line_text.encode("utf-8")[:byte_offset].decode("utf-8", errors="ignore")
+    return _utf16_len(prefix)
 
 
 def to_vscode_diagnostics(report: LintReport, source_text: str) -> list[dict[str, Any]]:
     """Map a :class:`LintReport` into the VS Code ``Diagnostic`` JSON the Problems panel consumes.
 
-    gmat-script positions are 1-indexed; VS Code ranges are 0-indexed. ``source`` and ``code`` let
-    the user filter gmat-copilot findings and click through to the rule.
+    gmat-script positions are 1-indexed byte columns; VS Code ranges are 0-indexed UTF-16 units, so
+    each column is converted (not merely decremented). ``LintDiagnostic`` keeps only the start
+    position, so the range is widened to the line end to give the Problems panel a visible squiggle.
+    ``source`` and ``code`` let the user filter gmat-copilot findings and click through to the rule.
     """
+    lines = source_text.splitlines()
     out: list[dict[str, Any]] = []
     for d in report.diagnostics:
         line0 = max(d.line - 1, 0)
-        char0 = max(d.column - 1, 0)
+        line_text = lines[line0] if 0 <= line0 < len(lines) else ""
+        end_char = _utf16_len(line_text)
+        start_char = min(_byte_col_to_utf16(line_text, d.column), end_char)
         out.append(
             {
                 "range": {
-                    "start": {"line": line0, "character": char0},
-                    "end": {"line": line0, "character": _line_end_char(source_text, d.line)},
+                    "start": {"line": line0, "character": start_char},
+                    "end": {"line": line0, "character": end_char},
                 },
                 "severity": _VSCODE_SEVERITY.get(d.severity, 3),
                 "source": "gmat-copilot",
@@ -282,7 +295,11 @@ def _on_message(self, message: Mapping[str, Any]) -> None:
         if msg_id is None:
             return  # an unrecognised notification — nothing to answer
         if method == "shutdown":
+            # Acknowledge, then stop the read loop so the child exits on its own. This is a private
+            # command protocol, not a real LSP server, so shutdown *is* the stop signal — the client
+            # need not also send `exit`, and its `proc.kill()` is only a backstop.
             self._respond(msg_id, {"ok": True})
+            self._running = False
             return
         event = threading.Event()
         with self._cancels_lock: