mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
## Description The Headroom dashboard tunnels `GET /favicon.ico` requests to the wrapped upstream provider instead of serving its own. No route matched `/favicon.ico` in `headroom/proxy/server.py`, so the request fell through to the catch-all passthrough route (`headroom/providers/proxy_routes.py:994-1026`) registered by `register_provider_routes(app, proxy)`, and got forwarded to whichever LLM backend the proxy is wrapping — burning a real upstream request (and possibly failing auth) for a browser's automatic favicon fetch while viewing `/dashboard`. Closes #1787 ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `headroom/proxy/server.py`: added a `GET /favicon.ico` route returning `Response(status_code=204)`, registered next to the existing `/dashboard` route — i.e. before `register_provider_routes(app, proxy)` (line ~4184) registers the passthrough catch-all, so it takes priority. - `tests/test_proxy_handler_helpers.py`: `_PassthroughRequest.url.path` was hardcoded to `/favicon.ico` as a generic "goes to passthrough" example, which encoded the bug as expected behavior. Changed to `/some/other/path` so the passthrough-helper test no longer depends on favicon requests going upstream. - `tests/test_proxy_favicon_route.py` (new): regression test spinning up the real FastAPI app via `create_app`/`TestClient`, asserting `GET /favicon.ico` returns 204 and `proxy.handle_passthrough` is never called. - `CHANGELOG.md`: added an entry under `## Unreleased` / `### Fixed`. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ python -m pytest tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py -q 28 passed $ python -m pytest tests/test_proxy_healthchecks.py tests/test_proxy_passthrough_integration.py tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py -q 41 passed, 19 skipped $ python -m ruff check headroom/proxy/server.py tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py All checks passed! $ python -m ruff format --check headroom/proxy/server.py tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py 3 files already formatted $ python -m mypy headroom/proxy/server.py (no errors) ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.13, local checkout, `python -m pytest`/`ruff`/`mypy` run directly (no `uv` available in this shell). - Exact command / steps: `python -m pytest tests/test_proxy_favicon_route.py -v` — this test builds the real proxy app with `create_app(ProxyConfig(...))`, wraps `client.app.state.proxy.handle_passthrough` with a mock, then issues `client.get("/favicon.ico")` via a real `TestClient` request through the full FastAPI routing stack (not a unit-level call of the handler function directly). - Observed result: response status is `204`, and `handle_passthrough` (the function that forwards to the upstream provider) is asserted `not_called()` — confirming the request is now intercepted before reaching the catch-all passthrough route, and does not tunnel to the wrapped provider. - Not tested: did not manually run `headroom wrap <provider>` end-to-end and open a real browser tab to `/dashboard` to visually confirm the favicon icon in the tab (the fix returns 204/no-icon rather than a real bundled `.ico` — browsers handle this fine, but the visual "no more broken/upstream favicon request" experience wasn't screenshotted). The FastAPI-level test above exercises the actual routing/dispatch path this bug lived in. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation (N/A — no user-facing docs describe dashboard route internals beyond CHANGELOG) - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated CHANGELOG.md where applicable ## Screenshots (if applicable) N/A — server-side route change, no UI change. ## Additional Notes Deliberately kept the fix minimal: no `StaticFiles` mount or general static-asset serving system was added, since a single favicon route doesn't warrant that abstraction. No real `.ico` binary asset was bundled either — a `204 No Content` response is sufficient for browsers and avoids maintaining a binary asset in the repo; this can be upgraded to serve a real branded icon later if desired. Co-authored-by: JD Davis <mxjerrett@gmail.com>
This commit is contained in:
parent
1573f1fd07
commit
3076e32172
4 changed files with 42 additions and 75 deletions
76
CHANGELOG.md
76
CHANGELOG.md
|
|
@ -8,79 +8,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
|
||||
## Unreleased
|
||||
|
||||
### Fixed
|
||||
- `headroom wrap cursor` no longer injects the `rtk` custom-instructions
|
||||
block into `.cursorrules` when rtk's own native Cursor hook registers
|
||||
successfully. rtk supports a real hook for Cursor via
|
||||
`rtk init --agent cursor` (the same mechanism headroom already uses for
|
||||
Claude Code), which rewrites shell commands transparently — the injected
|
||||
`.cursorrules` text duplicated that guidance for no benefit. `wrap cursor`
|
||||
now tries the native hook first and only falls back to injecting
|
||||
`.cursorrules` if hook registration fails (#756).
|
||||
- `headroom wrap claude` no longer leaves a dead `ANTHROPIC_BASE_URL` in a
|
||||
project's `.claude/settings.local.json` after an unclean exit (`SIGKILL`,
|
||||
OOM, reboot, or terminal/tmux close via `SIGHUP`, which was not caught).
|
||||
`_write_claude_wrap_base_url`/`_restore_claude_wrap_base_url` only removed
|
||||
or restored the entry from the wrap process's own `finally` block, so a
|
||||
crash skipped it and every later bare `claude` invocation in that project
|
||||
inherited the stale proxy URL and hung indefinitely retrying a dead port.
|
||||
A wrap session now stamps a sidecar marker (pid, port, prior value); the
|
||||
next `wrap`, `unwrap`, or `headroom doctor` run detects a marker whose pid
|
||||
is dead or reused and restores the recorded prior value automatically.
|
||||
`claude()` also now catches `SIGHUP` alongside the existing `SIGTERM`
|
||||
handler ([#1768](https://github.com/headroomlabs-ai/headroom/issues/1768)).
|
||||
- Non-finite values (`NaN`, `Infinity`) in `proxy_savings.json` or in upstream
|
||||
cost/token metadata no longer crash the proxy or corrupt the savings
|
||||
dashboard. `SavingsTracker`'s numeric coercion caught only `TypeError` and
|
||||
`ValueError`, so `int(float('inf'))` raised an uncaught `OverflowError` while
|
||||
loading persisted state (`SavingsTracker.__init__` failed and the proxy would
|
||||
not start), and `float('nan')`/`float('inf')` passed straight through, then
|
||||
serialized to `NaN`/`Infinity` literals that the dashboard's `JSON.parse`
|
||||
rejects. `json.loads` accepts those literals, so one bad write poisoned every
|
||||
later start. Both coercion helpers now also catch `OverflowError` and reject
|
||||
non-finite floats, failing open to safe defaults.
|
||||
- `headroom learn` now honors `CLAUDE_CONFIG_DIR`. It resolved the Claude
|
||||
config directory as `~/.claude` and wrote global memory to
|
||||
`~/.claude/CLAUDE.md`, so users who relocate their Claude config via that
|
||||
env var had `learn` scan the wrong directory and detect no projects. The
|
||||
scanner and memory writer now read/write the configured directory
|
||||
([#1630](https://github.com/headroomlabs-ai/headroom/issues/1630)).
|
||||
- `--backend bedrock` now fails fast with an actionable error when temporary
|
||||
AWS credentials (`AWS_SESSION_TOKEN`) are used but botocore is not installed
|
||||
(e.g. the slim default Docker image). litellm's session-token auth path
|
||||
imports botocore, so the missing dependency previously surfaced only at
|
||||
request time as a misleading `authentication_error: No module named
|
||||
'botocore'`. The proxy now tells the user to install the `bedrock` extra up
|
||||
front ([#1551](https://github.com/headroomlabs-ai/headroom/issues/1551)).
|
||||
- Content detection no longer crashes the proxy on text containing an
|
||||
orphaned `+++ ` target line with no preceding `--- ` source line (common in
|
||||
`set -x` xtrace output and partial diffs). The bundled `unidiff` 0.4.0 parser
|
||||
panics on that input instead of returning an error; the Rust diff detector now
|
||||
contains the panic and treats the fragment as plain text, so the request is
|
||||
compressed and forwarded normally instead of returning HTTP 500
|
||||
([#1547](https://github.com/headroomlabs-ai/headroom/issues/1547)).
|
||||
- Proactive expansion blocks injected into user turns are now wrapped in
|
||||
`<headroom_proactive_expansion>` XML tags, giving downstream consumers
|
||||
(LLMs, loggers, attribution parsers) a machine-readable provenance
|
||||
boundary and preventing misattribution in multi-agent threads.
|
||||
- **cli:** the startup banner no longer advertises
|
||||
`HEADROOM_COMPRESSION_STABLE_AFTER_TURN` and
|
||||
`HEADROOM_STALE_READ_COMPRESS_AFTER_TURNS` as tuning knobs. Both were read
|
||||
only to render the `Performance Tuning` banner section and were never wired
|
||||
into the compression path, so setting them changed the banner but had no
|
||||
effect on behavior. The banner now surfaces only the embedding sidecar,
|
||||
which is a real, consumed setting.
|
||||
- **memory/embedder:** cap CPU thread oversubscription in the local
|
||||
torch/sentence-transformers embedder. Concurrent encodes previously each
|
||||
fanned out to ~`os.cpu_count()` BLAS/OpenMP threads, so under load the memory
|
||||
path starved the asyncio event loop and spiked `/livez` latency to several
|
||||
seconds. CPU encodes now run on a dedicated, size-limited executor whose
|
||||
workers each pin their thread pool, bounding total embedding threads to
|
||||
`HEADROOM_EMBED_CONCURRENCY` × `HEADROOM_EMBED_NUM_THREADS` (defaults
|
||||
`min(4, cpu)` × 1). The ONNX embedder already capped its threads; this brings
|
||||
the torch path to parity
|
||||
([#198](https://github.com/headroomlabs-ai/headroom/issues/198)).
|
||||
|
||||
### Changed
|
||||
|
||||
* **telemetry:** anonymous usage telemetry is now **opt-in** (off by default) instead of opt-out. Nothing is collected or sent unless you set `HEADROOM_TELEMETRY=on` or pass `--telemetry` to `headroom proxy` / `headroom install apply`. `is_telemetry_enabled()` is fail-closed — only explicit on-values (`on`/`true`/`1`/`yes`/`enable`/`enabled`) enable it; unset, empty, or unrecognized values stay disabled. The existing `--no-telemetry` flag and `HEADROOM_TELEMETRY=off` remain accepted for back-compat, and install manifests now write the `HEADROOM_TELEMETRY` value explicitly so generated deployments are unambiguous.
|
||||
|
|
@ -169,7 +96,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
* **cli:** the startup banner no longer advertises `HEADROOM_COMPRESSION_STABLE_AFTER_TURN` and `HEADROOM_STALE_READ_COMPRESS_AFTER_TURNS` as tuning knobs. Both were read only to render the `Performance Tuning` banner section and were never wired into the compression path, so setting them changed the banner but had no effect on behavior. The banner now surfaces only the embedding sidecar, which is a real, consumed setting.
|
||||
* **memory/embedder:** cap CPU thread oversubscription in the local torch/sentence-transformers embedder. Concurrent encodes previously each fanned out to ~`os.cpu_count()` BLAS/OpenMP threads, so under load the memory path starved the asyncio event loop and spiked `/livez` latency to several seconds. CPU encodes now run on a dedicated, size-limited executor whose workers each pin their thread pool, bounding total embedding threads to `HEADROOM_EMBED_CONCURRENCY` × `HEADROOM_EMBED_NUM_THREADS` (defaults `min(4, cpu)` × 1). The ONNX embedder already capped its threads; this brings the torch path to parity ([#198](https://github.com/headroomlabs-ai/headroom/issues/198)).
|
||||
* **proxy:** Buffered passthrough routes (e.g. `GET /v1/models`) no longer return an opaque HTTP 502 when an OpenAI-compatible upstream closes a pooled keep-alive connection mid-response (`httpx.RemoteProtocolError` / "incomplete chunked read"). Headroom now retries the request once on a fresh connection — mirroring a direct `curl` — and only returns a clear `upstream_protocol_error` 502 if the upstream is genuinely sending an incomplete response ([#1112](https://github.com/chopratejas/headroom/issues/1112)).
|
||||
|
||||
* **cursor:** `headroom wrap cursor` no longer injects the `rtk` custom-instructions block into `.cursorrules` when rtk's own native Cursor hook registers successfully. rtk supports a real hook for Cursor via `rtk init --agent cursor` (the same mechanism headroom already uses for Claude Code), which rewrites shell commands transparently — the injected `.cursorrules` text duplicated that guidance for no benefit. `wrap cursor` now tries the native hook first and only falls back to injecting `.cursorrules` if hook registration fails (#756).
|
||||
* **proxy:** The Headroom dashboard no longer tunnels `GET /favicon.ico` to the wrapped upstream provider. No route matched that path, so it fell through to the proxy's catch-all passthrough route and was forwarded to the configured Anthropic/OpenAI/etc. backend — burning a real upstream request (and possibly failing auth) for a browser's automatic favicon fetch on `/dashboard`. A dedicated `/favicon.ico` route now answers with `204 No Content` directly, registered ahead of the passthrough catch-all (#1787).
|
||||
|
||||
## [0.29.0](https://github.com/headroomlabs-ai/headroom/compare/v0.28.0...v0.29.0) (2026-07-03)
|
||||
|
||||
|
|
|
|||
|
|
@ -2910,6 +2910,14 @@ def create_app(config: ProxyConfig | None = None) -> FastAPI:
|
|||
"""Serve the Headroom dashboard UI."""
|
||||
return get_dashboard_html()
|
||||
|
||||
@app.get("/favicon.ico")
|
||||
async def favicon() -> Response:
|
||||
# Registered before register_provider_routes' catch-all passthrough
|
||||
# route so browsers' automatic favicon requests for /dashboard are
|
||||
# answered locally instead of being tunneled to the wrapped upstream
|
||||
# provider (GH #1787).
|
||||
return Response(status_code=204)
|
||||
|
||||
DASHBOARD_STATS_CACHE_TTL_SECONDS = 5.0
|
||||
_stats_snapshot_lock = asyncio.Lock()
|
||||
_stats_snapshot: dict[str, Any] = {"expires_at": 0.0, "value": None}
|
||||
|
|
|
|||
31
tests/test_proxy_favicon_route.py
Normal file
31
tests/test_proxy_favicon_route.py
Normal file
|
|
@ -0,0 +1,31 @@
|
|||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
pytest.importorskip("fastapi")
|
||||
pytest.importorskip("httpx")
|
||||
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
from headroom.proxy.server import ProxyConfig, create_app
|
||||
|
||||
|
||||
def test_favicon_is_served_locally_and_never_reaches_passthrough(monkeypatch) -> None:
|
||||
"""GH #1787: /favicon.ico must not be tunneled to the upstream provider."""
|
||||
monkeypatch.setenv("HEADROOM_SKIP_UPSTREAM_CHECK", "1")
|
||||
config = ProxyConfig(
|
||||
optimize=False,
|
||||
cache_enabled=False,
|
||||
rate_limit_enabled=False,
|
||||
cost_tracking_enabled=False,
|
||||
)
|
||||
app = create_app(config)
|
||||
|
||||
with TestClient(app) as client:
|
||||
with patch.object(
|
||||
client.app.state.proxy, "handle_passthrough", new=AsyncMock()
|
||||
) as passthrough:
|
||||
response = client.get("/favicon.ico")
|
||||
|
||||
assert response.status_code == 204
|
||||
passthrough.assert_not_called()
|
||||
|
|
@ -80,7 +80,7 @@ class _ChatGPTAccountRequest:
|
|||
class _PassthroughRequest:
|
||||
method = "GET"
|
||||
headers = {}
|
||||
url = SimpleNamespace(path="/favicon.ico", query="")
|
||||
url = SimpleNamespace(path="/some/other/path", query="")
|
||||
|
||||
async def body(self) -> bytes:
|
||||
return b""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue