Skip to content

fix(actions): compute hello world wf#5109

Merged
abcxff merged 1 commit into
mainfrom
05-27-fix_actions_compute_hello_world_wf
Jun 1, 2026
Merged

fix(actions): compute hello world wf#5109
abcxff merged 1 commit into
mainfrom
05-27-fix_actions_compute_hello_world_wf

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented May 27, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Contributor Author

abcxff commented May 27, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5109 May 27, 2026 02:16 Destroyed
@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 27, 2026

🚅 Deployed to the rivet-pr-5109 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web Jun 1, 2026 at 7:34 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jun 1, 2026 at 4:49 pm
website 😴 Sleeping (View Logs) Web May 30, 2026 at 3:20 am
frontend-inspector ❌ Build Failed (View Logs) Web May 27, 2026 at 2:21 am
ladle ✅ Success (View Logs) Web May 27, 2026 at 2:19 am
mcp-hub ✅ Success (View Logs) Web May 27, 2026 at 2:17 am

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 27, 2026

Code Review PR 5109

Summary: This PR makes two changes.

  1. docker/compute-hello-world/server.js - Moves the startup log from the server.listen callback to module-level, replacing the callback with an empty function.
  2. .github/workflows/compute-hello-world.yml - Restricts the :latest Docker tag to push events only (not workflow_dispatch).

server.js Bug: Log fires before server is ready to accept connections

server.listen is asynchronous. The callback fires after the OS bind completes. Moving the log to module level means it prints before listen succeeds. If listen fails (e.g. port already bound), the message will have already printed, giving users a false signal that the server is ready.

Recommendation: Keep console.log inside the listen callback, updating only the message text.

server.js Code quality: empty callback

Replacing the callback with an empty arrow function is a no-op. Either omit the callback entirely or restore a meaningful log inside it.


compute-hello-world.yml Semantic issue: workflow_dispatch no longer tags :latest

The original behavior tagged :latest for both push and workflow_dispatch. The new code silently drops :latest from manual dispatch runs. If an operator triggers a manual run to hotfix the image, :latest will not be updated and may be left stale.

Whether this is intentional is not clear - the PR description is blank. If intentional, add a comment explaining the rationale.

compute-hello-world.yml Code quality: empty string produces a blank tag line

When github.event_name is workflow_dispatch, the ternary evaluates to an empty string, giving docker/build-push-action a tags input with a blank line. Most versions of the action ignore empty entries silently, but this is fragile.


General

  • PR description is blank. For a change that alters observable server startup behavior and Docker tagging semantics, context is important.
  • No tests. A smoke test verifying the server starts and responds on the configured port would catch the moved-log regression.

Summary table

| Location | Severity | Finding |
| server.js | Medium | Log prints before server is bound; false-ready signal if listen fails |
| server.js | Low | Empty callback is noise; omit or restore a meaningful log |
| compute-hello-world.yml | Medium | workflow_dispatch no longer updates :latest; undocumented semantic change |
| compute-hello-world.yml | Low | Blank-line tag entry is fragile across action versions |
| PR description | Low | Empty description, no rationale provided |

The medium-severity server.js issue (log before bind) is the most important to address before merge.

@abcxff abcxff force-pushed the 05-27-fix_actions_compute_hello_world_wf branch from 140985f to 3a5c9b2 Compare June 1, 2026 19:34
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5109 June 1, 2026 19:34 Destroyed
@abcxff abcxff marked this pull request as ready for review June 1, 2026 19:34
@abcxff abcxff merged commit bd7f8bb into main Jun 1, 2026
13 of 14 checks passed
@abcxff abcxff deleted the 05-27-fix_actions_compute_hello_world_wf branch June 1, 2026 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants