chore: gitignore Spec Kit/GSD local tooling (clean untracked clutter) - #172
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a ChangesLocal Planning Tooling Configuration
Network Sharing & Tailscale Implementation Plan
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~8 minutes Possibly related PRs
🚥 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 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 |
|
| Filename | Overview |
|---|---|
| .gitignore | Adds two targeted ignore patterns for local Spec Kit/GSD tooling; gitignore rule ordering is correct and the existing .claude/skills/omnivoice/ un-ignore is preserved. |
| docs/superpowers/plans/2026-05-30-network-sharing.md | 943-line network sharing implementation plan; contains a hardcoded local developer path in a test run command, and the PIN cookie in the middleware template is missing httponly=True. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[".claude/skills/speckit-foo/"] --> B{".claude/*\nmatches?"}
B -->|yes| C[ignored]
C --> D{"!.claude/skills/\nmatches parent?"}
D -->|yes| E[un-ignored]
E --> F{"!.claude/skills/**\nmatches?"}
F -->|yes| G[un-ignored]
G --> H{".claude/skills/speckit-*/\nmatches?"}
H -->|yes| I["ignored (speckit-* stays local)"]
A2[".claude/skills/omnivoice/"] --> B2{".claude/*\nmatches?"}
B2 -->|yes| C2[ignored]
C2 --> D2{"!.claude/skills/\nmatches parent?"}
D2 -->|yes| E2[un-ignored]
E2 --> F2{"!.claude/skills/**\nmatches?"}
F2 -->|yes| G2[un-ignored]
G2 --> H2{".claude/skills/speckit-*/\nmatches?"}
H2 -->|no| I2["tracked (MCP skill preserved)"]
Reviews (1): Last reviewed commit: "chore: gitignore Spec Kit/GSD local tool..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| - [ ] **Step 2: Run it — expect failure** | ||
|
|
||
| Run: `cd /Users/user4/Desktop/github/OmniVoice && python -m pytest tests/test_network_share.py -q` |
There was a problem hiding this comment.
The run command embeds a full local filesystem path (/Users/user4/Desktop/github/OmniVoice), committing a developer's username and machine layout into the repo history. Any contributor following this plan will also get a confusing path that won't exist on their machine.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if request.cookies.get("ov_pin") != pin: | ||
| response.set_cookie("ov_pin", pin, samesite="lax") |
There was a problem hiding this comment.
httponly flag in plan template
The template sets response.set_cookie("ov_pin", pin, samesite="lax") without httponly=True, which leaves the cookie readable by any JavaScript running on the page. If the page ever serves content from a path that allows script injection, the PIN can be exfiltrated. The session-storage path already exposes the PIN to JS intentionally, but the cookie should add httponly=True to avoid creating a second JS-accessible store unnecessarily — or the cookie approach should be dropped in favour of sessionStorage alone.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/superpowers/plans/2026-05-30-network-sharing.md`:
- Line 81: Replace the hardcoded absolute path in the command `cd
/Users/user4/Desktop/github/OmniVoice && python -m pytest
tests/test_network_share.py -q` with a repository-relative invocation (or remove
the `cd` entirely); update the line to either `python -m pytest
tests/test_network_share.py -q` or `cd <repo-root-relative-path> && python -m
pytest tests/test_network_share.py -q` (preferably the first) so the command
runs from any environment without relying on `/Users/user4/...`.
- Around line 409-412: The middleware that reads PIN from request.headers.get,
request.query_params.get, or request.cookies.get("ov_pin") exposes two risks:
query params leak in logs/referrers and cookies persist indefinitely; update the
handler that reads/sets the PIN so (1) when setting the ov_pin cookie include an
explicit expiry/max_age and secure attributes (e.g., HttpOnly, Secure, SameSite)
and (2) add server-side validation of cookie age or issue timestamps with the
cookie and reject stale PINs in the same middleware/function that calls
request.cookies.get("ov_pin"); also ensure the sharing UI/docs clearly warn that
?pin= is exposed in logs/referrers.
- Around line 406-407: The middleware currently exempts requests using a broad
check path.startswith("/favicon") which will match unintended routes; update the
condition in the handler that returns await call_next(request) to remove the
broad startswith check and either rely on "/favicon.ico" being present in
_SHELL_PATHS or replace it with a more specific test such as matching exact
"/favicon.ico" or a stricter pattern (e.g., path.startswith("/favicon.") for
extensions). Locate the conditional that references _SHELL_PATHS and
path.startswith in the same block (the function that calls call_next(request))
and modify that expression accordingly so only intended favicon paths are
exempted.
- Around line 138-146: _find_free_port closes the test socket after finding a
port which creates a TOCTOU race where uvicorn may fail to bind; update the
enable() function (the code that starts uvicorn) to catch binding errors (e.g.,
OSError/EADDRINUSE) and retry: on failure call _find_free_port again and attempt
to re-bind/start uvicorn a bounded number of times (with a short backoff) before
giving up, logging each retry and the final error; alternatively, add a clear
doc comment in enable() noting the race and recommending retries if you prefer
not to implement automatic retries.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b4dfb7e-7b1c-44d2-9aeb-cf430b1f4e88
📒 Files selected for processing (2)
.gitignoredocs/superpowers/plans/2026-05-30-network-sharing.md
|
|
||
| - [ ] **Step 2: Run it — expect failure** | ||
|
|
||
| Run: `cd /Users/user4/Desktop/github/OmniVoice && python -m pytest tests/test_network_share.py -q` |
There was a problem hiding this comment.
Remove hardcoded absolute path.
The test run command contains a hardcoded absolute path /Users/user4/Desktop/github/OmniVoice that is environment-specific and will fail on other machines. Use a relative path or remove the cd command entirely since pytest should be run from the repository root.
🔧 Proposed fix
-Run: `cd /Users/user4/Desktop/github/OmniVoice && python -m pytest tests/test_network_share.py -q`
+Run: `python -m pytest tests/test_network_share.py -q`📝 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.
| Run: `cd /Users/user4/Desktop/github/OmniVoice && python -m pytest tests/test_network_share.py -q` | |
| Run: `python -m pytest tests/test_network_share.py -q` |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~81-~81: The official name of this software platform is spelled with a capital “H”.
Context: ...Step 2: Run it — expect failure** Run: `cd /Users/user4/Desktop/github/OmniVoice && python -m pytest tests/tes...
(GITHUB)
🤖 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 `@docs/superpowers/plans/2026-05-30-network-sharing.md` at line 81, Replace the
hardcoded absolute path in the command `cd /Users/user4/Desktop/github/OmniVoice
&& python -m pytest tests/test_network_share.py -q` with a repository-relative
invocation (or remove the `cd` entirely); update the line to either `python -m
pytest tests/test_network_share.py -q` or `cd <repo-root-relative-path> &&
python -m pytest tests/test_network_share.py -q` (preferably the first) so the
command runs from any environment without relying on `/Users/user4/...`.
| def _find_free_port(base: int, tries: int = 20) -> int: | ||
| for p in range(base, base + tries): | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
| try: | ||
| s.bind(("0.0.0.0", p)) | ||
| return p | ||
| except OSError: | ||
| continue | ||
| raise RuntimeError("no free share port available") |
There was a problem hiding this comment.
Race condition in port availability check.
The _find_free_port function has a time-of-check to time-of-use (TOCTOU) race condition. The socket is closed immediately after successfully binding, which means another process could bind to that port before uvicorn starts. While this is a common pattern and the race window is small, uvicorn's bind could still fail.
Consider handling bind failures in the enable() function with retry logic, or document this known limitation.
🛡️ Proposed fix: Add error handling in enable()
async def enable(app) -> ShareState:
global _server, _task, _state
if _state.enabled:
return _state
port = _find_free_port(BACKEND_PORT + 1)
pin = _gen_pin()
config = uvicorn.Config(app, host="0.0.0.0", port=port, log_level="warning")
server = uvicorn.Server(config)
server.install_signal_handlers = lambda: None
_task = asyncio.create_task(server.serve())
for _ in range(100):
if getattr(server, "started", False):
break
await asyncio.sleep(0.05)
+ if not getattr(server, "started", False):
+ # Clean up failed start
+ server.should_exit = True
+ raise RuntimeError(f"Share server failed to bind to port {port} within timeout")🤖 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 `@docs/superpowers/plans/2026-05-30-network-sharing.md` around lines 138 - 146,
_find_free_port closes the test socket after finding a port which creates a
TOCTOU race where uvicorn may fail to bind; update the enable() function (the
code that starts uvicorn) to catch binding errors (e.g., OSError/EADDRINUSE) and
retry: on failure call _find_free_port again and attempt to re-bind/start
uvicorn a bounded number of times (with a short backoff) before giving up,
logging each retry and the final error; alternatively, add a clear doc comment
in enable() noting the race and recommending retries if you prefer not to
implement automatic retries.
| if path in _SHELL_PATHS or path.startswith("/assets/") or path.startswith("/favicon"): | ||
| return await call_next(request) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Overly broad path exemption for favicon.
The middleware exempts both path.startswith("/favicon") and path in _SHELL_PATHS (which includes "/favicon.ico"). The startswith("/favicon") check will match any path beginning with /favicon, such as /favicon_admin or /favicondata.
Since /favicon.ico is already in _SHELL_PATHS, the startswith check should be removed or made more specific (e.g., path.startswith("/favicon.")).
♻️ Proposed fix
path = request.url.path
- if path in _SHELL_PATHS or path.startswith("/assets/") or path.startswith("/favicon"):
+ if path in _SHELL_PATHS or path.startswith("/assets/"):
return await call_next(request)Or if other favicon formats are needed:
- if path in _SHELL_PATHS or path.startswith("/assets/") or path.startswith("/favicon"):
+ if path in _SHELL_PATHS or path.startswith("/assets/") or path.startswith("/favicon."):🤖 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 `@docs/superpowers/plans/2026-05-30-network-sharing.md` around lines 406 - 407,
The middleware currently exempts requests using a broad check
path.startswith("/favicon") which will match unintended routes; update the
condition in the handler that returns await call_next(request) to remove the
broad startswith check and either rely on "/favicon.ico" being present in
_SHELL_PATHS or replace it with a more specific test such as matching exact
"/favicon.ico" or a stricter pattern (e.g., path.startswith("/favicon.") for
extensions). Locate the conditional that references _SHELL_PATHS and
path.startswith in the same block (the function that calls call_next(request))
and modify that expression accordingly so only intended favicon paths are
exempted.
| request.headers.get("x-omnivoice-pin") | ||
| or request.query_params.get("pin") | ||
| or request.cookies.get("ov_pin") | ||
| or "" |
There was a problem hiding this comment.
Consider security implications of PIN in query parameter and cookie.
The middleware accepts PIN from three sources: header, query parameter, and cookie. Two considerations:
-
Query parameter exposure: The
?pin=parameter will appear in server logs, browser history, and referrer headers. This is documented as intentional for QR code flows, but ensure this risk is communicated to users in the sharing UI and documentation. -
Cookie persistence: The
ov_pincookie persists the PIN across browser sessions (no expiry set on line 418). This means a PIN remains valid even after sharing is disabled, until the cookie expires or is cleared. Consider addingmax_ageor validating cookie age server-side.
🔒 Proposed fix: Add cookie expiry
if not secrets.compare_digest(supplied, pin):
return JSONResponse({"detail": "PIN required"}, status_code=401)
response = await call_next(request)
if request.cookies.get("ov_pin") != pin:
- response.set_cookie("ov_pin", pin, samesite="lax")
+ response.set_cookie("ov_pin", pin, max_age=86400, samesite="lax", httponly=True, secure=False)
+ # max_age=86400 = 24 hours; httponly prevents JS access; secure=False because LAN is HTTP🤖 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 `@docs/superpowers/plans/2026-05-30-network-sharing.md` around lines 409 - 412,
The middleware that reads PIN from request.headers.get,
request.query_params.get, or request.cookies.get("ov_pin") exposes two risks:
query params leak in logs/referrers and cookies persist indefinitely; update the
handler that reads/sets the PIN so (1) when setting the ov_pin cookie include an
explicit expiry/max_age and secure attributes (e.g., HttpOnly, Secure, SameSite)
and (2) add server-side validation of cookie age or issue timestamps with the
cookie and reject stale PINs in the same middleware/function that calls
request.cookies.get("ov_pin"); also ensure the sharing UI/docs clearly warn that
?pin= is exposed in logs/referrers.
1 participant
.specify/and.claude/skills/speckit-*/are local planning tooling (Spec Kit / GSD), not OmniVoice code — they've shown as untracked all along. Ignore them sogit statusis clean (the tracked.claude/skills/omnivoice/MCP skill is unaffected — targeted glob). Also tracks the network-sharing implementation plan to pair with its committed design spec.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores