Skip to content

fix: preserve nested command failure diagnostics - #3089

Merged
thymikee merged 1 commit into
callstack:mainfrom
samwize:codex/preserve-stderr
Oct 1, 2026
Merged

thymikee merged 1 commit into
callstack:mainfrom
samwize:codex/preserve-stderr

Conversation

@samwize

@samwize samwize commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Preserve diagnostic stderr beyond the generic 400-character limit so nested command failure reasons remain visible in JSON errors and request diagnostics.
  • Keep secret redaction before truncation. Retain up to 8,192 characters, keeping the beginning and end when longer. Other diagnostic fields keep their existing limits.
  • Scope: 4 files covering the redactor, synthetic regression tests, repeated error normalization, and debugging documentation. No command execution or retry behavior changes.

Validation

  • Commit: 82cacfc2112697f6e57f43dc2c526650d2c8a1ee.
  • Focused kernel redaction and error-normalization suites: 14 tests passed.
  • pnpm check:affected --run: all runnable checks passed, including 11,426 tests across 1,336 files, formatting, lint, typechecking, layering, Fallow, and build.
  • Regression evidence from the original local patch: the pre-fix implementation failed 5 focused cases; the fixed implementation passed. Fixtures contain synthetic diagnostics only.
  • GitHub CI and coverage remain pending. No live-device run is claimed for this diagnostic-only change; it preserves evidence, not repairs the underlying command failure.

Review in cubic

- Retain redacted stderr up to 8192 characters and preserve both ends beyond the limit.
- Cover truncation, redaction and repeated normalization without expanding other diagnostic fields.
@thymikee

thymikee commented Oct 1, 2026

Copy link
Copy Markdown
Member

This PR is ready on code at 82cacfc. Stderr now keeps its head and tail in nested command failures, and I found nothing that blocks it.

Not blocking, take or leave: (1) redactScopeData truncates before it replaces registered literals (packages/host-kit/src/internal/diagnostics.ts#L270), so a literal that straddles a cut can leave a fragment in the request log. Running replaceSensitiveValues before redactDiagnosticData would make truncation the last step on every route. (2) SENSITIVE_ASSIGNMENT_RE in packages/kernel/src/redaction.ts#L6 misses authorization, cookie, access key and private key, which SENSITIVE_KEY_RE covers, so an "Authorization: Basic ..." or "Cookie: ..." line in stderr is not redacted. Up to 8192 chars can now show, so building both regexes from one key list and adding one stderr test would close it. (3) If the head cut falls between a secret key assignment and its "[REDACTED]", a second pass can replace the truncation marker (packages/kernel/src/redaction.ts#L73). A marker that starts with a character the value pattern cannot match would avoid it. (4) The stderr carve-out applies to all six redactDiagnosticData callers, but the docs only describe the JSON errors view (website/docs/docs/debugging-profiling.md#L84). One sentence saying request logs and session traces share the same budget would help.

I did not run any tests. The pre-fix failure claims and the Authorization and Cookie gaps come from reading the code, not from running it. CI is still pending. This change touches the shared redactor, so Integration, Coverage and Smoke all run it, and any test that expects the old 400-char stderr cut would fail there because of this PR. Before merge, the required checks (Integration Tests, Coverage, Smoke Tests, Typecheck & Package, Repo Guards, command-docs-gate) must pass on 82cacfc.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 1, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/kernel/src/redaction.ts">

<violation number="1" location="packages/kernel/src/redaction.ts:75">
P3: The new head/tail truncation can split an emoji at either retained boundary, so JSON consumers receive an unpaired surrogate and the diagnostic character is corrupted. Trim each retained boundary when it cuts a surrogate pair before concatenating the marker.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

const marker = `\n${TRUNCATION_SUFFIX}\n`;
const retainedLength = REDACTED_STDERR_MAX_LENGTH - marker.length;
const headLength = Math.ceil(retainedLength / 2);
return `${value.slice(0, headLength)}${marker}${value.slice(-(retainedLength - headLength))}`;

@cubic-dev-ai cubic-dev-ai Bot Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The new head/tail truncation can split an emoji at either retained boundary, so JSON consumers receive an unpaired surrogate and the diagnostic character is corrupted. Trim each retained boundary when it cuts a surrogate pair before concatenating the marker.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/kernel/src/redaction.ts, line 75:

<comment>The new head/tail truncation can split an emoji at either retained boundary, so JSON consumers receive an unpaired surrogate and the diagnostic character is corrupted. Trim each retained boundary when it cuts a surrogate pair before concatenating the marker.</comment>

<file context>
@@ -62,10 +63,17 @@ function redactString(value: string, keyHint?: string): string {
+    const marker = `\n${TRUNCATION_SUFFIX}\n`;
+    const retainedLength = REDACTED_STDERR_MAX_LENGTH - marker.length;
+    const headLength = Math.ceil(retainedLength / 2);
+    return `${value.slice(0, headLength)}${marker}${value.slice(-(retainedLength - headLength))}`;
+  }
   if (value.length <= REDACTED_STRING_MAX_LENGTH) return value;
</file context>
Suggested change
return `${value.slice(0, headLength)}${marker}${value.slice(-(retainedLength - headLength))}`;
const head = value.slice(0, headLength).replace(/[\uD800-\uDBFF]$/, '');
const tail = value.slice(-(retainedLength - headLength)).replace(/^[\uDC00-\uDFFF]/, '');
return `${head}${marker}${tail}`;
Fix with cubic

@samwize

samwize commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Real-world example motivating this change: an iOS app launch failed, and open --debug --json returned this in error.details.stderr:

ERROR: The application failed to launch. (com.apple.dt.CoreDeviceError error 10002 (0x2712))
       BundleIdentifier = <redacted-app-id>
       ----------------------------------------
           The request to open "<redacted-app-id>" failed. (FBSOpenApplicationServiceErrorDomain error 1 (0x01))
           BSErrorCodeDescription = RequestDenied
           NSLocalizedFailureR...<truncated>

The original field was exactly 400 characters, including the literal ...<truncated> marker. The app identifier is replaced above, so this sanitized excerpt has a different length. Device identifiers, local paths, session metadata and diagnostic IDs are omitted.

The output stopped partway through the failure-reason field name, before its value. A second app launch also produced exactly 400 characters, ending with FBSOpenApplicationReq...<truncated>. The pre-fix diagnostic redactor explicitly applies that cap and marker.

This confirms lost diagnostic output, not the underlying launch failure's cause. The failure did not recur after the change, so there is no live before/after comparison of the missing text. Preservation and secret redaction are covered by the synthetic regression tests; the change does not claim to fix app launching.

@thymikee
thymikee merged commit 556e6e1 into callstack:main Oct 1, 2026
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants