Repository navigation
fix(sca): restore incremental analysis runtime and guarded license scope - #469
Conversation
There was a problem hiding this comment.
Assessment: APPROVE / no blocking findings after correction at 1efca034308fff9990998baa82fa6721330ce6f6.
The deep review found and fixed one P2 issue: passing request.signal as a github.paginate parameter did not bound file pagination. The exact pinned github-script bundle reconstructs page requests with only method/URL/headers. Real request tracing showed the signal on metadata calls but absent on both file pages. A stalled later page outlived the 30-second deadline; the controlled original run returned only when the page was released after 33 seconds.
The fix binds the existing shared request options to listFiles.defaults(...). No additional timer, retry, state or fallback path is introduced. The same pinned-client scenario then aborts at the original deadline (30.6 seconds including harness teardown), before the delayed response.
Regression coverage uses the real Octokit class supplied by the pinned github-script action and a local HTTP server. It expires the signal only after a later page arrives, without timing sleeps, and verifies rejection, no subsequent API call and no exemption publication. Existing context/revision/checkout fixtures are reused. The workflow runs the existing unit cases plus this regression in the actual action runtime. Local result: 23 passed, zero failures/cancellations/skips. Against the original production file the new case times out, exits nonzero and cleans up. The passing regression takes about 22 ms.
Other closures were checked: Node PATH reaches the subsequent shell; current license policy/tool hashes agree; before/after revision drift and API failure fail closed; incomplete pagination and changed fingerprints use complete checks; paths with spaces/newlines remain single arguments; license errors prevent analyzer execution. Ten actual shell-consumer scenarios and four real license-eye checks passed. Syntax, YAML parsing and diff checks passed. Hosted final validation run 40254763201 passed, including all 23 scope tests. The first pushed test fixture inherited the Shanghai runner's GitHub proxy for localhost and failed; the final test-only correction uses direct Node fetch, with the real pinned pagination client and abort assertions retained. This proxy issue was independently reproduced with an unreachable proxy before the correction was pushed.
No pre-existing unresolved review threads were present. Source worktrees and the user's unrelated local changes were preserved. No subagents were used. Full hosted MatrixOne incremental performance after companion deployment is still separate from this correctness review.
Submitted as COMMENT because the authenticated account is also the PR author; this is an approval recommendation, not a GitHub approval vote.
…31142) ## What type of PR is this? - [ ] API-change - [x] BUG - [x] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Related to #29562. ## What this PR does / why we need it: Recent [SCA job 121193349037](https://github.com/matrixorigin/matrixone/actions/runs/40201453562/job/121193349037) silently ran a full graph because Node was missing from the shell PATH, despite an existing incremental selector. Its frozen PR inputs would have selected 34 of 507 packages. - Preserve compatibility with deployed CI: when Node is absent, explicitly report the reason, validate the complete graph and run both analyzers over `./...`. Node-present CI retains incremental selection; selector failures stay fatal. Local invocations still run the complete graph. - Explain conservative fallback decisions and report selected/total package counts on stderr, keeping stdout exclusively package arguments. - Reuse existing immutable-input selection, package ownership and reverse-import graph. Preserve full-scan overrides, discovery failures, analyzer failures and both analyzers' identical package scope. - Add regression coverage at the real Make/Go discovery boundary for missing runtime, dirty/ignored inputs, closure thresholds and transitive test imports. Companion CI PR: matrixorigin/CI#469. It exposes the runner's existing Node without a download and safely restores incremental license selection. Both merge orders are supported; older CI uses complete checks until the runtime enhancement is deployed. ### Validation - `node --test .github/ci/*.test.cjs`: 36 passed on the compatibility correction, including real Go discovery, missing-Node full execution, graph failure, and both analyzer failure paths. - Both configured analyzers passed in matched full/incremental experiments under 4 CPU / 8 GiB, with warmed compiled exports and separate cold analyzer caches: elapsed **313.54s → 113.54s (-63.8%)**; peak working set **7.95 GiB → 5.75 GiB (-27.6%)**; raw cgroup peak **8.00 GiB → 5.96 GiB**; no OOM. - The benchmark uses PR #31105's historical changed-file list against the same frozen application source. It bypasses dirty-checkout rejection only to measure selection; the public-interface fixtures separately verify immutable provenance. These are local measurements, not promised hosted-CI timings. - `git diff --check` passed. No Go implementation changes, so Go race detection is not applicable; JavaScript fixture tests cover this delta. - Existing molint pin/rules, analyzer concurrency and GC settings are unchanged. Hosted CI remains pending. Coverage UT admission and linter-driver upgrades are separate work. ### CI compatibility correction [Failed job 121266163435](https://github.com/matrixorigin/matrixone/actions/runs/40225473222/job/121266163435) used CI `30b0e58`, before the companion PR exposed Node on PATH. The hard runtime prerequisite has been removed: missing Node now retains complete validation with an explicit diagnostic. Incremental savings require deployment of the companion runtime enhancement; this correction does not claim savings on older CI. Hosted verification of the correction remains pending. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
What type of PR is this?
Which issue(s) this PR fixes:
Related to matrixorigin/matrixone#29562.
What this PR does / why we need it:
Recent SCA job 121193349037 silently ran both analyzers over
./...: the runner-provided Node runtime was unavailable on the shell PATH. Its license check also fell back to a full scan because validated policy fingerprints were stale.core.addPath. No SDK download or concurrency/GC tradeoff.request.signal. No new timer, retry, or fallback state is needed.Validation
node --testruns the 22 dependency-free unit cases.Companion: matrixorigin/matrixone#31142. Both merge orders are supported: the MatrixOne companion keeps complete SCA when Node is unavailable; this PR enables incremental selection on runners.