Skip to content

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

Merged
wpak-ai merged 4 commits into
cppalliance:masterfrom
whisper67265:fix/require-submodules-in-trigger-script
Jul 31, 2026
Merged

Remove the silent two-library default from trigger-add-submodules.sh#73
wpak-ai merged 4 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 and rejects empty or whitespace-only values.
    • Dispatch payloads continue omitting empty fields and now report failures with the correct status.
  • Documentation

    • Clarified API dispatch instructions, submodule requirements, token usage, and workflow token mappings.
  • Tests

    • Added coverage for trigger validation, payload generation, repository and token handling, translation dispatches, and GitHub API errors.
    • Improved dispatch request mocking and payload inspection.

@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: 7e45a4fb-1389-43da-8803-b0ade6b7ac9f

📥 Commits

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

📒 Files selected for processing (11)
  • .github/workflows/assets/lib.sh
  • .github/workflows/heartbeat.yml
  • CHANGELOG.md
  • README.md
  • docs/GETTING-STARTED.md
  • scripts/trigger-add-submodules.sh
  • scripts/trigger-dispatch-common.sh
  • tests/helpers/http_mock.bash
  • tests/test_trigger_add_submodules.bats
  • tests/test_trigger_dispatch_common.bats
  • tests/test_trigger_start_translation.bats
🚧 Files skipped from review as they are similar to previous changes (5)
  • docs/GETTING-STARTED.md
  • tests/test_trigger_start_translation.bats
  • CHANGELOG.md
  • tests/helpers/http_mock.bash
  • scripts/trigger-add-submodules.sh

📝 Walkthrough

Walkthrough

The change requires --submodules for the add-submodules trigger, preserves API-side auto-discovery, updates dispatch JSON construction and exit-status handling, clarifies token mappings, and adds Bats coverage with an offline GitHub dispatch stub.

Changes

Trigger dispatch updates

Layer / File(s) Summary
Shared dispatch helpers and test infrastructure
scripts/trigger-dispatch-common.sh, tests/helpers/http_mock.bash, tests/test_trigger_dispatch_common.bats, CHANGELOG.md
Dispatch JSON uses positional jq arguments. Repository and token resolution, HTTP handling, fallback status propagation, and mock infrastructure have test coverage.
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, empty, or whitespace-only --submodules values. Documentation distinguishes the required CLI argument from API auto-discovery. 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. Local instructions describe API dispatch usage.
Workflow token mapping documentation
.github/workflows/assets/lib.sh, .github/workflows/heartbeat.yml, README.md, docs/GETTING-STARTED.md
Documentation distinguishes workflow SYNC_TOKEN/GITHUB_TOKEN mappings from the ephemeral GH_TOKEN used by gh.

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

Suggested reviewers: wpak-ai

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning README.md and workflow files add token-mapping documentation that is not covered by [#65] or the stated PR objectives. Remove the unrelated token documentation changes, or link an issue that defines this additional scope.
Docstring Coverage ⚠️ Warning Docstring coverage is 27.59% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: removing the silent default from trigger-add-submodules.sh.
Linked Issues check ✅ Passed The changes satisfy [#65] by requiring non-empty --submodules, updating documentation, fixing jq 1.7 handling, and adding the required Bats coverage.
✨ 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
@whisper67265
whisper67265 requested a review from wpak-ai July 31, 2026 09:11
@wpak-ai
wpak-ai merged commit 1aa62f4 into cppalliance:master Jul 31, 2026
4 checks passed
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

3 participants