From 2a2768211e098b0dc2463e5895bccbb1d547c866 Mon Sep 17 00:00:00 2001 From: Haider Date: Sun, 23 Aug 2026 22:36:09 +0530 Subject: [PATCH 1/6] release: v0.9.6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness release for `check --checks *` + native bridge resilience. Changelog: - altimate-core 0.7.0 upgrade + consumer contract sync surfaces findings many `check --checks {validate,semantic,grade,pii,migration,safety, policy,compare}` paths previously missed. Teams gating CI on `--fail-on error|warning` may see new failures on unchanged SQL — these are real findings, not regressions in your code. (#1090) - Truncated tool-output files were being deleted the moment they were written since 2026-08-14, due to a 48-bit Identifier timestamp wrap. Both cleanups now age files by mtime; stat failures fail safe (keep the file). (#1113, closes #1112) - Native bridge no longer poisons itself for the process lifetime on a transient NAPI load failure — registration now caches an in-flight promise, cleared on failure so subsequent calls can retry. Fix + adversarial tests from the v0.9.6 release review (Chaos Gremlin). Deferred to follow-up issues: #1124 (grace-window flag), #1125 (rule catalog docs), #1126 (legacy-shape-fallback removal), #1127 (NAPI-load- failure CI job), #1128 (truncate.ts/truncation.ts consolidation). Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 16 +++ docs/docs/usage/check.md | 2 + .../src/altimate/native/dispatcher.ts | 29 +++- .../skill/release-v0.9.6-adversarial.test.ts | 132 ++++++++++++++++++ 4 files changed, 175 insertions(+), 4 deletions(-) create mode 100644 packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fb6c7aeb3..b0425d118c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.9.6] - 2026-08-23 + +Correctness release: `check --checks {validate,semantic,grade,pii,migration,safety,policy,compare}` and the PII/lineage/migration tools now tell you the truth. Several previously returned false-clean or wrong output — teams gating CI on `check --fail-on error|warning` may see new failures on unchanged SQL. **These are real findings the tool previously missed, not regressions in your code.** Also fixes a data-hygiene bug where truncated tool-output files were being deleted the moment they were written. + +### Changed + +- **`check --checks *` now surfaces the findings it was missing — expect newly-failing CI runs.** `altimate-core` upgrade `0.5.1 → 0.7.0` plus a consumer-contract sync catches a class of latent shape-mismatch bugs where the CLI was reading fields the engine no longer emits and rendering false-clean output as a result. Concretely: (a) `check --checks validate` gated on the wrong field and passed every file, now maps `ValidationError.location` and fails closed on engine failure; (b) `check --checks semantic` treated `valid:true` as clean, but `valid` means "plannable" — cartesian products came back "OK" for months, now reads `findings` and never gates on `valid`; (c) `check --checks grade` read fields `evaluate()` doesn't return, so no grade or finding ever surfaced, now reads `overall_grade`/`scores.overall`/`lint.findings` (with nested findings from `validation.errors` + `safety.threats`); (d) `check --checks pii` mapped the column name to the numeric column-position field and printed `[object Object]` for `{ Custom: string }` classifications, now reports the exposing alias and stringifies properly; (e) `check --checks migration` rendered "SAFE" for every migration including `DROP COLUMN`, now reads `findings`/`safe`/`overall_risk`; (f) `check --checks compare` rendered "IDENTICAL" for different queries, now reads `differences`; (g) `check --checks policy` titled on `pass` (engine returns `allowed`), so clean SQL always rendered "VIOLATIONS FOUND", now inverted; (h) `check --checks safety` maps engine `high → error` and `medium → warning` (previously both degraded to `info`), so `--fail-on error|warning` no longer silently passes high-risk SQL injection threats; a new `unbalanced_quote` safety rule ships from the engine. Every finding's `rule` field is machine-readable in `--format json` output — that's where rule catalog lives. (#1090) +- **`altimate-core-track-lineage` no longer returns "0 edges" for everything.** The tool read `edges` off the top-level result; the engine has always emitted them at `queries[].edges`, so lineage tracking silently produced nothing. Now collects correctly, renders `impact_map`, formats `{table, column}` refs, and renders ERROR instead of "0 edges" when the engine call itself fails. (#1090) +- **`schema.detect_pii` returned zero findings for every scan.** Same shape-mismatch class: the detector read `piiData.findings`, but the engine's `PiiReport` is `{ columns, pii_count, … }`. Both cache and live paths now filter `classification !== "None"` through the shared `piiColumnsFromReport` helper. Malformed `PiiReport` shapes now throw rather than silently yield zero findings. Failed per-column classifications flip `success` to `false` — the schema-detect-pii tool renders "PII Scan: ERROR" with any partial findings attached, instead of a clean "no findings" verdict. (#1090) +- **`altimate-core-{compare,column-lineage,extract-metadata,import-ddl}` no longer crash on the default invocation.** Empty-string dialects now coerce to `undefined` before hitting `Schema.fromDdl`, centralized as `dialectHint()` in `native/engine-coerce.ts` and applied across all 7 dialect-forwarding handlers. (#1090) + +### Fixed + +- **Truncated tool-output files were being deleted the moment they were written (since 2026-08-14).** `Identifier.create` packs `timestamp * 4096 + counter` into 6 bytes and wraps every ~795 days; the 26th wrap landed 2026-08-14T11:19:55Z. Both truncation cleanups (`tool/truncate.ts` Effect service and `tool/truncation.ts` legacy module — used by bootstrap, bash, prompt) computed a pre-wrap cutoff astronomically larger than every post-wrap file's decoded timestamp, so every truncated tool output written after Aug 14 was garbage-collected on the next cleanup pass. Both cleanups now age files by `mtime` (which doesn't wrap); `stat` failures keep the file (deletion fails safe, so a dangling symlink or transient FS error can't wipe real cached truncations). The Effect-service cleanup now `stat`s through the injected `FSUtil.Service` for consistency with every other operation. Tagged `upstream_fix` — the wrap-prone encoding is upstream OpenCode code. (#1113, closes #1112) +- **Native bridge no longer poisons itself for the process lifetime on a transient NAPI load failure.** The lazy registration hook was nulled *before* the `await fn()` that loads all bridge handler modules; if the altimate-core NAPI binding failed to load mid-sequence (older glibc, unusual arch, permissions), every subsequent `Dispatcher.call` — not just the failing one — threw `No native handler for X` for the rest of the process, and the CLI had to be restarted. Registration is now cached as an in-flight promise: concurrent callers share one attempt, and on failure the cached promise is cleared so subsequent calls can retry. (v0.9.6 review) + ## [0.9.5] - 2026-08-10 Windows `grep` back for the ~16% of Windows users it silently broke since v0.9.2, plus a mid-session YOLO toggle and a welcome panel that stops eating half of narrow terminals. diff --git a/docs/docs/usage/check.md b/docs/docs/usage/check.md index 851bb2c01b..59949884ff 100644 --- a/docs/docs/usage/check.md +++ b/docs/docs/usage/check.md @@ -50,6 +50,8 @@ altimate-code check --fail-on warning | `semantic` | Semantic validation (cartesian products, wrong JOINs) | Optional | No | | `grade` | SQL quality grading with recommendations | Optional | No | +Every finding carries a machine-readable `rule` field (e.g. `unbalanced_quote`, `sql_injection`, `L003`) — visible in `--format json` output, and how the safety, lint, and policy rule inventory is discovered in practice. + By default, `lint` and `safety` are enabled. Override with `--checks`: ```bash diff --git a/packages/opencode/src/altimate/native/dispatcher.ts b/packages/opencode/src/altimate/native/dispatcher.ts index abeb40898b..af4f2091b5 100644 --- a/packages/opencode/src/altimate/native/dispatcher.ts +++ b/packages/opencode/src/altimate/native/dispatcher.ts @@ -20,15 +20,20 @@ export function register(method: BridgeMethod, handler: NativeHandler): void { /** Lazy registration hook — set by native/index.ts */ let _ensureRegistered: (() => Promise) | null = null +/** In-flight registration promise (deduped across concurrent callers). */ +let _registrationPromise: Promise | null = null + /** Clear all registered handlers and lazy registration hook (for test isolation). */ export function reset(): void { nativeHandlers.clear() _ensureRegistered = null + _registrationPromise = null } /** Called by native/index.ts to set the lazy registration function. */ export function setRegistrationHook(fn: () => Promise): void { _ensureRegistered = fn + _registrationPromise = null } /** Dispatch a method call to the registered native handler. */ @@ -36,11 +41,27 @@ export async function call( method: M, params: (typeof BridgeMethods)[M] extends { params: infer P } ? P : never, ): Promise<(typeof BridgeMethods)[M] extends { result: infer R } ? R : never> { - // Lazy registration: load all handler modules on first call + // Lazy registration: load all handler modules on first call. Cache the + // in-flight promise so concurrent callers share one attempt; on failure + // clear the cached promise so a subsequent call can retry. Previously + // ``_ensureRegistered`` was nulled BEFORE the await, so a transient NAPI + // load failure poisoned the bridge for the process lifetime — every + // subsequent ``call`` threw ``No native handler for X`` with no way to + // recover without restarting the CLI. if (_ensureRegistered) { - const fn = _ensureRegistered - _ensureRegistered = null - await fn() + if (!_registrationPromise) { + const fn = _ensureRegistered + _registrationPromise = fn().then( + () => { + _ensureRegistered = null + }, + (err) => { + _registrationPromise = null + throw err + }, + ) + } + await _registrationPromise } const native = nativeHandlers.get(method as string) diff --git a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts new file mode 100644 index 0000000000..05d7369bf0 --- /dev/null +++ b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts @@ -0,0 +1,132 @@ +/** + * Adversarial coverage for the v0.9.6 release payload. + * + * Focus: fixes that landed IN this release (not the whole PR history) — + * 1. Dispatcher retry-after-registration-failure (v0.9.6 review gremlin fix) + * 2. Truncation cleanup: fail-safe on stat error, mtime-based aging + * + * Not covered here (existing test suites are authoritative): + * - altimate-core 0.7.0 shape corrections — see + * packages/opencode/test/altimate/altimate-core-e2e.test.ts (26 files, + * real-engine "consumer contract sync" blocks land there) + * - Truncation ID-wrap regression — see + * packages/opencode/test/tool/truncation.test.ts + */ +import { afterEach, beforeAll, afterAll, beforeEach, describe, expect, test } from "bun:test" + +import * as Dispatcher from "../../src/altimate/native/dispatcher" + +beforeAll(() => { + process.env.ALTIMATE_TELEMETRY_DISABLED = "true" +}) +afterAll(() => { + delete process.env.ALTIMATE_TELEMETRY_DISABLED +}) + +describe("v0.9.6 release: Dispatcher registration retry", () => { + beforeEach(() => { + Dispatcher.reset() + Dispatcher.setRegistrationHook(null as any) + }) + afterEach(() => { + Dispatcher.reset() + Dispatcher.setRegistrationHook(null as any) + }) + + test("failed registration does NOT poison future calls — hook re-runs on next call", async () => { + // Reproduces the gremlin finding: previously _ensureRegistered was + // nulled BEFORE the await, so a transient failure permanently disabled + // lazy registration for the rest of the process — every subsequent + // call threw "No native handler for X" with no path to recovery. + let attempts = 0 + Dispatcher.setRegistrationHook(async () => { + attempts += 1 + if (attempts === 1) { + throw new Error("simulated NAPI load failure") + } + Dispatcher.register("ping", async () => ({ status: "recovered" })) + }) + + // First call: registration fails, error propagates. + await expect(Dispatcher.call("ping", {} as any)).rejects.toThrow( + "simulated NAPI load failure", + ) + expect(attempts).toBe(1) + + // Second call: registration hook MUST run again (this is the fix). + // Before v0.9.6 this call threw "No native handler for ping" because + // _ensureRegistered was permanently nulled after the first attempt. + const result = await Dispatcher.call("ping", {} as any) + expect(result).toEqual({ status: "recovered" }) + expect(attempts).toBe(2) + }) + + test("concurrent calls share ONE registration attempt on both success and failure", async () => { + // Guard the dedup property — the fix caches the in-flight promise so + // 100 racing calls trigger the hook exactly once, not 100 times. + let attempts = 0 + let resolveRegistration: () => void = () => {} + const registrationGate = new Promise((resolve) => { + resolveRegistration = resolve + }) + Dispatcher.setRegistrationHook(async () => { + attempts += 1 + await registrationGate + Dispatcher.register("ping", async () => ({ status: "ok" })) + }) + + // Fire 20 concurrent calls before registration completes. + const calls = Array.from({ length: 20 }, () => Dispatcher.call("ping", {} as any)) + // Let the concurrent callers all enter the registration branch. + await new Promise((r) => setTimeout(r, 10)) + resolveRegistration() + + const results = await Promise.all(calls) + expect(results).toHaveLength(20) + for (const r of results) expect(r).toEqual({ status: "ok" }) + // Critical: the hook fired exactly ONCE despite 20 concurrent callers. + expect(attempts).toBe(1) + }) + + test("successful registration clears the hook — a later call does NOT re-run it", async () => { + // Guard against overcorrection: the fix must still short-circuit after + // success, otherwise every subsequent call would eagerly re-import all + // handler modules for no reason. + let attempts = 0 + Dispatcher.setRegistrationHook(async () => { + attempts += 1 + Dispatcher.register("ping", async () => ({ status: "ok" })) + }) + + await Dispatcher.call("ping", {} as any) + expect(attempts).toBe(1) + + // Later call: hook must NOT re-run (it was cleared on success). + await Dispatcher.call("ping", {} as any) + expect(attempts).toBe(1) + }) + + test("reset() clears both the hook and the cached in-flight promise", async () => { + // The reset added a new field (_registrationPromise) that also needs + // to be cleared, otherwise test isolation regresses: a failed + // registration in one test would leave the cached rejected promise + // pinned for the next test. + let attempts = 0 + Dispatcher.setRegistrationHook(async () => { + attempts += 1 + throw new Error("boom") + }) + await expect(Dispatcher.call("ping", {} as any)).rejects.toThrow("boom") + expect(attempts).toBe(1) + + // Reset should wipe both hook and cached promise. Setting a new hook + // must let call() invoke IT, not a stale rejected promise. + Dispatcher.reset() + Dispatcher.setRegistrationHook(async () => { + Dispatcher.register("ping", async () => ({ status: "fresh" })) + }) + + const result = await Dispatcher.call("ping", {} as any) + expect(result).toEqual({ status: "fresh" }) + }) +}) From 3645a9d3eb1fce7ca37243bb77aeadc60e1c064b Mon Sep 17 00:00:00 2001 From: Haider Date: Sun, 23 Aug 2026 22:52:38 +0530 Subject: [PATCH 2/6] =?UTF-8?q?release:=20v0.9.6=20review=20=E2=80=94=20di?= =?UTF-8?q?spatcher=20race=20guard=20+=20qualify=20`rule`=20wording?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses coderabbit findings on the release PR: - **Major** — dispatcher.ts: without generation tracking, a stale registration attempt's `.then` handler could clobber replacement state installed mid-flight by a concurrent `reset()` or `setRegistrationHook()`. Success path nulled `_ensureRegistered` (wiping a replacement hook); failure path nulled `_registrationPromise` (breaking dedup for a newer in-flight promise). Fix: bump a generation counter on every mutation entry point; the settle handlers only mutate cached state if their captured generation is still current. 2 new adversarial tests cover both races. - **Minor** — CHANGELOG.md and docs/docs/usage/check.md: the "every finding carries a `rule` field" claim isn't quite true — `lint` findings may omit `rule` if the engine didn't attach one (matches the documented Finding Object where `rule` is optional). Qualified the wording in both files to "when set" / "when the underlying engine attaches one" so users know to check for presence before switching on it. Local verification: `bun test test/skill/release-v0.9.6-adversarial.test.ts test/altimate/dispatcher.test.ts` → 14/14 pass (2 new race tests). `bun turbo typecheck` clean. Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 2 +- docs/docs/usage/check.md | 2 +- .../src/altimate/native/dispatcher.ts | 20 ++++- .../skill/release-v0.9.6-adversarial.test.ts | 86 +++++++++++++++++++ 4 files changed, 105 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0425d118c..8c22df5ebc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ Correctness release: `check --checks {validate,semantic,grade,pii,migration,safe ### Changed -- **`check --checks *` now surfaces the findings it was missing — expect newly-failing CI runs.** `altimate-core` upgrade `0.5.1 → 0.7.0` plus a consumer-contract sync catches a class of latent shape-mismatch bugs where the CLI was reading fields the engine no longer emits and rendering false-clean output as a result. Concretely: (a) `check --checks validate` gated on the wrong field and passed every file, now maps `ValidationError.location` and fails closed on engine failure; (b) `check --checks semantic` treated `valid:true` as clean, but `valid` means "plannable" — cartesian products came back "OK" for months, now reads `findings` and never gates on `valid`; (c) `check --checks grade` read fields `evaluate()` doesn't return, so no grade or finding ever surfaced, now reads `overall_grade`/`scores.overall`/`lint.findings` (with nested findings from `validation.errors` + `safety.threats`); (d) `check --checks pii` mapped the column name to the numeric column-position field and printed `[object Object]` for `{ Custom: string }` classifications, now reports the exposing alias and stringifies properly; (e) `check --checks migration` rendered "SAFE" for every migration including `DROP COLUMN`, now reads `findings`/`safe`/`overall_risk`; (f) `check --checks compare` rendered "IDENTICAL" for different queries, now reads `differences`; (g) `check --checks policy` titled on `pass` (engine returns `allowed`), so clean SQL always rendered "VIOLATIONS FOUND", now inverted; (h) `check --checks safety` maps engine `high → error` and `medium → warning` (previously both degraded to `info`), so `--fail-on error|warning` no longer silently passes high-risk SQL injection threats; a new `unbalanced_quote` safety rule ships from the engine. Every finding's `rule` field is machine-readable in `--format json` output — that's where rule catalog lives. (#1090) +- **`check --checks *` now surfaces the findings it was missing — expect newly-failing CI runs.** `altimate-core` upgrade `0.5.1 → 0.7.0` plus a consumer-contract sync catches a class of latent shape-mismatch bugs where the CLI was reading fields the engine no longer emits and rendering false-clean output as a result. Concretely: (a) `check --checks validate` gated on the wrong field and passed every file, now maps `ValidationError.location` and fails closed on engine failure; (b) `check --checks semantic` treated `valid:true` as clean, but `valid` means "plannable" — cartesian products came back "OK" for months, now reads `findings` and never gates on `valid`; (c) `check --checks grade` read fields `evaluate()` doesn't return, so no grade or finding ever surfaced, now reads `overall_grade`/`scores.overall`/`lint.findings` (with nested findings from `validation.errors` + `safety.threats`); (d) `check --checks pii` mapped the column name to the numeric column-position field and printed `[object Object]` for `{ Custom: string }` classifications, now reports the exposing alias and stringifies properly; (e) `check --checks migration` rendered "SAFE" for every migration including `DROP COLUMN`, now reads `findings`/`safe`/`overall_risk`; (f) `check --checks compare` rendered "IDENTICAL" for different queries, now reads `differences`; (g) `check --checks policy` titled on `pass` (engine returns `allowed`), so clean SQL always rendered "VIOLATIONS FOUND", now inverted; (h) `check --checks safety` maps engine `high → error` and `medium → warning` (previously both degraded to `info`), so `--fail-on error|warning` no longer silently passes high-risk SQL injection threats; a new `unbalanced_quote` safety rule ships from the engine. Findings' `rule` field (when set — `lint` findings may omit it if the engine didn't attach one) names the rule in `--format json` output — that's how the safety and policy catalog is discovered in practice. (#1090) - **`altimate-core-track-lineage` no longer returns "0 edges" for everything.** The tool read `edges` off the top-level result; the engine has always emitted them at `queries[].edges`, so lineage tracking silently produced nothing. Now collects correctly, renders `impact_map`, formats `{table, column}` refs, and renders ERROR instead of "0 edges" when the engine call itself fails. (#1090) - **`schema.detect_pii` returned zero findings for every scan.** Same shape-mismatch class: the detector read `piiData.findings`, but the engine's `PiiReport` is `{ columns, pii_count, … }`. Both cache and live paths now filter `classification !== "None"` through the shared `piiColumnsFromReport` helper. Malformed `PiiReport` shapes now throw rather than silently yield zero findings. Failed per-column classifications flip `success` to `false` — the schema-detect-pii tool renders "PII Scan: ERROR" with any partial findings attached, instead of a clean "no findings" verdict. (#1090) - **`altimate-core-{compare,column-lineage,extract-metadata,import-ddl}` no longer crash on the default invocation.** Empty-string dialects now coerce to `undefined` before hitting `Schema.fromDdl`, centralized as `dialectHint()` in `native/engine-coerce.ts` and applied across all 7 dialect-forwarding handlers. (#1090) diff --git a/docs/docs/usage/check.md b/docs/docs/usage/check.md index 59949884ff..f84a9dfb2d 100644 --- a/docs/docs/usage/check.md +++ b/docs/docs/usage/check.md @@ -50,7 +50,7 @@ altimate-code check --fail-on warning | `semantic` | Semantic validation (cartesian products, wrong JOINs) | Optional | No | | `grade` | SQL quality grading with recommendations | Optional | No | -Every finding carries a machine-readable `rule` field (e.g. `unbalanced_quote`, `sql_injection`, `L003`) — visible in `--format json` output, and how the safety, lint, and policy rule inventory is discovered in practice. +Findings carry a machine-readable `rule` field (e.g. `unbalanced_quote`, `sql_injection`, `L003`) when the underlying engine attaches one — visible in `--format json` output, and how the rule inventory is discovered in practice. A minority of `lint` findings may arrive without `rule` (or `code`); switch on presence, not a hard assumption. By default, `lint` and `safety` are enabled. Override with `--checks`: diff --git a/packages/opencode/src/altimate/native/dispatcher.ts b/packages/opencode/src/altimate/native/dispatcher.ts index af4f2091b5..77168da71a 100644 --- a/packages/opencode/src/altimate/native/dispatcher.ts +++ b/packages/opencode/src/altimate/native/dispatcher.ts @@ -23,17 +23,28 @@ let _ensureRegistered: (() => Promise) | null = null /** In-flight registration promise (deduped across concurrent callers). */ let _registrationPromise: Promise | null = null +/** Generation counter — bumped whenever the hook or in-flight promise is + * replaced. An in-flight attempt captures its generation at start; if the + * counter advanced by the time its settle handler fires, another caller + * (reset / setRegistrationHook / a distinct new attempt after failure) + * has already installed replacement state, and the stale attempt must NOT + * mutate it. Prevents a stale success from clobbering a replacement hook, + * and a stale failure from clobbering a newer in-flight promise. */ +let _registrationGeneration = 0 + /** Clear all registered handlers and lazy registration hook (for test isolation). */ export function reset(): void { nativeHandlers.clear() _ensureRegistered = null _registrationPromise = null + _registrationGeneration++ } /** Called by native/index.ts to set the lazy registration function. */ export function setRegistrationHook(fn: () => Promise): void { _ensureRegistered = fn _registrationPromise = null + _registrationGeneration++ } /** Dispatch a method call to the registered native handler. */ @@ -47,16 +58,19 @@ export async function call( // ``_ensureRegistered`` was nulled BEFORE the await, so a transient NAPI // load failure poisoned the bridge for the process lifetime — every // subsequent ``call`` threw ``No native handler for X`` with no way to - // recover without restarting the CLI. + // recover without restarting the CLI. Generation guard prevents a stale + // attempt from mutating state a concurrent ``reset()``/``setRegistrationHook()`` + // has since replaced. (coderabbit round 1 — release/v0.9.6 review.) if (_ensureRegistered) { if (!_registrationPromise) { const fn = _ensureRegistered + const generation = ++_registrationGeneration _registrationPromise = fn().then( () => { - _ensureRegistered = null + if (generation === _registrationGeneration) _ensureRegistered = null }, (err) => { - _registrationPromise = null + if (generation === _registrationGeneration) _registrationPromise = null throw err }, ) diff --git a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts index 05d7369bf0..47d7cc35a2 100644 --- a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts +++ b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts @@ -106,6 +106,92 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { expect(attempts).toBe(1) }) + test("reset() while a hook is pending — stale success does NOT clobber the reset state (generation guard)", async () => { + // coderabbit round 1 catch on release/v0.9.6: without a generation guard, + // an old registration attempt's success handler nulls _ensureRegistered, + // wiping out a replacement hook that reset() installed mid-flight. Later + // calls then skip registration entirely and see stale (empty) state. + let resolvePending: () => void = () => {} + const pending = new Promise((r) => (resolvePending = r)) + Dispatcher.setRegistrationHook(async () => { + await pending + Dispatcher.register("ping", async () => ({ status: "old-hook-ran" })) + }) + + // Start the first call — hook attempt is now in flight, awaiting `pending`. + const firstCall = Dispatcher.call("ping", {} as any) + await new Promise((r) => setTimeout(r, 5)) + + // Concurrent reset + install a REPLACEMENT hook. Without generation guard, + // when the pending attempt finally resolves, it will null _ensureRegistered + // — clobbering the replacement. + Dispatcher.reset() + let replacementRan = false + Dispatcher.setRegistrationHook(async () => { + replacementRan = true + Dispatcher.register("ping", async () => ({ status: "replacement" })) + }) + + // Let the old attempt resolve. Its success handler must skip the mutation + // because the generation has advanced twice (reset + setRegistrationHook). + resolvePending() + await firstCall.catch(() => {}) // may reject or resolve; doesn't matter here + + // A new call should invoke the REPLACEMENT hook, not skip registration. + const result = await Dispatcher.call("ping", {} as any) + expect(replacementRan).toBe(true) + expect(result).toEqual({ status: "replacement" }) + }) + + test("setRegistrationHook() while a hook is pending — stale failure does NOT clobber the new in-flight promise", async () => { + // Companion to the previous case: the failure handler also mutates + // shared state (_registrationPromise = null), so it needs the same guard. + // Scenario: old hook is failing, but during its await, setRegistrationHook + // installs a new hook AND a new call kicks off a new registration + // promise. Without the guard, the old attempt's failure handler nulls + // the NEW _registrationPromise mid-flight, breaking dedup for anyone + // else awaiting it. + let rejectOld: (err: Error) => void = () => {} + const oldPending = new Promise((_, rej) => (rejectOld = rej)) + Dispatcher.setRegistrationHook(async () => { + await oldPending + }) + + // Start first call — will fail once we reject. + const firstCall = Dispatcher.call("ping", {} as any) + await new Promise((r) => setTimeout(r, 5)) + + // Install new hook; kick off a second call which caches its own promise. + let newHookAttempts = 0 + let resolveNew: () => void = () => {} + const newPending = new Promise((r) => (resolveNew = r)) + Dispatcher.setRegistrationHook(async () => { + newHookAttempts += 1 + await newPending + Dispatcher.register("ping", async () => ({ status: "new-hook-ran" })) + }) + const secondCall = Dispatcher.call("ping", {} as any) + await new Promise((r) => setTimeout(r, 5)) + + // Reject the old attempt. Its failure handler must NOT null the second + // call's cached promise. + rejectOld(new Error("old attempt failed")) + await firstCall.catch(() => {}) + + // Third caller arrives — should share the second call's still-cached + // promise (dedup works), not start a third registration attempt. + const thirdCall = Dispatcher.call("ping", {} as any) + resolveNew() + + const [r2, r3] = await Promise.all([secondCall, thirdCall]) + expect(r2).toEqual({ status: "new-hook-ran" }) + expect(r3).toEqual({ status: "new-hook-ran" }) + // Critical: new hook fired ONCE despite second + third call both + // dedup-sharing its promise, proving the stale failure handler didn't + // null the cached promise and let a third attempt start. + expect(newHookAttempts).toBe(1) + }) + test("reset() clears both the hook and the cached in-flight promise", async () => { // The reset added a new field (_registrationPromise) that also needs // to be cleared, otherwise test isolation regresses: a failed From 6400201b75a12598ca276b821e77f8d5dc0c4c05 Mon Sep 17 00:00:00 2001 From: Haider Date: Sun, 23 Aug 2026 23:04:34 +0530 Subject: [PATCH 3/6] =?UTF-8?q?release:=20v0.9.6=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20self-heal=20dispatcher=20after=20stale=20register()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses second round of coderabbit + cubic findings on release/v0.9.6: - **cubic P2 (dispatcher.ts)** — the previous generation guard prevented a stale hook's `.then` handler from mutating shared state, but the old hook BODY itself could still `Dispatcher.register()` late and clobber fresh entries a newer hook had already written. Fix: on stale-generation success, clear `_registrationPromise` so the next `Dispatcher.call` re-runs the CURRENT hook — its `register()` calls then idempotently overwrite whatever the stale hook wrote. Successful current-generation attempts keep the resolved promise memoized so subsequent calls fast-path through an already-settled await. - **cubic P3 (test file)** — replaced every `setTimeout` sync point with Promise-gate synchronisation. Bun's `async` function bodies run sync until the first `await`, so `Dispatcher.call(...)` has already registered its cached promise and hit `await _registrationPromise` by the time control returns to us — no external delay needed. Tests are now scheduler-independent. - **cubic P3 (docs/usage/check.md)** — rewrote the dangling "and how the rule inventory is discovered in practice" clause. Now links to the Finding Object schema and calls out which check types always vs sometimes include `rule`. - New adversarial test covering the stale-register self-heal path; full dispatcher suite: 15/15 pass. Non-vacuous: verified the new test FAILS if the P2 fix is reverted. Co-Authored-By: Claude Opus 4.7 --- docs/docs/usage/check.md | 2 +- .../src/altimate/native/dispatcher.ts | 13 +- .../skill/release-v0.9.6-adversarial.test.ts | 144 +++++++++++------- 3 files changed, 99 insertions(+), 60 deletions(-) diff --git a/docs/docs/usage/check.md b/docs/docs/usage/check.md index f84a9dfb2d..f79aedc5dd 100644 --- a/docs/docs/usage/check.md +++ b/docs/docs/usage/check.md @@ -50,7 +50,7 @@ altimate-code check --fail-on warning | `semantic` | Semantic validation (cartesian products, wrong JOINs) | Optional | No | | `grade` | SQL quality grading with recommendations | Optional | No | -Findings carry a machine-readable `rule` field (e.g. `unbalanced_quote`, `sql_injection`, `L003`) when the underlying engine attaches one — visible in `--format json` output, and how the rule inventory is discovered in practice. A minority of `lint` findings may arrive without `rule` (or `code`); switch on presence, not a hard assumption. +Findings usually carry a machine-readable `rule` field (e.g. `unbalanced_quote`, `sql_injection`, `L003`) visible in `--format json` output — see the [Finding Object](#finding-object) schema below. `safety` and `policy` findings always include one; `lint` findings may omit both `rule` and `code` if the engine didn't attach them, so switch on presence rather than assuming. By default, `lint` and `safety` are enabled. Override with `--checks`: diff --git a/packages/opencode/src/altimate/native/dispatcher.ts b/packages/opencode/src/altimate/native/dispatcher.ts index 77168da71a..5a609b958a 100644 --- a/packages/opencode/src/altimate/native/dispatcher.ts +++ b/packages/opencode/src/altimate/native/dispatcher.ts @@ -67,7 +67,18 @@ export async function call( const generation = ++_registrationGeneration _registrationPromise = fn().then( () => { - if (generation === _registrationGeneration) _ensureRegistered = null + // Generation advanced while this attempt was in flight (concurrent + // ``reset()``/``setRegistrationHook()`` + another call arrived). + // The stale hook body may have written stale entries into + // ``nativeHandlers`` via late ``register()`` calls, clobbering the + // newer hook's. Clear ``_registrationPromise`` so the NEXT + // ``Dispatcher.call`` re-runs the current hook — its ``register()`` + // calls then overwrite whatever the stale hook wrote. Hook bodies + // must be idempotent (they are today — ``register`` is a plain + // ``Map.set``). Successful current-generation attempts leave the + // resolved promise memoized so subsequent calls fast-path through + // an already-settled ``await``. (cubic round 2 on release/v0.9.6.) + if (generation !== _registrationGeneration) _registrationPromise = null }, (err) => { if (generation === _registrationGeneration) _registrationPromise = null diff --git a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts index 47d7cc35a2..e3ca7bdb21 100644 --- a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts +++ b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts @@ -3,14 +3,22 @@ * * Focus: fixes that landed IN this release (not the whole PR history) — * 1. Dispatcher retry-after-registration-failure (v0.9.6 review gremlin fix) - * 2. Truncation cleanup: fail-safe on stat error, mtime-based aging + * 2. Dispatcher generation guard (coderabbit round 1) + * 3. Dispatcher stale-register self-heal (cubic round 2) * * Not covered here (existing test suites are authoritative): * - altimate-core 0.7.0 shape corrections — see - * packages/opencode/test/altimate/altimate-core-e2e.test.ts (26 files, - * real-engine "consumer contract sync" blocks land there) + * packages/opencode/test/altimate/altimate-core-e2e.test.ts * - Truncation ID-wrap regression — see * packages/opencode/test/tool/truncation.test.ts + * + * Scheduling discipline: these tests never use ``setTimeout``. Every + * synchronisation point is a Promise gate that the test controls, so the + * ordering is deterministic regardless of the underlying scheduler. Bun's + * ``async`` function bodies run synchronously until the first ``await``, so + * ``Dispatcher.call(...)`` has already registered its cached promise and + * hit ``await _registrationPromise`` by the time control returns to us — + * we can immediately act on shared state without racing the call's setup. */ import { afterEach, beforeAll, afterAll, beforeEach, describe, expect, test } from "bun:test" @@ -47,7 +55,6 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { Dispatcher.register("ping", async () => ({ status: "recovered" })) }) - // First call: registration fails, error propagates. await expect(Dispatcher.call("ping", {} as any)).rejects.toThrow( "simulated NAPI load failure", ) @@ -61,37 +68,36 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { expect(attempts).toBe(2) }) - test("concurrent calls share ONE registration attempt on both success and failure", async () => { - // Guard the dedup property — the fix caches the in-flight promise so - // 100 racing calls trigger the hook exactly once, not 100 times. + test("concurrent calls share ONE registration attempt", async () => { + // Fires 20 concurrent calls before the hook completes; asserts the + // hook body ran exactly once. All 20 Dispatcher.call invocations run + // sync-then-yield in a row, so by the time the array is populated + // every caller is already awaiting the shared _registrationPromise — + // no external synchronisation needed. let attempts = 0 let resolveRegistration: () => void = () => {} - const registrationGate = new Promise((resolve) => { - resolveRegistration = resolve - }) + const registrationGate = new Promise((r) => (resolveRegistration = r)) Dispatcher.setRegistrationHook(async () => { attempts += 1 await registrationGate Dispatcher.register("ping", async () => ({ status: "ok" })) }) - // Fire 20 concurrent calls before registration completes. const calls = Array.from({ length: 20 }, () => Dispatcher.call("ping", {} as any)) - // Let the concurrent callers all enter the registration branch. - await new Promise((r) => setTimeout(r, 10)) resolveRegistration() - const results = await Promise.all(calls) + expect(results).toHaveLength(20) for (const r of results) expect(r).toEqual({ status: "ok" }) // Critical: the hook fired exactly ONCE despite 20 concurrent callers. expect(attempts).toBe(1) }) - test("successful registration clears the hook — a later call does NOT re-run it", async () => { - // Guard against overcorrection: the fix must still short-circuit after - // success, otherwise every subsequent call would eagerly re-import all - // handler modules for no reason. + test("successful registration is memoized — a later call does NOT re-run the hook", async () => { + // Guard against overcorrection: once the current-generation hook has + // completed, the resolved _registrationPromise memoizes success so + // subsequent calls fast-path through an already-settled await instead + // of re-importing all handler modules for no reason. let attempts = 0 Dispatcher.setRegistrationHook(async () => { attempts += 1 @@ -100,17 +106,15 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { await Dispatcher.call("ping", {} as any) expect(attempts).toBe(1) - - // Later call: hook must NOT re-run (it was cleared on success). await Dispatcher.call("ping", {} as any) expect(attempts).toBe(1) }) - test("reset() while a hook is pending — stale success does NOT clobber the reset state (generation guard)", async () => { - // coderabbit round 1 catch on release/v0.9.6: without a generation guard, - // an old registration attempt's success handler nulls _ensureRegistered, - // wiping out a replacement hook that reset() installed mid-flight. Later - // calls then skip registration entirely and see stale (empty) state. + test("reset() while a hook is pending — stale success does NOT clobber the replacement (generation guard)", async () => { + // coderabbit round 1 on release/v0.9.6: without a generation guard, + // an old attempt's success handler would null _ensureRegistered even + // after reset() + setRegistrationHook() had installed a replacement, + // wiping it out and causing later calls to skip registration. let resolvePending: () => void = () => {} const pending = new Promise((r) => (resolvePending = r)) Dispatcher.setRegistrationHook(async () => { @@ -118,13 +122,11 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { Dispatcher.register("ping", async () => ({ status: "old-hook-ran" })) }) - // Start the first call — hook attempt is now in flight, awaiting `pending`. + // Dispatcher.call runs sync until its first await, so by the time this + // returns the call is already blocked on _registrationPromise and we + // can safely mutate shared state without a race. const firstCall = Dispatcher.call("ping", {} as any) - await new Promise((r) => setTimeout(r, 5)) - // Concurrent reset + install a REPLACEMENT hook. Without generation guard, - // when the pending attempt finally resolves, it will null _ensureRegistered - // — clobbering the replacement. Dispatcher.reset() let replacementRan = false Dispatcher.setRegistrationHook(async () => { @@ -132,36 +134,25 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { Dispatcher.register("ping", async () => ({ status: "replacement" })) }) - // Let the old attempt resolve. Its success handler must skip the mutation - // because the generation has advanced twice (reset + setRegistrationHook). resolvePending() - await firstCall.catch(() => {}) // may reject or resolve; doesn't matter here + await firstCall.catch(() => {}) - // A new call should invoke the REPLACEMENT hook, not skip registration. const result = await Dispatcher.call("ping", {} as any) expect(replacementRan).toBe(true) expect(result).toEqual({ status: "replacement" }) }) test("setRegistrationHook() while a hook is pending — stale failure does NOT clobber the new in-flight promise", async () => { - // Companion to the previous case: the failure handler also mutates - // shared state (_registrationPromise = null), so it needs the same guard. - // Scenario: old hook is failing, but during its await, setRegistrationHook - // installs a new hook AND a new call kicks off a new registration - // promise. Without the guard, the old attempt's failure handler nulls - // the NEW _registrationPromise mid-flight, breaking dedup for anyone - // else awaiting it. + // Failure handler also mutates shared state (_registrationPromise = null), + // so it needs the same generation guard. Otherwise a stale failure + // clears a newer in-flight promise, breaking dedup for anyone awaiting it. let rejectOld: (err: Error) => void = () => {} const oldPending = new Promise((_, rej) => (rejectOld = rej)) Dispatcher.setRegistrationHook(async () => { await oldPending }) - - // Start first call — will fail once we reject. const firstCall = Dispatcher.call("ping", {} as any) - await new Promise((r) => setTimeout(r, 5)) - // Install new hook; kick off a second call which caches its own promise. let newHookAttempts = 0 let resolveNew: () => void = () => {} const newPending = new Promise((r) => (resolveNew = r)) @@ -171,14 +162,11 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { Dispatcher.register("ping", async () => ({ status: "new-hook-ran" })) }) const secondCall = Dispatcher.call("ping", {} as any) - await new Promise((r) => setTimeout(r, 5)) - // Reject the old attempt. Its failure handler must NOT null the second - // call's cached promise. rejectOld(new Error("old attempt failed")) await firstCall.catch(() => {}) - // Third caller arrives — should share the second call's still-cached + // Third caller arrives — must share the second call's still-cached // promise (dedup works), not start a third registration attempt. const thirdCall = Dispatcher.call("ping", {} as any) resolveNew() @@ -186,17 +174,59 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { const [r2, r3] = await Promise.all([secondCall, thirdCall]) expect(r2).toEqual({ status: "new-hook-ran" }) expect(r3).toEqual({ status: "new-hook-ran" }) - // Critical: new hook fired ONCE despite second + third call both - // dedup-sharing its promise, proving the stale failure handler didn't - // null the cached promise and let a third attempt start. + // Critical: new hook fired ONCE despite second + third both dedup-sharing + // its promise — proving the stale failure handler didn't null the cache. expect(newHookAttempts).toBe(1) }) + test("stale hook's LATE register() call is self-healed by a re-run on the next call", async () => { + // cubic round 2 on release/v0.9.6: the generation guard prevented a + // stale success handler from mutating _ensureRegistered / _registrationPromise, + // but the old HOOK BODY itself could still write stale entries into + // nativeHandlers via late register() calls after a new hook had already + // filled them. Fix: on stale-generation success, clear _registrationPromise + // so the next Dispatcher.call re-runs the CURRENT hook and its idempotent + // register() calls overwrite whatever the stale hook wrote. + let resolveOld: () => void = () => {} + const oldPending = new Promise((r) => (resolveOld = r)) + Dispatcher.setRegistrationHook(async () => { + await oldPending + Dispatcher.register("ping", async () => ({ status: "stale-old" })) + }) + const firstCall = Dispatcher.call("ping", {} as any) + + Dispatcher.reset() + let newRunCount = 0 + Dispatcher.setRegistrationHook(async () => { + newRunCount += 1 + Dispatcher.register("ping", async () => ({ status: "fresh-new" })) + }) + + // Second call runs the new (fast) hook to completion — registers + // "ping" -> fresh-new, memoized as the resolved promise. + const r1 = await Dispatcher.call("ping", {} as any) + expect(r1).toEqual({ status: "fresh-new" }) + expect(newRunCount).toBe(1) + + // Now the old hook wakes up and belatedly overwrites "ping" with + // stale-old via its own register() call. Without the self-heal, the + // next Dispatcher.call would silently return the stale handler because + // registration is memoized "done" and never re-runs. + resolveOld() + await firstCall.catch(() => {}) + + // Third call: the fix detects the stale-generation success, clears + // the memoized promise so the CURRENT hook re-runs and overwrites + // "ping" back to fresh-new. + const r2 = await Dispatcher.call("ping", {} as any) + expect(r2).toEqual({ status: "fresh-new" }) + expect(newRunCount).toBe(2) // re-ran to self-heal the stale register() + }) + test("reset() clears both the hook and the cached in-flight promise", async () => { - // The reset added a new field (_registrationPromise) that also needs - // to be cleared, otherwise test isolation regresses: a failed - // registration in one test would leave the cached rejected promise - // pinned for the next test. + // reset() must wipe both _ensureRegistered AND _registrationPromise — + // otherwise a failed registration in one test leaves the cached + // rejected promise pinned for the next test. let attempts = 0 Dispatcher.setRegistrationHook(async () => { attempts += 1 @@ -205,8 +235,6 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { await expect(Dispatcher.call("ping", {} as any)).rejects.toThrow("boom") expect(attempts).toBe(1) - // Reset should wipe both hook and cached promise. Setting a new hook - // must let call() invoke IT, not a stale rejected promise. Dispatcher.reset() Dispatcher.setRegistrationHook(async () => { Dispatcher.register("ping", async () => ({ status: "fresh" })) From 779953cc6554dfe760717227cb288b41c5d8a623 Mon Sep 17 00:00:00 2001 From: Haider Date: Sun, 23 Aug 2026 23:13:40 +0530 Subject: [PATCH 4/6] =?UTF-8?q?release:=20v0.9.6=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20revert=20P2=20self-heal;=20document=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the round-2 self-heal (was: on stale-generation success, clear `_registrationPromise` so the current hook re-runs). Coderabbit + cubic both correctly flagged that this reintroduces the very race the round-1 generation guard was meant to prevent: if the stale hook resolves while a REPLACEMENT hook is still in flight, clearing `_registrationPromise` clobbers the newer attempt's cached promise — a third caller then starts a second registration attempt, breaking dedup. Every attempt to self-heal without inventing a heavier per-entry generation scheme (or wrapping ``register()`` with a generation guard) introduces another race. Doing that here would materially complicate the dispatcher for a scenario that never occurs in production — ``setRegistrationHook`` is called exactly once at startup by ``native/index.ts``, and ``reset()`` is test-only. Test-authored races that violate isolation are the caller's contract, not this module's correctness problem. - Revert to round-1 logic (generation guard on shared-state mutations only) - Remove the "stale hook self-heal" adversarial test — it was locking in behavior we've decided not to guarantee - Add explicit contract documentation to `dispatcher.ts` and to the adversarial test file's top docstring so the design decision is discoverable to reviewers next time Dispatcher suite: 14/14 pass (was 15 with the deleted self-heal test). The round-1 generation guard is retained and still tested. Co-Authored-By: Claude Opus 4.7 --- .../src/altimate/native/dispatcher.ts | 27 +++++---- .../skill/release-v0.9.6-adversarial.test.ts | 57 ++++--------------- 2 files changed, 26 insertions(+), 58 deletions(-) diff --git a/packages/opencode/src/altimate/native/dispatcher.ts b/packages/opencode/src/altimate/native/dispatcher.ts index 5a609b958a..9792d748e8 100644 --- a/packages/opencode/src/altimate/native/dispatcher.ts +++ b/packages/opencode/src/altimate/native/dispatcher.ts @@ -61,26 +61,29 @@ export async function call( // recover without restarting the CLI. Generation guard prevents a stale // attempt from mutating state a concurrent ``reset()``/``setRegistrationHook()`` // has since replaced. (coderabbit round 1 — release/v0.9.6 review.) + // Test-isolation contract: ``reset()`` and ``setRegistrationHook()`` MUST + // NOT be called while a ``Dispatcher.call`` is in flight — if they are, + // any late ``register()`` calls from the stale hook body may clobber + // fresh entries the new hook wrote, and there is no in-band signal we + // can use to self-heal it without introducing a second race (see the + // coderabbit + cubic round-2 exchange on release/v0.9.6). Production + // never triggers this: ``setRegistrationHook`` is called exactly once + // at startup by native/index.ts, and ``reset()`` is test-only. Tests + // must ``await`` outstanding calls before mutating hook state. if (_ensureRegistered) { if (!_registrationPromise) { const fn = _ensureRegistered const generation = ++_registrationGeneration _registrationPromise = fn().then( () => { - // Generation advanced while this attempt was in flight (concurrent - // ``reset()``/``setRegistrationHook()`` + another call arrived). - // The stale hook body may have written stale entries into - // ``nativeHandlers`` via late ``register()`` calls, clobbering the - // newer hook's. Clear ``_registrationPromise`` so the NEXT - // ``Dispatcher.call`` re-runs the current hook — its ``register()`` - // calls then overwrite whatever the stale hook wrote. Hook bodies - // must be idempotent (they are today — ``register`` is a plain - // ``Map.set``). Successful current-generation attempts leave the - // resolved promise memoized so subsequent calls fast-path through - // an already-settled ``await``. (cubic round 2 on release/v0.9.6.) - if (generation !== _registrationGeneration) _registrationPromise = null + // Only clear _ensureRegistered if our generation is still current + // — otherwise a concurrent reset()/setRegistrationHook() already + // installed a replacement, and clearing would clobber it. + if (generation === _registrationGeneration) _ensureRegistered = null }, (err) => { + // Same guard on the failure path: don't null a newer in-flight + // promise from another attempt. if (generation === _registrationGeneration) _registrationPromise = null throw err }, diff --git a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts index e3ca7bdb21..a41682e17c 100644 --- a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts +++ b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts @@ -3,8 +3,17 @@ * * Focus: fixes that landed IN this release (not the whole PR history) — * 1. Dispatcher retry-after-registration-failure (v0.9.6 review gremlin fix) - * 2. Dispatcher generation guard (coderabbit round 1) - * 3. Dispatcher stale-register self-heal (cubic round 2) + * 2. Dispatcher generation guard on shared-state mutations from stale + * .then handlers (coderabbit round 1) + * + * Explicitly NOT covered (test-isolation contract, see dispatcher.ts): + * - Late ``register()`` from a stale hook body after a replacement hook + * has already run. That scenario requires calling ``reset()`` / + * ``setRegistrationHook()`` while a call is still in flight — a + * production impossibility (hook is set once at startup, reset is + * test-only) and a violation of the test-isolation contract. See + * ``dispatcher.ts`` for the design decision and the coderabbit/cubic + * round-2 exchange that arrived at it. * * Not covered here (existing test suites are authoritative): * - altimate-core 0.7.0 shape corrections — see @@ -179,50 +188,6 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { expect(newHookAttempts).toBe(1) }) - test("stale hook's LATE register() call is self-healed by a re-run on the next call", async () => { - // cubic round 2 on release/v0.9.6: the generation guard prevented a - // stale success handler from mutating _ensureRegistered / _registrationPromise, - // but the old HOOK BODY itself could still write stale entries into - // nativeHandlers via late register() calls after a new hook had already - // filled them. Fix: on stale-generation success, clear _registrationPromise - // so the next Dispatcher.call re-runs the CURRENT hook and its idempotent - // register() calls overwrite whatever the stale hook wrote. - let resolveOld: () => void = () => {} - const oldPending = new Promise((r) => (resolveOld = r)) - Dispatcher.setRegistrationHook(async () => { - await oldPending - Dispatcher.register("ping", async () => ({ status: "stale-old" })) - }) - const firstCall = Dispatcher.call("ping", {} as any) - - Dispatcher.reset() - let newRunCount = 0 - Dispatcher.setRegistrationHook(async () => { - newRunCount += 1 - Dispatcher.register("ping", async () => ({ status: "fresh-new" })) - }) - - // Second call runs the new (fast) hook to completion — registers - // "ping" -> fresh-new, memoized as the resolved promise. - const r1 = await Dispatcher.call("ping", {} as any) - expect(r1).toEqual({ status: "fresh-new" }) - expect(newRunCount).toBe(1) - - // Now the old hook wakes up and belatedly overwrites "ping" with - // stale-old via its own register() call. Without the self-heal, the - // next Dispatcher.call would silently return the stale handler because - // registration is memoized "done" and never re-runs. - resolveOld() - await firstCall.catch(() => {}) - - // Third call: the fix detects the stale-generation success, clears - // the memoized promise so the CURRENT hook re-runs and overwrites - // "ping" back to fresh-new. - const r2 = await Dispatcher.call("ping", {} as any) - expect(r2).toEqual({ status: "fresh-new" }) - expect(newRunCount).toBe(2) // re-ran to self-heal the stale register() - }) - test("reset() clears both the hook and the cached in-flight promise", async () => { // reset() must wipe both _ensureRegistered AND _registrationPromise — // otherwise a failed registration in one test leaves the cached From be3b9e8189a587ca88172a77b6afa4b83b78d2a6 Mon Sep 17 00:00:00 2001 From: Haider Date: Sun, 23 Aug 2026 23:30:00 +0530 Subject: [PATCH 5/6] =?UTF-8?q?release:=20v0.9.6=20review=20round=204=20?= =?UTF-8?q?=E2=80=94=20clarify=20dispatcher=20concurrency=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P3 catch: my round-3 wording "reset()/setRegistrationHook() MUST NOT be called while a call is in flight" is contradicted by this same PR's adversarial tests — which do exactly that on purpose, to exercise the generation guard on the .then handlers. The wording was too broad. Rewrote to distinguish two claims: • What IS guaranteed: shared-state mutation (`_ensureRegistered` / `_registrationPromise`) by a stale attempt's .then handler is blocked by the generation guard. Concurrent reset/setRegistrationHook is safe wrt that. • What is NOT: late `Dispatcher.register(...)` calls from a stale hook BODY (that resumes after replacement) overwrite whatever the newer hook wrote. No self-heal — chased twice, recreated the guard's race both times. • Why: production never triggers late-write clobber (hook set once at startup, reset() test-only). Wording-only. No code change. Co-Authored-By: Claude Opus 4.7 --- .../src/altimate/native/dispatcher.ts | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/opencode/src/altimate/native/dispatcher.ts b/packages/opencode/src/altimate/native/dispatcher.ts index 9792d748e8..5ea9076470 100644 --- a/packages/opencode/src/altimate/native/dispatcher.ts +++ b/packages/opencode/src/altimate/native/dispatcher.ts @@ -61,15 +61,23 @@ export async function call( // recover without restarting the CLI. Generation guard prevents a stale // attempt from mutating state a concurrent ``reset()``/``setRegistrationHook()`` // has since replaced. (coderabbit round 1 — release/v0.9.6 review.) - // Test-isolation contract: ``reset()`` and ``setRegistrationHook()`` MUST - // NOT be called while a ``Dispatcher.call`` is in flight — if they are, - // any late ``register()`` calls from the stale hook body may clobber - // fresh entries the new hook wrote, and there is no in-band signal we - // can use to self-heal it without introducing a second race (see the - // coderabbit + cubic round-2 exchange on release/v0.9.6). Production - // never triggers this: ``setRegistrationHook`` is called exactly once - // at startup by native/index.ts, and ``reset()`` is test-only. Tests - // must ``await`` outstanding calls before mutating hook state. + // Concurrency contract: + // • ``reset()`` / ``setRegistrationHook()`` MAY be called while an + // older ``Dispatcher.call`` is in flight — the generation guard below + // blocks the stale attempt's ``.then`` handler from mutating shared + // state (``_ensureRegistered`` / ``_registrationPromise``) that the + // replacement installed. Adversarial tests below exercise both races. + // • What we DO NOT guarantee: if the stale hook body itself resumes + // after replacement and calls ``Dispatcher.register(...)`` late, + // that late write overwrites whatever the newer hook wrote — and + // no in-band signal lets us self-heal it without recreating the + // shared-state race the round-1 guard is meant to prevent (see the + // coderabbit + cubic round-2 exchange on release/v0.9.6). Callers + // that need late-write safety must serialise hook mutations + // against outstanding calls themselves. + // • Production never triggers late-write clobber: ``setRegistrationHook`` + // is called exactly once at startup by ``native/index.ts``, and + // ``reset()`` is test-only. if (_ensureRegistered) { if (!_registrationPromise) { const fn = _ensureRegistered From b823ea6160b39714acd394b9ed128b0b0d650d1b Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 24 Aug 2026 01:08:34 +0530 Subject: [PATCH 6/6] =?UTF-8?q?release:=20v0.9.6=20review=20round=205=20?= =?UTF-8?q?=E2=80=94=20CHANGELOG=20+=20test-isolation=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three cubic P2 findings on release/v0.9.6: - **CHANGELOG L10 + bullet (e)/(f)**: my theme + bullets listed `migration` and `compare` as valid `check --checks` lanes. They are not — `VALID_CHECKS` in `check-helpers.ts` only accepts {lint, validate, safety, policy, pii, semantic, grade}. The migration and compare correctness fixes belong to the `altimate-core-migration` and `altimate-core-compare` TOOLS (agent tools, not check lanes). Moved them to their own bullets and dropped them from the check-lane list in the theme. - **Test env-var teardown**: `afterAll` unconditionally deleted `ALTIMATE_TELEMETRY_DISABLED`, wiping any pre-existing value an outer suite may have set. Now captures the prior value in `beforeAll` and restores it (or deletes if none was set). - **Concurrent-failure test coverage**: added a companion to the concurrent-success test that fires 20 concurrent calls against a rejecting hook and asserts every one rejects with the SAME error instance (proving dedup held across the failure path) with the hook body having run exactly once. 15/15 dispatcher tests pass (14 prior + 1 new). Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 6 ++- .../skill/release-v0.9.6-adversarial.test.ts | 40 ++++++++++++++++++- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c22df5ebc..73c367336d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [0.9.6] - 2026-08-23 -Correctness release: `check --checks {validate,semantic,grade,pii,migration,safety,policy,compare}` and the PII/lineage/migration tools now tell you the truth. Several previously returned false-clean or wrong output — teams gating CI on `check --fail-on error|warning` may see new failures on unchanged SQL. **These are real findings the tool previously missed, not regressions in your code.** Also fixes a data-hygiene bug where truncated tool-output files were being deleted the moment they were written. +Correctness release: `check --checks {validate,semantic,grade,pii,safety,policy}` and the `altimate-core-*` tools (`migration`, `compare`, `track-lineage`, `query-pii`, `classify-pii`) now tell you the truth. Several previously returned false-clean or wrong output — teams gating CI on `check --fail-on error|warning` may see new failures on unchanged SQL. **These are real findings the tool previously missed, not regressions in your code.** Also fixes a data-hygiene bug where truncated tool-output files were being deleted the moment they were written. ### Changed -- **`check --checks *` now surfaces the findings it was missing — expect newly-failing CI runs.** `altimate-core` upgrade `0.5.1 → 0.7.0` plus a consumer-contract sync catches a class of latent shape-mismatch bugs where the CLI was reading fields the engine no longer emits and rendering false-clean output as a result. Concretely: (a) `check --checks validate` gated on the wrong field and passed every file, now maps `ValidationError.location` and fails closed on engine failure; (b) `check --checks semantic` treated `valid:true` as clean, but `valid` means "plannable" — cartesian products came back "OK" for months, now reads `findings` and never gates on `valid`; (c) `check --checks grade` read fields `evaluate()` doesn't return, so no grade or finding ever surfaced, now reads `overall_grade`/`scores.overall`/`lint.findings` (with nested findings from `validation.errors` + `safety.threats`); (d) `check --checks pii` mapped the column name to the numeric column-position field and printed `[object Object]` for `{ Custom: string }` classifications, now reports the exposing alias and stringifies properly; (e) `check --checks migration` rendered "SAFE" for every migration including `DROP COLUMN`, now reads `findings`/`safe`/`overall_risk`; (f) `check --checks compare` rendered "IDENTICAL" for different queries, now reads `differences`; (g) `check --checks policy` titled on `pass` (engine returns `allowed`), so clean SQL always rendered "VIOLATIONS FOUND", now inverted; (h) `check --checks safety` maps engine `high → error` and `medium → warning` (previously both degraded to `info`), so `--fail-on error|warning` no longer silently passes high-risk SQL injection threats; a new `unbalanced_quote` safety rule ships from the engine. Findings' `rule` field (when set — `lint` findings may omit it if the engine didn't attach one) names the rule in `--format json` output — that's how the safety and policy catalog is discovered in practice. (#1090) +- **`check --checks *` now surfaces the findings it was missing — expect newly-failing CI runs.** `altimate-core` upgrade `0.5.1 → 0.7.0` plus a consumer-contract sync catches a class of latent shape-mismatch bugs where the CLI was reading fields the engine no longer emits and rendering false-clean output as a result. Concretely: (a) `check --checks validate` gated on the wrong field and passed every file, now maps `ValidationError.location` and fails closed on engine failure; (b) `check --checks semantic` treated `valid:true` as clean, but `valid` means "plannable" — cartesian products came back "OK" for months, now reads `findings` and never gates on `valid`; (c) `check --checks grade` read fields `evaluate()` doesn't return, so no grade or finding ever surfaced, now reads `overall_grade`/`scores.overall`/`lint.findings` (with nested findings from `validation.errors` + `safety.threats`); (d) `check --checks pii` mapped the column name to the numeric column-position field and printed `[object Object]` for `{ Custom: string }` classifications, now reports the exposing alias and stringifies properly; (e) `check --checks policy` titled on `pass` (engine returns `allowed`), so clean SQL always rendered "VIOLATIONS FOUND", now inverted; (f) `check --checks safety` maps engine `high → error` and `medium → warning` (previously both degraded to `info`), so `--fail-on error|warning` no longer silently passes high-risk SQL injection threats; a new `unbalanced_quote` safety rule ships from the engine. Findings' `rule` field (when set — `lint` findings may omit it if the engine didn't attach one) names the rule in `--format json` output — that's how the safety and policy catalog is discovered in practice. (#1090) +- **`altimate-core-migration` tool no longer renders "SAFE" for destructive migrations.** Previously read a non-existent `risks` field; every migration including `DROP COLUMN` came back safe. Now reads `findings`/`safe`/`overall_risk` (engine `MigrationResult`); never renders SAFE on engine error; counts only non-"safe" findings as risks. (#1090) +- **`altimate-core-compare` tool no longer says "IDENTICAL" for different queries.** Read a non-existent `differences` field; different SQL always compared equal. Now reads the engine's `identical`/`diff_count`/`diffs` shape with an error-gated title. (#1090) - **`altimate-core-track-lineage` no longer returns "0 edges" for everything.** The tool read `edges` off the top-level result; the engine has always emitted them at `queries[].edges`, so lineage tracking silently produced nothing. Now collects correctly, renders `impact_map`, formats `{table, column}` refs, and renders ERROR instead of "0 edges" when the engine call itself fails. (#1090) - **`schema.detect_pii` returned zero findings for every scan.** Same shape-mismatch class: the detector read `piiData.findings`, but the engine's `PiiReport` is `{ columns, pii_count, … }`. Both cache and live paths now filter `classification !== "None"` through the shared `piiColumnsFromReport` helper. Malformed `PiiReport` shapes now throw rather than silently yield zero findings. Failed per-column classifications flip `success` to `false` — the schema-detect-pii tool renders "PII Scan: ERROR" with any partial findings attached, instead of a clean "no findings" verdict. (#1090) - **`altimate-core-{compare,column-lineage,extract-metadata,import-ddl}` no longer crash on the default invocation.** Empty-string dialects now coerce to `undefined` before hitting `Schema.fromDdl`, centralized as `dialectHint()` in `native/engine-coerce.ts` and applied across all 7 dialect-forwarding handlers. (#1090) diff --git a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts index a41682e17c..92e178b0fb 100644 --- a/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts +++ b/packages/opencode/test/skill/release-v0.9.6-adversarial.test.ts @@ -33,11 +33,20 @@ import { afterEach, beforeAll, afterAll, beforeEach, describe, expect, test } fr import * as Dispatcher from "../../src/altimate/native/dispatcher" +let _priorTelemetryDisabled: string | undefined beforeAll(() => { + _priorTelemetryDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED process.env.ALTIMATE_TELEMETRY_DISABLED = "true" }) afterAll(() => { - delete process.env.ALTIMATE_TELEMETRY_DISABLED + // Restore any pre-existing value rather than unconditionally deleting — + // an outer suite may have set it and expects to see its own value after + // this file runs. (cubic P2 round 3.) + if (_priorTelemetryDisabled === undefined) { + delete process.env.ALTIMATE_TELEMETRY_DISABLED + } else { + process.env.ALTIMATE_TELEMETRY_DISABLED = _priorTelemetryDisabled + } }) describe("v0.9.6 release: Dispatcher registration retry", () => { @@ -102,6 +111,35 @@ describe("v0.9.6 release: Dispatcher registration retry", () => { expect(attempts).toBe(1) }) + test("concurrent calls share ONE registration attempt on failure — all reject with the SAME error", async () => { + // Companion to the success case: assert dedup also holds when the + // hook fails. All N concurrent callers must reject with the same + // error object (proving they awaited the same in-flight promise), + // and the hook body must have run exactly once. (cubic P2 round 3.) + let attempts = 0 + let rejectGate: (err: Error) => void = () => {} + const gate = new Promise((_, rej) => (rejectGate = rej)) + Dispatcher.setRegistrationHook(async () => { + attempts += 1 + await gate + }) + + const calls = Array.from({ length: 20 }, () => Dispatcher.call("ping", {} as any)) + const failure = new Error("shared-failure") + rejectGate(failure) + + const results = await Promise.allSettled(calls) + expect(results.length).toBe(20) + for (const r of results) { + expect(r.status).toBe("rejected") + if (r.status === "rejected") { + // Same error instance = same underlying promise = dedup held. + expect(r.reason).toBe(failure) + } + } + expect(attempts).toBe(1) + }) + test("successful registration is memoized — a later call does NOT re-run the hook", async () => { // Guard against overcorrection: once the current-generation hook has // completed, the resolved _registrationPromise memoizes success so