Skip to content

gh add: better tracking of added/modified files#34

Merged
dsyme merged 14 commits intomainfrom
copilot/fix-33
Aug 13, 2025
Merged

gh add: better tracking of added/modified files#34
dsyme merged 14 commits intomainfrom
copilot/fix-33

Conversation

Copy link
Contributor

Copilot AI commented Aug 13, 2025

This PR implements comprehensive file tracking and explicit rollback functionality for the gh aw add --pr command, while enhancing user experience with consistent console formatting and simplifying the API.

Problem

The current add --pr command had several issues:

  1. Only staged files from .github/workflows/ directory, missing other generated files
  2. No automatic cleanup when git operations failed, leaving orphaned files
  3. Inconsistent console output with plain text messages
  4. Unnecessary backward compatibility layer cluttering the API

Solution

File Tracking & Explicit Rollback

  • Tracks all file operations during workflow addition (markdown files, lock files, .gitattributes, includes)
  • Provides explicit rollback on git operation failures with original content restoration at each error point
  • Ensures proper staging of all tracked files for complete commits
  • Improved error handling by replacing defer-based rollback with explicit rollback calls for better control flow

Enhanced User Experience

  • Consistent console formatting using the console package throughout the codebase
  • Styled error messages with ✗ symbols going to stderr
  • Informational messages with ℹ symbols for better visibility
  • Success messages with ✓ symbols for positive feedback
  • Warning messages with ⚠ symbols for non-critical issues

API Simplification

  • Removed backward compatibility wrapper AddWorkflow function
  • Streamlined API by having callers use AddWorkflowWithTracking directly
  • Updated all tests to use the simplified API

Implementation

The solution uses a modular FileTracker architecture (in separate file pkg/cli/file_tracker.go) with explicit rollback pattern:

// Add workflow files using tracking logic
if err := AddWorkflowWithTracking(workflow, number, verbose, engineOverride, name, force, tracker); err != nil {
    if rollbackErr := tracker.RollbackAllFiles(verbose); rollbackErr != nil && verbose {
        fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to rollback files: %v", rollbackErr)))
    }
    return fmt.Errorf("failed to add workflow: %w", err)
}

This explicit approach provides better control over when rollback occurs and makes the error handling flow more transparent compared to defer-based cleanup.

Testing

All existing tests pass with the new implementation. The console formatting provides immediate visual feedback making it easier to identify issues during development and usage.

Fixes #33.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 2 commits August 13, 2025 11:50
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add FileTracker struct to track created/modified files during workflow operations
- Modify AddWorkflowWithRepoAndPR to use file tracking with rollback on git operation failure
- Create AddWorkflowWithTracking function that tracks all file operations (md, lock.yml, .gitattributes)
- Update stageWorkflowChanges to stage specific tracked files instead of entire directories
- Add compileWorkflowWithTracking to track compilation-generated files
- Implement rollback functionality that deletes created files on error
- Add comprehensive tests for file tracker functionality
- Maintain backward compatibility with existing AddWorkflow function

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] add --pr command must git stage/commit all generated files Implement comprehensive file tracking and rollback for add --pr command Aug 13, 2025
Copilot AI requested a review from pelikhan August 13, 2025 12:13
…iles

- Move FileTracker struct and methods to pkg/cli/file_tracker.go for better organization
- Add OriginalContent map to store file content before modification
- Enhance TrackModified() to capture original content automatically
- Add RollbackModifiedFiles() method to restore files to original state
- Add RollbackAllFiles() method for comprehensive rollback
- Update defer function to use RollbackAllFiles() instead of RollbackCreatedFiles()
- Fix timing of tracking calls to happen before file modification
- Add comprehensive tests for all rollback scenarios

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 13, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.contoso.com
    • Triggering command: /tmp/go-build3984810454/b256/cli.test -test.testlogfile=/tmp/go-build3984810454/b256/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Implement comprehensive file tracking and rollback for add --pr command Implement comprehensive file tracking with modular rollback for add --pr command Aug 13, 2025
Copilot AI requested a review from pelikhan August 13, 2025 12:42
Copy link
Contributor

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply console formatting helpers from the console package for error messages

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 13, 2025

@copilot apply console formatting helpers from the console package for error messages

Applied console formatting helpers for error messages. Converted warning and error messages to use console.FormatWarningMessage(), console.FormatErrorMessage(), console.FormatSuccessMessage(), and console.FormatInfoMessage() throughout the codebase. Error messages now properly go to stderr with styled formatting. Commit c84acf2

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.contoso.com
    • Triggering command: /tmp/go-build3945037323/b001/cli.test -test.testlogfile=/tmp/go-build3945037323/b001/testlog.txt -test.paniconexit0 -test.v=true -test.timeout=5m0s (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Implement comprehensive file tracking with modular rollback for add --pr command Implement comprehensive file tracking with console formatting and API cleanup for add --pr command Aug 13, 2025
Copilot AI requested a review from pelikhan August 13, 2025 13:20
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 13, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.contoso.com
    • Triggering command: /tmp/go-build3426441333/b256/cli.test -test.testlogfile=/tmp/go-build3426441333/b256/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
    • Triggering command: /tmp/go-build1111799091/b256/cli.test -test.testlogfile=/tmp/go-build1111799091/b256/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Implement comprehensive file tracking with console formatting and API cleanup for add --pr command Implement comprehensive file tracking with explicit rollback and console formatting for add --pr command Aug 13, 2025
Copilot AI requested a review from pelikhan August 13, 2025 14:11
@pelikhan pelikhan changed the title Implement comprehensive file tracking with explicit rollback and console formatting for add --pr command gh add: better tracking of added/modified files Aug 13, 2025
@pelikhan pelikhan marked this pull request as ready for review August 13, 2025 15:52
@dsyme dsyme merged commit 426324b into main Aug 13, 2025
6 checks passed
@dsyme dsyme deleted the copilot/fix-33 branch August 13, 2025 16:22
pelikhan added a commit that referenced this pull request Aug 25, 2025
* Replace check-team-member shared action with inlined JavaScript script (#22)

* Initial plan

* Replace check-team-member template with inlined JavaScript script

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Fix code formatting (remove trailing whitespace)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Add check_team_member.cjs to tsconfig.json and create comprehensive tests

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>

* Refactor compute_text_action.yaml as embedded JavaScript with sanitization (#25)

* Simplify add-reaction feature by removing fallback and try/catch logic (#24)

* Initial plan

* Reimplement add-reaction feature as inlined JavaScript

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Remove "remove" functionality from add-reaction feature, simplify to only support adding reactions

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Remove fallback and try/catch clauses from add-reaction feature

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>

* Remove compute-text action dependency and template files from workflow system (#29)

* Fix linter issues: Remove unused reactionActionTemplate variable (#30)

* Add output text to test-claude and inline JavaScript instead of shared actions (#32)

* Remove obsolete reaction_action.yaml template file (#34)

* Remove unnecessary content permissions and checkout steps from task and add_reaction jobs (#35)

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Nov 5, 2025
This commit addresses CodeQL security alert #34 by removing detailed
error messages from workflow compilation failures that could potentially
expose sensitive information from secret configurations.

**Alert Details:**
- Alert Number: #34
- Severity: High
- Rule: go/clear-text-logging
- CWE: CWE-312, CWE-315, CWE-359

**Changes Made:**
- Modified pkg/cli/mcp_add.go line 147-149 to log generic error message
- Removed direct inclusion of error details in console output
- Error details are no longer passed through fmt.Sprintf which could
  leak sensitive data from the compilation process

**Security Impact:**
- Prevents potential exposure of secret values or sensitive configuration
  data in compilation error messages
- Maintains user experience by still informing them of compilation failure
- Users can still run 'gh aw compile --verbose' for detailed debugging

**Testing:**
- The fix maintains existing functionality
- Generic error message provides sufficient information for users
- Sensitive data no longer flows from secrets to logging output

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Nov 21, 2025
…ert #34)

This fix addresses CWE-312, CWE-315, and CWE-359 vulnerabilities where
sensitive data from secret configurations could flow through error messages
to logging output.

Changes:
- Removed detailed error messages from workflow compilation failures
- Changed from logging full error details to generic error messages
- Added security-focused comments explaining the fix

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
github-actions bot pushed a commit that referenced this pull request Dec 30, 2025
…tion

- Add //go:build integration to run_command_test.go (TestProgressFlagSignature: 30.26s)
- Add //go:build integration to compile_command_test.go (TestCompileWorkflows: 10.24s)
- Add //go:build integration to mcp_inspect_headers_test.go (TestConnectHTTPMCPServer: 20s)

These tests are already isolated in dedicated integration matrix groups but
were running in BOTH the unit test job and integration jobs, causing ~60s
of duplicate test execution per CI run.

Expected impact:
- Unit test time: 110s → 50s (45% reduction)
- Integration test time: No change (tests already in matrix)
- Net savings: ~60 seconds per run (~100 minutes/day)

Fixes duplicate test execution identified by CI Coach workflow run #34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants