refactor(wrappers): extract route logic into composite actions#172
Merged
Conversation
Wrappers totalled 6,515 lines against ADR 0004's thin-wrapper intent. The sdd-execute trio was ~1,900 lines of pure copy; every wrapper re-inlined its github-script decide block and shared helpers because there was no shared-code mechanism for hand-written wrappers. Move the heavy deterministic logic into composite actions hosted in .github/actions/, referenced cross-repo by pinned ref like locks and shared fragments. Wrappers keep triggers, concurrency, the permission gate, and the lock call. Extraction is verbatim; only vars/TIER become action inputs. - 14 composite actions; sdd-route-execute is shared by all three tiers via a tier input, eliminating the triplication - wrappers: 6,515 -> 2,078 lines (sdd-execute 963 -> 245 each, dispatch 1,199 -> 506) - installer pins .github/actions refs alongside lock refs - ADR 0015 records the pattern and the vars/secrets->inputs rule actionlint, shellcheck, markdownlint clean; lock drift gate untouched; extracted blocks proven byte-identical to their origins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughExtracts routing, precondition, and utility logic into hosted composite GitHub Actions and updates wrapper workflows and installer tooling to delegate to those actions. ChangesRefactor wrapper routing logic into composite actions
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The command-table consistency check scanned only wrappers/*.yml for the firstWord === '/<name>' routing idiom. ADR 0015 moved that idiom into the composite actions, so the check reported every command as unrouted. Scan .github/actions/*/action.yml alongside the wrappers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/actions/sdd-fastpath-approve/action.yml:
- Around line 208-230: When moving lifecycle labels on the tracking issue, also
attempt to remove the alternative fast-path label 'sdd:fastpath-review' in the
same way you remove 'sdd:fastpath' so a stale label isn't left behind; update
the try/catch block(s) around github.rest.issues.removeLabel (the block that
currently targets 'sdd:fastpath') to attempt removal of both 'sdd:fastpath' and
'sdd:fastpath-review' (either two removeLabel calls each wrapped with their own
try/catch or a loop over the two names), and ensure error logging uses core.info
as done now; keep the subsequent github.rest.issues.addLabels call for
'sdd:in-progress' unchanged.
In @.github/actions/sdd-monitor/action.yml:
- Around line 27-28: Remove the duplicated "script: |" literal inside the with:
block so only a single "script: |" key defines the multiline script, and then
outdent all lines of the script body (reduce their indentation by two spaces) so
the script content is properly nested under the single script key; locate the
duplicated line in the action's with: block (the excessive "script: |" entry)
and adjust the following lines' indentation accordingly.
In @.github/actions/sdd-route-dispatch/action.yml:
- Around line 39-41: The yaml contains an accidental nested "script: |" inside
the actions/github-script body which becomes part of the JavaScript and causes a
syntax error; remove the inner "script: |" so the block begins directly with the
JS (e.g., the line before "const event = context.eventName;"), ensuring the
actions/github-script "script" key contains only the intended JS code.
In @.github/actions/sdd-route-execute/action.yml:
- Around line 614-616: The code sets maxIters via
parseInt(process.env.SDD_MAX_REVIEW_ITERATIONS || '3', 10) which yields NaN for
non-numeric env values and thus disables the cap; change the logic around
parseInt to validate and coerce the value: read the env var, parseInt with radix
10 into maxIters, then if Number.isNaN(maxIters) or maxIters < 1 set maxIters to
a safe default (3) — ensure priorIters comparison (priorIters >= maxIters) uses
this validated numeric maxIters so the auto-revise cap cannot be bypassed by a
bad env value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c98f13e-82a5-4cec-9d44-55392d30305e
📒 Files selected for processing (28)
.github/actions/sdd-auto-merge/action.yml.github/actions/sdd-dispatch-compute/action.yml.github/actions/sdd-fastpath-approve/action.yml.github/actions/sdd-monitor/action.yml.github/actions/sdd-pr-sanitize/action.yml.github/actions/sdd-route-dispatch/action.yml.github/actions/sdd-route-execute/action.yml.github/actions/sdd-route-review/action.yml.github/actions/sdd-route-spec/action.yml.github/actions/sdd-route-triage/action.yml.github/actions/sdd-route-validate/action.yml.github/actions/sdd-spec-failure-handoff/action.yml.github/actions/sdd-triage-dedupe-tasks/action.yml.github/actions/sdd-triage-promote-ready/action.ymldecisions/0015-wrapper-logic-in-composite-actions.mdscripts/quick-setup.shwrappers/sdd-dispatch.ymlwrappers/sdd-execute-haiku.ymlwrappers/sdd-execute-opus.ymlwrappers/sdd-execute-sonnet.ymlwrappers/sdd-monitor.ymlwrappers/sdd-pr-sanitize.ymlwrappers/sdd-review.ymlwrappers/sdd-spec.ymlwrappers/sdd-triage-dedupe-tasks.ymlwrappers/sdd-triage-promote-ready.ymlwrappers/sdd-triage.ymlwrappers/sdd-validate.yml
An off-by-one in the extraction prepended a stray 'script: |' line to the github-script body in sdd-monitor and sdd-route-dispatch, which would throw a JS syntax error at runtime. The per-action parity check compared against a slice that included the same stray line, so it passed falsely. Both bodies now match their pre-refactor originals byte-for-byte (base 351b779). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two small defensive fixes, kept separate from the verbatim refactor: - sdd-route-execute: guard a non-numeric SDD_MAX_REVIEW_ITERATIONS so a NaN cap cannot silently disable the auto-revise bound (falls back to 3). - sdd-fastpath-approve: also clear sdd:fastpath-review during the lifecycle transition so the both-labels-present edge cannot strand a stale label; no-op when absent. Completes the intent noted in the existing comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/test-command-table.py (1)
201-211:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate error messages to reflect that routing sources now include composite actions.
The error messages mention only
wrappers/*.yml, but the code now scans bothwrappers/*.ymland.github/actions/*/action.yml(per the refactor in ADR 0015). This creates a mismatch between the diagnostic output and the actual behavior.📝 Proposed fix to update error messages
for cmd in sorted(wrapper_not_in_table): - wrappers = ", ".join(routes[cmd]) + sources = ", ".join(routes[cmd]) failures.append( - f"wrapper routes `{cmd}` ({wrappers}) but it is not in " + f"routing layer routes `{cmd}` ({sources}) but it is not in " f"shared/sdd-interaction.md command table" ) for cmd in sorted(table_not_in_wrapper): failures.append( f"shared/sdd-interaction.md command table includes `{cmd}` " - f"but no wrappers/*.yml routes it" + f"but the routing layer (wrappers + composite actions) does not route it" )🤖 Prompt for 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. In `@scripts/test-command-table.py` around lines 201 - 211, The diagnostic strings are outdated: update the two failures.append messages that reference only "wrappers/*.yml" (the ones built from variables wrapper_not_in_table/routes and table_not_in_wrapper) to mention both sources now scanned (e.g. "wrappers/*.yml and .github/actions/*/action.yml" or "wrappers and composite actions"), so the error text matches the refactor that includes composite actions; change the messages at the failures.append calls that construct those two errors accordingly.
🤖 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.
Outside diff comments:
In `@scripts/test-command-table.py`:
- Around line 201-211: The diagnostic strings are outdated: update the two
failures.append messages that reference only "wrappers/*.yml" (the ones built
from variables wrapper_not_in_table/routes and table_not_in_wrapper) to mention
both sources now scanned (e.g. "wrappers/*.yml and .github/actions/*/action.yml"
or "wrappers and composite actions"), so the error text matches the refactor
that includes composite actions; change the messages at the failures.append
calls that construct those two errors accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f008b5f9-2e41-4b1e-abf1-12d5083775e7
📒 Files selected for processing (5)
.github/actions/sdd-fastpath-approve/action.yml.github/actions/sdd-monitor/action.yml.github/actions/sdd-route-dispatch/action.yml.github/actions/sdd-route-execute/action.ymlscripts/test-command-table.py
💤 Files with no reviewable changes (2)
- .github/actions/sdd-route-dispatch/action.yml
- .github/actions/sdd-monitor/action.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/sdd-fastpath-approve/action.yml
- .github/actions/sdd-route-execute/action.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Wrappers totalled 6,515 lines against ADR 0004's thin-wrapper intent. Two causes:
sdd-execute-{haiku,sonnet,opus}were line-identical exceptTIER, themodel:<tier>gate, the concurrency prefix, and the lock ref — ~1,900 lines of pure copy.shared/*.mdare gh-aw prompt imports, unusable by hand-written wrappers; there were no composite actions. Every wrapper re-inlined itsgithub-scriptdecide block plus shared helpers (permission gate, parent-walk, tree-walk). Theactions/github-scriptSHA pin appeared 21 times.Fix
Move the heavy deterministic logic into 14 composite actions in
.github/actions/, referenced cross-repo by pinned ref — the same mechanism ADR 0004 uses for locks and ADR 0002 forshared/fragments. Wrappers keep triggers, concurrency, the permission gate, and the lock call (ADR 0004 §7: the route job stays the real gate).sdd-route-executeis shared by all three tiers via atierinput, eliminating the triplication.Extraction is verbatim; the only edits are
vars.*/TIER→ action inputs. Secrets never enter an action — the auto-merge App token is minted in the wrapper and passed asgithub-token.Other changes
scripts/quick-setup.shrewrites.github/actions/*@<ref>alongside lock refs, or a--refinstall would leave actions on@mainwhile the lock is pinned.uses:-can't-be-dynamic constraint, and thevars/secrets→inputsrule.Verification
.github/workflows.@main.Notes
action.yml(treats them as workflows), so nolint.ymlglob change — actions are covered by YAML parse + the e2e run.distillery-sync.yml(no route script) and dispatch's small matrix/label/comment scripts (tightly coupled to the job graph, low value)./execute, a CHANGES_REQUESTED auto-revise, aneeds-humanresume, a merged fast-path PR) is the remaining behavioral check before merge.🤖 Generated with Claude Code