fix: build runit#154
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughDetects a cloud-build YAML (user-provided or default), parses build/runtime props, and runs a new Runit wrapper that calls ChangesCloud Build Deployment Support
Sequence DiagramsequenceDiagram
participant CLI
participant BuilderFactory
participant FS
participant Runit
participant DockerImageBuilder
CLI->>BuilderFactory: invoke `build` with inputs/args
BuilderFactory->>BuilderFactory: parse argv (--cloud-build-config, --debug-instance)
BuilderFactory->>FS: findCloudBuildYaml() -> resolve path or fallback
alt YAML found
BuilderFactory->>FS: read YAML file
BuilderFactory->>BuilderFactory: parse/normalize props
BuilderFactory->>Runit: construct RunitInput (region, credential, props, userAgent)
Runit->>DockerImageBuilder: build(..., enableSsh=debugInstance, props, runtime)
DockerImageBuilder-->>Runit: return image metadata
Runit-->>BuilderFactory: { image }
BuilderFactory-->>CLI: return deployed image info
else no YAML
BuilderFactory-->>CLI: continue existing builder selection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 58: Update the dependency entry for
"`@serverless-devs/docker-image-builder`" in package.json to remove the caret and
pin the exact pre-release version "0.0.0-20260518-164317-160dd89efac1" so
installs are reproducible; locate the dependency key
"`@serverless-devs/docker-image-builder`" and replace the version string
"^0.0.0-20260518-164317-160dd89efac1" with the exact version string
"0.0.0-20260518-164317-160dd89efac1".
In `@src/commands-help/build.ts`:
- Around line 12-13: Add the two new CLI flags shown in the example usage to the
help.option table so they appear in help output: add entries for
"--cloud-build-config" (with a short description like "Path to cloud build
config file") and "--debug-instance" (with a description like "Enable debug
instance for cloud build") in the help.option structure inside
src/commands-help/build.ts so they are listed alongside the other options when
using the help command.
In `@src/index.ts`:
- Line 136: The debug log currently prints the entire inputs object via
logger.debug(`inputs: ${JSON.stringify(inputs, null, 2)}`) which may leak
secrets; change this to only log a redacted or whitelisted subset: create a
sanitized copy (e.g., redact keys like apiKey, password, token) or build an
object with only non-sensitive fields and log that instead; update the call to
logger.debug to use the sanitizedInputs (or whitelist object) and keep the
original inputs variable untouched; reference logger.debug and the inputs
variable when making the change.
In `@src/subCommands/build/index.ts`:
- Around line 81-82: The code is logging full build configuration which can leak
secrets; add a sanitizer (e.g., sanitizeConfig or maskSecrets) and use it
wherever buildConfig is logged (the logger.debug calls in the build subcommand,
including the existing Build config log and the later log at the other call
site) to redact fields such as AccessKeySecret, password, token, secret,
privateKey, registryPassword, and any keys matching /key|secret|token|password/i
before calling JSON.stringify; update the logger.debug invocations to serialize
the sanitized object instead of the raw buildConfig.
- Around line 75-79: Guard against a null/empty cloudBuildYaml path before
calling fs.readFileSync: check this.cloudBuildYamlAbsPath (assigned to
buildYamlPath) for null/undefined/empty string and throw or return a clear,
actionable error (e.g., "cloud build yaml path not provided") before attempting
to read; then proceed to call fs.readFileSync and parse into buildConfig
(yaml.load) only after the presence check passes so readFileSync cannot throw
due to a missing path.
In `@src/subCommands/build/runit/index.ts`:
- Around line 22-26: The code is passing a potentially relative build.code
(aliased to codeUri) into dir which depends on CWD; change the code to resolve
build.code to an absolute path against baseDir before using it (e.g., compute an
absoluteCode = path.resolve(baseDir, codeUri || "") or path.join(baseDir,
codeUri) and pass absoluteCode as dir), updating both occurrences where dir:
codeUri is used (look for codeUri and the this.inputs.props.build destructure
and the subsequent object that sets dir) so relative cloud-build config paths
work in non-default execution contexts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaf98d27-1432-40ff-bca5-afd1b2394a2c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonpublish.yamlsrc/commands-help/build.tssrc/index.tssrc/interface/index.tssrc/subCommands/build/index.tssrc/subCommands/build/runit/index.tssrc/subCommands/build/runit/interface/index.ts
|
|
||
| public async build(inputs: IInputs) { | ||
| await super.handlePreRun(inputs, false); | ||
| logger.debug(`inputs: ${JSON.stringify(inputs, null, 2)}`); |
There was a problem hiding this comment.
Do not debug-log the full inputs payload.
This can leak credentials/secrets into logs. Log only non-sensitive fields (or a redacted clone).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` at line 136, The debug log currently prints the entire inputs
object via logger.debug(`inputs: ${JSON.stringify(inputs, null, 2)}`) which may
leak secrets; change this to only log a redacted or whitelisted subset: create a
sanitized copy (e.g., redact keys like apiKey, password, token) or build an
object with only non-sensitive fields and log that instead; update the call to
logger.debug to use the sanitizedInputs (or whitelist object) and keep the
original inputs variable untouched; reference logger.debug and the inputs
variable when making the change.
| const buildYamlPath = this.cloudBuildYamlAbsPath; | ||
|
|
||
| try { | ||
| const buildYamlContent = fs.readFileSync(buildYamlPath, 'utf8'); | ||
| const buildConfig = yaml.load(buildYamlContent) as any; |
There was a problem hiding this comment.
Add an explicit null/empty guard for buildYamlPath before readFileSync.
this.cloudBuildYamlAbsPath is nullable; calling readFileSync without a guard can throw a non-actionable runtime error.
Suggested diff
async runit() {
const buildYamlPath = this.cloudBuildYamlAbsPath;
+ if (!buildYamlPath) {
+ throw new Error('Cloud build config path is not initialized. Call findCloudBuildYaml() first.');
+ }
try {
const buildYamlContent = fs.readFileSync(buildYamlPath, 'utf8');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/subCommands/build/index.ts` around lines 75 - 79, Guard against a
null/empty cloudBuildYaml path before calling fs.readFileSync: check
this.cloudBuildYamlAbsPath (assigned to buildYamlPath) for null/undefined/empty
string and throw or return a clear, actionable error (e.g., "cloud build yaml
path not provided") before attempting to read; then proceed to call
fs.readFileSync and parse into buildConfig (yaml.load) only after the presence
check passes so readFileSync cannot throw due to a missing path.
| logger.debug(`Build config: ${JSON.stringify(buildConfig, null, 2)}`); | ||
|
|
There was a problem hiding this comment.
Stop logging full configs/inputs because they expose secrets.
These logs can leak AccessKeySecret, registry passwords, and other sensitive fields into debug output. Mask or omit sensitive keys before logging.
Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/subCommands/build/index.ts` around lines 81 - 82, The code is logging
full build configuration which can leak secrets; add a sanitizer (e.g.,
sanitizeConfig or maskSecrets) and use it wherever buildConfig is logged (the
logger.debug calls in the build subcommand, including the existing Build config
log and the later log at the other call site) to redact fields such as
AccessKeySecret, password, token, secret, privateKey, registryPassword, and any
keys matching /key|secret|token|password/i before calling JSON.stringify; update
the logger.debug invocations to serialize the sanitized object instead of the
raw buildConfig.
| code: codeUri, | ||
| setup, | ||
| timeoutMinutes, | ||
| baseContainerConfig | ||
| } = this.inputs.props.build; |
There was a problem hiding this comment.
Resolve build.code to an absolute path before passing dir.
dir: codeUri depends on current working directory. Relative paths from cloud-build config can break in non-default execution contexts; resolve against baseDir first.
Suggested diff
+import path from 'path';
...
- await build({
+ const resolvedCodeDir = path.isAbsolute(codeUri) ? codeUri : path.resolve(this.inputs.baseDir, codeUri);
+
+ await build({
...
- dir: codeUri,
+ dir: resolvedCodeDir,Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/subCommands/build/runit/index.ts` around lines 22 - 26, The code is
passing a potentially relative build.code (aliased to codeUri) into dir which
depends on CWD; change the code to resolve build.code to an absolute path
against baseDir before using it (e.g., compute an absoluteCode =
path.resolve(baseDir, codeUri || "") or path.join(baseDir, codeUri) and pass
absoluteCode as dir), updating both occurrences where dir: codeUri is used (look
for codeUri and the this.inputs.props.build destructure and the subsequent
object that sets dir) so relative cloud-build config paths work in non-default
execution contexts.
mozhou52
force-pushed
the
build-runit
branch
3 times, most recently
from
350e78e to
583cefb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 58: The package.json currently pins a pre-release dependency
"`@serverless-devs/docker-image-builder`" at version
"0.0.0-20260518-164317-160dd89efac1"; replace this pre-release with a stable
semver release (or a caret/tilde range to the intended stable version) if this
is meant for production, or add a clear comment in package.json / project docs
explaining why this pre-release is required and a migration plan to a stable
release; ensure any CI/CD or lockfile updates are committed so the change is
effective.
- Line 57: Update the "undici" dependency entry in package.json from the pinned
^5.28.5 to a current 8.x version (e.g., ^8.3.0 or the latest), then run your
package manager to refresh lockfiles (npm install / npm update or yarn install)
and run the test suite to ensure no breaking changes; target the "undici"
dependency line in package.json to make this change and verify any code that
imports undici remains compatible with the newer major version.
In `@src/subCommands/build/runit/interface/index.ts`:
- Around line 4-29: Remove the redundant top-level region property from the
RunitInput interface: delete the region field in the RunitInput declaration so
callers use props.region consistently; update any construction/assignment code
that currently duplicates props.region into RunitInput (the place that populates
RunitInput) to stop setting that top-level region, and ensure all code continues
to read region from props.region only (reference: RunitInput and IProps.region).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1558d55-ce2c-464d-ba13-fe508ea4e8d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonpublish.yamlsrc/commands-help/build.tssrc/index.tssrc/interface/index.tssrc/subCommands/build/index.tssrc/subCommands/build/runit/index.tssrc/subCommands/build/runit/interface/index.ts
✅ Files skipped from review due to trivial changes (1)
- publish.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/interface/index.ts
- src/commands-help/build.ts
- src/subCommands/build/runit/index.ts
- src/index.ts
- src/subCommands/build/index.ts
| "temp-dir": "^2.0.0", | ||
| "tty-table": "^4.2.3", | ||
| "uuid": "^9.0.1", | ||
| "undici": "^5.28.5", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of undici npm package and are there any known security vulnerabilities in version 5.28.5?
💡 Result:
Latest version (as of 2026-05-19): undici 8.3.0 is the latest published version on npm [1]. Known security vulnerabilities in undici 5.28.5: Snyk’s vulnerability advisory indicates multiple issues affecting undici versions earlier than 5.28.5 (e.g., Insecure Randomness via Math.random boundary selection, and others) and states that they are fixed by upgrading to 5.28.5 (or higher) [2]. Additionally, the undici project’s own “Security Release” notes for v5.28.3 mention a CVE-2024-24758 related to Proxy-Authorization header not cleared on cross-origin redirect, which would be relevant to that older patch line [3]. For undici specifically at 5.28.5, Snyk does not show a dedicated page in the retrieved sources that lists “direct vulnerabilities” for exactly 5.28.5; however, the above Snyk findings clearly indicate that the high-level issues disclosed for nearby 5.28.x versions are addressed by upgrading to 5.28.5 [2]. If you need an exhaustive CVE list that explicitly enumerates every issue affecting exactly 5.28.5, I can run one more targeted search focused on “undici@5.28.5 vulnerabilities / CVEs” in a subsequent step.
Citations:
- 1: https://www.npmjs.com/package/undici
- 2: https://security.snyk.io/package/npm/undici/5.28.2
- 3: https://togithub.com/nodejs/undici/releases/tag/v5.28.3
Update undici to a current version.
The undici dependency is pinned to version ^5.28.5, but the latest version is 8.3.0. This is 3 major versions behind. While 5.28.5 itself has no known vulnerabilities, the significant version gap means missing important security patches, bug fixes, and improvements. Update to version 8.x or the latest available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 57, Update the "undici" dependency entry in
package.json from the pinned ^5.28.5 to a current 8.x version (e.g., ^8.3.0 or
the latest), then run your package manager to refresh lockfiles (npm install /
npm update or yarn install) and run the test suite to ensure no breaking
changes; target the "undici" dependency line in package.json to make this change
and verify any code that imports undici remains compatible with the newer major
version.
| "tty-table": "^4.2.3", | ||
| "uuid": "^9.0.1", | ||
| "undici": "^5.28.5", | ||
| "@serverless-devs/docker-image-builder": "0.0.0-20260518-164317-160dd89efac1", |
There was a problem hiding this comment.
Consider stability implications of using a pre-release dependency.
The @serverless-devs/docker-image-builder dependency uses a pre-release version (0.0.0-20260518-164317-160dd89efac1). Pre-release versions may lack stability guarantees, could contain undocumented breaking changes, and may not receive long-term support.
If this is for testing or development purposes, ensure it's replaced with a stable release before production deployment. If the pre-release is intentional, document the rationale and plan for migrating to a stable version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 58, The package.json currently pins a pre-release
dependency "`@serverless-devs/docker-image-builder`" at version
"0.0.0-20260518-164317-160dd89efac1"; replace this pre-release with a stable
semver release (or a caret/tilde range to the intended stable version) if this
is meant for production, or add a clear comment in package.json / project docs
explaining why this pre-release is required and a migration plan to a stable
release; ensure any CI/CD or lockfile updates are committed so the change is
effective.
| export interface RunitInput extends _IInputs { | ||
| region: string; | ||
| credential: ICredentials; | ||
| props: IProps; | ||
| userAgent: string; | ||
| baseDir: string; | ||
| } | ||
|
|
||
| export interface IProps { | ||
| region: string; | ||
| build: { | ||
| setup: string; | ||
| code: string; | ||
| timeoutMinutes?: number; | ||
| buildSpec?: string; | ||
| baseContainerConfig?: ICustomContainerConfig; | ||
| }; | ||
| runtime: { | ||
| registryMode?: 'oci' | 'fc-registry'; | ||
| image: string; | ||
| username?: string; | ||
| password?: string; | ||
| cpu?: string | number; | ||
| memory?: string | number; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check usage of region fields in RunitInput and IProps to determine if duplication is intentional
# Search for accesses to top-level region
echo "=== Top-level region usage (input.region) ==="
rg -nP --type=ts -C3 '\binput\.region\b' src/subCommands/build/
# Search for accesses to nested props.region
echo -e "\n=== Nested props.region usage (props.region or input.props.region) ==="
rg -nP --type=ts -C3 '\bprops\.region\b' src/subCommands/build/
# Search for RunitInput construction/usage
echo -e "\n=== RunitInput construction sites ==="
ast-grep --pattern 'const $_ = {
$$$
region: $_,
$$$
props: {
$$$
region: $_,
$$$
}
$$$
}'Repository: devsapp/fc3
Length of output: 1095
🏁 Script executed:
# Search more comprehensively for uses of the top-level region field
# Check if this.inputs.region or input.region appears anywhere in the codebase
echo "=== Searching for top-level region field access ==="
rg -nP --type=ts '\b(this\.inputs\.region|input\.region)\b' src/subCommands/build/
echo -e "\n=== Check RunitInput usage context ==="
rg -nP --type=ts -B5 -A5 'RunitInput' src/subCommands/build/ | head -60
echo -e "\n=== Verify if top-level region is used in Runit class methods ==="
rg -nP --type=ts 'this\.inputs\.region' src/Repository: devsapp/fc3
Length of output: 2657
Remove the unused region field from RunitInput interface.
The region field at the top level of RunitInput (line 5) is never accessed anywhere in the codebase. The implementation always reads from props.region instead. The top-level field is populated from props.region during construction (line 101 in src/subCommands/build/index.ts) but never used, creating redundant duplication. Remove this field to improve clarity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/subCommands/build/runit/interface/index.ts` around lines 4 - 29, Remove
the redundant top-level region property from the RunitInput interface: delete
the region field in the RunitInput declaration so callers use props.region
consistently; update any construction/assignment code that currently duplicates
props.region into RunitInput (the place that populates RunitInput) to stop
setting that top-level region, and ensure all code continues to read region from
props.region only (reference: RunitInput and IProps.region).
5680c42 to
4bb9fa3
Compare
2 participants
fix: build runit
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests