*: introduce a setup-only mode to the build-push-action

[adityamaru]

This setup-only mode will setup a docker builder with the stickydisk mounted but will not run a Docker build. The use case here is to allow customers to then run their custom Tilt files or Docker commands against our builder. The other subtle change is that we only cleanup in the post step of this builder action. It is still to be seen if you can start several of these builders at the same time in a workflow but we can do that as a follow on.

---

Important

Introduce setup-only mode in build-push-action to set up Docker builder without running a build, with updates to inputs, tests, and builder logic.

• Behavior:
  • Adds setup-only input in action.yml to set up Docker builder without running a build.
  • Updates startBlacksmithBuilder() in main.ts to handle setupOnly input, skipping build reporting if true.
  • Modifies builder setup logic in main.ts to set builder as global default if setupOnly is true.
• Testing:
  • Updates jest.config.ts to include module name mappings for mocks.
  • Adds mock implementations in src/__mocks__ for execa and Blacksmith services.
  • Updates tests in blacksmith-builder.test.ts to cover setupOnly scenarios.
• Misc:
  • Adds execa dependency in package.json.
  • Updates setup_builder.ts to use execa for starting buildkitd in detached mode when setupOnly is true.
  • Adds getUseBuilderArgs() in context.ts to support setting builder as global default.

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

[adityamaru]

Still need to test this but this is the shape of the change that I am thinking

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 0344a4e in 2 minutes and 35 seconds

More details

• Looked at 333 lines of code in 7 files
• Skipped 2 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  The new input is named setup-only here, but later in code it’s referenced as setupOnly. Ensure the naming is consistent (either use kebab-case or camelCase) to avoid potential input retrieval issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The reviewer claims there's an inconsistency but hasn't provided evidence showing where 'setupOnly' is actually used. The src/context.ts snippet shown doesn't contain this reference. Without seeing the actual inconsistent usage, we can't verify if this is a real issue. We should err on the side of assuming the implementation is correct unless proven otherwise.
  I might be missing other parts of the codebase where this inconsistency exists. The reviewer might have seen something I haven't.
  Even if the inconsistency exists somewhere else, we need clear evidence in the provided context to justify keeping this comment. Without it, we risk making incorrect assumptions.
  Delete the comment because we don't have clear evidence of the claimed inconsistency in the provided context.

2. action.yml:14

• Draft comment:
  Input key 'setup-only' is defined here, but the code fetches 'setupOnly'. Ensure consistent naming to avoid potential issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Without seeing the actual code that uses this parameter, I cannot verify if there's truly an inconsistency. The comment is making an assumption about code we can't see. According to the rules, if understanding the comment requires more context (such as seeing other files), we should delete it. This is exactly such a case.
  The inconsistency might actually exist in the code, and if it does, this would be a real issue that could cause runtime problems.
  While the issue could be real, we don't have enough context to verify it. The rules clearly state that we need STRONG EVIDENCE to keep a comment, and we should delete comments if understanding them requires more context.
  Delete the comment because we don't have enough context to verify the claimed inconsistency between the action.yml and the actual code usage.

3. src/context.ts:100

• Draft comment:
  The input is fetched via core.getBooleanInput('setupOnly') while YAML defines 'setup‐only'. Verify that the naming conversion is consistent.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment claims there's a YAML definition somewhere that uses 'setup-only', but I don't see this YAML file in the provided context. Without seeing the actual YAML definition, I can't verify if there's really a mismatch. The code is internally consistent in using 'setupOnly'. The comment is making assumptions about external files we can't see.
  I could be wrong if there's a YAML file somewhere that defines this input as 'setup-only'. Also, there might be a convention in this codebase to use kebab-case for input names that I'm not aware of.
  Even if there is a YAML file somewhere using 'setup-only', I don't have enough context to be certain about the correct naming. Without seeing the YAML, this comment is speculative.
  The comment should be deleted because it makes assumptions about files we can't see and lacks clear evidence that the change it suggests is correct.

4. src/main.ts:73

• Draft comment:
  In setup-only mode, dockerfilePath is not resolved and an empty string is passed to setupStickyDisk. Ensure this is the intended behavior and properly documented.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
  The comment is asking the author to ensure that the behavior is intended and properly documented. This violates the rule against asking the author to confirm their intention or ensure behavior. However, it does point out a specific issue with the code, which is that an empty string is passed to a function, which could be a potential problem. The comment could be rephrased to focus on the potential issue rather than asking for confirmation.

5. src/setup_builder.ts:330

• Draft comment:
  When setupOnly is true, the call to reporter.reportBuild is skipped. Confirm that passing an empty dockerfilePath ('') is safe and that no unintended reporting occurs.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment is speculative and asks for verification rather than pointing out a clear issue. The code shows that dockerfilePath is not used at all when setupOnly=true, so there's no risk. The comment is asking the author to double-check something that isn't actually a problem.
  Maybe there's some edge case where reportBuild() gets called even when setupOnly=true? Maybe there are other uses of dockerfilePath I'm missing?
  The code is very clear - the if statement completely guards the reportBuild() call, and dockerfilePath isn't used anywhere else in the function. There's no way for an empty dockerfilePath to cause issues when setupOnly=true.
  This comment should be deleted as it asks for verification of a non-issue and doesn't identify any actual problems that need fixing.

6. action.yml:19

• Draft comment:
  Typo: In the description for 'add-hosts', it currently reads 'List of a customs host-to-IP mapping'. Please change 'customs' to 'custom'.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. action.yml:25

• Draft comment:
  Typo: In the description for 'annotations', it reads 'List of annotation to set to the image'. For clarity and consistency, consider changing it to 'List of annotations to set to the image'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. src/main.ts:294

• Draft comment:
  Typographical issue: The variable name 'metadatadt' (line 294) seems to be a typo. Consider renaming it to a clearer name (e.g., 'metadataStr') to improve code readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_UQxDgLqNggavdaH4

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on d24b35c in 1 minute and 58 seconds

More details

• Looked at 332 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. src/context.ts:98

• Draft comment:
  Input naming mismatch: In action.yml the input is declared as 'setup‐only', but here it's retrieved as 'setupOnly'. Consider using core.getBooleanInput('setup‐only') to ensure consistency with the YAML definition.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment makes a reasonable point about naming consistency, but I can't verify if 'setup-only' is actually the correct name without seeing action.yml. The hyphenated version would be consistent with other hyphenated input names in the code like 'github-token', 'build-args', etc. However, there are also camelCase properties in the interface like nofallback.
  Without seeing action.yml, I cannot be 100% certain which naming convention is correct. The existing codebase shows mixed usage of both hyphenated and camelCase names.
  While the comment's suggestion aligns with the hyphenated pattern seen in other inputs, making this change without confirming the actual name in action.yml could introduce bugs.
  Since I cannot verify the actual input name in action.yml, and the comment requires this external context to validate, the comment should be removed.

2. action.yml:14

• Draft comment:
  Consider aligning the input key naming with code usage. The input is declared as 'setup‐only' here, but the code expects 'setupOnly'. Either update the YAML or adjust the code for consistent naming.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%

1. The comment makes an assumption about code that isn't shown in the diff or context. 2. Without seeing the actual code usage, we can't verify if there really is an inconsistency. 3. The hyphenated format 'setup-only' is actually a common convention in YAML/GitHub Actions. 4. We should err on the side of removing comments when we don't have strong evidence.
   The code usage might actually exist somewhere else in the codebase, and there could be a real inconsistency that needs to be fixed.
   Even if the code exists elsewhere, we don't have the evidence in front of us to verify the claim. Making assumptions about code we can't see violates our review principles.
   Delete the comment because we don't have concrete evidence of the claimed inconsistency, and making assumptions about unseen code could lead to incorrect suggestions.

3. action.yml:19

• Draft comment:
  Typographical suggestion: Update the description on line 19 to read "List of custom host-to-IP mappings (e.g., docker:10.180.0.1)" instead of "List of a customs host-to-IP mapping (e.g., docker:10.180.0.1)".
• Reason this comment was not posted:
  Comment was on unchanged code.

4. src/state-helper.ts:68

• Draft comment:
  There's a missing newline at the end of the file. Although this is a minor style issue, it's preferable to end files with a newline to conform with common best practices.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While technically correct, this is a very minor style issue. Most modern IDEs and git tools handle this automatically. It doesn't affect functionality. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category.
  The missing newline could cause issues with some tools or in some environments. It is a widely accepted best practice to end files with newlines.
  While true, this is exactly the kind of minor issue that should be handled by linters or IDE settings, not PR comments. It's too trivial for manual review.
  Delete this comment as it's too minor of an issue to warrant a PR comment. This should be handled by automated tools.

Workflow ID: wflow_wtlgURj5FLtZOmIh

---

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.

[ellipsis-dev]

In the commitStickyDisk function, the warning message on line 212 starts with a lowercase 'error'. For consistency with the other error messages in the file, consider capitalizing it (e.g., 'Error committing sticky disk:').

Suggested change

	core.warning('error committing sticky disk:', error);
	core.warning('Error committing sticky disk:', error);

[ellipsis-dev]

❌ Changes requested. Incremental review on a9e7d6d in 1 minute and 49 seconds

More details

• Looked at 332 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 15 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  Typo: Consider changing 'customs host-to-IP mapping' to 'custom host-to-IP mapping'.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. src/__tests__/blacksmith-builder.test.ts:48

• Draft comment:
  New test input 'setupOnly' is added. Add tests for setup-only enabled behavior.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. src/context.ts:61

• Draft comment:
  New property 'setupOnly' is added to Inputs. Verify naming consistency across the project.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify naming consistency, which is against the rules as it asks for confirmation or verification. It does not provide a specific suggestion or point out a specific issue.

4. src/main.ts:78

• Draft comment:
  Conditional check for setup-only mode skips dockerfile path resolution. Ensure users are aware of this behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that users are aware of a specific behavior, which violates the rule against asking the author to ensure something. It doesn't provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.

5. src/main.ts:213

• Draft comment:
  Early return in setup-only mode prevents further configuration. Verify cleanup steps in post are sufficient.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to verify cleanup steps, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue with the code.

6. src/reporter.ts:199

• Draft comment:
  commitStickyDisk function looks correct; consider adding debug logging if exposeId is empty.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The code already handles empty exposeId gracefully. Adding debug logging wouldn't change functionality. Looking at the rest of the file, empty exposeIds appear to be a normal case, not an error condition. The comment is more of a suggestion than pointing out a real issue. The first part of the comment just says the function "looks correct" which is not actionable.
  Maybe logging empty exposeIds could help with debugging issues in production. The codebase might have conventions around debug logging that I'm not aware of.
  The code already has error logging for actual failures. Adding debug logging for an expected empty value case would add noise without clear benefit. If debug logging was important, it would likely be consistent across all similar functions.
  The comment should be deleted as it's not pointing out a real issue - empty exposeIds are handled appropriately and additional logging would not improve the code.

7. src/setup_builder.ts:311

• Draft comment:
  The new 'setupOnly' parameter in setupStickyDisk correctly skips build reporting; confirm passing an empty dockerfilePath is acceptable.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to confirm if passing an empty dockerfilePath is acceptable. This falls under asking the author to confirm their intention, which is against the rules. Therefore, this comment should be removed.

8. src/state-helper.ts:62

• Draft comment:
  New functions setExposeId/getExposeId are added. Ensure state key naming is consistent across the action.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

9. action.yml:14

• Draft comment:
  New input 'setup-only' added. Ensure the documentation and default value are consistent with boolean inputs across the action.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

10. src/__tests__/blacksmith-builder.test.ts:46

• Draft comment:
  Updated test mock now includes 'setupOnly'. Consider adding dedicated tests for setup-only mode behavior.
• 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 setupOnly flag was clearly added as part of this change, which suggests it's a new feature. Not having tests for a new feature flag is a legitimate concern. However, we don't have enough context to know if setup-only mode actually changes any behavior that would need testing. The comment is speculative without more evidence.
  I don't see any evidence in this file that setupOnly actually affects any behavior - it could be an unused flag or handled elsewhere. Without seeing the implementation, I can't be certain tests are missing.
  While true that we don't see the implementation, adding a new flag to mockInputs strongly implies it's used somewhere and affects behavior. Test coverage is important for new features.
  Given the rules emphasize only keeping comments with strong evidence, and we can't see how setupOnly is used, we should err on the side of removing this speculative comment.

11. src/reporter.ts:198

• Draft comment:
  The new commitStickyDisk function is a clear addition. Consider validating environment variables (e.g. GITHUB_REPO_NAME, VM_ID) before making the gRPC call to improve error clarity.
• 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%

1. The code already handles missing env vars by defaulting to empty strings. 2. This pattern is used consistently throughout the codebase. 3. There's error handling via try-catch. 4. The suggestion would add complexity without clear benefit. 5. The current approach seems intentional rather than an oversight.
   The comment might have a point about error clarity - empty strings could lead to confusing errors later. Also, some of these env vars might be truly required for the function to work.
   While validation could help, the consistent use of fallbacks and error handling throughout the codebase suggests this is an intentional pattern. The try-catch will catch any resulting errors.
   The comment should be deleted as it suggests changing an intentional pattern that's consistently used throughout the codebase, and the existing error handling is sufficient.

12. src/state-helper.ts:62

• Draft comment:
  New functions setExposeId and getExposeId are straightforward. Ensure their usage is consistently tracked across the action for reliable state management.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure consistent tracking of certain functions, which falls under the category of asking the author to ensure something is tested or verified. This violates the rules.

13. action.yml:19

• Draft comment:
  Typo found in the description for 'add-hosts' input: consider replacing 'customs' with 'custom' (or 'custom host-to-IP mapping') for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

14. action.yml:25

• Draft comment:
  Typo in the 'annotations' input description: consider revising 'List of annotation to set to the image' to 'List of annotations to set on the image' for grammatical correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. src/main.ts:294

• Draft comment:
  Typo alert: The variable name 'metadatadt' (line 294) appears to be a typographical error. Consider renaming it to something clearer like 'metadataStr' or simply 'metadata' for better clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_ulGGm9UPTbZcXhrV

---

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.

[ellipsis-dev]

Consider adding explicit tests covering the setupOnly behavior in setupStickyDisk.

[ellipsis-dev]

❌ Changes requested. Incremental review on a16ab71 in 1 minute and 39 seconds

More details

• Looked at 401 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 19 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  Ensure consistency between the input name 'setup-only' (in YAML) and its mapping to 'setupOnly' in code. Consider documenting this mapping for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment makes assumptions about code we can't see. We don't have access to the JavaScript code where this parameter might be used. The comment is asking for verification rather than pointing out a concrete issue. It's speculative and asks for documentation without clear evidence of a problem.
  What if there actually is an inconsistency in the code that could cause runtime issues? What if this is a common source of bugs in GitHub Actions?
  Without seeing the actual code where this parameter is used, we can't verify if there's a real issue. The comment is speculative and asks for verification rather than pointing out a concrete problem.
  Delete this comment as it violates multiple rules: it asks for verification, is speculative, and lacks concrete evidence of an actual issue.

2. main.ts:94

• Draft comment:
  Prefer using core.info() instead of console.log() for logging to ensure consistency with action logging.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. setup_builder.ts:118

• Draft comment:
  In setup-only mode, the buildkitd process is spawned detached and unreffed; ensure that any errors from the detached process are appropriately monitored since they won’t propagate to the parent process.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. setup_builder.ts:347

• Draft comment:
  When setup-only is true, the code bypasses calling reporter.reportBuild; consider explicitly logging or handling the absence of report data to avoid confusion.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. setup_builder.ts:328

• Draft comment:
  Consider handling AbortController cancellations explicitly. An aborted controller may throw an error that isn't clearly identified as such.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. state-helper.ts:62

• Draft comment:
  Ensure that state values for 'exposeId' are consistently saved and retrieved. The added getter/setter pair seems correct; verify usage in all modules.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

7. action.yml:14

• Draft comment:
  The new 'setup-only' input is clearly added. It’s mapped to a boolean by default. Good job keeping consistency with other inputs.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

8. src/__tests__/blacksmith-builder.test.ts:47

• Draft comment:
  The test initialization now includes the 'setupOnly' property. It would be beneficial to add tests that specifically validate the behavior when setupOnly is true.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

9. src/context.ts:59

• Draft comment:
  The addition of the setupOnly property in the Inputs interface is straightforward. The naming conversion (from 'setup-only' to setupOnly) is handled consistently.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

10. src/main.ts:78

• Draft comment:
  Skipping dockerfile path resolution when setupOnly is true is appropriate. Consider using core.info instead of console.log for logging the exposeId for consistency with other logging calls.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

11. src/main.ts:213

• Draft comment:
  The early return for setup-only mode (after builder setup) correctly prevents further configuration and the actual build run. This is clear and meets the intended design.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

12. src/reporter.ts:199

• Draft comment:
  The new function commitStickyDisk follows the existing pattern. Consider optionally verifying that exposeId is non-empty before attempting commit, to avoid reporting with an empty identifier.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

13. src/setup_builder.ts:112

• Draft comment:
  In startBuildkitd, using detached mode with unref() for setup-only mode is implemented correctly. This ensures the build-push process can exit while leaving buildkitd running.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

14. src/setup_builder.ts:344

• Draft comment:
  In setupStickyDisk, the logic to skip reporting the build (and thus omitting the buildId) in setup-only mode is correctly implemented. Ensure that an empty dockerfilePath passed under setup-only mode doesn’t lead to unintended side effects in reporter.reportBuild.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

15. src/state-helper.ts:62

• Draft comment:
  The addition of setExposeId and getExposeId functions integrates well with the new setup-only mode. No issues detected.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

16. action.yml:19

• Draft comment:
  Typo: in the description on line 19, it says 'customs host-to-IP mapping'. Consider changing 'customs' to 'custom' to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

17. action.yml:22

• Draft comment:
  Typo: in the description on line 22, 'List of extra privileged entitlement' should probably be 'List of extra privileged entitlements' to indicate multiple items.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. action.yml:25

• Draft comment:
  Typo: in the description on line 25, it says 'List of annotation to set to the image'. Consider changing 'annotation' to 'annotations' for grammatical consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

19. src/state-helper.ts:68

• Draft comment:
  The file is missing a newline at the end. While this is a minor stylistic issue, it's generally good practice to end files with a newline to adhere to standard conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at the end of files is a common convention, this is a very minor issue that would typically be handled by editor settings or linting rules. It doesn't affect functionality and is more of a stylistic preference. Most modern IDEs automatically add this newline.
  The comment is technically correct and points out a real convention. Missing newlines can cause issues with some tools and make diffs less clean.
  However, this is exactly the kind of minor, obvious issue that should be handled by automation rather than code review comments. It doesn't warrant human attention.
  Delete this comment as it's too minor and would be better handled by linting rules or editor settings.

Workflow ID: wflow_stXI94vm9eVptJAr

---

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.

[ellipsis-dev]

There's a minor inconsistency in the error message in the commitStickyDisk function. The warning message at line 212 uses a lowercase 'error committing sticky disk:' while similar messages in this file start with a capital letter (e.g., 'Error reporting build completed:'). Consider updating it to 'Error committing sticky disk:' for consistency.

Suggested change

	core.warning('error committing sticky disk:', error);
	core.warning('Error committing sticky disk:', error);

[ellipsis-dev]

❌ Changes requested. Incremental review on 5e492a9 in 1 minute and 57 seconds

More details

• Looked at 402 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 18 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  Input 'setup-only' added with default 'false'. Consider documenting that it's treated as a boolean despite the string default.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This appears to be following an established pattern in the file where boolean flags use string 'false' defaults. The comment suggests adding documentation about boolean treatment, but this seems unnecessary since: 1) It's a common pattern in GitHub Actions, 2) It's consistent with other parameters in this file, 3) The description already implies boolean behavior by saying "defaults to false".
  The comment might be pointing out a legitimate documentation concern that could help new users understand the parameter behavior better.
  The boolean treatment is already clear from context and follows GitHub Actions conventions. Adding explicit documentation about type conversion would be redundant and potentially cluttering.
  The comment should be deleted as it suggests adding unnecessary documentation for a standard pattern that's already clear from context.

2. src/main.ts:78

• Draft comment:
  When setupOnly is true, dockerfilePath is bypassed. Ensure this behavior is clearly documented for maintainability.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that a behavior is documented, which falls under the rule of not asking the author to ensure something is done. It doesn't provide a specific code suggestion or point out a potential issue with the code itself.

3. src/main.ts:213

• Draft comment:
  Early return in setup-only mode is clear, but consider explicitly logging or testing the exit path for clarity.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. src/reporter.ts:195

• Draft comment:
  Function commitStickyDisk appears correct; consider adding JSDoc comments for the new parameter 'shouldCommit' for clarity.
• 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%
  While documentation is generally good, this seems like a fairly straightforward boolean parameter with a clear name and default value. The function itself is also quite simple and its purpose is clear from the name. The parameter name 'shouldCommit' is self-documenting. Adding JSDoc here would be nice-to-have but not critical.
  Documentation helps future maintainers understand the code better. The exact meaning of 'shouldCommit' might not be as obvious to others as it seems.
  While documentation is valuable, we should avoid nitpicky comments that don't materially improve code quality. The boolean parameter name and default value make its purpose quite clear.
  This comment, while well-intentioned, is more of a nice-to-have suggestion rather than a critical code quality issue that needs to be addressed.

5. src/setup_builder.ts:108

• Draft comment:
  Good to see detached mode when 'setup-only' is true. Verify that unref() suffices for process cleanup in all environments.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify the behavior of unref() in different environments, which violates the rule against asking the author to confirm or ensure behavior. It does not provide a specific suggestion or point out a clear issue.

6. src/setup_builder.ts:345

• Draft comment:
  In setupStickyDisk, passing an empty dockerfilePath when in setup-only mode is intentional. Consider adding inline documentation to clarify why reporting is skipped.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

7. src/state-helper.ts:62

• Draft comment:
  setExposeId and getExposeId added for sticky disk tracking. Looks good; ensure that state is reliably persisted in your CI environment.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that state is reliably persisted in the CI environment. This falls under the category of asking the author to ensure something is tested or verified, which is against the rules.

8. src/context.ts:99

• Draft comment:
  Ensure the new 'setupOnly' input is correctly parsed and defaults to false. Adding tests to cover boolean parsing would improve robustness.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The core.getBooleanInput() function is a well-tested part of the GitHub Actions toolkit that already handles boolean parsing and defaults. Adding || false would be redundant since getBooleanInput already handles this. The suggestion to add tests could be useful, but that's a separate concern from the actual code change and shouldn't block this PR.
  I could be wrong about how core.getBooleanInput works - maybe it doesn't handle defaults properly. Also, tests are generally good practice.
  Looking at @actions/core documentation confirms getBooleanInput handles boolean parsing and defaults properly. While tests are good, that's a separate enhancement that shouldn't block this simple input addition.
  The comment should be deleted as it suggests redundant code changes and makes testing suggestions that, while valid, are separate from this specific code change.

9. src/main.ts:77

• Draft comment:
  In setup‐only mode, dockerfilePath resolution is skipped. Consider adding a debug log here to clarify that behavior for maintainers.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

10. src/main.ts:212

• Draft comment:
  An early return is used when setup‐only mode is enabled. A clarifying inline comment here would help future maintainers understand that this branch omits subsequent build setup steps.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

11. src/setup_builder.ts:118

• Draft comment:
  In setup‐only mode, buildkitd is spawned as a detached process. Ensure that unref() is sufficient for managing the child process lifecycle and consider logging the spawned process's PID for debugging purposes.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

12. src/setup_builder.ts:347

• Draft comment:
  When setup-only is true, the build reporting is skipped. An inline comment explaining this conditional logic would help clarify why the reportBuild call is omitted.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

13. src/reporter.ts:199

• Draft comment:
  Consider logging detailed results or errors in commitStickyDisk to aid in debugging if the sticky disk commit fails.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

14. src/state-helper.ts:62

• Draft comment:
  The new setExposeId and getExposeId functions look correct; consider adding tests or documentation to ensure state is preserved as expected across action steps.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

15. action.yml:19

• Draft comment:
  Typo detected: In the description for the 'add-hosts' input, the phrase 'List of a customs host-to-IP mapping' appears to contain a typo. Consider changing 'customs' to 'custom'.
• Reason this comment was not posted:
  Comment was on unchanged code.

16. action.yml:25

• Draft comment:
  Typo detected: In the description for the 'annotations' input, the phrase 'List of annotation to set to the image' should likely be corrected to 'List of annotations to set to the image'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

17. src/main.ts:294

• Draft comment:
  Typo alert: The variable 'metadatadt' (line 294) appears to be a typographical error. Consider renaming it to something more descriptive and consistent, such as 'metadataStr' or simply 'metadataData'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. src/state-helper.ts:68

• Draft comment:
  Minor style issue: the file is missing a trailing newline at the end. Please add a newline at the end of the file.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This is a very minor formatting issue. Most IDEs and code formatters automatically handle trailing newlines. It's not related to logic or functionality. It's the kind of thing that should be handled by automated tools rather than manual review comments. The rules specifically say not to make obvious or unimportant comments.
  The missing newline could potentially cause issues with some tools or in some environments. It is a widely accepted best practice to end files with newlines.
  While it is a best practice, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than PR comments. If it's important to the project, it should be enforced by a linter.
  This comment should be deleted as it's too minor and would be better handled by automated tooling.

Workflow ID: wflow_gsEKdyagdl8ZFx0x

---

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.

[ellipsis-dev]

Consider adding explicit test cases for setup‐only mode to verify that no build is triggered when setupOnly is true.

[ellipsis-dev]

Consider changing the warning message from 'error committing sticky disk:' to 'Error committing sticky disk:' for consistency and clarity.

Suggested change

	core.warning('error committing sticky disk:', error);
	core.warning('Error committing sticky disk:', error);

[ellipsis-dev]

❌ Changes requested. Incremental review on 94bf350 in 2 minutes and 1 seconds

More details

• Looked at 422 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. src/main.ts:220

• Draft comment:
  Using process.exit(0) in setup-only mode may bypass normal cleanup. Consider returning from the function so that post-actions can run if needed.
• Reason this comment was not posted:
  Marked as duplicate.

2. src/setup_builder.ts:118

• Draft comment:
  In setup-only mode, the buildkitd process is spawned with 'detached' mode but still pipes stdout/stderr. Verify that piping in detached processes is intentional and doesn’t leak resources.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
  The comment is asking the author to verify if the behavior of piping stdout/stderr in detached mode is intentional and doesn't leak resources. This falls under asking the author to confirm their intention, which is against the rules. However, it does point out a potential issue with resource leakage, which could be useful. I need to decide if the potential usefulness outweighs the rule violation.

3. src/setup_builder.ts:242

• Draft comment:
  The variable 'nativeMultiPlatformBuildsEnabled' is always false due to the hardcoded 'false &&'. If multi-platform builds aren’t supported yet, consider removing the condition or leaving a comment for future work.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. src/main.ts:103

• Draft comment:
  Checking error messages with a substring (e.g., 'buildkitd') can be brittle. Consider using specific error types or codes for more robust error handling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. action.yml:19

• Draft comment:
  Typo: In the 'add-hosts' input description, 'List of a customs host-to-IP mapping' should probably be 'List of a custom host-to-IP mapping'.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. action.yml:25

• Draft comment:
  Typo: In the 'annotations' input description, 'List of annotation to set to the image' should probably be 'List of annotations to set to the image'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. action.yml:22

• Draft comment:
  Typo: In the 'allow' input description, consider using 'List of extra privileged entitlements' instead of 'List of extra privileged entitlement' for consistency with a list.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. src/state-helper.ts:68

• Draft comment:
  The file is missing a newline at the end. Please add a newline character after line 68 to adhere to common file formatting conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment is about the new code (the getExposeId function). Adding a trailing newline is a common convention and many linters enforce this. However, this is a very minor formatting issue that would typically be caught by linters, formatters, or the build process. It's not a significant enough issue to warrant a PR comment.
  The missing newline could cause issues with some tools and is considered a best practice by many style guides. Some would argue this is worth fixing.
  While technically correct, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. If it's important to the project, it should be enforced by a linter.
  Delete this comment as it's too minor of a formatting issue to warrant a PR comment. This should be handled by automated tools.

Workflow ID: wflow_Z3pcxck5G01ntmOn

---

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.

[ellipsis-dev]

For consistency with other log messages (e.g., 'Error reporting build completed:' at line 109), consider capitalizing 'Error' in the warning message on line 212 (currently 'error committing sticky disk:').

Suggested change

	core.warning('error committing sticky disk:', error);
	core.warning('Error committing sticky disk:', error);

[ellipsis-dev]

❌ Changes requested. Incremental review on 7d7d6da in 2 minutes and 7 seconds

More details

• Looked at 467 lines of code in 8 files
• Skipped 2 files when reviewing.
• Skipped posting 17 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  The new 'setup-only' input is clear, but consider aligning naming conventions (e.g. use kebab-case for all inputs) to match other inputs.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. src/__tests__/blacksmith-builder.test.ts:47

• Draft comment:
  Good addition of the 'setupOnly' flag in tests; ensure that tests also cover the setupOnly true branch for startBlacksmithBuilder.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The setupOnly parameter was just added to mockInputs, which strongly suggests it's a new feature. The fact that it's added to the test setup but not used in any test cases is a red flag. However, the comment starts with "Ensure that..." which violates our rules. Additionally, without seeing the main code that uses this parameter, we can't be 100% certain that testing the true case is necessary.
  I don't have visibility into the main code that uses setupOnly, so I can't be certain that testing the true case is important. The parameter might be unused or might not have any effect yet.
  While true that I don't have full context, the very fact that setupOnly was added to the test setup strongly implies it's meant to be used, and having no test coverage for the true case is likely an oversight.
  While the suggestion for test coverage is valid, the comment's phrasing as "Ensure that..." violates our rules. We should delete it and potentially rephrase it more directly.

3. src/context.ts:100

• Draft comment:
  Adding the 'setupOnly' boolean input is clear. Ensure that downstream logic gracefully handles when this flag is set.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
  The comment is asking the author to ensure that the downstream logic handles a specific case. This is similar to asking the author to ensure the behavior is intended, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.

4. src/main.ts:95

• Draft comment:
  Consider logging sticky disk exposeId more clearly; the record of exposeId is important for debugging.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

5. src/setup_builder.ts:109

• Draft comment:
  Ensure that the execa command string is parsed correctly; using detached and cleanup options in setup-only mode is a good approach. Verify that unref() is supported in your execa version.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify the support of unref() in their execa version, which is related to dependency management. This violates the rule against commenting on dependency versions or asking the author to verify behavior.

6. src/setup_builder.ts:355

• Draft comment:
  Great conditional on setupOnly when reporting the build to the control plane. This avoids unwanted metric reporting during setup-only mode.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 10% <= threshold 50%
  This comment seems to be purely informative, explaining the benefit of a condition in the code. It doesn't suggest any changes or ask for confirmation on any specific aspect of the code.

7. src/state-helper.ts:62

• Draft comment:
  The new state functions for exposeId are straightforward. Consider documenting what 'exposeId' represents for future maintainers.
• 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 comment suggests adding documentation, which could be helpful. However, this file follows a clear pattern where state values are managed without detailed documentation. The meaning of exposeId likely depends on the broader system context. Adding documentation here would be inconsistent with the file's style and the documentation would likely be better placed elsewhere.
  Documentation is generally good practice. Maybe this particular ID is especially important or non-obvious compared to other state values?
  While documentation is valuable, this file consistently manages state without detailed docs. If exposeId needs explanation, it should be documented where it's conceptually defined, not in the state management utility.
  Delete the comment. Requesting documentation here would be inconsistent with the file's pattern and wouldn't be the right place for explaining the concept.

8. src/main.ts:230

• Draft comment:
  Using process.exit(0) in setup-only mode might bypass the post cleanup actions. Ensure this is intentional or consider a more graceful termination so that post steps can run properly.
• Reason this comment was not posted:
  Marked as duplicate.

9. src/setup_builder.ts:126

• Draft comment:
  Ensure that using execa in detached mode with 'buildkitd.subprocess.unref()' is fully supported in the dependency version. This pattern assumes the 'subprocess' property exists; consider adding a safeguard or documentation if not.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
  The comment is asking the author to ensure that a specific pattern is supported in the dependency version, which violates the rule against asking the author to ensure behavior is intended or tested. However, it also suggests adding a safeguard or documentation, which is a specific code suggestion. The comment is partially useful but mostly violates the rules.

10. src/reporter.ts:199

• Draft comment:
  Consider adding validation for required environment variables (e.g. BLACKSMITH_STICKYDISK_TOKEN) before committing the sticky disk. This can help avoid silent failures if the variables are not set.
• 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 code follows an established pattern in the codebase where env vars default to empty strings. The function is already in a try-catch block that will handle failures. The API/service layer would likely reject invalid credentials. Adding validation here would be inconsistent with the rest of the file's approach to env vars.
  The comment raises a valid point about potential silent failures. Maybe there's a security risk in sending empty tokens to the API.
  The code is consistent with the file's established patterns and error handling approach. If env var validation was critical, it would likely be implemented file-wide, not just in this function.
  Delete the comment as it suggests a pattern that differs from the codebase's established approach to handling environment variables.

11. src/setup_builder.ts:348

• Draft comment:
  Fallback to '/dev/vdb' when no device is returned may not be valid in all environments. Consider making this fallback configurable or adding a check to ensure the device exists.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. action.yml:18

• Draft comment:
  The description for the 'add-hosts' input contains a typo ('customs')—consider changing it to 'custom' for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

13. action.yml:19

• Draft comment:
  Typo: Consider replacing 'customs' with 'custom' (or 'customized') in the description for 'add-hosts' on line 19 to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

14. action.yml:25

• Draft comment:
  Typo: Consider changing 'List of annotation to set to the image' to 'List of annotations to set to the image' on line 25 for grammatical accuracy.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. src/main.ts:232

• Draft comment:
  The comment on line 232 reads "// Let's remove the default " and appears to be incomplete. Could you please clarify or complete the commentary to better describe what is intended?
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking for clarification on an incomplete comment in the code. This is not a code suggestion or a request for a test, and it doesn't point out a specific issue with the code. It violates the rule against asking the PR author to clarify or explain their intention.

16. src/main.ts:306

• Draft comment:
  The variable name 'metadatadt' on line 306 appears to be a typographical error. Consider renaming it to something more descriptive (e.g. 'metadataStr' or 'formattedMetadata') for better clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

17. src/reporter.ts:212

• Draft comment:
  Typographical inconsistency: In the commitStickyDisk function, the error log on line 212 uses a lowercase 'error' ('error committing sticky disk:') whereas similar messages elsewhere use a capitalized 'Error'. For consistency and readability, consider updating it to 'Error committing sticky disk:'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While there is indeed an inconsistency in capitalization, this is a very minor stylistic issue that doesn't affect functionality. The comment is about something that's purely cosmetic and doesn't impact code quality in a meaningful way. According to the rules, we should not make comments that are obvious or unimportant.
  The inconsistency could make the logs slightly harder to grep or filter. Also, maintaining consistent style can be important for code readability.
  While consistency is good, this is too minor of an issue to warrant a PR comment. The benefit of fixing this doesn't outweigh the cognitive overhead of another PR comment.
  This comment should be deleted as it addresses a trivial stylistic issue that doesn't meaningfully impact code quality or functionality.

Workflow ID: wflow_XY59k5xqwSPIgWI3

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on 0012f7d in 2 minutes and 3 seconds

More details

• Looked at 464 lines of code in 8 files
• Skipped 2 files when reviewing.
• Skipped posting 12 drafted comments based on config settings.

1. src/setup_builder.ts:123

• Draft comment:
  Review the use of detached child process in setup-only mode. Ensure you have proper logging and external cleanup strategy for the buildkitd process that is not tied to the action lifecycle.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The code appears to be intentionally using detached mode to keep buildkitd running after the action exits. The logging is already handled via logStream. The cleanup:false is intentional based on the comment. This seems to be the desired behavior rather than an oversight. The comment is asking for verification rather than pointing out a clear issue.
  The comment could be pointing out legitimate concerns about orphaned processes and resource cleanup that I'm missing. Process management is complex and can have subtle issues.
  While process management is complex, the code shows clear intentionality in its design choices with appropriate logging and error handling already in place. The comment is asking for verification rather than identifying a specific issue.
  Delete this comment as it's asking for verification of intended behavior rather than pointing out a clear issue that needs fixing. The code already has logging and appears to handle the detached process appropriately.

2. src/main.ts:230

• Draft comment:
  Using process.exit(0) in the middle of the async workflow can bypass further cleanup steps. Ensure that detached processes and the post-step cleanup (e.g. tailnet leaving and cache commit) run as expected in setup‐only mode.
• Reason this comment was not posted:
  Marked as duplicate.

3. src/__tests__/blacksmith-builder.test.ts:47

• Draft comment:
  The new 'setupOnly' input is added in the code, but there are no dedicated tests covering setup‐only behavior. Consider adding tests specifically for when setupOnly is true to validate that build reporting is skipped and the process exits appropriately.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. src/context.ts:99

• Draft comment:
  The new 'setupOnly' boolean input is handled consistently. Just ensure that the naming convention (setup-only in YAML vs. setupOnly in code) remains clear in documentation.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. src/setup_builder.ts:109

• Draft comment:
  The startBuildkitd function now accepts a 'setupOnly' flag and uses detached mode if true. Ensure that the detached buildkitd process is monitored/logged appropriately since errors may not propagate back to the main process.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment appears to be speculative, asking to "ensure" monitoring is appropriate rather than pointing out a specific issue. The code already has logging and error handling in place. The comment starts with "Ensure that..." which is a red flag per the rules. The comment isn't pointing out a clear problem that needs fixing.
  The code could potentially have issues with detached process monitoring that I'm missing. Maybe there are edge cases where errors could be lost.
  While monitoring detached processes is important, the code already handles this appropriately by piping output to logs and having error handlers. The comment isn't pointing out any specific issues that need fixing.
  This comment should be deleted as it's speculative and starts with "ensure that", asking for verification rather than pointing out a specific issue that needs fixing.

6. src/setup_builder.ts:352

• Draft comment:
  When in setup-only mode, an empty string is passed as the dockerfile path to setupStickyDisk. Consider making this behavior explicit (e.g., use a constant or add a comment) so that it’s clear that reporting to the control plane is intentionally skipped.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. src/reporter.ts:199

• Draft comment:
  The new commitStickyDisk function swallows errors with a warning. Ensure that ignoring commit failures doesn't hide critical issues that might affect resource cleanup.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment raises a valid concern about error handling, but several factors make me think it should be deleted: 1) The error handling is consistent with the rest of the file 2) The comment is speculative about "might affect" 3) The comment asks to "ensure" something rather than making a specific suggestion 4) Without more context about the criticality of sticky disk commits, we can't be sure this is wrong.
  I could be wrong about the criticality of sticky disk commits - maybe they are more important than other operations and deserve stricter error handling.
  While sticky disk commits may be important, the consistent error handling pattern across the file suggests this is an intentional design choice that has been deemed acceptable for this system.
  Delete the comment. It asks for verification rather than making a specific suggestion, and the error handling follows established patterns in the codebase.

8. src/state-helper.ts:62

• Draft comment:
  The addition of setExposeId and getExposeId functions is straightforward. Confirm that the persistent state keys used are consistent with other state functions for future maintainability.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

9. src/main.ts:95

• Draft comment:
  Passing an empty string for the dockerfile path in setup-only mode is acceptable given the new logic. Just double-check that downstream processes that rely on this parameter handle an empty string gracefully.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

10. action.yml:19

• Draft comment:
  Typographical error: In the 'add-hosts' description on line 19, 'customs' should likely be 'custom'.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. action.yml:22

• Draft comment:
  Typographical error: In the 'allow' input description on line 22, consider changing 'entitlement' to 'entitlements' for grammatical correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. action.yml:25

• Draft comment:
  Typographical error: In the 'annotations' input description on line 25, 'annotation' should be pluralized to 'annotations' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_1kgzOXS3aHqqc7rE

---

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.

[ellipsis-dev]

Consider capitalizing the error message in the catch block of the commitStickyDisk function for consistency with other error messages in the file. For example, change 'error committing sticky disk:' to 'Error committing sticky disk:'

Suggested change

	core.warning('error committing sticky disk:', error);
	core.warning('Error committing sticky disk:', error);

[ellipsis-dev]

❌ Changes requested. Incremental review on 178ac91 in 1 minute and 48 seconds

More details

• Looked at 464 lines of code in 8 files
• Skipped 2 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. src/setup_builder.ts:119

• Draft comment:
  The execa import and usage: ensure that the version and its named/default import style are correct. Newer versions may require a default import. Validate that execa is properly imported as used here.
• 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 ensure that the import style and version of the 'execa' library are correct. It suggests validating the import style, which is not a specific code suggestion or a request for a test. It falls under the category of asking the author to double-check something, which is against the rules.

2. src/blacksmith-builder.test.ts:47

• Draft comment:
  Tests for setup-only mode are not present. Consider adding test coverage for when setupOnly is true so that behavior (no build report, early exit) is verified.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. src/main.ts:230

• Draft comment:
  Avoid calling process.exit(0) inside async group callbacks as it bypasses remaining cleanup and post actions. Consider returning from the function or otherwise signaling completion so that the post-step cleanup can run.
• Reason this comment was not posted:
  Marked as duplicate.

4. src/main.ts:102

• Draft comment:
  When reporting failures in catch blocks, error.message is accessed directly. Consider adding a type check (e.g. error instanceof Error) to avoid potential runtime issues if a non-Error is thrown.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. src/context.ts:100

• Draft comment:
  The new 'setup-only' input is correctly parsed into setupOnly. Double-check that all downstream functions properly handle both 'false', 'undefined', and 'true' values for setupOnly to avoid accidental misinterpretation.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

6. action.yml:19

• Draft comment:
  Typographical issue: Consider changing 'List of a customs host-to-IP mapping' to 'List of custom host-to-IP mappings' for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. action.yml:25

• Draft comment:
  Typographical issue: Change 'List of annotation to set to the image' to 'List of annotations to set on the image'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. action.yml:22

• Draft comment:
  Typographical issue: Consider revising 'List of extra privileged entitlement' to 'List of extra privileged entitlements' to properly denote a list.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. src/reporter.ts:212

• Draft comment:
  Consider capitalizing the first letter in the warning message on line 212 ('error committing sticky disk:') to make it consistent with other log messages.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment points out a real inconsistency in error message capitalization. The change would make the codebase more consistent. However, this is an extremely minor stylistic issue that doesn't affect functionality. The rules say not to make purely informative comments or obvious/unimportant changes.
  The inconsistency is real and fixing it would improve code quality marginally. But is this important enough to warrant a PR comment?
  While consistency is good, this is too minor of an issue to block or comment on a PR. The rules specifically say not to make unimportant comments.
  Delete the comment. While technically correct, it's too minor of a stylistic issue to warrant a PR comment.

Workflow ID: wflow_Smr7ZlAHnw9ChfKX

---

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.

[ellipsis-dev]

In setup-only mode, execa is used with detached: true. It is recommended to also call buildkitd.unref() so that the child process does not keep the Node event loop alive unnecessarily.

[ellipsis-dev]

In setupStickyDisk, the variable buildId remains undefined in setup-only mode. For consistency with the function signature (which expects buildId to be null if absent), consider explicitly setting it to null.

Suggested change

	let buildId: string | undefined = undefined;
	let buildId: string | null = null;

[ellipsis-dev]

❌ Changes requested. Incremental review on ecd277f in 1 minute and 43 seconds

More details

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

1. src/setup_builder.ts:116

• Draft comment:
  Using a shell command with 'nohup' and '&' outside execa’s process control may lead to orphan processes. Consider managing the detachment within execa or using spawn for better process control.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. action.yml:18

• Draft comment:
  Typo in description: consider changing "customs host-to-IP mapping" to "custom host-to-IP mapping".
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is suggesting a change in the description text, which is not related to the code itself. The rules specify not to ask the PR author to update the PR description. This comment seems to be about a typo in a description, which is not directly related to the code functionality or logic.

3. src/__tests__/blacksmith-builder.test.ts:48

• Draft comment:
  Consider adding tests for the new 'setup-only' mode, which is now part of the inputs.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. src/main.ts:229

• Draft comment:
  Using process.exit(0) in the middle of an async run may prevent pending cleanup/other async tasks from completing. Consider a more graceful exit strategy if possible.
• Reason this comment was not posted:
  Marked as duplicate.

5. src/setup_builder.ts:109

• Draft comment:
  Ensure the detached mode using execa with nohup is behaving as intended. Consider documenting any implications or cleanup steps needed for detached processes.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that the behavior is intended and to consider documenting implications, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written.

6. action.yml:19

• Draft comment:
  It looks like there is a typographical error in the description for 'add-hosts'. Consider changing 'a customs host-to-IP mapping' to 'a custom host-to-IP mapping' for better clarity.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative as it points out a typographical error in a description, which is not directly related to the code functionality or logic. It doesn't suggest a code change or improvement.

7. action.yml:25

• Draft comment:
  The description for 'annotations' currently reads 'List of annotation to set to the image'. Consider changing it to 'List of annotations to set to the image' to maintain plurality consistency with the input name.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. src/reporter.ts:212

• Draft comment:
  Typographical suggestion: Consider capitalizing the error message in the catch block of commitStickyDisk to be consistent with other error messages (e.g., 'Error committing sticky disk:' instead of 'error committing sticky disk:').
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the inconsistency, it's an extremely minor stylistic issue. The rules state not to make comments that are obvious or unimportant. Capitalization of error messages is not a significant enough issue to warrant a PR comment, as it doesn't affect functionality or code quality in any meaningful way.
  The inconsistency could indicate a broader pattern of inconsistent error handling that might be worth addressing. Also, consistent error message formatting can help with log parsing and searching.
  While consistency is good, this is too minor of an issue to block or comment on a PR. If there's a broader style concern, it should be addressed through linting rules or style guides, not PR comments.
  Delete this comment as it violates the rule about not making obvious or unimportant comments. This kind of minor stylistic feedback creates noise without adding significant value.

Workflow ID: wflow_EOgF727JSheNBJQV

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on d02b11e in 1 minute and 41 seconds

More details

• Looked at 461 lines of code in 8 files
• Skipped 2 files when reviewing.
• Skipped posting 14 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  Nice addition of the 'setup-only' input. Consider clarifying in the description that this disables build reporting as well as build execution.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. context.ts:99

• Draft comment:
  Good extension to the Inputs interface to include 'setupOnly'. Ensure downstream logic correctly handles the absence of a dockerfile when setupOnly is true.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. main.ts:230

• Draft comment:
  Using process.exit(0) in setup-only mode bypasses further cleanup. Consider returning or invoking a proper exit that still allows post actions to run.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. main.ts:73

• Draft comment:
  Proper handling of null dockerfile path when setupOnly is true. Confirm that downstream reporting logic does not rely on a valid dockerfile when in setup-only mode.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. setup_builder.ts:121

• Draft comment:
  The if/else blocks for starting buildkitd in non-setup vs setup-only modes are identical. Consider refactoring to remove duplication.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. setup_builder.ts:357

• Draft comment:
  Fallback to '/dev/vdb' is hardcoded. Consider making this configurable or at least document the assumption.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

7. setup_builder.ts:355

• Draft comment:
  Good use of condition to avoid reporting build when setup-only is true. Verify that skipping reporting aligns with downstream analytics requirements.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

8. state-helper.ts:62

• Draft comment:
  State helper functions look good. Ensure that saving state with raw strings (e.g., exposeId) is sufficient for later retrieval.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

9. src/setup_builder.ts:355

• Draft comment:
  In setupStickyDisk, when setupOnly is true, an empty string ('') is passed as dockerfilePath. Ensure this behavior is intentional and that reporter.reportBuild isn’t invoked when dockerfilePath is empty.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
  The comment is asking the author to ensure that a specific behavior is intentional, which violates the rule against asking for confirmation of intention. However, it also points out a potential issue with the logic that could be useful for the author to consider. The comment could be rephrased to suggest a code change or to highlight the potential issue more directly without asking for confirmation.

10. action.yml:19

• Draft comment:
  Typo noticed: The description 'List of a customs host-to-IP mapping' might be intended to read 'List of a custom host-to-IP mapping' or 'List of custom host-to-IP mappings'. Please adjust the wording for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. action.yml:25

• Draft comment:
  Typo noticed: In the description 'List of annotation to set to the image', consider changing 'annotation' to 'annotations' to indicate that multiple items might be specified.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. src/main.ts:306

• Draft comment:
  The variable name 'metadatadt' appears to be a typographical error. It seems like it should be something like 'metadataStr' or a similar descriptive name. Please consider renaming it for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. src/reporter.ts:212

• Draft comment:
  Consider updating the error log message from 'error committing sticky disk:' to 'Error committing sticky disk:' to maintain consistency in capitalization.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about inconsistency, this is an extremely minor stylistic issue. Error messages are for developers/debugging and slight capitalization differences don't impact functionality. The rules state not to make comments that are obvious or unimportant. This feels like an unimportant nitpick.
  The inconsistency could make logs harder to grep/search if someone is looking for error messages. Also, maintaining consistent style is important for code quality.
  While consistency is good, this is too minor to warrant a PR comment. The benefit of fixing this is minimal compared to the overhead of making a PR change and review.
  Delete this comment as it addresses an extremely minor stylistic issue that doesn't meaningfully impact code quality or functionality.

14. src/state-helper.ts:68

• Draft comment:
  Minor: The file is missing a newline at the end. Adding a newline can help maintain consistency and avoid issues with some tools.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_6anj4jFSQHOQHU2U

---

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.

[ellipsis-dev]

The execa call for starting buildkitd is duplicated in both the if and else blocks. Since they use identical options, consider refactoring to remove this redundancy.

[ellipsis-dev]

❌ Changes requested. Incremental review on 71b681d in 1 minute and 57 seconds

More details

• Looked at 565 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 11 drafted comments based on config settings.

1. action.yml:18

• Draft comment:
  Typo: consider replacing ‘customs’ with ‘custom’ in the ‘add-hosts’ input description.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. action.yml:14

• Draft comment:
  Ensure the new input 'setup-only' (with default 'false') is consistently used with the camelCase property (setupOnly) in your code. This is fine so long as the mapping is documented.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  Consider adding a newline at the end of file for consistency with common coding standards.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  Add a newline at the end of this file to follow best practices.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. src/main.ts:230

• Draft comment:
  Avoid using process.exit(0) to terminate the action in setup-only mode. Instead, consider returning from the function to allow the post step and any cleanup logic to execute properly.
• Reason this comment was not posted:
  Marked as duplicate.

6. src/setup_builder.ts:121

• Draft comment:
  The if/else branches for starting buildkitd in 'setup-only' and non-setup modes are identical. Consider consolidating them to avoid redundant code.
• Reason this comment was not posted:
  Marked as duplicate.

7. action.yml:19

• Draft comment:
  Typo in description: "List of a customs host-to-IP mapping" may be intended to be "List of a custom host-to-IP mapping". Consider removing the extra 's' from 'customs'.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. action.yml:25

• Draft comment:
  Typo in description: "List of annotation to set to the image" should likely be "List of annotations to set on the image" to match plural usage and improve clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  The file is missing a newline at the end. Adding a newline would make the file align with common style conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While missing trailing newlines are a real style issue, this is exactly the kind of thing that should be caught by linters and automated tools, not manual review comments. If the project cares about trailing newlines, it should have a linter rule for it. If it doesn't have such a rule, then it's not important enough to comment on.
  Maybe the project doesn't have proper linting set up yet, and manual enforcement of style is needed during the transition period?
  Even if linting isn't set up yet, style nitpicks like trailing newlines are too minor to warrant manual review comments. They create noise that distracts from more important issues.
  Delete this comment. Missing trailing newlines should be caught by automated tools, not manual review.

10. src/__mocks__/execa.ts:7

• Draft comment:
  There is an unnecessary trailing whitespace at the end of line 7. Consider removing it for a cleaner diff.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing whitespace, this is an extremely minor formatting issue. Most modern IDEs automatically trim trailing whitespace. This kind of nitpick doesn't require developer attention and doesn't impact code quality or functionality.
  The trailing whitespace could theoretically cause inconsistency in the codebase formatting. Some teams have strict formatting rules.
  Even if formatting consistency is important, this is better handled by automated formatting tools or pre-commit hooks rather than PR comments.
  This comment should be deleted as it's too minor and doesn't require developer attention. It violates the rule about not making obvious or unimportant comments.

11. src/main.ts:306

• Draft comment:
  The variable name 'metadatadt' (line 306) appears to be a typographical error. Consider renaming it to something clearer like 'metadataStr' or simply 'metadataJson' for better readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_lCVVw4rp6LY2rdCz

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on aeffb72 in 1 minute and 42 seconds

More details

• Looked at 516 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  Ensure the input naming for 'setup-only' is documented. The code converts it to 'setupOnly' in context; consistency in naming in docs helps users.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is suggesting to ensure that the naming convention is documented. This falls under asking the PR author to ensure something is documented, which is not allowed by the rules. The comment does not provide a specific code suggestion or ask for a test to be written, so it should be removed.

2. src/main.ts:229

• Draft comment:
  Using process.exit(0) in setup-only mode may bypass later cleanup steps. Confirm that immediate termination is intended.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment raises a valid concern - process.exit(0) would immediately terminate and skip the post-cleanup section. However, looking at the setupOnly mode context, it seems this is intentional since we don't want to do build-related cleanup when we're only setting up the builder. The comment is speculative ("may bypass") and asks for confirmation rather than pointing out a clear issue.
  I could be wrong about the cleanup requirements - maybe some of those cleanup steps are still important even in setup-only mode. The sticky disk commitment in particular might be needed.
  Looking at the code more carefully, the cleanup steps that would be skipped are primarily related to build artifacts and mounts. In setup-only mode, we haven't started a build or created artifacts that need cleanup, so skipping these steps is likely intentional.
  The comment should be deleted because it asks for confirmation of intended behavior rather than pointing out a clear issue, and the process.exit(0) behavior appears intentional in setup-only mode where cleanup is not needed.

3. src/setup_builder.ts:119

• Draft comment:
  Review the use of execa with a complex 'nohup' command. Confirm that shell redirection and detached process handling works reliably across environments.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to confirm the behavior of a command across environments, which violates the rule against asking for confirmation or ensuring behavior. It does not provide a specific code suggestion or ask for a specific test to be written.

4. action.yml:18

• Draft comment:
  Typo: Consider changing 'customs host-to-IP mapping' to 'custom host-to-IP mapping' for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

5. src/setup_builder.ts:117

• Draft comment:
  Using a shell command with nohup and redirection in execa may be less robust. Consider using spawn with an argument array to avoid potential shell injection risks.
• 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 comment raises a valid security concern - using shell strings can be risky compared to argument arrays. However, in this case: 1) The command is entirely hardcoded with no user input 2) It's running as sudo anyway 3) The change was made deliberately to handle nohup and redirection which is harder with spawn 4) The code is in a build system where security boundaries are already crossed.
  I may be underestimating the security risks. Even hardcoded strings can be dangerous if they contain special characters that could be interpreted by the shell.
  While shell injection is generally a concern, in this specific context with a hardcoded command running as root in a build environment, the practical risk is minimal compared to the benefits of using shell features like nohup and redirection.
  The comment should be deleted. While it raises a theoretically valid point, the current implementation is a reasonable tradeoff given the context and requirements.

6. src/__mocks__/execa.ts:7

• Draft comment:
  Consider adding a newline at the end of the file for consistency with POSIX file standards.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

7. action.yml:19

• Draft comment:
  Typo: 'List of a customs host-to-IP mapping' should likely be 'List of a custom host-to-IP mapping' (or similar) to properly describe the input.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. action.yml:25

• Draft comment:
  Typographical error: 'List of annotation to set to the image' should probably be changed to 'List of annotations to set on the image' for better grammatical clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_7j1vEHvUwbmUGO1E

---

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.

[ellipsis-dev]

No explicit test case for the new 'setup-only' mode was added. Consider adding tests to verify that when setupOnly is true, the builder setup is performed without proceeding to build configuration.

[ellipsis-dev]

There's a missing newline at the end of the file. Include a trailing newline for consistency and compatibility.

[ellipsis-dev]

❌ Changes requested. Incremental review on dbbe04b in 1 minute and 40 seconds

More details

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

1. src/main.ts:88

• Draft comment:
  When calling startAndConfigureBuildkitd, consider passing the setupOnly flag (i.e. inputs.setupOnly) so that the buildkitd startup honors setup‐only mode.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. src/main.ts:227

• Draft comment:
  In setup‐only mode the code calls process.exit(0) to abort further processing. It would be beneficial to add a comment explaining why an early exit is safe in this mode and that no additional cleanup is required.
• 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 comment raises a valid point about documenting why early exit is safe. However, looking at the code structure:

1. The cleanup is handled in a separate post function that runs regardless
2. The code already has a clear comment explaining what setup-only mode does
3. The early exit pattern is common in Node.js
4. The comment is asking for documentation of something that is clear from the code structure
   I could be underestimating the importance of explicitly documenting exit behavior. Future maintainers might not be familiar with how Node.js handles cleanup in this case.
   While documentation is good, the code structure makes it clear that cleanup happens in a separate post function. The existing comment already explains what setup-only mode does. Additional documentation would be redundant.
   The comment should be deleted. The code is already well-documented and the safety of the early exit is clear from the structure of the code with its separate post cleanup function.

3. src/main.ts:88

• Draft comment:
  Pass the setupOnly flag to startAndConfigureBuildkitd. Currently, the call doesn't include inputs.setupOnly, so even in setup-only mode, buildkitd is started the normal way.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. src/main.ts:227

• Draft comment:
  Using process.exit(0) in setup-only mode may bypass important cleanup steps. Consider a more graceful termination mechanism.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%

1. The code does have a post-cleanup function that handles important cleanup tasks.
2. Using process.exit(0) could potentially bypass this cleanup.
3. However, looking at the setupOnly mode context, it's clear this is an intentional early exit path for when we only want to set up the builder.
4. The cleanup steps are mainly relevant for when we actually run a build, not just setup.
   The comment raises a valid technical point about process.exit(0) behavior, but may not understand the intentional design of the setupOnly mode which deliberately wants to skip build-related cleanup.
   While process.exit(0) can be dangerous, in this specific case it appears to be an intentional design choice for the setupOnly mode where we want to exit early before any build-related resources are allocated.
   The comment should be deleted because the use of process.exit(0) appears intentional for the setupOnly mode where cleanup is not needed, and the comment does not provide clear evidence that any important cleanup steps are actually being bypassed.

5. action.yml:19

• Draft comment:
  Typo on line 19: 'List of a customs host-to-IP mapping' should likely be 'List of a custom host-to-IP mapping' or 'List of custom host-to-IP mappings'.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. action.yml:25

• Draft comment:
  Typo on line 25: 'List of annotation to set to the image' should be corrected to 'List of annotations to set to the image' for grammatical consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. action.yml:22

• Draft comment:
  Typo on line 22: 'List of extra privileged entitlement' should probably be 'List of extra privileged entitlements' to indicate that multiple entitlements can be specified.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  There is an extra trailing whitespace at the end of line 12. Please remove it.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Trailing whitespace is a very minor style issue that would typically be handled by automatic code formatting or linting tools. It doesn't affect functionality at all. The rules specifically say not to make comments that are obvious or unimportant. This seems like a perfect example of an unimportant comment.
  Maybe some projects have strict style guidelines where trailing whitespace is a serious issue that must be addressed in code review.
  Even if there are strict style guidelines, this is exactly the kind of thing that should be handled by automated tools, not manual code review comments. Code review should focus on more substantial issues.
  This comment should be deleted as it's too minor and would be better handled by automated tooling.

9. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  It appears that the file does not end with a newline. Adding a newline at the end of the file is a common convention and improves compatibility with various tools.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While missing newlines can cause issues with some tools, this is a very minor issue that would likely be caught by linting or formatting tools. It's not a logic issue or code quality problem that requires manual review attention. The comment is technically correct but not important enough to warrant manual intervention.
  The missing newline could potentially cause merge conflicts or issues with some tools, so maybe it is worth mentioning?
  No, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It violates the rule about not commenting on things that would be caught by the build.
  Delete this comment as it's a minor formatting issue that should be handled by automated tools rather than manual review.

10. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  Please remove the trailing whitespace on line 23 after the closing brace. This is a cosmetic issue that can be auto-formatted.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Trailing whitespace is a minor style issue that should be handled by automated linting or formatting tools. It doesn't affect functionality and isn't worth a manual review comment. Most IDEs and git configs automatically strip trailing whitespace anyway.
  The comment is technically correct - there is trailing whitespace. And since this is a new file, it is about changed code.
  While technically correct, this kind of minor style issue should be handled by automation, not manual review comments. It falls under the "obvious or unimportant" rule.
  Delete this comment as it points out an unimportant style issue that should be handled by automated tools rather than manual review.

Workflow ID: wflow_v6aAAfYnUPcSWpYc

---

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.

[ellipsis-dev]

❌ Changes requested. Incremental review on 59d2bab in 2 minutes and 0 seconds

More details

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

1. src/main.ts:88

• Draft comment:
  Passing the setupOnly flag to startAndConfigureBuildkitd is missing. This means that even in setup-only mode, buildkitd might not be started in the intended way. Consider changing the call to include inputs.setupOnly.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. src/setup_builder.ts:140

• Draft comment:
  The logic for starting buildkitd in setup-only mode duplicates piping logic with the non-setup-only branch. Consider refactoring the duplicate stdout/stderr piping block into a shared helper function.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. src/main.ts:88

• Draft comment:
  This call to startAndConfigureBuildkitd doesn’t pass the setupOnly flag. If inputs.setupOnly is true, the builder should be started with the setup-only mode. Consider updating the call to: await startAndConfigureBuildkitd(parallelism, inputs.platforms, inputs.setupOnly).
• Reason this comment was not posted:
  Comment was on unchanged code.

4. src/setup_builder.ts:141

• Draft comment:
  The code uses two different methods for spawning buildkitd: spawn (for normal mode) and execa with shell redirection (for setup-only mode). This inconsistency can make maintenance harder. Consider standardizing the approach if possible.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

5. src/reporter.ts:181

• Draft comment:
  The Metric class is instantiated with an object literal (e.g., new Metric({ type: metricType, value: {case: 'intValue', value: BigInt(value)} })). Ensure that the actual Metric implementation supports this initialization as the mock implementation only defines default properties.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. src/__tests__/blacksmith-builder.test.ts:45

• Draft comment:
  There are no test cases covering the new setup-only mode. Consider adding tests to verify that when setupOnly is true the action skips the Docker build process and exits with the expected message.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. action.yml:19

• Draft comment:
  Typographical error: In the description for 'add-hosts', consider changing 'customs host-to-IP mapping' to 'custom host-to-IP mapping'.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. action.yml:25

• Draft comment:
  Typographical error: In the description for 'annotations', consider updating 'List of annotation to set to the image' to 'List of annotations to set on the image' for better clarity and grammatical correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. action.yml:22

• Draft comment:
  Typographical suggestion: In the description for 'allow', consider changing 'List of extra privileged entitlement' to 'List of extra privileged entitlements' for grammatical consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  The file is missing a newline at the end. Please add a newline (an empty line) after the closing brace to adhere to common file formatting conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at EOF is a common convention, this is a very minor formatting issue that would typically be handled automatically by code formatters or linters. The comment doesn't point out any logical issues or code quality problems that need addressing.
  The missing newline could cause issues with some tools or version control systems. It's a widely accepted best practice.
  While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It doesn't affect the code's functionality.
  Delete this comment as it's too minor of a formatting issue to warrant a manual review comment. This should be handled by automated tooling.

11. src/__mocks__/execa.ts:7

• Draft comment:
  There is a trailing whitespace at the end of line 7. Please remove the extra space after '});' for cleaner diffs and consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing whitespace, this is an extremely minor formatting issue. Most IDEs automatically strip trailing whitespace, and this kind of comment creates noise without adding meaningful value to code quality. The file is a mock file for testing purposes, making formatting even less critical.
  The trailing whitespace could theoretically cause inconsistency in the codebase if left unchecked, and some teams do have strict formatting rules about this.
  However, this is exactly the kind of minor formatting issue that should be handled by automated tooling (like Prettier or ESLint) rather than manual code review comments. It's too trivial for human review.
  Delete this comment as it's too minor and obvious to warrant a code review comment. This kind of formatting issue should be handled by automated tools.

12. src/state-helper.ts:68

• Draft comment:
  The file is missing a newline at the end. Please add a newline to conform to common file formatting conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at the end of files is a common convention, this is a very minor formatting issue that would typically be handled by editor settings or automated formatting tools. It doesn't affect functionality and is not related to the actual code changes being made (adding new expose ID functions).
  The comment is technically correct about the missing newline. Missing newlines can cause issues with some tools and make diffs less clean.
  However, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual code review comments. It's too trivial for a PR comment.
  The comment should be deleted as it's too minor and would be better handled by automated formatting tools.

Workflow ID: wflow_7HtCX3DZr44CEHRu

---

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.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 6f64f62 in 1 minute and 45 seconds

More details

• Looked at 578 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 14 drafted comments based on config settings.

1. src/main.ts:229

• Draft comment:
  Consider whether calling process.exit(0) here is appropriate. Exiting abruptly might skip additional cleanup or post steps. Instead, you could return from the function to allow graceful cleanup.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment makes a valid point - process.exit(0) would skip the post cleanup function. Looking at the post function, it does important cleanup like unmounting disks and shutting down buildkitd. However, in setup-only mode, we want to leave the builder running for Docker to use it, so we actually don't want to run the cleanup. The process.exit(0) is intentionally skipping cleanup in this case.
  Am I sure that skipping cleanup is the right thing to do in setup-only mode? Could there be other cleanup tasks besides the builder that should still run?
  Looking at the post function, all the cleanup tasks are related to the builder - unmounting disks, shutting down buildkitd, etc. In setup-only mode, we explicitly want to leave these running for Docker to use. There don't appear to be any other cleanup tasks that would be skipped.
  The process.exit(0) appears to be intentional to skip cleanup when in setup-only mode, since we want to leave the builder running. The comment's suggestion would actually break the intended functionality.

2. src/main.ts:77

• Draft comment:
  The use of an empty string for dockerfilePath in setup-only mode is intentional, but ensure downstream functions handle empty paths appropriately.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that downstream functions handle empty paths appropriately. This is a form of asking the author to double-check or ensure behavior, which violates the rules. The comment does not provide a specific suggestion or ask for a specific test to be written.

3. src/setup_builder.ts:140

• Draft comment:
  The execa call for setup-only mode is well-handled. Verify that the detached process and unref() are behaving as expected in your deployment environment.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

4. src/setup_builder.ts:369

• Draft comment:
  Document that reporting to the control plane is intentionally skipped in setup-only mode. Future maintainers might be confused by the conditional buildResponse logic.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The existing comment is clear and concise. The suggested change adds "to avoid confusion with the conditional buildResponse logic" which doesn't actually explain anything more meaningful. The code flow is straightforward - in setup mode, don't report the build. The conditional logic isn't confusing and doesn't need explanation.
  Perhaps there is some deeper architectural reason for not reporting builds in setup mode that should be documented? Maybe future maintainers would benefit from understanding the bigger picture?
  While architectural context can be valuable, comments should be focused on explaining non-obvious code. The behavior here is clear from the code and existing comment. If there's important architectural context, it belongs in design docs or higher-level documentation.
  The comment should be deleted. The existing code comment is sufficient and the suggested change doesn't add meaningful information.

5. src/reporter.ts:199

• Draft comment:
  The commitStickyDisk function catches errors and logs a warning. Consider whether you want to propagate the error for critical failures or if swallowing it is acceptable in your failure strategy.
• 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 comment questions whether swallowing errors is appropriate, but looking at the codebase patterns, this appears to be an intentional design choice across all reporting functions. The comment is asking the author to "consider" something that was likely already considered as part of the broader error handling strategy. The comment isn't pointing out a clear issue that needs fixing.
  The error handling could theoretically be problematic if this is a critical operation that should never fail silently. I don't have full context about how important sticky disk commits are.
  Given the consistent pattern across the codebase and explicit comments about intentionally swallowing errors, plus the fact that this is a reporting function, the current error handling appears to be an intentional design choice rather than an oversight.
  Delete this comment. It's questioning an intentional design pattern without providing clear evidence that the current approach is wrong.

6. src/main.ts:88

• Draft comment:
  Consider passing the setup-only flag to startAndConfigureBuildkitd. Currently, the call (line 88) does not include inputs.setupOnly, even though the function now accepts a third parameter. For consistency, change it to: startAndConfigureBuildkitd(parallelism, inputs.platforms, inputs.setupOnly).
• Reason this comment was not posted:
  Comment was on unchanged code.

7. src/main.ts:229

• Draft comment:
  The use of process.exit(0) in setup-only mode may bypass some cleanup logic in the post step. Ensure that this abrupt termination is intended and that any necessary cleanup is handled before exiting.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

8. src/reporter.ts:183

• Draft comment:
  Verify that instantiating the Metric with a BigInt value (using { case: 'intValue', value: BigInt(value) }) is compatible with the service’s proto definitions. This usage could lead to runtime type issues if mismatched.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. src/__tests__/blacksmith-builder.test.ts:47

• Draft comment:
  Consider adding tests that explicitly cover the new setup-only mode to verify that the builder is set up correctly and that the build process is skipped as intended.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. action.yml:19

• Draft comment:
  Typo: In the 'add-hosts' input description, consider changing 'a customs host-to-IP mapping' to 'a custom host-to-IP mapping'.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. action.yml:22

• Draft comment:
  Typo: In the 'allow' input description, consider changing 'extra privileged entitlement' to 'extra privileged entitlements' for proper plurality.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. action.yml:25

• Draft comment:
  Typo: In the 'annotations' input description, consider changing 'List of annotation to set to the image' to 'List of annotations to set on the image'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  A newline is missing at the end of the file (EOF). Please add a newline to conform to standard formatting conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Missing newlines at EOF are a common source of git diff noise and can cause issues with some tools. However, this is a very minor issue that would likely be caught by linting or formatting tools during the build process. The rules specifically state not to comment on things that would be caught by the build.
  Missing newlines at EOF can cause real issues in some scenarios, and it's a widely accepted best practice to include them.
  While true, this is exactly the kind of minor issue that should be handled by automated tooling rather than manual review comments.
  The comment should be deleted as it violates the rule about not commenting on issues that would be caught by the build.

14. src/state-helper.ts:67

• Draft comment:
  It looks like there's no newline at the end of the file. Consider adding a trailing newline to follow common style conventions.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_kPE2wSVq5QexvHrj

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on e862d9a in 40 seconds

More details

• Looked at 23 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. src/main.ts:77

• Draft comment:
  Good change. Consider using null (or undefined) instead of an empty string for dockerfilePath in setup-only mode to semantically indicate the absence of a dockerfile. This might prevent potential downstream issues where an empty string is treated as a valid path.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

2. src/main.ts:77

• Draft comment:
  Using the ternary operator here works, but consider using a more explicit sentinel value (e.g. null) instead of an empty string for the setup-only case. This would make it clearer that no dockerfile is expected.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. src/main.ts:78

• Draft comment:
  The check if (!inputs.setupOnly && !dockerfilePath) works as intended, but ensure that downstream functions (like setupStickyDisk) correctly interpret an empty string. If not, consider aligning types by using null instead.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. src/main.ts:303

• Draft comment:
  There's a variable named 'metadatadt' which looks like it might be a typo. Consider renaming it to something clearer (e.g., 'metadataStr') to improve readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_qvgv7UQjsoSBi0i8

---

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

[ellipsis-dev]

❌ Changes requested. Incremental review on ec82030 in 1 minute and 35 seconds

More details

• Looked at 522 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. src/main.ts:77

• Draft comment:
  Consider validating the empty dockerfilePath when setupOnly is true. Using an empty string here is intended to skip build reporting, but ensure downstream functions (like reportBuild) can safely handle an empty string.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. action.yml:14

• Draft comment:
  The addition of the 'setup-only' input is clear. Consider adding a short note describing its use-case and ensuring the naming is consistent with the 'setupOnly' property in the code.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. context.ts:99

• Draft comment:
  The Inputs interface now includes 'setupOnly' (mapped from the 'setup-only' input). Ensure tests and mocks use the same casing and that documentation reflects this new option.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. main.ts:77

• Draft comment:
  Here, the dockerfile path is set to an empty string for setup-only mode. This bypasses reporting to the control plane, which is intentional. Confirm that downstream logic correctly handles an empty dockerfilePath when setupOnly is true.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. main.ts:227

• Draft comment:
  Using process.exit(0) in setup-only mode ends the process abruptly. This may bypass subsequent cleanup steps. Consider having a graceful return to allow post-run cleanup (if necessary) instead of an immediate exit.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. setup_builder.ts:117

• Draft comment:
  The buildkitd command uses 'nohup' and appends '&', while the execa options already set detached: true. This redundancy could be simplified by removing '&' from the command.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. setup_builder.ts:342

• Draft comment:
  The logic to skip reporting the build to the control plane when setupOnly is true looks correct. Just ensure monitoring and logging correctly handle this mode.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. state-helper.ts:62

• Draft comment:
  New helper functions for setting and getting the exposeId are added. Verify that these are appropriately used in cleanup logic to ensure resources are correctly tracked.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. action.yml:19

• Draft comment:
  Typo: Consider changing 'a customs host-to-IP mapping' to 'a custom host-to-IP mapping' or 'custom host-to-IP mapping entries' for grammatical correctness.
• Reason this comment was not posted:
  Comment was on unchanged code.

10. action.yml:22

• Draft comment:
  Typo: In the description for 'allow', consider changing 'extra privileged entitlement' to 'extra privileged entitlements' to match the list.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. action.yml:25

• Draft comment:
  Typo: In the description for 'annotations', consider changing 'List of annotation to set to the image' to 'List of annotations to set for the image' for consistency and clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  There is no newline at the end of the file. Adding a newline can help maintain consistency with common style guidelines.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at end of file is a common style convention, this seems like a very minor issue that would likely be caught by linting or formatting tools. The comment doesn't point out any functional issues or suggest important code improvements. It's purely about style formatting.
  The missing newline could cause issues with some tools or when concatenating files. Some version control systems also prefer files ending in newlines.
  While true, this is still a minor formatting issue that should be handled by automated tools rather than manual review comments. It doesn't affect functionality.
  Delete this comment as it's too minor of a style issue to warrant a manual review comment. This kind of formatting should be handled by automated tools.

13. src/__mocks__/execa.ts:7

• Draft comment:
  There seems to be an extra trailing whitespace after the closing parenthesis and semicolon on line 7. Please remove it for consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the whitespace, this is an extremely minor formatting issue. Most IDEs and linters automatically handle trailing whitespaces. This kind of nitpick doesn't require developer attention and doesn't impact code quality or functionality.
  The trailing whitespace could be part of a project's style guide that I'm not aware of. Some teams are very strict about whitespace consistency.
  Even if whitespace is important to the team, this is the kind of issue that should be handled by automated tooling (linters/formatters) rather than manual code review comments.
  Delete this comment as it's too minor and should be handled by automated tooling rather than taking up reviewer and developer attention.

Workflow ID: wflow_jbz0wd45LUkJJ1kB

---

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.

[ellipsis-dev]

Avoid using process.exit(0) in the main execution block. Exiting abruptly may bypass cleanup (post steps) that are important for resource deallocation. Consider returning early or restructuring to allow post steps to run.

[ellipsis-dev]

Add a newline at the end of the file and remove the trailing space on line 12.

Suggested change

	}
	}

[ellipsis-dev]

❌ Changes requested. Incremental review on efef99a in 1 minute and 36 seconds

More details

• Looked at 513 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  Ensure the new input 'setup-only' is consistently named (kebab-case here vs camelCase in code).
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. context.ts:99

• Draft comment:
  New boolean input 'setupOnly' is added; consider adding comments to clarify how an empty dockerfile path is handled when setupOnly is true.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. blacksmith-builder.test.ts:45

• Draft comment:
  Consider adding tests for the new setup-only mode to verify early exit and to cover branch when setupOnly is true.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. main.ts:227

• Draft comment:
  Using process.exit(0) in setup-only mode may skip cleanup steps in the post action. Consider returning gracefully to allow cleanup execution.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. setup_builder.ts:117

• Draft comment:
  Consider using execa with a command array (command + args) rather than a full shell string to avoid potential shell injection issues.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. action.yml:19

• Draft comment:
  Typo: In the 'add-hosts' input description, 'customs' should probably be replaced with 'custom' (e.g., 'List of a custom host-to-IP mapping').
• Reason this comment was not posted:
  Comment was on unchanged code.

7. action.yml:22

• Draft comment:
  Typo: In the 'allow' input description, consider using the plural form 'entitlements' instead of 'entitlement' since it lists multiple items (e.g., 'List of extra privileged entitlements').
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. action.yml:25

• Draft comment:
  Typo: In the 'annotations' input description, it might be more appropriate to say 'List of annotations to set on the image' to match the plural nature of the list.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  Minor issue: The file is missing a newline at the end. It's a best practice to end files with a newline to ensure compatibility with various tools.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at end of file is indeed a common best practice, this is a very minor issue that would typically be handled by editor settings or linting rules. It's not something that requires manual review comments. The comment doesn't suggest any meaningful code changes that would impact functionality or maintainability.
  The missing newline could potentially cause issues with some tools or version control systems. It is a real, objective issue that was spotted correctly.
  While technically correct, this is exactly the kind of minor, obvious issue that should be handled by automated tools rather than manual review comments. It doesn't warrant human attention.
  Delete this comment. This kind of minor formatting issue should be handled by linting rules or editor settings, not manual review comments.

10. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  Cosmetic issue: There appears to be a trailing whitespace after the closing brace on line 12. Consider removing the extra space for consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing whitespace, this is an extremely minor formatting issue. Most modern IDEs and code formatters would automatically handle this. It's not a logic issue, doesn't affect functionality, and is too trivial to warrant a PR comment. This falls under the "Do NOT make comments that are obvious or unimportant" rule.
  The trailing whitespace could be part of a team's style guide or linting rules. Maybe it's more important than I think.
  If trailing whitespace was important enough to enforce, it would typically be handled by automated linting or formatting tools, not manual code review comments.
  This comment should be deleted as it's too trivial and would be better handled by automated tools.

11. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  There is no newline at the end of the file. For consistency and to avoid potential issues with some tools, please add a newline after the last line.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct, missing newlines at end of file are typically automatically fixed by most IDEs and build tools. This is a very minor issue that doesn't affect functionality. The rules state not to comment on things that would be obviously caught by the build.
  Missing newlines can cause issues with some tools and make diffs harder to read. It's a widely accepted best practice to have newlines at end of files.
  While true, this is exactly the kind of minor issue that should be handled by automated tooling rather than manual review comments. Most modern development environments handle this automatically.
  Delete this comment as it's too minor and would likely be caught by automated tooling.

12. src/__mocks__/execa.ts:7

• Draft comment:
  Minor note: It looks like there is trailing whitespace on line 7 after the closing parenthesis. It would be cleaner to remove the extra space.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Trailing whitespace is a minor code style issue. While it's technically correct, it's the kind of thing that should be handled by automated formatting tools or editor settings, not PR comments. It doesn't affect functionality and is too trivial to warrant manual review attention.
  The comment is technically correct - there is trailing whitespace. Maybe keeping consistent code style is important for the project.
  Even if code style is important, trailing whitespace is too minor and should be handled by automation, not human review comments.
  Delete this comment as it violates the rule about not making obvious or unimportant comments.

13. src/main.ts:303

• Draft comment:
  There appears to be a typographical error in the variable name 'metadatadt' (used to store the stringified metadata). Consider renaming it to something like 'metadataFormatted' or simply 'metadata' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_9X3yYb7EyDkhlD2h

---

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.

[ellipsis-dev]

Avoid using process.exit(0) in setup-only mode because it can prevent the post cleanup steps from running. Consider returning from the function instead to allow proper resource cleanup.

Suggested change

	process.exit(0);
	return;

[ellipsis-dev]

❌ Changes requested. Incremental review on 13a0d0a in 2 minutes and 0 seconds

More details

• Looked at 513 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. src/setup_builder.ts:117

• Draft comment:
  Using a shell command string with nohup and redirection in execa may hide errors. Consider passing command arguments as an array for better error handling and portability.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While using array arguments is generally better practice with child process execution, in this case the command requires shell features (nohup, redirection) that wouldn't work with array arguments. The code is explicitly using shell: '/bin/bash' showing this is intentional. The error handling appears adequate with error event listener and stdio piping.
  The comment makes a valid general point about best practices, but may not have fully considered why shell features are needed here.
  The shell command string is actually necessary here due to the nohup and redirection requirements. Converting to array arguments would break the functionality.
  The comment should be deleted as the current implementation is intentionally using shell features that require command string format.

2. action.yml:19

• Draft comment:
  Typo in description: 'customs' should be 'custom'.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is pointing out a typo in the description, which is not related to the code itself. According to the rules, comments should not ask the PR author to update the PR description or focus on purely informative aspects. Therefore, this comment should be removed.

3. src/main.ts:77

• Draft comment:
  Passing an empty dockerfile path in setup-only mode; confirm that setupStickyDisk handles an empty string correctly.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to confirm that a specific function handles an empty string correctly. This is a request for confirmation, which violates the rule against asking the author to confirm their intention or ensure behavior is intended.

4. src/main.ts:227

• Draft comment:
  Avoid using process.exit(0) here; consider returning gracefully to allow all cleanup routines to run and improve testability.
• Reason this comment was not posted:
  Marked as duplicate.

5. src/reporter.ts:183

• Draft comment:
  Ensure that the Metric constructor accepts initialization with the provided object literal; verify compatibility with the proto class.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. action.yml:19

• Draft comment:
  Typo: The description for 'add-hosts' uses 'customs host-to-IP mapping'. Consider replacing 'customs' with 'custom' to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. action.yml:22

• Draft comment:
  Typo: In the description for 'allow', the phrase 'extra privileged entitlement' should likely be 'extra privileged entitlements' to indicate a list.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. action.yml:25

• Draft comment:
  Typo: The description for 'annotations' reads 'List of annotation to set to the image'. Consider changing it to 'List of annotations to set to the image' for consistency with the plural form.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  There's no newline at the end of the file. Please add a newline after the closing brace to adhere to common coding conventions.
• 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%
  Missing newlines at EOF are generally considered a code style issue. Many linters and editors enforce this. However, this is a generated/mock file based on the path (mocks) and naming convention (_pb.ts suggesting Protocol Buffers). Style issues in generated files are usually not worth commenting on since they'll be overwritten anyway.
  The newline at EOF is a real style issue that could cause git diffs to be messier. Some would argue it's worth fixing even in generated files.
  Since this appears to be a generated mock file, maintaining style consistency is less important as it will likely be regenerated. The comment adds noise without providing significant value.
  Delete this comment as it's a minor style issue on what appears to be a generated/mock file.

10. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  There appears to be a trailing space on line 12 after the closing brace. Removing any unnecessary trailing whitespace would help keep the file tidy.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing space, this is an extremely minor formatting issue. Most modern IDEs and formatters handle trailing whitespace automatically. This kind of nitpick doesn't add value to the code review and could be distracting from more important issues.
  The trailing space could be a sign of inconsistent code formatting practices that might accumulate over time. It's a valid observation about code cleanliness.
  While code cleanliness matters, this is too minor to warrant a PR comment. This should be handled by automated tooling like prettier or eslint, not manual review comments.
  Delete this comment as it's too minor and would be better handled by automated formatting tools.

11. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  There's no newline at the end of the file. Please add a newline after the final closing brace to meet common code style guidelines.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at the end of files is a common convention, this is a very minor style issue. Most modern editors and linters automatically handle this. The comment doesn't point out any logical issues or bugs. It's purely about code style.
  The missing newline could cause issues with some tools or version control systems. It's a widely accepted best practice to have a newline at the end of files.
  While true, this is exactly the kind of minor style issue that should be handled by automated tooling rather than code review comments. If it's important to the project, it should be enforced by a linter.
  Delete this comment as it's too minor of a style issue to warrant a manual review comment.

12. src/__mocks__/execa.ts:7

• Draft comment:
  Minor typographical issue: it looks like there's an unnecessary trailing space on line 7 after the closing '});'. It's a trivial cosmetic detail, but cleaning it up can help maintain code quality.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing space, this is an extremely minor formatting issue. Most IDEs automatically trim trailing whitespace, and this kind of nitpick doesn't meaningfully impact code quality. This feels like noise that distracts from more important review feedback.
  The trailing whitespace could theoretically cause inconsistency in the codebase if some files have it and others don't. It's a valid style concern.
  However, this is exactly the kind of minor formatting issue that should be handled by automated tooling like ESLint or Prettier, not manual code review comments. Code review should focus on more substantive issues.
  Delete this comment as it's too minor of an issue to warrant a code review comment. This should be handled by automated formatting tools instead.

13. src/main.ts:303

• Draft comment:
  Typo: The variable 'metadatadt' on line 303 looks like it might be a misspelling. Consider renaming it to something more clear like 'metadataStr' or simply 'metadataString' for better readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_ShJX6T8u456sSpCP

---

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.

[ellipsis-dev]

Avoid using process.exit(0) in setup-only mode as it may bypass necessary cleanup in post steps. Consider returning gracefully after final cleanup is performed.

Suggested change

	process.exit(0);
	return;

[ellipsis-dev]

Consider removing nohup from the buildkitdCommand when using execa in detached mode; it may be redundant.

Suggested change

	const buildkitdCommand = 'nohup sudo buildkitd --debug --config=buildkitd.toml --allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host > buildkitd.log 2>&1 &';
	const buildkitdCommand = 'sudo buildkitd --debug --config=buildkitd.toml --allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host > buildkitd.log 2>&1 &';

[ellipsis-dev]

❌ Changes requested. Incremental review on 530a66c in 1 minute and 44 seconds

More details

• Looked at 560 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. action.yml:14

• Draft comment:
  The new input 'setup-only' is correctly added. Ensure that naming is consistent across the code (YAML 'setup-only' vs. code 'setupOnly').
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. main.ts:77

• Draft comment:
  Using an empty string for dockerfilePath in setup-only mode is acceptable if intended for skipping build-reporting; ensure downstream functions handle an empty dockerfile path gracefully.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. main.ts:227

• Draft comment:
  Exiting the process with process.exit(0) in setup-only mode is abrupt. Consider a graceful return to improve testability and ensure proper cleanup.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. setup_builder.ts:129

• Draft comment:
  Using execa for starting buildkitd in setup-only mode is a good alternative to spawn. Verify that the detached process behaves as expected and that logs are correctly captured.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. setup_builder.ts:354

• Draft comment:
  In setupStickyDisk, skipping build reporting when setupOnly is true is reasonable, but ensure that an empty dockerfile path does not cause issues in downstream reporting.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

6. src/main.ts:77

• Draft comment:
  In setup-only mode, the dockerfilePath is set to an empty string. Please ensure that downstream functions gracefully handle an empty path, as they might expect a valid value for reporting.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that downstream functions handle an empty path gracefully. This is a request for confirmation or verification, which violates the rule against asking the author to ensure behavior is intended or tested.

7. src/setup_builder.ts:129

• Draft comment:
  When in setup-only mode, the code uses 'execa' with a detached nohup shell command. Please verify that this reliably detaches and logs output consistently across environments.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify the behavior of the code across environments, which violates the rule against asking for confirmation or verification of behavior. It does not provide a specific suggestion or point out a clear issue with the code.

8. src/reporter.ts:183

• Draft comment:
  The metric value is wrapped using BigInt. Ensure that the receiving agent supports BigInt values in the metric payload.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

9. action.yml:18

• Draft comment:
  Typo in the 'add-hosts' description: 'List of a customs host-to-IP mapping (e.g., docker:10.180.0.1)' should be corrected to 'List of a custom host-to-IP mapping (e.g., docker:10.180.0.1)'.
• Reason this comment was not posted:
  Comment was on unchanged code.

10. action.yml:25

• Draft comment:
  Typo in the 'annotations' description: 'List of annotation to set to the image' should be 'List of annotations to set to the image'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  The file is missing a newline at the end. Please add a newline to comply with POSIX standards and ensure consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While missing newlines at end of file can cause issues with some tools and is considered good practice, this is a very minor issue that would likely be caught by linting or formatting tools. It's not a logic issue or code quality problem that requires manual review attention. The comment, while technically correct, is too minor to warrant manual intervention.
  The missing newline could potentially cause issues with some source control systems or tools. Some would argue this is a valid standard to enforce.
  While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It doesn't affect functionality or code quality.
  Delete this comment as it's too minor of an issue for manual review. This should be handled by automated formatting tools.

12. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  There is trailing whitespace after the closing brace on line 23, and the file does not end with a newline. Please remove the extra space and add a newline at the end of the file to conform with standard coding conventions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While this is technically correct, these are very minor style issues that should be handled by automated formatting tools or pre-commit hooks. The comment doesn't point out any logical issues or suggest meaningful improvements to the code. Most IDEs automatically handle these issues.
  The issues pointed out could affect git diffs and some tools do enforce newlines at end of file. Some teams consider consistent formatting important for maintainability.
  While formatting consistency is good, these are extremely minor issues that should be handled automatically rather than through code review comments. They don't affect functionality or meaningful code quality.
  Delete this comment as it points out trivial formatting issues that should be handled by automated tools rather than manual review.

13. src/__mocks__/execa.ts:7

• Draft comment:
  There's a trailing whitespace at the end of line 7. It's a minor cosmetic issue, but it's best to remove the extra space to maintain consistency in code formatting.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Trailing whitespace is a minor formatting issue that should be handled by automated linting or formatting tools, not manual code review. These kinds of comments create noise without adding value. Most modern IDEs and git configs automatically strip trailing whitespace.
  Perhaps maintaining consistent formatting is important for this project, and they don't have automated tools set up for this yet?
  Even if formatting consistency is important, this is too minor to warrant a PR comment. This should be handled by tooling, not human reviewers.
  Delete this comment as it addresses a trivial formatting issue that should be handled by automated tools rather than manual review.

Workflow ID: wflow_ajowG8pRKiWDId9U

---

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.

[ellipsis-dev]

Avoid calling process.exit(0) directly in the action. Instead, consider an early return to allow proper cleanup and state persistence in GitHub Actions.

Suggested change

	process.exit(0);
	return;

[ellipsis-dev]

❌ Changes requested. Incremental review on 04586e8 in 2 minutes and 3 seconds

More details

• Looked at 568 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. src/main.ts:226

• Draft comment:
  Using process.exit(0) for setup-only mode bypasses any further cleanup. Ensure that skipping post-run cleanup (like removing temp directories and logging) is acceptable.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment raises a valid point about cleanup being skipped. However, looking at the code more carefully:

1. The setup-only mode is specifically designed to just set up the builder without running a build
2. The main cleanup that would be skipped is unmounting disks and committing sticky disks
3. The code already handles cleanup before the process.exit(0) in the main try/catch block
4. The post section appears to be redundant cleanup in case the main cleanup fails
   I could be wrong about the post section being redundant - maybe there are important cleanup tasks that really need to happen even in setup-only mode.
   Looking at the code again, the cleanup in the main block (around line 353) is comprehensive and handles all the same tasks as the post block. The post block appears to be just a safety net.
   The comment raises a valid concern but appears to be incorrect - the code already handles cleanup properly before the process.exit(0), so no changes are needed.

2. src/main.ts:77

• Draft comment:
  Passing an empty string as dockerfile path in setup-only mode is appropriate, but ensure downstream functions (like setupStickyDisk) properly handle an empty path.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. src/setup_builder.ts:129

• Draft comment:
  The use of execa in the setup-only branch spawns a detached process via a shell command. Ensure that the command's output and error handling are sufficient to detect startup issues.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that the command's output and error handling are sufficient, which violates the rule against asking the author to ensure things. It does not provide a specific suggestion or ask for a specific test to be written.

4. src/context.ts:99

• Draft comment:
  Ensure the naming consistency for the 'setup-only' boolean. The input is named 'setup-only' but later referenced as 'setupOnly' in code; confirm that this transformation follows internal conventions.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. src/reporter.ts:183

• Draft comment:
  Creating the Metric instance using a BigInt for the value is unusual. Ensure that downstream systems correctly handle this type.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

6. action.yml:14

• Draft comment:
  Consider expanding the description for the new 'setup-only' input to include an example or more context on its intended use. This will help users understand when and why to enable it.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

7. src/setup_builder.ts:129

• Draft comment:
  In the setup-only branch, a shell command using 'nohup' with execa is used to spawn buildkitd. This approach differs from the spawn method used in the normal branch. For consistency and maintainability it might help to unify the process-spawning strategy.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The two approaches serve different purposes - one is for attached processes (spawn) and one is for detached background processes (nohup + execa). The difference is intentional based on the setupOnly flag. Using nohup is a standard way to run detached processes. Unifying them would likely make the code more complex or lose the desired behavior differences.
  The comment raises a valid point about code consistency. Having two different ways to do similar things can sometimes lead to maintenance issues.
  However, in this case the different approaches are justified by different requirements - attached vs detached processes. Forcing a single approach would likely make the code worse.
  The comment should be deleted. The different approaches are intentional and appropriate for their different use cases. Unifying them would not improve the code.

8. jest.config.ts:8

• Draft comment:
  Switching the transformer for both TS and JS files to babel-jest (instead of ts-jest for TS files) could have unforeseen effects on TypeScript specific features. Confirm that this change is intentional.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

9. action.yml:19

• Draft comment:
  Typo: In the add-hosts description, 'a customs host-to-IP mapping' should likely be 'a custom host-to-IP mapping'.
• Reason this comment was not posted:
  Comment was on unchanged code.

10. action.yml:25

• Draft comment:
  Typo: In the annotations description, 'List of annotation to set to the image' should be 'List of annotations to set on the image' to improve grammatical correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  Minor cosmetic issue: The closing brace on line 12 has trailing whitespace. Please remove the extra space.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While technically correct, comments about whitespace are usually not worth calling out in PR reviews. These issues are better handled by automated formatters or linters. It's too minor to warrant human attention and discussion. Additionally, this is in a mock file, which makes it even less important.
  The comment is technically accurate and does point out a real issue. Some teams might have strict formatting requirements.
  Even for teams with strict formatting requirements, this kind of issue should be caught and fixed by automated tools, not manual review comments. It's noise in the PR review process.
  Delete this comment as it's too minor and would be better handled by automated tooling.

12. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  The file is missing a newline at the end. Adding a trailing newline can help prevent some issues in certain environments.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While missing newlines at EOF can cause minor issues in some situations (like git diffs or concatenation), this is a relatively minor issue. Modern editors and build tools typically handle this automatically. This is more of a style/convention issue than a critical problem. The comment doesn't suggest a clear code change that would significantly improve the code quality.
  The missing newline could potentially cause issues in some development workflows or tools. Some linters and build systems explicitly check for this.
  While true, this is the kind of minor issue that would be better handled by automated tooling (like EditorConfig or a linter) rather than manual code review comments.
  Delete the comment as it's too minor of an issue for a PR review comment and would be better handled by automated tooling.

13. src/__mocks__/execa.ts:7

• Draft comment:
  Minor issue: There's a trailing whitespace after the closing parenthesis on line 7. It would be best to remove it to keep the code style consistent.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing whitespace, this is an extremely minor code style issue. Most IDEs automatically trim trailing whitespace, and this kind of nitpick doesn't improve code quality in any meaningful way. It's the kind of thing that should be handled by automated linting rather than PR comments.
  The trailing whitespace could be part of the team's style guide and considered important for consistency.
  Even if it is part of a style guide, this is too minor to warrant a manual PR comment and should be caught by automated tooling like ESLint or Prettier.
  Delete this comment as it's too minor and should be handled by automated tooling rather than manual review comments.

Workflow ID: wflow_WdjGjeFkEVfYKybf

---

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.

[ellipsis-dev]

Using process.exit(0) in setup-only mode may prematurely terminate the runner and skip important post/cleanup steps. Consider refactoring to return early so that the post step (and its cleanup logic) is still executed.

Suggested change

	process.exit(0);
	return;

[ellipsis-dev]

❌ Changes requested. Incremental review on ab514e3 in 1 minute and 49 seconds

More details

• Looked at 570 lines of code in 12 files
• Skipped 2 files when reviewing.
• Skipped posting 22 drafted comments based on config settings.

1. src/main.ts:230

• Draft comment:
  Using process.exit(0) in setup-only mode abruptly ends execution and bypasses any remaining cleanup. Consider ensuring that all necessary cleanup routines are called before exiting.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment raises a valid concern - process.exit(0) would skip the post cleanup section. However, looking at the code more carefully, I notice that the main cleanup routines are actually duplicated in two places: once in the main try/catch block (L355-435) and once in the post section (L443-504). The code does proper cleanup before the process.exit(0) call, including leaving tailnet, shutting down buildkitd, and unmounting disks. So while the comment is technically correct that it bypasses the post section, it's not actually causing any issues since cleanup is handled beforehand.
  Am I sure that ALL cleanup routines are duplicated? Could there be some unique cleanup in the post section that would be missed?
  Looking at the post section again, it performs the exact same steps in the same order: leave tailnet, shutdown buildkitd, unmount disks, clean temp dir, and commit sticky disk. There are no unique cleanup routines that would be missed.
  The comment should be deleted. While technically correct that process.exit(0) bypasses the post section, all cleanup routines are already handled in the main section before the exit call, so there is no actual issue here.

2. src/main.ts:177

• Draft comment:
  The comment '// Let's remove the default' is ambiguous. Ensure the intended behavior and consider clarifying or removing the comment if no action is taken.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. src/setup_builder.ts:128

• Draft comment:
  The use of execa with a nohup command in setup-only mode creates a detached process. Confirm this behavior is intended and consider logging or handling potential errors that may arise from a detached process.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment asks for confirmation of intended behavior and suggests considering error handling. However, the code shows this is clearly intentional through the if/else structure, and error handling is already implemented. The comment violates the rule about not asking authors to confirm their intentions. It's also not pointing out a clear issue that needs fixing.
  Maybe there are specific error handling patterns for detached processes that aren't being followed here? Maybe the comment sees a real issue with the error handling approach?
  The code already has comprehensive error handling through the error event handler and log piping. The detached process is an intentional design choice for setup-only mode. Any additional error handling would be speculative.
  Delete this comment as it violates the rule about asking authors to confirm intentions and doesn't identify a clear, actionable issue that needs fixing.

4. src/setup_builder.ts:367

• Draft comment:
  The inode usage check in setupStickyDisk reports a warning if usage >80%. Consider if this should trigger a non-recoverable error or additional remediation steps.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

5. src/reporter.ts:183

• Draft comment:
  When constructing a new Metric in reportMetric, ensure that the Metric class constructor accepts an object as used. Verify compatibility with the external library's expected API.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. action.yml:14

• Draft comment:
  New 'setup-only' input added. Ensure that the YAML kebab-case name ('setup‑only') is documented as being converted to camelCase ('setupOnly') in the code.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

7. jest.config.ts:8

• Draft comment:
  The combined regex '^.+.(ts|js)$' and the new moduleNameMapper entries for the buf mocks simplify the configuration. This looks good.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

8. package.json:29

• Draft comment:
  Babel packages have been updated and an 'execa' dependency added. Verify that these versions work well together in your environment. Also, consider ensuring the file ends with a newline for consistency.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

9. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  Consider adding a trailing newline at the end of the file for POSIX compliance.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

10. src/__mocks__/@buf/blacksmith_vm-agent.connectrpc_es/stickydisk/v1/stickydisk_connect.ts:23

• Draft comment:
  Consider adding a newline at the end of the file.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

11. src/__tests__/blacksmith-builder.test.ts:47

• Draft comment:
  Test mocks now include the new 'setupOnly' property and correctly expect 'startAndConfigureBuildkitd' to be called with an additional boolean parameter. This update looks appropriate.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

12. src/context.ts:99

• Draft comment:
  Added 'setupOnly' flag to the Inputs interface and getInputs. This change is consistent with the new workflow input and its usage in other parts of the code.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

13. src/main.ts:77

• Draft comment:
  When 'setupOnly' is true, an empty string is used for the dockerfilePath. While this avoids reporting a build, consider whether using null might better express the absence of a dockerfile.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

14. src/reporter.ts:195

• Draft comment:
  The new commitStickyDisk function nicely wraps the agent client's commitStickyDisk call. Consider logging more details on errors if debugging sticky disk issues becomes necessary.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

15. src/setup_builder.ts:128

• Draft comment:
  The 'startBuildkitd' function now accepts a 'setupOnly' parameter and uses a nohup + execa approach when true. Ensure that the detached process is properly managed and cleaned up afterwards.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

16. src/setup_builder.ts:354

• Draft comment:
  In setupStickyDisk, the 'setupOnly' flag properly bypasses build reporting. Verify that downstream consumers correctly handle an undefined buildId when in setup-only mode.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

17. src/state-helper.ts:62

• Draft comment:
  New functions setExposeId and getExposeId have been added for state management. This looks consistent with how other state values are handled.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

18. action.yml:19

• Draft comment:
  Typographical error: consider changing 'customs' in 'List of a customs host-to-IP mapping' to 'custom' (or 'custom host-to-IP mapping') for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

19. action.yml:25

• Draft comment:
  Typographical error: consider updating the description from 'List of annotation to set to the image' to 'List of annotations to set on the image' for grammatical correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

20. src/__mocks__/@buf/blacksmith_vm-agent.bufbuild_es/stickydisk/v1/stickydisk_pb.ts:12

• Draft comment:
  There is no newline at the end of the file. Although this is a minor stylistic issue, adding a final newline is considered best practice in many projects.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
  The comment is about a stylistic issue, specifically the lack of a newline at the end of a file. While this is a common best practice, the comment is purely informative and doesn't suggest a specific code change or highlight a potential issue with the code's functionality. It doesn't align with the rules for useful comments, which should focus on code suggestions or potential issues.

21. src/__mocks__/execa.ts:7

• Draft comment:
  Minor: There is a trailing whitespace after the closing parenthesis on line 7 that should be removed for consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the trailing whitespace, this is an extremely minor formatting issue that would typically be handled by code formatters or linters. It doesn't affect functionality and is not a significant code quality issue. The rules state not to make comments that are obvious or unimportant.
  Perhaps consistent formatting is more important than I'm giving it credit for, especially in a new file that's being added.
  While consistency is good, this is too minor of an issue to warrant a PR comment. This should be handled by automated tooling rather than manual review comments.
  Delete this comment as it points out an extremely minor formatting issue that doesn't warrant manual intervention in code review.

22. src/state-helper.ts:68

• Draft comment:
  It looks like the file doesn't end with a newline character. Please add a newline at the end of the file to maintain consistency with common file formatting practices.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The rules state not to make purely informative comments or obvious/unimportant ones. While missing newlines can cause issues in some cases, this is a minor style issue that would likely be caught by linting or formatting tools. The comment doesn't suggest a critical code change that would affect functionality.
  Missing newlines can cause issues with some tools and git diffs. It's a widely accepted best practice to end files with newlines.
  While true, this is exactly the kind of minor style issue that should be handled by automated tools rather than manual review comments. It doesn't affect the logic or functionality.
  This comment should be deleted as it's a minor style issue that doesn't require manual review attention.

Workflow ID: wflow_Wh7kboX6xjbkycmy

---

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.

[ellipsis-dev]

Exiting early with process.exit(0) in setup-only mode may bypass some cleanup routines. Consider a more graceful termination to ensure all resources are properly released.