Skip to content

fix: resolve workspace file browser to groves/ instead of projects/#302

Open
ptone wants to merge 3 commits into
GoogleCloudPlatform:mainfrom
ptone:scion/dev-issue-130
Open

fix: resolve workspace file browser to groves/ instead of projects/#302
ptone wants to merge 3 commits into
GoogleCloudPlatform:mainfrom
ptone:scion/dev-issue-130

Conversation

@ptone
Copy link
Copy Markdown
Member

@ptone ptone commented Jun 4, 2026

Summary

  • Fix: hubManagedProjectPath() now resolves to ~/.scion/groves/<slug>/ (the actual git checkout mounted as /workspace in agents) instead of ~/.scion/projects/<slug>/ (which only contains project metadata and Telegram plugin downloads)
  • Fallback preserved: If groves/ has no workspace content, falls back to projects/ for backward compatibility
  • Default changed: When neither directory has content, defaults to groves/ path instead of projects/

Fixes issue 130

Test plan

  • Updated TestHubManagedProjectPath — default path now resolves to groves/
  • Added TestHubManagedProjectPath_PrefersGrovesOverProjects — when both dirs have content, groves/ wins
  • Added TestHubManagedProjectPath_FallsBackToProjectsWhenGrovesEmpty — falls back to projects/ when groves/ has no real content
  • Added TestHubManagedProjectPath_DefaultsToGrovesWhenNeitherHasContent — defaults to groves/ when neither has content
  • All hub package tests pass

ptone added 2 commits June 3, 2026 23:37
The Hub UI file browser was showing the wrong directory contents. The
hubManagedProjectPath() function resolved workspace paths to
~/.scion/projects/<slug>/ (project metadata) instead of
~/.scion/groves/<slug>/ (the actual git checkout mounted as /workspace
in agents).

Reverse the lookup priority: check groves/ first, fall back to
projects/, and default to groves/ when neither has content.

Fixes #130
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request resolves a workspace file browser path resolution issue by reversing the lookup priority in hubManagedProjectPath() to prefer the actual git checkout directory (groves/<slug>) over the metadata directory (projects/<slug>). The changes also include updated and new unit tests to verify this behavior. The review feedback suggests adding a defensive check to prevent empty slugs from resolving to parent directories, which could lead to unintended operations, along with a corresponding unit test to verify this validation.

Comment thread pkg/hub/handlers.go
Comment on lines 3768 to 3769
func hubManagedProjectPath(slug string) (string, error) {
globalDir, err := config.GetGlobalDir()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

An empty slug passed to hubManagedProjectPath could resolve to the parent directory (e.g., ~/.scion/groves or ~/.scion/projects). Since hasWorkspaceContent checks if the directory contains any files other than .scion or shared-dirs, a parent directory containing other projects would return true, causing the function to incorrectly return the parent directory. This could lead to catastrophic operations (like directory deletion or overwriting) on the entire parent directory. Adding a defensive check for an empty slug prevents this.

Suggested change
func hubManagedProjectPath(slug string) (string, error) {
globalDir, err := config.GetGlobalDir()
func hubManagedProjectPath(slug string) (string, error) {
if slug == "" {
return "", errors.New("project slug cannot be empty")
}
globalDir, err := config.GetGlobalDir()

Comment on lines +114 to 118
// When neither has content, should default to groves/
path, err := hubManagedProjectPath(slug)
require.NoError(t, err)
assert.Equal(t, grovesDir, path, "should fall back to groves path when projects dir only contains infrastructure dirs")
assert.Equal(t, grovesDir, path, "should default to groves path when neither dir has workspace content")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Add a unit test to verify that hubManagedProjectPath returns an error when an empty slug is provided, ensuring the defensive check works as expected.

	// When neither has content, should default to groves/
	path, err := hubManagedProjectPath(slug)
	require.NoError(t, err)
	assert.Equal(t, grovesDir, path, "should default to groves path when neither dir has workspace content")
}

func TestHubManagedProjectPath_EmptySlug(t *testing.T) {
	_, err := hubManagedProjectPath("")
	assert.Error(t, err)
	assert.Contains(t, err.Error(), "project slug cannot be empty")
}

Prevent hubManagedProjectPath from resolving to the parent directory
when called with an empty slug. Add unit test for this case.
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.

1 participant