From 48bfb36fb391c45007fda33b900fb838ff5c4ce4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Jul 2026 16:33:47 +0000 Subject: [PATCH 1/6] Initial plan From f5775ede1f46864a6f6807e959c1b8fe1b30fe70 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Jul 2026 17:01:46 +0000 Subject: [PATCH 2/6] specs/tests: complete SPDD 2026-07-08 follow-ups Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../docs/specs/model-alias-specification.md | 23 ++++++-- ...pository-package-manifest-specification.md | 5 +- .../docs/specs/safe-outputs-specification.md | 56 ++++++++++++++++++- pkg/cli/add_command.go | 5 ++ pkg/cli/add_command_test.go | 40 +++++++++++++ pkg/workflow/tools_validation_test.go | 10 ++++ scratchpad/guard-policies-specification.md | 3 +- 7 files changed, 133 insertions(+), 9 deletions(-) diff --git a/docs/src/content/docs/specs/model-alias-specification.md b/docs/src/content/docs/specs/model-alias-specification.md index 31694a10026..a4417eae9d5 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 next alias key already exists in the visited-set. +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..a1117919a6e 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. +For R-PKG-R001 scope, package-installed documentation files are treated the same as workflow files: a `README.md` installed as part of the package MUST be removed during package removal unless it has been modified, in which case R-PKG-R002 applies. + ## 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..e2324054664 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -1990,6 +1990,54 @@ 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` | +| `create_pull_request_review_comment` | `createPullRequestReviewCommentHandler` | +| `submit_pull_request_review` | `submitPullRequestReviewHandler` | +| `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` | +| `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 +5374,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 +5390,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..ac490fefabe 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -305,6 +305,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..7385691af55 100644 --- a/pkg/cli/add_command_test.go +++ b/pkg/cli/add_command_test.go @@ -620,6 +620,46 @@ 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") + 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") +} + func TestAddSkillFileWithTracking_PreservesPathFromSkillsRoot(t *testing.T) { gitRoot := testutil.TempDir(t, "test-add-skill-path-*") resolved := &ResolvedWorkflow{ 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 From b4b6707720e92fd56e8ccd4675500caafd58ab7c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Jul 2026 01:46:44 +0000 Subject: [PATCH 3/6] Address rollback ownership and spec mapping review gaps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../docs/specs/model-alias-specification.md | 2 +- ...pository-package-manifest-specification.md | 2 +- .../docs/specs/safe-outputs-specification.md | 3 +++ pkg/cli/add_command.go | 21 +++++++++++++++++-- pkg/cli/add_command_test.go | 5 +++++ pkg/cli/add_workflow_pr.go | 4 ---- 6 files changed, 29 insertions(+), 8 deletions(-) diff --git a/docs/src/content/docs/specs/model-alias-specification.md b/docs/src/content/docs/specs/model-alias-specification.md index a4417eae9d5..0fd8426e6af 100644 --- a/docs/src/content/docs/specs/model-alias-specification.md +++ b/docs/src/content/docs/specs/model-alias-specification.md @@ -851,7 +851,7 @@ This section defines the caller-facing runtime lifecycle for resolving a model a 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 next alias key already exists in the visited-set. +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. 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 a1117919a6e..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,7 +153,7 @@ 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. -For R-PKG-R001 scope, package-installed documentation files are treated the same as workflow files: a `README.md` installed as part of the package MUST be removed during package removal unless it has been modified, in which case R-PKG-R002 applies. +> **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 diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index e2324054664..c4956a6da68 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -2012,8 +2012,10 @@ The following table defines the exact `createHandlers()` function used for each | `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")` | @@ -2029,6 +2031,7 @@ The following table defines the exact `createHandlers()` function used for each | `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")` | diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index ac490fefabe..b4974f83c7f 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -282,6 +282,18 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow 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 { + var gitAttributesPath string + gitAttributesExisted := false + if tracker != nil { + if gitRoot, err := gitutil.FindGitRoot(); err == nil { + gitAttributesPath = filepath.Join(gitRoot, ".gitattributes") + gitAttributesExisted = fileutil.FileExists(gitAttributesPath) + if gitAttributesExisted { + tracker.TrackModified(gitAttributesPath) + } + } + } + addLog.Print("Configuring .gitattributes") if updated, err := ensureGitAttributes(); err != nil { addLog.Printf("Failed to configure .gitattributes: %v", err) @@ -289,8 +301,13 @@ 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 { + if tracker != nil && !gitAttributesExisted && gitAttributesPath != "" { + tracker.TrackCreated(gitAttributesPath) + } + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes")) + } } } diff --git a/pkg/cli/add_command_test.go b/pkg/cli/add_command_test.go index 7385691af55..08376df96ec 100644 --- a/pkg/cli/add_command_test.go +++ b/pkg/cli/add_command_test.go @@ -625,6 +625,8 @@ func TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure(t *testing workflowsDir := setupMinimalGitRepo(t, tempDir) validContent := []byte("---\nengine: claude\n---\n\n# workflow\n") + // WorkflowName "nested/blocked" intentionally causes write failure because + // addWorkflowWithTracking does not create nested workflow directories. workflows := []*ResolvedWorkflow{ { Spec: &WorkflowSpec{ @@ -658,6 +660,9 @@ func TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure(t *testing _, 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) { 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) } From 60aba46c0af0446a8c0da233beb476705bc3621d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Jul 2026 01:52:10 +0000 Subject: [PATCH 4/6] Refactor gitattributes rollback tracking in add flow Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_command.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index b4974f83c7f..182197c53d0 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -277,22 +277,27 @@ 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 { + return "", false + } + + path = filepath.Join(gitRoot, ".gitattributes") + existed = fileutil.FileExists(path) + if tracker != nil && existed { + tracker.TrackModified(path) + } + + return path, existed +} + // 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 { - var gitAttributesPath string - gitAttributesExisted := false - if tracker != nil { - if gitRoot, err := gitutil.FindGitRoot(); err == nil { - gitAttributesPath = filepath.Join(gitRoot, ".gitattributes") - gitAttributesExisted = fileutil.FileExists(gitAttributesPath) - if gitAttributesExisted { - tracker.TrackModified(gitAttributesPath) - } - } - } + gitAttributesPath, gitAttributesExisted := prepareGitAttributesTracking(tracker) addLog.Print("Configuring .gitattributes") if updated, err := ensureGitAttributes(); err != nil { @@ -302,6 +307,9 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow } // Don't fail the entire operation if gitattributes update fails } else if updated { + if gitAttributesPath == "" { + gitAttributesPath, _ = prepareGitAttributesTracking(nil) + } if tracker != nil && !gitAttributesExisted && gitAttributesPath != "" { tracker.TrackCreated(gitAttributesPath) } From 853326d5b4a4c36f7b8ceb309f41501fb326ff3b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Jul 2026 01:57:28 +0000 Subject: [PATCH 5/6] Refine gitattributes tracking helpers for clarity Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_command.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 182197c53d0..7e6bb8849ae 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -292,6 +292,13 @@ func prepareGitAttributesTracking(tracker *FileTracker) (path string, existed bo return path, existed } +func trackGitAttributesCreatedFile(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) @@ -307,12 +314,7 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow } // Don't fail the entire operation if gitattributes update fails } else if updated { - if gitAttributesPath == "" { - gitAttributesPath, _ = prepareGitAttributesTracking(nil) - } - if tracker != nil && !gitAttributesExisted && gitAttributesPath != "" { - tracker.TrackCreated(gitAttributesPath) - } + trackGitAttributesCreatedFile(tracker, gitAttributesPath, gitAttributesExisted, updated) if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes")) } From 7e8eac56784b1d77d5f7068315ba0c20cd4406f4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Jul 2026 02:02:43 +0000 Subject: [PATCH 6/6] Address final validation nits in add tracking Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_command.go | 5 +++-- pkg/cli/add_command_test.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 7e6bb8849ae..861a8459f56 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -280,6 +280,7 @@ func addWorkflows(ctx context.Context, workflows []*ResolvedWorkflow, opts AddOp 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 } @@ -292,7 +293,7 @@ func prepareGitAttributesTracking(tracker *FileTracker) (path string, existed bo return path, existed } -func trackGitAttributesCreatedFile(tracker *FileTracker, path string, existed bool, updated bool) { +func trackGitAttributesIfCreated(tracker *FileTracker, path string, existed bool, updated bool) { if tracker == nil || path == "" || existed || !updated { return } @@ -314,7 +315,7 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow } // Don't fail the entire operation if gitattributes update fails } else if updated { - trackGitAttributesCreatedFile(tracker, gitAttributesPath, gitAttributesExisted, updated) + trackGitAttributesIfCreated(tracker, gitAttributesPath, gitAttributesExisted, updated) if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes")) } diff --git a/pkg/cli/add_command_test.go b/pkg/cli/add_command_test.go index 08376df96ec..2aebbd63d1d 100644 --- a/pkg/cli/add_command_test.go +++ b/pkg/cli/add_command_test.go @@ -625,7 +625,7 @@ func TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure(t *testing workflowsDir := setupMinimalGitRepo(t, tempDir) validContent := []byte("---\nengine: claude\n---\n\n# workflow\n") - // WorkflowName "nested/blocked" intentionally causes write failure because + // workflow name "nested/blocked" intentionally causes write failure because // addWorkflowWithTracking does not create nested workflow directories. workflows := []*ResolvedWorkflow{ {