fix: address 11 CodeRabbit findings from PR #11#12
Merged
Conversation
1. Remove trailing </content></invoke> artifacts from README 2. Add tui.auto_launch to minimal preset YAML (was bypassing applyPreset) 3. Escape python path in AppleScript to handle spaces/special chars 4. Fix test taxonomy: integration/ is mock-based, not end-to-end 5. Fix coaching preset docs: enables communication, not status line 6. Align security reporting policy between SECURITY.md and issue template 7. Document scoped shell:true usage in SECURITY.md (trusted commands only) 8. Add blank lines after headings in security vulnerability template 9. Add calendar credential scope compatibility check 10. Add descriptive alt-text for feed platform diagram 11. Use approximate test counts (1400+) to prevent staleness 12. Fix GuardTester API in docs/cli.md (was still using old API) 13. Fix pre-existing venv-sensitive test assertions in tui-launcher tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR updates documentation and guidance templates across the repository, including security vulnerability reporting procedures, architecture documentation counts, CLI preset descriptions, and adds OAuth scope validation to the calendar feed script. Changes are primarily documentation with one minor code addition. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture.md`:
- Around line 35-36: Update the diagram description text to match the
architecture baseline by changing the producer and segment counts used in the
line describing the feed platform (replace "8 producers" and "21 status line
segments" with "5 producers" and "19 segments") and change the source reference
to a relative file link pointing to the actual asset (use
"docs/assets/feed-platform.excalidraw" as the source link); locate and edit the
feed-platform diagram description string and the source reference text
(feed-platform.excalidraw) in the document to apply these exact replacements.
In `@scripts/calendar-feed.py`:
- Around line 87-91: The credentials loading/validation is bypassing scope
checks when creds.scopes is falsy; update the call to
Credentials.from_authorized_user_file to pass the defined SCOPES so loaded
credentials always include scope metadata, and change the validation in the
block that uses creds/scopes to check for exact matching against entries in
SCOPES (not suffix matching) — locate the
Credentials.from_authorized_user_file(TOKEN_PATH) call and replace it to include
SCOPES, and update the conditional that references creds.scopes and the any(...
endswith ...) logic to require exact URI membership (e.g., any(s in SCOPES for s
in creds.scopes) or set intersection) so only valid Calendar API scope URIs are
accepted.
In `@src/core/tui-launcher.ts`:
- Around line 215-218: The problem is that pythonCmd is embedded un-quoted into
an AppleScript "do script" string so paths with spaces split; before creating
escapedCmd and calling spawn, shell-quote the python executable path (e.g.,
produce a quotedPythonCmd that wraps pythonCmd in single quotes and escapes any
internal single quotes using the standard '\'' substitution), then perform the
existing backslash/double-quote escapes on that quoted value and use it in the
spawn call (update escapedCmd and the spawn argument that builds the AppleScript
string to reference the quoted-and-escaped python path instead of raw
pythonCmd).
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1fbbad6-4c56-4a5c-8508-edd1801f5f36
📒 Files selected for processing (10)
.github/ISSUE_TEMPLATE/security_vulnerability.mdCONTRIBUTING.mdREADME.mdSECURITY.mddocs/architecture.mddocs/cli.mdscripts/calendar-feed.pysrc/cli/commands/init.tsxsrc/core/tui-launcher.tstests/core/tui-launcher.test.ts
Comment on lines
+35
to
+36
| > **Diagram:** Feed platform showing 8 producers polling data sources on staggered intervals, writing to an atomic cache bus, consumed by 21 status line segments. | ||
| > **Source:** feed-platform.excalidraw (in docs/assets/) -- open in [Excalidraw](https://excalidraw.com) for an editable hand-drawn version. |
There was a problem hiding this comment.
Update feed-platform counts and source link format to match docs rules.
The diagram description uses counts that conflict with the documented architecture baseline, and the source reference should be a relative file link (for example, docs/assets/feed-platform.excalidraw) rather than plain text.
As per coding guidelines, "Architecture descriptions match current code (v1.2, feed platform, 19 segments, 5 producers)" and "Links to source files must use relative paths and point to files that exist."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture.md` around lines 35 - 36, Update the diagram description
text to match the architecture baseline by changing the producer and segment
counts used in the line describing the feed platform (replace "8 producers" and
"21 status line segments" with "5 producers" and "19 segments") and change the
source reference to a relative file link pointing to the actual asset (use
"docs/assets/feed-platform.excalidraw" as the source link); locate and edit the
feed-platform diagram description string and the source reference text
(feed-platform.excalidraw) in the document to apply these exact replacements.
…quoting - calendar-feed.py: pass SCOPES to from_authorized_user_file(), use exact URI set intersection instead of suffix matching for scope compatibility - tui-launcher.ts: use single-quote shell-quoting for AppleScript path embedding to properly handle paths with spaces or special characters Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/tui-launcher.ts`:
- Around line 216-219: The embedded AppleScript string currently injects
shellQuoted (built from pythonCmd) directly into a double-quoted AppleScript
literal, which breaks if pythonCmd contains backslashes or double quotes; update
the code that constructs the AppleScript argument passed to spawn (the array
with "-e", `tell application "Terminal" to do script "${...} -m hookwise_tui"`)
to first escape backslashes and double quotes for AppleScript: take shellQuoted,
replace backslashes with double-backslashes (do this replacement first), then
replace double-quote characters with backslash-quote, and use that resulting
applescriptEscaped value in the template string; reference the variables
shellQuoted and pythonCmd and the spawn call so you can locate and replace the
interpolation safely.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be32fd0f-8f00-4869-85f0-f85fd1b7d8e7
📒 Files selected for processing (2)
scripts/calendar-feed.pysrc/core/tui-launcher.ts
- Update .coderabbit.yaml baseline to match actual code: 8 producers, 21 segments (was stale at 5/19) - Fix source reference in docs/architecture.md to use relative file link - Add AppleScript-layer escaping in tui-launcher.ts to prevent malformed osascript when pythonCmd contains backslashes or double quotes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Accept Go-based content from main for CONTRIBUTING.md, SECURITY.md, and docs/cli.md (replacing stale TypeScript references). Accept deletion of src/cli/commands/init.tsx, src/core/tui-launcher.ts, and tests/core/tui-launcher.test.ts (removed by PR #65). Co-Authored-By: Claude Opus 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
1 participant
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
Addresses all 11 CodeRabbit review comments from PR #11 (docs audit fix).
</content></invoke>generation artifactstui.auto_launch: trueto minimal preset YAML (was bypassingapplyPreset())integration/is mock-based, not end-to-endshell: trueusage1400+) to prevent stalenessTest plan
npm run docs:buildpasses clean🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes
Chores