fix(review): address consensus review findings on #1064

Major:
- `countByCategory` used an object literal plus `in`, so every `Object.prototype`
  member passed the allowlist: a finding categorised `toString` both minted a
  dimension and evaluated `<native function> + 1` into a
  `Record<string, number>`. Now `Object.create(null)` + `Object.hasOwn`. Zod
  makes this unreachable today; the guard exists for when validation is bypassed.
- `review_post_outcome` had no stated cardinality and three paths that skipped
  it. The contract is now written down — exactly one per **completed** review, so
  absence means the review failed rather than that an event was lost — and
  enforced with a latch plus a `finally` rather than by control flow that only
  looked exhaustive. `not_requested` moved ahead of the `--output` write and the
  stdout render; a new `not_attempted` bucket covers a run that dies between the
  completed review and the post attempt; a throwing `resolveGitHubTarget()`
  reports `target_unresolved`.

Minor:
- `classifyReviewFailure` dropped the `message.includes("git diff")` fallback its
  own docstring disclaimed. It was unreachable for the real git path — `execFile`
  always sets `cmd`, and its message begins `Command failed: `, so the `cmd`
  check returns first.
- Added the adversarial prototype-key case to the guard test; the existing
  ordinary-string case cannot reach it.
- Added coverage that a throwing `Telemetry.track` cannot propagate out of either
  emitter, which is what the two empty `catch` blocks promise.

Nits:
- Deleted the e2e's 500 ms sleep. `proc.exited` already implies the flush landed:
  the CLI awaits `shutdown()` → `flush()` → the sink's HTTP response.
- `ALTIMATE_TELEMETRY_DISABLED` restored conditionally; unconditional assignment
  wrote the literal string `"undefined"` when the variable was originally absent.
- `postStartedAt` moved inside `if (args.post)`, and the `if`/`if` pair is now a
  single branch.
- Corrected the e2e comment claiming the object-valued `by_category` follows the
  house convention — the sibling map-shaped fields stringify at the call site.

Also covers three of the review's flagged test gaps: `stale_manifest` /
`degraded` field mapping, and CLI-level control flow through a real process — a
new e2e drives `--post` with an unwritable `--output` and asserts exactly one
post outcome. Both new unit tests and the new e2e were mutation-checked against
their fixes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb

Author: sahrizvi

docs/docs/reference/telemetry.md

@@ -48,7 +48,7 @@ We collect the following categories of events:
 | `validator_check` | A completion-gate validator ran on session end — validator name, `ok` boolean, step, retry count, `enforced` flag (false in shadow mode), and structured `details` (model counts, elapsed time, concurrency limit — no SQL or model content). Only emitted when `ALTIMATE_VALIDATORS_ENABLED=1` or `ALTIMATE_VALIDATORS_SHADOW=1`. See [Validators](../data-engineering/validators.md). |
 | `validator_retries_exhausted` | A session terminated with unresolved validator failures after exhausting the synthetic-retry budget — names of the failing validators (no failure body content). |
 | `review_run` | A dbt/SQL review completed or failed — `invocation` (`cli` for `altimate-code review`, `tool` for the `dbt_pr_review` tool), status, duration, and on success the verdict, the pre-gating verdict, mode, risk tier, and finding counts by severity and by category. No file paths, model or column names, finding titles or bodies, SQL, diff content, or repository/branch/PR names. |
-| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. No repository, PR, or comment content. |
+| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `not_attempted`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. Exactly one per **completed** review: a review that failed emits `review_run: failed` and no post event, so absence means the review failed rather than that an event was lost. `not_attempted` is publication requested but never reached (a bad `--output` path, a stdout write error). No repository, PR, or comment content. |
 
 ### Notes on the review events
 

packages/opencode/src/altimate/review/telemetry.ts

@@ -24,10 +24,14 @@ export type ReviewInvocation = "cli" | "tool"
  * malformed category would otherwise become a new dimension.
  */
 function countByCategory(findings: Finding[]): Record<string, number> {
-  const counts: Record<string, number> = {}
+  // Prototype-less, and membership tested with Object.hasOwn: `{}` plus `in` accepted every
+  // Object.prototype member, so a finding categorised `toString` both minted a dimension and
+  // evaluated `<native function> + 1` into a Record<string, number>. Zod makes that unreachable
+  // today, but this guard exists precisely for the case where validation was bypassed.
+  const counts: Record<string, number> = Object.create(null)
   for (const category of ReviewCategory.options) counts[category] = 0
   for (const finding of findings) {
-    if (finding.category in counts) counts[finding.category] += 1
+    if (Object.hasOwn(counts, finding.category)) counts[finding.category] += 1
   }
   return counts
 }
@@ -41,15 +45,19 @@ function countByCategory(findings: Finding[]): Record<string, number> {
  * rather than inventing buckets that can never occur.
  *
  * Matching is on the fixed prefix the config loader throws with, and on the spawn identity of the
- * git child process — not broad substring matching over the message, which would drift the moment
- * anything is reworded.
+ * git child process (`err.cmd`, set by `execFile`) — not broad substring matching over the
+ * message, which would drift the moment anything is reworded. A `message.includes("git diff")`
+ * fallback used to sit below the `cmd` check; it was unreachable for the real git path (execFile
+ * always sets `cmd`, and its message begins "Command failed: ") and contradicted this paragraph.
+ *
+ * The `Failed to load` prefix is itself string matching. It is accurate against the config loader
+ * today; a typed error at the throw site is what would make it robust.
  */
 export function classifyReviewFailure(err: unknown): "config_error" | "git_error" | "error" {
   const message = err instanceof Error ? err.message : String(err)
   if (message.startsWith("Failed to load")) return "config_error"
   const cmd = (err as { cmd?: unknown } | undefined)?.cmd
   if (typeof cmd === "string" && /(^|[\\/\s])git(\s|$)/.test(cmd)) return "git_error"
-  if (message.startsWith("git ") || message.includes("git diff")) return "git_error"
   return "error"
 }
 
@@ -113,9 +121,16 @@ export function emitReviewRun(input: {
   }
 }
 
-/** Emitted on the CLI path only — the tool does not publish. */
+/**
+ * Emitted on the CLI path only — the tool does not publish.
+ *
+ * CONTRACT: exactly one of these per *completed* review, never more and never fewer. A review that
+ * threw never reached a publication phase, so it gets `review_run: failed` and no post event —
+ * absence therefore means "the review failed", not "telemetry was lost". The caller enforces the
+ * once-ness with a latch plus a `finally`; see cli/cmd/review.ts.
+ */
 export function emitReviewPostOutcome(input: {
-  outcome: "not_requested" | "target_unresolved" | "full" | "partial" | "summary_failed"
+  outcome: "not_requested" | "not_attempted" | "target_unresolved" | "full" | "partial" | "summary_failed"
   durationMs: number
   sessionID: string
 }): void {

packages/opencode/src/altimate/telemetry/index.ts

@@ -777,7 +777,11 @@ export namespace Telemetry {
         /** `partial` covers every "not fully posted as attempted" state PostResult can express —
          *  inline comments fell back, a post error was recorded, or no review id came back. The
          *  shape cannot distinguish finer outcomes than that. */
-        outcome: "not_requested" | "target_unresolved" | "full" | "partial" | "summary_failed"
+        /** `not_attempted`: publication was requested, but the invocation died between the
+         *  completed review and the post attempt (a bad `--output` path, a stdout write error).
+         *  Emitted from the caller's `finally` so a completed review always carries exactly one
+         *  post outcome. */
+        outcome: "not_requested" | "not_attempted" | "target_unresolved" | "full" | "partial" | "summary_failed"
         duration_ms: number
       }
   // altimate_change end

packages/opencode/src/cli/cmd/review.ts

@@ -104,54 +104,83 @@ export const ReviewCommand = cmd({
       }
       emitReviewRun({ invocation: "cli", durationMs: Date.now() - startedAt, sessionID: "", envelope: env })
 
-      if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2))
-
-      // Primary output → stdout (pipeable). Diagnostics below → stderr via UI.
-      if (args.json) {
-        process.stdout.write(JSON.stringify(env, null, 2) + "\n")
-      } else {
-        process.stdout.write(renderSummary(env) + "\n")
+      // altimate_change start — publication is its own event: it happens after the review is
+      // computed and can partially succeed, so it must not fold into review_run.
+      //
+      // CONTRACT: exactly one review_post_outcome per COMPLETED review. A review that threw
+      // returned above with `review_run: failed` and never reached a publication phase, so the
+      // absence of a post event means "the review failed" and never "telemetry was lost".
+      //
+      // Enforced by a latch plus the finally below rather than by the control flow being
+      // obviously exhaustive — it was not. The `not_requested` emit used to sit AFTER the
+      // `--output` write and the stdout render, so a bad `--output` path produced a completed
+      // review with no post event at all, indistinguishable from a dropped event.
+      let postOutcomeEmitted = false
+      function emitPostOnce(outcome: Parameters<typeof emitReviewPostOutcome>[0]["outcome"], durationMs: number) {
+        if (postOutcomeEmitted) return
+        postOutcomeEmitted = true
+        emitReviewPostOutcome({ outcome, durationMs, sessionID: "" })
       }
 
-      // altimate_change — publication is its own event: it happens after the review is computed
-      // and can partially succeed, so it must not fold into review_run.
-      const postStartedAt = Date.now()
-      const postDuration = () => Date.now() - postStartedAt
-      if (!args.post) {
-        emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" })
-      }
-      if (args.post) {
-        const target = await resolveGitHubTarget()
-        if (!target) {
-          emitReviewPostOutcome({ outcome: "target_unresolved", durationMs: postDuration(), sessionID: "" })
-          UI.println(
-            "⚠️  --post requested but GITHUB_TOKEN / GITHUB_REPOSITORY / PR number could not be resolved; skipping post.",
-          )
+      try {
+        // Emitted before anything that can throw, so the no-publication case cannot be lost.
+        if (!args.post) emitPostOnce("not_requested", 0)
+
+        if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2))
+
+        // Primary output → stdout (pipeable). Diagnostics below → stderr via UI.
+        if (args.json) {
+          process.stdout.write(JSON.stringify(env, null, 2) + "\n")
         } else {
-          let r
+          process.stdout.write(renderSummary(env) + "\n")
+        }
+
+        if (args.post) {
+          // Started here, not above: the `not_requested` path reports 0 and never reads these, and
+          // capturing them earlier made that hardcoded 0 look like an oversight.
+          const postStartedAt = Date.now()
+          const postDuration = () => Date.now() - postStartedAt
+          let target
           try {
-            r = await postGitHubReview(env, target)
+            target = await resolveGitHubTarget()
           } catch (err) {
-            // A throw here means the summary comment itself failed; nothing was published.
-            emitReviewPostOutcome({ outcome: "summary_failed", durationMs: postDuration(), sessionID: "" })
+            // Defensive today — the resolver returns undefined rather than throwing — but the
+            // contract should not rest on that staying true. No summary was attempted.
+            emitPostOnce("target_unresolved", postDuration())
             throw err
           }
-          emitReviewPostOutcome({
-            outcome: classifyPostOutcome(r),
-            durationMs: postDuration(),
-            sessionID: "",
-          })
-          const where = `${target.owner}/${target.repo}#${target.prNumber}`
-          if (r.postError) {
-            UI.println(`⚠️  Posted the summary comment to ${where}, but the review event failed: ${r.postError}`)
-          } else {
+          if (!target) {
+            emitPostOnce("target_unresolved", postDuration())
             UI.println(
-              `Posted review to ${where}` +
-                (r.inlineFellBack ? " (inline comments fell back to summary-only)" : ""),
+              "⚠️  --post requested but GITHUB_TOKEN / GITHUB_REPOSITORY / PR number could not be resolved; skipping post.",
             )
+          } else {
+            let r
+            try {
+              r = await postGitHubReview(env, target)
+            } catch (err) {
+              // A throw here means the summary comment itself failed; nothing was published.
+              emitPostOnce("summary_failed", postDuration())
+              throw err
+            }
+            emitPostOnce(classifyPostOutcome(r), postDuration())
+            const where = `${target.owner}/${target.repo}#${target.prNumber}`
+            if (r.postError) {
+              UI.println(`⚠️  Posted the summary comment to ${where}, but the review event failed: ${r.postError}`)
+            } else {
+              UI.println(
+                `Posted review to ${where}` +
+                  (r.inlineFellBack ? " (inline comments fell back to summary-only)" : ""),
+              )
+            }
           }
         }
+      } finally {
+        // Anything that threw between the completed review and the post attempt — a bad
+        // `--output` path, a stdout write error. Latched, so a real outcome always wins.
+        emitPostOnce("not_attempted", 0)
       }
+      // altimate_change end
 
       // Gate: exit non-zero when blocking, so CI fails the check.
       if (env.mode === "gate" && env.verdict === "REQUEST_CHANGES") {

packages/opencode/test/altimate/review/telemetry.test.ts

@@ -95,6 +95,54 @@ describe("review_run", () => {
     expect(Object.keys(byCategory)).toHaveLength(ReviewCategory.options.length)
   })
 
+  test("a category naming an Object.prototype member cannot slip past the guard", () => {
+    // The ordinary-string case above cannot catch this: `{}` plus `in` returns true for every
+    // prototype member, so `toString` both minted a dimension AND made `counts[k] += 1` evaluate
+    // `<native function> + 1` — a string inside a Record<string, number>. Fails before the
+    // Object.create(null) / Object.hasOwn fix.
+    const events = captureEvents()
+    emitReviewRun({
+      invocation: "cli",
+      durationMs: 1,
+      sessionID: "",
+      envelope: envelope({
+        findings: [
+          { category: "toString", severity: "warning" },
+          { category: "constructor", severity: "warning" },
+          { category: "valueOf", severity: "warning" },
+          { category: "__proto__", severity: "warning" },
+        ],
+      }),
+    })
+
+    const byCategory = (events[0] as any).by_category
+    expect(Object.keys(byCategory)).toHaveLength(ReviewCategory.options.length)
+    for (const v of Object.values(byCategory)) expect(typeof v).toBe("number")
+  })
+
+  test("stale_manifest and degraded are carried from the envelope", () => {
+    // Same `=== true` normalisation as tier_forced, which has its own test; these two had none,
+    // and the shared envelope() helper omits staleManifest so every other test covers only the
+    // undefined case.
+    const events = captureEvents()
+    emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() })
+    expect((events[0] as any).stale_manifest).toBe(false)
+    expect((events[0] as any).degraded).toBe(false)
+
+    events.length = 0
+    emitReviewRun({
+      invocation: "cli",
+      durationMs: 1,
+      sessionID: "",
+      envelope: envelope({
+        staleManifest: true,
+        summary: { critical: 0, warning: 0, suggestion: 0, degraded: true },
+      }),
+    })
+    expect((events[0] as any).stale_manifest).toBe(true)
+    expect((events[0] as any).degraded).toBe(true)
+  })
+
   test("the tool path carries its session, the CLI path does not", () => {
     const events = captureEvents()
     emitReviewRun({ invocation: "tool", durationMs: 1, sessionID: "ses_abc", envelope: envelope() })
@@ -142,6 +190,22 @@ describe("review_run", () => {
   })
 })
 
+describe("telemetry failure isolation", () => {
+  // The two empty catch blocks in the emitters are the "observability must never break
+  // functionality" guarantee. Removing either one fails these and nothing else.
+  test("a throwing Telemetry.track cannot propagate out of either emitter", () => {
+    spyOn(Telemetry, "track").mockImplementation(() => {
+      throw new Error("buffer full")
+    })
+
+    expect(() =>
+      emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }),
+    ).not.toThrow()
+    expect(() => emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", error: new Error("x") })).not.toThrow()
+    expect(() => emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" })).not.toThrow()
+  })
+})
+
 describe("failure classification", () => {
   test("the config loader's fixed prefix", () => {
     expect(classifyReviewFailure(new Error("Failed to load .altimate/review.yml: bad yaml"))).toBe("config_error")
@@ -152,6 +216,13 @@ describe("failure classification", () => {
     expect(classifyReviewFailure(err)).toBe("git_error")
   })
 
+  test("a git-shaped message without a cmd is not a git error", () => {
+    // The message fallback that used to classify this was unreachable for the real git path
+    // (execFile always sets `cmd`, and its message starts "Command failed: ") and contradicted
+    // the docstring's promise not to substring-match. Removed.
+    expect(classifyReviewFailure(new Error("git diff exploded"))).toBe("error")
+  })
+
   test("anything else is `error` rather than an invented bucket", () => {
     // The engine degrades rather than throwing for missing manifests, dispatcher failures and the
     // AI lane, so there are no buckets for those — they never arrive here.
@@ -209,7 +280,11 @@ describe("caller attribution", () => {
       expect(run.data.baseData.properties.source).toBe("plugin:claude-code")
       expect(post.data.baseData.properties.source).toBe("plugin:claude-code")
     } finally {
-      process.env.ALTIMATE_TELEMETRY_DISABLED = origDisabled
+      // Unlike the two restores below, this was unconditional: an originally-absent variable
+      // came back as the string "undefined", leaking a disabled-telemetry flag into later tests
+      // and any child process they spawn.
+      if (origDisabled !== undefined) process.env.ALTIMATE_TELEMETRY_DISABLED = origDisabled
+      else delete process.env.ALTIMATE_TELEMETRY_DISABLED
       if (origCs !== undefined) process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = origCs
       else delete process.env.APPLICATIONINSIGHTS_CONNECTION_STRING
       if (origClient !== undefined) process.env.ALTIMATE_CLI_CLIENT = origClient