fix: replace <PACK> placeholders, harden path traversal checks, strengthen empty-list tests#52
Conversation
<PACK> placeholders, harden path traversal checks, strengthen empty-list tests
…i-line) The single-line if chain in new() (added in this PR) exceeds ruff-format's preferred wrapping. Auto-applying `uv run ruff format` to clear the `lint` (ruff format --check) and `quality-gate` failures on PR #52. No semantic change. 381 tests still pass, mypy strict clean, both ruff check and ruff format clean.
|
/review |
|
Codex (@codex) review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a broken pack-validator agent script by replacing literal <PACK> placeholders, hardens CLI name validation against path traversal patterns, and strengthens a few tests to ensure no subprocesses run for empty hook/validation lists.
Changes:
- Update
.claude/agents/pack-validator.mdto iterate over changed packs and use$PACKconsistently; stop suppressing stderr for the primary pytest run. - Harden
nboot render(spec name → output dir) andnboot new(project name → output dir) by stripping whitespace and rejecting separators,.., and leading-dot names. - Improve tests by asserting
subprocess.runis not invoked for empty hooks/validations; add an assertion fortest_frameworkin async test detection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| .claude/agents/pack-validator.md | Fix pack placeholder usage by looping over changed packs and using $PACK. |
| src/navi_bootstrap/cli.py | Strengthen name/path validation for output directories derived from spec/project names. |
| tests/test_hooks.py | Ensure empty-hooks path does not spawn subprocesses. |
| tests/test_validate.py | Ensure empty-validations path does not spawn subprocesses. |
| tests/test_init.py | Add assertion to cover test_framework output from test detection. |
Comments suppressed due to low confidence (1)
src/navi_bootstrap/cli.py:496
stripped_nameis used foroutput_dir, but the unstrippednameis still passed intobuild_spec_for_new()(and influencesspec["name"]andpkg_dir). This can create a project directory like "myproj" while generating a spec/package path based on " myproj ". Consider normalizing once and usingstripped_nameconsistently for both the filesystem path and the generated spec.
stripped_name = name.strip() if name else ""
if (
not stripped_name
or "/" in stripped_name
or "\\" in stripped_name
or ".." in stripped_name
or stripped_name.startswith(".")
):
raise click.ClickException(
f"Unsafe project name {name!r}. "
"Names must not contain path separators, '..', or start with a dot."
)
output_dir = Path(stripped_name)
if output_dir.exists():
raise click.ClickException(
f"Directory {name!r} already exists. nboot new is for greenfield projects only."
)
# Build spec from CLI args
try:
spec_data = build_spec_for_new(
name,
description=description,
license_id=license_id,
python_version=python_version,
author_name=author,
)
| 3. Run the pack-specific test: | ||
|
|
||
| ```bash | ||
| pack_snake=$(echo <PACK> | tr '-' '_') | ||
| uv run pytest tests/test_${pack_snake}_pack.py -v 2>/dev/null || \ | ||
| pack_snake=$(echo "$PACK" | tr '-' '_') | ||
| uv run pytest tests/test_${pack_snake}_pack.py -v || \ | ||
| uv run pytest tests/ -k "$pack_snake" -v | ||
| ``` |
There was a problem hiding this comment.
Step 3 uses "$PACK" but, as written, it runs after the loop in step 2. In a shell this will only test the last pack (or fail if step 3 is run standalone). Consider moving the pack-specific test commands inside the same for PACK in ...; do ...; done loop, or adding a separate loop for step 3 so each changed pack gets tested deterministically.
| "Name must be a non-empty single path segment without '.' or '..' components. " | ||
| "Use --out to specify an explicit output path." |
There was a problem hiding this comment.
The error text says "without '.' or '..' components", but the actual validation is stripped_name.startswith('.') and ".." in stripped_name (substring), which also rejects names like ".foo" and "foo..bar". Update the message to match the exact rules being enforced (e.g., “must not start with '.' and must not contain '..'”), or adjust the checks to match the wording.
| "Name must be a non-empty single path segment without '.' or '..' components. " | |
| "Use --out to specify an explicit output path." | |
| "Name must be a non-empty single path segment, must not start with '.', " | |
| "and must not contain '..'. Use --out to specify an explicit output path." |
| if out is None: | ||
| name = spec_data["name"] | ||
| if not name or "/" in name or "\\" in name: | ||
| stripped_name = name.strip() if isinstance(name, str) else "" | ||
| if ( | ||
| not stripped_name | ||
| or "/" in stripped_name | ||
| or "\\" in stripped_name | ||
| or ".." in stripped_name | ||
| or stripped_name.startswith(".") | ||
| ): | ||
| raise click.ClickException( | ||
| f"Unsafe spec name {name!r} cannot be used as output directory. " | ||
| "Name must be a non-empty single path segment without '.' or '..' components. " | ||
| "Use --out to specify an explicit output path." | ||
| ) | ||
| output_dir = Path(name) | ||
| output_dir = Path(stripped_name) | ||
| else: |
There was a problem hiding this comment.
PR description mentions tightening spec-name validation for apply/diff, but the only spec-name-as-path validation appears under the render command (render_cmd). If apply/diff also need similar hardening, it looks missing; otherwise, the PR description should be updated to reference render instead.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eec6ef804
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| pack_snake=$(echo "$PACK" | tr '-' '_') | ||
| uv run pytest tests/test_${pack_snake}_pack.py -v || \ |
There was a problem hiding this comment.
When multiple packs are changed, step 2 loops over each PACK, but step 3 executes only once after done, so pack_snake is derived from the last loop value and only that pack’s test runs. This can let regressions in earlier changed packs pass unnoticed because their tests/test_<pack>_pack.py (or fallback -k) never executes.
Useful? React with 👍 / 👎.
1. [Codex P1 + Copilot] pack-validator.md: pack-specific pytest step
was outside the per-pack `for PACK ... done` loop, so $PACK held
only the LAST iteration's value and earlier packs' tests were
silently skipped. A regression in earlier-changed packs would slip
through the validator. Moved the test inside the same loop body
and renumbered the cross-cutting tests as step 3.
2. [Copilot] cli.py render_cmd error message: previous wording
('without dot or dot-dot components') didn't match the actual
checks (`startswith('.')` rejects '.foo'; `'..' in name`
substring rejects 'foo..bar'). Reworded to match the rules:
'must not start with .', 'must not contain .. or path separators'.
Skipped from the same review batch:
- Copilot's note that PR description claims apply/diff hardening:
only render and new construct Path-from-name; apply/diff take
--target as a Click Path. PR description should be updated but
code is correct.
381 tests still pass, mypy strict clean, ruff + format clean.
…ngthen empty-list tests Agent-Logs-Url: https://github.com/Project-Navi/navi-bootstrap/sessions/604f9871-000e-4f48-b728-1c2a85fe6c4c Co-authored-by: Fieldnote-Echo <202828230+Fieldnote-Echo@users.noreply.github.com>
…i-line) The single-line if chain in new() (added in this PR) exceeds ruff-format's preferred wrapping. Auto-applying `uv run ruff format` to clear the `lint` (ruff format --check) and `quality-gate` failures on PR #52. No semantic change. 381 tests still pass, mypy strict clean, both ruff check and ruff format clean.
1. [Codex P1 + Copilot] pack-validator.md: pack-specific pytest step
was outside the per-pack `for PACK ... done` loop, so $PACK held
only the LAST iteration's value and earlier packs' tests were
silently skipped. A regression in earlier-changed packs would slip
through the validator. Moved the test inside the same loop body
and renumbered the cross-cutting tests as step 3.
2. [Copilot] cli.py render_cmd error message: previous wording
('without dot or dot-dot components') didn't match the actual
checks (`startswith('.')` rejects '.foo'; `'..' in name`
substring rejects 'foo..bar'). Reworded to match the rules:
'must not start with .', 'must not contain .. or path separators'.
Skipped from the same review batch:
- Copilot's note that PR description claims apply/diff hardening:
only render and new construct Path-from-name; apply/diff take
--target as a Click Path. PR description should be updated but
code is correct.
381 tests still pass, mypy strict clean, ruff + format clean.
9465690 to
765a9a3
Compare
| stripped_name = name.strip() if name else "" | ||
| if ( | ||
| not stripped_name | ||
| or "/" in stripped_name | ||
| or "\\" in stripped_name | ||
| or ".." in stripped_name | ||
| or stripped_name.startswith(".") | ||
| ): | ||
| raise click.ClickException( | ||
| f"Unsafe project name {name!r}. " | ||
| "Names must not contain path separators or start with a dot." | ||
| "Names must not contain path separators, '..', or start with a dot." | ||
| ) | ||
| output_dir = Path(name) | ||
| output_dir = Path(stripped_name) |
There was a problem hiding this comment.
new() strips whitespace into stripped_name for output_dir, but later uses the original name when building the spec (build_spec_for_new(name, ...)). If the user passes leading/trailing whitespace, the directory created and the spec’s internal name (and derived paths like src_dir) can diverge, potentially creating unexpected paths in the generated project. Consider normalizing once (e.g., replace name with stripped_name) and using the normalized value consistently for both output_dir and the spec construction.
| raise click.ClickException( | ||
| f"Unsafe project name {name!r}. " | ||
| "Names must not contain path separators or start with a dot." | ||
| "Names must not contain path separators, '..', or start with a dot." |
There was a problem hiding this comment.
The new() validation rejects empty/whitespace-only names (not stripped_name), but the error message only mentions path separators/".."/dot prefix. Consider updating the message to also reflect the non-empty requirement (and that surrounding whitespace is stripped) so users get an accurate failure reason.
| "Names must not contain path separators, '..', or start with a dot." | |
| "Surrounding whitespace is stripped, and the resulting name must not be empty, " | |
| "contain path separators, contain '..', or start with a dot." |
| scratch=$(mktemp -d -t nboot-scratch-XXXX) | ||
| uv run nboot new "$scratch" |
There was a problem hiding this comment.
The validation snippet creates an absolute scratch path via mktemp -d ... and then runs nboot new "$scratch", but nboot new rejects names containing path separators (see src/navi_bootstrap/cli.py:new), so this command chain will fail as written. Adjust the instructions to create a scratch parent directory and run nboot new <project-name> inside it (or otherwise ensure the name argument is a single path segment), then point --target at the created project directory.
| scratch=$(mktemp -d -t nboot-scratch-XXXX) | |
| uv run nboot new "$scratch" | |
| scratch_parent=$(mktemp -d -t nboot-scratch-XXXX) | |
| scratch_name=scratch | |
| scratch="$scratch_parent/$scratch_name" | |
| ( | |
| cd "$scratch_parent" && | |
| uv run nboot new "$scratch_name" | |
| ) |
Several issues across the agent script, CLI validation, and tests: literal
<PACK>placeholders in the pack-validator agent rendered commands non-functional;..path components were not blocked in spec/project name validation; and empty-list tests for hooks/validations lacked subprocess mock assertions..claude/agents/pack-validator.mdfor PACK in $(git diff ...)loop —<PACK>was a literal string, making everynboot apply/diffcall invalidpack_snake=$(echo "$PACK" | ...)to use the loop variable9465690) so each changed pack is tested, not just the last one2>/dev/nullstderr suppression from the firstpytestinvocationsrc/navi_bootstrap/cli.pyrender_cmd(the only command that constructs an outputPathfromspec.namewhen--outis omitted): replace exact{".", ".."}membership check with".." in stripped_namesubstring check; addstartswith(".")guard; apply consistentname.strip()normalization.apply/diffare unchanged because they take--targetas a ClickPath, no name→path conversion.new(CLI arg becomes the outputPath): add".." in namesubstring check; applyname.strip()normalization —renderandnewnow use identical rules.9465690) to actually describe the rules being enforced: "must not start with '.', must not contain '..' or path separators".Tests
test_empty_hooks/test_empty_validations: add@patch+mock_run.assert_not_called()— previously the tests only checked the return value, not that no subprocess was spawnedtest_detects_async_test_functions: addassert result["test_framework"] == "pytest"to cover the previously untested fieldReview remediation log
3eec6ef—ruff formaton the new validation block to clear lint + quality-gate9465690— Codex P1 (pack tests outside loop) + Copilot (error wording mismatch)