fix(bmad-init): correctly resolve output_folder paths outside project root
When output_folder was set to an absolute path (e.g. /Users/me/outputs),
the {project-root}/{value} result template stored it as
{project-root}//absolute/path. resolve_project_root_placeholder then did
a naive string replace, producing /project//absolute/path — a broken path
that workflows could not resolve.
For relative paths outside the root (e.g. ../../sibling), the same naive
replace left un-normalized paths like /project/../../sibling in the
resolved config, which some tools mishandled.
Fix resolve_project_root_placeholder to strip the {project-root} token,
detect whether the remainder is absolute (returning it directly) or
relative (joining with project root and normalizing via os.path.normpath).
Fix apply_result_template to skip the template entirely when raw_value is
already an absolute path, and to normalize the result for relative-but-
outside paths. This covers the bmad-init SKILL write path, which bakes
the resolved path directly into config.yaml.
Add 7 tests covering all three path cases (absolute, relative-with-
traversal, normal in-project) for both functions.
This commit is contained in:
parent
819d373e2e
commit
5e7d0bc178
|
|
@ -166,9 +166,27 @@ def resolve_project_root_placeholder(value, project_root):
|
||||||
"""Replace {project-root} placeholder with actual path."""
|
"""Replace {project-root} placeholder with actual path."""
|
||||||
if not value or not isinstance(value, str):
|
if not value or not isinstance(value, str):
|
||||||
return value
|
return value
|
||||||
if '{project-root}' in value:
|
if '{project-root}' not in value:
|
||||||
return value.replace('{project-root}', str(project_root))
|
return value
|
||||||
return value
|
|
||||||
|
# Strip the {project-root} token to inspect what remains, so we can
|
||||||
|
# correctly handle absolute paths stored as "{project-root}//absolute/path"
|
||||||
|
# (produced by the "{project-root}/{value}" template applied to an absolute value).
|
||||||
|
suffix = value.replace('{project-root}', '', 1)
|
||||||
|
|
||||||
|
# Strip the one path separator that follows the token (if any)
|
||||||
|
if suffix.startswith('/') or suffix.startswith('\\'):
|
||||||
|
remainder = suffix[1:]
|
||||||
|
else:
|
||||||
|
remainder = suffix
|
||||||
|
|
||||||
|
if os.path.isabs(remainder):
|
||||||
|
# The original value was an absolute path stored with a {project-root}/ prefix.
|
||||||
|
# Return the absolute path directly — no joining needed.
|
||||||
|
return remainder
|
||||||
|
|
||||||
|
# Relative path: join with project root and normalize to resolve any .. segments.
|
||||||
|
return os.path.normpath(os.path.join(str(project_root), remainder))
|
||||||
|
|
||||||
|
|
||||||
def parse_var_specs(vars_string):
|
def parse_var_specs(vars_string):
|
||||||
|
|
@ -222,9 +240,22 @@ def apply_result_template(var_def, raw_value, context):
|
||||||
if not result_template:
|
if not result_template:
|
||||||
return raw_value
|
return raw_value
|
||||||
|
|
||||||
|
# If the user supplied an absolute path and the template would prefix it with
|
||||||
|
# "{project-root}/", skip the template entirely to avoid producing a broken path
|
||||||
|
# like "/my/project//absolute/path".
|
||||||
|
if isinstance(raw_value, str) and os.path.isabs(raw_value):
|
||||||
|
return raw_value
|
||||||
|
|
||||||
ctx = dict(context)
|
ctx = dict(context)
|
||||||
ctx['value'] = raw_value
|
ctx['value'] = raw_value
|
||||||
return expand_template(result_template, ctx)
|
result = expand_template(result_template, ctx)
|
||||||
|
|
||||||
|
# Normalize the resulting path to resolve any ".." segments (e.g. when the user
|
||||||
|
# entered a relative path such as "../../outside-dir").
|
||||||
|
if isinstance(result, str) and '{' not in result and os.path.isabs(result):
|
||||||
|
result = os.path.normpath(result)
|
||||||
|
|
||||||
|
return result
|
||||||
|
|
||||||
|
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
|
|
|
||||||
|
|
@ -110,6 +110,37 @@ class TestResolveProjectRootPlaceholder(unittest.TestCase):
|
||||||
def test_non_string(self):
|
def test_non_string(self):
|
||||||
self.assertEqual(resolve_project_root_placeholder(42, Path('/test')), 42)
|
self.assertEqual(resolve_project_root_placeholder(42, Path('/test')), 42)
|
||||||
|
|
||||||
|
def test_absolute_path_stored_with_prefix(self):
|
||||||
|
"""Absolute output_folder entered by user is stored as '{project-root}//abs/path'
|
||||||
|
by the '{project-root}/{value}' template. It must resolve to '/abs/path', not
|
||||||
|
'/project//abs/path'."""
|
||||||
|
result = resolve_project_root_placeholder(
|
||||||
|
'{project-root}//Users/me/outside', Path('/Users/me/myproject')
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/Users/me/outside')
|
||||||
|
|
||||||
|
def test_relative_path_with_traversal_is_normalized(self):
|
||||||
|
"""A relative path like '../../sibling' produces '{project-root}/../../sibling'
|
||||||
|
after the template. It must resolve to the normalized absolute path, not the
|
||||||
|
un-normalized string '/project/../../sibling'."""
|
||||||
|
result = resolve_project_root_placeholder(
|
||||||
|
'{project-root}/../../sibling', Path('/Users/me/myproject')
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/Users/sibling')
|
||||||
|
|
||||||
|
def test_relative_path_one_level_up(self):
|
||||||
|
result = resolve_project_root_placeholder(
|
||||||
|
'{project-root}/../outside-outputs', Path('/project/root')
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/project/outside-outputs')
|
||||||
|
|
||||||
|
def test_standard_relative_path_unchanged(self):
|
||||||
|
"""Normal in-project relative paths continue to work correctly."""
|
||||||
|
result = resolve_project_root_placeholder(
|
||||||
|
'{project-root}/_bmad-output', Path('/project/root')
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/project/root/_bmad-output')
|
||||||
|
|
||||||
|
|
||||||
class TestExpandTemplate(unittest.TestCase):
|
class TestExpandTemplate(unittest.TestCase):
|
||||||
|
|
||||||
|
|
@ -147,6 +178,39 @@ class TestApplyResultTemplate(unittest.TestCase):
|
||||||
result = apply_result_template(var_def, 'English', {})
|
result = apply_result_template(var_def, 'English', {})
|
||||||
self.assertEqual(result, 'English')
|
self.assertEqual(result, 'English')
|
||||||
|
|
||||||
|
def test_absolute_value_skips_project_root_template(self):
|
||||||
|
"""When the user enters an absolute path, the '{project-root}/{value}' template
|
||||||
|
must not be applied — doing so would produce '/project//absolute/path'."""
|
||||||
|
var_def = {'result': '{project-root}/{value}'}
|
||||||
|
result = apply_result_template(
|
||||||
|
var_def, '/Users/me/shared-outputs', {'project-root': '/Users/me/myproject'}
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/Users/me/shared-outputs')
|
||||||
|
|
||||||
|
def test_relative_traversal_value_is_normalized(self):
|
||||||
|
"""A relative path like '../../outside' combined with the project-root template
|
||||||
|
must produce a clean normalized absolute path, not '/project/../../outside'."""
|
||||||
|
var_def = {'result': '{project-root}/{value}'}
|
||||||
|
result = apply_result_template(
|
||||||
|
var_def, '../../outside-dir', {'project-root': '/Users/me/myproject'}
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/Users/outside-dir')
|
||||||
|
|
||||||
|
def test_relative_one_level_up_is_normalized(self):
|
||||||
|
var_def = {'result': '{project-root}/{value}'}
|
||||||
|
result = apply_result_template(
|
||||||
|
var_def, '../sibling-outputs', {'project-root': '/project/root'}
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/project/sibling-outputs')
|
||||||
|
|
||||||
|
def test_normal_relative_value_unchanged(self):
|
||||||
|
"""Standard in-project relative paths still produce the expected joined path."""
|
||||||
|
var_def = {'result': '{project-root}/{value}'}
|
||||||
|
result = apply_result_template(
|
||||||
|
var_def, '_bmad-output', {'project-root': '/project/root'}
|
||||||
|
)
|
||||||
|
self.assertEqual(result, '/project/root/_bmad-output')
|
||||||
|
|
||||||
|
|
||||||
class TestLoadModuleYaml(unittest.TestCase):
|
class TestLoadModuleYaml(unittest.TestCase):
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue