Skip to content

Remove the silent two-library default from trigger-add-submodules.sh - #73

Open
whisper67265 wants to merge 2 commits into
cppalliance:masterfrom
whisper67265:fix/require-submodules-in-trigger-script
Open

Remove the silent two-library default from trigger-add-submodules.sh#73
whisper67265 wants to merge 2 commits into
cppalliance:masterfrom
whisper67265:fix/require-submodules-in-trigger-script

Conversation

@whisper67265

@whisper67265 whisper67265 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Close #65.

Summary by CodeRabbit

  • Behavior Changes

    • The add-submodules trigger now requires --submodules; whitespace-only values are rejected with a clear usage/error message.
    • Dispatch JSON generation was adjusted to avoid a jq naming collision and continues omitting empty client payload fields.
  • Documentation

    • Updated getting-started and endpoint contract docs to clarify API-based dispatching and the required --submodules argument.
  • Tests

    • Added/expanded Bats coverage for add-submodules, shared dispatch helpers, and start-translation behavior (including payload validation and non-204 failure handling).
    • Improved test HTTP mocking to safely stub GitHub dispatch requests and inspect logged request bodies.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 14658672-94fa-4207-b8da-ea432f8fd0a0

📥 Commits

Reviewing files that changed from the base of the PR and between fd00796 and 346ea95.

📒 Files selected for processing (2)
  • scripts/trigger-add-submodules.sh
  • tests/test_trigger_add_submodules.bats
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_trigger_add_submodules.bats

📝 Walkthrough

Walkthrough

The change requires --submodules for the add-submodules trigger, preserves API-side auto-discovery behavior, avoids a jq 1.7 naming collision, updates trigger documentation, and adds Bats coverage with an offline GitHub dispatch stub.

Changes

Trigger dispatch updates

Layer / File(s) Summary
Shared dispatch helpers and tests
scripts/trigger-dispatch-common.sh, tests/helpers/http_mock.bash, tests/test_trigger_dispatch_common.bats, CHANGELOG.md
The dispatch JSON reducer uses kv_pairs, while shared tests cover payload construction, repository/token resolution, and HTTP success or failure handling through a curl stub.
Required add-submodules selection
scripts/trigger-add-submodules.sh, docs/GETTING-STARTED.md, docs/endpoint-contract.md, tests/test_trigger_add_submodules.bats, tests/test_getting_started.bats, CHANGELOG.md
The script rejects missing or empty --submodules values, documentation distinguishes CLI requirements from API auto-discovery, and tests verify validation and payload contents.
Translation trigger coverage
tests/test_trigger_start_translation.bats, docs/GETTING-STARTED.md
Tests cover help, unknown options, default and explicit language payloads, and non-204 API responses; sync-translation instructions reference API dispatch usage only.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% 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: removing the silent submodule default from trigger-add-submodules.sh.
Linked Issues check ✅ Passed The PR satisfies #65 by requiring --submodules, updating docs, fixing jq 1.7 payload building, and adding the requested Bats coverage.
Out of Scope Changes check ✅ Passed The added changelog entries, docs wording updates, and test helpers all support the linked objectives and do not appear unrelated.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/trigger-add-submodules.sh`:
- Around line 84-88: Update the SUBMODULES validation in
scripts/trigger-add-submodules.sh to trim leading and trailing whitespace before
checking whether the selection is empty. Reject whitespace-only values through
the existing error, usage, and exit path, while preserving valid comma-separated
library names.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8ec7acf1-07ab-4945-8e54-a4c8f5d75399

📥 Commits

Reviewing files that changed from the base of the PR and between fc7bc90 and fd00796.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • docs/GETTING-STARTED.md
  • docs/endpoint-contract.md
  • scripts/trigger-add-submodules.sh
  • scripts/trigger-dispatch-common.sh
  • tests/helpers/http_mock.bash
  • tests/test_getting_started.bats
  • tests/test_trigger_add_submodules.bats
  • tests/test_trigger_dispatch_common.bats
  • tests/test_trigger_start_translation.bats

Comment thread scripts/trigger-add-submodules.sh
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.

Remove the silent two-library default from trigger-add-submodules.sh

1 participant