fix: route help command through kernel registry instead of hardcoded list#58
Conversation
|
Nice work @ekowi. The in-memory DB and mocked APIs make the tests actually usable. Just two quick things: skip the emoji tests on Windows so CI doesn’t red-ball, and let’s merge this into Fix that and it's good to go! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemove local help interception so kernel help is used, normalize only the first CLI token to lowercase, skip flaky tests on Windows, and add a release-gated PyPI publish job in the python-ci workflow. ChangesHelp Command Delegation to Kernel
Windows Platform Test Skipping
CI Workflow PyPI Publishing Consolidation
Sequence DiagramsequenceDiagram
participant CLI as app_with_lower
participant Local as _handle_local_command
participant Kernel as get_kernel()/run_help
CLI->>Local: call with argv[1] lowercased
Local->>Kernel: returns "unhandled" for "help"
CLI->>Kernel: execute_command / run_help("") (kernel lists registry commands)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_data.py (1)
19-28:⚠️ Potential issue | 🟠 MajorFix Windows path handling in
run_csv_viewinstead of skipping the test
tests/test_data.pyskipstest_run_csv_view_*onwin32, buttrushell/commands/data.pyparses the argument withshlex.split(args or ""). With the defaultposix=Truebehavior, unquoted Windows paths likeC:\Users\temp\file.csvget mangled (backslashes stripped/treated as escapes), leading toPath(arguments[0])pointing to a non-existent path—so the skip is masking a real Windows failure.Update
run_csv_viewto treatargsas a single file path (e.g., strip surrounding quotes and pass directly toPath(...)), or useshlex.split(..., posix=False)only if tokenization is truly required. Then remove the Windows skip (or temporarily passfile_path.as_posix()in the tests to validate behavior while fixing production).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_data.py` around lines 19 - 28, The test skip hides a Windows-only bug in run_csv_view: the function currently does shlex.split(args or "") with default posix=True which mangles unquoted Windows paths; fix run_csv_view in trushell/commands/data.py by treating the incoming args as a single pathname (e.g., trim surrounding quotes and pass directly to Path) or, if you really need tokenization, call shlex.split(args or "", posix=False) so backslashes are preserved; update the code path that builds Path(arguments[0]) to use the corrected parsing logic (function run_csv_view) so Windows paths resolve correctly and then remove the platform skip in tests.
🧹 Nitpick comments (1)
tests/test_nav.py (1)
27-39: ⚡ Quick winConsider whether path normalization could avoid the skip.
The skip is reasonable for CI stability, but the underlying issue is that
str(first)andstr(second)(with Windows backslashes) may not match the paths as rendered in Rich's exported text due to escaping differences.If you want to enable the test on Windows in the future, consider normalizing paths before comparison (e.g., converting both to POSIX format or using a regex pattern that accepts both forward and back slashes).
💡 Potential future enhancement to enable Windows testing
def test_run_jump_multiple_matches(tmp_path, monkeypatch): first = tmp_path / "src" first.mkdir() second = tmp_path / "my_src" second.mkdir() (tmp_path / "docs").mkdir() monkeypatch.chdir(tmp_path) result = run_jump("src") assert "Multiple matches found. Please be more specific." in result - assert str(first) in result - assert str(second) in result + # Normalize paths for cross-platform comparison + assert first.as_posix() in result.replace("\\", "/") + assert second.as_posix() in result.replace("\\", "/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_nav.py` around lines 27 - 39, Normalize the paths produced by run_jump and the expected paths in the test_run_jump_multiple_matches assertion so Windows backslashes don't break the comparison: update the test to compare normalized forms (e.g., use first.as_posix()/second.as_posix() or apply .replace("\\", "/") to both the result and expected strings) or normalize the result from run_jump before asserting; reference the test name test_run_jump_multiple_matches and the run_jump call along with variables first and second when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/python-ci.yml:
- Around line 57-71: Replace floating action refs with pinned commit SHAs for
actions/checkout, actions/setup-python, and pypa/gh-action-pypi-publish used in
the Publish to PyPI job; specifically update the checkout step (currently using
actions/checkout@v4) to include persist-credentials: false and point to the
official v4 commit SHA, and pin actions/setup-python@v5 and
pypa/gh-action-pypi-publish@release/v1 to their respective commit SHAs so the
Build package and Publish to PyPI steps use immutable action references.
- Around line 45-49: The workflow currently never triggers releases, so update
the workflow triggers to include a release event that allows the pypi-publish
job to run; specifically add an on: release: types: [published] (or include
release alongside existing push/pull_request) so the job guarded by if:
github.event_name == 'release' && github.event.action == 'published' and the
pypi-publish job name "pypi-publish" will be executed when a release is
published.
---
Outside diff comments:
In `@tests/test_data.py`:
- Around line 19-28: The test skip hides a Windows-only bug in run_csv_view: the
function currently does shlex.split(args or "") with default posix=True which
mangles unquoted Windows paths; fix run_csv_view in trushell/commands/data.py by
treating the incoming args as a single pathname (e.g., trim surrounding quotes
and pass directly to Path) or, if you really need tokenization, call
shlex.split(args or "", posix=False) so backslashes are preserved; update the
code path that builds Path(arguments[0]) to use the corrected parsing logic
(function run_csv_view) so Windows paths resolve correctly and then remove the
platform skip in tests.
---
Nitpick comments:
In `@tests/test_nav.py`:
- Around line 27-39: Normalize the paths produced by run_jump and the expected
paths in the test_run_jump_multiple_matches assertion so Windows backslashes
don't break the comparison: update the test to compare normalized forms (e.g.,
use first.as_posix()/second.as_posix() or apply .replace("\\", "/") to both the
result and expected strings) or normalize the result from run_jump before
asserting; reference the test name test_run_jump_multiple_matches and the
run_jump call along with variables first and second when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 969dc8b0-e2fe-40c7-ac97-233059fd3810
📒 Files selected for processing (5)
.github/workflows/python-ci.yml.github/workflows/workflow.ymltests/test_data.pytests/test_nav.pytrushell/cli.py
💤 Files with no reviewable changes (1)
- .github/workflows/workflow.yml
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
|
|
||
| - name: Install build tools | ||
| run: python -m pip install --upgrade build | ||
|
|
||
| - name: Build package | ||
| run: python -m build | ||
|
|
||
| - name: Publish to PyPI | ||
| uses: pypa/gh-action-pypi-publish@release/v1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/python-ci.yml"
echo "== File: $FILE =="
nl -ba "$FILE" | sed -n '1,140p'Repository: TruFoundation/TruShell
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/python-ci.yml"
echo "== File: $FILE =="
cat -n "$FILE" | sed -n '45,95p'Repository: TruFoundation/TruShell
Length of output: 968
🌐 Web query:
actions/checkout persist-credentials default value
💡 Result:
The default value for the persist-credentials input in the actions/checkout GitHub Action is true [1][2][3]. When set to true (the default), the action configures the authentication token or SSH key with the local git configuration [1][4]. This allows subsequent steps in your workflow to run authenticated git commands (such as git fetch or git push) automatically [2][5]. The action ensures security by removing these credentials during the post-job cleanup phase [2][5]. If you wish to disable this behavior, you can explicitly set persist-credentials to false [2][4].
Citations:
- 1: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 2: https://github.com/actions/checkout/?tab=readme-ov-file
- 3: https://github.com/actions/checkout/blob/main/action.yml
- 4: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 5: https://github.com/actions/checkout
Pin actions to commit SHAs and disable checkout credential persistence in the PyPI release publish job
- The publish job uses floating action refs (
actions/checkout@v4,actions/setup-python@v5,pypa/gh-action-pypi-publish@release/v1), weakening supply-chain guarantees. actions/checkout@v4is run withoutpersist-credentials: false(default istrue), so checkout auth credentials may persist beyond what the build/publish steps require.
Suggested hardening
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<full_sha_for_v4>
+ with:
+ persist-credentials: false
@@
- uses: actions/setup-python@v5
+ uses: actions/setup-python@<full_sha_for_v5>
@@
- uses: pypa/gh-action-pypi-publish@release/v1
+ uses: pypa/gh-action-pypi-publish@<full_sha_for_release_v1>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 57-57: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 57-57: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 60-60: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 71-71: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/python-ci.yml around lines 57 - 71, Replace floating
action refs with pinned commit SHAs for actions/checkout, actions/setup-python,
and pypa/gh-action-pypi-publish used in the Publish to PyPI job; specifically
update the checkout step (currently using actions/checkout@v4) to include
persist-credentials: false and point to the official v4 commit SHA, and pin
actions/setup-python@v5 and pypa/gh-action-pypi-publish@release/v1 to their
respective commit SHAs so the Build package and Publish to PyPI steps use
immutable action references.
Source: Linters/SAST tools
ekowi
force-pushed
the
fix/help-uses-kernel-registry
branch
from
fe510fb to
16d31f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/python-ci.yml (2)
57-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin actions to commit SHAs and disable credential persistence.
The publish job has supply-chain and credential-hygiene issues:
- Actions use floating refs (
@v4,@v5,@release/v1) instead of pinned commit SHAs.actions/checkoutdefaults topersist-credentials: true, leaving checkout credentials available to subsequent steps unnecessarily.🔒 Suggested hardening
- - uses: actions/checkout@v4 + - uses: actions/checkout@<commit_sha_for_v4> + with: + persist-credentials: false - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@<commit_sha_for_v5> with: python-version: '3.12' - name: Publish to PyPI - uses: pypa/gh-action-pypi-publish@release/v1 + uses: pypa/gh-action-pypi-publish@<commit_sha_for_release_v1>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-ci.yml around lines 57 - 71, Pin the GitHub Actions references and disable credential persistence: replace floating refs for actions/checkout, actions/setup-python and pypa/gh-action-pypi-publish with specific commit SHAs (use the latest verified SHAs) and add persist-credentials: false to the actions/checkout step to prevent checkout credentials leaking to subsequent steps; ensure any other action refs in this workflow are similarly pinned and that publish steps only run with least-privilege credentials.Source: Linters/SAST tools
3-8:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
releasetrigger so thepypi-publishjob can execute.The workflow only triggers on
pushandpull_requestevents. Thepypi-publishjob at line 49 is guarded byif: github.event_name == 'release' && github.event.action == 'published', so it will never run.🔧 Proposed fix
on: push: branches: [main] pull_request: branches: [main] + release: + types: [published]Then simplify the job condition:
- if: github.event_name == 'release' && github.event.action == 'published' + if: github.event_name == 'release'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-ci.yml around lines 3 - 8, The workflow only triggers on push/pull_request so the pypi-publish job never runs; update the workflow's top-level on: to include a release trigger (e.g., add release: with types: [published]) so GitHub will fire on published releases, then simplify the pypi-publish job's guard (the job named pypi-publish) to rely on github.event_name == 'release' (or remove the redundant action check) so the job executes when a release is published.
🧹 Nitpick comments (1)
trushell/cli.py (1)
37-37: 💤 Low valueConsider using spread syntax for cleaner list construction.
The list concatenation
[first] + restcan be simplified using spread syntax[first, *rest].♻️ Proposed refactor
- raw = " ".join([first] + rest) + raw = " ".join([first, *rest])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trushell/cli.py` at line 37, Replace the list concatenation used to build the raw command string in trushell/cli.py: instead of constructing the list with [first] + rest (where first and rest are the parsed command parts) use Python's unpacking syntax [first, *rest] to produce a cleaner, more idiomatic list before joining into raw; update the expression that sets the variable raw to build the list with unpacking and leave the surrounding join logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_data.py`:
- Around line 49-70: The test's skip reason is incorrect and its padding
assertion is fragile; update the skip reason for
test_run_csv_view_short_rows_are_padded to reflect that Windows path formatting
is not the reason (or remove the skip if irrelevant), and replace the loose
assert " " in output with a deterministic check that run_csv_view output
contains three columns per row (i.e., the short second row was padded to 3
cells) — for example, inspect the table row string(s) produced by run_csv_view
and assert the presence of an explicit empty/padded cell in the third column or
assert the expected number of column separators so the test directly verifies
padding implemented in data.run_csv_view (refer to run_csv_view and the padding
behavior in data.py lines ~61-64).
- Line 19: Update the skip reason on the pytest.mark.skipif in
tests/test_data.py to accurately describe the failure: explain that run_csv_view
uses shlex.split which mangles Windows backslash paths (e.g., C:\tmp\file ->
C:tmpfile), causing Path(...).exists() to fail and tests (like those asserting
rendered output in run_csv_view) to report "File '...' not found." rather than
the expected warning/table; reference run_csv_view and the related tests (e.g.,
test_run_csv_view_file_not_found / other run_csv_view assertions) in the reason
string so it's clear why the tests are skipped on Windows.
---
Duplicate comments:
In @.github/workflows/python-ci.yml:
- Around line 57-71: Pin the GitHub Actions references and disable credential
persistence: replace floating refs for actions/checkout, actions/setup-python
and pypa/gh-action-pypi-publish with specific commit SHAs (use the latest
verified SHAs) and add persist-credentials: false to the actions/checkout step
to prevent checkout credentials leaking to subsequent steps; ensure any other
action refs in this workflow are similarly pinned and that publish steps only
run with least-privilege credentials.
- Around line 3-8: The workflow only triggers on push/pull_request so the
pypi-publish job never runs; update the workflow's top-level on: to include a
release trigger (e.g., add release: with types: [published]) so GitHub will fire
on published releases, then simplify the pypi-publish job's guard (the job named
pypi-publish) to rely on github.event_name == 'release' (or remove the redundant
action check) so the job executes when a release is published.
---
Nitpick comments:
In `@trushell/cli.py`:
- Line 37: Replace the list concatenation used to build the raw command string
in trushell/cli.py: instead of constructing the list with [first] + rest (where
first and rest are the parsed command parts) use Python's unpacking syntax
[first, *rest] to produce a cleaner, more idiomatic list before joining into
raw; update the expression that sets the variable raw to build the list with
unpacking and leave the surrounding join logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3c87c56-2bed-4da5-8a88-90ec62424199
📒 Files selected for processing (6)
.github/workflows/python-ci.yml.github/workflows/workflow.ymltests/test_data.pytests/test_help_docs.pytests/test_nav.pytrushell/cli.py
💤 Files with no reviewable changes (1)
- .github/workflows/workflow.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_help_docs.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/python-ci.yml (2)
57-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin actions to commit SHAs and disable credential persistence.
The publish job has supply-chain and credential-hygiene issues:
- Actions use floating refs (
@v4,@v5,@release/v1) instead of pinned commit SHAs.actions/checkoutdefaults topersist-credentials: true, leaving checkout credentials available to subsequent steps unnecessarily.🔒 Suggested hardening
- - uses: actions/checkout@v4 + - uses: actions/checkout@<commit_sha_for_v4> + with: + persist-credentials: false - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@<commit_sha_for_v5> with: python-version: '3.12' - name: Publish to PyPI - uses: pypa/gh-action-pypi-publish@release/v1 + uses: pypa/gh-action-pypi-publish@<commit_sha_for_release_v1>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-ci.yml around lines 57 - 71, Pin the GitHub Actions references and disable credential persistence: replace floating refs for actions/checkout, actions/setup-python and pypa/gh-action-pypi-publish with specific commit SHAs (use the latest verified SHAs) and add persist-credentials: false to the actions/checkout step to prevent checkout credentials leaking to subsequent steps; ensure any other action refs in this workflow are similarly pinned and that publish steps only run with least-privilege credentials.Source: Linters/SAST tools
3-8:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
releasetrigger so thepypi-publishjob can execute.The workflow only triggers on
pushandpull_requestevents. Thepypi-publishjob at line 49 is guarded byif: github.event_name == 'release' && github.event.action == 'published', so it will never run.🔧 Proposed fix
on: push: branches: [main] pull_request: branches: [main] + release: + types: [published]Then simplify the job condition:
- if: github.event_name == 'release' && github.event.action == 'published' + if: github.event_name == 'release'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-ci.yml around lines 3 - 8, The workflow only triggers on push/pull_request so the pypi-publish job never runs; update the workflow's top-level on: to include a release trigger (e.g., add release: with types: [published]) so GitHub will fire on published releases, then simplify the pypi-publish job's guard (the job named pypi-publish) to rely on github.event_name == 'release' (or remove the redundant action check) so the job executes when a release is published.
🧹 Nitpick comments (1)
trushell/cli.py (1)
37-37: 💤 Low valueConsider using spread syntax for cleaner list construction.
The list concatenation
[first] + restcan be simplified using spread syntax[first, *rest].♻️ Proposed refactor
- raw = " ".join([first] + rest) + raw = " ".join([first, *rest])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trushell/cli.py` at line 37, Replace the list concatenation used to build the raw command string in trushell/cli.py: instead of constructing the list with [first] + rest (where first and rest are the parsed command parts) use Python's unpacking syntax [first, *rest] to produce a cleaner, more idiomatic list before joining into raw; update the expression that sets the variable raw to build the list with unpacking and leave the surrounding join logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_data.py`:
- Around line 49-70: The test's skip reason is incorrect and its padding
assertion is fragile; update the skip reason for
test_run_csv_view_short_rows_are_padded to reflect that Windows path formatting
is not the reason (or remove the skip if irrelevant), and replace the loose
assert " " in output with a deterministic check that run_csv_view output
contains three columns per row (i.e., the short second row was padded to 3
cells) — for example, inspect the table row string(s) produced by run_csv_view
and assert the presence of an explicit empty/padded cell in the third column or
assert the expected number of column separators so the test directly verifies
padding implemented in data.run_csv_view (refer to run_csv_view and the padding
behavior in data.py lines ~61-64).
- Line 19: Update the skip reason on the pytest.mark.skipif in
tests/test_data.py to accurately describe the failure: explain that run_csv_view
uses shlex.split which mangles Windows backslash paths (e.g., C:\tmp\file ->
C:tmpfile), causing Path(...).exists() to fail and tests (like those asserting
rendered output in run_csv_view) to report "File '...' not found." rather than
the expected warning/table; reference run_csv_view and the related tests (e.g.,
test_run_csv_view_file_not_found / other run_csv_view assertions) in the reason
string so it's clear why the tests are skipped on Windows.
---
Duplicate comments:
In @.github/workflows/python-ci.yml:
- Around line 57-71: Pin the GitHub Actions references and disable credential
persistence: replace floating refs for actions/checkout, actions/setup-python
and pypa/gh-action-pypi-publish with specific commit SHAs (use the latest
verified SHAs) and add persist-credentials: false to the actions/checkout step
to prevent checkout credentials leaking to subsequent steps; ensure any other
action refs in this workflow are similarly pinned and that publish steps only
run with least-privilege credentials.
- Around line 3-8: The workflow only triggers on push/pull_request so the
pypi-publish job never runs; update the workflow's top-level on: to include a
release trigger (e.g., add release: with types: [published]) so GitHub will fire
on published releases, then simplify the pypi-publish job's guard (the job named
pypi-publish) to rely on github.event_name == 'release' (or remove the redundant
action check) so the job executes when a release is published.
---
Nitpick comments:
In `@trushell/cli.py`:
- Line 37: Replace the list concatenation used to build the raw command string
in trushell/cli.py: instead of constructing the list with [first] + rest (where
first and rest are the parsed command parts) use Python's unpacking syntax
[first, *rest] to produce a cleaner, more idiomatic list before joining into
raw; update the expression that sets the variable raw to build the list with
unpacking and leave the surrounding join logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3c87c56-2bed-4da5-8a88-90ec62424199
📒 Files selected for processing (6)
.github/workflows/python-ci.yml.github/workflows/workflow.ymltests/test_data.pytests/test_help_docs.pytests/test_nav.pytrushell/cli.py
💤 Files with no reviewable changes (1)
- .github/workflows/workflow.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_help_docs.py
🛑 Comments failed to post (2)
tests/test_data.py (2)
19-19:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check if there are CI logs or issues documenting the Windows failures for these specific tests. # Search for Windows-related test failure documentation rg -i -C 3 "test_run_csv_view.*windows|windows.*test_run_csv_view" --type md # Check for any existing CI configuration mentioning Windows fd -e yml -e yaml . .github/workflows --exec rg -l "windows|win32" {}Repository: TruFoundation/TruShell
Length of output: 48
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the test file around the skipped tests echo "=== tests/test_data.py (lines 1-120) ===" nl -ba tests/test_data.py | sed -n '1,120p' echo echo "=== Find skip reason string occurrences ===" rg -n "Windows path formatting breaks tmp_path assertions" -S . || true echo echo "=== Find win32 usage ===" rg -n "win32" tests -S || true rg -n "sys\.platform\s*==\s*[\"']win32[\"']" -S . || true echo echo "=== Look for tmp_path assertions in these tests ===" rg -n "tmp_path" tests/test_data.py -n || true rg -n "assert.*path|path.*assert|os\.path|Path\(" tests/test_data.py -S || true echo echo "=== Check GitHub workflows for windows runners ===" fd -e yml -e yaml . .github/workflows | head -n 50 fd -e yml -e yaml . .github/workflows --exec rg -n "windows|win32" {} || trueRepository: TruFoundation/TruShell
Length of output: 168
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== tests/test_data.py (lines 1-120) ===" cat -n tests/test_data.py | sed -n '1,120p' echo echo "=== Find skip reason string occurrences ===" rg -n "Windows path formatting breaks tmp_path assertions" -S . || true echo echo "=== Find win32 usage ===" rg -n "win32" tests -S || true rg -n 'sys\.platform\s*==\s*["'\'']win32["'\'']' -S . || true echo echo "=== Look for tmp_path/path assertions in tests/test_data.py ===" rg -n "tmp_path" tests/test_data.py -S || true rg -n "assert.*path|path.*assert|os\.path|Path\(" tests/test_data.py -S || true echo echo "=== Check GitHub workflows for windows runners ===" fd -e yml -e yaml . .github/workflows | head -n 100 fd -e yml -e yaml . .github/workflows --exec rg -n "windows|win32" {} || trueRepository: TruFoundation/TruShell
Length of output: 5586
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Locate run_csv_view implementation ===" fd -t f "data.py" trushell tests || true # Find function definition rg -n "def run_csv_view" -S trushell || true rg -n "run_csv_view\(" -S trushell | head -n 50 echo echo "=== Show surrounding code for run_csv_view ===" # Pick the most likely file discovered above; fallback to a search for 'run_csv_view' file(s) files=$(rg -l "def run_csv_view" -S trushell || true) echo "Files: $files" for f in $files; do echo "--- $f ---" # show function body area (first ~120 lines from match) awk 'NR>=FNR{ }' /dev/null # Use sed around the first match line ln=$(rg -n "def run_csv_view" -S "$f" | head -n1 | cut -d: -f1) start=$((ln-5)); end=$((ln+160)) sed -n "${start},${end}p" "$f" done echo echo "=== Open .github/workflows/python-ci.yml ===" if [ -f .github/workflows/python-ci.yml ]; then sed -n '1,220p' .github/workflows/python-ci.yml | cat -n else echo "python-ci.yml not found" fi echo echo "=== Search for windows in workflow ===" rg -n "windows|win32" .github/workflows/python-ci.yml || trueRepository: TruFoundation/TruShell
Length of output: 5052
🏁 Script executed:
python3 - <<'PY' import shlex samples = [ r"C:\tmp\empty.csv", r"C:\Users\me\test\users.csv", r"\\server\share\file.csv", r"C:\t\u\empty.csv", ] for s in samples: print("INPUT:", s) try: print("SPLIT:", shlex.split(s)) except Exception as e: print("ERROR:", e) print("-"*40) PYRepository: TruFoundation/TruShell
Length of output: 461
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n tests/test_nav.py | sed -n '1,120p'Repository: TruFoundation/TruShell
Length of output: 1783
Fix the Windows skip reason text in
tests/test_data.py
- The skipped tests don’t assert on
tmp_pathformatting; they only assert onrun_csv_view’s rendered output (e.g., “Warning: File is empty.” / headers/rows).- The likely Windows failure is that
run_csv_view()callsshlex.split(args), which mangles Windows backslash paths (e.g.,C:\tmp\empty.csvbecomesC:tmpempty.csv), soPath(...).exists()fails and the output becomes “File '...' not found.” instead of the expected warning/table.test_run_csv_view_file_not_foundisn’t skipped because it passes a simple relative filename (missing_file.csv) without Windows separators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_data.py` at line 19, Update the skip reason on the pytest.mark.skipif in tests/test_data.py to accurately describe the failure: explain that run_csv_view uses shlex.split which mangles Windows backslash paths (e.g., C:\tmp\file -> C:tmpfile), causing Path(...).exists() to fail and tests (like those asserting rendered output in run_csv_view) to report "File '...' not found." rather than the expected warning/table; reference run_csv_view and the related tests (e.g., test_run_csv_view_file_not_found / other run_csv_view assertions) in the reason string so it's clear why the tests are skipped on Windows.
49-70:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify the skip reason for this test as well.
Like the other skipped tests,
test_run_csv_view_short_rows_are_paddeddoes not assert on path formatting—it asserts on CSV content (A, B, C, 1, 3) and whitespace to verify padding. The skip reason "Windows path formatting breaks tmp_path assertions" does not match the actual test logic.Additionally, the assertion
assert " " in output(Line 70) is somewhat fragile because Rich table output naturally contains whitespace for formatting. It doesn't specifically verify that the short row (row 2 with only 2 cells) was padded to 3 cells. However, given that the upstream contract (data.py:61-64) guarantees padding by extending short rows with empty strings, the test should pass if the function works correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_data.py` around lines 49 - 70, The test's skip reason is incorrect and its padding assertion is fragile; update the skip reason for test_run_csv_view_short_rows_are_padded to reflect that Windows path formatting is not the reason (or remove the skip if irrelevant), and replace the loose assert " " in output with a deterministic check that run_csv_view output contains three columns per row (i.e., the short second row was padded to 3 cells) — for example, inspect the table row string(s) produced by run_csv_view and assert the presence of an explicit empty/padded cell in the third column or assert the expected number of column separators so the test directly verifies padding implemented in data.run_csv_view (refer to run_csv_view and the padding behavior in data.py lines ~61-64).
Summary of ChangesCore Fix (Issue #49)Removed the hardcoded Tests
Maintainer Feedback
Out of Scope (noted for follow-up)
|
|
Great work @ekowi! |
2 participants
What
Removes the hardcoded
helphandler in_handle_local_command()so thehelpcommand is dispatched by TruKernel torun_help(), which readsthe live registry dynamically.
Why
The old handler returned a static string that omitted plugin-registered
commands (e.g.
gstatus) and would go stale as new commands were added.Changes
trushell/cli.py: removed hardcoded help block from_handle_local_commandtests/test_help_docs.py: added 2 new tests + fixed 1 pre-existing broken testTests
All 3 tests in
test_help_docs.pypass. No regressions introduced.Closes #49
Summary by CodeRabbit
Bug Fixes
Tests
Chores