Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/project/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ var createTemplateURLFlag string
var createGitBranchFlag string
var createAppNameFlag string
var createListFlag bool
var createSubdirFlag string

// Handle to client's create function used for testing
// TODO - Find best practice, such as using an Interface and Struct to create a client
Expand Down Expand Up @@ -66,6 +67,7 @@ name your app 'agent' (not create an AI Agent), use the --name flag instead.`,
{Command: "create agent my-agent-app", Meaning: "Create a new AI Agent app"},
{Command: "create my-project -t slack-samples/deno-hello-world", Meaning: "Start a new project from a specific template"},
{Command: "create --name my-project", Meaning: "Create a project named 'my-project'"},
{Command: "create my-project -t org/monorepo --subdir apps/my-app", Meaning: "Create from a subdirectory of a template"},
}),
Args: cobra.MaximumNArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -79,6 +81,7 @@ name your app 'agent' (not create an AI Agent), use the --name flag instead.`,
cmd.Flags().StringVarP(&createGitBranchFlag, "branch", "b", "", "name of git branch to checkout")
cmd.Flags().StringVarP(&createAppNameFlag, "name", "n", "", "name for your app (overrides the name argument)")
cmd.Flags().BoolVar(&createListFlag, "list", false, "list available app templates")
cmd.Flags().StringVar(&createSubdirFlag, "subdir", "", "subdirectory within the template to use as project root")
Comment thread
srtaalej marked this conversation as resolved.
Outdated

return cmd
}
Expand Down Expand Up @@ -141,6 +144,7 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args []
AppName: appNameArg,
Template: template,
GitBranch: createGitBranchFlag,
Subdir: createSubdirFlag,
Comment thread
zimeg marked this conversation as resolved.
}
clients.EventTracker.SetAppTemplate(template.GetTemplatePath())

Expand Down
46 changes: 46 additions & 0 deletions cmd/project/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,52 @@ func TestCreateCommand(t *testing.T) {
cm.IO.AssertNotCalled(t, "SelectPrompt", mock.Anything, "Select an app:", mock.Anything, mock.Anything)
},
},
"passes subdir flag to create function": {
CmdArgs: []string{"--template", "slack-samples/bolt-js-starter-template", "--subdir", "apps/my-app"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cm.IO.On("SelectPrompt", mock.Anything, "Select an app:", mock.Anything, mock.Anything).
Return(
iostreams.SelectPromptResponse{
Flag: true,
Option: "slack-samples/bolt-js-starter-template",
},
nil,
)
cm.IO.On("SelectPrompt", mock.Anything, "Select a language:", mock.Anything, mock.Anything).
Return(
iostreams.SelectPromptResponse{
Flag: true,
Option: "slack-samples/bolt-js-starter-template",
},
nil,
)
createClientMock = new(CreateClientMock)
createClientMock.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", nil)
CreateFunc = createClientMock.Create
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
template, err := create.ResolveTemplateURL("slack-samples/bolt-js-starter-template")
require.NoError(t, err)
expected := create.CreateArgs{
Template: template,
Subdir: "apps/my-app",
}
createClientMock.AssertCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything, expected)
},
},
"list flag ignores subdir": {
CmdArgs: []string{"--list", "--subdir", "foo"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
createClientMock = new(CreateClientMock)
CreateFunc = createClientMock.Create
},
ExpectedOutputs: []string{
"Getting started",
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
createClientMock.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
},
},
"lists all templates with --list flag": {
CmdArgs: []string{"--list"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
Expand Down
70 changes: 68 additions & 2 deletions internal/pkg/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type CreateArgs struct {
AppName string
Template Template
GitBranch string
Subdir string
}

// Create will create a new Slack app on the file system and app manifest on the Slack API.
Expand Down Expand Up @@ -121,8 +122,19 @@ func Create(ctx context.Context, clients *shared.ClientFactory, log *logger.Logg
}))

// Create the project from a templateURL
if err := createApp(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, log, clients.Fs); err != nil {
return "", slackerror.Wrap(err, slackerror.ErrAppCreate)
subdir, err := normalizeSubdir(createArgs.Subdir)
if err != nil {
return "", err
}

if subdir != "" {
if err := createAppFromSubdir(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, subdir, log, clients.Fs); err != nil {
return "", slackerror.Wrap(err, slackerror.ErrAppCreate)
}
} else {
if err := createApp(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, log, clients.Fs); err != nil {
return "", slackerror.Wrap(err, slackerror.ErrAppCreate)
}
Comment on lines +134 to +141
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

praise: clean implementation choice!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

πŸͺ¬ question(non-blocking): Would passing subdir to createApp in all instances - either a default "." or the normalized path - reduce this implementation more? I'm concerned that a change to the temporary directory logic might require repeated change if we keep separate setups for this.

}

// Change into the project directory to configure defaults and dependencies
Expand Down Expand Up @@ -343,6 +355,60 @@ func createApp(ctx context.Context, dirPath string, template Template, gitBranch
return nil
}

// normalizeSubdir cleans the subdir path and returns "" if it resolves to root.
func normalizeSubdir(subdir string) (string, error) {
Comment on lines +354 to +355
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

praise: very nice! πŸ‘ŒπŸ»

if subdir == "" {
return "", nil
}
cleaned := filepath.Clean(subdir)
if cleaned == "." || cleaned == "/" {
return "", nil
}
Comment on lines +359 to +362
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: After filepath.Clean() maybe we should use filepath.isLocal(cleaned).

This function appears to check if the file path is within the subtree and prevent traversal attacks where the path tries to access files outside of the root directory (template directory).

I haven't used it personally, but it seems like a good security measure.

Image

if strings.HasPrefix(cleaned, "..") || filepath.IsAbs(cleaned) {
return "", slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("subdirectory path %q must be relative and within the template", subdir)
Comment thread
srtaalej marked this conversation as resolved.
Outdated
}
return cleaned, nil
}

// createAppFromSubdir clones the full template into a temp directory, then copies
// only the specified subdirectory to the final project path.
func createAppFromSubdir(ctx context.Context, dirPath string, template Template, gitBranch string, subdir string, log *logger.Logger, fs afero.Fs) error {
tmpDir, err := os.MkdirTemp("", "slack-create-*")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: We should not use os. calls directly whenever possible. These calls are very hard to test because we can't mock it - it will create a temp directory during the unit test.

Instead, we should use our fs afero.Fs filesystem library. This uses os in production and a memory-based file system in tests.

I imagine we can use afero.GetTempDir() and afero.TempDir() to look something like:

tempDirRoot := afero.GetTempDir(fs, "")
tempDirPath, err := afero.TempDir(fs, tmpDirRoot, "slack-create-")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ty for sharing - i didnt know we were using a package for temp directories πŸ‘ this is a game changer πŸ’―

if err != nil {
return slackerror.Wrap(err, "failed to create temporary directory")
}
// Remove so createApp can create it fresh (go-git requires non-existent target)
os.Remove(tmpDir)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: Why do we make the temp directory on L377 and then immediately delete it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

MkdirTemp generates unique path names which we want for createApp. But we don't want the directory itself which is why its deleted right away. Its a workaround to extract the unique name creation part of the MkdirTemp function so we can use it in createApp.

defer os.RemoveAll(tmpDir)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: Likewise, here I think we should use fs so that we can write unit tests that do not modify the actual file system.


if err := createApp(ctx, tmpDir, template, gitBranch, log, fs); err != nil {
return err
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

praise: Clever and clean approach!


subdirPath := filepath.Join(tmpDir, subdir)
info, err := os.Stat(subdirPath)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: If we're using the fs then it's important that we'll need to use fs.Stat(subdirPath) so that it reads the real file system in production and memory-based file system during tests.

if err != nil {
if os.IsNotExist(err) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: I think this is okay, but if you want to mock the result during tests (to test both exist and not exist stats) then you can use the clients.Os.IsNotExist(err) method. Usually we do this by passing clients.Os in function similar to fs (which is clients.Fs).

return slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("subdirectory %q was not found in the template", subdir).
Comment thread
srtaalej marked this conversation as resolved.
Outdated
WithRemediation("Check that the path exists in the template at %q", template.GetTemplatePath())
Comment on lines +389 to +391
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TIL: %q vs %s

}
return slackerror.Wrap(err, "failed to access subdirectory")
}
if !info.IsDir() {
return slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("path %q in the template is not a directory", subdir)
Comment thread
srtaalej marked this conversation as resolved.
Outdated
}

return goutils.CopyDirectory(goutils.CopyDirectoryOpts{
Src: subdirPath,
Dst: dirPath,
IgnoreDirectories: []string{".git", ".venv", "node_modules"},
IgnoreFiles: []string{".DS_Store"},
})
Comment thread
zimeg marked this conversation as resolved.
}

// InstallProjectDependencies installs the project runtime dependencies or
// continues with next steps if that fails. You can specify the manifestSource
// for the project configuration file (default: ManifestSourceLocal)
Expand Down
124 changes: 124 additions & 0 deletions internal/pkg/create/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ package create
import (
"fmt"
"net/http"
"os"
"path/filepath"
"testing"

"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/experiment"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackhttp"
Expand Down Expand Up @@ -183,6 +185,128 @@ func TestCreateGitArgs(t *testing.T) {
assert.Equal(t, expectedArgs, testGitArgs)
}

func TestNormalizeSubdir(t *testing.T) {
tests := map[string]struct {
input string
expected string
expectError bool
}{
"empty string returns empty": {
input: "",
expected: "",
},
"dot returns empty": {
input: ".",
expected: "",
},
"slash returns empty": {
input: "/",
expected: "",
},
"simple subdir": {
input: "pydantic-ai/",
expected: "pydantic-ai",
},
"dot-prefixed subdir": {
input: "./my-app",
expected: "my-app",
},
"nested subdir": {
input: "apps/my-app",
expected: "apps/my-app",
},
"parent traversal is rejected": {
input: "../escape",
expectError: true,
},
"nested parent traversal is rejected": {
input: "foo/../../escape",
expectError: true,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
result, err := normalizeSubdir(tc.input)
if tc.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.Equal(t, tc.expected, result)
}
})
}
}

func TestCreateAppFromSubdir(t *testing.T) {
tests := map[string]struct {
setupTemplate func(t *testing.T) string
subdir string
expectError bool
errorContains string
expectFiles []string
}{
"extracts subdirectory from local template": {
setupTemplate: func(t *testing.T) string {
tmpDir := t.TempDir()
// Create a subdirectory with a file
subdir := filepath.Join(tmpDir, "apps", "my-app")
require.NoError(t, os.MkdirAll(subdir, 0755))
require.NoError(t, os.WriteFile(filepath.Join(subdir, "manifest.json"), []byte(`{}`), 0644))
// Create a file at root that should NOT be copied
require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "README.md"), []byte("root readme"), 0644))
return tmpDir
},
subdir: "apps/my-app",
expectFiles: []string{"manifest.json"},
},
"returns error for nonexistent subdirectory": {
setupTemplate: func(t *testing.T) string {
return t.TempDir()
},
subdir: "nonexistent",
expectError: true,
errorContains: "was not found in the template",
},
"returns error when subdir path is a file": {
setupTemplate: func(t *testing.T) string {
tmpDir := t.TempDir()
require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "not-a-dir"), []byte("file"), 0644))
return tmpDir
},
subdir: "not-a-dir",
expectError: true,
errorContains: "is not a directory",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
templateDir := tc.setupTemplate(t)
outputDir := t.TempDir()
// Remove output dir so CopyDirectory can create it
require.NoError(t, os.Remove(outputDir))

template := Template{path: templateDir, isLocal: true}
log := logger.New(func(event *logger.LogEvent) {})
fs := afero.NewOsFs()

err := createAppFromSubdir(t.Context(), outputDir, template, "", tc.subdir, log, fs)

if tc.expectError {
assert.Error(t, err)
if tc.errorContains != "" {
assert.Contains(t, err.Error(), tc.errorContains)
}
} else {
assert.NoError(t, err)
for _, f := range tc.expectFiles {
_, statErr := os.Stat(filepath.Join(outputDir, f))
assert.NoError(t, statErr, "expected file %s to exist", f)
}
}
})
}
}

func Test_Create_installProjectDependencies(t *testing.T) {
tests := map[string]struct {
experiments []string
Expand Down
6 changes: 6 additions & 0 deletions internal/slackerror/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ const (
ErrSocketConnection = "socket_connection_error"
ErrScopesExceedAppConfig = "scopes_exceed_app_config"
ErrStreamingActivityLogs = "streaming_activity_logs_error"
ErrSubdirNotFound = "subdir_not_found"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

πŸ’‘ thought(non-blocking): Would it make sense to group this alongside other template errors? To me the referenced subdir wouldn't be clear at a glance. I'd suggest:

template_subdir_not_found

Which is so close to template_path_not_found!

ErrSurveyConfigNotFound = "survey_config_not_found"
ErrSystemConfigIDNotFound = "system_config_id_not_found"
ErrSystemRequirementsFailed = "system_requirements_failed"
Expand Down Expand Up @@ -1391,6 +1392,11 @@ Otherwise start your app for local development with: %s`,
Message: "Failed to stream the most recent activity logs",
},

ErrSubdirNotFound: {
Code: ErrSubdirNotFound,
Message: "The specified subdirectory was not found in the template repository",
},

ErrSurveyConfigNotFound: {
Code: ErrSurveyConfigNotFound,
Message: "Survey config not found",
Expand Down
Loading