Skip to content

fix: dedupe builtin harness flag aliases#2071

Merged
yohamta0 merged 1 commit into
mainfrom
fix/harness-dedupe-builtin-flag-aliases
Apr 30, 2026
Merged

fix: dedupe builtin harness flag aliases#2071
yohamta0 merged 1 commit into
mainfrom
fix/harness-dedupe-builtin-flag-aliases

Conversation

@yohamta0

@yohamta0 yohamta0 commented Apr 30, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Collaborator

Summary

  • normalize built-in harness flag aliases to kebab-case before merging DAG harness defaults, step with config, and provider default flags
  • prevent duplicate --skip-git-repo-check flags when the same option is provided with mixed snake_case and kebab-case keys
  • add regression coverage for DAG harness inheritance and built-in provider config merging

Root Cause

Built-in harness providers normalize option keys from snake_case to kebab-case only when building CLI args. That let equivalent keys like skip_git_repo_check and skip-git-repo-check coexist in merged config maps and produce duplicate CLI flags.

Testing

  • make fmt
  • go test ./internal/core ./internal/core/spec ./internal/runtime/builtin/harness -count=1

Summary by CodeRabbit

  • Bug Fixes

    • Fixed harness flag alias handling during step configuration overrides to prevent duplicate entries when using underscore and hyphenated versions of the same flag.
    • Improved configuration merging to consistently normalize builtin harness flag keys across default and override layers.

  • Tests

    • Added test coverage for harness flag alias deduplication behavior.

@coderabbitai

coderabbitai Bot commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e34e3fc-2af2-4976-a041-acdcb95c54ab

📥 Commits

Reviewing files that changed from the base of the PR and between f689f32 and d869e33.

📒 Files selected for processing (5)
  • internal/core/harness.go
  • internal/core/spec/builder_test.go
  • internal/core/spec/step.go
  • internal/runtime/builtin/harness/harness.go
  • internal/runtime/builtin/harness/harness_test.go
📝 Walkthrough

Walkthrough

This PR introduces a normalization pipeline for builtin harness flag configuration that canonicalizes underscore-separated keys to kebab-case while preserving reserved keys, integrates this normalization into spec-level and runtime-level config merging, and updates tests to verify alias deduplication and override behavior.

Changes

Cohort / File(s) Summary
Core Normalization
internal/core/harness.go		 Introduces NormalizeBuiltinHarnessFlagKeys function that clones input maps, canonicalizes non-reserved keys by converting underscores to kebab-case, merges entries with the same canonical key, and deep-copies values. Imports strings to support canonicalization.
Spec-level Config Merging
internal/core/spec/step.go, internal/core/spec/builder_test.go		 Updates mergeHarnessConfig to determine effective provider from step-level config with DAG-level fallback, then normalizes builtin harness flag keys for both step and DAG configs before merging. Adds test case verifying hyphenated alias overrides underscored variant without retaining both keys.
Runtime Provider Config Merging
internal/runtime/builtin/harness/harness.go, internal/runtime/builtin/harness/harness_test.go		 Updates mergeProviderDefaultConfig to normalize builtin harness flag keys for both default and incoming configs before merging. Test expectations updated to use normalized flag names (skip-git-repo-check), with new subtest verifying alias deduplication.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PR #1978: Introduced the harness executor and provider defaults logic that this PR now complements with key normalization for consistent flag handling across builtin harness configurations.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: deduplicating builtin harness flag aliases by normalizing them to kebab-case before merging configurations.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/harness-dedupe-builtin-flag-aliases

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@yohamta0 yohamta0 merged commit a70a446 into main Apr 30, 2026
16 of 17 checks passed
@yohamta0 yohamta0 deleted the fix/harness-dedupe-builtin-flag-aliases branch April 30, 2026 07:37
@coderabbitai coderabbitai Bot mentioned this pull request May 31, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@yohamta0