Skip to content

fix(pegboard): create missing normal runner configs#4619

Open
NathanFlurry wants to merge 1 commit into04-12-fix_epoxy_reuse_cached_optimistic_valuefrom
04-12-fix_pegboard_create_missing_normal_runner_configs
Open

fix(pegboard): create missing normal runner configs#4619
NathanFlurry wants to merge 1 commit into04-12-fix_epoxy_reuse_cached_optimistic_valuefrom
04-12-fix_pegboard_create_missing_normal_runner_configs

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

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
Member Author

NathanFlurry commented Apr 13, 2026

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

PR Review: fix(pegboard): create missing normal runner configs

Summary: This is a minimal, targeted bug fix - a single character change that inverts a boolean condition.


Correctness

The fix is clearly correct. The function is named pegboard_runner_config_ensure_normal_if_missing and documented as:

Creates a default normal runner config for this namespace and runner name if one does not already exist.

The original condition if !pool_res.is_empty() caused the upsert to fire only when results already existed, which is the exact opposite of the intended behavior. The fix if pool_res.is_empty() correctly creates the config only when none is found, matching the function name and doc comment.

This was a classic logic inversion bug.


Observations

  • Impact: The bug would cause ensure_normal_if_missing to silently no-op on first setup (when no config exists) and redundantly upsert on every subsequent call. Depending on whether callers handle the missing config gracefully downstream, this could cause subtle initialization failures for normal runner configs.
  • No data corruption risk: Since the upsert path uses upsert (not insert), the redundant writes under the old logic would not cause data corruption, just unnecessary work. The real harm is the missing creation on first call.
  • Test coverage: No tests are added or modified. Given this is a bug fix for an "ensure if missing" pattern, a regression test covering the empty-result branch would help prevent recurrence. Not blocking for a one-liner fix, but worth considering.
  • Stale doc comment: The comment says "Returns true when the config was created" but the function signature returns Result<()>. Minor cleanup opportunity.

Verdict

The fix is correct and the change is minimal and safe. Approve once out of draft.

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.

1 participant