feat: enforce debug/host guard and document API versioning#60
Conversation
|
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:
📝 WalkthroughWalkthroughEnforces a startup guard refusing Flask --debug when --host is non-loopback, adds module version (0.1.0.dev0), creates CHANGELOG and deprecation-policy docs, annotates API reference fields as stable/experimental/deprecated, and adds CLI/tests for loopback validation and URL formatting. ChangesVersion, security guard, deprecation policy, and API stability
Sequence Diagram: startup validation flowsequenceDiagram
participant CLI
participant ArgParse
participant ValidateStartupCLI
participant IsLoopbackHost
participant FlaskApp
CLI->>ArgParse: parse args (--debug, --host)
ArgParse-->>ValidateStartupCLI: args
ValidateStartupCLI->>IsLoopbackHost: normalize host / check loopback
IsLoopbackHost-->>ValidateStartupCLI: True/False
alt non-loopback + debug
ValidateStartupCLI->>CLI: write error to stderr & SystemExit(1)
else loopback or no debug
ValidateStartupCLI-->>FlaskApp: proceed to run()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_cli_args.py (1)
337-342: ⚡ Quick winAssert the stderr contract on the rejection path.
This only checks the exit code, so the “clear stderr message” part of the guard can regress unnoticed. Capture stderr here and assert a stable substring from the error text.
Suggested test update
- def test_validate_startup_cli_rejects_non_loopback_debug(self) -> None: + def test_validate_startup_cli_rejects_non_loopback_debug( + self, capsys: pytest.CaptureFixture[str] + ) -> None: parser = build_cli_parser() args = parser.parse_args(["--host", "0.0.0.0", "--debug"]) with pytest.raises(SystemExit) as exc_info: validate_startup_cli(args) assert exc_info.value.code == 1 + assert "--debug is only allowed with a loopback --host" in capsys.readouterr().err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_args.py` around lines 337 - 342, Update the test to also capture stderr and assert a stable substring from the error output: add the pytest capsys fixture to test_validate_startup_cli_rejects_non_loopback_debug, call validate_startup_cli via the existing pytest.raises(SystemExit) context, then after the context use capsys.readouterr().err and assert it contains a stable substring indicating the rejection (e.g. keywords like "debug" and "loopback" or a short phrase your validate_startup_cli emits) to ensure the clear stderr message is verified.docs/api-reference.md (1)
247-261: ⚡ Quick winConsider adding Stability column to stats endpoint for consistency.
The stats endpoint table lacks a Stability column while GET /api/projects, GET /api/projects/.../sessions, GET /api/sessions/.../, and GET /api/export/state all include stability annotations. Adding a Stability column here would provide consistent documentation style and help future-proof this endpoint against unintentional breaking changes.
🤖 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/api-reference.md` around lines 247 - 261, Add a "Stability" column to the stats endpoint table in docs/api-reference.md: update the header row to include "Stability" and add stability annotations (e.g., "stable", "experimental", "deprecated") for each listed field such as files_touched, commands_run, urls_accessed, conversation_turns, wall_clock_seconds, wall_clock_display, cost_estimate_usd, tool_result_summary, stop_reason_summary, entry_type_counts, sidechain_message_count, api_error_count, and compaction_events so the table format matches the other endpoints (GET /api/projects, GET /api/projects/.../sessions, GET /api/sessions/.../, GET /api/export/state).
🤖 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 `@app.py`:
- Around line 18-29: The startup banner prints the host verbatim (so IPv6 like
::1 becomes an invalid URL such as http://::1:5000); when formatting the
displayed address (where the app constructs the startup URL), detect IPv6
literals (e.g. host contains ':' or use is_loopback_host/hostname variable) and
wrap the host in square brackets before appending the port (produce
http://[::1]:5000), ensuring you don't double-bracket already-bracketed hosts;
alternatively, if you prefer, avoid rendering a clickable URL when the host
contains a colon.
---
Nitpick comments:
In `@docs/api-reference.md`:
- Around line 247-261: Add a "Stability" column to the stats endpoint table in
docs/api-reference.md: update the header row to include "Stability" and add
stability annotations (e.g., "stable", "experimental", "deprecated") for each
listed field such as files_touched, commands_run, urls_accessed,
conversation_turns, wall_clock_seconds, wall_clock_display, cost_estimate_usd,
tool_result_summary, stop_reason_summary, entry_type_counts,
sidechain_message_count, api_error_count, and compaction_events so the table
format matches the other endpoints (GET /api/projects, GET
/api/projects/.../sessions, GET /api/sessions/.../, GET /api/export/state).
In `@tests/test_cli_args.py`:
- Around line 337-342: Update the test to also capture stderr and assert a
stable substring from the error output: add the pytest capsys fixture to
test_validate_startup_cli_rejects_non_loopback_debug, call validate_startup_cli
via the existing pytest.raises(SystemExit) context, then after the context use
capsys.readouterr().err and assert it contains a stable substring indicating the
rejection (e.g. keywords like "debug" and "loopback" or a short phrase your
validate_startup_cli emits) to ensure the clear stderr message is verified.
🪄 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: 86ed3edd-167a-4f72-8895-7e62b9cd49f2
📒 Files selected for processing (7)
CHANGELOG.mdCONTRIBUTING.mdREADME.mdapp.pydocs/api-reference.mddocs/deprecation-policy.mdtests/test_cli_args.py
|
@CodeRabbit, please review this PR fully |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== app.py ==="
cat app.py
echo ""
echo "=== tests/test_cli_args.py ==="
cat tests/test_cli_args.py
echo ""
echo "=== CHANGELOG.md ==="
cat CHANGELOG.md
echo ""
echo "=== CONTRIBUTING.md ==="
cat CONTRIBUTING.md
echo ""
echo "=== README.md ==="
cat README.md
echo ""
echo "=== docs/api-reference.md ==="
cat docs/api-reference.md
echo ""
echo "=== docs/deprecation-policy.md ==="
cat docs/deprecation-policy.mdLength of output: 51063
SummaryThe PR lands three well-scoped changes — a debug/host startup guard, versioning + CHANGELOG + deprecation policy, and API stability documentation. The overall shape is solid. A handful of issues worth addressing are listed below.
|
| Field | Type | Stability | Description |
|---|---|---|---|
project |
string | stable | … |
session_id |
string | stable | … |
title |
string | stable | … |
role |
string | stable | … |
timestamp |
string | null | stable | … |
snippet |
string | experimental | ~160 chars around match; length may change |
CHANGELOG.md
6. Missing footer reference link (Keep a Changelog convention)
Keep a Changelog expects each version heading [0.1.0] to be a link resolved at the file footer:
[0.1.0]: https://github.com/cppalliance/claude-code-chat-browser/releases/tag/v0.1.0or a GitHub comparison link. Without it, [0.1.0] renders as plain text, not a hyperlink.
docs/deprecation-policy.md
7. "At least one release" notice window is very short
This is listed as a principle choice (pre-1.0 project), so it is intentional, but consider adding a note that the window may extend for fields used by the bundled SPA (which tracks no external versioning). For example:
For fields actively read by the bundled SPA, the deprecation period will span at least two releases to allow the SPA and policy to be updated atomically.
Not a blocker — just a suggestion for integrators reading the policy.
Minor Nits
README.mdOptionscode block (python app.py --port 8080 --host 0.0.0.0) appears before the security warning box; a short inline comment# never add --debug herewould be a helpful signal even before the reader reaches the warning.CONTRIBUTING.mdis clean and well updated.
Verdict
The security guard logic and tests are correct. The three areas most worth fixing before merge:
- The redundant
len(parts) == 4check (low effort, avoids confusion for future readers). - The missing
Stabilitycolumn on the search endpoint table (consistency with the rest of the doc). - The CHANGELOG footer reference link (convention compliance).
- Simplify 127.x.x.x loopback check; reject empty host in format_listen_url - Add malformed 127.* tests; search endpoint stability column - CHANGELOG [0.1.0] footer link; SPA two-release deprecation note - README inline warning on 0.0.0.0 example
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
7-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd footer comparison link for
[Unreleased]per Keep a Changelog convention.The changelog follows the Keep a Changelog format but is missing the footer reference link. Add a comparison link pointing to the unreleased commits (typically comparing the latest tag or initial commit to HEAD).
📎 Suggested footer addition
Add this section at the end of the file (adjust the repository URL and comparison base as appropriate):
- `export_count` on `GET /api/export/state` (documented only; still returned). Use `last_export_session_count`. Removal planned in a follow-up release per [deprecation policy](docs/deprecation-policy.md). + +[Unreleased]: https://github.com/owner/repo/compare/v0.0.0...HEADIf no prior release exists, compare against the initial commit or use
/commits/masterinstead of a comparison URL.🤖 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 `@CHANGELOG.md` around lines 7 - 22, Add the missing Keep a Changelog footer link for the [Unreleased] section by appending a comparison reference at the end of CHANGELOG.md that points from the previous release tag (or the initial commit/master if no prior tag) to HEAD; ensure the link label matches "[Unreleased]" and update the comparison URL to your repository and chosen base (e.g., v0.1.0..HEAD or initial-commit..HEAD) so the Unreleased header resolves to that diff URL.
🤖 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.
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 7-22: Add the missing Keep a Changelog footer link for the
[Unreleased] section by appending a comparison reference at the end of
CHANGELOG.md that points from the previous release tag (or the initial
commit/master if no prior tag) to HEAD; ensure the link label matches
"[Unreleased]" and update the comparison URL to your repository and chosen base
(e.g., v0.1.0..HEAD or initial-commit..HEAD) so the Unreleased header resolves
to that diff URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 608705d8-fae1-4f60-b0c5-7ab36a582d08
📒 Files selected for processing (6)
CHANGELOG.mdCONTRIBUTING.mdapp.pydocs/api-reference.mddocs/deprecation-policy.mdtests/test_cli_args.py
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/deprecation-policy.md
- app.py
- tests/test_cli_args.py
- docs/api-reference.md
|
@CodeRabbit, Can you check @timon0305 's comments are addressed correctly? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR `#60` Review Comments by timon0305 ==="
gh api repos/cppalliance/claude-code-chat-browser/pulls/60/comments --jq '.[] | select(.user.login == "timon0305") | {id: .id, path: .path, line: .line, body: .body, position: .position}'
echo ""
echo "=== PR `#60` Issue Comments by timon0305 ==="
gh api repos/cppalliance/claude-code-chat-browser/issues/60/comments --jq '.[] | select(.user.login == "timon0305") | {id: .id, body: .body}'
echo ""
echo "=== All PR reviews by timon0305 ==="
gh api repos/cppalliance/claude-code-chat-browser/pulls/60/reviews --jq '.[] | select(.user.login == "timon0305") | {id: .id, state: .state, body: .body}'Length of output: 3347 🏁 Script executed: #!/bin/bash
echo "=== app.py (is_loopback_host and related functions) ==="
cat app.py
echo ""
echo "=== CHANGELOG.md ==="
cat CHANGELOG.md
echo ""
echo "=== docs/deprecation-policy.md ==="
cat docs/deprecation-policy.md
echo ""
echo "=== docs/api-reference.md ==="
cat docs/api-reference.mdLength of output: 23874
Comment 1 —
|
3 participants
Closes #58
Summary
Single PR for Monday API policy and security work on
claude-code-chat-browser:--debugwith non-loopback--host(e.g.0.0.0.0). Loopback + debug and0.0.0.0without debug remain allowed.__version__(0.1.0),CHANGELOG.md, anddocs/deprecation-policy.mdlinked from README and CONTRIBUTING.stable/experimental/deprecated) documented indocs/api-reference.mdfor export state, project list, session list, and session detail.export_countis deprecated in docs only — still returned in JSON (removal is Thursday).Sprint items
--debugwith non-loopback host (3 pt)Changes
app.py(is_loopback_host,validate_startup_cli),tests/test_cli_args.pyCHANGELOG.md,docs/deprecation-policy.md,app.py(__version__)docs/api-reference.md,README.md,CONTRIBUTING.mdTest plan
python -m pytest -q— full suite greenpython -m mypypython -m pytest tests/test_cli_args.py::TestAppArgparse -v --no-cov— 26 passedpython app.py --host 0.0.0.0 --debug→ stderr error, exit 1python app.py --host 127.0.0.1 --debug→ starts, debug onpython app.py --host 0.0.0.0(no--debug) → starts, debug offnpm test(no JS changes; optional)Out of scope
export_countfrom responses (Thursday #4)sessions.jsdispatch registry (Tuesday #1)Summary by CodeRabbit
New Features
Changed
Documentation
Deprecations
export_countin GET /api/export/state deprecated; uselast_export_session_count.Tests