Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Embeds the agentic command router directly into generated workflows, removing checkout/setup steps. The resulting script currently exceeds Linux’s per-environment-variable limit.
Changes:
- Adds deterministic CommonJS dependency bundling.
- Inlines the router into
actions/github-script. - Updates generation tests and generated workflow output.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/central_slash_command_workflow.go |
Emits the bundled router inline. |
pkg/workflow/central_slash_command_workflow_test.go |
Updates generation assertions. |
pkg/workflow/agentic_commands_script.go |
Implements module collection and bundling. |
pkg/workflow/agentic_commands_script_test.go |
Tests bundling and missing dependencies. |
actions/setup/js/agentic_commands_embed.go |
Embeds router modules. |
.github/workflows/agentic_commands.yml |
Regenerates the inline router workflow. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| const { main } = require('` + SetupActionDestination + `/route_slash_command.cjs'); | ||
| await main(); | ||
| `) | ||
| WriteJavaScriptToYAML(&b, agenticCommandsScript) |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
@copilot create integration tests that "render" the JavaScript and run it. |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
The inline router approach still introduces a blocking runtime risk: it moves several thousand lines of JavaScript into the actions/github-script with.script input, which is passed through step input/environment plumbing with hard size limits on hosted runners. This has already produced a ~215 KB generated workflow, so the design is still too close to platform limits to be safe.
Blocking theme
- Inlining the full CommonJS closure into
github-scripttrades checkout/setup latency for a much larger workflow payload and step input. - The current tests only assert textual embedding, not whether the generated step remains within GitHub Actions' practical input/env limits.
- A safer design needs either a much smaller bootstrap script or an execution path that does not depend on shoving the whole router through
with.script.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.77 AIC · ⌖ 6.23 AIC · ⊞ 6.9K
Comment /review to run again
…avaScript Documents the architectural decision to embed JS modules via Go's embed directive and inline the router bundle into the generated github-script step, eliminating the checkout + setup-action steps from generated workflows. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (181 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Ponytail pass: one double-pass of comment stripping and a redundant line-trim loop can go.
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #52986 · auto · 42.9 AIC · ⌖ 6 AIC · ⊞ 7.2K
Comment /ponytail to run again
| script.WriteString(` | ||
| await __ghAwMain(); | ||
| `) | ||
| return trimAgenticCommandsScript(removeJavaScriptComments(script.String())), nil |
There was a problem hiding this comment.
L89: delete: bundleAgenticCommandsScript strips comments via removeJavaScriptComments, then WriteJavaScriptToYAML (central_slash_command_workflow.go) strips comments again on the same string. Drop the inner call, comment removal happens once at the YAML-write step.
| return trimAgenticCommandsScript(removeJavaScriptComments(script.String())), nil | ||
| } | ||
|
|
||
| func trimAgenticCommandsScript(script string) string { |
There was a problem hiding this comment.
L92-99: yagni: trimAgenticCommandsScript re-walks every line to trim trailing whitespace, but WriteJavaScriptToYAML already skips/rewrites each line when inlining to YAML. Drop this pass.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness risks and two test gaps.
⚠️ The diff was truncated at 3000 lines (the PR adds 3,506 lines toagentic_commands.ymlalone). This review focuses on the Go source files, which were read directly from the repository. The generated YAML was not reviewed in detail.
📋 Key Themes & Highlights
Issues Found
- Circular dependency → infinite recursion (
agentic_commands_script.go:100) —collectAgenticCommandsModuleshas no cycle guard. Two mutually-requiring modules will overflow the stack. sync.Oncecaches failures (agentic_commands_script.go:24) — a transient error on the first call permanently poisonsgetAgenticCommandsScript()for the process lifetime. Add a comment if this is intentional (i.e., the only failure mode is a build-time missing embed).(go/redacted):embedexplicit list is fragile (agentic_commands_embed.go:8) — new.cjsmodules added to the directory won't be embedded automatically; consider*.cjswildcard.TestGetAgenticCommandsScriptshares package-levelOncestate — callbundleAgenticCommandsScript(setupjs.AgenticCommandsScripts, ...)directly to avoid test-ordering dependencies.
Positive Highlights
- ✅ Deterministic module ordering via
sort.Strings— reproducible generated YAML. - ✅ The
__ghAwRequireshim is clean and well-scoped; module caching via__ghAwModuleCacheavoids redundant execution. - ✅ Path traversal guard in
collectAgenticCommandsModules(..prefix check) is a good defensive touch. - ✅
removeJavaScriptCommentsapplied before output keeps the inlined bundle readable and compact. - ✅ Good test coverage for the happy path and missing-dependency error.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 64.2 AIC · ⌖ 9.54 AIC · ⊞ 7.7K
Comment /matt to run again
| ) | ||
|
|
||
| func getAgenticCommandsScript() (string, error) { | ||
| agenticCommandsScriptOnce.Do(func() { |
There was a problem hiding this comment.
[/codebase-design] sync.Once caches both success and failure permanently — if bundling fails once (e.g., due to a missing embedded file in a test build), all subsequent calls in the same process return the same error, with no recovery path.
💡 Suggested fix
Either use sync.OnceValues (Go 1.21+) which has the same semantics but is cleaner, or — if one-time caching of errors is undesirable — protect with a sync.Mutex and only cache on success:
var (
agenticCommandsScriptOnce sync.Once
agenticCommandsScript string
agenticCommandsScriptErr error
)The current design is fine if the only failure mode is a broken binary (missing (go/redacted):embed file), which would fail at compile time anyway. If that's the guarantee, add a comment explaining that the Once caches errors intentionally because a missing embed is always a fatal build error.
@copilot please address this.
| } | ||
| return trimmed.String() | ||
| } | ||
|
|
There was a problem hiding this comment.
[/codebase-design] collectAgenticCommandsModules has no circular dependency guard. If module A requires module B and module B requires module A, this recurses infinitely and panics with a stack overflow.
💡 Suggested fix
Add an in-progress set alongside the modules map:
func collectAgenticCommandsModules(scripts fs.FS, moduleName string, modules map[string]string) error {
moduleName = path.Clean(strings.TrimPrefix(moduleName, "./"))
// ... path validation ...
if _, exists := modules[moduleName]; exists {
return nil // already collected
}
// Mark as in-progress *before* recursing to break cycles
modules[moduleName] = "" // placeholder
content, err := fs.ReadFile(scripts, moduleName)
if err != nil { ... }
modules[moduleName] = string(content)
// ... recurse into dependencies ...
}The placeholder "" entry in the map will short-circuit any back-edge before the real content is assigned. This is a minimal change that eliminates the cycle risk without additional data structures.
@copilot please address this.
|
|
||
| _, err := bundleAgenticCommandsScript(scripts, "main.cjs") | ||
| require.ErrorContains(t, err, `failed to read agentic commands module "missing.cjs"`) | ||
| } |
There was a problem hiding this comment.
[/tdd] No test covers circular dependency detection. The current implementation will infinitely recurse if two bundled modules require each other — a test should document this expectation before the guard is (or isn't) added.
💡 Suggested test
func TestBundleAgenticCommandsScriptFailsForCircularDependency(t *testing.T) {
scripts := fstest.MapFS{
"a.cjs": {Data: []byte(`require("./b.cjs");`)},
"b.cjs": {Data: []byte(`require("./a.cjs");`)},
}
_, err := bundleAgenticCommandsScript(scripts, "a.cjs")
// Either succeeds (if cycles are harmless by design) or returns a
// descriptive error — either way the behaviour is explicit.
_ = err
}Even if circular deps cannot appear in practice with the current JS modules, the test pins the contract and prevents future regressions.
@copilot please address this.
| // AgenticCommandsScripts contains the JavaScript modules used by the generated | ||
| // agentic commands router. | ||
| // | ||
| //go:embed route_slash_command.cjs add_reaction.cjs add_workflow_run_comment.cjs |
There was a problem hiding this comment.
[/codebase-design] The (go/redacted):embed directives in agentic_commands_embed.go list every .cjs file explicitly. If a new module is added to actions/setup/js/ but forgotten here, the compiler silently produces a broken bundle at runtime (missing module error). An explicit (go/redacted):embed *.cjs wildcard would auto-include all current and future modules.
💡 Suggested fix
(go/redacted):embed *.cjs
var AgenticCommandsScripts embed.FSThis is safe because the bundler's collectAgenticCommandsModules only loads files reachable from the entrypoint — unused .cjs files in the FS are simply ignored. The wildcard makes the embed self-maintaining.
The explicit listing does have one advantage: it documents intent. If the explicit list is intentional (to restrict which files are embedded), add a comment explaining the policy.
@copilot please address this.
|
|
||
| func TestGetAgenticCommandsScript(t *testing.T) { | ||
| script, err := getAgenticCommandsScript() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
[/tdd] TestGetAgenticCommandsScript uses getAgenticCommandsScript() which is memoised with sync.Once at package level. If this test runs after a previous test that caused the Once to fire (e.g., in the same process), the result is already cached and the test isn't exercising a fresh call. Consider either resetting the once between tests or testing bundleAgenticCommandsScript directly (passing setupjs.AgenticCommandsScripts explicitly) to keep the test deterministic and isolated.
@copilot please address this.
There was a problem hiding this comment.
The inlined JS bundle faithfully mirrors the source modules in actions/setup/js/. No blocking issues found:
removeXmlCommentsdepth-tracking is intentional security hardening (prevents bypass via<!-- <!-- --> PAYLOAD -->) and matches the documented behavior in the source file.neutralizeAllMentionsis defined insanitize_content_core.cjsand called internally bysanitizeContentCore, which is invoked fromsanitize_content.cjswhen no allowedAliases are configured — it is not dead code.- The 220 KB workflow file size is fine;
github-scriptreads thescript:block from the checked-out file, not from an environment variable, so there is no size constraint here.
The approach (removing the checkout + setup dependency by inlining the CommonJS bundle) is clean and the bundler output is deterministic.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 124.3 AIC · ⌖ 7.92 AIC · ⊞ 5.6K
The generated agentic commands router previously required checkout and
gh-aw-actions/setupsteps before routing. This change embeds the tested JavaScript modules in the compiler and emits them directly into thegithub-scriptstep for faster startup.Changes
Compiler
Generated workflow
actions/github-scriptstep.