feat(orchestrator): test workflow engine — suite definitions, taxonomy filters, structured results#790
Closed
frostebite wants to merge 5 commits intomainfrom
Closed
feat(orchestrator): test workflow engine — suite definitions, taxonomy filters, structured results#790frostebite wants to merge 5 commits intomainfrom
frostebite wants to merge 5 commits intomainfrom
Conversation
Initial scaffold for the test workflow engine service directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostebite
added
enhancement
📝 WalkthroughWalkthroughAdds a Test Workflow Engine: new action inputs and BuildParameters, a test-workflow module (types, parser, taxonomy filter, result reporter, orchestrator service), unit tests and README, and an early-exit path in the CLI to execute YAML test suites and emit structured results. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Action (index.ts)
participant Parser as TestSuiteParser
participant Orchestrator as TestWorkflowService
participant Unity as Unity Engine
participant Reporter as TestResultReporter
participant FS as Filesystem
CI->>Orchestrator: executeTestSuite(suitePath, parameters)
Orchestrator->>Parser: parseSuiteFile(suitePath)
Parser-->>Orchestrator: TestSuiteDefinition + run groups
Orchestrator->>Orchestrator: iterate groups (sequential)
Orchestrator->>Unity: spawn process with buildUnityArgs(run)
Unity-->>Reporter: emit test result (XML/JSON)
Reporter->>FS: writeResults(results, outputPath, format)
Reporter-->>Orchestrator: TestResult
Orchestrator-->>CI: aggregated results (failures count)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/model/orchestrator/services/test/README.md (1)
1-5: Consider expanding this scaffold README with a minimal contract section.Since this is the only in-repo doc for the new engine, adding a short “Current status / Not yet implemented / Planned inputs-outputs” section would prevent misuse and set expectations for draft behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test/README.md` around lines 1 - 5, Add a short "Contract" section under the "Test Workflow Engine" README (e.g., a header like "Current status / Not yet implemented / Planned inputs-outputs") that clearly lists the current implementation status, which features are unimplemented, and the minimal expected inputs and outputs (YAML test suite definition shape, taxonomy filter format, and the structure of test results). Keep it concise: one bullet for status, one for inputs (fields/types), one for outputs (fields/types), and one for planned extensions so consumers know what to rely on when using the draft engine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/test/README.md`:
- Line 5: Replace the ambiguous line "See GitHub Issue for full specification."
with a direct reference to the issue by appending the issue number or full URL
(for example replace that exact string with "See GitHub Issue `#788`" or "See
https://github.com/<owner>/<repo>/issues/788" as appropriate) so readers can
navigate directly to the spec; edit the README.md entry containing "See GitHub
Issue for full specification." to include that issue identifier.
---
Nitpick comments:
In `@src/model/orchestrator/services/test/README.md`:
- Around line 1-5: Add a short "Contract" section under the "Test Workflow
Engine" README (e.g., a header like "Current status / Not yet implemented /
Planned inputs-outputs") that clearly lists the current implementation status,
which features are unimplemented, and the minimal expected inputs and outputs
(YAML test suite definition shape, taxonomy filter format, and the structure of
test results). Keep it concise: one bullet for status, one for inputs
(fields/types), one for outputs (fields/types), and one for planned extensions
so consumers know what to rely on when using the draft engine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9736fe28-d54e-4745-9fa7-fc713706ef09
📒 Files selected for processing (1)
src/model/orchestrator/services/test/README.md
|
|
||
| Service for YAML-based test suite definitions, taxonomy filtering, and structured test results. | ||
|
|
||
| See GitHub Issue for full specification. |
There was a problem hiding this comment.
Add a direct issue reference/link.
“See GitHub Issue” is ambiguous without the issue number/URL, so readers can’t quickly navigate to the spec. Please link the exact issue (e.g., #788) in this line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test/README.md` at line 5, Replace the
ambiguous line "See GitHub Issue for full specification." with a direct
reference to the issue by appending the issue number or full URL (for example
replace that exact string with "See GitHub Issue `#788`" or "See
https://github.com/<owner>/<repo>/issues/788" as appropriate) so readers can
navigate directly to the spec; edit the README.md entry containing "See GitHub
Issue for full specification." to include that issue identifier.
This was referenced Mar 5, 2026
…omy filtering, and structured results (#790) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
src/model/orchestrator/services/test-workflow/test-suite-parser.ts (3)
87-95: UseTypeErrorfor type validation errors.Similar to the
needscheck, useTypeErrorwhen validating thatfiltersis a key-value object.♻️ Suggested fix
if (raw.filters !== undefined) { if (typeof raw.filters !== 'object' || Array.isArray(raw.filters)) { - throw new Error(`Run '${raw.name}': 'filters' must be a key-value object`); + throw new TypeError(`Run '${raw.name}': 'filters' must be a key-value object`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-suite-parser.ts` around lines 87 - 95, The validation for raw.filters currently throws a generic Error; change it to throw a TypeError to match the needs check style — i.e., where you detect that raw.filters is not an object or is an array, throw a TypeError with the same message (e.g., in the block handling raw.filters and assigning run.filters); keep the rest of the logic (initializing run.filters and copying entries) unchanged.
222-262: Consider usingundefinedinstead ofnullfor consistency.The project's ESLint configuration (unicorn/no-null) prefers
undefinedovernull. This affects the return type and several return statements indetectCircularDependencies.♻️ Suggested fix
- private static detectCircularDependencies(suite: TestSuiteDefinition): string | null { + private static detectCircularDependencies(suite: TestSuiteDefinition): string | undefined { const adjacency = new Map<string, string[]>(); for (const run of suite.runs) { adjacency.set(run.name, run.needs ?? []); } const visited = new Set<string>(); const visiting = new Set<string>(); - const dfs = (node: string, path: string[]): string | null => { + const dfs = (node: string, path: string[]): string | undefined => { if (visiting.has(node)) { const cycleStart = path.indexOf(node); const cycle = path.slice(cycleStart).concat(node); + return `Circular dependency: ${cycle.join(' -> ')}`; } if (visited.has(node)) { - return null; + return undefined; } // ... rest of function visiting.delete(node); visited.add(node); - return null; + + return undefined; }; for (const run of suite.runs) { const result = dfs(run.name, []); if (result) return result; } - return null; + return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-suite-parser.ts` around lines 222 - 262, The detectCircularDependencies function currently returns null in several places; change its return type to string | undefined and update the inner dfs function signature to return string | undefined, replacing all "return null" with "return undefined" (and any callers that explicitly compare to null to handle undefined or use falsy checks). Update the final return to return undefined instead of null and ensure any code that calls detectCircularDependencies (or checks dfs results) is adjusted accordingly to accept undefined.
64-69: UseTypeErrorfor type validation errors.Per ESLint unicorn/prefer-type-error, when throwing errors for type checks (checking if
needsis an array), useTypeErrorinstead of the genericError.♻️ Suggested fix
if (raw.needs !== undefined) { if (!Array.isArray(raw.needs)) { - throw new Error(`Run '${raw.name}': 'needs' must be an array of strings`); + throw new TypeError(`Run '${raw.name}': 'needs' must be an array of strings`); } run.needs = raw.needs; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-suite-parser.ts` around lines 64 - 69, The code in test-suite-parser.ts currently throws a generic Error when raw.needs fails the array type check; change that to throw a TypeError to satisfy the unicorn/prefer-type-error rule—replace the throw new Error(...) in the block that checks Array.isArray(raw.needs) with throw new TypeError(...) and keep the same message, leaving the surrounding logic that assigns run.needs = raw.needs intact.src/model/orchestrator/services/test-workflow/test-workflow-types.ts (1)
23-25: Use camelCase for the interface property name.The property
extensible_groupsuses snake_case which violates TypeScript naming conventions. Consider renaming toextensibleGroupsand handling the YAML field name mapping during parsing.♻️ Suggested fix
export interface TaxonomyDefinition { - extensible_groups: TaxonomyDimension[]; + extensibleGroups: TaxonomyDimension[]; }Then update the parser to map from
extensible_groupsin YAML toextensibleGroupsin the typed object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-workflow-types.ts` around lines 23 - 25, Rename the TaxonomyDefinition interface property extensible_groups to camelCase extensibleGroups and update any references to TaxonomyDefinition to use extensibleGroups; then modify the YAML parsing/mapping logic (where the YAML is deserialized into the typed object) to map the incoming extensible_groups field to extensibleGroups so external snake_case remains supported while the TypeScript model uses camelCase.src/model/orchestrator/services/test-workflow/test-workflow.test.ts (1)
173-220: Consider renaming the callback parameter for clarity.Per ESLint unicorn/prevent-abbreviations, the variable
ein the filter callbacks should be renamed toerrorfor clarity.♻️ Suggested fix
- expect(errors.some((e) => e.includes('Duplicate'))).toBe(true); + expect(errors.some((error) => error.includes('Duplicate'))).toBe(true);Apply similarly to lines 207 and 217.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-workflow.test.ts` around lines 173 - 220, Rename the short callback parameter `e` to `error` in the .some filter callbacks that inspect the `errors` array in the test cases for TestSuiteParser.validateSuite (the expectations that currently use errors.some((e) => ...)); update both occurrences checking 'Duplicate' and 'editMode' (and the one checking 'depends on itself' if present) so the callback reads errors.some((error) => error.includes(...)) to satisfy the unicorn/prevent-abbreviations rule and improve clarity.src/model/orchestrator/services/test-workflow/test-result-reporter.ts (1)
27-52: Regex-based XML parsing is fragile; consider an XML parser for production.The regex patterns assume well-formed XML with standard double-quoted attributes. Edge cases like escaped quotes inside attribute values, single-quoted attributes, or malformed XML could cause incorrect parsing.
For a scaffold this is acceptable, but consider using a lightweight XML parser (e.g.,
fast-xml-parser) before production release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-result-reporter.ts` around lines 27 - 52, Replace the fragile regex-based parsing in parseJUnitXml with a real XML parser (e.g., fast-xml-parser or DOMParser): parse xmlContent into an object, locate the testsuite element, and read its attributes to set runName, totalTests, failureCount, skippedCount, and duration (preserve existing fallback defaults). Ensure the parser handles both single- and double-quoted attributes and escaped characters, and keep the TestResult shape returned by parseJUnitXml unchanged for callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 21-36: When the test workflow path is taken
(buildParameters.testSuitePath and TestWorkflowService.executeTestSuite), the
function currently returns without setting CI outputs; call
Output.setBuildVersion and Output.setAndroidVersionCode with appropriate
sentinel or empty values and set Output.setEngineExitCode to 0 on all-passing
tests or a non-zero sentinel (e.g., 1) when totalFailed > 0 before returning, so
downstream steps always have defined outputs; update the code around the
TestWorkflowService.executeTestSuite block to compute totalFailed then set these
three outputs accordingly prior to the early return.
In `@src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts`:
- Line 30: The code copies TaxonomyFilterService.BUILT_IN_DIMENSIONS shallowly
causing mutations of inner objects when merging custom dimensions; in
loadTaxonomy, avoid mutating BUILT_IN_DIMENSIONS by deep-cloning its entries
before merging (e.g., create a new dimensions array from
TaxonomyFilterService.BUILT_IN_DIMENSIONS where each dimension object and its
values array are copied), and when merging custom dimensions replace
existing.values with a new array (or create a new merged object) instead of
calling existing.values.push(value) so built-in state is never mutated across
calls.
- Around line 160-169: The RegExp construction in the taxonomy filter block (the
trimmed.startsWith('/') && trimmed.endsWith('/') branch) uses untrusted input
and is vulnerable to ReDoS; replace unsafe RegExp usage by either: 1) switching
to a linear-time regex engine such as the re2 package (create an RE2 instance
instead of new RegExp for the pattern), or 2) if re2 cannot be introduced,
enforce strict runtime protections by (a) validating and bounding the pattern
length (reject patterns above a safe threshold), and (b) executing the regex
test off the main thread with a timeout (e.g., a Worker/worker_threads task that
returns false on timeout) to avoid blocking the event loop; update the catch
behavior to treat rejected/timeout cases as literal matches and surface a debug
log message for rejected patterns.
In `@src/model/orchestrator/services/test-workflow/test-result-reporter.ts`:
- Around line 278-282: The CDATA block written in TestResultReporter (where
lines.push is used to output failure.stackTrace) can be broken if
failure.stackTrace contains "]]>"; update the reporter to sanitize/split the
stack trace before wrapping in <![CDATA[...]]> by replacing every occurrence of
"]]>" with "]]]]><![CDATA[>" (or otherwise escape/split the terminator) so the
CDATA section remains valid; ensure this transformation is applied to the value
used in the lines.push call that currently writes
`<![CDATA[${failure.stackTrace}]]>` and keep using TestResultReporter.escapeXml
for other XML content as appropriate.
In `@src/model/orchestrator/services/test-workflow/test-suite-parser.ts`:
- Around line 64-69: The needs array is assigned without element type checks;
update the validation in the block that checks raw.needs (the code referencing
raw.name and assigning run.needs) to ensure every element is a string: iterate
over raw.needs, confirm typeof item === 'string' for each entry, and if any
element fails, throw a descriptive Error including the run name and the
offending index/value (e.g., "Run '...': 'needs' element at index X must be a
string"). Only assign run.needs = raw.needs after all elements pass validation.
In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts`:
- Around line 43-44: The group concurrency is being defeated because
TestWorkflowService.executeTestRun uses synchronous child process calls
(execSync) while callers use Promise.all; replace execSync inside executeTestRun
with an async non-blocking alternative (e.g., promisified child_process.exec or
a spawn wrapped in a Promise) so executeTestRun truly returns a Promise that
resolves/rejects based on the child process exit, capturing stdout/stderr, exit
code, and errors; update any related timeout/cleanup logic to use the async
handle and ensure callers using Promise.all can run runs in parallel.
- Around line 188-199: When both run.editMode and run.playMode are true, the
current branch only configures EditMode and silently skips PlayMode; update
test-workflow-service to perform two sequential Unity invocations instead: build
args for EditMode (as currently done) and call executeTestRun/executeTestSuite
for that run, then build args for PlayMode (with '-runTests' and '-testPlatform
PlayMode') and call executeTestRun/executeTestSuite again; ensure you
propagate/aggregate results or errors from both invocations (e.g., fail the
overall run if either invocation fails) and reference the existing run object
and functions executeTestRun and executeTestSuite when locating where to add the
second invocation and result handling.
- Around line 227-237: The path builder uses params.editorVersion directly and
can produce "undefined" in returned paths; validate params.editorVersion at the
start of the routine (the function that returns platform-specific Unity paths)
and either throw a clear error or substitute a sensible fallback before
constructing the paths. Specifically, check params.editorVersion for
undefined/empty and handle it prior to the win32/darwin/Linux returns so
Unity.exe/Unity.app/Unity path strings are never built with "undefined" — update
the function that contains the platform branch (references to
params.editorVersion and the three return statements for Windows, macOS, and
Linux) to perform this validation and return/raise accordingly.
- Around line 63-65: The code unsafely asserts params.testResultFormat when
calling TestResultReporter.writeResults; instead validate
params.testResultFormat against allowed values ('junit','json','both') and
map/normalize it to a safe union before calling writeResults. In the block
around resultFormat/resultPath, replace the assertion with a small validation:
read params.testResultFormat into resultFormat, if it matches one of the allowed
strings use it, otherwise set a sensible default (e.g., 'both'), then pass that
validated value to TestResultReporter.writeResults.
- Around line 92-103: The command string built in TestWorkflowService (using
unityPath, args and resultFile) is vulnerable to shell injection because
execSync runs via the shell; instead, replace the execSync(command, ...) call
with execFileSync (or execFile) and pass unityPath as the file and the
individual arguments (including the test result path derived from
params.testResultPath/run.name) as an array so special characters are not
interpreted by a shell; keep the same options (timeout, cwd, stdio, encoding)
when invoking execFileSync and continue using
TestWorkflowService.resolveUnityPath to resolve the binary path.
---
Nitpick comments:
In `@src/model/orchestrator/services/test-workflow/test-result-reporter.ts`:
- Around line 27-52: Replace the fragile regex-based parsing in parseJUnitXml
with a real XML parser (e.g., fast-xml-parser or DOMParser): parse xmlContent
into an object, locate the testsuite element, and read its attributes to set
runName, totalTests, failureCount, skippedCount, and duration (preserve existing
fallback defaults). Ensure the parser handles both single- and double-quoted
attributes and escaped characters, and keep the TestResult shape returned by
parseJUnitXml unchanged for callers.
In `@src/model/orchestrator/services/test-workflow/test-suite-parser.ts`:
- Around line 87-95: The validation for raw.filters currently throws a generic
Error; change it to throw a TypeError to match the needs check style — i.e.,
where you detect that raw.filters is not an object or is an array, throw a
TypeError with the same message (e.g., in the block handling raw.filters and
assigning run.filters); keep the rest of the logic (initializing run.filters and
copying entries) unchanged.
- Around line 222-262: The detectCircularDependencies function currently returns
null in several places; change its return type to string | undefined and update
the inner dfs function signature to return string | undefined, replacing all
"return null" with "return undefined" (and any callers that explicitly compare
to null to handle undefined or use falsy checks). Update the final return to
return undefined instead of null and ensure any code that calls
detectCircularDependencies (or checks dfs results) is adjusted accordingly to
accept undefined.
- Around line 64-69: The code in test-suite-parser.ts currently throws a generic
Error when raw.needs fails the array type check; change that to throw a
TypeError to satisfy the unicorn/prefer-type-error rule—replace the throw new
Error(...) in the block that checks Array.isArray(raw.needs) with throw new
TypeError(...) and keep the same message, leaving the surrounding logic that
assigns run.needs = raw.needs intact.
In `@src/model/orchestrator/services/test-workflow/test-workflow-types.ts`:
- Around line 23-25: Rename the TaxonomyDefinition interface property
extensible_groups to camelCase extensibleGroups and update any references to
TaxonomyDefinition to use extensibleGroups; then modify the YAML parsing/mapping
logic (where the YAML is deserialized into the typed object) to map the incoming
extensible_groups field to extensibleGroups so external snake_case remains
supported while the TypeScript model uses camelCase.
In `@src/model/orchestrator/services/test-workflow/test-workflow.test.ts`:
- Around line 173-220: Rename the short callback parameter `e` to `error` in the
.some filter callbacks that inspect the `errors` array in the test cases for
TestSuiteParser.validateSuite (the expectations that currently use
errors.some((e) => ...)); update both occurrences checking 'Duplicate' and
'editMode' (and the one checking 'depends on itself' if present) so the callback
reads errors.some((error) => error.includes(...)) to satisfy the
unicorn/prevent-abbreviations rule and improve clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e32772fd-3f03-43a9-b322-58f7262ac3f1
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (11)
action.ymlsrc/index.tssrc/model/build-parameters.tssrc/model/input.tssrc/model/orchestrator/services/test-workflow/index.tssrc/model/orchestrator/services/test-workflow/taxonomy-filter-service.tssrc/model/orchestrator/services/test-workflow/test-result-reporter.tssrc/model/orchestrator/services/test-workflow/test-suite-parser.tssrc/model/orchestrator/services/test-workflow/test-workflow-service.tssrc/model/orchestrator/services/test-workflow/test-workflow-types.tssrc/model/orchestrator/services/test-workflow/test-workflow.test.ts
Comment on lines
+21
to
+36
|
|
||
| // If a test suite path is provided, use the test workflow engine | ||
| // instead of the standard build execution path | ||
| if (buildParameters.testSuitePath) { | ||
| core.info('[TestWorkflow] Test suite path detected, using test workflow engine'); | ||
| const results = await TestWorkflowService.executeTestSuite(buildParameters.testSuitePath, buildParameters); | ||
|
|
||
| const totalFailed = results.reduce((sum, r) => sum + r.failed, 0); | ||
| if (totalFailed > 0) { | ||
| core.setFailed(`Test workflow completed with ${totalFailed} failure(s)`); | ||
| } else { | ||
| core.info('[TestWorkflow] All test runs passed'); | ||
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Consider setting outputs for consistency with the build path.
The test workflow path doesn't call Output.setBuildVersion, Output.setAndroidVersionCode, or Output.setEngineExitCode before returning. If downstream workflows depend on these outputs, they'll be undefined when using the test workflow path.
If this is intentional (test mode doesn't produce builds), consider documenting this or setting a sentinel value for engineExitCode based on test results.
💡 Optional: Set exit code output for consistency
const totalFailed = results.reduce((sum, r) => sum + r.failed, 0);
if (totalFailed > 0) {
core.setFailed(`Test workflow completed with ${totalFailed} failure(s)`);
} else {
core.info('[TestWorkflow] All test runs passed');
}
+
+ // Set exit code output for workflow consistency
+ await Output.setEngineExitCode(totalFailed > 0 ? 1 : 0);
return;
}🧰 Tools
🪛 ESLint
[error] 28-28: Array#reduce() is not allowed
(unicorn/no-array-reduce)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 21 - 36, When the test workflow path is taken
(buildParameters.testSuitePath and TestWorkflowService.executeTestSuite), the
function currently returns without setting CI outputs; call
Output.setBuildVersion and Output.setAndroidVersionCode with appropriate
sentinel or empty values and set Output.setEngineExitCode to 0 on all-passing
tests or a non-zero sentinel (e.g., 1) when totalFailed > 0 before returning, so
downstream steps always have defined outputs; update the code around the
TestWorkflowService.executeTestSuite block to compute totalFailed then set these
three outputs accordingly prior to the early return.
| * from an optional taxonomy file. | ||
| */ | ||
| static loadTaxonomy(filePath?: string): TaxonomyDimension[] { | ||
| const dimensions = [...TaxonomyFilterService.BUILT_IN_DIMENSIONS]; |
There was a problem hiding this comment.
Mutation of BUILT_IN_DIMENSIONS inner arrays corrupts state across calls.
The BUILT_IN_DIMENSIONS array is copied shallowly on line 30, but when merging custom dimensions with existing built-in ones (lines 41-47), existing.values.push(value) mutates the original BUILT_IN_DIMENSIONS entries. Since readonly only prevents reassignment of the array reference (not mutation of contained objects), subsequent calls to loadTaxonomy will see the accumulated values from all previous calls.
🐛 Proposed fix: deep-copy dimensions to avoid mutation
static loadTaxonomy(filePath?: string): TaxonomyDimension[] {
- const dimensions = [...TaxonomyFilterService.BUILT_IN_DIMENSIONS];
+ const dimensions = TaxonomyFilterService.BUILT_IN_DIMENSIONS.map((d) => ({
+ name: d.name,
+ values: [...d.values],
+ }));
if (filePath && fs.existsSync(filePath)) {Also applies to: 37-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts` at
line 30, The code copies TaxonomyFilterService.BUILT_IN_DIMENSIONS shallowly
causing mutations of inner objects when merging custom dimensions; in
loadTaxonomy, avoid mutating BUILT_IN_DIMENSIONS by deep-cloning its entries
before merging (e.g., create a new dimensions array from
TaxonomyFilterService.BUILT_IN_DIMENSIONS where each dimension object and its
values array are copied), and when merging custom dimensions replace
existing.values with a new array (or create a new merged object) instead of
calling existing.values.push(value) so built-in state is never mutated across
calls.
Comment on lines
+160
to
+169
| if (trimmed.startsWith('/') && trimmed.endsWith('/') && trimmed.length > 2) { | ||
| const pattern = trimmed.slice(1, -1); | ||
| try { | ||
| const regex = new RegExp(pattern); | ||
| return regex.test(testValue); | ||
| } catch { | ||
| // Invalid regex, treat as literal | ||
| return testValue === trimmed; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file to understand the full context
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '140,180p'Repository: game-ci/unity-builder
Length of output: 1484
🏁 Script executed:
# Search for the function containing this code to understand data flow
ast-grep --pattern 'function $name($$$) {
$$$
if (trimmed.startsWith("/")) {
$$$
}
$$$
}'Repository: game-ci/unity-builder
Length of output: 47
🌐 Web query:
ReDoS regex denial of service Node.js safe-regex library validation 2024 2025
💡 Result:
- What
safe-regexactually validates (Node.js/JS ReDoS): it’s a heuristic checker that flags potentially catastrophic backtracking by analyzing the regex’s structure—specifically it “limit[s] the star height to 1” and (optionally) caps total repetitions viaopts.limit(default 25). It returns false for invalid regex strings. It does not measure runtime on attacker-crafted inputs. [1] - Accuracy in practice: the maintainer explicitly warns the module has both false positives and false negatives, and recommends
vuln-regex-detectorfor improved accuracy. [1] - 2024–2025 maintenance reality:
safe-regex’s latest version is 2.1.1 and it was last updated/published ~6 years ago (i.e., not actively maintained through 2024–2025), even though it remains widely depended upon. [2] - If you’re using regex for untrusted-input validation in Node.js (2024–2025 guidance):
- Treat
safe-regexas a coarse lint, not a security guarantee; confirm reachability by untrusted input and add input length limits/timeouts where possible. [1] - Prefer linear-time engines (e.g., RE2 bindings) where compatible; Node’s default regex engine is the built-in backtracking engine, and switching to a linear-time engine is a common mitigation recommended by security guidance. [1][3]
- Treat
- If you need an alternative package:
safe-regex2exists and is actively versioned (e.g., listed with a current “latest” release by third-party package intel), but it’s still in the same general space of “detection” rather than a guaranteed defense. [4]
Sources:
[1] davisjam/safe-regex README (GitHub) (github.com)
[2] Snyk package page for safe-regex (version/last updated) (security.snyk.io)
[3] Ajv security notes on regex/ReDoS + linear-time engines (ajv.js.org)
[4] Aikido Intel package page for safe-regex2 (latest version listing) (intel.aikido.dev)
Citations:
- 1: https://github.com/davisjam/safe-regex
- 2: https://security.snyk.io/package/npm/safe-regex?utm_source=openai
- 3: https://ajv.js.org/security.html?utm_source=openai
- 4: https://intel.aikido.dev/packages/npm/safe-regex2?utm_source=openai
🌐 Web query:
Node.js timeout regex execution catastrophic backtracking prevention
💡 Result:
Practical ways to prevent catastrophic backtracking (ReDoS) in Node.js
- Prefer a linear-time regex engine for untrusted input
- Use RE2 bindings (no backtracking → avoids catastrophic backtracking by design): the
re2npm package (“node-re2”). It’s commonly recommended specifically to mitigate ReDoS in Node.js. [1]
- Enable V8’s non-backtracking fallback / linear engine (where available)
- V8 added an experimental non-backtracking RegExp engine and exposes controls such as:
--regexp-backtracks-before-fallback=N(fallback triggers after “excessive” backtracking)--enable-experimental-regexp-engine(enables the experimental engine features, including a non-standardl“linear” flag in V8) [2]
- Whether/how you can use these depends on your Node.js build/version and runtime flags, but this is the closest thing to an “engine-level” mitigation inside V8. [2]
- Do not rely on “timeouting” a normal JS RegExp on the main thread
- JavaScript’s built-in
RegExpexecution is synchronous; if it gets stuck, it blocks the event loop. There isn’t a standard per-regex timeout knob you can pass toRegExp.prototype.exec/test. (This is the core issue discussed in “timing out regex” questions.) [3]
- Isolate regex work so you can enforce a timeout
- Run the match in a Worker thread / separate process, and kill it if it exceeds a deadline. This doesn’t make the regex faster, but it prevents one request from freezing the whole server (a common operational mitigation). (This is the typical solution when people ask for “regex timeouts” in JS.) [3]
- Prevent vulnerable patterns from reaching production
- Static checks:
safe-regexcan flag potentially catastrophic regexes (heuristic-based). [4] - Linters/analyzers: security tooling and articles commonly recommend rules/tools that detect “vulnerable regular expressions,” and to refactor patterns to remove ambiguous nested quantifiers, etc. [5]
- Input hardening (still useful even with other mitigations)
- Put strict length limits on user-controlled strings before regexing them; catastrophic cases often trigger on long near-misses. (Common mitigation in ReDoS guidance.) [5]
Rule of thumb: if the pattern or the input is attacker-controlled, either use RE2 [1] or run the match off-thread with a kill switch [3], and add static detection (safe-regex) as a guardrail. [4]
Sources
[1] re2 (node-re2) npm package description (RE2 bindings; protects against ReDoS) — https://www.npmjs.com/package/re2
[2] V8 blog: “An additional non-backtracking RegExp engine” (flags, fallback threshold, linear engine) — https://v8.dev/blog/non-backtracking-regexp
[3] Stack Overflow: “Timing out regex to avoid catastrophic backtracking” (JS regex is synchronous; timeout requires isolation) — https://stackoverflow.com/questions/50749499/timing-out-regex-to-avoid-catastrophic-backtracking
[4] safe-regex npm package (detect potentially catastrophic regexes) — https://www.npmjs.com/package/safe-regex
[5] SonarSource: “Vulnerable regular expressions in JavaScript” (ReDoS causes, mitigations, mentions RE2 bindings/tools) — https://www.sonarsource.com/blog/vulnerable-regular-expressions-javascript/
ReDoS vulnerability: regex constructed from untrusted input.
User-supplied filter patterns (e.g., /pattern/) are compiled directly into RegExp without validation (line 163). Malicious patterns with catastrophic backtracking (e.g., /(a+)+$/) can cause denial-of-service by consuming excessive CPU time. The existing catch block only handles syntax errors, not slow execution.
To mitigate:
- Preferred: Replace with RE2 bindings (
re2npm package) for linear-time matching on untrusted patterns - Practical: Add strict length limits on
valueSpecbefore regex compilation - Detection: Use
safe-regexas a heuristic check during development (note: requires static pattern validation, not runtime) - Isolation: If using standard
RegExp, consider moving regex execution to a Worker thread with a timeout to prevent blocking the event loop
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 162-162: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 ESLint
[error] 164-164: Expected blank line before this statement.
(padding-line-between-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts`
around lines 160 - 169, The RegExp construction in the taxonomy filter block
(the trimmed.startsWith('/') && trimmed.endsWith('/') branch) uses untrusted
input and is vulnerable to ReDoS; replace unsafe RegExp usage by either: 1)
switching to a linear-time regex engine such as the re2 package (create an RE2
instance instead of new RegExp for the pattern), or 2) if re2 cannot be
introduced, enforce strict runtime protections by (a) validating and bounding
the pattern length (reject patterns above a safe threshold), and (b) executing
the regex test off the main thread with a timeout (e.g., a Worker/worker_threads
task that returns false on timeout) to avoid blocking the event loop; update the
catch behavior to treat rejected/timeout cases as literal matches and surface a
debug log message for rejected patterns.
Comment on lines
+278
to
+282
| lines.push(` <failure message="${TestResultReporter.escapeXml(failure.message)}">`); | ||
| if (failure.stackTrace) { | ||
| lines.push(` <![CDATA[${failure.stackTrace}]]>`); | ||
| } | ||
| lines.push(' </failure>'); |
There was a problem hiding this comment.
CDATA section can be broken by ]]> in stack trace.
If failure.stackTrace contains the literal string ]]>, it will prematurely close the CDATA section and produce malformed XML. CDATA cannot contain ]]> as-is.
🐛 Proposed fix: escape or split CDATA terminator
+ /**
+ * Sanitize content for CDATA by splitting any ]]> sequences.
+ */
+ private static sanitizeCData(content: string): string {
+ return content.replace(/\]\]>/g, ']]]]><![CDATA[>');
+ }
+
private static toJUnitXml(results: TestResult[]): string {Then in the CDATA output:
if (failure.stackTrace) {
- lines.push(` <![CDATA[${failure.stackTrace}]]>`);
+ lines.push(` <![CDATA[${TestResultReporter.sanitizeCData(failure.stackTrace)}]]>`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lines.push(` <failure message="${TestResultReporter.escapeXml(failure.message)}">`); | |
| if (failure.stackTrace) { | |
| lines.push(` <![CDATA[${failure.stackTrace}]]>`); | |
| } | |
| lines.push(' </failure>'); | |
| lines.push(` <failure message="${TestResultReporter.escapeXml(failure.message)}">`); | |
| if (failure.stackTrace) { | |
| lines.push(` <![CDATA[${TestResultReporter.sanitizeCData(failure.stackTrace)}]]>`); | |
| } | |
| lines.push(' </failure>'); |
Suggested change
| lines.push(` <failure message="${TestResultReporter.escapeXml(failure.message)}">`); | |
| if (failure.stackTrace) { | |
| lines.push(` <![CDATA[${failure.stackTrace}]]>`); | |
| } | |
| lines.push(' </failure>'); | |
| /** | |
| * Sanitize content for CDATA by splitting any ]]> sequences. | |
| */ | |
| private static sanitizeCData(content: string): string { | |
| return content.replace(/\]\]>/g, ']]]]><![CDATA[>'); | |
| } |
🧰 Tools
🪛 ESLint
[error] 278-278: Do not call Array#push() multiple times.
(unicorn/no-array-push-push)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-result-reporter.ts` around
lines 278 - 282, The CDATA block written in TestResultReporter (where lines.push
is used to output failure.stackTrace) can be broken if failure.stackTrace
contains "]]>"; update the reporter to sanitize/split the stack trace before
wrapping in <![CDATA[...]]> by replacing every occurrence of "]]>" with
"]]]]><![CDATA[>" (or otherwise escape/split the terminator) so the CDATA
section remains valid; ensure this transformation is applied to the value used
in the lines.push call that currently writes `<![CDATA[${failure.stackTrace}]]>`
and keep using TestResultReporter.escapeXml for other XML content as
appropriate.
Comment on lines
+64
to
+69
| if (raw.needs !== undefined) { | ||
| if (!Array.isArray(raw.needs)) { | ||
| throw new Error(`Run '${raw.name}': 'needs' must be an array of strings`); | ||
| } | ||
| run.needs = raw.needs; | ||
| } |
There was a problem hiding this comment.
Validate that each element in needs is a string.
The array is assigned directly without validating that each element is actually a string. If the YAML contains non-string values like needs: [1, null], they would be accepted silently.
🛡️ Proposed fix
if (raw.needs !== undefined) {
if (!Array.isArray(raw.needs)) {
throw new TypeError(`Run '${raw.name}': 'needs' must be an array of strings`);
}
- run.needs = raw.needs;
+ if (!raw.needs.every((n: unknown) => typeof n === 'string')) {
+ throw new TypeError(`Run '${raw.name}': 'needs' must contain only strings`);
+ }
+ run.needs = raw.needs as string[];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (raw.needs !== undefined) { | |
| if (!Array.isArray(raw.needs)) { | |
| throw new Error(`Run '${raw.name}': 'needs' must be an array of strings`); | |
| } | |
| run.needs = raw.needs; | |
| } | |
| if (raw.needs !== undefined) { | |
| if (!Array.isArray(raw.needs)) { | |
| throw new TypeError(`Run '${raw.name}': 'needs' must be an array of strings`); | |
| } | |
| if (!raw.needs.every((n: unknown) => typeof n === 'string')) { | |
| throw new TypeError(`Run '${raw.name}': 'needs' must contain only strings`); | |
| } | |
| run.needs = raw.needs as string[]; | |
| } |
🧰 Tools
🪛 ESLint
[error] 66-66: new Error() is too unspecific for a type check. Use new TypeError() instead.
(unicorn/prefer-type-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-suite-parser.ts` around
lines 64 - 69, The needs array is assigned without element type checks; update
the validation in the block that checks raw.needs (the code referencing raw.name
and assigning run.needs) to ensure every element is a string: iterate over
raw.needs, confirm typeof item === 'string' for each entry, and if any element
fails, throw a descriptive Error including the run name and the offending
index/value (e.g., "Run '...': 'needs' element at index X must be a string").
Only assign run.needs = raw.needs after all elements pass validation.
src/model/orchestrator/services/test-workflow/test-workflow-service.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+63
to
+65
| const resultFormat = params.testResultFormat; | ||
| if (resultPath) { | ||
| TestResultReporter.writeResults(allResults, resultPath, resultFormat as 'junit' | 'json' | 'both'); |
There was a problem hiding this comment.
Unsafe type assertion; validate testResultFormat before use.
The cast resultFormat as 'junit' | 'json' | 'both' bypasses type checking. If params.testResultFormat contains an unexpected value, writeResults may behave unexpectedly or fail silently.
🛡️ Proposed fix: validate and default the format
const resultPath = params.testResultPath;
- const resultFormat = params.testResultFormat;
+ const validFormats = ['junit', 'json', 'both'] as const;
+ const resultFormat = validFormats.includes(params.testResultFormat as any)
+ ? (params.testResultFormat as 'junit' | 'json' | 'both')
+ : 'both';
if (resultPath) {
- TestResultReporter.writeResults(allResults, resultPath, resultFormat as 'junit' | 'json' | 'both');
+ TestResultReporter.writeResults(allResults, resultPath, resultFormat);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts`
around lines 63 - 65, The code unsafely asserts params.testResultFormat when
calling TestResultReporter.writeResults; instead validate
params.testResultFormat against allowed values ('junit','json','both') and
map/normalize it to a safe union before calling writeResults. In the block
around resultFormat/resultPath, replace the assertion with a small validation:
read params.testResultFormat into resultFormat, if it matches one of the allowed
strings use it, otherwise set a sensible default (e.g., 'both'), then pass that
validated value to TestResultReporter.writeResults.
Comment on lines
+92
to
+103
| // Build the full Unity command | ||
| const unityPath = TestWorkflowService.resolveUnityPath(params); | ||
| const command = `"${unityPath}" ${args} -testResults "${resultFile}"`; | ||
|
|
||
| core.info(`[TestWorkflow] Executing: ${command}`); | ||
|
|
||
| execSync(command, { | ||
| timeout: timeoutMs, | ||
| stdio: 'pipe', | ||
| encoding: 'utf8', | ||
| cwd: params.projectPath || process.cwd(), | ||
| }); |
There was a problem hiding this comment.
Potential command injection via shell metacharacters in paths.
The command string (line 94) is built by concatenating unityPath and resultFile into a shell command. Since execSync uses shell by default, special characters in params.testResultPath or run.name could allow command injection.
Consider using execFileSync (or async equivalent) which bypasses the shell, passing arguments as an array.
🔒️ Proposed fix: use execFile with argument array
-import { execSync } from 'node:child_process';
+import { execFile } from 'node:child_process';
+import { promisify } from 'node:util';
+
+const execFileAsync = promisify(execFile);- const command = `"${unityPath}" ${args} -testResults "${resultFile}"`;
- core.info(`[TestWorkflow] Executing: ${command}`);
-
- execSync(command, {
- timeout: timeoutMs,
- stdio: 'pipe',
- encoding: 'utf8',
- cwd: params.projectPath || process.cwd(),
- });
+ const argList = [...args.split(' ').filter(Boolean), '-testResults', resultFile];
+ core.info(`[TestWorkflow] Executing: ${unityPath} ${argList.join(' ')}`);
+
+ await execFileAsync(unityPath, argList, {
+ timeout: timeoutMs,
+ cwd: params.projectPath || process.cwd(),
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts`
around lines 92 - 103, The command string built in TestWorkflowService (using
unityPath, args and resultFile) is vulnerable to shell injection because
execSync runs via the shell; instead, replace the execSync(command, ...) call
with execFileSync (or execFile) and pass unityPath as the file and the
individual arguments (including the test result path derived from
params.testResultPath/run.name) as an array so special characters are not
interpreted by a shell; keep the same options (timeout, cwd, stdio, encoding)
when invoking execFileSync and continue using
TestWorkflowService.resolveUnityPath to resolve the binary path.
Comment on lines
+188
to
+199
| } else if (run.editMode && run.playMode) { | ||
| // Both modes: run EditMode first, then PlayMode will require a separate invocation | ||
| // For combined mode, use EditMode (the service handles sequencing) | ||
| args.push('-runTests'); | ||
| args.push('-testPlatform EditMode'); | ||
| } else if (run.playMode) { | ||
| args.push('-runTests'); | ||
| args.push('-testPlatform PlayMode'); | ||
| } else if (run.editMode) { | ||
| args.push('-runTests'); | ||
| args.push('-testPlatform EditMode'); | ||
| } |
There was a problem hiding this comment.
Combined editMode+playMode only runs EditMode; PlayMode tests are silently skipped.
When both run.editMode and run.playMode are true, only EditMode is configured (lines 191-192). The comment states "the service handles sequencing," but no such sequencing logic exists in executeTestRun or executeTestSuite.
Either implement dual-mode execution (two Unity invocations) or document that users should create separate runs for each mode.
Do you want me to propose an implementation that handles both modes with separate invocations?
🧰 Tools
🪛 ESLint
[error] 192-192: Do not call Array#push() multiple times.
(unicorn/no-array-push-push)
[error] 195-195: Do not call Array#push() multiple times.
(unicorn/no-array-push-push)
[error] 198-198: Do not call Array#push() multiple times.
(unicorn/no-array-push-push)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts`
around lines 188 - 199, When both run.editMode and run.playMode are true, the
current branch only configures EditMode and silently skips PlayMode; update
test-workflow-service to perform two sequential Unity invocations instead: build
args for EditMode (as currently done) and call executeTestRun/executeTestSuite
for that run, then build args for PlayMode (with '-runTests' and '-testPlatform
PlayMode') and call executeTestRun/executeTestSuite again; ensure you
propagate/aggregate results or errors from both invocations (e.g., fail the
overall run if either invocation fails) and reference the existing run object
and functions executeTestRun and executeTestSuite when locating where to add the
second invocation and result handling.
Comment on lines
+227
to
+237
| // Default paths by platform | ||
| if (process.platform === 'win32') { | ||
| return `C:/Program Files/Unity/Hub/Editor/${params.editorVersion}/Editor/Unity.exe`; | ||
| } | ||
| if (process.platform === 'darwin') { | ||
| return `/Applications/Unity/Hub/Editor/${params.editorVersion}/Unity.app/Contents/MacOS/Unity`; | ||
| } | ||
|
|
||
| // Linux default (Docker container path) | ||
| return '/opt/unity/Editor/Unity'; | ||
| } |
There was a problem hiding this comment.
Handle undefined editorVersion to avoid invalid paths.
If params.editorVersion is undefined, the constructed paths will contain the literal string "undefined" (e.g., /Applications/Unity/Hub/Editor/undefined/Unity.app/...). Consider adding validation or a fallback.
🛡️ Proposed fix: validate editorVersion
private static resolveUnityPath(params: BuildParameters): string {
const envUnityPath = process.env.UNITY_PATH ?? process.env.UNITY_EDITOR;
if (envUnityPath) {
return envUnityPath;
}
+ if (!params.editorVersion) {
+ throw new Error('Unity editor version is required but not specified. Set UNITY_PATH environment variable or provide editorVersion.');
+ }
+
// Default paths by platform📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Default paths by platform | |
| if (process.platform === 'win32') { | |
| return `C:/Program Files/Unity/Hub/Editor/${params.editorVersion}/Editor/Unity.exe`; | |
| } | |
| if (process.platform === 'darwin') { | |
| return `/Applications/Unity/Hub/Editor/${params.editorVersion}/Unity.app/Contents/MacOS/Unity`; | |
| } | |
| // Linux default (Docker container path) | |
| return '/opt/unity/Editor/Unity'; | |
| } | |
| private static resolveUnityPath(params: BuildParameters): string { | |
| const envUnityPath = process.env.UNITY_PATH ?? process.env.UNITY_EDITOR; | |
| if (envUnityPath) { | |
| return envUnityPath; | |
| } | |
| if (!params.editorVersion) { | |
| throw new Error('Unity editor version is required but not specified. Set UNITY_PATH environment variable or provide editorVersion.'); | |
| } | |
| // Default paths by platform | |
| if (process.platform === 'win32') { | |
| return `C:/Program Files/Unity/Hub/Editor/${params.editorVersion}/Editor/Unity.exe`; | |
| } | |
| if (process.platform === 'darwin') { | |
| return `/Applications/Unity/Hub/Editor/${params.editorVersion}/Unity.app/Contents/MacOS/Unity`; | |
| } | |
| // Linux default (Docker container path) | |
| return '/opt/unity/Editor/Unity'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts`
around lines 227 - 237, The path builder uses params.editorVersion directly and
can produce "undefined" in returned paths; validate params.editorVersion at the
start of the routine (the function that returns platform-specific Unity paths)
and either throw a clear error or substitute a sensible fallback before
constructing the paths. Specifically, check params.editorVersion for
undefined/empty and handle it prior to the win32/darwin/Linux returns so
Unity.exe/Unity.app/Unity path strings are never built with "undefined" — update
the function that contains the platform branch (references to
params.editorVersion and the three return statements for Windows, macOS, and
Linux) to perform this validation and return/raise accordingly.
Replace execSync with promisified exec so Promise.all actually runs test groups in parallel. Add native timeout support via exec options. Add 50MB maxBuffer for large Unity output. Fix ESLint violations (variable naming, padding lines, array push consolidation). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #790 +/- ##
==========================================
+ Coverage 31.25% 35.41% +4.15%
==========================================
Files 84 88 +4
Lines 4563 5038 +475
Branches 1103 1238 +135
==========================================
+ Hits 1426 1784 +358
- Misses 3137 3254 +117
🚀 New features to boost your workflow:
|
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The orchestrator-develop branch no longer exists. Update all fallback clone commands and test fixtures to use main instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/model/orchestrator/workflows/build-automation-workflow.ts (1)
101-103: Extract this clone-fallback block into a shared helper.Lines 101-103 now mirror the same branch-resolution shell snippet in
src/model/orchestrator/workflows/async-workflow.ts. Keeping this policy duplicated in two template strings makes future fallback changes easy to drift again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/workflows/build-automation-workflow.ts` around lines 101 - 103, Extract the duplicated clone-fallback shell snippet into a shared helper function (e.g., renderCloneFallbackSnippet or getCloneFallbackSnippet) and replace the inline template string in both build-automation-workflow.ts and async-workflow.ts with calls to that helper; the helper should return the exact snippet currently used (the echo + git clone -b main ... || git clone ...) so future changes are made in one place, and update imports/exports accordingly so build-automation-workflow.ts and async-workflow.ts use the new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/orchestrator-async-checks.yml:
- Line 57: Replace the hardcoded "git clone -b main ..." invocation so the
workflow checks out the dispatched branch/commit instead of always bootstrapping
from main; specifically, stop using the literal "git clone -b main" and either
use actions/checkout@v4 with ref: ${{ github.ref }} (or ref: ${{ github.sha }})
or adjust the git clone to use the dispatched ref variable (github.ref or
github.sha) so the workflow validates the exact revision of the dispatch.
---
Nitpick comments:
In `@src/model/orchestrator/workflows/build-automation-workflow.ts`:
- Around line 101-103: Extract the duplicated clone-fallback shell snippet into
a shared helper function (e.g., renderCloneFallbackSnippet or
getCloneFallbackSnippet) and replace the inline template string in both
build-automation-workflow.ts and async-workflow.ts with calls to that helper;
the helper should return the exact snippet currently used (the echo + git clone
-b main ... || git clone ...) so future changes are made in one place, and
update imports/exports accordingly so build-automation-workflow.ts and
async-workflow.ts use the new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2efb14ba-4f25-4eda-a403-28f20e737d6b
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (5)
.github/workflows/build-tests-mac.yml.github/workflows/orchestrator-async-checks.ymlsrc/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.tssrc/model/orchestrator/workflows/async-workflow.tssrc/model/orchestrator/workflows/build-automation-workflow.ts
✅ Files skipped from review due to trivial changes (1)
- src/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.ts
| CHECKS_UPDATE: ${{ github.event.inputs.checksObject }} | ||
| run: | | ||
| git clone -b orchestrator-develop https://github.com/game-ci/unity-builder | ||
| git clone -b main https://github.com/game-ci/unity-builder |
There was a problem hiding this comment.
Clone the dispatched ref instead of always bootstrapping from main.
Line 57 now forces checks-update to run from the default-branch code even when this workflow is dispatched from a feature branch. That makes async checks validate the wrong revision and can hide regressions in the branch under review.
Suggested fix
- git clone -b main https://github.com/game-ci/unity-builder
+ BRANCH="${ORCHESTRATOR_BRANCH#refs/heads/}"
+ git clone -b "$BRANCH" https://github.com/game-ci/unity-builder \
+ || git clone -b main https://github.com/game-ci/unity-builder📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| git clone -b main https://github.com/game-ci/unity-builder | |
| BRANCH="${ORCHESTRATOR_BRANCH#refs/heads/}" | |
| git clone -b "$BRANCH" https://github.com/game-ci/unity-builder \ | |
| || git clone -b main https://github.com/game-ci/unity-builder |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/orchestrator-async-checks.yml at line 57, Replace the
hardcoded "git clone -b main ..." invocation so the workflow checks out the
dispatched branch/commit instead of always bootstrapping from main;
specifically, stop using the literal "git clone -b main" and either use
actions/checkout@v4 with ref: ${{ github.ref }} (or ref: ${{ github.sha }}) or
adjust the git clone to use the dispatched ref variable (github.ref or
github.sha) so the workflow validates the exact revision of the dispatch.
Member
Author
|
Closing — all orchestrator code has been extracted to the standalone Content from this PR (test workflow engine — suite definitions, taxonomy filters, structured results) is fully present in the orchestrator repo. See PR #819 for the extraction. |
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a fully implemented test workflow engine to the Orchestrator module. This enables YAML-based test suite definitions with multi-dimensional taxonomy filtering, structured result reporting, and sequential execution dependencies. Modeled on battle-tested patterns from production Unity projects with 300+ tests across multiple suites.
Implementation
needs:dependencies. Uses Kahn's topological sort for correct execution ordering.New action.yml inputs (5)
testSuitePathtestSuiteEventtestTaxonomyPathtestResultFormattestResultPathExample suite definition
Test coverage
43 unit tests covering:
Related Issues
Documentation
11-test-workflow-engine.mdxTest plan
tsc --noEmit— no type errorsSummary by CodeRabbit
New Features
Documentation
Chores
Tracking: