fix(sdk): address CodeRabbit runtime bridge and docs findings

This commit is contained in:
Tom Boucher
2026-05-05 19:59:56 -04:00
parent fb58731008
commit 8ad2e3877f
7 changed files with 66 additions and 15 deletions

View File

@@ -138,3 +138,44 @@ All workflow file names use hyphens; `<step name="...">` attributes inside those
### "Follow the X workflow" prose fragments are non-standard — use "Execute end-to-end."
After stripping prose @-refs, some command `<process>` blocks retained bolded "**Follow the X workflow**" fragments. ADR-0002 standard is `Execute end-to-end.` for single-workflow commands. Routing commands with flag dispatch use `execute the X workflow end-to-end.` in routing bullets (no bold, no redundant path).
---
## Recurring CodeRabbit review patterns (2026-05-05, PRs #3152/#3154/#3155)
### Changeset metadata drift (`pr:` points at issue instead of PR)
- In `.changeset/*.md`, reviewers repeatedly flag `pr:` values that accidentally reference issue ids.
- **Rule**: `pr:` must equal the GitHub PR number carrying the change.
- **Pre-flight check**: before push, verify each new changeset file against current branch PR number.
### Test diagnostics quality for command-output parsing
- Even when behavior is correct, CR requests clearer failure surfaces before `.map()` on parsed output.
- **Rule**: after `JSON.parse`, assert output object shape (e.g., `Array.isArray(output.phases)`) with raw-output-prefix diagnostics.
- This prevents opaque `TypeError` failures and shortens triage loops when CLI output shape changes.
### Merge gate discipline: CodeRabbit pass is necessary but not sufficient
- CI/checks can be green while unresolved review threads still block clean merge policy.
- **Rule**: always gate on all three together: required checks green, CodeRabbit pass, unresolved thread count = 0.
- Keep using GraphQL `reviewThreads` as authoritative unresolved state, not summary comments/check badge alone.
---
## SDK Runtime Bridge review synthesis (PR #3158, 2026-05-05)
### What we fixed
- Deepened one **SDK Runtime Bridge Module** seam (`sdk/src/query-runtime-bridge.ts`) for dispatch routing and observability.
- Replaced orphan event typing with a canonical union (`RuntimeBridgeEvent`).
- Made bridge observability non-intrusive: `onDispatchEvent` now runs behind a safe emitter so callback failures cannot alter dispatch outcomes.
- Corrected strict-mode event semantics: strict native-adapter rejection now reports `dispatchMode: 'native'` (no fake subprocess attempt).
- Preserved execution policy defaults by passing `allowFallbackToSubprocess` through as `undefined` when unset (no forced override in `GSDTools`).
- Fixed transport decision ordering: fallback-disabled guard now throws before emitting subprocess decision events.
- Added explicit invariant in `subprocessReason` for impossible states (fail loud on contract drift).
- Updated user-facing docs (`README.md`, `docs/CLI-TOOLS.md`, `docs/ARCHITECTURE.md`) and ADR narrative consistency.
### What we should not do again
- Do not let observability callbacks sit on the critical path without isolation.
- Do not emit structured events that claim a transport mode that never happened.
- Do not force option defaults at call sites when policy Modules already define defaults.
- Do not keep duplicate/inert exported types; expose one canonical union Interface.
- Do not emit decision events before guard checks that may reject the path.
- Do not leave architectural docs with ambiguous seam ownership between CLI and SDK paths.

View File

@@ -215,6 +215,7 @@ For the full configuration reference — all settings, git branching strategies,
| [Commands](docs/COMMANDS.md) | Every command with flags and examples |
| [Configuration](docs/CONFIGURATION.md) | Full config schema, model profiles, git branching |
| [Architecture](docs/ARCHITECTURE.md) | How the multi-agent orchestration works |
| [CLI Tools](docs/CLI-TOOLS.md) | `gsd-sdk query` and programmatic SDK dispatch seams |
| [Features](docs/FEATURES.md) | Complete feature index |
| [Changelog](CHANGELOG.md) | What changed in each release |

View File

@@ -55,7 +55,7 @@ GSD is a **meta-prompting framework** that sits between the user and AI coding a
┌──────▼──────────────▼─────────────────▼──────────────┐
│ CLI TOOLS LAYER │
│ gsd-sdk query (sdk/src/query) + gsd-tools.cjs │
│ SDK Runtime Bridge Module routes native vs fallback │
│ Programmatic SDK bridge: GSDTools/query-runtime-bridge.ts │
└──────────────────────┬───────────────────────────────┘
│
┌──────────────────────▼───────────────────────────────┐

View File

@@ -62,7 +62,7 @@ Use this when authoring workflows, not when you only need the command list below
| `node gsd-tools.cjs roadmap analyze` | `gsd-sdk query roadmap analyze` |
**SDK state reads:** `gsd-sdk query state json` / `state.json` and `gsd-sdk query state load` / `state.load` currently share one native handler (rebuilt STATE.md frontmatter — CJS `cmdStateJson`). The legacy CJS `state load` payload (`config`, `state_raw`, existence flags) is still **CLI-only** via `node …/gsd-tools.cjs state load` until a separate registry handler exists. Full routing and golden rules: [QUERY-HANDLERS.md](../sdk/src/query/QUERY-HANDLERS.md).
**SDK state reads:** `state.json` and `state.load` are both registered query handlers with parity coverage. You can invoke them through `gsd-sdk query …` and through the SDK Runtime Bridge (`GSDTools` → `sdk/src/query-runtime-bridge.ts`), honoring `allowFallbackToSubprocess` / `strictSdk` and emitting `onDispatchEvent` observability. For direct typed dispatch, use `createRegistry()` from `sdk/src/query/index.ts`. Full routing and golden rules: [QUERY-HANDLERS.md](../sdk/src/query/QUERY-HANDLERS.md).
**CLI-only (not in registry):** e.g. **graphify**, **from-gsd2** / **gsd2-import** — call `gsd-tools.cjs` until registered.

View File

@@ -78,7 +78,7 @@ export class GSDTools {
execJsonFallback: (legacyCommand, legacyArgs) => this.exec(legacyCommand, legacyArgs),
execRawFallback: (legacyCommand, legacyArgs) => this.execRaw(legacyCommand, legacyArgs),
strictSdk: opts.strictSdk,
allowFallbackToSubprocess: opts.allowFallbackToSubprocess ?? false,
allowFallbackToSubprocess: opts.allowFallbackToSubprocess,
onDispatchEvent: opts.onDispatchEvent,
});

View File

@@ -54,7 +54,6 @@ export class GSDTransport {
}
} else {
const reason = this.subprocessReason(request, policy);
onDecision?.({ dispatchMode: 'subprocess', reason });
if (!policy.allowFallbackToSubprocess && reason === 'native_unregistered') {
throw GSDToolsError.failure(
`Subprocess fallback disabled: command '${request.registryCommand}' cannot run without native dispatch`,
@@ -63,6 +62,7 @@ export class GSDTransport {
null,
);
}
onDecision?.({ dispatchMode: 'subprocess', reason });
}
return this.dispatchSubprocess(request);
@@ -77,7 +77,10 @@ export class GSDTransport {
if (request.workstream) return 'workstream_forced';
if (!policy.preferNative) return 'native_not_preferred';
if (!this.registry.has(request.registryCommand)) return 'native_unregistered';
return 'native_not_preferred';
throw new Error(
`Unexpected subprocess reason state for command '${request.registryCommand}' with preferNative=${String(policy.preferNative)} and workstream=${String(request.workstream)}`,
);
}
private shouldRethrowNativeError(error: unknown, policy: TransportPolicyLike): boolean {

View File

@@ -40,14 +40,12 @@ export interface RuntimeBridgeHotpathEvent {
errorKind?: 'timeout' | 'failure';
}
export interface RuntimeBridgeEvent {
type: 'query_dispatch' | 'query_hotpath_dispatch';
}
export type RuntimeBridgeEvent = RuntimeBridgeDispatchEvent | RuntimeBridgeHotpathEvent;
export interface RuntimeBridgeOptions {
strictSdk?: boolean;
allowFallbackToSubprocess?: boolean;
onDispatchEvent?: (event: RuntimeBridgeDispatchEvent | RuntimeBridgeHotpathEvent) => void;
onDispatchEvent?: (event: RuntimeBridgeEvent) => void;
}
/**
@@ -71,6 +69,14 @@ export class QueryRuntimeBridge {
return resolveQueryCommand(command, args, this.registry);
}
private emit(event: RuntimeBridgeEvent): void {
try {
this.options?.onDispatchEvent?.(event);
} catch {
// Observability must never break dispatch behavior.
}
}
async execute(input: RuntimeBridgeExecuteInput): Promise<unknown> {
const startedAt = Date.now();
if (this.options?.strictSdk && !this.registry.has(input.registryCommand)) {
@@ -80,12 +86,12 @@ export class QueryRuntimeBridge {
input.legacyArgs,
null,
);
this.options?.onDispatchEvent?.({
this.emit({
type: 'query_dispatch',
command: input.registryCommand,
legacyCommand: input.legacyCommand,
mode: input.mode,
dispatchMode: 'subprocess',
dispatchMode: 'native',
reason: 'native_unregistered',
durationMs: Date.now() - startedAt,
outcome: 'error',
@@ -111,7 +117,7 @@ export class QueryRuntimeBridge {
},
});
this.options?.onDispatchEvent?.({
this.emit({
type: 'query_dispatch',
command: input.registryCommand,
legacyCommand: input.legacyCommand,
@@ -124,7 +130,7 @@ export class QueryRuntimeBridge {
return result;
} catch (error) {
const kind = error instanceof GSDToolsError ? error.classification.kind : 'failure';
this.options?.onDispatchEvent?.({
this.emit({
type: 'query_dispatch',
command: input.registryCommand,
legacyCommand: input.legacyCommand,
@@ -155,7 +161,7 @@ export class QueryRuntimeBridge {
registryArgs,
mode,
);
this.options?.onDispatchEvent?.({
this.emit({
type: 'query_hotpath_dispatch',
command: registryCommand,
legacyCommand,
@@ -167,7 +173,7 @@ export class QueryRuntimeBridge {
return result;
} catch (error) {
const kind = error instanceof GSDToolsError ? error.classification.kind : 'failure';
this.options?.onDispatchEvent?.({
this.emit({
type: 'query_hotpath_dispatch',
command: registryCommand,
legacyCommand,