Repository navigation
feat(fill): read text from stdin with --text-stdin - #3351
Conversation
fill can take its text from stdin instead of argv so a secret never appears in the CLI's process arguments. The read is bounded to 64 KiB and strips exactly one trailing LF or CRLF; a text argument, a terminal, empty input and invalid UTF-8 are refused with typed reasons that never echo the input. The value goes through the existing sensitivity contract: it is registered as a diagnostics secret in the CLI and daemon scopes, and the daemon keeps it out of recorded actions unless --record-as parameterizes it. While recording is armed the flag needs --record-as or --no-record. Batch steps, MCP tools and env/config defaults cannot set it. Closes callstack#3260
There was a problem hiding this comment.
All reported issues were addressed across 25 files
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
- Mask target parse failures under --text-stdin: a stray positional may be the secret, and the generic target error echoed it. - Keep a leading byte order mark and refuse lone surrogates in string chunks instead of replacing them. - Only refuse batch steps that set textStdin to true. - Clarify help and docs: passing neither --record-as nor --no-record while recording is an error, invalid UTF-8 is refused, and printf %s is the verbatim way to pipe a value. - Tighten tests to assert each case's own input is never echoed.
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
- A string stream may split a surrogate pair across chunks, and the per-chunk lone-surrogate check refused valid input. A trailing high surrogate is now held back until the next string chunk; a byte chunk or the end of input still refuses it as invalid UTF-8. - Size a string chunk with Buffer.byteLength before encoding it, so an oversized chunk is refused without being copied first.
4e7597c to
0964704
Compare
|
Would it be simpler to treat Not blocking, and you can take or leave it: the armed-recording refusal and the extended diagnostic-registration predicate are only tested by calling the helpers directly, so one router-level test that sends an armed Two bot threads on CI is green and there are no conflicts. This read of 0964704 is from the code only, and I did not run the CLI or the tests. The earlier live iOS run was at 9d2bc90 and did not show the |
A fill whose text is sensitive (--record-as set or --text-stdin) now decides it through one predicate, isSensitiveFillText, for the response payloads, the recorder skip and diagnostics registration. Without --record-as, the response and result text read [REDACTED] and value-carrying selector candidates are dropped, so an unrecorded or --no-record stdin fill no longer echoes the value in --json output or the client result. An error leaving the router now passes through the request's registered sensitive values, so a backend message that echoes the value after admission is redacted as well.
|
Thanks for the careful read. Addressed in 19d3ffd:
No part of the value is in stdout, and a follow-up One question: |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
- recordAs and textStdin mark fill text as sensitive, but the request boundary only checks that flags is an object. A direct daemon request with textStdin: "true" was read as unmarked, so the value came back in data.text; recordAs: 42 failed later as an UNKNOWN TypeError. Both are now refused as INVALID_ARGS before any device work. - Assert the --no-record response text is exactly [REDACTED].
|
The fixes from the earlier review at 0964704 are in at 68acc36, but one problem remains in the error path. The two earlier cubic-dev-ai threads are addressed at this commit: the malformed
Not blocking, and fine to take or leave: in CI is green, with the single reported check passing, and there are no conflicts. I read the code only and did not run the tests or a live replay. I also did not check whether nested batch steps that carry |
- The error exit replaced every value registered for the request across
the whole error. Maestro inputText and --record-as literals are
registered too, so a short value such as "1" rewrote logPath,
diagnosticId and the error of a non-fill replay. Only a fill marked
sensitive is redacted now, only its own text, and only in message,
hint, cause and details, with the same placeholder as the success
path (${VAR} with --record-as, [REDACTED] otherwise).
- Refuse textStdin on a daemon batch step: a failed step returns its
positionals, and the CLI already refuses it there.
- Remove redactRegisteredSensitiveValues from host-kit.
|
Thanks. Addressed in 2230e33, which also merges main and resolves the
Live check on an iOS 18.6 simulator (Settings), sending raw daemon requests, 68acc36 vs. this commit:
|
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
The earlier findings from 68acc36 are fixed in 2230e33, and CI is green, but two review points still need work before merge. There are no conflicts. I read the code only. I did not run the new router tests, a live device fill, or a replay. I also did not check whether the iOS runner, Android helper, or web backend echo the typed text in their errors; the router tests use a stub that throws a hand-written message. The replayed The The CLI guard at Not blocking, and you can take or leave it: After the replay redaction and the batch rule are in, rerun the checks on this PR. |
…in batch - A same-session replay runs its fill through the replay-scoped invoker, which registers the sensitive value but returned the fill's error as is. It now redacts that error like handleRequest does, so the replay message carries the fill's placeholder. - The daemon batch validator refuses only textStdin: true, matching the CLI guard; false is the ordinary argv path. - The error path uses the success path's parameterizeSensitiveString for values and keys, so a whitespace-only value collapses the string. - Docs: limit the redaction guarantee to value-bearing fields.
|
Thanks. Addressed in 14eafc4, which also merges main.
The three new router tests fail at 2230e33. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Thanks for the update. At 14eafc4, the scoped replay response is now redacted, the docs name the fields that are redacted, and the batch If a stdin secret contains the placeholder (for example The open Cubic P1 thread on this (#3351 (comment)) still stands. These threads are fixed at 14eafc4 and can be resolved: scoped replay redaction (#3351 (comment)), docs field list (#3351 (comment)), and the batch Both Smoke Tests jobs were still running when I checked, with no failure so far. The new changes only touch the error-redaction wrapper, the batch guard and one selectors export, so I expect little overlap with the smoke routes. I did not run the new router tests or a replay. I also did not check whether a real iOS, Android or web backend error echoes the typed text, since the tests use a stub error. Once the helper fix and its test are in and the checks finish green, this is ready for another look. |
…sponses
parameterizeSensitiveString split its input on the placeholder before
looking for the value, so a value that contains the placeholder
(`abc[REDACTED]xyz`, or `${VAR}` inside a `--record-as VAR` value) was
never found and came back whole in a backend error. The helper now
collapses the whole string to the placeholder whenever the value still
occurs outside a placeholder. A value found only inside placeholders
(`$` in `${DOLLAR}`) stays as before.
|
Thanks. Fixed in 6cdfbc7.
A real backend does echo the typed text: the iOS runner's One thing this does not cover: the |
|
This looks ready for maintainer review. At 6cdfbc7 both fixes from the earlier review are in: [REDACTED] inside a longer string now collapses, and the replay path wraps its response with the same redaction. The 16 checks are green, and the two doc and batch threads are also fixed at head. There are no conflicts. Not blocking, and you can take or leave it: the web stub in request-router-fill-text-stdin.test.ts at line 174 never echoes the typed text on success, so the I traced collapseIfLiteralRemains by hand and did not run the router test or interaction-common.test.ts. The live iOS simulator run has no attached artifact. The The four earlier inline threads from cubic-dev-ai no longer apply, so please resolve them: the collapse of abc[REDACTED]xyz is fixed in parameterized-recorded-fill.ts #3351 (comment), the replay invoker response now goes through redactSensitiveFillResponse #3351 (comment), the commands.md guarantee now covers success data and message/hint/cause/details #3351 (comment), and the batch refusal now matches projection.ts #3351 (comment). Nothing else stands in the way of maintainer merge review. |
…eString Covers a value that contains the placeholder, a value that crosses a placeholder already in the string, a value found only inside placeholders, and a plain replace that is stable on a second pass.
|
Thanks. Added You're right about the success half of the router test: the web stub never echoes, so only the error half guards the regression. The four cubic threads were already resolved when I checked. Opened #3392 for the read-back gap. Besides |
|
The new commit b508271 fixes the earlier concern, and I found no new problems in this change. The The four cubic-dev-ai threads are fixed at this head, so please resolve them: the replay redaction fix, the router error redaction, the docs wording for daemon-owned fields, and the batch check for Smoke Tests is still running and has not failed. The new commit only adds a unit test file in |
Summary
fillcan now take its text from stdin, so a secret never appears in the CLI's argv:\nor\r\n. A text argument, a TTY, empty input and invalid UTF-8 areINVALID_ARGSwith typed reasons (fill_text_source_conflict,fill_text_stdin_*); no message echoes the input.session.actionsunless--record-asparameterizes it. While recording is armed,--text-stdinneeds--record-asor--no-record(fill_text_stdin_unparameterized_recording). Happy to switch this to recording the step without its text if you prefer.25 files; scope stays within the
fillfamily. Closes #3260.Validation
Tested at 9d2bc90 (includes the review fixes):
pnpm check:affected --runpassed (all runnable checks; device and toolchain lanes are GitHub-authoritative). CI pending.Live run on an iPhone 16 simulator (iOS 18.6, Settings search field): stdin fill succeeded; an armed recording refused it without
--record-asand published${PASSWORD}with it. The value was absent from the daemon log, session events and the.adscript. It still appears in the native XCTest session log, which is #3261 and out of scope here.