feat(web): add exec hook task and batched apidocs ingestion#104
feat(web): add exec hook task and batched apidocs ingestion#104PrzemyslawKlys merged 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Pull Request ReviewThis PR adds two significant extensibility features to PowerForge's web pipeline system: exec task and batched apidocs. Overall, this is a well-implemented feature addition with good test coverage and documentation. Strengths
Observations and Suggestions1. Process Handling (Low Priority)According to .NET docs, when using asynchronous output/error redirection, you should call the parameterless WaitForExit() after the timed version to ensure all output is flushed. This prevents a subtle race condition that could cause output truncation. Current code (line 85): Suggested: var finished = process.WaitForExit(Math.Max(1, timeoutSeconds) * 1000);
if (finished) {
process.WaitForExit(); // Ensure async output/error streams complete
}2. Security Documentation (Low Priority)Consider adding a note in documentation that pipeline JSON files should be treated as trusted code (similar to Makefiles, Dockerfiles). The exec task is intentionally powerful as an extensibility hook. The implementation is already secure: UseShellExecute = false prevents shell injection, and ArgumentList.Add() is used appropriately. 3. Test Coverage (Low Priority)Consider adding tests for:
Security Review
Final VerdictThis PR is ready to merge with minor optional improvements noted above. The implementation is:
The WaitForExit pattern suggestion is the only technical improvement worth considering before merge, but it is a subtle edge case. Great work on this feature! Review by Claude Code |
Summary
execpipeline task as a stable extensibility hook for custom tooling/generatorsapidocsstepinputs/entriesbatches (CSharp + PowerShell)execis non-cacheableapidocsbatch outputs are discovered from nested inputsWebPipelineRunnerExecTaskTestsWebPipelineRunnerApiDocsBatchTestsWhy
Validation
dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~WebPipelineRunnerExecTaskTests|FullyQualifiedName~WebPipelineRunnerApiDocsBatchTests"dotnet test .\PSPublishModule.sln -c Release