Skip to content

Expose command keys with safe human output and exact JSON values - #61

Draft
rmcdaniel wants to merge 8 commits into
mainfrom
claude/216-command-keys
Draft

rmcdaniel wants to merge 8 commits into
mainfrom
claude/216-command-keys

Conversation

@rmcdaniel

@rmcdaniel rmcdaniel commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Part of shared #216. Signal and update commands accept or generate a command key, retain it across uncertain responses, show receipt flags and expose retained-command conflicts. Keys follow the Server's 255 Unicode-character limit and preserve exact valid bytes.

Human output removes terminal controls and escapes Console markup in request IDs, errors and conflict details. Uncertain-outcome advice explains how to reuse the key without interpolating it into a shell command. JSON/JSONL writes bypass Console formatting so structured values remain exact. Invalid UTF-8 refuses before POST with a valid JSON error; local validation keeps its existing INVALID exit code.

Source qualification at fbede8f6d8066143cf38c7be6232884b55b0b6e4: the full suite completes 977 tests / 18,071 assertions with one live-Server smoke skip and 11 existing PHPUnit deprecations. Focused output/command coverage at parent d02b31e completes 254 tests / 1,961 assertions. The final helper-only refactor is covered by the repeated full suite. The original output matrix has 28 expected failures; the correction also fixes Console formatting of JSON/JSONL and preserves legacy validator exit codes. Main was integrated by a normal merge.

Keep this PR draft with auto-merge off while fresh hosted checks, source review and the remaining #216 qualification run. No version bump or retry-safe release is claimed. The published lossy-response CLI/SDK cohort, qualified backend receipt/intent/retention behavior, CHANGELOGs and all eight documentation locales remain acceptance work in #216.

rmcdaniel and others added 3 commits October 20, 2026 17:08
Part of durable-workflow/.github#216 (safe signal/update retries after a lost response). Work in progress; see the draft PR body for the remaining steps.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@rmcdaniel

Copy link
Copy Markdown
Member Author

Exact-head review of 6ebce456748bd3a7872df91db44a881f7ab09adf found a human-output injection boundary in the new request-key rendering.

--request-id accepts arbitrary valid UTF-8, including newlines, terminal control sequences, Symfony console markup and shell metacharacters. The new paths write that value directly with writeln() and interpolate it unquoted into the retry suggestion --request-id=%s.

A synthetic PHP 8.4 command harness against this head confirmed, without printing the hazardous bytes themselves:

  • a newline in the key reaches human output and creates a second forged-looking status line;
  • an OSC terminal escape reaches human output unchanged;
  • a key containing shell syntax appears verbatim and unquoted in the suggested retry fragment.

This is not evidence that the CLI executed the shell syntax, and the draft is unreleased. It is a terminal/log spoofing and unsafe copy-paste guidance defect. The same repository already has an appropriate pattern in CancellationCascadeRenderer: replace C0/C1 terminal controls and pass text through OutputFormatter::escape().

Please sanitize and formatter-escape request IDs and retained-receipt fields in every human-output path. Do not render an arbitrary key as an unquoted shell fragment; either provide platform-safe quoting or present the exact key separately without executable-looking syntax. Structured JSON should continue preserving the exact key bytes through JSON escaping. Add focused newline, ESC/OSC, console-markup and shell-metacharacter regressions for success, conflict and uncertain transport errors.

Minor documentation correction in the same slice: docs/cli-reference.md still says 1-255 bytes; the implemented and Server contract is 1-255 Unicode characters.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Focused correction boundary for the existing exact-head finding on 6ebce456748bd3a7872df91db44a881f7ab09adf:

Keep the wire and structured-output contract byte-exact. Apply terminal-safe rendering only at the human boundary:

  • sanitize C0/C1 controls and then OutputFormatter::escape() the request ID in writeCommandReceiptLines() and writeHumanErrorRequestIdContext();
  • do the same for retained_command_id, retained_command_type, retained_outcome, and each conflict part before requestIdConflictLines() reaches writeln();
  • ensure the request_id.conflict recommendation cannot reintroduce those raw fields through its interpolated message; and
  • remove the executable-looking --request-id=%s fragment from the uncertain-outcome recommendation. Formatter escaping is not shell quoting. Refer to the separately displayed request ID and direct operators to JSON mode for its exact encoded value instead of emitting a platform-ambiguous command fragment.

Do not sanitize the request body, request_id JSON field, server_response, or structured recommendation data; json_encode already represents controls without terminal execution and automation needs exact bytes.

Focused regressions should cover signal and update success, request-ID conflict, and uncertain transport failure with newline, ESC/OSC, console markup, quotes/whitespace and shell metacharacters. Human output must contain no raw terminal controls, forged line, active markup or executable-looking retry fragment; JSON must decode back to the exact original values. Also correct docs/cli-reference.md from 1–255 bytes to 1–255 Unicode characters. This refines the existing draft finding and adds no release gate beyond correcting it.

@rmcdaniel rmcdaniel changed the title Add --request-id to workflow:signal and show retained receipts Expose command keys with safe human output and exact JSON values Nov 2, 2026
@rmcdaniel

Copy link
Copy Markdown
Member Author

General pushed the corrected head d02b31ea0e0612148c5afde30a4e56fe6f9ccd5e to this existing PR. It includes the human-output correction, exact JSON/JSONL writes, valid JSON refusal for malformed UTF-8 keys and preserved local-validation exit codes, after a normal main merge.

The full local suite completes 977 tests / 18,071 assertions with one live-Server smoke skip and 11 existing PHPUnit deprecations. Focused command/output coverage completes 254 tests / 1,961 assertions. Controls include newline/C1/OSC/markup/shell characters, decorated and plain human output, signal/update success/conflict/uncertainty, exact wire/JSON values and pre-POST validation.

All 16 current-head hosted checks succeeded, including PHP 8.2/8.3/8.4/8.5, PHAR smoke, reproducible PHAR build, required qualification, Sonar and CodeQL. Build evidence.

The prior human-output finding needs source review on this corrected head. The PR remains draft with auto-merge off; this does not claim #216's backend or published lossy-response acceptance, and makes no release/version change.

@rmcdaniel

Copy link
Copy Markdown
Member Author

The three Sonar maintainability findings in BaseCommand.php are corrected at fbede8f6d8066143cf38c7be6232884b55b0b6e4: extract HTTP body formatting, consolidate optional request-ID lookup, and use ordered match (true) exception recommendations. The output strings, sanitization boundary, JSON values and exception precedence are unchanged.

Repeated full suite: 977 tests / 18,071 assertions, one existing live-Server smoke skip, 11 existing PHPUnit deprecations; PHP 8.3.36, PHPunit 11.5.55, Docker task sandbox. PHP syntax and diff checks pass. Fresh hosted analysis is pending; no threshold or finding was suppressed. This remains draft with auto-merge off until the broader #216 acceptance work qualifies.

@sonarqubecloud

sonarqubecloud Bot commented Nov 2, 2026

Copy link
Copy Markdown

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security focused rereview accepts current exact head fbede8f6d8066143cf38c7be6232884b55b0b6e4; the human-output injection finding is resolved.

Reviewed 6ebce456..fbede8f6, with source corrections in ebe8b89f, 98ccea86, d02b31ea, and the maintainability-only fbede8f6 delta:

  • every request-key, retained-command, conflict, error-message, reason, validation and recommendation value that reaches human output now passes through C0/C1 replacement plus OutputFormatter::escape();
  • the uncertain-outcome human recommendation no longer interpolates the key into executable-looking shell syntax. It points to the separately rendered safe key and JSON mode for an exact copy;
  • wire bodies and structured JSON/JSONL retain exact opaque values. OUTPUT_RAW prevents the console formatter from interpreting encoded JSON as markup;
  • malformed UTF-8 is refused before POST while producing valid JSON, and a valid key on a separate local-validator failure retains exact JSON context and the original invalid-input exit code;
  • the final helper extraction and ordered match (true) preserve the prior exception/recommendation precedence and do not change the sanitization, JSON or wire paths.

The regression matrix covers signal/update success, conflict and uncertain transport paths; C0/C1/newline/OSC, formatter markup and shell characters; decorated/plain human output; and exact structured round trips. git diff --check passes. All 16 current-head hosted checks are green, including PHP 8.2–8.5, PHAR smoke/reproducibility, Sonar and CodeQL.

This clears only the output finding. The PR correctly remains draft/auto-merge off until the broader #216 native #934 contract and published lost-response/concurrent-cohort acceptance are complete. No Security release claim or additional hold.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant