-
Notifications
You must be signed in to change notification settings - Fork 298
[docs] Consolidate engine architecture, JS sanitization pipeline, activation output transforms into dev.md v2.9 #17999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||||||||||||||||
| # Developer Instructions | ||||||||||||||||||
|
|
||||||||||||||||||
| **Version**: 2.8 | ||||||||||||||||||
| **Last Updated**: 2026-02-23 | ||||||||||||||||||
| **Version**: 2.9 | ||||||||||||||||||
| **Last Updated**: 2026-02-24 | ||||||||||||||||||
| **Purpose**: Consolidated development guidelines for GitHub Agentic Workflows | ||||||||||||||||||
|
|
||||||||||||||||||
| This document consolidates specifications from the scratchpad directory into unified developer instructions. It provides architecture patterns, security guidelines, code organization rules, and testing practices. | ||||||||||||||||||
|
|
@@ -197,7 +197,27 @@ Rationale: | |||||||||||||||||
| - New engines added without affecting existing ones | ||||||||||||||||||
| - Clear boundaries reduce merge conflicts | ||||||||||||||||||
|
|
||||||||||||||||||
| **3. Test Organization Pattern** | ||||||||||||||||||
| **3. Engine Interface Architecture** | ||||||||||||||||||
|
|
||||||||||||||||||
| The engine system implements Interface Segregation Principle (ISP) with 7 focused interfaces composed into a single composite interface (`CodingAgentEngine`): | ||||||||||||||||||
|
|
||||||||||||||||||
| ``` | ||||||||||||||||||
| CodingAgentEngine (composite) | ||||||||||||||||||
| ├── Engine – core identity (GetID, GetDisplayName, IsExperimental) | ||||||||||||||||||
| ├── CapabilityProvider – feature flags (SupportsFirewall, SupportsMaxTurns, ...) | ||||||||||||||||||
| ├── WorkflowExecutor – GitHub Actions step generation | ||||||||||||||||||
| ├── MCPConfigProvider – MCP server configuration rendering | ||||||||||||||||||
| ├── LogParser – log metric extraction | ||||||||||||||||||
| └── SecurityProvider – secret names and detection model | ||||||||||||||||||
| ``` | ||||||||||||||||||
|
|
||||||||||||||||||
| `BaseEngine` provides default implementations for `CapabilityProvider`, `LogParser`, and `SecurityProvider`. New engines embed `BaseEngine` and override only the methods they need to customize. | ||||||||||||||||||
|
|
||||||||||||||||||
| **Engine Registry**: `EngineRegistry` provides centralized registration, lookup by ID or prefix, and plugin-support validation. Use it rather than direct struct instantiation. | ||||||||||||||||||
|
|
||||||||||||||||||
| **Adding a new engine**: For the full implementation checklist including interface compliance tests, see `scratchpad/adding-new-engines.md`. | ||||||||||||||||||
|
|
||||||||||||||||||
| **4. Test Organization Pattern** | ||||||||||||||||||
|
|
||||||||||||||||||
| Pattern: Tests live alongside implementation with descriptive names | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -1209,6 +1229,39 @@ func SanitizeLabel(label string) string { | |||||||||||||||||
| } | ||||||||||||||||||
| ``` | ||||||||||||||||||
|
|
||||||||||||||||||
| ### JavaScript Content Sanitization Pipeline | ||||||||||||||||||
|
|
||||||||||||||||||
| The JavaScript sanitization module (`actions/setup/js/sanitize_content_core.cjs`) applies a multi-stage pipeline to all incoming content before it is written to GitHub resources: | ||||||||||||||||||
|
|
||||||||||||||||||
| ``` | ||||||||||||||||||
| Input Text | ||||||||||||||||||
| │ | ||||||||||||||||||
| ▼ hardenUnicodeText() | ||||||||||||||||||
| ├─ Unicode normalization (NFC) | ||||||||||||||||||
| ├─ HTML entity decoding ← prevents entity-encoded bypass attacks | ||||||||||||||||||
| ├─ Zero-width character removal | ||||||||||||||||||
| ├─ Bidirectional control removal | ||||||||||||||||||
| └─ Full-width ASCII conversion | ||||||||||||||||||
| │ | ||||||||||||||||||
| ▼ ANSI escape sequence removal | ||||||||||||||||||
| │ | ||||||||||||||||||
| ▼ neutralizeTemplateDelimiters() ← T24 defense-in-depth | ||||||||||||||||||
| ├─ Jinja2/Liquid: {{ }} → \{\{ | ||||||||||||||||||
| ├─ ERB: <%= %> → \<%= | ||||||||||||||||||
| ├─ JS template literals: ${ } → \$\{ | ||||||||||||||||||
| └─ Jekyll/Liquid directives: {% %} → \{% | ||||||||||||||||||
| │ | ||||||||||||||||||
| ▼ neutralizeMentions() | ||||||||||||||||||
| │ | ||||||||||||||||||
| ▼ Output (safe text) | ||||||||||||||||||
| ``` | ||||||||||||||||||
|
Comment on lines
+1234
to
+1257
|
||||||||||||||||||
|
|
||||||||||||||||||
| **HTML Entity Decoding**: Before @mention detection, all entity variants are decoded—named (`@`), decimal (`@`), hexadecimal (`@`), and double-encoded (`&#64;`). This prevents attackers from using entity-encoded `@` symbols to trigger unwanted user notifications. | ||||||||||||||||||
|
|
||||||||||||||||||
| **Template Delimiter Neutralization (T24)**: Template syntax delimiters are escaped as a defense-in-depth measure. GitHub's markdown rendering does not evaluate these patterns, but explicit neutralization documents the defense and protects against future integration scenarios where content might reach a template engine. Logs a warning when patterns are detected. | ||||||||||||||||||
|
|
||||||||||||||||||
| Both defenses are automatic and apply unconditionally to `sanitizeIncomingText()`, `sanitizeContentCore()`, and `sanitizeContent()`. | ||||||||||||||||||
|
|
||||||||||||||||||
| ### Template Injection Prevention | ||||||||||||||||||
|
|
||||||||||||||||||
| **Safe Template Evaluation**: | ||||||||||||||||||
|
|
@@ -1477,6 +1530,32 @@ func routeWorkflow(event Event) (string, error) { | |||||||||||||||||
| } | ||||||||||||||||||
| ``` | ||||||||||||||||||
|
|
||||||||||||||||||
| ### Activation Output Transformations | ||||||||||||||||||
|
|
||||||||||||||||||
| The compiler automatically rewrites three specific `needs.activation.outputs.*` expressions to `steps.sanitized.outputs.*` when they appear inside the activation job itself. A GitHub Actions job cannot reference its own outputs via `needs.<job-name>.*`—those references are only valid in downstream jobs. | ||||||||||||||||||
|
|
||||||||||||||||||
| **Transformed expressions** (within the activation job only): | ||||||||||||||||||
|
|
||||||||||||||||||
| | From | To | | ||||||||||||||||||
| |------|----| | ||||||||||||||||||
| | `needs.activation.outputs.text` | `steps.sanitized.outputs.text` | | ||||||||||||||||||
| | `needs.activation.outputs.title` | `steps.sanitized.outputs.title` | | ||||||||||||||||||
| | `needs.activation.outputs.body` | `steps.sanitized.outputs.body` | | ||||||||||||||||||
|
|
||||||||||||||||||
| **Not transformed** (remain as `needs.activation.outputs.*` since they are consumed by later jobs): | ||||||||||||||||||
| `comment_id`, `comment_repo`, `slash_command`, `issue_locked` | ||||||||||||||||||
|
|
||||||||||||||||||
| **Why this matters for runtime-import**: When a workflow uses `{{#runtime-import}}` to include an external file at runtime (without recompiling), any new references to `needs.activation.outputs.{text|title|body}` introduced by that file will work correctly because the compiler pre-generates all known expressions and applies the transformation before execution. | ||||||||||||||||||
|
|
||||||||||||||||||
| **Implementation**: `pkg/workflow/expression_extraction.go::transformActivationOutputs()` | ||||||||||||||||||
|
Comment on lines
+1548
to
+1550
|
||||||||||||||||||
| **Why this matters for runtime-import**: When a workflow uses `{{#runtime-import}}` to include an external file at runtime (without recompiling), any new references to `needs.activation.outputs.{text|title|body}` introduced by that file will work correctly because the compiler pre-generates all known expressions and applies the transformation before execution. | |
| **Implementation**: `pkg/workflow/expression_extraction.go::transformActivationOutputs()` | |
| **Why this matters for runtime-import**: The rewrite only applies to expressions that the compiler extracts at compile time. Imports without inputs are emitted as `{{#runtime-import ...}}`, and their file contents are not parsed for expression extraction or rewrite. New references to `needs.activation.outputs.{text|title|body}` introduced *solely* inside runtime-imported files will therefore **not** be rewritten. When authoring runtime-imported content, either: | |
| - use `steps.sanitized.outputs.{text|title|body}` directly, or | |
| - enable inlined imports for that file so the compiler can see and transform the expressions during extraction. | |
| **Implementation**: `pkg/workflow/expression_extraction.go::transformActivationOutputs()` — this runs during expression extraction over the content known at compile time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The section says CodingAgentEngine is composed from “7 focused interfaces”, but the diagram only lists 6 and omits
ModelEnvVarProvider(present inpkg/workflow/agentic_engine.gowhereCodingAgentEngineembedsModelEnvVarProvider). Also, theEngineinterface includesGetDescription()in code, but it’s missing from the “core identity” method list here. Please update the diagram/text to match the current interface set/signatures.