feat: AXI-compliant output with TOON format, hooks, and content-first home#33
Merged
feat: AXI-compliant output with TOON format, hooks, and content-first home#33
Conversation
…irst home Refactor gcpath from Rich-table-first output to AXI-compliant TOON-first output following the AXI specification (https://axi.md/). New capabilities: - TOON format as default output (token-efficient, structured for AI agents) - `--format toon|json|yaml|rich` flag replaces old `--json`/`--yaml` flags - `--fields` flag for controlling output columns (replaces `--long`) - `--full` flag to expand truncated labels/tags - Content-first home view: `gcpath` with no args shows live dashboard - Pre-computed aggregates: `count: N of M total` on list outputs - Contextual `help[]` sections with next-step suggestions - Structured errors to stdout in TOON format (no more Rich stderr markup) - Ambient context hooks: `gcpath hook install` for Claude Code and Codex - `gcpath hook run` outputs compact session-start dashboard - Definitive empty states: `0 resources found` not empty output - All interactive prompts removed (no more `typer.confirm`) New files: - `src/gcpath/toon.py` — TOON encoder wrapper + AXI helpers - `src/gcpath/hooks.py` — Claude Code / Codex session hook management - `tests/test_hooks.py` — Hook management tests Design decisions: - `tree` keeps classic unicode tree output (not TOON) — agents use `ls -R` - `diagram` keeps raw Mermaid/D2 output with `--diagram-format` flag - `toon-format` library (git dep) handles TOON encoding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
- Coverage 87.29% 86.93% -0.36%
==========================================
Files 9 11 +2
Lines 1786 2265 +479
==========================================
+ Hits 1559 1969 +410
- Misses 227 296 +69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements AXI compliance for the gcpath CLI, introducing the TOON output format and adding several new commands including find, ancestors, stats, and diagram. It also introduces ambient context hooks for agent integration and refactors output serialization into dedicated modules. Review feedback suggests improving error handling by catching specific exceptions, enhancing type safety with more precise return hints, and restoring removed docstrings to maintain code documentation.
Restructure the README to lead with gcpath's agent-native qualities: read-only safety, AXI-compliant TOON output, ambient context hooks, and Agent Skill integration. Add output format comparison table and dedicated Agent Integration section with hook setup instructions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR makes gcpath agent-native and read-only: introduces TOON as the default output format with a unified Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "gcpath CLI"
participant Hooks as "src/gcpath/hooks.py"
participant FS as "Config File (claude/codex JSON)"
participant Cache as "gcpath.cache"
participant Toon as "src/gcpath/toon.py"
User->>CLI: run `gcpath hook install`
CLI->>Hooks: install_hooks()
Hooks->>FS: read/write JSON settings (atomic)
FS-->>Hooks: confirm changes
Hooks-->>CLI: return install status
CLI-->>User: print status (TOON/Rich)
User->>CLI: run `gcpath hook run`
CLI->>Hooks: run_session_start()
Hooks->>Cache: get_cache_info(), read_cache_raw() if fresh
Cache-->>Hooks: cache metadata / org rows
Hooks->>Toon: build TOON payload (toon_dashboard / toon_table)
Toon-->>Hooks: encoded payload
Hooks-->>CLI: stdout payload
CLI-->>User: display dashboard (TOON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Restore docstring on _resolve_scope in cli.py - Narrow except Exception to specific GCP/gcpath exceptions in path command - Fix return type hint on _truncate_metadata (Any -> Dict[str, str]) - Add docstring on serialize_resource - Replace /tmp paths in test_serializers.py to resolve SonarCloud S5443 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tests/test_cli.py (1)
623-627: Tighten or remove this smoke test.This assertion passes for almost any non-usage failure, so it will not catch a broken JSON format path.
test_ls_json_outputbelow already covers the real behavior with a mocked hierarchy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 623 - 627, Remove the weak smoke test test_format_json_output entirely (the behavior is already covered by test_ls_json_output with mocks); delete the test function that calls runner.invoke(app, ["--format", "json", "ls"]) and its assertion so the suite no longer contains this redundant assertion that "--format" not in result.output or result.exit_code != 2.src/gcpath/cli.py (6)
260-277: Remove unusedctxparameter.The
ctxparameter is never used in_show_home. The function always outputs TOON format (as per AXI Principle 8 for content-first home view) and doesn't need to checkctx.obj["output_format"].♻️ Proposed fix
-def _show_home(ctx: typer.Context) -> None: +def _show_home() -> None: """Content-first home view (AXI Principle 8)."""And update the call site at line 257:
if ctx.invoked_subcommand is None: - _show_home(ctx) + _show_home()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 260 - 277, Remove the unused ctx parameter from _show_home: update the function signature to take no ctx, delete any unused references, and adjust the caller that currently passes a Context (the call at the original call site near where _show_home is invoked) to call _show_home() without arguments; ensure the function still uses _display_bin_path, get_cache_info, toon_encode, and typer.Exit as before.
38-56: Consider making_ALL_LS_FIELDSa public constant.
_ALL_LS_FIELDSis imported from serializers with a leading underscore (indicating it's private), but it's used in CLI for field validation. Consider renaming it toALL_LS_FIELDSin serializers.py to indicate it's part of the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 38 - 56, The CLI imports and uses the private symbol _ALL_LS_FIELDS from the serializers module; make it a public constant by renaming _ALL_LS_FIELDS to ALL_LS_FIELDS in the serializers module (and export it if the module defines __all__), then update the import in cli.py to import ALL_LS_FIELDS and replace all usages of _ALL_LS_FIELDS with ALL_LS_FIELDS (search for symbol occurrences across the codebase and update tests or docs that reference it).
773-780: Remove unusedtarget_resource_nameparameter.The
target_resource_nameparameter is never used in the function body. Either remove it or add logic that uses it for contextual help.♻️ Proposed fix
-def _ls_help_lines(recursive: bool, target_resource_name: Optional[str]) -> List[str]: +def _ls_help_lines(recursive: bool) -> List[str]: lines = [] if not recursive: lines.append("Run `gcpath ls <resource> -R` for nested listing") else: lines.append("Run `gcpath ls <resource>` for direct children only") lines.append("Run `gcpath find <pattern>` to search by name") return linesAnd update the call site at line 942:
- help_lines = _ls_help_lines(recursive, target_resource_name) + help_lines = _ls_help_lines(recursive)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 773 - 780, The helper function _ls_help_lines defines an unused parameter target_resource_name; remove this unused parameter from the function signature (_ls_help_lines(recursive: bool)) and update all call sites that pass target_resource_name to instead call with a single boolean argument (search for _ls_help_lines(...) usages) so signatures match; alternatively, if contextual help is desired, use target_resource_name inside _ls_help_lines to customize the help strings—choose one approach and make consistent edits to the function and its callers (including the CLI help generation that currently invokes _ls_help_lines).
331-381: Refactor to reduce cognitive complexity.Static analysis flags this function with cognitive complexity 18 (limit is 15). The rich output branch has many conditional checks that could be extracted to a helper function.
♻️ Proposed refactor: extract rich table building
+def _build_cache_status_rich_table(info) -> None: + from rich.table import Table + from rich.console import Console + console = Console() + if not info.exists: + rprint(f"[yellow]No cache file found at {CACHE_FILE}[/yellow]") + return + table = Table(show_header=False, box=None, padding=(0, 1)) + table.add_column("Key", style="bold") + table.add_column("Value") + table.add_row("Status", "[green]Fresh[/green]" if info.fresh else "[yellow]Stale[/yellow]") + if info.age_seconds is not None: + table.add_row("Age", format_age(info.age_seconds)) + if info.size_bytes is not None: + size_kb = info.size_bytes / 1024 + size_str = f"{size_kb / 1024:.1f} MB" if size_kb >= 1024 else f"{size_kb:.1f} KB" + table.add_row("Size", size_str) + if info.version is not None: + table.add_row("Version", str(info.version)) + if info.scope is not None: + table.add_row("Scope", info.scope) + table.add_row("Organizations", str(info.org_count)) + table.add_row("Folders", str(info.folder_count)) + table.add_row("Projects", str(info.project_count)) + table.add_row("Location", str(CACHE_FILE)) + console.print(table) + + `@cache_app.command`("status") def cache_status(ctx: typer.Context) -> None: """Show cache status information.""" fmt = ctx.obj.get("output_format", "toon") info = get_cache_info() if fmt == "rich": - from rich.table import Table - ... + _build_cache_status_rich_table(info) return print(toon_cache_status(...))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 331 - 381, The cache_status function's "rich" branch is too complex—extract the table construction and printing into a helper (e.g., build_and_print_rich_cache_table or render_rich_cache_table) that accepts the cache info object (the result of get_cache_info()) and CACHE_FILE; move everything from "if fmt == 'rich':" up to the console.print(table) (including imports of rich.table.Table and rich.console.Console, creation of the Table, column definitions, all conditional table.add_row(...) calls, and the console.print) into that helper, then replace the original branch with a single call to that helper and return; ensure the helper handles info.exists early (rprint and return) and accepts/uses fields info.fresh, info.age_seconds, info.size_bytes, info.version, info.scope, info.org_count, info.folder_count, info.project_count and formats sizes/age exactly as before.
668-684: Consider privacy implications of displaying email.The code displays the user's email address (line 674) when it's a Gmail account. While this is helpful for debugging authentication issues, consider:
- This is displayed to stdout, not logged
- It only shows for Gmail accounts (consumer accounts)
This seems acceptable for CLI user feedback, but worth noting if there are any compliance requirements around displaying user identifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 668 - 684, The CLI currently reads google.auth.default() and prints the full Gmail address into account_msg (credentials, account_msg, rprint); to avoid exposing full email by default, change the logic that builds account_msg to either (a) mask the local-part (e.g. show first char + "...@domain") or (b) only show the domain (e.g. "(`@gmail.com`)"), and gate printing the full address behind an explicit opt-in (a CLI flag or env var like GCPATH_SHOW_ACCOUNT). Update the block that sets account_msg and the rprint calls to use the masked/domain-only string by default and only reveal the full credentials.account when the opt-in flag/env is set.
1468-1505: Clarify semantics oftotal_searchedcount.The
total_searchedvariable is set to the count of pattern matches before label/tag filtering is applied (line 1469), but the output says "N of M searched". This might be confusing since "searched" typically implies the search scope (all resources), not the pattern matches.Consider either:
- Renaming to
total_matcheswith output "N of M matches"- Or counting actual search scope and outputting "N matches of M searched"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 1468 - 1505, The variable total_searched currently holds len(all_matches) (pattern matches) which is misleadingly named; change its name to total_matches (replace total_searched) to reflect that it counts matches prior to label/tag filtering, and update the call to toon_find to pass total_matches (or rename the toon_find parameter if required) so the output reads "N of M matches" (or otherwise update any wording in toon_find that uses that value). Locate usages: total_searched, all_matches, items, and the final toon_find(...) call and make the rename/wording change consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 266-291: The README incorrectly documents non-existent shorthand
flags `--json` and `--yaml`; update the docs to remove the sentence claiming
those shorthands exist and replace any example usages that use `--json`/`--yaml`
with the canonical `--format json` and `--format yaml`; verify this aligns with
the CLI's `output_format` option which only defines `--format`, and change all
README examples that previously used `--json`/`--yaml` to use `--format
json`/`--format yaml` instead.
In `@src/gcpath/hooks.py`:
- Around line 58-63: The SessionStart hook matching is inconsistent: some places
use endswith(...) and others use substring checks which can misidentify
unrelated hooks; replace all ad-hoc match checks around hooks["SessionStart"]
(the blocks that compare entry.get("command", ""), including the instance using
endswith and the substring checks referenced at lines 84-88, 109-114, 128-132,
191-209) with a single exact matcher and centralize it into a helper (e.g.,
is_managed_hook(entry, command) or match_managed_command) that returns True only
when entry is a dict and entry.get("command") == command; then use that helper
in the loops to detect, update, or remove the managed hook and continue to call
_write_json(_CLAUDE_SETTINGS_PATH, data) only after a definitive change.
- Line 239: The current truthiness check treats a 0.0 cache age as missing;
update the logic that sets age_str so a zero age is considered valid by testing
for None (or using "is not None") instead of truthiness: call
format_age(info.age_seconds) when info.age_seconds is not None (or is not
Falsey) and only use "unknown" when info.age_seconds is actually None/absent;
reference the age_str assignment and the format_age and info.age_seconds symbols
to locate and change the condition.
- Around line 41-44: The _write_json function currently writes directly to the
target file which can leave a truncated/invalid file if the process dies
mid-write; change it to write to a temporary file in the same directory, flush
and fsync the temp file, close it, then atomically replace the target using
os.replace (or Path.replace) so the config is always either old or new. Update
the _write_json implementation (function name: _write_json) to create the parent
directory, open a NamedTemporaryFile (delete=False) in the same dir, json.dump
to it, call f.flush(); os.fsync(f.fileno()); close the temp file and then call
os.replace(temp_path, path) and ensure proper cleanup on exceptions.
In `@src/gcpath/toon.py`:
- Around line 11-15: Replace the import-time assert with raising the project's
GCPathError when the encode API is missing: check that toon_format.encode
(referenced as _toon_encode) is callable after importing and if not raise
GCPathError with a clear message (e.g., "toon_format missing encode — check
dependency version") instead of using assert; ensure the check runs at import
time deterministically (not behind optimizations) and use the GCPathError class
already used across the project for consistency.
---
Nitpick comments:
In `@src/gcpath/cli.py`:
- Around line 260-277: Remove the unused ctx parameter from _show_home: update
the function signature to take no ctx, delete any unused references, and adjust
the caller that currently passes a Context (the call at the original call site
near where _show_home is invoked) to call _show_home() without arguments; ensure
the function still uses _display_bin_path, get_cache_info, toon_encode, and
typer.Exit as before.
- Around line 38-56: The CLI imports and uses the private symbol _ALL_LS_FIELDS
from the serializers module; make it a public constant by renaming
_ALL_LS_FIELDS to ALL_LS_FIELDS in the serializers module (and export it if the
module defines __all__), then update the import in cli.py to import
ALL_LS_FIELDS and replace all usages of _ALL_LS_FIELDS with ALL_LS_FIELDS
(search for symbol occurrences across the codebase and update tests or docs that
reference it).
- Around line 773-780: The helper function _ls_help_lines defines an unused
parameter target_resource_name; remove this unused parameter from the function
signature (_ls_help_lines(recursive: bool)) and update all call sites that pass
target_resource_name to instead call with a single boolean argument (search for
_ls_help_lines(...) usages) so signatures match; alternatively, if contextual
help is desired, use target_resource_name inside _ls_help_lines to customize the
help strings—choose one approach and make consistent edits to the function and
its callers (including the CLI help generation that currently invokes
_ls_help_lines).
- Around line 331-381: The cache_status function's "rich" branch is too
complex—extract the table construction and printing into a helper (e.g.,
build_and_print_rich_cache_table or render_rich_cache_table) that accepts the
cache info object (the result of get_cache_info()) and CACHE_FILE; move
everything from "if fmt == 'rich':" up to the console.print(table) (including
imports of rich.table.Table and rich.console.Console, creation of the Table,
column definitions, all conditional table.add_row(...) calls, and the
console.print) into that helper, then replace the original branch with a single
call to that helper and return; ensure the helper handles info.exists early
(rprint and return) and accepts/uses fields info.fresh, info.age_seconds,
info.size_bytes, info.version, info.scope, info.org_count, info.folder_count,
info.project_count and formats sizes/age exactly as before.
- Around line 668-684: The CLI currently reads google.auth.default() and prints
the full Gmail address into account_msg (credentials, account_msg, rprint); to
avoid exposing full email by default, change the logic that builds account_msg
to either (a) mask the local-part (e.g. show first char + "...@domain") or (b)
only show the domain (e.g. "(`@gmail.com`)"), and gate printing the full address
behind an explicit opt-in (a CLI flag or env var like GCPATH_SHOW_ACCOUNT).
Update the block that sets account_msg and the rprint calls to use the
masked/domain-only string by default and only reveal the full
credentials.account when the opt-in flag/env is set.
- Around line 1468-1505: The variable total_searched currently holds
len(all_matches) (pattern matches) which is misleadingly named; change its name
to total_matches (replace total_searched) to reflect that it counts matches
prior to label/tag filtering, and update the call to toon_find to pass
total_matches (or rename the toon_find parameter if required) so the output
reads "N of M matches" (or otherwise update any wording in toon_find that uses
that value). Locate usages: total_searched, all_matches, items, and the final
toon_find(...) call and make the rename/wording change consistently.
In `@tests/test_cli.py`:
- Around line 623-627: Remove the weak smoke test test_format_json_output
entirely (the behavior is already covered by test_ls_json_output with mocks);
delete the test function that calls runner.invoke(app, ["--format", "json",
"ls"]) and its assertion so the suite no longer contains this redundant
assertion that "--format" not in result.output or result.exit_code != 2.
🪄 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
Run ID: e1fefc03-bc4f-4e29-8ae1-46e36ead557e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
CLAUDE.mdREADME.mdpyproject.tomlsrc/gcpath/cli.pysrc/gcpath/hooks.pysrc/gcpath/serializers.pysrc/gcpath/toon.pytests/test_cli.pytests/test_hooks.pytests/test_serializers.py
- Extract _cache_status_rich and _stats_rich to reduce cognitive complexity (S3776) - Extract _validate_stats_resource helper for stats command - Remove unused params: ctx in _show_home, level in _prepare_hierarchy_command, target_resource_name in _ls_help_lines (S1172) - Replace duplicated "gcpath hook run" literal with _GCPATH_HOOK_COMMAND constant (S1192) - Extract _check_hook_entries to reduce get_hook_status complexity (S3776) - Fix unused variables in test_serializers.py (S1481) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/gcpath/hooks.py (3)
235-235:⚠️ Potential issue | 🟡 MinorTreat
age_seconds == 0.0as valid age.Line 235 uses a truthiness check, so a fresh cache with
0.0age is shown asunknown.Suggested fix
- age_str = format_age(info.age_seconds) if info.age_seconds else "unknown" + age_str = format_age(info.age_seconds) if info.age_seconds is not None else "unknown"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/hooks.py` at line 235, The code treats info.age_seconds with a truthiness check causing 0.0 to be considered "unknown"; update the assignment to explicitly check for None (or use is not None) so that 0.0 is passed to format_age; locate the expression using info.age_seconds and replace the truthy test with an explicit None check so format_age(info.age_seconds) is called for zero values and only "unknown" used when info.age_seconds is None.
58-63:⚠️ Potential issue | 🟠 MajorUse one centralized managed-hook matcher across install/uninstall/status.
Current logic mixes
endswith(...)and substring checks (in ...), which can remove/report unrelated hooks that merely mentiongcpath hook run(e.g., Line 87/131/185).Suggested fix
+def _is_managed_hook(command_value: str) -> bool: + return command_value.strip().endswith(_GCPATH_HOOK_COMMAND) @@ - if isinstance(entry, dict) and entry.get("command", "").endswith(_GCPATH_HOOK_COMMAND): + if isinstance(entry, dict) and _is_managed_hook(entry.get("command", "")): @@ - if not (isinstance(entry, dict) and _GCPATH_HOOK_COMMAND in entry.get("command", "")) + if not (isinstance(entry, dict) and _is_managed_hook(entry.get("command", ""))) @@ - if isinstance(entry, dict) and entry.get("command", "").endswith(_GCPATH_HOOK_COMMAND): + if isinstance(entry, dict) and _is_managed_hook(entry.get("command", "")): @@ - if not (isinstance(entry, dict) and _GCPATH_HOOK_COMMAND in entry.get("command", "")) + if not (isinstance(entry, dict) and _is_managed_hook(entry.get("command", ""))) @@ - if isinstance(entry, dict) and _GCPATH_HOOK_COMMAND in entry.get("command", ""): + if isinstance(entry, dict) and _is_managed_hook(entry.get("command", "")): return True, entry.get("command") == commandAlso applies to: 84-88, 109-114, 128-132, 182-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/hooks.py` around lines 58 - 63, The loop in SessionStart uses inconsistent matching (endswith and substring checks) and can accidentally match unrelated hooks; add a single helper (e.g., is_managed_hook(entry, command) or matches_gcpath_hook(entry)) and use it everywhere (install/uninstall/status) instead of ad-hoc endswith/in checks; the helper should check that entry is a dict, that entry.get("command") exactly equals the canonical managed command or is the canonical command with only allowed args (do not use simple substring matches), and update calls in the SessionStart loop (where entry.get("command") == command and entry["command"] = command and _write_json), plus the other locations mentioned (lines handling hooks at install/uninstall/status), to call the new helper for detection and replacement.
41-44:⚠️ Potential issue | 🟠 MajorMake hook config writes atomic.
Line 43 truncates and rewrites in place. If the process is interrupted, settings can be left corrupted.
Suggested fix
+import os +import tempfile @@ def _write_json(path: Path, data: Dict[str, Any]) -> None: path.parent.mkdir(parents=True, exist_ok=True) - with open(path, "w") as f: - json.dump(data, f, indent=2) + tmp_fd, tmp_name = tempfile.mkstemp(dir=str(path.parent), prefix=path.name, suffix=".tmp") + try: + with os.fdopen(tmp_fd, "w") as f: + json.dump(data, f, indent=2) + f.flush() + os.fsync(f.fileno()) + Path(tmp_name).replace(path) + finally: + tmp_path = Path(tmp_name) + if tmp_path.exists(): + tmp_path.unlink(missing_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/hooks.py` around lines 41 - 44, The current _write_json function truncates the target file in place which can leave a corrupted file if interrupted; change it to write atomically by writing JSON to a temporary file in the same directory (use tempfile.NamedTemporaryFile or write to path.parent / (path.name + ".tmp") with delete=False), flush and os.fsync the file descriptor after json.dump, close it, then atomically replace the target with os.replace(tmp_path, path); finally fsync the target directory (open path.parent with os.open(..., os.O_DIRECTORY) and os.fsync) and close that descriptor to ensure directory metadata is persisted. Keep the function name _write_json and imports for tempfile, os, and ensure exception-safe cleanup of the temp file on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gcpath/hooks.py`:
- Around line 30-38: The _read_json function may return non-dict JSON
(list/scalar), causing callers that expect a dict to crash; update _read_json to
validate the decoded value with isinstance(obj, dict) and if it's not a dict,
log a warning including the path and type and return None instead of returning
the raw value so downstream code (callers of _read_json) always either gets a
dict or None.
In `@src/gcpath/serializers.py`:
- Around line 125-136: The empty-path returns toon_empty("resources", "",
help_lines) which omits the "count" header; instead preserve the same "count"
field as in the non-empty path by constructing an empty data dict like data =
{"count": f"{len(items)} of {total_in_scope} total", "resources": []}, encoding
it with toon_encode and wrapping with with_help before returning; update the
branch that currently calls toon_empty(...) (referencing items, total_in_scope,
toon_empty, toon_encode, with_help) so empty ls responses include the count
header.
---
Duplicate comments:
In `@src/gcpath/hooks.py`:
- Line 235: The code treats info.age_seconds with a truthiness check causing 0.0
to be considered "unknown"; update the assignment to explicitly check for None
(or use is not None) so that 0.0 is passed to format_age; locate the expression
using info.age_seconds and replace the truthy test with an explicit None check
so format_age(info.age_seconds) is called for zero values and only "unknown"
used when info.age_seconds is None.
- Around line 58-63: The loop in SessionStart uses inconsistent matching
(endswith and substring checks) and can accidentally match unrelated hooks; add
a single helper (e.g., is_managed_hook(entry, command) or
matches_gcpath_hook(entry)) and use it everywhere (install/uninstall/status)
instead of ad-hoc endswith/in checks; the helper should check that entry is a
dict, that entry.get("command") exactly equals the canonical managed command or
is the canonical command with only allowed args (do not use simple substring
matches), and update calls in the SessionStart loop (where entry.get("command")
== command and entry["command"] = command and _write_json), plus the other
locations mentioned (lines handling hooks at install/uninstall/status), to call
the new helper for detection and replacement.
- Around line 41-44: The current _write_json function truncates the target file
in place which can leave a corrupted file if interrupted; change it to write
atomically by writing JSON to a temporary file in the same directory (use
tempfile.NamedTemporaryFile or write to path.parent / (path.name + ".tmp") with
delete=False), flush and os.fsync the file descriptor after json.dump, close it,
then atomically replace the target with os.replace(tmp_path, path); finally
fsync the target directory (open path.parent with os.open(..., os.O_DIRECTORY)
and os.fsync) and close that descriptor to ensure directory metadata is
persisted. Keep the function name _write_json and imports for tempfile, os, and
ensure exception-safe cleanup of the temp file on failure.
🪄 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
Run ID: dd675503-4b87-4350-a11c-fe6f0d3825e1
📒 Files selected for processing (4)
src/gcpath/cli.pysrc/gcpath/hooks.pysrc/gcpath/serializers.pytests/test_serializers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/gcpath/cli.py
- Add custom CodeQL workflow with config that excludes test paths from URL sanitization checks (fixes 5 false-positive high severity alerts) - Add 23 targeted tests for hook commands, rich format outputs, fresh cache home view, and format validation (cli.py coverage 73% → 87%) - Add codecov.yml with appropriate thresholds (patch ≥80%, project ±2%) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_cli.py (1)
1011-1015: Several new tests only assertexit_code == 0and mock-call counts.Most of the newly added
*_rich,hook_status*, andcache_status_rich*cases only verify that the command did not crash. They won’t catch regressions in the actual rendered output (e.g., rich table contents, hook status fields, cache size rendering). Consider adding at least one substantivein result.stdoutassertion per case (e.g."claude"/"codex"for hook status,"example.com"or"folders/"for ancestors/rich,"4.0 KB"fortest_cache_status_rich). Otherwise the ~14% coverage bump is largely line-coverage only and doesn't exercise behavior.Also applies to: 1101-1105, 1116-1120, 1134-1138, 1143-1146, 1149-1152, 1156-1159, 1162-1165, 1168-1175, 1178-1185, 1189-1191, 1195-1198, 1201-1204, 1209-1212, 1215-1218, 1226-1229, 1243-1247, 1250-1253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 1011 - 1015, Tests like test_hook_status (and the other *_rich, hook_status*, cache_status_rich* cases) only assert exit_code and mock call counts, so they don't validate rendered output; update each such test (e.g., test_hook_status which calls runner.invoke(app, ["hook","status"]) and inspects result) to also assert at least one substantive substring in result.stdout that verifies behavior — for hook status assert provider names like "claude" or "codex", for ancestors/rich outputs assert domain/folder fragments like "example.com" or "folders/", and for cache size assertions assert formatted sizes like "4.0 KB"; keep the existing mock setup and exit_code checks and add one concise in result.stdout assertion per test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cli.py`:
- Around line 623-627: Replace the weak assertion in test_format_json_output:
instead of asserting a tautology, assert the CLI parser accepted the flag by
checking result.exit_code != 2 (Typer usage error) AND that the output does not
contain an unrecognized-option/usage error about the flag (e.g. ensure strings
like "No such option", "unrecognized arguments", or "Usage" referring to
"--format" are not in result.output). Locate the test function
test_format_json_output and the runner.invoke(app, ["--format", "json", "ls"])
call and update the assertion to these two concrete checks on result.exit_code
and result.output.
- Around line 1242-1247: There is a duplicate test function named
test_ls_rich_format causing ruff F811; remove the later redefinition (the one
that only asserts exit_code) so the original test (which checks result.stdout
for "Path") remains; locate the duplicate by the function name
test_ls_rich_format in the tests/test_cli.py diff and delete the redundant
function definition and its associated decorators/mocks (e.g.,
`@patch`("gcpath.core.Hierarchy.load")) to leave a single canonical test.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 1011-1015: Tests like test_hook_status (and the other *_rich,
hook_status*, cache_status_rich* cases) only assert exit_code and mock call
counts, so they don't validate rendered output; update each such test (e.g.,
test_hook_status which calls runner.invoke(app, ["hook","status"]) and inspects
result) to also assert at least one substantive substring in result.stdout that
verifies behavior — for hook status assert provider names like "claude" or
"codex", for ancestors/rich outputs assert domain/folder fragments like
"example.com" or "folders/", and for cache size assertions assert formatted
sizes like "4.0 KB"; keep the existing mock setup and exit_code checks and add
one concise in result.stdout assertion per test.
🪄 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
Run ID: 309ba328-4826-402b-9963-1e1eb80640b7
📒 Files selected for processing (4)
.github/codeql/codeql-config.yml.github/workflows/codeql.ymlcodecov.ymltests/test_cli.py
✅ Files skipped from review due to trivial changes (3)
- codecov.yml
- .github/workflows/codeql.yml
- .github/codeql/codeql-config.yml
- Remove duplicate test_ls_rich_format (ruff F811 CI blocker) - Tighten test_format_json_output assertion to check exit_code and option parsing - Replace import-time assert in toon.py with GCPathError for deterministic check - Keep count header in empty toon_ls output for schema consistency - Make _write_json atomic via tmp-file + os.replace - Validate JSON root type is dict in _read_json - Centralize managed hook matching with _is_managed_hook helper - Handle 0.0 cache age as fresh data (is not None check) - Narrow broad Exception catch in _parse_resource_arg to GCP/GCPath errors - Replace /tmp paths in hook status mock data with user config paths - Update README to drop non-existent --json/--yaml shorthand docs - Add minimal contents:read permission to CI workflow Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Replace bare `except Exception` in `_resolve_target_path_prefix` with specific `(GCPathError, gcp_exceptions.GoogleAPICallError)` as suggested in review - Remove custom CodeQL workflow files that conflict with the repository's default CodeQL setup, causing SARIF upload failures Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gcpath/cli.py`:
- Around line 1054-1065: The tree command must remain classic-only: remove or
disable the JSON/YAML branch that uses _get_dumper and serialize_tree (the block
guarded by if fmt in ("json", "yaml") that calls _get_dumper, serialize_tree
with hctx.nodes_to_process and hctx.projects_by_parent, and prints the dumper
output). Instead either (a) raise/return an explicit "unsupported format" error
when fmt is "json" or "yaml", or (b) ignore/override fmt and always render the
classic Rich tree; ensure callers of the tree handler (the function invoking
fmt/_get_dumper/serialize_tree) cannot emit structured output for tree and
update any user-facing message to reflect that tree only supports the classic
format.
- Line 283: The code treats age_seconds as falsy so 0.0 is rendered "unknown";
change the condition in the assignment of age_str to test for None explicitly
(e.g., use "if info.age_seconds is not None") and call
format_age(info.age_seconds) when present; update the expression that sets
age_str (referencing the variable age_str and function format_age and the
attribute info.age_seconds) so freshly written caches with age 0.0 are shown
correctly instead of "unknown".
- Around line 1468-1470: The current code sets total_searched to
len(all_matches) after pattern filtering, producing incorrect "X of Y searched"
counts; change this to compute the candidate count before applying the pattern
by adding a helper (e.g. _count_search_candidates) that uses the same
type-filter selection logic as _search_hierarchy, call that helper to set
total_searched, then call _search_hierarchy(hierarchy, pattern, resource_type)
and sort_resources as before; ensure the new helper mirrors the candidate
selection used inside _search_hierarchy so the candidate count and actual search
behavior remain consistent.
- Around line 663-694: The handler currently emits an error for default TOON
format; instead build and print a valid, parseable empty TOON "ls" payload in
_handle_empty_hierarchy: replace the final print(toon_error(...)) with code that
constructs the TOON list response (header "count: 0 of 0 total" and resources:
[]), and serialize/print it using the existing TOON serializer (use the same
helper that toon_ls or the TOON serializer uses) so callers get an empty list
result rather than an error.
- Around line 205-218: The handler handle_error should re-raise typer.Exit
immediately to avoid treating it as an unexpected Exception; add an early check
for isinstance(e, typer.Exit) (or catch click.exceptions.Exit) at the top of
handle_error and simply raise e so existing structured exits are preserved,
leaving the rest of the branches (GCPathError, gcp_exceptions.PermissionDenied,
gcp_exceptions.ServiceUnavailable, generic Exception) unchanged.
In `@src/gcpath/hooks.py`:
- Around line 71-83: The SessionStart hook handling is using a flat
{"command":..., "timeout":10000} shape and treats entries as direct command
dicts; instead update _install_claude_code to append a nested matcher group
object (e.g., {"matcher": {...}, "hooks":[{"type":"command","command":...,
"timeout":10}]}) under hooks["SessionStart"] and replace any managed entry by
updating that group's hooks["type":"command"] handler (use timeout 10 seconds,
not 10000); update _uninstall_claude_code to remove the handler by inspecting
each group's "hooks" array and removing matching handlers (or removing the group
if empty), and update _check_hook_entries to search inside each group["hooks"]
for handlers with "type":"command" and the managed command instead of treating
SessionStart entries as top-level dicts.
In `@src/gcpath/serializers.py`:
- Around line 33-51: The current _default_fields_for_items switches the entire
output schema to _PROJECT_LS_FIELDS when any Project exists, causing non-project
rows to get an empty project_id; change _default_fields_for_items to always
return _DEFAULT_LS_FIELDS, and instead add project_id only for Project rows when
constructing each row (i.e., in the per-item serialization logic that iterates
items), by detecting isinstance(obj, Project) and injecting project_id into that
row's output; apply the same fix pattern to the other occurrence noted around
lines 132-135.
- Around line 192-196: The empty-branch currently returns toon_empty(...) which
yields a different schema than the non-empty branch; change the empty branch to
return the same TOON list schema: include a "count" key (e.g., f"{len(items)} of
{total} total" or preserve existing count variable) and an empty "resources"
list, while still passing the help lines; update the code path that now calls
toon_empty(...) to instead construct and return the list response with keys
"count" and "resources": [], reusing pattern, help_lines, and any existing
total/count variables so gcpath find keeps a consistent schema with the
non-empty branch.
🪄 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
Run ID: 1d876a99-8267-4bc0-8d58-1ed316cb983c
📒 Files selected for processing (7)
.github/workflows/ci.ymlREADME.mdsrc/gcpath/cli.pysrc/gcpath/hooks.pysrc/gcpath/serializers.pysrc/gcpath/toon.pytests/test_cli.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/gcpath/toon.py
Comment on lines
205
to
218
| def handle_error(e: Exception) -> None: | ||
| """Central error handler for CLI.""" | ||
| if isinstance(e, GCPathError): | ||
| error_console.print(f"[red]Error:[/red] {e}") | ||
| print(toon_error(str(e))) | ||
| elif isinstance(e, gcp_exceptions.PermissionDenied): | ||
| error_console.print( | ||
| "[red]Permission Denied:[/red] Ensure you have the required permissions and are authenticated." | ||
| ) | ||
| error_console.print( | ||
| "[dim]Hint: Run 'gcloud auth application-default login'[/dim]" | ||
| ) | ||
| print(toon_error( | ||
| "Permission Denied. Ensure you have the required permissions and are authenticated.", | ||
| ["Run 'gcloud auth application-default login'"], | ||
| )) | ||
| elif isinstance(e, gcp_exceptions.ServiceUnavailable): | ||
| error_console.print( | ||
| "[red]Service Unavailable:[/red] The GCP API is currently unreachable." | ||
| ) | ||
| print(toon_error("The GCP API is currently unreachable.")) | ||
| elif isinstance(e, Exception): | ||
| error_console.print(f"[red]Unexpected Error:[/red] {e}") | ||
| print(toon_error(f"Unexpected error: {e}")) | ||
| logging.exception("Unexpected error occurred") | ||
| raise typer.Exit(code=1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Typer exits are caught by a broad Exception handler in the installed Typer/Click version.
python - <<'PY'
import typer
print(typer.Exit.__mro__)
print("is Exception:", issubclass(typer.Exit, Exception))
PYRepository: tardigrde/gcpath
Length of output: 177
🏁 Script executed:
# First, examine the handle_error function and surrounding context
cd src/gcpath && head -n 250 cli.py | tail -n 100Repository: tardigrde/gcpath
Length of output: 3495
🏁 Script executed:
# Search for all typer.Exit raises in the codebase
rg "typer\.Exit" --type py -nRepository: tardigrde/gcpath
Length of output: 943
🏁 Script executed:
# Look for exception handlers that catch Exception broadly
rg "except Exception" --type py -n -A 3Repository: tardigrde/gcpath
Length of output: 4208
🏁 Script executed:
# Search for validation functions that might raise typer.Exit or similar errors
rg "raise typer\.Exit|raise.*Exit" --type py -n -B 2 -A 1Repository: tardigrde/gcpath
Length of output: 4031
🏁 Script executed:
# Check imports at the top of cli.py to confirm typer is imported
head -n 30 src/gcpath/cli.pyRepository: tardigrde/gcpath
Length of output: 750
🏁 Script executed:
# Verify that typer.Exit would be caught by the Exception handler by examining the exact flow
# Check one command handler to confirm the pattern
sed -n '945,950p' src/gcpath/cli.pyRepository: tardigrde/gcpath
Length of output: 252
🏁 Script executed:
# Verify the validation function in main() that raises typer.Exit
sed -n '243,250p' src/gcpath/cli.pyRepository: tardigrde/gcpath
Length of output: 445
🏁 Script executed:
# Check if there are any existing typer.Exit checks in handle_error or before calling it
sed -n '205,230p' src/gcpath/cli.pyRepository: tardigrde/gcpath
Length of output: 1063
🌐 Web query:
typer.Exit exception inheritance Python Click
💡 Result:
In Typer (built on Click), typer.Exit is an exception used to terminate CLI execution gracefully with an optional exit code (default 0 for success, non-zero for errors). typer.Abort is a special case that prints "Aborted!" and exits with code 1. Typer's core.py handles click.exceptions.Exit and click.Abort specifically: except click.exceptions.Exit as e: if standalone_mode: sys.exit(e.exit_code) else: return e.exit_code This confirms typer.Exit is an alias or direct reference to click.exceptions.Exit, inheriting its behavior. Click's ClickException (base for user-facing errors) is handled separately with e.show and sys.exit(e.exit_code). To catch typer.Exit in except clauses, use: try: # code except typer.Exit: # handles normal exit except Exception as e: # other errors Since it's click.exceptions.Exit under the hood, except click.exceptions.Exit also works. Typer docs recommend raising typer.Exit directly for consistency. Official Typer terminating docs: https://typer.tiangolo.com/tutorial/terminating/ Typer core.py source: https://github.com/tiangolo/typer/blob/master/typer/core.py Click exceptions docs: https://click.palletsprojects.com/en/stable/exceptions/
Citations:
- 1: https://typer.tiangolo.com/tutorial/terminating/
- 2: https://github.com/tiangolo/typer/blob/master/typer/core.py
- 3: https://typer.tiangolo.com/tutorial/terminating
- 4: https://click.palletsprojects.com/en/stable/exceptions/
- 5: https://github.com/tiangolo/typer/blob/master/typer/main.py
Re-raise typer.Exit exceptions before rendering as unexpected errors.
Command bodies catch Exception, so validation paths that print a structured error and raise typer.Exit can flow into handle_error and produce a second "Unexpected error" message. typer.Exit inherits from click.exceptions.Exit, which is an Exception, so it matches the broad isinstance(e, Exception) check at line 216.
Proposed fix
def handle_error(e: Exception) -> None:
+ if isinstance(e, typer.Exit):
+ raise e
if isinstance(e, GCPathError):
print(toon_error(str(e)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/cli.py` around lines 205 - 218, The handler handle_error should
re-raise typer.Exit immediately to avoid treating it as an unexpected Exception;
add an early check for isinstance(e, typer.Exit) (or catch
click.exceptions.Exit) at the top of handle_error and simply raise e so existing
structured exits are preserved, leaving the rest of the branches (GCPathError,
gcp_exceptions.PermissionDenied, gcp_exceptions.ServiceUnavailable, generic
Exception) unchanged.
| print(toon_encode({"help": help_lines})) | ||
| raise typer.Exit() | ||
|
|
||
| age_str = format_age(info.age_seconds) if info.age_seconds else "unknown" |
There was a problem hiding this comment.
Treat 0.0 cache age as known.
A just-written fresh cache can have age_seconds == 0.0; this branch renders it as unknown.
Proposed fix
- age_str = format_age(info.age_seconds) if info.age_seconds else "unknown"
+ age_str = format_age(info.age_seconds) if info.age_seconds is not None else "unknown"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| age_str = format_age(info.age_seconds) if info.age_seconds else "unknown" | |
| age_str = format_age(info.age_seconds) if info.age_seconds is not None else "unknown" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/cli.py` at line 283, The code treats age_seconds as falsy so 0.0
is rendered "unknown"; change the condition in the assignment of age_str to test
for None explicitly (e.g., use "if info.age_seconds is not None") and call
format_age(info.age_seconds) when present; update the expression that sets
age_str (referencing the variable age_str and function format_age and the
attribute info.age_seconds) so freshly written caches with age 0.0 are shown
correctly instead of "unknown".
Comment on lines
+663
to
694
| def _handle_empty_hierarchy(fmt: str) -> None: | ||
| if fmt in ("json", "yaml"): | ||
| dumper = _get_dumper(fmt) | ||
| if dumper: | ||
| print(dumper([])) | ||
| return | ||
|
|
||
| import google.auth | ||
|
|
||
| account_msg = "" | ||
| try: | ||
| credentials, _ = google.auth.default() | ||
| if hasattr(credentials, "account") and credentials.account: | ||
| if credentials.account.endswith("@gmail.com"): | ||
| account_msg = f" (Account: {credentials.account})" | ||
| except Exception: | ||
| pass | ||
|
|
||
| rprint( | ||
| f"[yellow]No organizations or projects found accessible to your account{account_msg}.[/yellow]" | ||
| ) | ||
| if not account_msg: | ||
| if fmt == "rich": | ||
| import google.auth | ||
| account_msg = "" | ||
| try: | ||
| credentials, _ = google.auth.default() | ||
| if hasattr(credentials, "account") and credentials.account: | ||
| if credentials.account.endswith("@gmail.com"): | ||
| account_msg = f" (Account: {credentials.account})" | ||
| except Exception: | ||
| pass | ||
| rprint( | ||
| "[dim]Hint: You might not have access to any organizations. " | ||
| "Projects without organizations are shown with //_ prefix.[/dim]" | ||
| f"[yellow]No organizations or projects found accessible to your account{account_msg}.[/yellow]" | ||
| ) | ||
| if not account_msg: | ||
| rprint( | ||
| "[dim]Hint: You might not have access to any organizations. " | ||
| "Projects without organizations are shown with //_ prefix.[/dim]" | ||
| ) | ||
| return | ||
|
|
||
| print(toon_error( | ||
| "No organizations or projects found accessible to your account", | ||
| ["Projects without organizations are shown with //_ prefix"], | ||
| )) | ||
|
|
There was a problem hiding this comment.
Return a parseable empty ls payload instead of error:.
For default TOON output, an accessible-but-empty hierarchy is a valid empty list result, not a CLI error. Emitting error: here bypasses the count/resources schema that toon_ls() already provides.
Proposed fix
- print(toon_error(
- "No organizations or projects found accessible to your account",
- ["Projects without organizations are shown with //_ prefix"],
- ))
+ print(toon_ls(
+ [],
+ 0,
+ help_lines=[
+ "No organizations or projects found accessible to your account",
+ "Projects without organizations are shown with //_ prefix",
+ ],
+ ))As per coding guidelines, "{**/toon.py,**/serializers.py}: Include count: N of M total header on list outputs in TOON format".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/cli.py` around lines 663 - 694, The handler currently emits an
error for default TOON format; instead build and print a valid, parseable empty
TOON "ls" payload in _handle_empty_hierarchy: replace the final
print(toon_error(...)) with code that constructs the TOON list response (header
"count: 0 of 0 total" and resources: []), and serialize/print it using the
existing TOON serializer (use the same helper that toon_ls or the TOON
serializer uses) so callers get an empty list result rather than an error.
Comment on lines
+1054
to
1065
| if fmt in ("json", "yaml"): | ||
| dumper = _get_dumper(fmt) | ||
| if dumper: | ||
| data = serialize_tree( | ||
| hctx.nodes_to_process, | ||
| hctx.projects_by_parent, | ||
| level, | ||
| orgless_projects, | ||
| type_filter=resource_type, | ||
| ) | ||
| print(dumper(data)) | ||
| return |
There was a problem hiding this comment.
Keep tree classic-only; do not emit JSON/YAML trees.
This branch makes tree --format json|yaml produce structured output, but the PR/design rule says agents should use ls -R and tree should stay a human tree. Remove this branch or make structured formats explicitly unsupported for tree.
As per coding guidelines, "{**/cli.py,**/formatters.py}: tree command outputs classic unicode tree format (Rich tree widget), not TOON; agents should use ls -R instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/cli.py` around lines 1054 - 1065, The tree command must remain
classic-only: remove or disable the JSON/YAML branch that uses _get_dumper and
serialize_tree (the block guarded by if fmt in ("json", "yaml") that calls
_get_dumper, serialize_tree with hctx.nodes_to_process and
hctx.projects_by_parent, and prints the dumper output). Instead either (a)
raise/return an explicit "unsupported format" error when fmt is "json" or
"yaml", or (b) ignore/override fmt and always render the classic Rich tree;
ensure callers of the tree handler (the function invoking
fmt/_get_dumper/serialize_tree) cannot emit structured output for tree and
update any user-facing message to reflect that tree only supports the classic
format.
Comment on lines
+1468
to
1470
| all_matches = sort_resources(_search_hierarchy(hierarchy, pattern, resource_type)) | ||
| total_searched = len(all_matches) | ||
|
|
There was a problem hiding this comment.
Use candidate count for find’s total_searched.
Line 1469 sets total_searched to the number of name matches, so normal searches report counts like 2 of 2 searched even when many more resources were inspected. Compute the candidate count before applying the pattern.
Proposed direction
- all_matches = sort_resources(_search_hierarchy(hierarchy, pattern, resource_type))
- total_searched = len(all_matches)
+ total_searched = _count_search_candidates(hierarchy, resource_type)
+ all_matches = sort_resources(_search_hierarchy(hierarchy, pattern, resource_type))Add _count_search_candidates() with the same type-filter candidate selection used by _search_hierarchy().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/cli.py` around lines 1468 - 1470, The current code sets
total_searched to len(all_matches) after pattern filtering, producing incorrect
"X of Y searched" counts; change this to compute the candidate count before
applying the pattern by adding a helper (e.g. _count_search_candidates) that
uses the same type-filter selection logic as _search_hierarchy, call that helper
to set total_searched, then call _search_hierarchy(hierarchy, pattern,
resource_type) and sort_resources as before; ensure the new helper mirrors the
candidate selection used inside _search_hierarchy so the candidate count and
actual search behavior remain consistent.
Comment on lines
+71
to
+83
| for entry in hooks["SessionStart"]: | ||
| if isinstance(entry, dict) and _is_managed_hook(entry.get("command", "")): | ||
| if entry.get("command") == command: | ||
| return False | ||
| entry["command"] = command | ||
| _write_json(_CLAUDE_SETTINGS_PATH, data) | ||
| return True | ||
|
|
||
| hooks["SessionStart"].append({ | ||
| "command": command, | ||
| "timeout": 10000, | ||
| }) | ||
| _write_json(_CLAUDE_SETTINGS_PATH, data) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the hooks.py file to see the actual implementation
wc -l src/gcpath/hooks.pyRepository: tardigrde/gcpath
Length of output: 83
🏁 Script executed:
# Read the entire hooks.py file to understand the structure
cat -n src/gcpath/hooks.pyRepository: tardigrde/gcpath
Length of output: 10138
🏁 Script executed:
# Also check if there are any tests or examples showing the expected schema
fd -e py -e json | xargs grep -l "SessionStart" | head -10Repository: tardigrde/gcpath
Length of output: 99
🏁 Script executed:
# Check the test file for expected schema
cat -n tests/test_hooks.pyRepository: tardigrde/gcpath
Length of output: 10804
🏁 Script executed:
# Search for any documentation or schema references in the repo
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | xargs grep -l -i "hook\|claude\|schema" 2>/dev/null | head -5Repository: tardigrde/gcpath
Length of output: 98
🏁 Script executed:
# Check if there are any JSON example files or fixtures
find . -type f -name "*.json" | head -10Repository: tardigrde/gcpath
Length of output: 135
🏁 Script executed:
# Check the CLAUDE.md file for any schema information
cat -n CLAUDE.mdRepository: tardigrde/gcpath
Length of output: 10989
🏁 Script executed:
# Also check README for any hook-related documentation
grep -A 10 -B 10 -i "hook" README.mdRepository: tardigrde/gcpath
Length of output: 3394
🌐 Web query:
Claude Code hooks schema SessionStart command handler documentation
💡 Result:
Claude Code's SessionStart hook runs command hooks (type: "command" only) when a session begins or resumes. It is configured in settings.json files like ~/.claude/settings.json or .claude/settings.json under the "hooks" object. Configuration schema structure: { "hooks": { "SessionStart": [ { "matcher": "startup|resume|clear|compact", // optional, matches source "hooks": [ { "type": "command", "command": "your-shell-command", "timeout": 30 // optional seconds } ] } ] } } Matchers for SessionStart (how session initiated): - startup: new session - resume: --resume, /resume - clear: /clear - compact: after compaction Input JSON (provided on stdin to command): { "session_id": "abc123", "transcript_path": "/path/to/session.jsonl", "cwd": "/current/working/dir", "hook_event_name": "SessionStart", "source": "startup", "model": "claude-sonnet-4-6", "agent_type": "optional-agent-name" // if --agent used } Output/Control: - Any plain text on stdout (exit 0) is added to Claude's context. - JSON on stdout (exit 0) parsed for hookSpecificOutput, e.g.: { "hookSpecificOutput": { "hookEventName": "SessionStart", "additionalContext": "Custom context string" } } - Exit codes control flow (common to hooks): 0=allow/continue, 1/2=block, etc. - Environment vars available: CLAUDE_PROJECT_DIR, CLAUDE_ENV_FILE (for persisting env vars). Keep hooks fast as they run every session. Use for dynamic context injection, env setup. For static context, use CLAUDE.md instead. Note: Potential bug in some versions where output not injected on new sessions (requires /clear workaround), per GitHub issue.
Citations:
- 1: https://code.claude.com/docs/en/hooks
- 2: https://docs.claude.com/en/docs/claude-code/hooks.md
- 3: https://code.claude.com/docs/en/hooks.md
- 4: https://docs.anthropic.com/en/docs/claude-code/hooks
- 5: http://code.claude.com/docs/en/hooks
- 6: https://docs.claude.com/en/docs/claude-code/hooks
- 7: https://code.claude.com/docs/en/settings
- 8: https://claude.yourdocs.dev/docs/claude-code/hooks-guide
Fix Claude Code hooks to use nested handler schema with correct timeout units.
The current code writes a flat structure {"command": ..., "timeout": 10000} directly under hooks.SessionStart, but Claude Code expects a nested structure with a matcher group containing a hooks array where each handler has "type": "command". The timeout of 10000 is also incorrect — it should be 10 (seconds), not a millisecond conversion.
This mismatch means gcpath hook install will report success while Claude Code silently ignores the hook.
Update _install_claude_code() (lines 71–84), _uninstall_claude_code() (lines 97–100), and _check_hook_entries() (lines 197–209) to write and search within the nested handler structure:
Expected structure and fix locations
hooks["SessionStart"].append({
- "command": command,
- "timeout": 10000,
+ "matcher": "*",
+ "hooks": [
+ {
+ "type": "command",
+ "command": command,
+ "timeout": 10,
+ }
+ ],
})Update uninstall and status checking to inspect group["hooks"] handlers rather than treating SessionStart entries as direct command dicts.
See Claude Code hooks reference: https://code.claude.com/docs/en/hooks
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/hooks.py` around lines 71 - 83, The SessionStart hook handling is
using a flat {"command":..., "timeout":10000} shape and treats entries as direct
command dicts; instead update _install_claude_code to append a nested matcher
group object (e.g., {"matcher": {...}, "hooks":[{"type":"command","command":...,
"timeout":10}]}) under hooks["SessionStart"] and replace any managed entry by
updating that group's hooks["type":"command"] handler (use timeout 10 seconds,
not 10000); update _uninstall_claude_code to remove the handler by inspecting
each group's "hooks" array and removing matching handlers (or removing the group
if empty), and update _check_hook_entries to search inside each group["hooks"]
for handlers with "type":"command" and the managed command instead of treating
SessionStart entries as top-level dicts.
Comment on lines
+33
to
+51
| _DEFAULT_LS_FIELDS = ("path", "type", "display_name") | ||
| _PROJECT_LS_FIELDS = ("path", "type", "display_name", "project_id") | ||
|
|
||
| _ALL_LS_FIELDS = ( | ||
| "path", | ||
| "type", | ||
| "display_name", | ||
| "resource_name", | ||
| "project_id", | ||
| "labels", | ||
| "tags", | ||
| ) | ||
|
|
||
|
|
||
| def _default_fields_for_items( | ||
| items: List[Tuple[str, Union[OrganizationNode, Folder, Project]]], | ||
| ) -> Tuple[str, ...]: | ||
| has_projects = any(isinstance(obj, Project) for _, obj in items) | ||
| return _PROJECT_LS_FIELDS if has_projects else _DEFAULT_LS_FIELDS |
There was a problem hiding this comment.
Keep the default ls schema to three fields.
Line 50 switches every row to _PROJECT_LS_FIELDS when any project is present, so folders and organizations also get project_id: "". Keep {path,type,display_name} as the default row shape and add project_id only on project rows.
Proposed adjustment
_DEFAULT_LS_FIELDS = ("path", "type", "display_name")
-_PROJECT_LS_FIELDS = ("path", "type", "display_name", "project_id")
@@
def _default_fields_for_items(
items: List[Tuple[str, Union[OrganizationNode, Folder, Project]]],
) -> Tuple[str, ...]:
- has_projects = any(isinstance(obj, Project) for _, obj in items)
- return _PROJECT_LS_FIELDS if has_projects else _DEFAULT_LS_FIELDS
+ return _DEFAULT_LS_FIELDS
@@
- rows = [
- _serialize_resource_fields(p, obj, effective_fields, full=full)
- for p, obj in items
- ]
+ rows = []
+ for p, obj in items:
+ row = _serialize_resource_fields(p, obj, effective_fields, full=full)
+ if fields is None and isinstance(obj, Project):
+ row["project_id"] = obj.project_id
+ rows.append(row)As per coding guidelines, "**/serializers.py: Default ls schema must be {path,type,display_name} (3 fields); add project_id for projects".
Also applies to: 132-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/serializers.py` around lines 33 - 51, The current
_default_fields_for_items switches the entire output schema to
_PROJECT_LS_FIELDS when any Project exists, causing non-project rows to get an
empty project_id; change _default_fields_for_items to always return
_DEFAULT_LS_FIELDS, and instead add project_id only for Project rows when
constructing each row (i.e., in the per-item serialization logic that iterates
items), by detecting isinstance(obj, Project) and injecting project_id into that
row's output; apply the same fix pattern to the other occurrence noted around
lines 132-135.
Comment on lines
+192
to
+196
| if not items: | ||
| empty_help = help_lines or [ | ||
| f"Run `gcpath find '{pattern}'` with a broader pattern" | ||
| ] | ||
| return toon_empty("resources", f"matching '{pattern}' found", empty_help) |
There was a problem hiding this comment.
Preserve count on empty find output.
The empty branch returns toon_empty(...), so gcpath find has a different schema when there are no matches. Return count plus an empty resources list, matching the non-empty branch.
Proposed fix
if not items:
empty_help = help_lines or [
f"Run `gcpath find '{pattern}'` with a broader pattern"
]
- return toon_empty("resources", f"matching '{pattern}' found", empty_help)
+ empty_data: Dict[str, Any] = {
+ "count": f"0 of {total_searched} searched",
+ "resources": [],
+ }
+ return with_help(toon_encode(empty_data), empty_help)As per coding guidelines, "{**/toon.py,**/serializers.py}: Include count: N of M total header on list outputs in TOON format".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/serializers.py` around lines 192 - 196, The empty-branch currently
returns toon_empty(...) which yields a different schema than the non-empty
branch; change the empty branch to return the same TOON list schema: include a
"count" key (e.g., f"{len(items)} of {total} total" or preserve existing count
variable) and an empty "resources" list, while still passing the help lines;
update the code path that now calls toon_empty(...) to instead construct and
return the list response with keys "count" and "resources": [], reusing pattern,
help_lines, and any existing total/count variables so gcpath find keeps a
consistent schema with the non-empty branch.
The `in` operator on strings like "example.com" triggers CodeQL rule py/incomplete-url-substring-sanitization. These are test output assertions, not URL sanitization code — suppress with lgtm comments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Alerts dismissed directly in GitHub Code Scanning as "used in tests". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ring sanitization' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
- Use simple substring check instead of urlparse which fails on TOON comma-separated rows - Remove unused urlparse import to silence CodeQL false positive
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_cli.py (3)
1070-1089: UseCacheInfoinstead of an unspeccedMagicMock.
_show_home()consumes the concrete cache-info shape fromsrc/gcpath/cache.py:27-39; an unspecced mock can hide missing/renamed fields or serialize mock attributes accidentally.Suggested concrete return value
`@patch`("gcpath.cli.get_cache_info") `@patch`("gcpath.cli.read_cache_raw") def test_home_view_with_fresh_cache(mock_raw, mock_info): - mock_info.return_value = MagicMock( + mock_info.return_value = CacheInfo( exists=True, fresh=True, age_seconds=120.0, size_bytes=1024, version=1, org_count=1, folder_count=5, project_count=10, + scope=None, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 1070 - 1089, Replace the unspecced MagicMock used in test_home_view_with_fresh_cache with a concrete CacheInfo instance (from src/gcpath/cache.py) so the test mirrors the real shape consumed by _show_home; construct CacheInfo with the fields exists=True, fresh=True, age_seconds=120.0, size_bytes=1024, version=1, org_count=1, folder_count=5, project_count=10 and set mock_info.return_value to that CacheInfo instance.
1010-1015: Make the plain hook status test deterministic.This test should not read the caller’s real hook configuration. Patch
get_hook_status, like the rich-mode tests do, and assert the non-rich output includes the expected providers.Suggested deterministic fixture
-@patch("gcpath.core.Hierarchy.load") -def test_hook_status(mock_load, mock_hierarchy): - mock_load.return_value = mock_hierarchy +@patch("gcpath.cli.get_hook_status", return_value={ + "claude": {"installed": True, "path_ok": True, "location": "~/.claude/settings.json"}, + "codex": {"installed": False, "path_ok": False, "location": "~/.codex/hooks.json"}, +}) +def test_hook_status(mock_status): result = runner.invoke(app, ["hook", "status"]) assert result.exit_code == 0 + mock_status.assert_called_once() + assert "claude" in result.stdout + assert "codex" in result.stdout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 1010 - 1015, The test_hook_status currently relies on real hook configuration; make it deterministic by patching gcpath.core.hooks.get_hook_status (similar to the rich-mode tests): in test_hook_status add a `@patch` for get_hook_status, set its return_value to a deterministic dict/list of providers (e.g., the providers you expect to see), keep mocking Hierarchy.load via mock_load.return_value = mock_hierarchy, call runner.invoke(app, ["hook", "status"]), assert exit_code == 0 and also assert that result.output (non-rich) contains the expected provider names from the mocked get_hook_status.
974-999: Tighten the--fieldsand--fullassertions.These tests currently pass even if
--fieldsis ignored or--fullstill truncates label output. Parse structured output for field projection, and assert all generated labels are present for the full-output path.Suggested test tightening
`@patch`("gcpath.core.Hierarchy.load") def test_ls_fields_flag(mock_load, mock_hierarchy): mock_load.return_value = mock_hierarchy - result = runner.invoke(app, ["ls", "--fields", "path,type"]) + result = runner.invoke(app, ["--format", "json", "ls", "--fields", "path,type"]) assert result.exit_code == 0 - assert "path" in result.stdout - assert "type" in result.stdout + data = json.loads(result.stdout) + assert data + assert all(set(item) == {"path", "type"} for item in data) @@ result = runner.invoke(app, ["ls", "--fields", "path,type,labels", "--full"]) assert result.exit_code == 0 + for i in range(8): + assert f"key{i}" in result.stdout + assert f"val{i}" in result.stdout + assert "truncated" not in result.stdout.lower()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 974 - 999, The tests (test_ls_fields_flag, test_ls_fields_invalid, test_ls_full_flag) are too loose because they only search stdout text; update them to parse the CLI's structured output and assert field projection and full label expansion precisely: call runner.invoke(app, ["ls", "--fields", "path,type"]) and parse the output format (CSV/JSON/table) to verify each row contains exactly the requested fields (no unexpected columns) and that invalid fields return exit_code 1 in test_ls_fields_invalid; for test_ls_full_flag, generate the hierarchy via make_test_hierarchy(), set f1.labels with 8 keys, invoke runner.invoke(app, ["ls", "--fields", "path,type,labels", "--full"]) and parse the labels cell to assert all 8 label key/value pairs are present (no truncation). Locate assertions around runner.invoke/app in those three test functions and replace simple substring checks with structured parsing and explicit membership/column-count checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 1070-1089: Replace the unspecced MagicMock used in
test_home_view_with_fresh_cache with a concrete CacheInfo instance (from
src/gcpath/cache.py) so the test mirrors the real shape consumed by _show_home;
construct CacheInfo with the fields exists=True, fresh=True, age_seconds=120.0,
size_bytes=1024, version=1, org_count=1, folder_count=5, project_count=10 and
set mock_info.return_value to that CacheInfo instance.
- Around line 1010-1015: The test_hook_status currently relies on real hook
configuration; make it deterministic by patching
gcpath.core.hooks.get_hook_status (similar to the rich-mode tests): in
test_hook_status add a `@patch` for get_hook_status, set its return_value to a
deterministic dict/list of providers (e.g., the providers you expect to see),
keep mocking Hierarchy.load via mock_load.return_value = mock_hierarchy, call
runner.invoke(app, ["hook", "status"]), assert exit_code == 0 and also assert
that result.output (non-rich) contains the expected provider names from the
mocked get_hook_status.
- Around line 974-999: The tests (test_ls_fields_flag, test_ls_fields_invalid,
test_ls_full_flag) are too loose because they only search stdout text; update
them to parse the CLI's structured output and assert field projection and full
label expansion precisely: call runner.invoke(app, ["ls", "--fields",
"path,type"]) and parse the output format (CSV/JSON/table) to verify each row
contains exactly the requested fields (no unexpected columns) and that invalid
fields return exit_code 1 in test_ls_fields_invalid; for test_ls_full_flag,
generate the hierarchy via make_test_hierarchy(), set f1.labels with 8 keys,
invoke runner.invoke(app, ["ls", "--fields", "path,type,labels", "--full"]) and
parse the labels cell to assert all 8 label key/value pairs are present (no
truncation). Locate assertions around runner.invoke/app in those three test
functions and replace simple substring checks with structured parsing and
explicit membership/column-count checks.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--format toon|json|yaml|richflag,--fields/--fullfor output control, content-first home view, ambient context hooks, pre-computed aggregates, structured errors on stdout, and definitive empty states--json/--yaml/--long/--yesflags and all interactive promptsWhat changed
New files:
src/gcpath/toon.py— TOON encoder wrapper + AXI helpers (error formatting, empty states, help sections, dashboards)src/gcpath/hooks.py— Claude Code / Codex session hook management (gcpath hook install/uninstall/status/run)tests/test_hooks.py— 20 tests for hook install, uninstall, idempotency, status, and session-start outputModified files:
src/gcpath/cli.py— Full AXI rewrite: format flag, home view, no prompts, structured errors, hook subcommandssrc/gcpath/serializers.py— TOON serializers alongside JSON/YAML,--fields/--fullsupport, truncationpyproject.toml— Addedtoon-formatgit dependencytests/test_cli.py— Rewritten for new API surfacetests/test_serializers.py— Added TOON serializer testsCLAUDE.md— Updated with AXI compliance docsDesign decisions
treekeeps classic unicode tree output (not TOON) — agents should usels -Rinsteaddiagramkeeps raw Mermaid/D2 output with its own--diagram-formatflag (avoids collision with global--format)toon-formatPython library handles TOON encoding;toon.pywraps it with gcpath-specific AXI conventionsTest plan
uv run python -m pytest tests/— 346 tests passuv run ruff check src/gcpath/— all checks passeduv run python -m mypy src/gcpath/— no issues foundgcpath lsshows correctcount: N of M totalgcpath(no args) shows dashboard withbin: ~/...gcpath hook installinstalls into~/.claude/settings.json🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--format {toon|json|yaml|rich}(default: TOON) and structured TOON outputs for list, stats, find, name, path, ancestors, cache, and dashboard viewshookcommand group: install / uninstall / run / status for ambient session-start hooks--fieldsand--fullforls/find; home/dashboard shown when invoked without subcommandChanged
--json/--yaml/--long/--yesflags; diagram format option renamed to--diagram-format/-dDocumentation
Tests