Repository navigation
feat(cli): add --json to source mutating + detail + stale commands (I3) - #497
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
@claude review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a ChangesJSON output for source commands
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Claude finished @teng-lin's task in 2m 22s —— View job Code Review — PR #497:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/notebooklm/cli/source.py`:
- Around line 1420-1431: The JSON output and summary over-report deletions
because the loop increments the `deleted` counter for any non-exception result;
update the deletion handling so only truthy/True delete results increment
`deleted` (or compute `deleted` from a list of successful IDs) and ensure the
`deleted_count` passed to `json_output_response` (and the non-JSON summary) uses
that corrected count; look for the deletion loop that updates `deleted` and the
JSON payload construction using `deleted`, `_candidates_payload()`,
`nb_id_resolved`, `candidates`, `failures`, and change the logic to treat a
False return from the delete operation as a failure (append to `failures`) and
only count True results as successful deletions.
🪄 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: 5a0c9a08-e6c6-4ff4-962a-646f053ff2c9
📒 Files selected for processing (3)
CHANGELOG.mdsrc/notebooklm/cli/source.pytests/unit/cli/test_source.py
| if json_output: | ||
| json_output_response( | ||
| { | ||
| "action": "clean", | ||
| "notebook_id": nb_id_resolved, | ||
| "status": "completed", | ||
| "candidates": _candidates_payload(), | ||
| "candidate_count": len(candidates), | ||
| "deleted_count": deleted, | ||
| "failure_count": len(failures), | ||
| "failures": [{"id": sid, "error": err} for sid, err in failures], | ||
| } |
There was a problem hiding this comment.
source clean --json can over-report successful deletions.
deleted_count is derived from a loop where any non-exception result increments deleted; a False delete result is currently counted as success. This makes JSON (and non-JSON summary) inaccurate and can hide failed deletions from automation.
Suggested fix
for i in range(0, len(delete_list), chunk_size):
chunk = delete_list[i : i + chunk_size]
delete_tasks = [client.sources.delete(nb_id_resolved, sid) for sid in chunk]
results = await asyncio.gather(*delete_tasks, return_exceptions=True)
for sid, r in zip(chunk, results, strict=True):
if isinstance(r, Exception):
failures.append((sid, str(r)))
- else:
+ elif r:
deleted += 1
+ else:
+ failures.append((sid, "delete returned False"))
if i + chunk_size < len(delete_list):
await asyncio.sleep(0.5)🤖 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 `@src/notebooklm/cli/source.py` around lines 1420 - 1431, The JSON output and
summary over-report deletions because the loop increments the `deleted` counter
for any non-exception result; update the deletion handling so only truthy/True
delete results increment `deleted` (or compute `deleted` from a list of
successful IDs) and ensure the `deleted_count` passed to `json_output_response`
(and the non-JSON summary) uses that corrected count; look for the deletion loop
that updates `deleted` and the JSON payload construction using `deleted`,
`_candidates_payload()`, `nb_id_resolved`, `candidates`, `failures`, and change
the logic to treat a False return from the delete operation as a failure (append
to `failures`) and only count True results as successful deletions.
Closes P2.T3 from the cli-ux-remediation plan. Eight ``source``
subcommands now accept the standard ``--json`` flag from
``cli/options.py:json_option`` and emit a structured JSON document on
stdout for parseable automation:
- ``source delete`` — {action, source_id, notebook_id, success, status}
- ``source delete-by-title`` — adds ``title``
- ``source rename`` — adds ``title`` (post-rename)
- ``source refresh`` — three-state status (refreshed/no_result)
- ``source clean`` — already_clean / dry_run / cancelled / completed
with per-deletion failure list
- ``source get`` — mirrors ``Source`` dataclass; ``found`` flag
- ``source add-drive`` — {action, source{id,title,type,url,
drive_file_id, mime_type}, notebook_id}
- ``source stale`` — PRESERVES the inverted exit-code semantics
documented in docs/cli-exit-codes.md
(stale=0, fresh=1) — JSON body carries the
boolean explicitly via {stale, fresh}
Non-JSON output, exit codes, and confirmation prompts are unchanged.
``source get`` not-found still exits 0 (Phase 3 / C1 will flip this);
the JSON branch surfaces ``{found: false, source: null}`` so callers
can already branch on the field today.
Adds 13 JSON-output smoke tests (``TestSourceJsonOutput``) using
``CliRunner`` with a mocked client. Two of the tests pin the inverted
``source stale --json`` exit codes so future refactors can't silently
flip them.
CHANGELOG entry under [Unreleased] / Added.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
00c2640 to
e65eef8
Compare
…3) (teng-lin#497) Closes P2.T3 from the cli-ux-remediation plan. Eight ``source`` subcommands now accept the standard ``--json`` flag from ``cli/options.py:json_option`` and emit a structured JSON document on stdout for parseable automation: - ``source delete`` — {action, source_id, notebook_id, success, status} - ``source delete-by-title`` — adds ``title`` - ``source rename`` — adds ``title`` (post-rename) - ``source refresh`` — three-state status (refreshed/no_result) - ``source clean`` — already_clean / dry_run / cancelled / completed with per-deletion failure list - ``source get`` — mirrors ``Source`` dataclass; ``found`` flag - ``source add-drive`` — {action, source{id,title,type,url, drive_file_id, mime_type}, notebook_id} - ``source stale`` — PRESERVES the inverted exit-code semantics documented in docs/cli-exit-codes.md (stale=0, fresh=1) — JSON body carries the boolean explicitly via {stale, fresh} Non-JSON output, exit codes, and confirmation prompts are unchanged. ``source get`` not-found still exits 0 (Phase 3 / C1 will flip this); the JSON branch surfaces ``{found: false, source: null}`` so callers can already branch on the field today. Adds 13 JSON-output smoke tests (``TestSourceJsonOutput``) using ``CliRunner`` with a mocked client. Two of the tests pin the inverted ``source stale --json`` exit codes so future refactors can't silently flip them. CHANGELOG entry under [Unreleased] / Added. Co-authored-by: Hanzo Dev <dev@hanzo.ai>
1 participant
Summary
P2.T3 from the cli-ux-remediation plan (
.sisyphus/plans/cli-ux-remediation/phase-2.md). Adds the standard--jsonflag to eightsourcesubcommands so shell scripts and AI agents can branch on a structured JSON document instead of scraping Rich-formatted text.What changed
Eight commands now accept
--json(via@json_optionfromcli/options.py):source delete—{action, source_id, notebook_id, success, status}source delete-by-title— addstitlesource rename— addstitle(post-rename)source refresh— three-state status (refreshed/no_result)source clean—already_clean/dry_run/cancelled/completed, with per-deletion failure listsource get— mirrors theSourcedataclass; carries afoundbooleansource add-drive— full source payload plus the Drivemime_typeanddrive_file_idechosource stale— PRESERVES the inverted exit-code semantics documented indocs/cli-exit-codes.md(stale=0, fresh=1). The JSON body carries the boolean explicitly via{"stale": <bool>, "fresh": <bool>}for callers who would prefer to branch on a field rather than the exit code.Non-JSON output, exit codes, and confirmation prompts are unchanged.
source geton not-found still exits 0 today (Phase 3 / C1 will flip that); the JSON branch already surfaces{"found": false, "source": null}so callers can branch on the field without text-scraping.Constraints honored
cli/source.pyandtests/unit/cli/test_source.pymodified — strict file isolation per the phase plan (T1, T2, T4, T5 run in parallel).cli/options.pyuntouched.helpers.handle_erroruntouched.source addand the four commands that already had inline--json(list,fulltext,guide,wait) untouched — only the eight in scope.sourcegroup docstring (P1.T3) is preserved.source staleandsource waitexit codes remain as documented indocs/cli-exit-codes.md.Test plan
TestSourceJsonOutputsmoke tests (CliRunner + mocked client).source stale --jsonto exit 0 when stale and exit 1 when fresh — regression guard against accidental inversion.uv run ruff format .— clean.uv run ruff check .— clean.uv run mypy src/notebooklm --ignore-missing-imports— clean.uv run pytest --cov=src/notebooklm --cov-fail-under=90— 3475 passed, coverage 92.56%, source.py at 94%.Plan / spec
.sisyphus/plans/cli-ux-remediation/phase-2.md#p2t3----json-on-source-commandsdocs/cli-exit-codes.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--jsonflag to eightsourcesubcommands (get, delete, delete-by-title, rename, refresh, add-drive, stale, clean) to emit structured JSON for automation.Documentation
--jsonbehavior, fields, and inverted exit semantics forstale.Tests
--jsonoutput shapes, exit-code semantics, and clean/dry-run/cancel paths.