diff --git a/.github/skills/developer-internals/SKILL.md b/.github/skills/developer-internals/SKILL.md index d57c3a7c83e..c3974bc3af1 100644 --- a/.github/skills/developer-internals/SKILL.md +++ b/.github/skills/developer-internals/SKILL.md @@ -27,17 +27,18 @@ graph LR WF[Workflow] --> CV[Centralized Validation] WF --> DV[Domain-Specific Validation] CV --> validation.go - DV --> strict_mode.go + DV --> strict_mode_validation.go + DV --> strict_mode_permissions_validation.go DV --> pip.go DV --> npm.go - DV --> expression_safety.go + DV --> expression_safety_validation.go DV --> engine.go DV --> mcp-config.go ``` ### Centralized Validation -**Location:** `pkg/workflow/validation.go` (782 lines) +**Location:** `pkg/workflow/validation.go` (core compile-time checks) **Purpose:** General-purpose validation that applies across the entire workflow system @@ -61,11 +62,11 @@ graph LR ### Domain-Specific Validation -Domain-specific validation is organized into separate files: +Domain-specific validation is organized into separate files in `pkg/workflow/`: #### Strict Mode Validation -**Files:** `pkg/workflow/strict_mode.go`, `pkg/workflow/validation_strict_mode.go` +**Files:** `pkg/workflow/strict_mode_validation.go` and the `strict_mode_*.go` validators Enforces security and safety constraints in strict mode: - `validateStrictPermissions()` - Refuses write permissions @@ -77,9 +78,7 @@ Enforces security and safety constraints in strict mode: **File:** `pkg/workflow/pip.go` -Validates Python package availability on PyPI: -- `validatePipPackages()` - Validates pip packages -- `validateUvPackages()` - Validates uv packages +Validates Python package availability on PyPI. #### NPM Package Validation @@ -89,16 +88,16 @@ Validates NPX package availability on npm registry. #### Expression Safety -**File:** `pkg/workflow/expression_safety.go` +**File:** `pkg/workflow/expression_safety_validation.go` -Validates GitHub Actions expression security with allowlist-based validation. +Validates GitHub Actions expression security with allowlist-based validation. The matching test coverage lives in `pkg/workflow/expression_safety_test.go`. ### Validation Decision Tree ```mermaid graph TD A[New Validation Requirement] --> B{Security or strict mode?} - B -->|Yes| C[strict_mode.go] + B -->|Yes| C[strict_mode_validation.go] B -->|No| D{Only applies to one domain?} D -->|Yes| E{Domain-specific file exists?} E -->|Yes| F[Add to domain file] diff --git a/.github/skills/error-pattern-safety/SKILL.md b/.github/skills/error-pattern-safety/SKILL.md index 960327ee712..ab85aea757c 100644 --- a/.github/skills/error-pattern-safety/SKILL.md +++ b/.github/skills/error-pattern-safety/SKILL.md @@ -72,23 +72,25 @@ With the JavaScript global flag (`/pattern/g`), zero-width matches can cause inf ## Validation Tests -All error patterns must pass these tests: +All error patterns must pass the same safety checks used by the repo’s unit suite: -### Go Tests (pkg/workflow/engine_error_patterns_infinite_loop_test.go) +### Go tests ```go // Test that pattern doesn't match empty string func TestPatternSafety(t *testing.T) { pattern := "your-pattern" regex := regexp.MustCompile(pattern) - + if regex.MatchString("") { t.Error("Pattern matches empty string!") } } ``` -### JavaScript Tests (pkg/workflow/js/validate_errors.test.cjs) +Run the relevant package tests with `make test-unit`. + +### JavaScript tests ```javascript test("should not match empty string", () => { @@ -97,16 +99,17 @@ test("should not match empty string", () => { }); ``` -## Safety Mechanisms in validate_errors.cjs +Use the relevant `*.test.cjs` suite under `actions/setup/js/` or `pkg/workflow/js/` for the area you changed, or run the repo’s JavaScript checks via `make test-js`. + +## Safety Mechanisms in the validation layer -The `validate_errors.cjs` script has built-in protections: +The repo’s validation helpers include built-in protections for dangerous regex patterns: -1. **Zero-width detection**: Checks if `regex.lastIndex` stops advancing -2. **Iteration warning**: Warns at 1000 iterations -3. **Hard limit**: Stops at 10,000 iterations to prevent hang +1. **Zero-width detection**: Checks whether a regex stops advancing across iterations +2. **Iteration warning**: Warns when repeated runs approach a hang threshold +3. **Hard limit**: Stops execution before runaway loops can lock the process ```javascript -// Safety check in validate_errors.cjs if (regex.lastIndex === lastIndex) { core.error(`Infinite loop detected! Pattern: ${pattern.pattern}`); break; @@ -191,17 +194,13 @@ Pattern: `\berror\b.*` // Requires word "error" Before committing pattern changes: - [ ] Run `make test-unit` -- [ ] Check `TestAllEnginePatternsSafe` passes -- [ ] Check `TestErrorPatternsNoInfiniteLoopPotential` passes -- [ ] Run JavaScript tests: `cd pkg/workflow/js && npm test` -- [ ] Verify pattern matches intended error messages -- [ ] Verify pattern doesn't match informational text +- [ ] Verify the relevant engine error-pattern tests still pass +- [ ] Run the JavaScript checks for the changed area with `make test-js` or the targeted Vitest suite +- [ ] Verify the pattern matches intended error messages +- [ ] Verify the pattern does not match informational text or empty-string edge cases ## References - Go regex syntax: https://pkg.go.dev/regexp/syntax - JavaScript regex: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions -- Test files: - - `pkg/workflow/engine_error_patterns_infinite_loop_test.go` - - `pkg/workflow/js/validate_errors.test.cjs` - - `pkg/workflow/error_pattern_tuning_test.go` +- Current repo validation: `make test-unit` and `make test-js` diff --git a/.github/skills/javascript-refactoring/SKILL.md b/.github/skills/javascript-refactoring/SKILL.md index 6d81f0e9a20..15518d5aa04 100644 --- a/.github/skills/javascript-refactoring/SKILL.md +++ b/.github/skills/javascript-refactoring/SKILL.md @@ -6,14 +6,19 @@ description: Split large JavaScript files into maintainable modules safely. # JavaScript Code Refactoring Guide -Use this guide to refactor JavaScript into separate `.cjs` files in gh-aw. +Use this guide to split JavaScript into maintainable CommonJS modules in gh-aw without drifting into dead embedding patterns. ## Overview -gh-aw uses CommonJS modules (`.cjs`) for JavaScript in GitHub Actions workflows. These files are: -- Embedded in the Go binary using `//go:embed` directives -- Bundled using a custom JavaScript bundler that inlines local `require()` calls -- Executed in GitHub Actions using `actions/github-script@v8` +The current gh-aw architecture is action-centric: + +- Shared JS modules live under `pkg/workflow/js/*.cjs` and `actions/setup/js/*.cjs` +- Action source files live under `actions//src/` +- Generated action bundles are committed under `actions//index.js` +- Shipping is driven by the action build pipeline (`make actions-build`, `gh aw actions-build`) and dependency maps such as `pkg/cli/actions_build_command.go` +- `pkg/workflow/js.go` is a stub; it no longer owns the runtime JavaScript shipping path for the main workflows + +If you are refactoring a workflow utility, prefer the current action/module architecture over any older `//go:embed` pattern. ### Top-Level Script Pattern @@ -42,27 +47,22 @@ module.exports = { main }; ``` **Why this pattern?** -- The bundler automatically injects `await main()` during inline execution in GitHub Actions -- This allows the script to be both imported (for testing) and executed (in workflows) -- It provides a clean separation between module definition and execution -- It enables better testing by allowing tests to import and call `main()` with mocks - -**Examples of top-level scripts:** -- `create_issue.cjs` - Creates GitHub issues -- `add_comment.cjs` - Adds comments to issues/PRs -- `add_labels.cjs` - Adds labels to issues/PRs -- `update_project.cjs` - Updates GitHub Projects +- The workflow bundler or action build step can wrap the script with `await main()` at execution time +- The module stays importable for tests while still being executable in GitHub Actions +- It makes unit testing easier and preserves a clean module boundary -All of these files export `main` but do not call it directly. +## Step 1: Put the code in the right source tree -## Step 1: Create the New .cjs File +Choose the correct location for the module before writing code: -Create your new file in `/home/runner/work/gh-aw/gh-aw/pkg/workflow/js/` with a descriptive name: +- Shared workflow utilities: `pkg/workflow/js/` +- Action-specific JavaScript: `actions//src/` or `actions/setup/js/` +- Generated bundle output: `actions//index.js` **File naming convention:** -- Use snake_case for filenames (e.g., `sanitize_content.cjs`, `load_agent_output.cjs`) -- Use `.cjs` extension (CommonJS module) -- Choose names that clearly describe the module's purpose +- Use snake_case for filenames (for example `sanitize_content.cjs`, `load_agent_output.cjs`) +- Use `.cjs` for CommonJS modules +- Keep the name aligned with the responsibility of the module **Example file structure:** ```javascript @@ -79,11 +79,9 @@ Create your new file in `/home/runner/work/gh-aw/gh-aw/pkg/workflow/js/` with a * @returns {string} Description of return value */ function myFunction(input) { - // Implementation return input; } -// Export the function(s) module.exports = { myFunction, }; @@ -91,20 +89,19 @@ module.exports = { **Key points:** - Include `// @ts-check` for TypeScript checking -- Include `/// ` for GitHub Actions types +- Include `/// ` when the module is used with GitHub Actions scripts - Use JSDoc comments for documentation -- Export functions using `module.exports = { ... }` -- Do NOT import `@actions/core` or `@actions/github` - these are available globally in GitHub Actions +- Export functions via `module.exports = { ... }` +- Do not import `@actions/core` or `@actions/github` directly unless the module is running in an action context that explicitly expects it -## Step 2: Add Tests +## Step 2: Add tests next to the module -Create a test file with the same base name plus `.test.cjs`: +Create a matching test beside the module using the same base name plus `.test.cjs`: -**Example: `pkg/workflow/js/my_module.test.cjs`** +**Example:** `pkg/workflow/js/my_module.test.cjs` ```javascript import { describe, it, expect, beforeEach, vi } from "vitest"; -// Mock the global objects that GitHub Actions provides const mockCore = { debug: vi.fn(), info: vi.fn(), @@ -112,145 +109,52 @@ const mockCore = { error: vi.fn(), setFailed: vi.fn(), setOutput: vi.fn(), - summary: { - addRaw: vi.fn().mockReturnThis(), - write: vi.fn().mockResolvedValue(), - }, }; -// Set up global mocks before importing the module global.core = mockCore; describe("myFunction", () => { beforeEach(() => { - // Reset mocks before each test vi.clearAllMocks(); }); - it("should handle basic input", async () => { - // Import the module to test + it("handles a normal input", async () => { const { myFunction } = await import("./my_module.cjs"); - - const result = myFunction("test input"); - - expect(result).toBe("expected output"); + expect(myFunction("test input")).toBe("expected output"); }); - it("should handle edge cases", async () => { + it("handles empty input", async () => { const { myFunction } = await import("./my_module.cjs"); - - const result = myFunction(""); - - expect(result).toBe(""); + expect(myFunction("")).toBe(""); }); }); ``` **Testing guidelines:** -- Use vitest for testing framework +- Use Vitest for test execution - Mock `core` and `github` globals as needed -- Use dynamic imports (`await import()`) to allow mocking before module load -- Clear mocks in `beforeEach` to ensure test isolation -- Test both success cases and error handling -- Follow existing test patterns in `pkg/workflow/js/*.test.cjs` files +- Use dynamic imports (`await import()`) to allow module setup at test time +- Clear mocks in `beforeEach` +- Cover success, failure, and edge cases **Run tests:** ```bash make test-js ``` -## Step 3: Add Embedded Variable in Go - -Add an `//go:embed` directive and variable in the appropriate Go file: - -### For shared utility functions (used by multiple scripts): - -Add to **`pkg/workflow/js.go`**: - -```go -//go:embed js/my_module.cjs -var myModuleScript string -``` - -Then add to the `GetJavaScriptSources()` function: - -```go -func GetJavaScriptSources() map[string]string { - return map[string]string{ - "sanitize_content.cjs": sanitizeContentScript, - "sanitize_label_content.cjs": sanitizeLabelContentScript, - "sanitize_workflow_name.cjs": sanitizeWorkflowNameScript, - "load_agent_output.cjs": loadAgentOutputScript, - "staged_preview.cjs": stagedPreviewScript, - "is_truthy.cjs": isTruthyScript, - "my_module.cjs": myModuleScript, // Add this line - } -} -``` - -### For main scripts (top-level scripts that use bundling): - -Add to **`pkg/workflow/scripts.go`**: - -```go -//go:embed js/my_script.cjs -var myScriptSource string -``` - -Then create a getter function with bundling: - -```go -var ( - myScript string - myScriptOnce sync.Once -) - -// getMyScript returns the bundled my_script script -// Bundling is performed on first access and cached for subsequent calls -func getMyScript() string { - myScriptOnce.Do(func() { - sources := GetJavaScriptSources() - bundled, err := BundleJavaScriptFromSources(myScriptSource, sources, "") - if err != nil { - scriptsLog.Printf("Bundling failed for my_script, using source as-is: %v", err) - // If bundling fails, use the source as-is - myScript = myScriptSource - } else { - myScript = bundled - } - }) - return myScript -} -``` - -**Important:** -- Variables in `js.go` are for **shared utilities** that get bundled into other scripts -- Variables in `scripts.go` are for **main scripts** that use the bundler to inline dependencies -- Use `sync.Once` pattern for lazy bundling in `scripts.go` -- The bundler will inline all local `require()` calls at runtime - -## Step 4: Register in the Bundler (if creating a shared utility) - -If you're creating a shared utility that will be used by other scripts via `require()`, it's automatically available through the `GetJavaScriptSources()` map (Step 3). - -**The bundler will:** -1. Detect `require('./my_module.cjs')` in any script -2. Look up the file in the `GetJavaScriptSources()` map -3. Inline the required module's content -4. Remove the `require()` statement -5. Deduplicate if the same module is required multiple times +## Step 3: Wire the module into the actual build path -**No additional bundler registration needed** - just ensure the file is in the `GetJavaScriptSources()` map. +Do not add a new `//go:embed` mapping just to ship a new runtime script. The current repo ships JavaScript through the action-generation/build pipeline. -## Step 5: Use Local Require in Other JavaScript Files +Use this checklist: -To use your new module in other JavaScript files, use CommonJS `require()`: +- Shared utility used by generated actions: update the relevant dependency mapping in `pkg/cli/actions_build_command.go` +- Action-specific source file: add the module under `actions//src/` +- Generated action bundle: rebuild with `make actions-build` +- Shared workflow source for runtime modules: keep it under `pkg/workflow/js/` and update the action or workflow definition that consumes it -**Example usage in another `.cjs` file:** +**Example design:** ```javascript -// @ts-check -/// - const { myFunction } = require("./my_module.cjs"); async function main() { @@ -261,225 +165,74 @@ async function main() { module.exports = { main }; ``` -**Important:** Top-level scripts should export `main` but **NOT** call it directly. The bundler injects `await main()` during inline execution in GitHub Actions. - -**Require guidelines:** -- Use relative paths starting with `./` -- Include the `.cjs` extension -- Use destructuring to import specific functions -- The bundler will inline the required module at compile time - -**Multiple requires example:** -```javascript -const { sanitizeContent } = require("./sanitize_content.cjs"); -const { loadAgentOutput } = require("./load_agent_output.cjs"); -const { generateStagedPreview } = require("./staged_preview.cjs"); -``` - -## Complete Example: Creating a New Utility Module - -Let's walk through creating a new `format_timestamp.cjs` utility: - -### 1. Create the file: `pkg/workflow/js/format_timestamp.cjs` - -```javascript -// @ts-check -/// - -/** - * Formats a timestamp to ISO 8601 format - * @param {Date|string|number} timestamp - Timestamp to format - * @returns {string} ISO 8601 formatted timestamp - */ -function formatTimestamp(timestamp) { - const date = timestamp instanceof Date ? timestamp : new Date(timestamp); - return date.toISOString(); -} - -/** - * Formats a timestamp to a human-readable string - * @param {Date|string|number} timestamp - Timestamp to format - * @returns {string} Human-readable timestamp - */ -function formatTimestampHuman(timestamp) { - const date = timestamp instanceof Date ? timestamp : new Date(timestamp); - return date.toLocaleString('en-US', { - dateStyle: 'medium', - timeStyle: 'short' - }); -} - -module.exports = { - formatTimestamp, - formatTimestampHuman, -}; -``` - -### 2. Create tests: `pkg/workflow/js/format_timestamp.test.cjs` - -```javascript -import { describe, it, expect } from "vitest"; - -describe("formatTimestamp", () => { - it("should format Date object to ISO 8601", async () => { - const { formatTimestamp } = await import("./format_timestamp.cjs"); - const date = new Date('2024-01-15T12:30:00Z'); - - const result = formatTimestamp(date); - - expect(result).toBe('2024-01-15T12:30:00.000Z'); - }); - - it("should format timestamp number to ISO 8601", async () => { - const { formatTimestamp } = await import("./format_timestamp.cjs"); - const timestamp = 1705323000000; // Jan 15, 2024 12:30:00 UTC - - const result = formatTimestamp(timestamp); - - expect(result).toBe('2024-01-15T12:30:00.000Z'); - }); -}); - -describe("formatTimestampHuman", () => { - it("should format Date object to human-readable string", async () => { - const { formatTimestampHuman } = await import("./format_timestamp.cjs"); - const date = new Date('2024-01-15T12:30:00Z'); - - const result = formatTimestampHuman(date); - - expect(result).toContain('Jan'); - expect(result).toContain('15'); - expect(result).toContain('2024'); - }); -}); -``` - -### 3. Add to `pkg/workflow/js.go`: - -```go -//go:embed js/format_timestamp.cjs -var formatTimestampScript string - -func GetJavaScriptSources() map[string]string { - return map[string]string{ - // ... existing entries ... - "format_timestamp.cjs": formatTimestampScript, - } -} -``` - -### 4. Use in another script: - -```javascript -// @ts-check -/// - -const { formatTimestamp } = require("./format_timestamp.cjs"); - -async function main() { - const now = new Date(); - core.info(`Current time: ${formatTimestamp(now)}`); -} - -module.exports = { main }; -``` - -**Note:** The script exports `main` but does not call it. The bundler will inject `await main()` when the script is executed inline in GitHub Actions. +## Step 4: Validate the refactor -### 5. Build and test: +Run the relevant checks for the area you changed: ```bash -# Format the code make fmt-cjs - -# Run JavaScript tests +make lint-cjs make test-js - -# Run Go tests (includes bundler tests) make test-unit - -# Build the binary (embeds JavaScript files) -make build +make actions-build ``` ## Verification Checklist -Before committing your refactored code: +Before committing your refactor: -- [ ] New `.cjs` file created in `pkg/workflow/js/` -- [ ] Tests created in corresponding `.test.cjs` file -- [ ] Tests pass with `make test-js` -- [ ] Embedded variable added in `pkg/workflow/js.go` or `pkg/workflow/scripts.go` -- [ ] If utility: Added to `GetJavaScriptSources()` map -- [ ] If main script: Created bundling getter function with `sync.Once` -- [ ] Local `require()` statements work correctly in other files +- [ ] New `.cjs` file created in the correct source directory +- [ ] Matching `.test.cjs` file created +- [ ] Tests pass with `make test-js` or the targeted Vitest suite +- [ ] The module is wired through the real action/workflow build path +- [ ] No stale embedding instructions were added for the current action-based JS build flow +- [ ] Local `require()` statements work correctly in other JS files - [ ] Code formatted with `make fmt-cjs` -- [ ] Code linted with `make lint-cjs` -- [ ] All Go tests pass with `make test-unit` -- [ ] Build succeeds with `make build` +- [ ] Relevant validation passes with `make lint-cjs` or `make test-unit` ## Common Patterns -### Pattern 1: Shared Utility Function +### Pattern 1: Shared Utility Module -Files like `sanitize_content.cjs`, `load_agent_output.cjs` that provide reusable functions: -- Add to `js.go` with `//go:embed` -- Add to `GetJavaScriptSources()` map -- Use via `require()` in other scripts +Files like `sanitize_content.cjs` or `load_agent_output.cjs` are best kept under `pkg/workflow/js/` or `actions/setup/js/` and consumed by other JS modules via `require()`. -### Pattern 2: Main Workflow Script +### Pattern 2: Action-specific file -Files like `create_issue.cjs`, `add_labels.cjs` that are top-level scripts: -- Add to `scripts.go` with `//go:embed` as `xxxSource` variable -- Create bundling getter function with `sync.Once` pattern -- These scripts can `require()` utilities from `GetJavaScriptSources()` -- **Must export `main` function but NOT call it** - the bundler injects `await main()` during execution +When the JavaScript belongs to a single action, keep it under `actions//src/` and regenerate the output bundle with `make actions-build`. -### Pattern 3: Log Parser +### Pattern 3: Top-level workflow script -Files like `parse_claude_log.cjs` that parse AI engine logs: -- Add to `js.go` with `//go:embed` -- Add case in `GetLogParserScript()` function -- Used by workflow compilation system +If the script is executed directly in a workflow, export `main` and omit the direct `await main()` call. The host build/runtime step handles execution. ## Troubleshooting -### Issue: "required file not found in sources" +### Issue: changes are not showing up in generated actions -**Cause:** File not added to `GetJavaScriptSources()` map +**Cause:** Action bundle was not rebuilt after editing the source file -**Solution:** Add the file to the map in `pkg/workflow/js.go` +**Solution:** +```bash +make actions-build +``` -### Issue: Tests fail with "core is not defined" +### Issue: tests fail with `core is not defined` **Cause:** Missing global mocks -**Solution:** Add proper mocks before importing the module: +**Solution:** ```javascript global.core = mockCore; -global.github = mockGithub; ``` -### Issue: Bundler fails with circular dependency +### Issue: the module is only used in one place -**Cause:** File A requires File B which requires File A +**Cause:** It was added to the wrong layer -**Solution:** Restructure to break the circular dependency, or combine the modules - -### Issue: Changes not reflected after rebuild - -**Cause:** Go build cache not recognizing embedded file changes - -**Solution:** -```bash -make clean -make build -``` +**Solution:** Move it to the action-specific source tree instead of creating a broad workflow-level registry entry. ## References -- Bundler implementation: `pkg/workflow/bundler.go` -- JavaScript sources registry: `pkg/workflow/js.go` -- Script bundling: `pkg/workflow/scripts.go` -- Existing test examples: `pkg/workflow/js/*.test.cjs` -- GitHub Actions script documentation: [actions/toolkit](https://github.com/actions/toolkit) +- `actions/README.md` - current action-generation/build workflow +- `pkg/cli/actions_build_command.go` - action dependency mapping +- `pkg/workflow/js/*.cjs` - existing shared module patterns +- `actions/setup/js/*.cjs` - action runtime/source examples diff --git a/.github/skills/messages/SKILL.md b/.github/skills/messages/SKILL.md index 8a7bf70c593..423a8d6b071 100644 --- a/.github/skills/messages/SKILL.md +++ b/.github/skills/messages/SKILL.md @@ -6,13 +6,19 @@ description: Add new safe-output message types and wire validation/rendering. # Adding New Message Types Guide -Use this guide to add a new message type to the safe-output messages system so it works in frontmatter, compiler parsing, JavaScript, and bundling. +Use this guide to add a new safe-output message type so it works in the current gh-aw pipeline: frontmatter → schema → Go compiler → JavaScript modules → action/workflow build output. ## Overview -The messages system lets workflow authors customize safe-output messages. Message flow: +The messages system lets workflow authors customize safe-output messages. The current architecture does not rely on the old `pkg/workflow/js.go` embedding registry for runtime shipping. -1. **Frontmatter** (YAML) → 2. **JSON Schema** → 3. **Go Compiler** → 4. **JavaScript Modules** → 5. **Bundler** +Current flow: + +1. **Frontmatter** (YAML) +2. **JSON Schema** +3. **Go Compiler** +4. **JavaScript module** under `pkg/workflow/js/` or `actions/setup/js/` +5. **Action/workflow bundle generation** via `make actions-build` or the relevant workflow build path ## Step 1: Update JSON Schema @@ -20,77 +26,55 @@ Add the new message field to `pkg/parser/schemas/main_workflow_schema.json` in t ```json { - "messages": { - "properties": { - "my-new-message": { - "type": "string", - "description": "Description of when this message is used. Available placeholders: {placeholder1}, {placeholder2}.", - "examples": [ - "Example message with {placeholder1}" - ] - } - } - } + "messages": { + "properties": { + "my-new-message": { + "type": "string", + "description": "Description of when this message is used. Available placeholders: {placeholder1}, {placeholder2}.", + "examples": [ + "Example message with {placeholder1}" + ] + } + } + } } ``` **Key points:** -- Use `kebab-case` for the YAML field name (e.g., `my-new-message`) -- Document all available placeholders in the description +- Use `kebab-case` for the YAML field name (for example `my-new-message`) +- Document placeholders in the description - Provide helpful examples -- Run `make build` after changes (schema is embedded in binary) +- Rebuild the schema-backed binary or run the relevant compile checks after changes ## Step 2: Update Go Struct -Add the new field to `SafeOutputMessagesConfig` in `pkg/workflow/compiler.go`: +Add the field to `SafeOutputMessagesConfig` in `pkg/workflow/compiler.go`: ```go type SafeOutputMessagesConfig struct { // ... existing fields ... - MyNewMessage string `yaml:"my-new-message,omitempty" json:"myNewMessage,omitempty"` // Description of the message + MyNewMessage string `yaml:"my-new-message,omitempty" json:"myNewMessage,omitempty"` } ``` **Key points:** -- Use `CamelCase` for Go field name -- Use `kebab-case` for YAML tag (matches frontmatter) -- Use `camelCase` for JSON tag (used in JavaScript) +- Use `CamelCase` for Go field names +- Use `kebab-case` for YAML tags +- Use `camelCase` for JSON tags - Add `omitempty` to both tags -## Step 3: Update Go Parser +## Step 3: Update the parser if needed -If needed, update the parser in `pkg/workflow/safe_outputs.go`: - -```go -func parseMessagesConfig(messagesMap map[string]any) *SafeOutputMessagesConfig { - config := &SafeOutputMessagesConfig{} - // ... existing parsing ... - - if myNewMessage, ok := messagesMap["my-new-message"].(string); ok { - config.MyNewMessage = myNewMessage - } - - return config -} -``` +If the message needs custom parsing logic, update the workflow parser in `pkg/workflow/safe_outputs.go` or the relevant config block. Most simple string fields will be wired automatically by the existing reflection-based parser. -**Note:** The parser uses reflection for most fields, so this step may not be needed for simple string fields. +## Step 4: Create the JavaScript message module -## Step 4: Create JavaScript Message Module - -Create a new file `pkg/workflow/js/messages_my_new.cjs`: +Create the new module in the current shared JS location, typically `pkg/workflow/js/`: ```javascript // @ts-check /// -/** - * My New Message Module - * - * This module provides the my-new-message generation - * for [describe when it's used]. - */ - const { getMessages, renderTemplate, toSnakeCase } = require("./messages_core.cjs"); /** @@ -99,164 +83,107 @@ const { getMessages, renderTemplate, toSnakeCase } = require("./messages_core.cj * @property {string} placeholder2 - Description of placeholder2 */ -/** - * Get the my-new-message, using custom template if configured. - * @param {MyNewMessageContext} ctx - Context for message generation - * @returns {string} The generated message - */ function getMyNewMessage(ctx) { - const messages = getMessages(); + const messages = getMessages(); + const templateContext = toSnakeCase(ctx); + const defaultMessage = "Default message with {placeholder1} and {placeholder2}"; - // Create context with both camelCase and snake_case keys - const templateContext = toSnakeCase(ctx); - - // Default message template - const defaultMessage = "Default message with {placeholder1} and {placeholder2}"; - - // Use custom message if configured - return messages?.myNewMessage - ? renderTemplate(messages.myNewMessage, templateContext) - : renderTemplate(defaultMessage, templateContext); + return messages?.myNewMessage + ? renderTemplate(messages.myNewMessage, templateContext) + : renderTemplate(defaultMessage, templateContext); } module.exports = { - getMyNewMessage, + getMyNewMessage, }; ``` **Key points:** -- File naming: `messages_.cjs` (flat structure, not subfolder) -- Import from `./messages_core.cjs` for shared utilities -- Use JSDoc for type definitions -- Provide sensible default message -- Support both custom and default templates +- File naming: `messages_.cjs` +- Reuse `./messages_core.cjs` for shared helpers +- Use JSDoc for types and default behavior +- Keep the default message sensible and deterministic -## Step 5: Add Tests +## Step 5: Add tests -Create `pkg/workflow/js/messages_my_new.test.cjs`: +Create a matching test file, for example `pkg/workflow/js/messages_my_new.test.cjs`: ```javascript import { describe, it, expect, beforeEach, vi } from "vitest"; -// Mock core global -const mockCore = { - warning: vi.fn(), -}; +const mockCore = { warning: vi.fn() }; global.core = mockCore; describe("getMyNewMessage", () => { - beforeEach(() => { - vi.clearAllMocks(); - delete process.env.GH_AW_SAFE_OUTPUT_MESSAGES; - }); - - it("should return default message when no custom message configured", async () => { - const { getMyNewMessage } = await import("./messages_my_new.cjs"); - - const result = getMyNewMessage({ - placeholder1: "value1", - placeholder2: "value2", - }); - - expect(result).toBe("Default message with value1 and value2"); - }); - - it("should use custom message when configured", async () => { - process.env.GH_AW_SAFE_OUTPUT_MESSAGES = JSON.stringify({ - myNewMessage: "Custom: {placeholder1}", - }); - - const { getMyNewMessage } = await import("./messages_my_new.cjs"); - - const result = getMyNewMessage({ - placeholder1: "test", - placeholder2: "ignored", - }); - - expect(result).toContain("Custom: test"); - }); + beforeEach(() => { + vi.clearAllMocks(); + delete process.env.GH_AW_SAFE_OUTPUT_MESSAGES; + }); + + it("returns the default message when no custom template is configured", async () => { + const { getMyNewMessage } = await import("./messages_my_new.cjs"); + const result = getMyNewMessage({ placeholder1: "value1", placeholder2: "value2" }); + expect(result).toBe("Default message with value1 and value2"); + }); + + it("uses the custom template when configured", async () => { + process.env.GH_AW_SAFE_OUTPUT_MESSAGES = JSON.stringify({ myNewMessage: "Custom: {placeholder1}" }); + const { getMyNewMessage } = await import("./messages_my_new.cjs"); + const result = getMyNewMessage({ placeholder1: "test", placeholder2: "ignored" }); + expect(result).toContain("Custom: test"); + }); }); ``` -Run tests with `make test-js`. +Run the relevant tests with `make test-js` or the targeted Vitest file. -## Step 6: Update Core Module TypeDef +## Step 6: Update the core JS type metadata and exports -Add the new property to the `SafeOutputMessages` typedef in `pkg/workflow/js/messages_core.cjs`: +Update the `SafeOutputMessages` typedef and the return object in `pkg/workflow/js/messages_core.cjs`, and re-export the message helper from `pkg/workflow/js/messages.cjs`. -```javascript -/** - * @typedef {Object} SafeOutputMessages - * @property {string} [footer] - Custom footer message template - * // ... existing properties ... - * @property {string} [myNewMessage] - Custom my-new-message template - */ -``` +## Step 7: Wire it into the real build path -Also update the `getMessages()` function return object: +Do not add any new `//go:embed` entries to `pkg/workflow/js.go` for a normal message module. The current system packages JavaScript through the action-generation/build path. -```javascript -return { - footer: rawMessages.footer, - // ... existing fields ... - myNewMessage: rawMessages.myNewMessage, -}; -``` +Instead: -## Step 7: Update Barrel File +- keep the JS module in `pkg/workflow/js/` or the relevant action folder, +- update the action dependency map or action source if needed, +- rebuild the action bundle with `make actions-build`. -Add the re-export to `pkg/workflow/js/messages.cjs`: +## Step 8: Use the message in consumer scripts ```javascript -// Re-export my new messages const { getMyNewMessage } = require("./messages_my_new.cjs"); -module.exports = { - // ... existing exports ... - getMyNewMessage, -}; +const message = getMyNewMessage({ + placeholder1: actualValue1, + placeholder2: actualValue2, +}); ``` -## Step 8: Register in Go Embeddings - -Add to `pkg/workflow/js.go`: - -```go -//go:embed js/messages_my_new.cjs -var messagesMyNewScript string -``` +## Step 9: Update documentation -Add to `GetJavaScriptSources()`: +Document the new message in the repo’s relevant safe-output docs, and keep the examples aligned with the current action-based JavaScript build flow. -```go -func GetJavaScriptSources() map[string]string { - return map[string]string{ - // ... existing entries ... - "messages_my_new.cjs": messagesMyNewScript, - } -} -``` - -## Step 9: Use in Consumer Scripts - -Import directly from the specific module in scripts that need it: +## Verification Checklist -```javascript -const { getMyNewMessage } = require("./messages_my_new.cjs"); +Before committing a message change: -// Use the message -const message = getMyNewMessage({ - placeholder1: actualValue1, - placeholder2: actualValue2, -}); -``` +- [ ] Frontmatter and schema updated +- [ ] Go config/struct updated if needed +- [ ] JS module created under the correct source tree +- [ ] Tests added and passing +- [ ] `messages_core.cjs` and `messages.cjs` updated if relevant +- [ ] Generated action/build output refreshed when required +- [ ] No stale embedding instructions are introduced for the current action-based JS build flow -## Step 10: Update Documentation +## References -Update `scratchpad/safe-output-messages.md`: -1. Add the new message to the "Message Categories" section -2. Document placeholders and usage -3. Add examples +- `actions/README.md` - current action-generation/build workflow +- `pkg/workflow/js/messages_core.cjs` - shared safe-output message helpers +- `pkg/workflow/js/messages.cjs` - message exports +- `pkg/parser/schemas/main_workflow_schema.json` - schema source of truth Update the Message Module Architecture table: ```markdown @@ -265,48 +192,6 @@ Update the Message Module Architecture table: | `messages_my_new.cjs` | My new message description | `getMyNewMessage` | ``` -## Verification Checklist +## Notes -Before committing: - -- [ ] JSON Schema updated in `pkg/parser/schemas/main_workflow_schema.json` -- [ ] Go struct updated in `pkg/workflow/compiler.go` -- [ ] Go parser handles new field (if needed) in `pkg/workflow/safe_outputs.go` -- [ ] JavaScript module created: `pkg/workflow/js/messages_my_new.cjs` -- [ ] Tests created: `pkg/workflow/js/messages_my_new.test.cjs` -- [ ] TypeDef updated in `messages_core.cjs` -- [ ] Barrel file updated: `messages.cjs` -- [ ] Go embed directive added in `js.go` -- [ ] Added to `GetJavaScriptSources()` map -- [ ] Consumer scripts updated to use minimal imports -- [ ] Documentation updated in `scratchpad/safe-output-messages.md` -- [ ] Tests pass: `make test-js` -- [ ] Build succeeds: `make build` -- [ ] Linting passes: `make lint` - -## File Summary - -| File | Purpose | Changes Needed | -|------|---------|----------------| -| `pkg/parser/schemas/main_workflow_schema.json` | JSON Schema | Add field definition | -| `pkg/workflow/compiler.go` | Go struct | Add struct field | -| `pkg/workflow/safe_outputs.go` | Parser | Add parsing logic (if needed) | -| `pkg/workflow/js/messages_my_new.cjs` | JavaScript module | Create new file | -| `pkg/workflow/js/messages_my_new.test.cjs` | Tests | Create new file | -| `pkg/workflow/js/messages_core.cjs` | Core utilities | Update typedef | -| `pkg/workflow/js/messages.cjs` | Barrel file | Add re-export | -| `pkg/workflow/js.go` | Go embeddings | Add embed directive | -| `scratchpad/safe-output-messages.md` | Documentation | Document new message | - -## Example: Adding `close-older-discussion` Message - -This message type was added following this process: - -1. **Schema**: Added `close-older-discussion` field with placeholders `{new_discussion_number}`, `{new_discussion_url}`, `{workflow_name}`, `{run_url}` -2. **Go struct**: Added `CloseOlderDiscussion string` field -3. **JavaScript**: Created `messages_close_discussion.cjs` with `getCloseOlderDiscussionMessage()` -4. **Tests**: Added corresponding test file -5. **Bundler**: Registered in `GetJavaScriptSources()` -6. **Consumer**: Used in `close_older_discussions.cjs` via direct import - -See these files for a working implementation example. +For current gh-aw work, keep message modules aligned with the action-generation flow instead of the historical Go-embed pattern. If you need an example, review the existing safe-output modules under `pkg/workflow/js/` and the generated action files under `actions/`. diff --git a/.github/skills/temporary-id-safe-output/SKILL.md b/.github/skills/temporary-id-safe-output/SKILL.md index 4fbc6b011a4..5cdc323637e 100644 --- a/.github/skills/temporary-id-safe-output/SKILL.md +++ b/.github/skills/temporary-id-safe-output/SKILL.md @@ -24,7 +24,7 @@ Example: `aw_abc`, `aw_abc123`, `aw_Test123` ### 1. Shared Module: `temporary_id.cjs` -Location: `pkg/workflow/js/temporary_id.cjs` +Location: `actions/setup/js/temporary_id.cjs` This module provides shared utilities for temporary ID handling: @@ -45,7 +45,7 @@ The `create_issue` job outputs a temporary ID map that other jobs can consume: **Go changes** (`pkg/workflow/create_issue.go`): - No changes needed - already outputs `temporary_id_map` -**JavaScript changes** (`pkg/workflow/js/create_issue.cjs`): +**JavaScript changes** (`actions/setup/js/create_issue.cjs`): - Generate temporary ID for each created issue - Build map of `temporary_id -> issue_number` - Output map via `core.setOutput("temporary_id_map", JSON.stringify(map))` @@ -128,7 +128,7 @@ if (resolved.wasTemporaryId) { #### Step 4: Update Agent Ingestion Validation -In `pkg/workflow/js/collect_ndjson_output.cjs`: +In `actions/setup/js/collect_ndjson_output.cjs`: Add validation for fields that accept temporary IDs: ```javascript @@ -202,7 +202,7 @@ safe-outputs: ### Unit Tests -Add tests in `pkg/workflow/js/temporary_id.test.cjs` for: +Add tests in `actions/setup/js/temporary_id.test.cjs` for: - `isTemporaryId()` with valid and invalid inputs - `resolveIssueNumber()` with temporary IDs and regular numbers - `loadTemporaryIdMap()` with various JSON inputs diff --git a/Makefile b/Makefile index 6002ffdd641..a2baf004ab3 100644 --- a/Makefile +++ b/Makefile @@ -489,6 +489,7 @@ test-scripts: build @echo "Running Bash script tests..." bash scripts/extract-workflow-frontmatter-keys_test.sh bash scripts/check-stale-lock-files_test.sh + bash scripts/check-skill-file-paths_test.sh bash scripts/resolve-base-commit_test.sh bash scripts/check-workflow-drift_test.sh ./$(BINARY_NAME) bash scripts/check-cgo-cjs-workflow-purity_test.sh @@ -925,6 +926,11 @@ check-stale-lock-files: bash scripts/check-stale-lock-files.sh; \ fi +# Fast guard: fails when a skill references a backticked repo path that no longer exists. +.PHONY: check-skill-file-paths +check-skill-file-paths: + @bash scripts/check-skill-file-paths.sh + # Check for drift between workflow markdown sources and generated lock files. # Compiles all .github/workflows/*.md files and fails if any .lock.yml would # change, reminding contributors to run 'make recompile' before committing. @@ -1137,7 +1143,7 @@ shellcheck-setup-sh: # Validate all project files .PHONY: lint -lint: check-stale-lock-files fmt-check fmt-check-json lint-cjs golint validate-model-alias-chains lint-action-sh shellcheck-setup-sh check-stale-schema-binary +lint: check-stale-lock-files check-skill-file-paths fmt-check fmt-check-json lint-cjs golint validate-model-alias-chains lint-action-sh shellcheck-setup-sh check-stale-schema-binary @echo "✓ All validations passed" # Install the binary locally @@ -1436,6 +1442,7 @@ help: @echo " validate-workflows - Validate compiled workflow lock files (depends on build)" @echo " check-workflow-drift - Check for drift between .md sources and .lock.yml files (builds binary if missing)" @echo " check-stale-lock-files - Fast guard: detect modified .md files without regenerated .lock.yml (no binary needed)" + @echo " check-skill-file-paths - Guard: reject invalid backticked repo paths in .github/skills/**/SKILL.md" @echo " check-stale-schema-binary - Guard: detect modified schema files under pkg/parser/schemas/ without a binary rebuild" @echo " install - Install binary locally" @echo " sync-action-pins - Sync actions-lock.json from .github/aw to pkg/actionpins/data and pkg/workflow/data (runs automatically during build)" diff --git a/scripts/check-skill-file-paths.sh b/scripts/check-skill-file-paths.sh new file mode 100755 index 00000000000..5b6426a33cc --- /dev/null +++ b/scripts/check-skill-file-paths.sh @@ -0,0 +1,95 @@ +#!/bin/bash +set -euo pipefail + +REPO_ROOT="$(pwd)" + +while [[ $# -gt 0 ]]; do + case "$1" in + --repo-root) + REPO_ROOT="${2:?--repo-root requires an argument}" + shift 2 + ;; + *) + echo "ERROR: unknown argument: $1" >&2 + echo "Usage: check-skill-file-paths.sh [--repo-root ]" >&2 + exit 1 + ;; + esac +done + +SKILL_ROOT="$REPO_ROOT/.github/skills" +if [[ ! -d "$SKILL_ROOT" ]]; then + echo "ERROR: skill directory not found: $SKILL_ROOT" >&2 + exit 1 +fi + +# Extract backtick-delimited strings from SKILL.md files and validate the ones that +# look like code-path references in the repo. This guard is intentionally narrow: +# it catches stale repo-relative paths in the code and action source tree without +# flagging documentation examples, package names, or wildcard/glob examples. +invalid=() +while IFS= read -r -d '' file; do + while IFS= read -r candidate; do + [[ -z "$candidate" ]] && continue + candidate="${candidate#@}" + [[ "$candidate" == *"://"* ]] && continue + [[ "$candidate" == *" "* ]] && continue + [[ "$candidate" == *'('* || "$candidate" == *')'* ]] && continue + [[ "$candidate" == *'{'* || "$candidate" == *'}'* ]] && continue + [[ "$candidate" == *'['* || "$candidate" == *']'* ]] && continue + [[ "$candidate" == *'"'* || "$candidate" == *"'"* ]] && continue + [[ "$candidate" == *"<"* || "$candidate" == *">"* || "$candidate" == *"*"* || "$candidate" == *"?"* ]] && continue + [[ "$candidate" == *"..."* ]] && continue + + if [[ "$candidate" == pkg/* ]]; then + : + elif [[ "$candidate" == scripts/* ]]; then + : + elif [[ "$candidate" == internal/* || "$candidate" == cmd/* || "$candidate" == eslint-factory/* ]]; then + : + elif [[ "$candidate" == .github/skills/* ]]; then + : + elif [[ "$candidate" =~ ^actions/.+\.(cjs|js|mjs|ts|md|yaml|yml)$ || "$candidate" =~ ^actions/.+/src/.+ || "$candidate" =~ ^actions/.+/index\.(cjs|js|mjs)$ ]]; then + : + else + continue + fi + + repo_path="$candidate" + if [[ "$repo_path" == /* ]]; then + repo_path="${repo_path#/}" + fi + + if [[ ! -e "$REPO_ROOT/$repo_path" ]]; then + invalid+=("$file: $candidate") + fi + done < <(python3 - "$file" <<'PY' +import pathlib, re, sys +path = pathlib.Path(sys.argv[1]) +text = path.read_text(encoding='utf-8', errors='ignore') +for match in re.findall(r'`([^`]+)`', text): + if not match or '://' in match: + continue + if any(ch.isspace() for ch in match): + continue + if any(ch in match for ch in "()[]{}'\"<>*?"): + continue + if '...' in match: + continue + if match.startswith('@'): + match = match[1:] + if '/' in match or match.startswith('.'): + print(match) +PY +) +done < <(find "$SKILL_ROOT" -type f -name 'SKILL.md' -print0 | sort -z) + +if [[ ${#invalid[@]} -gt 0 ]]; then + echo "ERROR: invalid repo paths referenced in skill docs:" >&2 + printf '%s\n' "${invalid[@]}" | sort -u >&2 + echo >&2 + echo "Fix the backticked file path so it matches a real repository file or directory." >&2 + exit 1 +fi + +echo "✓ All backticked repo paths in skill docs exist." diff --git a/scripts/check-skill-file-paths_test.sh b/scripts/check-skill-file-paths_test.sh new file mode 100755 index 00000000000..f321b76afdc --- /dev/null +++ b/scripts/check-skill-file-paths_test.sh @@ -0,0 +1,56 @@ +#!/bin/bash +set +o histexpand +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TEST_SCRIPT="$SCRIPT_DIR/check-skill-file-paths.sh" + +pass() { echo "PASS: $1"; } +fail() { echo "FAIL: $1"; echo " $2"; exit 1; } + +TMP_ROOT="$(mktemp -d)" +trap 'rm -rf "$TMP_ROOT"' EXIT + +# Valid skill references should pass. +VALID_ROOT="$TMP_ROOT/valid" +mkdir -p "$VALID_ROOT/.github/skills/example-skill" "$VALID_ROOT/pkg/workflow/js" +cat > "$VALID_ROOT/.github/skills/example-skill/SKILL.md" <<'EOF' +Use `pkg/workflow/js/messages_core.cjs` and `./README.md`. +EOF +printf '%s\n' 'example' > "$VALID_ROOT/README.md" +: > "$VALID_ROOT/pkg/workflow/js/messages_core.cjs" +VALID_OUT="$TMP_ROOT/valid.out" +if (cd "$VALID_ROOT" && bash "$TEST_SCRIPT" --repo-root "$VALID_ROOT" >"$VALID_OUT" 2>&1); then + pass "valid skill file paths pass" +else + fail "valid skill file paths should pass" "$(cat "$VALID_OUT")" +fi + +# Invalid repo paths should fail. +INVALID_ROOT="$TMP_ROOT/invalid" +mkdir -p "$INVALID_ROOT/.github/skills/example-skill" "$INVALID_ROOT/pkg/workflow/js" +cat > "$INVALID_ROOT/.github/skills/example-skill/SKILL.md" <<'EOF' +This doc references the stale repo path `pkg/workflow/nope.cjs` and the package name `github/gh-aw`, which should be treated differently. +EOF +INVALID_OUT="$TMP_ROOT/invalid.out" +if (cd "$INVALID_ROOT" && bash "$TEST_SCRIPT" --repo-root "$INVALID_ROOT" >"$INVALID_OUT" 2>&1); then + fail "invalid skill path should fail" "$(cat "$INVALID_OUT")" +elif grep -q "pkg/workflow/nope.cjs" "$INVALID_OUT" && ! grep -q "github/gh-aw" "$INVALID_OUT"; then + pass "invalid skill file paths fail with the offending path while ignoring package-name references" +else + fail "invalid skill path output did not distinguish stale file paths from package names" "$(cat "$INVALID_OUT")" +fi + +# Missing skill directory should error. +MISSING_ROOT="$TMP_ROOT/missing" +mkdir -p "$MISSING_ROOT" +MISSING_OUT="$TMP_ROOT/missing.out" +if (cd "$MISSING_ROOT" && bash "$TEST_SCRIPT" --repo-root "$MISSING_ROOT" >"$MISSING_OUT" 2>&1); then + fail "missing skill directory should fail" "$(cat "$MISSING_OUT")" +elif grep -qi "skill directory not found" "$MISSING_OUT"; then + pass "missing skill directory exits with an error" +else + fail "missing skill directory output was unexpected" "$(cat "$MISSING_OUT")" +fi + +echo "All skill path checks passed."