CI: harden release-check script execution paths#67920
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Greptile SummaryThis PR hardens the release-check script by deriving execution mode from the selected suite via Confidence Score: 5/5Safe to merge; the one flagged item is a P2 clarification question about an unmentioned filter change, not a blocking defect. All core changes (npm runner wiring, suite-to-mode derivation, OK marker fallback, workflow injection hardening) are correct. The only finding is a P2 query about whether the PACKAGED_QA_RUNTIME_PATHS removal was intentional — it does not affect script correctness if those files are no longer shipped. scripts/openclaw-cross-os-release-checks.ts — confirm the isPackagedDistPath change at lines 508-510 is intentional. Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/openclaw-cross-os-release-checks.ts
Line: 508-510
Comment:
**Unmentioned behavior change removes QA runtime paths from inventory**
`PACKAGED_QA_RUNTIME_PATHS` previously allowed `dist/extensions/qa-channel/runtime-api.js` and `dist/extensions/qa-lab/runtime-api.js` to pass through the QA extension omission filter and be included in the packaged-dist inventory snapshot. Now every path under `OMITTED_QA_EXTENSION_PREFIXES` unconditionally returns `false`, silently dropping those two files from the inventory. The PR description doesn't mention this as an intentional change — was this a deliberate cleanup (confirming those files are no longer shipped), or did it slip in unintentionally?
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "CI: narrow release-check hardening diff" | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f085306031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Integrity check bypass: OK marker accepted via regex scraping of untrusted stdout/log content
DescriptionThe cross-OS release checks validate agent behavior by looking for an Key issues:
Vulnerable code: // fallback when stdout is not JSON
...stdout.matchAll(/"(?:finalAssistantVisibleText|finalAssistantRawText|text)"\s*:\s*"([^"]*)"/gu)
// accepts OK from stdout or from reading logPath
const logTexts = parseAgentPayloadTexts(readFileSync(options.logPath, "utf8"));
return logTexts.some((text) => text.trim() === "OK");RecommendationTreat the OK marker as a strongly-typed, structured signal and only accept it from a single canonical location in a successfully parsed JSON payload. Suggested fix:
Example hardened approach: function agentOutputHasExpectedOkMarker(stdout: string): boolean {
let payload: any;
try {
payload = JSON.parse(stdout);
} catch {
return false; // no fallbacks
}
// Require a canonical field and exact match.
const text = payload?.finalAssistantVisibleText;
return typeof text === "string" && text.trim() === "OK";
}If log-based recovery is required for platform reasons, write a dedicated JSON artifact (single-purpose file) that is not mixed with other logs and validate its full JSON content and schema before accepting Analyzed PR: #67920 at commit Last updated on: 2026-04-25T14:03:14Z |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
visionik
left a comment
There was a problem hiding this comment.
Thanks for the fix! Overlap with #66884 means only part of this PR is still needed once #66884 merges (which is imminent). Could you:
- Rebase on latest main after #66884 lands, and
- Drop the .github/workflows/openclaw-cross-os-release-checks-reusable.yml changes from this PR — they will already be resolved by #66884 using the same env-var-substitution pattern, so keeping them here would create a conflict.
What remains then is your scripts/openclaw-cross-os-release-checks.ts refactor (the resolveExecutionModeForSuite helper, the typed CommandInvocation / RunCommandOptions, and the resolveNpmRunner wiring in installPackageSpec) plus the matching tests. That is genuinely separate work and useful on its own — happy to review once narrowed. See #68428 for the full template-injection tracking context.
|
@visionik Thanks, makes sense. I checked #66884 and it is still open right now, so I'll wait for that PR to land first. Once #66884 merges, I'll rebase this branch onto the latest |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@visionik The workflow file in this PR now matches current
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f96fb09f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| try { | ||
| const logTexts = parseAgentPayloadTexts(readFileSync(options.logPath, "utf8")); | ||
| return logTexts.some((text) => text.trim() === "OK"); |
There was a problem hiding this comment.
Restrict OK fallback check to current command output
This fallback reads and scans the entire log file, so it can return a false green if that file already contains an older OK marker. Because command logs are opened in append mode and the output directory is not cleared before each run, reused runner temp paths (common on self-hosted runners) can leave stale *-agent.log content; a current failed/empty agent response can still pass this check by matching a prior run's OK.
Useful? React with 👍 / 👎.
Summary
scripts/openclaw-cross-os-release-checks.tsnow derives execution mode from the selectedsuite, usesresolveNpmRunnerfor the npm install path, and adds typed command invocation options for the Windows-safe npm execution flow.test/scripts/openclaw-cross-os-release-checks.test.tsnow coversresolveExecutionModeForSuite().Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
npm.cmdexecution path by reusingresolveNpmRunner.Regression Test Plan (if applicable)
test/scripts/openclaw-cross-os-release-checks.test.tsresolveExecutionModeForSuite()keeps the expected fresh/upgrade mapping and throws for unknown suites.User-visible / Behavior Changes
None.
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) YesYes/No) NoYes, explain risk + mitigation: this narrows the command execution surface by derivingmodeinside the script and using the repo's Windows-safe npm runner helper instead of shelling throughnpm.cmd.Repro + Verification
Environment
Steps
pnpm test test/scripts/openclaw-cross-os-release-checks.test.ts.scripts/openclaw-cross-os-release-checks.tsfor theresolveExecutionModeForSuite()helper andresolveNpmRunnerwiring.Expected
resolveExecutionModeForSuite()maps suites to the expected execution mode and rejects unsupported suites.resolveNpmRunnerrather than relying on the default.cmdshell path.Actual
modefromsuitewhen needed.resolveNpmRunner.pnpm test test/scripts/openclaw-cross-os-release-checks.test.tspassed locally.Human Verification (required)
main, and ranpnpm test test/scripts/openclaw-cross-os-release-checks.test.ts.resolveNpmRunnerinvocation.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations