feat(gstack2): unified execution-result contract#9
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
We've triggered an ultrareview automatically — This PR introduces a new execution-result contract and validation logic for external commands, changing how tool outputs are captured and reported — a subtle bug here could silently present incomplete or failed commands as success, breaking the effect flow across all six skill packages.. I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 11m 39s
7 issues found across 19 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/gstack2/generate-skill-tree.ts">
<violation number="1" location="scripts/gstack2/generate-skill-tree.ts:518">
P2: Browser setup can be incorrectly blocked after a successful `runtime-bootstrap ... options` or `preview` call: this rule says every optional-runtime result must validate as an execution-result envelope, but those commands return their existing `{ ok, action, ... }` payloads and therefore fail that validator as malformed. The implementation currently applies the envelope at the `gstack state effect` boundary, so scoping this instruction to that command (until the other boundaries migrate) keeps the generated contract consistent with actual runtime output.</violation>
</file>
<file name="runtime/cli.js">
<violation number="1" location="runtime/cli.js:445">
P2: External command output can be corrupted when a UTF-8 character spans two pipe chunks. Decoding each chunk separately turns a valid character such as `é` into replacement characters before it is saved in `data.stdout`/`data.stderr`, so consumers no longer receive the command's actual evidence. Buffer the raw bytes and decode once after the stream ends, or use a `StringDecoder` per stream and flush it on close.</violation>
<violation number="2" location="runtime/cli.js:468">
P2: Empty and oversized results are classified internally but that classification is lost before callers see it: `runExternalEffect` rewrites the rejection to `EXTERNAL_EFFECT_UNCERTAIN`. This prevents an automation client from consuming the advertised `EXECUTION_EMPTY`/`EXECUTION_DEGRADED` outcome or its stable code. Preserve and render a validated non-success execution result (while keeping the effect state uncertain) when propagating these boundary failures.</violation>
</file>
<file name="runtime/execution-result.js">
<violation number="1" location="runtime/execution-result.js:19">
P3: The exported schema is still mutable below its root despite being presented as frozen. A consumer can rewrite nested constraints such as the success `evidence.minItems` rule, which makes the in-memory contract diverge from the intended immutable shared definition. Recursively freezing the schema tree would make the exported contract actually immutable.</violation>
<violation number="2" location="runtime/execution-result.js:30">
P3: Schema-only consumers can classify whitespace-only evidence as an evidenced success, while the runtime validator rejects it as malformed. `minLength` and `minItems` do not require any non-whitespace character, so the shared schema no longer matches the validator's `trim()` rule. Adding a non-whitespace pattern to both textual fields keeps externally validated envelopes consistent with runtime behavior.</violation>
<violation number="3" location="runtime/execution-result.js:58">
P2: JSON rendering can emit a success envelope that violates the published required `data` field. The validator accepts `data: undefined` (or a function) and returns it here, but `JSON.stringify` subsequently omits that field; a `BigInt` instead makes rendering throw. Validating that `data` serializes to a defined JSON value and returning `EXECUTION_MALFORMED` otherwise would keep accepted results renderable and schema-conformant.</violation>
<violation number="4" location="runtime/execution-result.js:64">
P3: Malformed builder input loses the stable error-code contract: `executionResult(null)` and `executionResult(undefined)` throw an uncoded native `TypeError` at this dereference instead of `EXECUTION_MALFORMED`. Safely reading the builder fields (or guarding that `input` is an object) lets the existing validator report the malformed input consistently.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
| The package/runtime compatibility tuple is \`schemaVersion=1\`, \`runtimeVersion=2.0.0\`, and \`skillApi=2.0\`; the machine-readable copy is \`references/support/runtime-contract.json\`. An incompatible active runtime is unavailable, not permission to upgrade it. | ||
|
|
||
| Every optional-runtime tool result must satisfy \`references/support/execution-result-contract.json\` before it is presented as success. Success requires non-empty evidence. Empty or malformed output and explicit degraded, unsupported, or failed statuses remain non-success with stable codes; human renderings must preserve that status and code. |
There was a problem hiding this comment.
P2: Browser setup can be incorrectly blocked after a successful runtime-bootstrap ... options or preview call: this rule says every optional-runtime result must validate as an execution-result envelope, but those commands return their existing { ok, action, ... } payloads and therefore fail that validator as malformed. The implementation currently applies the envelope at the gstack state effect boundary, so scoping this instruction to that command (until the other boundaries migrate) keeps the generated contract consistent with actual runtime output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gstack2/generate-skill-tree.ts, line 518:
<comment>Browser setup can be incorrectly blocked after a successful `runtime-bootstrap ... options` or `preview` call: this rule says every optional-runtime result must validate as an execution-result envelope, but those commands return their existing `{ ok, action, ... }` payloads and therefore fail that validator as malformed. The implementation currently applies the envelope at the `gstack state effect` boundary, so scoping this instruction to that command (until the other boundaries migrate) keeps the generated contract consistent with actual runtime output.</comment>
<file context>
@@ -514,6 +515,8 @@ Some retained helpers are shell scripts. \`gstack doctor\` verifies Bash and, on
The package/runtime compatibility tuple is \`schemaVersion=1\`, \`runtimeVersion=2.0.0\`, and \`skillApi=2.0\`; the machine-readable copy is \`references/support/runtime-contract.json\`. An incompatible active runtime is unavailable, not permission to upgrade it.
+Every optional-runtime tool result must satisfy \`references/support/execution-result-contract.json\` before it is presented as success. Success requires non-empty evidence. Empty or malformed output and explicit degraded, unsupported, or failed statuses remain non-success with stable codes; human renderings must preserve that status and code.
+
The developer-only fallback is \`node references/support/runtime-bootstrap.mjs install --source <reviewed-checkout> --capability <name> [matching browser flags] --yes\`; show its trust warning and use it only when the user explicitly selects a checkout they reviewed. If the packaged bootstrap is unavailable, stop capability setup instead of guessing a checkout-relative command. Deferring installation records no consent and must not block pure judgment.
</file context>
| Every optional-runtime tool result must satisfy \`references/support/execution-result-contract.json\` before it is presented as success. Success requires non-empty evidence. Empty or malformed output and explicit degraded, unsupported, or failed statuses remain non-success with stable codes; human renderings must preserve that status and code. | |
| Results returned by \`gstack state effect\` must satisfy \`references/support/execution-result-contract.json\` before they are presented as success. Success requires non-empty evidence. Empty or malformed output and explicit degraded, unsupported, or failed statuses remain non-success with stable codes; human renderings must preserve that status and code. |
| const remaining = maxCapturedBytes - captured.bytes; | ||
| if (remaining <= 0) { captured.truncated = true; return; } | ||
| const buffer = Buffer.from(chunk); | ||
| captured[key] += buffer.subarray(0, remaining).toString(); |
There was a problem hiding this comment.
P2: External command output can be corrupted when a UTF-8 character spans two pipe chunks. Decoding each chunk separately turns a valid character such as é into replacement characters before it is saved in data.stdout/data.stderr, so consumers no longer receive the command's actual evidence. Buffer the raw bytes and decode once after the stream ends, or use a StringDecoder per stream and flush it on close.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/cli.js, line 445:
<comment>External command output can be corrupted when a UTF-8 character spans two pipe chunks. Decoding each chunk separately turns a valid character such as `é` into replacement characters before it is saved in `data.stdout`/`data.stderr`, so consumers no longer receive the command's actual evidence. Buffer the raw bytes and decode once after the stream ends, or use a `StringDecoder` per stream and flush it on close.</comment>
<file context>
@@ -431,12 +437,49 @@ async function runExternalCommand(command, { cwd, env, stdout, stderr }) {
+ const remaining = maxCapturedBytes - captured.bytes;
+ if (remaining <= 0) { captured.truncated = true; return; }
+ const buffer = Buffer.from(chunk);
+ captured[key] += buffer.subarray(0, remaining).toString();
+ captured.bytes += Math.min(buffer.length, remaining);
+ if (buffer.length > remaining) captured.truncated = true;
</file context>
| if (evidence.length === 0) { | ||
| reject(cliError( | ||
| `External command ${name} exited 0 but produced no output; verify its side effect before reconciling it as applied.`, | ||
| EXECUTION_RESULT_ERROR_CODES.EMPTY, |
There was a problem hiding this comment.
P2: Empty and oversized results are classified internally but that classification is lost before callers see it: runExternalEffect rewrites the rejection to EXTERNAL_EFFECT_UNCERTAIN. This prevents an automation client from consuming the advertised EXECUTION_EMPTY/EXECUTION_DEGRADED outcome or its stable code. Preserve and render a validated non-success execution result (while keeping the effect state uncertain) when propagating these boundary failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/cli.js, line 468:
<comment>Empty and oversized results are classified internally but that classification is lost before callers see it: `runExternalEffect` rewrites the rejection to `EXTERNAL_EFFECT_UNCERTAIN`. This prevents an automation client from consuming the advertised `EXECUTION_EMPTY`/`EXECUTION_DEGRADED` outcome or its stable code. Preserve and render a validated non-success execution result (while keeping the effect state uncertain) when propagating these boundary failures.</comment>
<file context>
@@ -431,12 +437,49 @@ async function runExternalCommand(command, { cwd, env, stdout, stderr }) {
+ if (evidence.length === 0) {
+ reject(cliError(
+ `External command ${name} exited 0 but produced no output; verify its side effect before reconciling it as applied.`,
+ EXECUTION_RESULT_ERROR_CODES.EMPTY,
+ ));
+ return;
</file context>
| } else if (typeof value.code !== "string" || !/^[A-Z][A-Z0-9_]{2,63}$/.test(value.code)) { | ||
| return invalid("Non-success execution requires a stable error code"); | ||
| } | ||
| return Object.freeze({ ...value, evidence: Object.freeze([...value.evidence]) }); |
There was a problem hiding this comment.
P2: JSON rendering can emit a success envelope that violates the published required data field. The validator accepts data: undefined (or a function) and returns it here, but JSON.stringify subsequently omits that field; a BigInt instead makes rendering throw. Validating that data serializes to a defined JSON value and returning EXECUTION_MALFORMED otherwise would keep accepted results renderable and schema-conformant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/execution-result.js, line 58:
<comment>JSON rendering can emit a success envelope that violates the published required `data` field. The validator accepts `data: undefined` (or a function) and returns it here, but `JSON.stringify` subsequently omits that field; a `BigInt` instead makes rendering throw. Validating that `data` serializes to a defined JSON value and returning `EXECUTION_MALFORMED` otherwise would keep accepted results renderable and schema-conformant.</comment>
<file context>
@@ -0,0 +1,89 @@
+ } else if (typeof value.code !== "string" || !/^[A-Z][A-Z0-9_]{2,63}$/.test(value.code)) {
+ return invalid("Non-success execution requires a stable error code");
+ }
+ return Object.freeze({ ...value, evidence: Object.freeze([...value.evidence]) });
+}
+
</file context>
| FAILED: "EXECUTION_FAILED", | ||
| }); | ||
|
|
||
| export const EXECUTION_RESULT_SCHEMA = Object.freeze({ |
There was a problem hiding this comment.
P3: The exported schema is still mutable below its root despite being presented as frozen. A consumer can rewrite nested constraints such as the success evidence.minItems rule, which makes the in-memory contract diverge from the intended immutable shared definition. Recursively freezing the schema tree would make the exported contract actually immutable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/execution-result.js, line 19:
<comment>The exported schema is still mutable below its root despite being presented as frozen. A consumer can rewrite nested constraints such as the success `evidence.minItems` rule, which makes the in-memory contract diverge from the intended immutable shared definition. Recursively freezing the schema tree would make the exported contract actually immutable.</comment>
<file context>
@@ -0,0 +1,89 @@
+ FAILED: "EXECUTION_FAILED",
+});
+
+export const EXECUTION_RESULT_SCHEMA = Object.freeze({
+ $schema: "https://json-schema.org/draft/2020-12/schema",
+ $id: "https://gstack.dev/schemas/execution-result-v1.json",
</file context>
| schemaVersion: { const: EXECUTION_RESULT_SCHEMA_VERSION }, | ||
| status: { enum: EXECUTION_RESULT_STATUSES }, | ||
| code: { type: ["string", "null"], pattern: "^[A-Z][A-Z0-9_]{2,63}$" }, | ||
| summary: { type: "string", minLength: 1 }, |
There was a problem hiding this comment.
P3: Schema-only consumers can classify whitespace-only evidence as an evidenced success, while the runtime validator rejects it as malformed. minLength and minItems do not require any non-whitespace character, so the shared schema no longer matches the validator's trim() rule. Adding a non-whitespace pattern to both textual fields keeps externally validated envelopes consistent with runtime behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/execution-result.js, line 30:
<comment>Schema-only consumers can classify whitespace-only evidence as an evidenced success, while the runtime validator rejects it as malformed. `minLength` and `minItems` do not require any non-whitespace character, so the shared schema no longer matches the validator's `trim()` rule. Adding a non-whitespace pattern to both textual fields keeps externally validated envelopes consistent with runtime behavior.</comment>
<file context>
@@ -0,0 +1,89 @@
+ schemaVersion: { const: EXECUTION_RESULT_SCHEMA_VERSION },
+ status: { enum: EXECUTION_RESULT_STATUSES },
+ code: { type: ["string", "null"], pattern: "^[A-Z][A-Z0-9_]{2,63}$" },
+ summary: { type: "string", minLength: 1 },
+ evidence: { type: "array", items: { type: "string", minLength: 1 } },
+ data: {},
</file context>
| status: input.status, | ||
| code: input.code ?? null, | ||
| summary: input.summary, | ||
| evidence: input.evidence ?? [], | ||
| data: input.data ?? null, |
There was a problem hiding this comment.
P3: Malformed builder input loses the stable error-code contract: executionResult(null) and executionResult(undefined) throw an uncoded native TypeError at this dereference instead of EXECUTION_MALFORMED. Safely reading the builder fields (or guarding that input is an object) lets the existing validator report the malformed input consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/execution-result.js, line 64:
<comment>Malformed builder input loses the stable error-code contract: `executionResult(null)` and `executionResult(undefined)` throw an uncoded native `TypeError` at this dereference instead of `EXECUTION_MALFORMED`. Safely reading the builder fields (or guarding that `input` is an object) lets the existing validator report the malformed input consistently.</comment>
<file context>
@@ -0,0 +1,89 @@
+export function executionResult(input) {
+ return validateExecutionResult({
+ schemaVersion: EXECUTION_RESULT_SCHEMA_VERSION,
+ status: input.status,
+ code: input.code ?? null,
+ summary: input.summary,
</file context>
| status: input.status, | |
| code: input.code ?? null, | |
| summary: input.summary, | |
| evidence: input.evidence ?? [], | |
| data: input.data ?? null, | |
| status: input?.status, | |
| code: input?.code ?? null, | |
| summary: input?.summary, | |
| evidence: input?.evidence ?? [], | |
| data: input?.data ?? null, |
|
Superseded by #10, which consolidates all GStack 2 runtime work (unified execution-result contract, execution profiles, capability readiness, workflow + runtime hardening) into one branch. This feature is folded in there. |
1 participant
Unified execution-result contract
Adds a shared GStack 2 execution-result contract so empty, malformed, degraded, unsupported, or failed tool output can no longer be presented as success. Integrated at the smallest high-leverage boundary (the optional runtime's external-effect path) without touching the six-skill public surface or any specialist judgment.
What's in it
runtime/execution-result.js— a frozen JSON Schema (draft 2020-12), four statuses (success/degraded/unsupported/failed), stable error codes (EXECUTION_EMPTY,EXECUTION_MALFORMED,EXECUTION_DEGRADED,EXECUTION_UNSUPPORTED,EXECUTION_FAILED), a validator, a builder, a dual JSON/human renderer, and anassertSuccessfulExecutionguard. Core rule: success requires non-empty evidence.runtime/cli.js—gstack state effectnow validates before rendering. An exit-zero command that produces no output becomesEXECUTION_EMPTY(uncertain, not success); output over the 1 MB capture limit becomesEXECUTION_DEGRADED; success emits a validated envelope carryingexitCode/executable/stdout/stderrindata.references/support/execution-result-contract.jsonin all six skill packages, plus a documented paragraph in eachRUNTIME.md(regenerated viabun run gen:gstack2, freshness gate passes).Compatibility
gstack state effectnow emits a clean execution-result envelope instead of its previous ad hoc{status, effectKey, idempotencyKey, result}wrapper. No skill reference or test consumes the dropped top-level keys; the equivalent data lives in the envelope'sdatafield. Child stdout/stderr are captured into the result rather than streamed live. Other runtime commands are unchanged.Verification
bun test test/gstack2-*.test.ts).git diff --check: clean.Remaining rollout
Additional optional-runtime tool boundaries can migrate to the shared validator incrementally; this PR covers the central external-effect path.
🤖 Generated with Claude Code
Summary by cubic
Adds a unified execution-result contract for GStack 2 and wires it into
gstack state effectso empty, malformed, degraded, or unsupported tool output can’t be reported as success. Standardizes JSON and human renderings and captures stdout/stderr for verifiable evidence.New Features
runtime/execution-result.jswith schema v1, statuses (success|degraded|unsupported|failed), stable error codes, validator, builder, renderer, andassertSuccessfulExecution.gstack state effectnow validates and emits the envelope viarenderExecutionResult(..., { json: true }).EXECUTION_EMPTY; oversized capture =>EXECUTION_DEGRADED.references/support/execution-result-contract.jsonand updatedRUNTIME.mdacross all six skills.Migration
gstack state effectnow outputs the validated envelope (withdata.exitCode,data.executable,data.stdout,data.stderr) instead of the old{status, effectKey, idempotencyKey, result}wrapper.Written for commit 2938cef. Summary will update on new commits.