[bpa] use the env var for sticky disk grpc port

[aayushshah15]

Important

Use environment variable for gRPC port in createBlacksmithAgentClient and update GitHub Actions and dependencies.

• Environment Variables:
  • createBlacksmithAgentClient in reporter.ts now uses process.env.BLACKSMITH_STICKY_DISK_GRPC_PORT for the gRPC port, defaulting to '5557'.
• GitHub Actions:
  • Update actions/checkout to v4 and actions/setup-node to v4 in build.yml and verify-build.yml.
  • Change Node.js version from '16' to '20' in both workflows.
• Dependencies:
  • Add axios version ^1.8.4 to package.json.

^{This description was created by for 207af33. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to fa55564 in 1 minute and 5 seconds

More details

• Looked at 15 lines of code in 1 files
• Skipped 2 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. src/reporter.ts:37

• Draft comment:
  Consider validating or casting the port value extracted from process.env (currently a string) to ensure it's a valid port number. Using a string is acceptable for template literals, but explicit conversion or validation might prevent runtime issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The port is either '5557' (hardcoded default) or comes from an environment variable. The value is only used in a URL string template. gRPC clients typically handle string ports fine. Adding parseInt() wouldn't provide much value since the default is already a valid port string. If an invalid port was provided via env var, the error would surface quickly during connection.
  The suggestion could prevent edge cases where invalid ports are provided via environment variables. Type safety is generally good practice.
  The current code is already safe - invalid ports will fail fast, the default is valid, and string ports are commonly used in Node.js networking. The suggestion adds complexity without clear benefit.
  Delete the comment as it suggests additional validation that isn't clearly necessary and the current code follows common Node.js patterns for port handling.

2. src/reporter.ts:37

• Draft comment:
  Consider validating that BLACKSMITH_STICKY_DISK_GRPC_PORT is a valid numeric port to avoid misuse.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The port is used in a URL string, not in any numeric operations. The default value '5557' is hardcoded and known good. Even invalid port numbers would still work in a URL string - they just wouldn't connect. The gRPC client library would handle connection failures appropriately. Adding Number() conversion doesn't provide meaningful validation.
  The comment raises a valid point about input validation. Invalid ports could cause connection issues that might be hard to debug.
  However, the suggested fix doesn't actually validate the port - it just converts to a number. A proper port validation would need range checks (1-65535). The current string approach is fine for URL construction.
  The comment should be deleted. The suggested change doesn't improve validation, and the current string-based approach is appropriate for URL construction.

3. src/reporter.ts:39

• Draft comment:
  Optionally, consider externalizing the host IP to an environment variable for greater configuration flexibility.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_8BZYycFxPhnUSwVx

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[adityamaru]

CI is failing @aayushshah15 , needs a regen?

[aayushshah15]

There's no diff upon regen locally. I'll check what's up in a bit

[ellipsis-dev]

👍 Looks good to me! Incremental review on 0fecb39 in 34 seconds

More details

• Looked at 42 lines of code in 3 files
• Skipped 2 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. package.json:35

• Draft comment:
  Added axios dependency with version ^1.8.4. Ensure this isn't duplicating or conflicting with existing usage.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. src/reporter.ts:37

• Draft comment:
  Good use of dynamic port from env var. Consider validating that the port value is numeric to avoid malformed URLs.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. src/setup_builder.ts:174

• Draft comment:
  Avoid using inline type casts for response properties; consider defining an interface for proper type safety.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. package.json:35

• Draft comment:
  Verify axios addition: Ensure this version doesn't clash with any transitive dependencies.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. src/reporter.ts:37

• Draft comment:
  Consider validating that BLACKSMITH_STICKY_DISK_GRPC_PORT is a valid numeric port.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

6. src/setup_builder.ts:174

• Draft comment:
  Avoid inline type assertions; define an interface for the response to improve type safety.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_v67U0GxhSKYQXL91

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on a83d951 in 45 seconds

More details

• Looked at 80 lines of code in 5 files
• Skipped 2 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. .github/workflows/build.yml:7

• Draft comment:
  Upgrade to checkout@v4 and setup-node@v4 with node '18' is valid. Confirm these versions are intentionally used.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. .github/workflows/verify-build.yml:19

• Draft comment:
  Upgrade node version to '18' here as well; ensure consistency with build workflow.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. package.json:35

• Draft comment:
  Addition of axios dependency is appropriate since it's imported in reporter.ts.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. src/reporter.ts:37

• Draft comment:
  Port configuration updated to use env variable; ensure BLACKSMITH_STICKY_DISK_GRPC_PORT is documented.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. src/setup_builder.ts:174

• Draft comment:
  Using inline type assertions for response. Consider defining an interface for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

6. .github/workflows/build.yml:7

• Draft comment:
  Upgraded checkout and setup-node actions to v4 and switched to Node 18. Verify that these changes are fully compatible with your project’s requirements.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

7. .github/workflows/verify-build.yml:13

• Draft comment:
  The verify-build workflow still uses checkout@v3. Consider upgrading it to v4 for consistency across workflows.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

8. package.json:33

• Draft comment:
  An explicit axios dependency (v^1.8.4) was added. Confirm that this version aligns with axios-retry and usage in your code.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

9. src/reporter.ts:37

• Draft comment:
  Using the BLACKSMITH_STICKY_DISK_GRPC_PORT env variable to configure the gRPC port increases flexibility. Ensure the environment variable is correctly set in deployment.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

10. src/setup_builder.ts:174

• Draft comment:
  Type casting the response fields in getStickyDisk could mask unexpected data. Consider validating that the response object contains the expected properties before using them.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

Workflow ID: wflow_WAqxa94H70Fu5YjM

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 48bab0a in 1 minute and 20 seconds

More details

• Looked at 90 lines of code in 5 files
• Skipped 2 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. .github/workflows/build.yml:7

• Draft comment:
  Good update to checkout and setup-node actions; ensure v4 changes are compatible with all workflows.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. .github/workflows/verify-build.yml:13

• Draft comment:
  Consistent upgrade to v4 actions is noted; check that fetching the correct ref is still valid.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. package.json:35

• Draft comment:
  Added axios dependency; verify version ^1.8.4 aligns with other packages using axios.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. src/reporter.ts:37

• Draft comment:
  Using env var BLACKSMITH_STICKY_DISK_GRPC_PORT to configure baseUrl is good; consider validating it's a valid port number.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. src/setup_builder.ts:174

• Draft comment:
  Type assertions on response in getStickyDisk may hide missing properties; consider using proper type definitions or validation.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
  The comment raises a valid point about type safety. Type assertions can indeed mask missing properties and lead to runtime errors. However, the code has fallbacks to empty strings with || '', suggesting the author is aware of potential undefined values. Without seeing the client API types or implementation, we can't be certain if proper type definitions are possible or if assertions are necessary here.
  I might be too focused on the theoretical type safety concern while missing practical constraints that make type assertions necessary here. The existing fallback handling suggests this is a deliberate pattern.
  While type safety is important, the code shows defensive programming with fallbacks, and type assertions may be necessary when dealing with external APIs where full type information isn't available.
  Without more context about the client API types and why assertions are being used, this comment may be speculative and not actionable enough to be useful.

6. .github/workflows/build.yml:7

• Draft comment:
  Bumped checkout and setup-node actions and updated Node version to 18. Ensure that these changes maintain compatibility with your codebase.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

7. .github/workflows/verify-build.yml:13

• Draft comment:
  Updated checkout and setup-node actions and Node version. Verify these updates are fully compatible with your workflow.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

8. package.json:35

• Draft comment:
  Added axios dependency. Confirm version ^1.8.4 works seamlessly with axios-retry and other axios usage.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

9. src/reporter.ts:37

• Draft comment:
  Using the BLACKSMITH_STICKY_DISK_GRPC_PORT env var for the gRPC port is a good enhancement. Optionally consider validating the port value.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

10. src/setup_builder.ts:174

• Draft comment:
  The type assertions for response properties are repeated. Consider assigning the casted response to a variable to avoid redundancy.
• Reason this comment was not posted:
  Confidence changes required: 40% <= threshold 50%
  None

Workflow ID: wflow_cN2dPnflLH2cxWLH

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on 207af33 in 2 minutes and 32 seconds

More details

• Looked at 90 lines of code in 5 files
• Skipped 2 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. .github/workflows/build.yml:7

• Draft comment:
  Upgraded checkout and setup-node to v4 with Node 20. Ensure all dependencies and scripts are compatible with Node 20.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is related to dependency changes and asks the PR author to ensure compatibility, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written.

2. .github/workflows/verify-build.yml:13

• Draft comment:
  Updated checkout and Node setup actions to v4 and Node 20. Verify that these changes don't impact build caching or environment compatibility.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify that the changes don't impact build caching or environment compatibility, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.

3. package.json:35

• Draft comment:
  Added the 'axios' dependency at version ^1.8.4. Confirm that this version is compatible with existing usage (e.g. axios-retry).
• Reason this comment was not posted:
  Confidence changes required: 40% <= threshold 50%
  None

4. src/reporter.ts:37

• Draft comment:
  Using env var BLACKSMITH_STICKY_DISK_GRPC_PORT for port configuration. Ensure the environment variable is correctly set in all deployment environments.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

5. .github/workflows/build.yml:7

• Draft comment:
  Updated checkout and setup-node actions to v4 and node version to 20. Ensure these upgrades are fully compatible with your build steps.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

6. .github/workflows/build.yml:23

• Draft comment:
  File missing newline at end. Consider adding one to follow style guidelines.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

7. .github/workflows/verify-build.yml:13

• Draft comment:
  Updated checkout and setup-node usage (v4, node '20') in verify-build workflow. Confirm caching and ref handling are as expected.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

8. package.json:32

• Draft comment:
  New dependency 'axios' added. Verify that this version (^1.8.4) is needed and consistent with its usage in reporter.ts.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

9. src/reporter.ts:37

• Draft comment:
  Using an env var for gRPC port is a useful improvement. Consider parameterizing the host IP as well if future flexibility is needed.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

10. src/setup_builder.ts:174

• Draft comment:
  Explicit type casts on sticky disk response properties help with type safety, but consider validating the response structure to avoid masking unexpected changes.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_0WBVEsdFcXV3IHCy

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.