diff --git a/docs/src/content/docs/specs/model-alias-specification.md b/docs/src/content/docs/specs/model-alias-specification.md index 31694a10026..0fd8426e6af 100644 --- a/docs/src/content/docs/specs/model-alias-specification.md +++ b/docs/src/content/docs/specs/model-alias-specification.md @@ -43,9 +43,10 @@ This document is governed by the GitHub Agentic Workflows project specifications 13. [Safeguards](#13-safeguards) 14. [Sync Notes](#14-sync-notes) 15. [Norms](#15-norms) -16. [Appendices](#appendices) -17. [References](#references) -18. [Change Log](#change-log) +16. [Operations](#16-operations) +17. [Appendices](#appendices) +18. [References](#references) +19. [Change Log](#change-log) --- @@ -769,7 +770,7 @@ This limit prevents runaway resolution in pathological alias maps and bounds the This section maps normative sections of this specification to the implementation files in `pkg/workflow/` that realize each requirement. Use this mapping to identify which files must be reviewed or updated when specification sections change. -**Last verified**: 2026-06-01 +**Last verified**: 2026-07-08 ### §4–§6 Parsing and Parameter Encoding @@ -844,6 +845,18 @@ This section provides a normative reference table for all MUST/SHALL requirement --- +## 16. Operations + +This section defines the caller-facing runtime lifecycle for resolving a model alias at compile time. + +1. **Invoke Resolution**: The implementation MUST parse the caller-provided model identifier into base identifier and parameter map before attempting alias expansion. +2. **Initialize Guards**: The implementation MUST initialize a depth counter (starting at 0) and a per-call visited-set before traversing alias references. +3. **Resolve Recursively**: For each alias hop, the implementation MUST increment depth, merge parameters per §8.3, and reject cycles when the current alias key already exists in the visited-set (before adding it for this hop). +4. **Enforce Limits**: The implementation MUST fail with a resolution error when depth exceeds the safeguard ceiling defined by R-MAF-S001. +5. **Return Deterministically**: The implementation MUST return the first conforming concrete model selected by §8 ordering rules, or a descriptive error when no candidate can be resolved. + +--- + ## Appendices ### Appendix A: Complete Resolution Example @@ -966,7 +979,7 @@ Model parameters are compile-time configuration values and are not derived from ### Version 1.2.0 (Draft) - **Added**: §13 Safeguards covering max alias-chain depth (R-MAF-S001), UTF-8 validity requirements (R-MAF-S002, R-MAF-S003), out-of-range `effort`/`temperature` handling (R-MAF-S004, R-MAF-S005), and corrupt builtin-alias-map behavior (R-MAF-S006, R-MAF-S007). -- **Added**: §14 Sync Notes mapping §§4–11 to implementation files in `pkg/workflow/` with loop-detection test references (last verified 2026-06-01). +- **Added**: §14 Sync Notes mapping §§4–11 to implementation files in `pkg/workflow/` with loop-detection test references (last verified 2026-07-08). - **Added**: §15 Norms reference table (`V-MAF-*` and `R-MAF-S*` IDs) for all MUST/SHALL requirements in §§4–13. - **Updated**: Table of Contents to include §§13–15. diff --git a/docs/src/content/docs/specs/repository-package-manifest-specification.md b/docs/src/content/docs/specs/repository-package-manifest-specification.md index 2917ae5037a..8d73e93b54d 100644 --- a/docs/src/content/docs/specs/repository-package-manifest-specification.md +++ b/docs/src/content/docs/specs/repository-package-manifest-specification.md @@ -153,6 +153,8 @@ The remove lifecycle uninstalls a previously installed package by deleting its i **R-PKG-R004**: After removal, if the target workflow directory is empty, the implementation MAY remove the empty directory. The implementation MUST NOT remove non-empty directories. +> **Note**: Package-installed documentation files (for example `README.md`) are within the scope of R-PKG-R001 and follow modified-file protection in R-PKG-R002. + ## 6. Documentation Package documentation is `README.md` in the package root. @@ -343,7 +345,7 @@ This section provides a normative reference table for all MUST/SHALL requirement This section maps normative sections of this specification to the implementation files in `pkg/cli/` and `pkg/parser/` that realize each requirement. -**Last verified**: 2026-06-01 +**Last verified**: 2026-07-08 ### §4 Manifest Format — Implementation Mapping @@ -361,6 +363,7 @@ This section maps normative sections of this specification to the implementation |---|---|---| | §5 File resolution | Resolving `files` list vs. auto-discovery under `workflows/` and `.github/workflows/` | `pkg/cli/add_package_manifest.go` (`resolveRepositoryPackage`) | | §5 Install ordering | Download → compile → write per-file sequencing | `pkg/cli/add_package_manifest.go`, `pkg/cli/add_command.go` | +| §5 Install rollback on write failure (R-PKG-003) | Write-failure abort plus rollback of files written earlier in the same add operation | `pkg/cli/add_command.go` (`addWorkflowsWithTracking`), `pkg/cli/add_command_test.go` (`TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure`) | ### §4.2 and §4.3 Verification Findings diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 5650cf7f9f3..c4956a6da68 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -1990,6 +1990,57 @@ For `x-safe-outputs-target-requirements["*"]`: - If no listed `anyOf` field is present, the request MUST be rejected with an MCP validation error. - When `target` is not `"*"`, implementations MUST follow each type's normal **Operational Semantics** context resolution behavior; this metadata MUST NOT add additional required runtime identifier fields. +### 7.0.2 Handler Function Cross-References + +The following table defines the exact `createHandlers()` function used for each safe output type defined in §7: + +| Safe Output Type | Handler Function in `actions/setup/js/safe_outputs_handlers.cjs` | +|---|---| +| `create_issue` | `createIssueHandler` | +| `add_comment` | `addCommentHandler` | +| `create_pull_request` | `createPullRequestHandler` | +| `noop` | `defaultHandler("noop")` | +| `comment_memory` | `defaultHandler("comment_memory")` | +| `update_issue` | `updateIssueHandler` | +| `close_issue` | `defaultHandler("close_issue")` | +| `link_sub_issue` | `defaultHandler("link_sub_issue")` | +| `create_discussion` | `defaultHandler("create_discussion")` | +| `update_discussion` | `defaultHandler("update_discussion")` | +| `close_discussion` | `defaultHandler("close_discussion")` | +| `update_pull_request` | `updatePullRequestHandler` | +| `close_pull_request` | `defaultHandler("close_pull_request")` | +| `merge_pull_request` | `defaultHandler("merge_pull_request")` | +| `mark_pull_request_as_ready_for_review` | `defaultHandler("mark_pull_request_as_ready_for_review")` | +| `push_to_pull_request_branch` | `pushToPullRequestBranchHandler` | +| `push_repo_memory` | `pushRepoMemoryHandler` | +| `create_pull_request_review_comment` | `createPullRequestReviewCommentHandler` | +| `submit_pull_request_review` | `submitPullRequestReviewHandler` | +| `dismiss_pull_request_review` | `dismissPullRequestReviewHandler` | +| `resolve_pull_request_review_thread` | `defaultHandler("resolve_pull_request_review_thread")` | +| `reply_to_pull_request_review_comment` | `defaultHandler("reply_to_pull_request_review_comment")` | +| `add_labels` | `defaultHandler("add_labels")` | +| `remove_labels` | `defaultHandler("remove_labels")` | +| `add_reviewer` | `defaultHandler("add_reviewer")` | +| `assign_milestone` | `defaultHandler("assign_milestone")` | +| `assign_to_agent` | `defaultHandler("assign_to_agent")` | +| `assign_to_user` | `defaultHandler("assign_to_user")` | +| `unassign_from_user` | `defaultHandler("unassign_from_user")` | +| `hide_comment` | `defaultHandler("hide_comment")` | +| `create_project` | `createProjectHandler` | +| `update_project` | `defaultHandler("update_project")` | +| `create_project_status_update` | `defaultHandler("create_project_status_update")` | +| `update_release` | `defaultHandler("update_release")` | +| `upload_asset` | `uploadAssetHandler` | +| `upload_artifact` | `uploadArtifactHandler` | +| `dispatch_workflow` | `defaultHandler("dispatch_workflow")` | +| `create_code_scanning_alert` | `defaultHandler("create_code_scanning_alert")` | +| `autofix_code_scanning_alert` | `defaultHandler("autofix_code_scanning_alert")` | +| `create_check_run` | `defaultHandler("create_check_run")` | +| `create_agent_session` | `defaultHandler("create_agent_session")` | +| `missing_tool` | `defaultHandler("missing_tool")` | +| `missing_data` | `defaultHandler("missing_data")` | +| `report_incomplete` | `defaultHandler("report_incomplete")` | + ### 7.1 Core Issue Operations #### Type: create_issue @@ -5326,6 +5377,11 @@ This section maps normative specification requirements (§3–§11) to implement | §7 Safe Output Type Definitions | Handler implementations for each type | `actions/setup/js/safe_outputs_handlers.cjs`, `actions/setup/js/safe_outputs_tools.json` | | §7.1 Core Issue Operations | `create_issue`, `add_comment`, `hide_comment`, `close_issue` | `actions/setup/js/add_comment.cjs`, `actions/setup/js/safe_outputs_handlers.cjs` | | §8 Protocol Exchange Patterns | stdio container transport, tool invocation, MCP server constraint enforcement | `actions/setup/js/safe_outputs_mcp_server.cjs`, `actions/setup/js/safe_outputs_mcp_server_http.cjs` | +| §8.3 MCE1 Early Validation | Invocation-time validation wiring through MCP server startup | `actions/setup/js/safe_outputs_mcp_server.cjs` (`startSafeOutputsServer` → `createHandlers()`), `actions/setup/js/safe_outputs_handlers.cjs` (`addCommentHandler`, `createIssueHandler`, `updatePullRequestHandler`) | +| §8.3 MCE2 Tool Description Disclosure | Tool descriptions/schemas exposed during MCP tool registration | `actions/setup/js/safe_outputs_mcp_server.cjs` (`registerPredefinedTools` call), `actions/setup/js/safe_outputs_tools_loader.cjs` (`registerPredefinedTools`) | +| §8.3 MCE3 Actionable Error Responses | Structured MCP validation errors with remediation guidance | `actions/setup/js/safe_outputs_mcp_server.cjs` (`start()` call serving handler responses), `actions/setup/js/safe_outputs_handlers.cjs` (`buildIntentErrorResponse`, `addCommentHandler`) | +| §8.3 MCE4 Dual Enforcement | Invocation-time max checks before NDJSON append plus processor-time enforcement | `actions/setup/js/safe_outputs_mcp_server.cjs` (`createHandlers()` call path), `actions/setup/js/safe_outputs_handlers.cjs` (`enforcePerTypeMax`, `appendSafeOutputCounted`) | +| §8.3 MCE5 Constraint Configuration Consistency | Shared safe-outputs config passed into handler creation for consistent limits | `actions/setup/js/safe_outputs_mcp_server.cjs` (`bootstrapSafeOutputsServer`, `createHandlers(server, appendSafeOutput, safeOutputsConfig)`), `actions/setup/js/safe_outputs_handlers.cjs` (`getSafeOutputsToolConfig`) | | §9 Content Integrity Mechanisms | Content sanitization pipeline | `actions/setup/js/safe_output_validator.cjs`, `actions/setup/js/safe_output_type_validator.cjs` | | §10 Execution Guarantees | Idempotency, staged mode enforcement | `actions/setup/js/safe_outputs_handlers.cjs`, `actions/setup/js/safe_output_action_handler.cjs` | | §11 Cache Memory Integrity | Cache key format, integrity branches, git-backed branching | `pkg/workflow/compiler_safe_outputs.go`, `actions/setup/js/safe_outputs_bootstrap.cjs` | @@ -5337,8 +5393,7 @@ Sync procedure: Sync follow-up tasks: -- **[Open]** Add cross-references from §7 handler definitions to exact function names in `actions/setup/js/safe_outputs_handlers.cjs` for all 15+ safe output types. -- **[Open]** Map §8.3 MCP Server Constraint Enforcement requirements (MCE1–MCE5) to specific validation calls in `actions/setup/js/safe_outputs_mcp_server.cjs`. +- Keep §7.0.2 and §8.3 mapping rows current when handler names or MCP wiring changes in `actions/setup/js/safe_outputs_handlers.cjs`, `actions/setup/js/safe_outputs_mcp_server.cjs`, or `actions/setup/js/safe_outputs_tools_loader.cjs`. --- diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index ef04ff1749c..861a8459f56 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -277,11 +277,36 @@ func addWorkflows(ctx context.Context, workflows []*ResolvedWorkflow, opts AddOp return addWorkflowsWithTracking(ctx, workflows, tracker, opts) } +func prepareGitAttributesTracking(tracker *FileTracker) (path string, existed bool) { + gitRoot, err := gitutil.FindGitRoot() + if err != nil { + addLog.Printf("Skipping .gitattributes tracking setup: failed to find git root: %v", err) + return "", false + } + + path = filepath.Join(gitRoot, ".gitattributes") + existed = fileutil.FileExists(path) + if tracker != nil && existed { + tracker.TrackModified(path) + } + + return path, existed +} + +func trackGitAttributesIfCreated(tracker *FileTracker, path string, existed bool, updated bool) { + if tracker == nil || path == "" || existed || !updated { + return + } + tracker.TrackCreated(path) +} + // addWorkflows handles workflow addition using pre-fetched content func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error { addLog.Printf("Adding %d workflow(s) with tracking: force=%v, disableSecurityScanner=%v", len(workflows), opts.Force, opts.DisableSecurityScanner) // Ensure .gitattributes is configured unless flag is set if !opts.NoGitattributes { + gitAttributesPath, gitAttributesExisted := prepareGitAttributesTracking(tracker) + addLog.Print("Configuring .gitattributes") if updated, err := ensureGitAttributes(); err != nil { addLog.Printf("Failed to configure .gitattributes: %v", err) @@ -289,8 +314,11 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err))) } // Don't fail the entire operation if gitattributes update fails - } else if updated && opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes")) + } else if updated { + trackGitAttributesIfCreated(tracker, gitAttributesPath, gitAttributesExisted, updated) + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes")) + } } } @@ -305,6 +333,11 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow } if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil { + if tracker != nil { + if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil { + return fmt.Errorf("failed to add workflow '%s' (rollback also failed): %w", resolved.Spec.String(), errors.Join(err, rollbackErr)) + } + } return fmt.Errorf("failed to add workflow '%s': %w", resolved.Spec.String(), err) } } diff --git a/pkg/cli/add_command_test.go b/pkg/cli/add_command_test.go index 09eed7a169d..2aebbd63d1d 100644 --- a/pkg/cli/add_command_test.go +++ b/pkg/cli/add_command_test.go @@ -620,6 +620,51 @@ func TestAddWorkflowWithTracking_ActionWorkflow_Force(t *testing.T) { assert.Equal(t, newContent, written) } +func TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure(t *testing.T) { + tempDir := testutil.TempDir(t, "test-add-workflows-rollback-*") + workflowsDir := setupMinimalGitRepo(t, tempDir) + + validContent := []byte("---\nengine: claude\n---\n\n# workflow\n") + // workflow name "nested/blocked" intentionally causes write failure because + // addWorkflowWithTracking does not create nested workflow directories. + workflows := []*ResolvedWorkflow{ + { + Spec: &WorkflowSpec{ + WorkflowPath: "workflows/ok.md", + WorkflowName: "ok", + }, + Content: validContent, + SourceInfo: &FetchedWorkflow{ + IsLocal: true, + }, + }, + { + Spec: &WorkflowSpec{ + WorkflowPath: "workflows/blocked.md", + WorkflowName: "nested/blocked", + }, + Content: validContent, + SourceInfo: &FetchedWorkflow{ + IsLocal: true, + }, + }, + } + + err := addWorkflowsWithTracking(context.Background(), workflows, NewFileTracker(), AddOptions{ + NoGitattributes: true, + DisableSecurityScanner: true, + Quiet: true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to write destination file") + + _, statErr := os.Stat(filepath.Join(workflowsDir, "ok.md")) + assert.True(t, os.IsNotExist(statErr), "successful writes from this operation should be rolled back on later write failure") + + _, blockedStatErr := os.Stat(filepath.Join(workflowsDir, "nested", "blocked.md")) + assert.True(t, os.IsNotExist(blockedStatErr), "failing workflow should not leave partial output files") +} + func TestAddSkillFileWithTracking_PreservesPathFromSkillsRoot(t *testing.T) { gitRoot := testutil.TempDir(t, "test-add-skill-path-*") resolved := &ResolvedWorkflow{ diff --git a/pkg/cli/add_workflow_pr.go b/pkg/cli/add_workflow_pr.go index f1ae86afa01..7d80ed32dab 100644 --- a/pkg/cli/add_workflow_pr.go +++ b/pkg/cli/add_workflow_pr.go @@ -89,10 +89,6 @@ func addWorkflowsWithPR(ctx context.Context, workflows []*ResolvedWorkflow, opts prOpts := opts if err := addWorkflowsWithTracking(ctx, workflows, tracker, prOpts); err != nil { addWorkflowPRLog.Printf("Failed to add workflows: %v", err) - // Rollback on error - if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil && opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to rollback files: %v", rollbackErr))) - } return 0, "", fmt.Errorf("failed to add workflows: %w", err) } diff --git a/pkg/workflow/tools_validation_test.go b/pkg/workflow/tools_validation_test.go index 3b2610ebc90..f6e40876ebf 100644 --- a/pkg/workflow/tools_validation_test.go +++ b/pkg/workflow/tools_validation_test.go @@ -477,6 +477,16 @@ func TestValidateGitHubGuardPolicy(t *testing.T) { shouldError: true, errorMsg: "'github.min-integrity' to be set", }, + { + name: "allowed-repos non-all without min-integrity fails", + toolsMap: map[string]any{ + "github": map[string]any{ + "allowed-repos": "public", + }, + }, + shouldError: true, + errorMsg: "'github.min-integrity' is required", + }, { name: "blocked-users as GitHub Actions expression is valid", toolsMap: map[string]any{ diff --git a/scratchpad/guard-policies-specification.md b/scratchpad/guard-policies-specification.md index c6008371c40..8bcb547fee1 100644 --- a/scratchpad/guard-policies-specification.md +++ b/scratchpad/guard-policies-specification.md @@ -473,7 +473,7 @@ The YAML key `repos` under `tools.github` is **deprecated** as of guard-policy s **Migration path**: Use `gh aw fix` to automatically migrate `repos:` to `allowed-repos:` in workflow frontmatter. -**Removal target**: The `repos` alias SHOULD be removed in a future major version of the spec (tentatively v2.0.0). When the alias is removed, implementations MUST reject `repos` as an unknown field with an error message that suggests `allowed-repos`. +**Removal target**: The `repos` alias SHOULD be removed in a future major version of the spec; tracking is managed in issue [#44357](https://github.com/github/gh-aw/issues/44357). When the alias is removed, implementations MUST reject `repos` as an unknown field with an error message that suggests `allowed-repos`. --- @@ -553,6 +553,7 @@ This section maps normative sections of this specification to the implementation | GP-01, GP-03 pattern validation | Repository pattern format validation (exact, wildcard, prefix) | `pkg/workflow/tools_validation_github.go` (`validateReposScope`, `validateRepoPattern`, `isValidOwnerOrRepo`) | | GP-02 `min-integrity` validation | Enum value check for `none`/`unapproved`/`approved`/`merged` | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`) | | GP-04 empty array rejection | Empty `allowed-repos` array detection and error | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`) | +| GP-11 cross-field consistency | `allowed-repos` non-`"all"` without `min-integrity` MUST fail validation | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`), `pkg/workflow/tools_validation_test.go` (`allowed-repos non-all without min-integrity fails`) | | GP-10 lockdown precedence | Lockdown + guard-policy conflict detection and warning | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`, `emitGitHubLockdownGuardPolicyWarning`) | ### Safe-Outputs Guard Policy Derivation