Repository navigation
feat: capture lossless native process results - #67
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Result contract src/Process/ProcessResult.php, src/Process/ResultProcessRunner.php, docs/architecture.md |
Adds the readonly ProcessResult type and the ResultProcessRunner interface. The architecture document describes captured output, exit status, and retained behavior of existing runner methods. |
Native process capture src/Process/NativeProcessRunner.php, tests/Process/NativeProcessRunnerTest.php, .changelog/lossless-process-result.md |
Adds captureResult() to the native runner. Tests check exact output bytes and statuses, process settings, timeout validation, startup errors, and the existing capture() failure contract. The changelog records the addition. |
Priority:
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant NativeProcessRunner
participant Subprocess
Caller->>NativeProcessRunner: captureResult(arguments, directory, timeout)
NativeProcessRunner->>Subprocess: Run without streaming sink
Subprocess-->>NativeProcessRunner: Exit code, stdout, stderr
NativeProcessRunner-->>Caller: ProcessResult
Possibly related issues
- php-fast-forward/code-standards issue 10: This change captures exit codes and raw output, but does not add the structured ECS or Rector result contracts.
- php-fast-forward/dev-tools issue 248: This change captures exit codes and separate raw output streams, but does not add JSON rendering or command-level structured payloads.
Merge Risk: 
8b04f
-
No concrete merge-blocking issue is identified; the change is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: adding lossless native process-result capture. Description check The description directly explains the new ResultProcessRunner capability, ProcessResult data, preserved exception behavior, compatibility constraints, and out-of-scope work. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 4 files. (2 skipped: 2 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit hears the subprocess run
With stdout and stderr kept apart
The exit code hops back intact
No streaming sink receives the bytes
Old capture keeps its former way
And carrots mark a clean result at play
Comment @coderabbitai help to get the list of available commands.
Changelog✅ Contribution check passed. Head: Base: Contribution: 1 new valid fragment(s). Policy: ordinary.
|
|
@codex review Please review exact draft HEAD 8c2726d. This six-file library slice adds optional lossless process capture without changing ProcessRunner, provider overrides, commands or native tool parsers. Independent final review and own canonical composer check passed 127 tests/865 assertions; the complete hosted PHP 8.3/8.4/8.5 matrix, reusable check, Changelog and CodeQL succeeded. No prior Codex request or running marker exists for this HEAD. Draft remains pending real external gates; no merge, tag or release is requested. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@coderabbitai review Please review HEAD 9f58096. This draft has not received a CodeRabbit review. The current-head PHP 8.3–8.5 matrix, reusable check, Changelog, CodeQL and Codex code/security reviews all completed successfully; independent review and the local 128-test / 1,012-assertion aggregate cover the exact combined tree. This request is serialized after the previously reported capacity window; no other review request is being sent concurrently. |
✅ Action performed
|
|
@coderabbitai full review Please review the complete current PR at HEAD 8b04fa9 against main 6005f41. Existing coverage still identifies 9f58096, so it is not counted as current-head approval. The normal main reconciliations preserve the six feature files; current-head PHP 8.3–8.5, reusable check, Changelog, CodeQL and Codex code/security reviews completed successfully. The exact combined tree also passed local tests and independent review. No review was in progress at readback; this is the single serialized request after the observed capacity window. |
✅ Action performed
|
Refs #10. This implements the process-result foundation; structured ECS/Rector parsing and CLI JSON remain outside this slice, so the issue stays open.
Add the optional ResultProcessRunner capability and immutable ProcessResult to capture exact exit status, stdout and stderr separately, including binary, invalid UTF-8 and ANSI bytes. Nonzero exits remain data; startup, timeout and invalid-bound failures retain exceptions. No parsing, normalization, channel concatenation or clean-result inference.
Native captureResult() uses the existing factory, argument vector, directory, credential filtering and finite timeout without calling the streaming sink. Existing ProcessRunner implementations, provider overrides, capture(), run(), CLI commands and ECS worker protocol remain unchanged. No mandatory interface method is added to custom transports.
Current HEAD
8b04fa99fa0af33bc84706d8b56ee700a628aa1fnormally merges actual main6005f4100ac871e18faf8ca57dd508ab8349a45a. Independently reviewed treedd5d8b27bf0dc5273152a8a47cab21b04fc56700: six feature files are preserved; four incoming skill files exactly match main; the previously integrated Twig evaluation is preserved. Feature diff SHA256a46c62179c1ffa44cce28553beaf472b72f83e105f3349b0a4bab9611e26b117; signature valid, hooks unchanged.The exact combined local tree passed 128 tests / 1,026 assertions, 41-PHP syntax/style, root Composer strict/platform and Changelog checks. The candidate also passed 33 native Twig checks, nested strict/platform and official actionlint, using a separately qualified isolated 15-package graph whose installed versions/source refs exactly match the nested lock; this is graph reuse, not a claim that old project metadata is this HEAD. The root 53-package graph remains exact. Focused transport coverage includes exits 0/2/255, independent raw channels, argv/cwd, synthetic credential filtering/restoration, no sink writes, finite/invalid bounds, startup errors and legacy capture failure behavior.
Current-head hosted PHP 8.3–8.5, reusable-workflow, Changelog and CodeQL checks passed. PHP 8.5 executed synthetic merge 1a6845d, whose tree exactly matches the reviewed tree above: 128 tests / 1,022 assertions, style and 33 native Twig checks passed. Codex code and security reviews completed without findings on 8b04fa9. CodeRabbit full review f5fe184e-7384-4f22-90c3-1c802ef34c40 completed on 8b04fa9 without actionable findings; exact source/covered SHA and successful check were verified before merge. Merged normally as 26152a0 on 2026-10-27 at 12:46:44 UTC. The merge tree matches the reviewed candidate exactly; main Quality40506464641, Changelog40506465995 and Push40506463574 all completed successfully. Issue #10 stays open for structured results/CLI JSON. No consumer mutation, tag or release is claimed.