| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Directory-based SARIF discovery misses existing files on Windows because glob patterns contain backslash separators.
1 open issue 1 file not reviewedPrepares the CodeQL Action for the runner-compatibility checks in #4221 through focused refactoring and expanded diagnostic validation.
Changes:
| File | Description |
|---|---|
| src/testing-utils.ts | Supports injected environments in the CLI test helper. |
| src/languages/swift.ts | Extracts Swift runner compatibility validation. |
| src/languages/swift.test.ts | Tests Swift and non-Swift platform handling. |
| src/init-action.ts | Calls the extracted Swift validation helper. |
| src/diagnostics.ts | Narrows diagnostic helpers’ configuration types. |
| src/codeql.ts | Routes selected environment accesses through an injected environment. |
| src/cli/output-cache.ts | Accepts a read-only environment for cache paths. |
| src/actions-util.ts | Accepts a read-only environment for temporary paths. |
| pr-checks/package.json | Declares the glob dependency. |
| pr-checks/check-diagnostics.ts | Adds recursive directory-based SARIF validation. |
| package-lock.json | Records the PR-checks dependency change. |
Review effort: Max
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for factoring these changes out into a separate PR!
Sorry, something went wrong.
| logger: Logger, | ||
| cmd: string, | ||
| checkVersion: boolean, | ||
| env: Env = getEnv(), |
There was a problem hiding this comment.
I think we probably want to have the CLI inherit env when we invoke it. Currently the CLI inherits process.env, which will match env in Actions, but in tests it might mismatch and result in confusing behaviour.
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch. It makes sense to propagate this down for consistency/sanity, even if there might not be a real scenario where this would bite us right now.
Sorry, something went wrong.
There was a problem hiding this comment.
This looks good now. Should we add some unit tests to check the forwarding?
Sorry, something went wrong.
There was a problem hiding this comment.
The cross-platform CLI environment refactor needs human validation, particularly where commands still use different environment sources.
1 open issue 1 resolved since last review 1 file not reviewedReview effort: Max
Sorry, something went wrong.
| logger: Logger, | ||
| cmd: string, | ||
| checkVersion: boolean, | ||
| env: Env = getEnv(), |
There was a problem hiding this comment.
This looks good now. Should we add some unit tests to check the forwarding?
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This branch contains a few cherry-picked commits from #4221 which can be reviewed and merged in isolation. Just some minor refactoring and enhancements.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist