Skip to content

Conversation

@heavyengineer
Copy link
Contributor

@heavyengineer heavyengineer commented Oct 5, 2025

The previous implementation had permissions issues in some scenarios and wasn't portable.

This PR introduces a shell script for adding the docker container to .mcp.json by manipulating an existing file, or creating a new.mcp.json file. The script is run in the project directory and determines the correct uid and gid for the container to run and all the necessary paths.
The user cd's to their project directory runs, e.g. ~/spec-workflow-mcp/containers/setup-mcp.sh and they should be good to go.

I've been using this successfully for a week or so. Works well in my experience.

@Pimzino Pimzino requested a review from Copilot November 7, 2025 13:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the spec-workflow-mcp container setup to use runtime user/group ID mapping via the --user flag, replacing the previous build-time user configuration. The changes include a new automated setup script, updated documentation, and Dockerfile modifications to support this new approach.

Key changes:

  • Introduces an automated setup script (setup-mcp.sh) that detects user/group IDs and configures .mcp.json automatically
  • Updates Dockerfile to make application files world-readable and removes fixed user configuration, enabling runtime user mapping
  • Updates documentation to reflect the new configuration approach with full project directory mounting

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
containers/setup-mcp.sh New automated setup script that configures .mcp.json with proper user/group IDs and manages .spec-workflow directory permissions
containers/README.md Updated documentation to include automated setup instructions and clarify the new runtime user mapping approach
containers/Dockerfile Modified to remove fixed user configuration, make files world-readable, and support runtime --user flag usage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@heavyengineer
Copy link
Contributor Author

Is there anything else i need to do to get this merged? or is it no longer useful? cheers

@Pimzino
Copy link
Owner

Pimzino commented Nov 26, 2025

Sorry mate, just havent had a change to go through these pull requests as I had a backlog of changes to implement and was doing those first.

Will review. Thank you for the contribution!

@Pimzino
Copy link
Owner

Pimzino commented Nov 26, 2025

PR Review Feedback

Hi @heavyengineer, thanks for this contribution! The setup script is well-written and addresses a real pain point with container permissions. However, there are a few items that need to be addressed before this can be merged:

🔧 Required Changes

1. Rebase on latest main
The main branch Dockerfile has evolved since this PR was created. It now includes:

  • SPEC_WORKFLOW_HOME environment variable for global state storage
  • Creates /workspace/.spec-workflow-mcp directory
  • Different directory structure

Please rebase your branch on the latest main to incorporate these changes.

2. Fix README markdown syntax
There's a missing closing ``` for the JSON code block around line 60-70 in the diff. The JSON example runs into the bash example without proper closure.

3. Default port inconsistency
The PR changes the default port from 5000 to 3000 in the Dockerfile CMD. Please keep the default at 5000 for backward compatibility, or document this as a breaking change.

4. Document security model change
The PR removes USER node from the Dockerfile, meaning the container runs as root by default unless --user is specified at runtime. This is a valid approach for the --user flag pattern, but it should be documented in the README (perhaps in the Security Considerations section) so users understand they should always use the --user flag.

✅ What looks good

  • The setup-mcp.sh script is comprehensive with proper error handling, help documentation, and cross-platform compatibility
  • The approach of using runtime --user flag is a solid solution for permission issues
  • Good backup functionality for existing .mcp.json files
  • Clear user prompts and colored output

Once these items are addressed, this should be good to merge. Let me know if you have any questions!

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.

2 participants